[RFC PATCH RESEND] drm/amd/display: Add back missing backlight level rounding

[RFC PATCH RESEND] drm/amd/display: Add back missing backlight level rounding

[Lukáš Krejčí]

[Why]
Having the rounding of the backlight value restored to the way it was
seemingly gets rid of backlight flickering on certain Stoney Ridge
laptops.

[How]
Rescale the backlight level between min and max input signal value and
round it to a number between 0x0 and 0xFF. Then, use the rounding mode
that was previously in driver_set_backlight_level() and
dmcu_set_backlight_level(), i.e. rescale the backlight level between 0x0
and 0x1000 by multiplying it by 0x101 and use the most significant bit
of the fraction (or in this case the 8th bit of the value because it's
the same thing, e.g. C3 * 0x101 = 0xC3C3 and C3 * 0x10101 = 0xC3C3C3) to
round it.

Fixes: 262485a50fd4 ("drm/amd/display: Expand dc to use 16.16 bit backlight")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204957
Signed-off-by: Lukáš Krejčí <lskrejci@gmail.com>
---

Notes:
    Bug:
    - Can be reproduced on HP 15-rb020nc (Stoney Ridge R2 E2-9000e APU) and
      Acer Aspire 3 A315-21G-91JL (Stoney Ridge R3 A4-9120 APU).
    
    - For me, the bug is inconsistent - it does not happen on every boot,
      but if it does happen, it's usually within three minutes after bootup.
    
    - It concerns the backlight. That means it can be reproduced on the
      framebuffer console.
    
    - Reproduces on Arch Linux (custom built) live CD, linux kernel v5.3.2
      and Ubuntu 19.04, kernel-ppa/mainline v5.0.0-rc1, v5.0.0, v5.3.2, v5.3.4,
      v5.4-rc2.
    
    - Can not be reproduced on kernel v5.3.4 with this patch applied or on
      v4.20.0, 4.20.17, 4.19.75 (this bug is a regression).
    
    - The other person that reproduced the issue (see the Bugzilla link
      above) confirmed that the patch works for them too.
    
    Patch:
    - Is the comment modified by this commit correct? Both
      driver_set_backlight_level() and dmcu_set_backlight_level() check the
      17th bit of `brightness` aka `backlight_pwm_u16_16`, but
      262485a50fd4532a8d71165190adc7a0a19bcc9e ("drm/amd/display: Expand dc
      to use 16.16 bit backlight") specifically mentions 0xFFFF as the max
      backlight value.
    
    - use_smooth_brightness is false (no DMCU firmware available) on my
      laptop, so the other code path (dmcu_set_backlight_level()) is
      untested.
    
    - I'm not sure why the rounding fixes the issue and whether this
      function is the right place to add back the rounding (and whether it
      even is the right way to solve the issue), so that's why this patch is
      RFC.

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a52f0b13a2c8..af9a5f46b671 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2083,17 +2083,22 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
 	 * The brightness input is in the range 0-255
 	 * It needs to be rescaled to be between the
 	 * requested min and max input signal
-	 *
-	 * It also needs to be scaled up by 0x101 to
-	 * match the DC interface which has a range of
-	 * 0 to 0xffff
 	 */
 	brightness =
 		brightness
-		* 0x101
+		* 0x100
 		* (caps.max_input_signal - caps.min_input_signal)
 		/ AMDGPU_MAX_BL_LEVEL
-		+ caps.min_input_signal * 0x101;
+		+ caps.min_input_signal * 0x100;
+
+	brightness = (brightness >> 8) + ((brightness >> 7) & 1);
+	/*
+	 * It also needs to be scaled up by 0x101 and
+	 * rounded off to match the DC interface which
+	 * has a range of 0 to 0x10000
+	 */
+	brightness *= 0x101;
+	brightness += (brightness >> 7) & 1;
 
 	if (dc_link_set_backlight_level(dm->backlight_link,
 			brightness, 0))
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH RESEND] drm/amd/display: Add back missing backlight level rounding

[Li, Sun peng (Leo)]

Thanks for the detailed notes! See reply inline.

