[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 <[email protected]> Sent: Thursday, May 8, 2025 5:51 AM To: Pillai, Aurabindo <[email protected]>; Wu, Ray <[email protected]>; Li, Sun peng (Leo) <[email protected]> Cc: Wentland, Harry <[email protected]>; Li, Roman <[email protected]>; Lin, Wayne <[email protected]>; Chung, ChiaHsuan (Tom) <[email protected]>; Zuo, Jerry <[email protected]>; Mohamed, Zaeem <[email protected]>; Wheeler, Daniel <[email protected]>; Hung, Alex <[email protected]>; [email protected] <[email protected]> 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 <[email protected]> > > [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 <[email protected]> > Signed-off-by: John Olender <[email protected]> > Signed-off-by: Aurabindo Pillai <[email protected]> > Signed-off-by: Ray Wu <[email protected]> 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;
