| Message ID | 20210718211415.143709-1-marex@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | backlight: pwm_bl: Avoid backlight flicker if backlight control GPIO is input | expand |
On Sun, Jul 18, 2021 at 11:14:15PM +0200, Marek Vasut wrote: > If the backlight enable GPIO is configured as input, the driver currently > unconditionally forces the GPIO to output-enable. This can cause backlight > flicker on boot e.g. in case the GPIO should not be enabled before the PWM > is configured and is correctly pulled low by external resistor. > > Fix this by extending the current check to differentiate between backlight > GPIO enable set as input and set as direction unknown. In case of input, > read the GPIO value to determine the pull resistor placement, set the GPIO > as output, and drive that exact value it was pulled to. In case of unknown > direction, retain previous behavior, that is set the GPIO as output-enable. > > Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Thierry Reding <treding@nvidia.com> > Cc: linux-pwm@vger.kernel.org > Cc: linux-fbdev@vger.kernel.org > To: dri-devel@lists.freedesktop.org > --- > NOTE: I think this whole auto-detection scheme should just be replaced by a > DT prop, because it is very fragile. I have some sympathy for this view... although I think the boat has already set sail. However, on the basis of making things less fragile, I think the underlying problem here is the assumption that it is safe to modify enable_gpio before the driver has imposed state upon the PWM (this assumption has always been made and, in addition to systems where the BL has a phandle will also risks flicker problems on systems where power_pwm_on_delay is not zero). This patch does not change the assumption that we can configure the GPIO before we modify the PWM state. This means it won't fix the problem for cases there the pin is HiZ by default but whose GPIOD_ASIS state is neither input nor output. I wonder if it might be better to move the code to configure the direction of enable_gpio out of the probe function and into pwm_backlight_power_on(): if (pb->enable_gpio) { if (gpiod_get_direction(pb->enable_gpio) != 0)) gpiod_direction_output(pb->enable_gpio, 1); else gpiod_set_value_can_sleep(pb->enable_gpio, 1); } By the time we reach this function the driver explicitly applies state to the GPIO then we know what the value must be. Daniel. > --- > drivers/video/backlight/pwm_bl.c | 35 +++++++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 10 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index e48fded3e414..7ec992b722eb 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -445,7 +445,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) > struct device_node *node = pdev->dev.of_node; > struct pwm_bl_data *pb; > struct pwm_state state; > - unsigned int i; > + unsigned int i, dir, val; > int ret; > > if (!data) { > @@ -487,16 +487,31 @@ static int pwm_backlight_probe(struct platform_device *pdev) > } > > /* > - * If the GPIO is not known to be already configured as output, that > - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the > - * direction to output and set the GPIO as active. > - * Do not force the GPIO to active when it was already output as it > - * could cause backlight flickering or we would enable the backlight too > - * early. Leave the decision of the initial backlight state for later. > + * If the GPIO is not known to be already configured as output, then: > + * - if the GPIO direction is input, read its current value to find out > + * whether the pin is pulled high or low (it is backlight control, so > + * it cannot be floating), change the direction to output and set the > + * GPIO such that it drives this strapped value. > + * Do not force the GPIO to state which is different than that to > + * which the GPIO was pulled to, this could cause backlight flicker > + * on boot e.g. in case the PWM is not ready yet. > + * - if the GPIO direction is unknown, tahat is, if gpiod_get_direction > + * returns -EINVAL, change the direction to output and set the GPIO > + * as active. > + * Do not force the GPIO to active when it was already output as it > + * could cause backlight flickering or we would enable the backlight > + * too early. Leave the decision of the initial backlight state for > + * later. > */ > - if (pb->enable_gpio && > - gpiod_get_direction(pb->enable_gpio) != 0) > - gpiod_direction_output(pb->enable_gpio, 1); > + if (pb->enable_gpio) { > + dir = gpiod_get_direction(pb->enable_gpio); > + if (dir != 0) { > + val = 1; > + if (dir == 1) > + val = gpiod_get_value_cansleep(pb->enable_gpio); > + gpiod_direction_output(pb->enable_gpio, val); > + } > + } > > pb->power_supply = devm_regulator_get(&pdev->dev, "power"); > if (IS_ERR(pb->power_supply)) { > -- > 2.30.2 >
On 7/19/21 1:22 PM, Daniel Thompson wrote: > On Sun, Jul 18, 2021 at 11:14:15PM +0200, Marek Vasut wrote: >> If the backlight enable GPIO is configured as input, the driver currently >> unconditionally forces the GPIO to output-enable. This can cause backlight >> flicker on boot e.g. in case the GPIO should not be enabled before the PWM >> is configured and is correctly pulled low by external resistor. >> >> Fix this by extending the current check to differentiate between backlight >> GPIO enable set as input and set as direction unknown. In case of input, >> read the GPIO value to determine the pull resistor placement, set the GPIO >> as output, and drive that exact value it was pulled to. In case of unknown >> direction, retain previous behavior, that is set the GPIO as output-enable. >> >> Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Daniel Thompson <daniel.thompson@linaro.org> >> Cc: Heiko Stuebner <heiko@sntech.de> >> Cc: Philipp Zabel <p.zabel@pengutronix.de> >> Cc: Thierry Reding <treding@nvidia.com> >> Cc: linux-pwm@vger.kernel.org >> Cc: linux-fbdev@vger.kernel.org >> To: dri-devel@lists.freedesktop.org >> --- >> NOTE: I think this whole auto-detection scheme should just be replaced by a >> DT prop, because it is very fragile. > > I have some sympathy for this view... although I think the boat has > already set sail. I'm not sure that's correct, we can simply say that any new uses of the pwm-backlight should specify the initial GPIO configuration, and for the legacy ones, use whatever is in the code now. > However, on the basis of making things less fragile, I think the > underlying problem here is the assumption that it is safe to modify > enable_gpio before the driver has imposed state upon the PWM (this > assumption has always been made and, in addition to systems where the BL > has a phandle will also risks flicker problems on systems where > power_pwm_on_delay is not zero). It is safe to modify the GPIO into defined state, but that defined state is not always out/enabled, that defined state depends on the hardware. > This patch does not change the assumption that we can configure the > GPIO before we modify the PWM state. This means it won't fix the problem > for cases there the pin is HiZ by default but whose GPIOD_ASIS state is > neither input nor output. That is correct, for pin that is floating, we lost. But then I would argue that if your backlight-enable GPIO is floating, the hardware is buggy, I would expect some pull resistor to keep the backlight off on power on on that GPIO. > I wonder if it might be better to move the code to configure the > direction of enable_gpio out of the probe function and into > pwm_backlight_power_on(): No, I tried that already. The first thing that is called on boot is pwm_backlight_power_off() to set the backlight to 0 (and thus set pwm to 0), but since pb->enabled is false, that is where the function exits with configuring PWM and without configuring the GPIO state. I also experimented with some "first time only" flag in those functions, but that looked ugly and complicated the runtime code. > if (pb->enable_gpio) { > if (gpiod_get_direction(pb->enable_gpio) != 0)) > gpiod_direction_output(pb->enable_gpio, 1); > else > gpiod_set_value_can_sleep(pb->enable_gpio, 1); > } > > By the time we reach this function the driver explicitly applies state > to the GPIO then we know what the value must be. See above, I don't think that's the best option.
On Tue, Jul 20, 2021 at 10:28:32PM +0200, Marek Vasut wrote: > On 7/19/21 1:22 PM, Daniel Thompson wrote: > > On Sun, Jul 18, 2021 at 11:14:15PM +0200, Marek Vasut wrote: > > > If the backlight enable GPIO is configured as input, the driver currently > > > unconditionally forces the GPIO to output-enable. This can cause backlight > > > flicker on boot e.g. in case the GPIO should not be enabled before the PWM > > > is configured and is correctly pulled low by external resistor. > > > > > > Fix this by extending the current check to differentiate between backlight > > > GPIO enable set as input and set as direction unknown. In case of input, > > > read the GPIO value to determine the pull resistor placement, set the GPIO > > > as output, and drive that exact value it was pulled to. In case of unknown > > > direction, retain previous behavior, that is set the GPIO as output-enable. > > > > > > Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT") > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > > Cc: Thierry Reding <treding@nvidia.com> > > > Cc: linux-pwm@vger.kernel.org > > > Cc: linux-fbdev@vger.kernel.org > > > To: dri-devel@lists.freedesktop.org > > > --- > > > NOTE: I think this whole auto-detection scheme should just be replaced by a > > > DT prop, because it is very fragile. > > > > I have some sympathy for this view... although I think the boat has > > already set sail. > > I'm not sure that's correct, we can simply say that any new uses of the > pwm-backlight should specify the initial GPIO configuration, and for the > legacy ones, use whatever is in the code now. I'm not 100% against the idea... however if we still have to get the code to read state from the hardware right for legacy cases that means we have to do the same work but with fewer people testing it. > > However, on the basis of making things less fragile, I think the > > underlying problem here is the assumption that it is safe to modify > > enable_gpio before the driver has imposed state upon the PWM (this > > assumption has always been made and, in addition to systems where the BL > > has a phandle will also risks flicker problems on systems where > > power_pwm_on_delay is not zero). > > It is safe to modify the GPIO into defined state, but that defined state is > not always out/enabled, that defined state depends on the hardware. It is only safe to do this once we know what the initial value should be and I'm not sure that value can comes exclusively from reading the pin. > > This patch does not change the assumption that we can configure the > > GPIO before we modify the PWM state. This means it won't fix the problem > > for cases there the pin is HiZ by default but whose GPIOD_ASIS state is > > neither input nor output. > > That is correct, for pin that is floating, we lost. But then I would argue > that if your backlight-enable GPIO is floating, the hardware is buggy, I > would expect some pull resistor to keep the backlight off on power on on > that GPIO. I didn't say that the pin was floating. I said that the pin was in a HiZ state meaning it could still be subject to pull up/down. However there are cases, such as when the regulator is off, where I think it is entirely legitimate for the enable pin to be floating. The current driver does the wrong thing here if the pin is set as input since if the regulator is off the initial enable_gpio value should be 0. > > I wonder if it might be better to move the code to configure the > > direction of enable_gpio out of the probe function and into > > pwm_backlight_power_on(): > > No, I tried that already. > > The first thing that is called on boot is pwm_backlight_power_off() to set > the backlight to 0 (and thus set pwm to 0), but since pb->enabled is false, > that is where the function exits with configuring PWM and without > configuring the GPIO state. > > I also experimented with some "first time only" flag in those functions, but > that looked ugly and complicated the runtime code. I followed that idea and came to a similar conclusion w.r.t. to the first time flag. I think a reasonably elegant approach can be reached by making pwm_backlight_initial_power_state() responsible for ensuring enable_gpio matches the observed hardware state (taking into account both the pin state and the regulator). I think this will fix both your flicker concerns whilst permitting the legitimate cases for a floating pin. Something like: diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index e48fded3e414..8d8959a70e44 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -409,6 +409,33 @@ static bool pwm_backlight_is_linear(struct platform_pwm_backlight_data *data) static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) { struct device_node *node = pb->dev->of_node; + bool active = true; + + /* + * If the enable GPIO is present, observable (either as input + * or output) and off then the backlight is not currently active. + * */ + if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) + active = false; + + if (!regulator_is_enabled(pb->power_supply)) + active = false; + + if (!pwm_is_enabled(pb->pwm)) + active = false; + + /* + * Synchronize the enable_gpio with the observed state of the + * hardware. + */ + if (pb->enable_gpio) + gpiod_direction_output(pb->enable_gpio, active); + + /* + * Do not change pb->enabled here! pb->enabled essentially + * tells us if we own one of the regulator's use counts and + * right now we do not. + */ /* Not booted with device tree or no phandle link to the node */ if (!node || !node->phandle) @@ -420,20 +447,7 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) * assume that another driver will enable the backlight at the * appropriate time. Therefore, if it is disabled, keep it so. */ - - /* if the enable GPIO is disabled, do not enable the backlight */ - if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) - return FB_BLANK_POWERDOWN; - - /* The regulator is disabled, do not enable the backlight */ - if (!regulator_is_enabled(pb->power_supply)) - return FB_BLANK_POWERDOWN; - - /* The PWM is disabled, keep it like this */ - if (!pwm_is_enabled(pb->pwm)) - return FB_BLANK_POWERDOWN; - - return FB_BLANK_UNBLANK; + return active ? FB_BLANK_UNBLANK: FB_BLANK_POWERDOWN; } static int pwm_backlight_probe(struct platform_device *pdev) @@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) goto err_alloc; } - /* - * If the GPIO is not known to be already configured as output, that - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the - * direction to output and set the GPIO as active. - * Do not force the GPIO to active when it was already output as it - * could cause backlight flickering or we would enable the backlight too - * early. Leave the decision of the initial backlight state for later. - */ - if (pb->enable_gpio && - gpiod_get_direction(pb->enable_gpio) != 0) - gpiod_direction_output(pb->enable_gpio, 1); - pb->power_supply = devm_regulator_get(&pdev->dev, "power"); if (IS_ERR(pb->power_supply)) { ret = PTR_ERR(pb->power_supply);
On 7/21/21 12:49 PM, Daniel Thompson wrote: [...] >>>> NOTE: I think this whole auto-detection scheme should just be replaced by a >>>> DT prop, because it is very fragile. >>> >>> I have some sympathy for this view... although I think the boat has >>> already set sail. >> >> I'm not sure that's correct, we can simply say that any new uses of the >> pwm-backlight should specify the initial GPIO configuration, and for the >> legacy ones, use whatever is in the code now. > > I'm not 100% against the idea... however if we still have to get the > code to read state from the hardware right for legacy cases that means > we have to do the same work but with fewer people testing it. We can do something like this: if (of_property_read_bool(np, "enable-active-high")) gpiod_direction_output(pb->enable_gpio, 1); else if (of_property_read_bool(np, "enable-active-low")) gpiod_direction_output(pb->enable_gpio, 0); else { WARN_ON_ONCE("Fix your DT"); // or some such notification ... legacy code path ... } Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c of_gpio_flags_quirks() uses, because we are headed into similar mess here I'm afraid. >>> However, on the basis of making things less fragile, I think the >>> underlying problem here is the assumption that it is safe to modify >>> enable_gpio before the driver has imposed state upon the PWM (this >>> assumption has always been made and, in addition to systems where the BL >>> has a phandle will also risks flicker problems on systems where >>> power_pwm_on_delay is not zero). >> >> It is safe to modify the GPIO into defined state, but that defined state is >> not always out/enabled, that defined state depends on the hardware. > > It is only safe to do this once we know what the initial value should be > and I'm not sure that value can comes exclusively from reading the pin. I agree, it is far from perfect, but so is the current code. However, see below regarding the floating backlight enable pin. >>> This patch does not change the assumption that we can configure the >>> GPIO before we modify the PWM state. This means it won't fix the problem >>> for cases there the pin is HiZ by default but whose GPIOD_ASIS state is >>> neither input nor output. >> >> That is correct, for pin that is floating, we lost. But then I would argue >> that if your backlight-enable GPIO is floating, the hardware is buggy, I >> would expect some pull resistor to keep the backlight off on power on on >> that GPIO. > > I didn't say that the pin was floating. I said that the pin was in a HiZ > state meaning it could still be subject to pull up/down. > > However there are cases, such as when the regulator is off, where I > think it is entirely legitimate for the enable pin to be floating. The > current driver does the wrong thing here if the pin is set as input > since if the regulator is off the initial enable_gpio value should be 0. Oh, right, that's a valid point. So if the pin is input, we can basically toss a coin. >>> I wonder if it might be better to move the code to configure the >>> direction of enable_gpio out of the probe function and into >>> pwm_backlight_power_on(): >> >> No, I tried that already. >> >> The first thing that is called on boot is pwm_backlight_power_off() to set >> the backlight to 0 (and thus set pwm to 0), but since pb->enabled is false, >> that is where the function exits with configuring PWM and without >> configuring the GPIO state. >> >> I also experimented with some "first time only" flag in those functions, but >> that looked ugly and complicated the runtime code. > > I followed that idea and came to a similar conclusion w.r.t. to the > first time flag. > > I think a reasonably elegant approach can be reached by making > pwm_backlight_initial_power_state() responsible for ensuring enable_gpio > matches the observed hardware state (taking into account both the pin > state and the regulator). I think this will fix both your flicker > concerns whilst permitting the legitimate cases for a floating pin. > > Something like: I think we are getting closer, but there is extra problem to this. > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index e48fded3e414..8d8959a70e44 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -409,6 +409,33 @@ static bool pwm_backlight_is_linear(struct platform_pwm_backlight_data *data) > static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) > { > struct device_node *node = pb->dev->of_node; > + bool active = true; > + > + /* > + * If the enable GPIO is present, observable (either as input > + * or output) and off then the backlight is not currently active. > + * */ > + if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) > + active = false; This will fail on iMX GPIO controller, where if the GPIO is output, you can read its state, but by default that state is what you wrote into the GPIO output value register, not what is the actual value on the pin (i.e. consider you have a strong pull resistor that overpowers the driver). To have a GPIO which is output and sample the actual pin value, you have to tweak the pinmux and enable the SION bit, then you get the actual value. But that is specific to the iMX GPIO controller/pinmux. So I suspect you might still want to check the direction here. > + if (!regulator_is_enabled(pb->power_supply)) > + active = false; > + > + if (!pwm_is_enabled(pb->pwm)) > + active = false; > + > + /* > + * Synchronize the enable_gpio with the observed state of the > + * hardware. > + */ > + if (pb->enable_gpio) > + gpiod_direction_output(pb->enable_gpio, active); > + > + /* > + * Do not change pb->enabled here! pb->enabled essentially > + * tells us if we own one of the regulator's use counts and > + * right now we do not. > + */ > > /* Not booted with device tree or no phandle link to the node */ > if (!node || !node->phandle) > @@ -420,20 +447,7 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) > * assume that another driver will enable the backlight at the > * appropriate time. Therefore, if it is disabled, keep it so. > */ > - > - /* if the enable GPIO is disabled, do not enable the backlight */ > - if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) > - return FB_BLANK_POWERDOWN; > - > - /* The regulator is disabled, do not enable the backlight */ > - if (!regulator_is_enabled(pb->power_supply)) > - return FB_BLANK_POWERDOWN; > - > - /* The PWM is disabled, keep it like this */ > - if (!pwm_is_enabled(pb->pwm)) > - return FB_BLANK_POWERDOWN; > - > - return FB_BLANK_UNBLANK; > + return active ? FB_BLANK_UNBLANK: FB_BLANK_POWERDOWN; > } > > static int pwm_backlight_probe(struct platform_device *pdev) > @@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) > goto err_alloc; > } > > - /* > - * If the GPIO is not known to be already configured as output, that > - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the > - * direction to output and set the GPIO as active. > - * Do not force the GPIO to active when it was already output as it > - * could cause backlight flickering or we would enable the backlight too > - * early. Leave the decision of the initial backlight state for later. > - */ > - if (pb->enable_gpio && > - gpiod_get_direction(pb->enable_gpio) != 0) > - gpiod_direction_output(pb->enable_gpio, 1); pwm_backlight_initial_power_state() is still called after pwm_apply_state() in pwm_backlight_probe(), so that might still be too late, no ?
On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: > On 7/21/21 12:49 PM, Daniel Thompson wrote: > > > I'm not sure that's correct, we can simply say that any new uses of the > > > pwm-backlight should specify the initial GPIO configuration, and for the > > > legacy ones, use whatever is in the code now. > > > > I'm not 100% against the idea... however if we still have to get the > > code to read state from the hardware right for legacy cases that means > > we have to do the same work but with fewer people testing it. > > We can do something like this: > > if (of_property_read_bool(np, "enable-active-high")) > gpiod_direction_output(pb->enable_gpio, 1); > else if (of_property_read_bool(np, "enable-active-low")) > gpiod_direction_output(pb->enable_gpio, 0); > else { > WARN_ON_ONCE("Fix your DT"); // or some such notification > ... legacy code path ... > } > > Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c > of_gpio_flags_quirks() uses, because we are headed into similar mess here > I'm afraid. I don't quite understand what you mean here. We are using gpiolib so for us there is no concept of active-high or active-low. The only concept for us is whether enable_gpio is asserted or not. What the DT property would be describing is purely whether the bootloader left the backlight on or off. This sails very close to the edge of what is in-scope for DT (at least it does it we can read the inherited state directly from the hardware). What it also means decisions about the DT bindings are more about whether, if the backlight is lit up, the bootloader should also disclose what it thinks it has established as the PWM duty cycle as well. Overall I have fairly grave concerns that this simply moves fragility into the bootloader rather then reducing it. Daniel.
On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: > On 7/21/21 12:49 PM, Daniel Thompson wrote: > > > > However, on the basis of making things less fragile, I think the > > > > underlying problem here is the assumption that it is safe to modify > > > > enable_gpio before the driver has imposed state upon the PWM (this > > > > assumption has always been made and, in addition to systems where the BL > > > > has a phandle will also risks flicker problems on systems where > > > > power_pwm_on_delay is not zero). > > > > > > It is safe to modify the GPIO into defined state, but that defined state is > > > not always out/enabled, that defined state depends on the hardware. > > > > It is only safe to do this once we know what the initial value should be > > and I'm not sure that value can comes exclusively from reading the pin. > > I agree, it is far from perfect, but so is the current code. Agreed. Current handling of enable pin isn't right. > However, see below regarding the floating backlight enable pin. > > > > > This patch does not change the assumption that we can configure the > > > > GPIO before we modify the PWM state. This means it won't fix the problem > > > > for cases there the pin is HiZ by default but whose GPIOD_ASIS state is > > > > neither input nor output. > > > > > > That is correct, for pin that is floating, we lost. But then I would argue > > > that if your backlight-enable GPIO is floating, the hardware is buggy, I > > > would expect some pull resistor to keep the backlight off on power on on > > > that GPIO. > > > > I didn't say that the pin was floating. I said that the pin was in a HiZ > > state meaning it could still be subject to pull up/down. > > > > However there are cases, such as when the regulator is off, where I > > think it is entirely legitimate for the enable pin to be floating. The > > current driver does the wrong thing here if the pin is set as input > > since if the regulator is off the initial enable_gpio value should be 0. > > Oh, right, that's a valid point. > > So if the pin is input, we can basically toss a coin. I don't think it is quite as bad as that: if the PWM and regulator are enabled then it is not okay for this pin to be floating. > > [...] > > I think a reasonably elegant approach can be reached by making > > pwm_backlight_initial_power_state() responsible for ensuring enable_gpio > > matches the observed hardware state (taking into account both the pin > > state and the regulator). I think this will fix both your flicker > > concerns whilst permitting the legitimate cases for a floating pin. > > > > Something like: > > I think we are getting closer, but there is extra problem to this. > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > index e48fded3e414..8d8959a70e44 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -409,6 +409,33 @@ static bool pwm_backlight_is_linear(struct platform_pwm_backlight_data *data) > > static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) > > { > > struct device_node *node = pb->dev->of_node; > > + bool active = true; > > + > > + /* > > + * If the enable GPIO is present, observable (either as input > > + * or output) and off then the backlight is not currently active. > > + * */ > > + if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) > > + active = false; > > This will fail on iMX GPIO controller, where if the GPIO is output, you can > read its state, but by default that state is what you wrote into the GPIO > output value register, not what is the actual value on the pin (i.e. > consider you have a strong pull resistor that overpowers the driver). > > To have a GPIO which is output and sample the actual pin value, you have to > tweak the pinmux and enable the SION bit, then you get the actual value. But > that is specific to the iMX GPIO controller/pinmux. You're describing a situation where we own a GPIO output pin and the value we believe we are driving into the pin is not being achieved due to some additional factor. Do we need to care about that? It sounds like the backlight driver won't work properly in this case since whatever value we set the enable_gpio then it will stay at the same value. > > [...] > > @@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > goto err_alloc; > > } > > - /* > > - * If the GPIO is not known to be already configured as output, that > > - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the > > - * direction to output and set the GPIO as active. > > - * Do not force the GPIO to active when it was already output as it > > - * could cause backlight flickering or we would enable the backlight too > > - * early. Leave the decision of the initial backlight state for later. > > - */ > > - if (pb->enable_gpio && > > - gpiod_get_direction(pb->enable_gpio) != 0) > > - gpiod_direction_output(pb->enable_gpio, 1); > > pwm_backlight_initial_power_state() is still called after pwm_apply_state() > in pwm_backlight_probe(), so that might still be too late, no ? The initial pwm_apply_state() is essentially a nop or, perhaps, a sanity check if you prefer to think if it that way. It can change the PWM period in some (non-DT) cases but only if the PWM is not already running... and the change of period should not start it running. Daniel.
On 7/21/21 6:12 PM, Daniel Thompson wrote: > On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: >> On 7/21/21 12:49 PM, Daniel Thompson wrote: >>>> I'm not sure that's correct, we can simply say that any new uses of the >>>> pwm-backlight should specify the initial GPIO configuration, and for the >>>> legacy ones, use whatever is in the code now. >>> >>> I'm not 100% against the idea... however if we still have to get the >>> code to read state from the hardware right for legacy cases that means >>> we have to do the same work but with fewer people testing it. >> >> We can do something like this: >> >> if (of_property_read_bool(np, "enable-active-high")) >> gpiod_direction_output(pb->enable_gpio, 1); >> else if (of_property_read_bool(np, "enable-active-low")) >> gpiod_direction_output(pb->enable_gpio, 0); >> else { >> WARN_ON_ONCE("Fix your DT"); // or some such notification >> ... legacy code path ... >> } >> >> Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c >> of_gpio_flags_quirks() uses, because we are headed into similar mess here >> I'm afraid. > > I don't quite understand what you mean here. We are using gpiolib so > for us there is no concept of active-high or active-low. The only > concept for us is whether enable_gpio is asserted or not. It would look the same -- just substitute in "enable-on-boot" and "disable-on-boot" DT property. > What the DT property would be describing is purely whether the > bootloader left the backlight on or off. Rather, it would simply control what is the default state of the backlight enable GPIO (enabled/disabled). > This sails very close to the > edge of what is in-scope for DT (at least it does it we can read > the inherited state directly from the hardware). The problem with reading it out of hardware is that the hardware might be in undefined state and expects Linux to define that state, so that does not always work. Hence my initial suggestion to add a DT property to define the state up front, instead of using these fragile heuristics. > What it also means decisions about the DT bindings are more about > whether, if the backlight is lit up, the bootloader should also disclose > what it thinks it has established as the PWM duty cycle as well. Please also consider the case where bootloader configures total minimum of the hardware to start Linux as soon as possible, i.e. it puts Linux in DRAM and jumps to Linux. > Overall I have fairly grave concerns that this simply moves > fragility into the bootloader rather then reducing it. Wait a minute, I think we disconnected somewhere. I would rather prefer to remove the fragility and bootloader dependency altogether, exactly to avoid depending on the state the bootloader left the hardware in.
On 7/21/21 6:43 PM, Daniel Thompson wrote: > On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: >> On 7/21/21 12:49 PM, Daniel Thompson wrote: >>>>> However, on the basis of making things less fragile, I think the >>>>> underlying problem here is the assumption that it is safe to modify >>>>> enable_gpio before the driver has imposed state upon the PWM (this >>>>> assumption has always been made and, in addition to systems where the BL >>>>> has a phandle will also risks flicker problems on systems where >>>>> power_pwm_on_delay is not zero). >>>> >>>> It is safe to modify the GPIO into defined state, but that defined state is >>>> not always out/enabled, that defined state depends on the hardware. >>> >>> It is only safe to do this once we know what the initial value should be >>> and I'm not sure that value can comes exclusively from reading the pin. >> >> I agree, it is far from perfect, but so is the current code. > > Agreed. Current handling of enable pin isn't right. > > >> However, see below regarding the floating backlight enable pin. >> >>>>> This patch does not change the assumption that we can configure the >>>>> GPIO before we modify the PWM state. This means it won't fix the problem >>>>> for cases there the pin is HiZ by default but whose GPIOD_ASIS state is >>>>> neither input nor output. >>>> >>>> That is correct, for pin that is floating, we lost. But then I would argue >>>> that if your backlight-enable GPIO is floating, the hardware is buggy, I >>>> would expect some pull resistor to keep the backlight off on power on on >>>> that GPIO. >>> >>> I didn't say that the pin was floating. I said that the pin was in a HiZ >>> state meaning it could still be subject to pull up/down. >>> >>> However there are cases, such as when the regulator is off, where I >>> think it is entirely legitimate for the enable pin to be floating. The >>> current driver does the wrong thing here if the pin is set as input >>> since if the regulator is off the initial enable_gpio value should be 0. >> >> Oh, right, that's a valid point. >> >> So if the pin is input, we can basically toss a coin. > > I don't think it is quite as bad as that: if the PWM and regulator > are enabled then it is not okay for this pin to be floating. So then we would have to check the regulator and pwm state, however Linux driver for those can reinit both the regulator and pwm block, so we are growing more and more heuristics. >>> [...] >>> I think a reasonably elegant approach can be reached by making >>> pwm_backlight_initial_power_state() responsible for ensuring enable_gpio >>> matches the observed hardware state (taking into account both the pin >>> state and the regulator). I think this will fix both your flicker >>> concerns whilst permitting the legitimate cases for a floating pin. >>> >>> Something like: >> >> I think we are getting closer, but there is extra problem to this. >> >>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c >>> index e48fded3e414..8d8959a70e44 100644 >>> --- a/drivers/video/backlight/pwm_bl.c >>> +++ b/drivers/video/backlight/pwm_bl.c >>> @@ -409,6 +409,33 @@ static bool pwm_backlight_is_linear(struct platform_pwm_backlight_data *data) >>> static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb) >>> { >>> struct device_node *node = pb->dev->of_node; >>> + bool active = true; >>> + >>> + /* >>> + * If the enable GPIO is present, observable (either as input >>> + * or output) and off then the backlight is not currently active. >>> + * */ >>> + if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0) >>> + active = false; >> >> This will fail on iMX GPIO controller, where if the GPIO is output, you can >> read its state, but by default that state is what you wrote into the GPIO >> output value register, not what is the actual value on the pin (i.e. >> consider you have a strong pull resistor that overpowers the driver). >> >> To have a GPIO which is output and sample the actual pin value, you have to >> tweak the pinmux and enable the SION bit, then you get the actual value. But >> that is specific to the iMX GPIO controller/pinmux. > > You're describing a situation where we own a GPIO output pin and the > value we believe we are driving into the pin is not being achieved due > to some additional factor. E.g. disabled PWM or regulator. > Do we need to care about that? It sounds like > the backlight driver won't work properly in this case since whatever > value we set the enable_gpio then it will stay at the same value. Possibly. >>> [...] >>> @@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) >>> goto err_alloc; >>> } >>> - /* >>> - * If the GPIO is not known to be already configured as output, that >>> - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the >>> - * direction to output and set the GPIO as active. >>> - * Do not force the GPIO to active when it was already output as it >>> - * could cause backlight flickering or we would enable the backlight too >>> - * early. Leave the decision of the initial backlight state for later. >>> - */ >>> - if (pb->enable_gpio && >>> - gpiod_get_direction(pb->enable_gpio) != 0) >>> - gpiod_direction_output(pb->enable_gpio, 1); >> >> pwm_backlight_initial_power_state() is still called after pwm_apply_state() >> in pwm_backlight_probe(), so that might still be too late, no ? > > The initial pwm_apply_state() is essentially a nop or, perhaps, a sanity > check if you prefer to think if it that way. > > It can change the PWM period in some (non-DT) cases but only if the PWM > is not already running... and the change of period should not start it > running. All right, let me give this a try.
On 7/21/21 9:01 PM, Marek Vasut wrote: [...] >>>> @@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct >>>> platform_device *pdev) >>>> goto err_alloc; >>>> } >>>> - /* >>>> - * If the GPIO is not known to be already configured as output, >>>> that >>>> - * is, if gpiod_get_direction returns either 1 or -EINVAL, >>>> change the >>>> - * direction to output and set the GPIO as active. >>>> - * Do not force the GPIO to active when it was already output >>>> as it >>>> - * could cause backlight flickering or we would enable the >>>> backlight too >>>> - * early. Leave the decision of the initial backlight state for >>>> later. >>>> - */ >>>> - if (pb->enable_gpio && >>>> - gpiod_get_direction(pb->enable_gpio) != 0) >>>> - gpiod_direction_output(pb->enable_gpio, 1); >>> >>> pwm_backlight_initial_power_state() is still called after >>> pwm_apply_state() >>> in pwm_backlight_probe(), so that might still be too late, no ? >> >> The initial pwm_apply_state() is essentially a nop or, perhaps, a sanity >> check if you prefer to think if it that way. >> >> It can change the PWM period in some (non-DT) cases but only if the PWM >> is not already running... and the change of period should not start it >> running. > > All right, let me give this a try. ACK, for this case I have here, this works too. Can you submit a proper patch, including my AB/TB and I think also the Fixes tag ?
On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote: > On 7/21/21 6:12 PM, Daniel Thompson wrote: > > On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: > > > On 7/21/21 12:49 PM, Daniel Thompson wrote: > > > > > I'm not sure that's correct, we can simply say that any new uses of the > > > > > pwm-backlight should specify the initial GPIO configuration, and for the > > > > > legacy ones, use whatever is in the code now. > > > > > > > > I'm not 100% against the idea... however if we still have to get the > > > > code to read state from the hardware right for legacy cases that means > > > > we have to do the same work but with fewer people testing it. > > > > > > We can do something like this: > > > > > > if (of_property_read_bool(np, "enable-active-high")) > > > gpiod_direction_output(pb->enable_gpio, 1); > > > else if (of_property_read_bool(np, "enable-active-low")) > > > gpiod_direction_output(pb->enable_gpio, 0); > > > else { > > > WARN_ON_ONCE("Fix your DT"); // or some such notification > > > ... legacy code path ... > > > } > > > > > > Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c > > > of_gpio_flags_quirks() uses, because we are headed into similar mess here > > > I'm afraid. > > > > I don't quite understand what you mean here. We are using gpiolib so > > for us there is no concept of active-high or active-low. The only > > concept for us is whether enable_gpio is asserted or not. > > It would look the same -- just substitute in "enable-on-boot" and > "disable-on-boot" DT property. > > > What the DT property would be describing is purely whether the > > bootloader left the backlight on or off. > > Rather, it would simply control what is the default state of the backlight > enable GPIO (enabled/disabled). What do you mean by default state? The current value of the pin or the desired state when the probe completes? > > This sails very close to the > > edge of what is in-scope for DT (at least it does it we can read > > the inherited state directly from the hardware). > > The problem with reading it out of hardware is that the hardware might be in > undefined state and expects Linux to define that state, so that does not > always work. Hence my initial suggestion to add a DT property to define the > state up front, instead of using these fragile heuristics. To achieve a flicker-free boot we must know the initial state of the backlight (not just the enable pin). > > What it also means decisions about the DT bindings are more about > > whether, if the backlight is lit up, the bootloader should also disclose > > what it thinks it has established as the PWM duty cycle as well. > > Please also consider the case where bootloader configures total minimum of > the hardware to start Linux as soon as possible, i.e. it puts Linux in DRAM > and jumps to Linux. I think I have been. I understood that preventing a flicker when booting with an unconfigured (and off) backlight[1] was the purpose of your patch! However that is a relatively easy case when considering flicker-free handover from bootloader since we don't have to inherit the duty cycle. [1] I guessed that the difference between your platform and the other pwm_bl users is that, for you, an uninitialized PWM looks like a 100% duty cycle hence you get a full-brightness flicker when we change the state of the enable_gpio pin before we have established the correct PWM duty cycle. Was I right? > > Overall I have fairly grave concerns that this simply moves > > fragility into the bootloader rather then reducing it. > > Wait a minute, I think we disconnected somewhere. I would rather prefer to > remove the fragility and bootloader dependency altogether, exactly to avoid > depending on the state the bootloader left the hardware in. The two fully flicker-free cases we support in pwm_bl are: 1. Backlight off after bootloader has completed. Backlight must be off after the probe completes (and never flicker on/off during the probe). This allows the display to put a splash image on the screen before lighting up the backlight (this avoids a flicker if there are a few frames between backlight coming on and the splash image being drawn). Naturally this requires help from the display system (and that the display system is aware of the backlight to be able to start it). 2. Backlight on with a splash image after bootloader has completed. Backlight must be on after the probe completes (and never flicker off/on during the probe). This also requires that the display system can take over the frame buffer without a flicker but that is completely independent of backlight. There is also a simpler case which is not "flicker-free" since the backlight may change level during the boot and may light up before there is an image on the screen (although we'd still to minimise flicker by ensuring there is only one change of backlight state/level during the probe (something your work will see fixed?): 3. Backlight is on after the probe completes. This is default if we don't know the display system will activate the backlight. This is an important legacy case since few userspaces know how to change the backlight power-state at boot. One oddity here is that #3 *also* needs to know the state of the backlight (on/off) to turn the backlight on without flickering (so it can figure out how to handle power_pwm_on_delay correctly) even though the final state is unconditionally on. That is the main reason I proposed an alternative to your patch (since this is currently broken). The other oddity is that the difference between #1 and #3 is due to *software* (which driver ends up responsible for unmuting the display) meaning that the bootloader/DT has no business discriminating between these two cases. Thus pwm_bl.c is based on making #2/#3 (which are similar) the default and switching to case #1 if there is a display driver to do the unblank (software) *and* that the backlight is currently off (currently read from hardware). Note that this is intentionally designed to that if the logic does go wrong we should get a small bug (a flicker) rather than a big one (a black screen). Wow! That is *way* longer than I intended when I started writing it. Anyhow I suspect any disconnect comes about due to the difference in backlight state *after* probe being, in part, to software structure rather than purely a hardware property. Daniel.
On 7/22/21 1:28 PM, Daniel Thompson wrote: > On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote: >> On 7/21/21 6:12 PM, Daniel Thompson wrote: >>> On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: >>>> On 7/21/21 12:49 PM, Daniel Thompson wrote: >>>>>> I'm not sure that's correct, we can simply say that any new uses of the >>>>>> pwm-backlight should specify the initial GPIO configuration, and for the >>>>>> legacy ones, use whatever is in the code now. >>>>> >>>>> I'm not 100% against the idea... however if we still have to get the >>>>> code to read state from the hardware right for legacy cases that means >>>>> we have to do the same work but with fewer people testing it. >>>> >>>> We can do something like this: >>>> >>>> if (of_property_read_bool(np, "enable-active-high")) >>>> gpiod_direction_output(pb->enable_gpio, 1); >>>> else if (of_property_read_bool(np, "enable-active-low")) >>>> gpiod_direction_output(pb->enable_gpio, 0); >>>> else { >>>> WARN_ON_ONCE("Fix your DT"); // or some such notification >>>> ... legacy code path ... >>>> } >>>> >>>> Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c >>>> of_gpio_flags_quirks() uses, because we are headed into similar mess here >>>> I'm afraid. >>> >>> I don't quite understand what you mean here. We are using gpiolib so >>> for us there is no concept of active-high or active-low. The only >>> concept for us is whether enable_gpio is asserted or not. >> >> It would look the same -- just substitute in "enable-on-boot" and >> "disable-on-boot" DT property. >> >>> What the DT property would be describing is purely whether the >>> bootloader left the backlight on or off. >> >> Rather, it would simply control what is the default state of the backlight >> enable GPIO (enabled/disabled). > > What do you mean by default state? The current value of the pin or the > desired state when the probe completes? I think that would be the later. >>> This sails very close to the >>> edge of what is in-scope for DT (at least it does it we can read >>> the inherited state directly from the hardware). >> >> The problem with reading it out of hardware is that the hardware might be in >> undefined state and expects Linux to define that state, so that does not >> always work. Hence my initial suggestion to add a DT property to define the >> state up front, instead of using these fragile heuristics. > > To achieve a flicker-free boot we must know the initial state of the > backlight (not just the enable pin). The backlight hardware might be in uninitialized state and then Linux should set the state, likely based on something in DT, because there is no previous state to read. >>> What it also means decisions about the DT bindings are more about >>> whether, if the backlight is lit up, the bootloader should also disclose >>> what it thinks it has established as the PWM duty cycle as well. >> >> Please also consider the case where bootloader configures total minimum of >> the hardware to start Linux as soon as possible, i.e. it puts Linux in DRAM >> and jumps to Linux. > > I think I have been. I understood that preventing a flicker when booting > with an unconfigured (and off) backlight[1] was the purpose of your patch! > > However that is a relatively easy case when considering flicker-free > handover from bootloader since we don't have to inherit the duty cycle. > > > [1] I guessed that the difference between your platform and the other > pwm_bl users is that, for you, an uninitialized PWM looks like a > 100% duty cycle hence you get a full-brightness flicker when we change > the state of the enable_gpio pin before we have established the > correct PWM duty cycle. Was I right? Right, that. In my case the enable GPIO is input (default) with pull resistor. >>> Overall I have fairly grave concerns that this simply moves >>> fragility into the bootloader rather then reducing it. >> >> Wait a minute, I think we disconnected somewhere. I would rather prefer to >> remove the fragility and bootloader dependency altogether, exactly to avoid >> depending on the state the bootloader left the hardware in. > > The two fully flicker-free cases we support in pwm_bl are: > > 1. Backlight off after bootloader has completed. Backlight must be > off after the probe completes (and never flicker on/off during the > probe). This allows the display to put a splash image on the screen > before lighting up the backlight (this avoids a flicker if there are > a few frames between backlight coming on and the splash image being > drawn). Naturally this requires help from the display system (and > that the display system is aware of the backlight to be able to start > it). > > 2. Backlight on with a splash image after bootloader has completed. > Backlight must be on after the probe completes (and never flicker > off/on during the probe). This also requires that the display system > can take over the frame buffer without a flicker but that is > completely independent of backlight. > > There is also a simpler case which is not "flicker-free" since the > backlight may change level during the boot and may light up before > there is an image on the screen (although we'd still to minimise > flicker by ensuring there is only one change of backlight state/level > during the probe (something your work will see fixed?): Actually no, my usecase is the backlight is not initialized by the bootloader at all, the pins are just strapped to default to the right values, the init is left to the kernel to do. > 3. Backlight is on after the probe completes. This is default if > we don't know the display system will activate the backlight. > This is an important legacy case since few userspaces know how > to change the backlight power-state at boot. > > One oddity here is that #3 *also* needs to know the state of the > backlight (on/off) to turn the backlight on without flickering > (so it can figure out how to handle power_pwm_on_delay correctly) > even though the final state is unconditionally on. That is the main > reason I proposed an alternative to your patch (since this is > currently broken). > > The other oddity is that the difference between #1 and #3 is due to > *software* (which driver ends up responsible for unmuting the display) > meaning that the bootloader/DT has no business discriminating between > these two cases. > > Thus pwm_bl.c is based on making #2/#3 (which are similar) the default > and switching to case #1 if there is a display driver to do the unblank > (software) *and* that the backlight is currently off (currently read > from hardware). Note that this is intentionally designed to that > if the logic does go wrong we should get a small bug (a flicker) rather > than a big one (a black screen). > > Wow! That is *way* longer than I intended when I started writing it. > Anyhow I suspect any disconnect comes about due to the difference in > backlight state *after* probe being, in part, to software structure > rather than purely a hardware property. Maybe this should be added to documentation.
On Thu, Jul 22, 2021 at 09:02:04PM +0200, Marek Vasut wrote: > On 7/22/21 1:28 PM, Daniel Thompson wrote: > > On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote: > > > On 7/21/21 6:12 PM, Daniel Thompson wrote: > > > > On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: > > > > > On 7/21/21 12:49 PM, Daniel Thompson wrote: > > > > [...] > > > > This sails very close to the > > > > edge of what is in-scope for DT (at least it does it we can read > > > > the inherited state directly from the hardware). > > > > > > The problem with reading it out of hardware is that the hardware might be in > > > undefined state and expects Linux to define that state, so that does not > > > always work. Hence my initial suggestion to add a DT property to define the > > > state up front, instead of using these fragile heuristics. > > > > To achieve a flicker-free boot we must know the initial state of the > > backlight (not just the enable pin). > > The backlight hardware might be in uninitialized state and then Linux should > set the state, likely based on something in DT, because there is no previous > state to read. There is always a previous state. The kernel doesn't care whether that previous state was imposed by a power-on reset, the bootloader or a kexec. For the driver to come up flicker-free in all the different cases we need to know whether the backlight is currently emitting light or not and, if it is emitting light, then we need to know what the duty cycle is (currently we inherit require the PWM driver to correctly inherit the duty cycle from the hardware). So far, the previous state has been observable by the lower level drivers (GPIO, PWM, regulator). I remain reluctant to provide workarounds for cases where it is not observable without motivating hardware. I certainly wouldn't want to make such bindings mandatory since observable hardware registers are a far more reliable source of truth than what the DT tells us about what it thinks the bootloader (or power-on reset) actually did ;-). > > > > Overall I have fairly grave concerns that this simply moves > > > > fragility into the bootloader rather then reducing it. > > > > > > Wait a minute, I think we disconnected somewhere. I would rather prefer to > > > remove the fragility and bootloader dependency altogether, exactly to avoid > > > depending on the state the bootloader left the hardware in. > > > > The two fully flicker-free cases we support in pwm_bl are: > > > > 1. Backlight off after bootloader has completed. Backlight must be > > off after the probe completes (and never flicker on/off during the > > probe). This allows the display to put a splash image on the screen > > before lighting up the backlight (this avoids a flicker if there are > > a few frames between backlight coming on and the splash image being > > drawn). Naturally this requires help from the display system (and > > that the display system is aware of the backlight to be able to start > > it). > > > > 2. Backlight on with a splash image after bootloader has completed. > > Backlight must be on after the probe completes (and never flicker > > off/on during the probe). This also requires that the display system > > can take over the frame buffer without a flicker but that is > > completely independent of backlight. > > > > There is also a simpler case which is not "flicker-free" since the > > backlight may change level during the boot and may light up before > > there is an image on the screen (although we'd still to minimise > > flicker by ensuring there is only one change of backlight state/level > > during the probe (something your work will see fixed?): > > Actually no, my usecase is the backlight is not initialized by the > bootloader at all, the pins are just strapped to default to the right > values, the init is left to the kernel to do. It doesn't matter to us who established the initial state. In this case the backlight is off at handover. That means you are either case #1 (display system will unblank" the backlight automatically when the reset of the display unblanks) or #3 (BL driver will "unblank" the backlight during the probe). Your changes should result in a fix to both these cases! > > 3. Backlight is on after the probe completes. This is default if > > we don't know the display system will activate the backlight. > > This is an important legacy case since few userspaces know how > > to change the backlight power-state at boot. > > > > One oddity here is that #3 *also* needs to know the state of the > > backlight (on/off) to turn the backlight on without flickering > > (so it can figure out how to handle power_pwm_on_delay correctly) > > even though the final state is unconditionally on. That is the main > > reason I proposed an alternative to your patch (since this is > > currently broken). > > > > The other oddity is that the difference between #1 and #3 is due to > > *software* (which driver ends up responsible for unmuting the display) > > meaning that the bootloader/DT has no business discriminating between > > these two cases. > > > > Thus pwm_bl.c is based on making #2/#3 (which are similar) the default > > and switching to case #1 if there is a display driver to do the unblank > > (software) *and* that the backlight is currently off (currently read > > from hardware). Note that this is intentionally designed to that > > if the logic does go wrong we should get a small bug (a flicker) rather > > than a big one (a black screen). > > > > Wow! That is *way* longer than I intended when I started writing it. > > Anyhow I suspect any disconnect comes about due to the difference in > > backlight state *after* probe being, in part, to software structure > > rather than purely a hardware property. > > Maybe this should be added to documentation. I'll see what I can do. Daniel.
On Fri, Jul 23, 2021 at 11:15:10AM +0100, Daniel Thompson wrote: > On Thu, Jul 22, 2021 at 09:02:04PM +0200, Marek Vasut wrote: > > On 7/22/21 1:28 PM, Daniel Thompson wrote: > > > On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote: > > > > On 7/21/21 6:12 PM, Daniel Thompson wrote: > > > > > On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: > > > > > > On 7/21/21 12:49 PM, Daniel Thompson wrote: > > > > > [...] > > > > > This sails very close to the > > > > > edge of what is in-scope for DT (at least it does it we can read > > > > > the inherited state directly from the hardware). > > > > > > > > The problem with reading it out of hardware is that the hardware might be in > > > > undefined state and expects Linux to define that state, so that does not > > > > always work. Hence my initial suggestion to add a DT property to define the > > > > state up front, instead of using these fragile heuristics. > > > > > > To achieve a flicker-free boot we must know the initial state of the > > > backlight (not just the enable pin). > > > > The backlight hardware might be in uninitialized state and then Linux should > > set the state, likely based on something in DT, because there is no previous > > state to read. > > There is always a previous state. The kernel doesn't care whether that > previous state was imposed by a power-on reset, the bootloader or a > kexec. > > For the driver to come up flicker-free in all the different cases we > need to know whether the backlight is currently emitting light or not > and, if it is emitting light, then we need to know what the duty cycle > is (currently we inherit require the PWM driver to correctly inherit the > duty cycle from the hardware). Oops... this is wrong (I think it is cross-talk from an old patch). We do not currently inherit the duty cycle. > So far, the previous state has been observable by the lower level > drivers (GPIO, PWM, regulator). I remain reluctant to provide > workarounds for cases where it is not observable without motivating > hardware. I certainly wouldn't want to make such bindings mandatory > since observable hardware registers are a far more reliable source of > truth than what the DT tells us about what it thinks the bootloader > (or power-on reset) actually did ;-). Which makes conclusion badly reasoned. However, until we can clearly articulate whether the problem we want to solve is describing the initial backlight state or specifying the default (post-probe) power state for the legacy cases I'm still content not to change things ;-). > > > [...] > > > Wow! That is *way* longer than I intended when I started writing it. > > > Anyhow I suspect any disconnect comes about due to the difference in > > > backlight state *after* probe being, in part, to software structure > > > rather than purely a hardware property. > > > > Maybe this should be added to documentation. > > I'll see what I can do. Done, see v3. I think it is better explained than the e-mail version. Daniel.
On 7/23/21 1:17 PM, Daniel Thompson wrote: > On Fri, Jul 23, 2021 at 11:15:10AM +0100, Daniel Thompson wrote: >> On Thu, Jul 22, 2021 at 09:02:04PM +0200, Marek Vasut wrote: >>> On 7/22/21 1:28 PM, Daniel Thompson wrote: >>>> On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote: >>>>> On 7/21/21 6:12 PM, Daniel Thompson wrote: >>>>>> On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote: >>>>>>> On 7/21/21 12:49 PM, Daniel Thompson wrote: >>>>>> [...] >>>>>> This sails very close to the >>>>>> edge of what is in-scope for DT (at least it does it we can read >>>>>> the inherited state directly from the hardware). >>>>> >>>>> The problem with reading it out of hardware is that the hardware might be in >>>>> undefined state and expects Linux to define that state, so that does not >>>>> always work. Hence my initial suggestion to add a DT property to define the >>>>> state up front, instead of using these fragile heuristics. >>>> >>>> To achieve a flicker-free boot we must know the initial state of the >>>> backlight (not just the enable pin). >>> >>> The backlight hardware might be in uninitialized state and then Linux should >>> set the state, likely based on something in DT, because there is no previous >>> state to read. >> >> There is always a previous state. The kernel doesn't care whether that >> previous state was imposed by a power-on reset, the bootloader or a >> kexec. >> >> For the driver to come up flicker-free in all the different cases we >> need to know whether the backlight is currently emitting light or not >> and, if it is emitting light, then we need to know what the duty cycle >> is (currently we inherit require the PWM driver to correctly inherit the >> duty cycle from the hardware). > > Oops... this is wrong (I think it is cross-talk from an old patch). We > do not currently inherit the duty cycle. There is that, and if you did, you would be telling PWM drivers not to reset/reinit the hardware in probe. I'm not sure whether that is a good idea. >> So far, the previous state has been observable by the lower level >> drivers (GPIO, PWM, regulator). I remain reluctant to provide >> workarounds for cases where it is not observable without motivating >> hardware. I certainly wouldn't want to make such bindings mandatory >> since observable hardware registers are a far more reliable source of >> truth than what the DT tells us about what it thinks the bootloader >> (or power-on reset) actually did ;-). > > Which makes conclusion badly reasoned. > > However, until we can clearly articulate whether the problem we want to > solve is describing the initial backlight state or specifying the default > (post-probe) power state for the legacy cases I'm still content not to > change things ;-). For me, it was always about specifying well defined default state of the backlight. >>>> [...] >>>> Wow! That is *way* longer than I intended when I started writing it. >>>> Anyhow I suspect any disconnect comes about due to the difference in >>>> backlight state *after* probe being, in part, to software structure >>>> rather than purely a hardware property. >>> >>> Maybe this should be added to documentation. >> >> I'll see what I can do. > > Done, see v3. I think it is better explained than the e-mail version. Thanks
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index e48fded3e414..7ec992b722eb 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -445,7 +445,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) struct device_node *node = pdev->dev.of_node; struct pwm_bl_data *pb; struct pwm_state state; - unsigned int i; + unsigned int i, dir, val; int ret; if (!data) { @@ -487,16 +487,31 @@ static int pwm_backlight_probe(struct platform_device *pdev) } /* - * If the GPIO is not known to be already configured as output, that - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the - * direction to output and set the GPIO as active. - * Do not force the GPIO to active when it was already output as it - * could cause backlight flickering or we would enable the backlight too - * early. Leave the decision of the initial backlight state for later. + * If the GPIO is not known to be already configured as output, then: + * - if the GPIO direction is input, read its current value to find out + * whether the pin is pulled high or low (it is backlight control, so + * it cannot be floating), change the direction to output and set the + * GPIO such that it drives this strapped value. + * Do not force the GPIO to state which is different than that to + * which the GPIO was pulled to, this could cause backlight flicker + * on boot e.g. in case the PWM is not ready yet. + * - if the GPIO direction is unknown, tahat is, if gpiod_get_direction + * returns -EINVAL, change the direction to output and set the GPIO + * as active. + * Do not force the GPIO to active when it was already output as it + * could cause backlight flickering or we would enable the backlight + * too early. Leave the decision of the initial backlight state for + * later. */ - if (pb->enable_gpio && - gpiod_get_direction(pb->enable_gpio) != 0) - gpiod_direction_output(pb->enable_gpio, 1); + if (pb->enable_gpio) { + dir = gpiod_get_direction(pb->enable_gpio); + if (dir != 0) { + val = 1; + if (dir == 1) + val = gpiod_get_value_cansleep(pb->enable_gpio); + gpiod_direction_output(pb->enable_gpio, val); + } + } pb->power_supply = devm_regulator_get(&pdev->dev, "power"); if (IS_ERR(pb->power_supply)) {
If the backlight enable GPIO is configured as input, the driver currently unconditionally forces the GPIO to output-enable. This can cause backlight flicker on boot e.g. in case the GPIO should not be enabled before the PWM is configured and is correctly pulled low by external resistor. Fix this by extending the current check to differentiate between backlight GPIO enable set as input and set as direction unknown. In case of input, read the GPIO value to determine the pull resistor placement, set the GPIO as output, and drive that exact value it was pulled to. In case of unknown direction, retain previous behavior, that is set the GPIO as output-enable. Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: Daniel Thompson <daniel.thompson@linaro.org> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Thierry Reding <treding@nvidia.com> Cc: linux-pwm@vger.kernel.org Cc: linux-fbdev@vger.kernel.org To: dri-devel@lists.freedesktop.org --- NOTE: I think this whole auto-detection scheme should just be replaced by a DT prop, because it is very fragile. --- drivers/video/backlight/pwm_bl.c | 35 +++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-)