On 3/11/22 10:33, Shirish S wrote:
> [Why]
> comparing pwm bl values (coverted) with user brightness(converted)
> levels in commit_tail leads to continuous setting of backlight via dmub
> as they don't to match.
Why do the values not match? It looks like the value mismatch
is our root cause. I remember a while back looking at an issue
where we the readback was from DMCU while we were setting BL
directly via PWM. I wonder if the opposite is happening now.
See this for the previous fix:
2bf3d62dabcc drm/amd/display: Get backlight from PWM if DMCU is not initialized
> This leads overdrive in queuing of commands to DMCU that sometimes lead
> to depending on load on DMCU fw:
>
> "[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
>
> [How]
> Store last successfully set backlight value and compare with it instead
> of pwm reads which is not what we should compare with.
>
Does BL work reliably after S3 or S4 with your change? I wonder if
there are use-cases that might break because we're no longer comparing
against the actual BL value but against a stored variable.
> Signed-off-by: Shirish S <shirish.s@xxxxxxx>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++++---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++++++
> 2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *cap
> max - min);
> }
>
> -static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> +static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> int bl_idx,
> u32 user_brightness)
> {
> @@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> DRM_DEBUG("DM: Failed to update backlight on eDP[%d]\n", bl_idx);
> }
>
> - return rc ? 0 : 1;
> + if (rc)
> + dm->actual_brightness[bl_idx] = user_brightness;
> }
>
> static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
> @@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> /* restore the backlight level */
> for (i = 0; i < dm->num_of_edps; i++) {
> if (dm->backlight_dev[i] &&
> - (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
> + (dm->actual_brightness[i] != dm->brightness[i]))
> amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> }
> #endif
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 372f9adf091a..321279bc877b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -540,6 +540,12 @@ struct amdgpu_display_manager {
> * cached backlight values.
> */
> u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
> + /**
> + * @actual_brightness:
"actual" seems misleading here. We might want to call this
"last" or something along those lines.
But let's first see if we can fix the mismatch of BL reads
and writes.
Harry
> + *
> + * last successfully applied backlight values.
> + */
> + u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
> };
>
> enum dsc_clock_force_state {
