Re: [PATCH] backlight: pwm_bl: configure pwm only once per backlight toggle

["Michal Vokáč" <michal.vokac@ysoft.com>]

On 17. 10. 19 10:10, Uwe Kleine-König wrote:
> A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
> pwm_get_state() return the last implemented state")) changed the
> semantic of pwm_get_state() and disclosed an (as it seems) common
> problem in lowlevel PWM drivers. By not relying on the period and duty
> cycle being retrievable from a disabled PWM this type of problem is
> worked around.
> 
> Apart from this issue only calling the pwm_get_state/pwm_apply_state
> combo once is also more effective.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> There are now two reports about 01ccf903edd6 breaking a backlight. As
> far as I understand the problem this is a combination of the backend pwm
> driver yielding surprising results and the pwm-bl driver doing things
> more complicated than necessary.
> 
> So I guess this patch works around these problems. Still it would be
> interesting to find out the details in the imx driver that triggers the
> problem. So Adam, can you please instrument the pwm-imx27 driver to
> print *state at the beginning of pwm_imx27_apply() and the end of
> pwm_imx27_get_state() and provide the results?
> 
> Note I only compile tested this change.

Hi Uwe,
I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+"
thread that I have a similar problem when you submitted this patch.

So here are my few cents:

My setup is as follows:
  - imx6dl-yapp4-draco with i.MX6Solo
  - backlight is controlled with inverted PWM signal
  - max brightness level = 32, default brightness level set to 32 in DT.

1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let
    pwm_get_state() return the last implemented state):

  - System boots to userspace and backlight is enabled all the time from
    power up.

    $ dmesg | grep state
    [    1.763381] get state end: -1811360608, enabled: 0
    [    4.939658] apply state start: -1797997308, enabled: 0
    [    4.946437] apply state start: -1797997436, enabled: 0
    [    4.952936] apply state start: -1797997420, enabled: 1

  - $ cat brightness
    32

  - $ echo 32 > brightness # nothing happens, max. brightness

  - $ echo 1 > brightness # backlight goes down to lowest level
    [   59.876561] apply state start: -1711276636, enabled: 1

  - $ echo 0 > brightness # backlight goes up to max. level, this is
                          # problem of the inverted PWM on i.MX we attempted
                          # to solve some time ago.
    [  102.456582] apply state start: -1711276700, enabled: 0
   
2. Backlight behavior on v5.4-rc3:

  - System boots to userspace and backlight is enabled all the time from
    power up.

  - $ dmesg | grep state
    [    1.773056] get state end: -1811319520, enabled: 0
    [    4.994003] apply state start: -1797948160, enabled: 0
    [    5.000617] get state end: -1811319520, enabled: 0
    [    5.006950] apply state start: -1797948292, enabled: 0
    [    5.013446] get state end: -1811319520, enabled: 0
    [    5.019602] apply state start: -1797948276, enabled: 1
    [    5.027536] get state end: -1811319520, enabled: 1

  - $ cat brightness
    32

  - $ echo 32 > brightness # backlight goes down
    [  189.956756] apply state start: -1711923804, enabled: 1
    [  189.965907] get state end: -1811319520, enabled: 1

  - $ echo 1 > brightness # backlight goes up to high level
    [  464.136633] apply state start: -1711923804, enabled: 1
    [  464.145625] get state end: -1811319520, enabled: 1

  - $ echo 0 > brightness # backlight goes up to highest level
    [  527.136640] apply state start: -1711923868, enabled: 0
    [  527.145538] get state end: -1811319520, enabled: 0

So the behavior is clearly inverted to how it worked prior to 01ccf903edd6
with the weird exception that the initial brightness level 32 is
not applied.

3. Backlight behavior on v5.4-rc3 + this patch:

  - System boots with backlight enabled. In the middle of kernel boot
    backlight is disabled.

  - $ dmesg | grep state
    [    1.773233] get state end: -1811319520, enabled: 0
    [    5.002753] apply state start: -1797948160, enabled: 0
    [    5.009397] get state end: -1811319520, enabled: 0
    [    5.015652] apply state start: -1797948276, enabled: 1
    [    5.026682] get state end: -1811319520, enabled: 1

  - $ cat brightness
    32

  - $ echo 32 > brightness # nothing happens, backlight is down

  - $ echo 1 > brightness # backlight goes to high level
    [  305.377274] apply state start: -1711915596, enabled: 1
    [  305.386374] get state end: -1811319520, enabled: 1

  - $ echo 0 > brightness # backlight goes to max brightness
    [  362.637297] apply state start: -1711915596, enabled: 0
    [  362.646325] get state end: -1811319520, enabled: 0

Same behavior as (2) but the default state from DT is apparently applied.

I only did this experiments. I did not delve into the code to track what is
going on in there yet.

Hopefully this helps you a bit and feel free to request other experiments,
Michal

>   drivers/video/backlight/pwm_bl.c | 34 +++++++++++---------------------
>   1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 746eebc411df..ddebd62b3978 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -42,10 +42,8 @@ struct pwm_bl_data {
>   
>   static void pwm_backlight_power_on(struct pwm_bl_data *pb)
>   {
> -	struct pwm_state state;
>   	int err;
>   
> -	pwm_get_state(pb->pwm, &state);
>   	if (pb->enabled)
>   		return;
>   
> @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
>   	if (err < 0)
>   		dev_err(pb->dev, "failed to enable power supply\n");
>   
> -	state.enabled = true;
> -	pwm_apply_state(pb->pwm, &state);
> -
>   	if (pb->post_pwm_on_delay)
>   		msleep(pb->post_pwm_on_delay);
>   
> @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
>   
>   static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>   {
> -	struct pwm_state state;
> -
> -	pwm_get_state(pb->pwm, &state);
> -	if (!pb->enabled)
> -		return;
> -
>   	if (pb->enable_gpio)
>   		gpiod_set_value_cansleep(pb->enable_gpio, 0);
>   
>   	if (pb->pwm_off_delay)
>   		msleep(pb->pwm_off_delay);
>   
> -	state.enabled = false;
> -	state.duty_cycle = 0;
> -	pwm_apply_state(pb->pwm, &state);
> -
>   	regulator_disable(pb->power_supply);
>   	pb->enabled = false;
>   }
>   
> -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
> +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state)
>   {
>   	unsigned int lth = pb->lth_brightness;
> -	struct pwm_state state;
>   	u64 duty_cycle;
>   
> -	pwm_get_state(pb->pwm, &state);
> -
>   	if (pb->levels)
>   		duty_cycle = pb->levels[brightness];
>   	else
>   		duty_cycle = brightness;
>   
> -	duty_cycle *= state.period - lth;
> +	duty_cycle *= state->period - lth;
>   	do_div(duty_cycle, pb->scale);
>   
>   	return duty_cycle + lth;
> @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>   
>   	if (brightness > 0) {
>   		pwm_get_state(pb->pwm, &state);
> -		state.duty_cycle = compute_duty_cycle(pb, brightness);
> +		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
> +		state.enabled = true;
>   		pwm_apply_state(pb->pwm, &state);
> +
>   		pwm_backlight_power_on(pb);
> -	} else
> +	} else {
>   		pwm_backlight_power_off(pb);
>   
> +		pwm_get_state(pb->pwm, &state);
> +		state.enabled = false;
> +		state.duty_cycle = 0;
> +		pwm_apply_state(pb->pwm, &state);
> +	}
> +
>   	if (pb->notify_after)
>   		pb->notify_after(pb->dev, brightness);
>   
>