[PATCH] amd/display: set backlight only if required

[Paul Menzel]

Dear Shirish,


Thank you for the patch.


Am 11.03.22 um 16:33 schrieb Shirish S:
> [Why]
> comparing pwm bl values (coverted) with user brightness(converted)

1.  co*n*verted
2.  Please add a space before the (.

> levels in commit_tail leads to continuous setting of backlight via dmub
> as they don't to match.

Maybe add a blank line between paragraphs.

> This leads overdrive in queuing of commands to DMCU that sometimes lead

What is “overdrive in queuing”?

lead*s*

> to depending on load on DMCU fw:

Leads to what? Words missing after *to*.

> "[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"

You could also add the example from your discussion with Harry.

> [How]
> Store last successfully set backlight value and compare with it instead
> of pwm reads which is not what we should compare with.

Maybe extend it with the information gotten from Harry, that this is 
expected, when ABM is enabled.


Kind regards,

Paul


> Signed-off-by: Shirish S <shirish.s at amd.com>
> ---
>   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:
> +	 *
> +	 * last successfully applied backlight values.
> +	 */
> +	u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
>   };
>   
>   enum dsc_clock_force_state {