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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 4/4/2025 9:08 PM, Ville Syrjälä wrote:
> 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>

Thanks for the review, the patch was pushed to drm-intel-next, missed to 
update earlier.


Regards,

Ankit

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