Re: [PATCH] drm/i915/vrr: Add vrr.vsync_{start,end} in vrr_params_changed

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Apr 04, 2025 at 01:35:40PM +0530, Ankit Nautiyal wrote:
> Add the missing vrr parameters in vrr_params_changed() helper.
> This ensures that changes in vrr.vsync_{start,end} trigger a call to
> appropriate helpers to update the VRR registers.
> 
> Fixes: e8cd188e91bb ("drm/i915/display: Compute vrr_vsync params")
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v6.10+
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c540e2cae1f0..ced8ba0f8d6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -969,7 +969,9 @@ static bool vrr_params_changed(const struct intel_crtc_state *old_crtc_state,
>  		old_crtc_state->vrr.vmin != new_crtc_state->vrr.vmin ||
>  		old_crtc_state->vrr.vmax != new_crtc_state->vrr.vmax ||
>  		old_crtc_state->vrr.guardband != new_crtc_state->vrr.guardband ||
> -		old_crtc_state->vrr.pipeline_full != new_crtc_state->vrr.pipeline_full;
> +		old_crtc_state->vrr.pipeline_full != new_crtc_state->vrr.pipeline_full ||
> +		old_crtc_state->vrr.vsync_start != new_crtc_state->vrr.vsync_start ||
> +		old_crtc_state->vrr.vsync_end != new_crtc_state->vrr.vsync_end;

These seem to yet another set of values that are potentially problematic
for the always_use_vrr_tg()==true case. I'm not sure how careful we
should be with this stuff...

And the whole encoder->update_pipe()/infoframe fastset stuff needs to
be rewritten to make sure the all the changes it does are either atomic
or happen on the correct side of the actual commit. Right now, for AS
SDP specifically, we enable/disable the infoframe potentially before
the actual commit, which I think is wrong at least for the disable case.

Also we still seem to be missing EMP_AS_SDP_TL completely.

Anyways, this patch isn't wrong at least so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static bool cmrr_params_changed(const struct intel_crtc_state *old_crtc_state,
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel