| Message ID | 20230608-backlight-pwm-avoid-flicker-v1-1-afd380d50174@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | backlight: pwm_bl: Avoid backlight flicker applying initial PWM state | expand |
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.
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
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
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.
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);
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(-) --- base-commit: ac9a78681b921877518763ba0e89202254349d1b change-id: 20230608-backlight-pwm-avoid-flicker-d2dd8d12f826 Best regards,