The initial PWM state returned by pwm_init_state() has a duty cycle
of 0 ns. To avoid backlight flicker when taking over an enabled
display from the bootloader, skip the initial pwm_apply_state()
and leave the PWM be until backlight_update_state() will apply the
state with the desired brightness.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
With a PWM driver that allows to inherit PWM state from the bootloader,
postponing the initial pwm_apply_state() with 0 ns duty cycle allows to
set the desired duty cycle before the PWM is set, avoiding a short flicker
if the backlight was previously enabled and will be enabled again.
---
 drivers/video/backlight/pwm_bl.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index fce412234d10..47a917038f58 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -531,12 +531,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (!state.period && (data->pwm_period_ns > 0))
 		state.period = data->pwm_period_ns;
 
-	ret = pwm_apply_state(pb->pwm, &state);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
-			ret);
-		goto err_alloc;
-	}
+	/*
+	 * No need to apply initial state, except in the error path.
+	 * State will be applied by backlight_update_status() on success.
+	 */
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 
@@ -573,7 +571,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(&pdev->dev,
 				"failed to setup default brightness table\n");
-			goto err_alloc;
+			goto err_apply;
 		}
 
 		for (i = 0; i <= data->max_brightness; i++) {
@@ -602,7 +600,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_alloc;
+		goto err_apply;
 	}
 
 	if (data->dft_brightness > data->max_brightness) {
@@ -619,6 +617,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, bl);
 	return 0;
 
+err_apply:
+	pwm_apply_state(pb->pwm, &state);
 err_alloc:
 	if (data->exit)
 		data->exit(&pdev->dev);

---
base-commit: ac9a78681b921877518763ba0e89202254349d1b
change-id: 20230608-backlight-pwm-avoid-flicker-d2dd8d12f826

Best regards,
-- 
Philipp Zabel <p.zabel@pengutronix.de>

Hello Philipp,

On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote:
> The initial PWM state returned by pwm_init_state() has a duty cycle
> of 0 ns.

This is only true for drivers without a .get_state() callback, isn't it?

> To avoid backlight flicker when taking over an enabled
> display from the bootloader, skip the initial pwm_apply_state()
> and leave the PWM be until backlight_update_state() will apply the
> state with the desired brightness.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> With a PWM driver that allows to inherit PWM state from the bootloader,
> postponing the initial pwm_apply_state() with 0 ns duty cycle allows to
> set the desired duty cycle before the PWM is set, avoiding a short flicker
> if the backlight was previously enabled and will be enabled again.
> ---
>  drivers/video/backlight/pwm_bl.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index fce412234d10..47a917038f58 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -531,12 +531,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	if (!state.period && (data->pwm_period_ns > 0))
>  		state.period = data->pwm_period_ns;
>  
> -	ret = pwm_apply_state(pb->pwm, &state);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> -			ret);
> -		goto err_alloc;
> -	}
> +	/*
> +	 * No need to apply initial state, except in the error path.

Why do you want to modify the PWM in the error path? I would have
expected not touching it at all in .probe() is fine?!

> +	 * State will be applied by backlight_update_status() on success.
> +	 */
>  
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Wed, Oct 18, 2023 at 11:07:41PM +0200, Uwe Kleine-König wrote:
> Hello Philipp,
>
> On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote:
> > The initial PWM state returned by pwm_init_state() has a duty cycle
> > of 0 ns.
>
> This is only true for drivers without a .get_state() callback, isn't it?

pwm_init_state() explicitly zeros the duty-cycle in order to avoid
problems when the default args have a different period to the currently
applied config:
https://elixir.bootlin.com/linux/latest/source/include/linux/pwm.h#L174


Daniel.


> > To avoid backlight flicker when taking over an enabled
> > display from the bootloader, skip the initial pwm_apply_state()
> > and leave the PWM be until backlight_update_state() will apply the
> > state with the desired brightness.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > With a PWM driver that allows to inherit PWM state from the bootloader,
> > postponing the initial pwm_apply_state() with 0 ns duty cycle allows to
> > set the desired duty cycle before the PWM is set, avoiding a short flicker
> > if the backlight was previously enabled and will be enabled again.
> > ---
> >  drivers/video/backlight/pwm_bl.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index fce412234d10..47a917038f58 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -531,12 +531,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >  	if (!state.period && (data->pwm_period_ns > 0))
> >  		state.period = data->pwm_period_ns;
> >
> > -	ret = pwm_apply_state(pb->pwm, &state);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> > -			ret);
> > -		goto err_alloc;
> > -	}
> > +	/*
> > +	 * No need to apply initial state, except in the error path.
>
> Why do you want to modify the PWM in the error path? I would have
> expected not touching it at all in .probe() is fine?!
>
> > +	 * State will be applied by backlight_update_status() on success.
> > +	 */
> >
> >  	memset(&props, 0, sizeof(struct backlight_properties));
> >
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hello Daniel,