On 2019-10-15 4:03 p.m., Lukáš Krejčí wrote:
> [Why]
> Having the rounding of the backlight value restored to the way it was
> seemingly gets rid of backlight flickering on certain Stoney Ridge
> laptops.
> 
> [How]
> Rescale the backlight level between min and max input signal value and
> round it to a number between 0x0 and 0xFF. Then, use the rounding mode
> that was previously in driver_set_backlight_level() and
> dmcu_set_backlight_level(), i.e. rescale the backlight level between 0x0
> and 0x1000 by multiplying it by 0x101 and use the most significant bit
> of the fraction (or in this case the 8th bit of the value because it's
> the same thing, e.g. C3 * 0x101 = 0xC3C3 and C3 * 0x10101 = 0xC3C3C3) to
> round it.
> 
> Fixes: 262485a50fd4 ("drm/amd/display: Expand dc to use 16.16 bit backlight")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204957
> Signed-off-by: Lukáš Krejčí <lskrejci@gmail.com>
> ---
> 
> Notes:
>     Bug:
>     - Can be reproduced on HP 15-rb020nc (Stoney Ridge R2 E2-9000e APU) and
>       Acer Aspire 3 A315-21G-91JL (Stoney Ridge R3 A4-9120 APU).
>     
>     - For me, the bug is inconsistent - it does not happen on every boot,
>       but if it does happen, it's usually within three minutes after bootup.
>     
>     - It concerns the backlight. That means it can be reproduced on the
>       framebuffer console.
>     
>     - Reproduces on Arch Linux (custom built) live CD, linux kernel v5.3.2
>       and Ubuntu 19.04, kernel-ppa/mainline v5.0.0-rc1, v5.0.0, v5.3.2, v5.3.4,
>       v5.4-rc2.
>     
>     - Can not be reproduced on kernel v5.3.4 with this patch applied or on
>       v4.20.0, 4.20.17, 4.19.75 (this bug is a regression).
>     
>     - The other person that reproduced the issue (see the Bugzilla link
>       above) confirmed that the patch works for them too.
>     
>     Patch:
>     - Is the comment modified by this commit correct? Both
>       driver_set_backlight_level() and dmcu_set_backlight_level() check the
>       17th bit of `brightness` aka `backlight_pwm_u16_16`, but
>       262485a50fd4532a8d71165190adc7a0a19bcc9e ("drm/amd/display: Expand dc
>       to use 16.16 bit backlight") specifically mentions 0xFFFF as the max
>       backlight value>     
>     - use_smooth_brightness is false (no DMCU firmware available) on my
>       laptop, so the other code path (dmcu_set_backlight_level()) is
>       untested.
>     
>     - I'm not sure why the rounding fixes the issue and whether this
>       function is the right place to add back the rounding (and whether it
>       even is the right way to solve the issue), so that's why this patch is
>       RFC.

We've seen some similar issues when fractional duty cycles are enabled for backlight pwm.
I attached a hack to the bugzilla ticket that disables it, please give that patch a shot
first. I'd rather not change this calculation for all panels if the flickering issue is
only a quirk for some.

Thanks,
Leo

> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index a52f0b13a2c8..af9a5f46b671 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2083,17 +2083,22 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
>  	 * The brightness input is in the range 0-255
>  	 * It needs to be rescaled to be between the
>  	 * requested min and max input signal
> -	 *
> -	 * It also needs to be scaled up by 0x101 to
> -	 * match the DC interface which has a range of
> -	 * 0 to 0xffff
>  	 */
>  	brightness =
>  		brightness
> -		* 0x101
> +		* 0x100
>  		* (caps.max_input_signal - caps.min_input_signal)
>  		/ AMDGPU_MAX_BL_LEVEL
> -		+ caps.min_input_signal * 0x101;
> +		+ caps.min_input_signal * 0x100;
> +
> +	brightness = (brightness >> 8) + ((brightness >> 7) & 1);
> +	/*
> +	 * It also needs to be scaled up by 0x101 and
> +	 * rounded off to match the DC interface which
> +	 * has a range of 0 to 0x10000
> +	 */
> +	brightness *= 0x101;
> +	brightness += (brightness >> 7) & 1;
>  
>  	if (dc_link_set_backlight_level(dm->backlight_link,
>  			brightness, 0))
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel