[PATCH 05/14] drm/amd/display: Defer BW-optimization-blocked DRR adjustments

[Pillai, Aurabindo]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi John,

Sure, the debug print is not really necessary. We'll update it before merging.

--

Thanks & Regards,
Aurabindo Pillai
________________________________
From: John Olender <john.olender at gmail.com>
Sent: Thursday, May 8, 2025 5:51 AM
To: Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; Wu, Ray <Ray.Wu at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>
Cc: Wentland, Harry <Harry.Wentland at amd.com>; Li, Roman <Roman.Li at amd.com>; Lin, Wayne <Wayne.Lin at amd.com>; Chung, ChiaHsuan (Tom) <ChiaHsuan.Chung at amd.com>; Zuo, Jerry <Jerry.Zuo at amd.com>; Mohamed, Zaeem <Zaeem.Mohamed at amd.com>; Wheeler, Daniel <Daniel.Wheeler at amd.com>; Hung, Alex <Alex.Hung at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Subject: Re: [PATCH 05/14] drm/amd/display: Defer BW-optimization-blocked DRR adjustments

On 5/6/25 10:34 PM, Ray Wu wrote:
> From: John Olender <john.olender at gmail.com>
>
> [Why & How]
> Instead of dropping DRR updates, defer them. This fixes issues where
> monitor continues to see incorrect refresh rate after VRR was turned off
> by userspace.
>
> Fixes: 32953485c558 ("drm/amd/display: Do not update DRR while BW optimizations pending")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3546
>
> Reviewed-by: Sun peng Li <sunpeng.li at amd.com>
> Signed-off-by: John Olender <john.olender at gmail.com>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
> Signed-off-by: Ray Wu <ray.wu at amd.com>

Thank you for reviewing and revising the original patch.  This commit
message is a clear improvement over the original.

I do have a concern about the debug print clean up though.  Please see
below.

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/amd/display/dc/core/dc.c          | 13 ++++++++++---
>  2 files changed, 12 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 36c16030fca9..5a38748703b3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -374,6 +374,8 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,
>  static inline bool is_dc_timing_adjust_needed(struct dm_crtc_state *old_state,
>                                              struct dm_crtc_state *new_state)
>  {
> +     if (new_state->stream->adjust.timing_adjust_pending)
> +             return true;
>        if (new_state->freesync_config.state ==  VRR_STATE_ACTIVE_FIXED)
>                return true;
>        else if (amdgpu_dm_crtc_vrr_active(old_state) != amdgpu_dm_crtc_vrr_active(new_state))
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 528e6fd546c5..6ec22c0d5b97 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -441,9 +441,15 @@ bool dc_stream_adjust_vmin_vmax(struct dc *dc,
>         * Don't adjust DRR while there's bandwidth optimizations pending to
>         * avoid conflicting with firmware updates.
>         */
> -     if (dc->ctx->dce_version > DCE_VERSION_MAX)
> -             if (dc->optimized_required || dc->wm_optimized_required)
> +     if (dc->ctx->dce_version > DCE_VERSION_MAX) {
> +             if (dc->optimized_required || dc->wm_optimized_required) {
> +                     if (!stream->adjust.timing_adjust_pending) {
> +                             stream->adjust.timing_adjust_pending = true;
> +                             DC_LOG_DEBUG("%s: deferring DRR update\n", __func__);
> +                     }
>                        return false;
> +             }
> +     }

Printing the start of a string of blocked updates without also printing
the end can result in misleading debug messages.

The original patch got around this by spamming out a debug print for
every blocked update.  Not ideal, but it let me to keep the focus of the
patch on the fix itself.

If the spam from the original patch is too much and adding an end
message is too messy, please consider removing the debug message entirely.

Thanks,
John

>
>        dc_exit_ips_for_hw_access(dc);
>
> @@ -3241,7 +3247,8 @@ static void copy_stream_update_to_stream(struct dc *dc,
>
>        if (update->crtc_timing_adjust) {
>                if (stream->adjust.v_total_min != update->crtc_timing_adjust->v_total_min ||
> -                     stream->adjust.v_total_max != update->crtc_timing_adjust->v_total_max)
> +                     stream->adjust.v_total_max != update->crtc_timing_adjust->v_total_max ||
> +                     stream->adjust.timing_adjust_pending)
>                        update->crtc_timing_adjust->timing_adjust_pending = true;
>                stream->adjust = *update->crtc_timing_adjust;
>                update->crtc_timing_adjust->timing_adjust_pending = false;

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250508/9f3ae243/attachment.htm>