On Fri, Oct 20, 2023 at 12:27:27PM +0100, Daniel Thompson wrote:
> On Wed, Oct 18, 2023 at 11:07:41PM +0200, Uwe Kleine-König wrote:
> > Hello Philipp,
> >
> > On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote:
> > > The initial PWM state returned by pwm_init_state() has a duty cycle
> > > of 0 ns.
> >
> > This is only true for drivers without a .get_state() callback, isn't it?
> 
> pwm_init_state() explicitly zeros the duty-cycle in order to avoid
> problems when the default args have a different period to the currently
> applied config:
> https://elixir.bootlin.com/linux/latest/source/include/linux/pwm.h#L174

Ah right, pwm_init_state() is strange in a different way than I
remembered :-) pwm_get_state() is only called to get .enabled set
appropriately.

Looking at the callers:

 - drivers/gpu/drm/solomon/ssd130x.c
   It does:
 	pwm_init_state(ssd130x->pwm, &pwmstate);
	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
	pwm_apply_state(ssd130x->pwm, &pwmstate);
	pwm_enable(ssd130x->pwm);

   A probably better result can be reached quicker using:
 	pwm_init_state(ssd130x->pwm, &pwmstate);
	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
	pwmstate.enabled = true;
	pwm_apply_state(ssd130x->pwm, &pwmstate);
 - drivers/hwmon/pwm-fan.c
   __set_pwm should probably explicitly set .enabled.
   All other calls to pwm_apply_state set .enabled explicitly.

 - drivers/input/misc/da7280.c
   explicitly sets .enabled after calling pwm_init_state()

 - drivers/input/misc/pwm-beeper.c
   explicitly sets .enabled after calling pwm_init_state()

 - drivers/input/misc/pwm-vibra.c
   explicitly sets .enabled after calling pwm_init_state()

 - drivers/leds/leds-pwm.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/leds/rgb/leds-pwm-multicolor.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/media/rc/ir-rx51.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/media/rc/pwm-ir-tx.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/regulator/pwm-regulator.c
   never sets .enabled, probably a bug

 - drivers/video/backlight/lm3630a_bl.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/video/backlight/lp855x_bl.c
   explictily sets .enabled before calling pwm_apply_state()

 - drivers/video/backlight/pwm_bl.c
   This is the one we currently discuss. I think even with the patch
   applied it uses the .enabled value returned by pwm_init_state() but
   it shouldn't.

 - drivers/video/fbdev/ssd1307fb.c
   Similar to drivers/gpu/drm/solomon/ssd130x.c. Probably the one was
   copied to the other given that it seems to handle the same hardware.

So all consumers using pwm_init_state() either don't use the .enabled
value returned by pwm_init_state() or at least shouldn't do that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Fri, Oct 20, 2023 at 02:11:48PM +0200, Uwe Kleine-König wrote:
> Hello Daniel,
>
> On Fri, Oct 20, 2023 at 12:27:27PM +0100, Daniel Thompson wrote:
> > On Wed, Oct 18, 2023 at 11:07:41PM +0200, Uwe Kleine-König wrote:
> > > Hello Philipp,
> > >
> > > On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote:
> > > > The initial PWM state returned by pwm_init_state() has a duty cycle
> > > > of 0 ns.
> > >
> > > This is only true for drivers without a .get_state() callback, isn't it?
> >
> > pwm_init_state() explicitly zeros the duty-cycle in order to avoid
> > problems when the default args have a different period to the currently
> > applied config:
> > https://elixir.bootlin.com/linux/latest/source/include/linux/pwm.h#L174
>
> Ah right, pwm_init_state() is strange in a different way than I
> remembered :-) pwm_get_state() is only called to get .enabled set
> appropriately.
>
> Looking at the callers:
>
>  <snip>

>  - drivers/video/backlight/lm3630a_bl.c
>    explictily sets .enabled before calling pwm_apply_state()
>
>  - drivers/video/backlight/lp855x_bl.c
>    explictily sets .enabled before calling pwm_apply_state()
>
>  - drivers/video/backlight/pwm_bl.c
>    This is the one we currently discuss. I think even with the patch
>    applied it uses the .enabled value returned by pwm_init_state() but
>    it shouldn't.

Agreed.

> So all consumers using pwm_init_state() either don't use the .enabled
> value returned by pwm_init_state() or at least shouldn't do that.

Looking a little deeper in the PWM code, it looks to me like pwm_bl.c
could just use pwm_adjust_config() during probe to transition between
bootloader settings and kernel settings!


Daniel.

On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote:
> The initial PWM state returned by pwm_init_state() has a duty cycle
> of 0 ns. To avoid backlight flicker when taking over an enabled
> display from the bootloader, skip the initial pwm_apply_state()
> and leave the PWM be until backlight_update_state() will apply the
> state with the desired brightness.

backlight_update_state() uses pwm_get_state() to update the PWM.

Without applying something that came from pwm_init_state() then
we will never adopt the reference values from pwm->args.


Daniel.