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

[Nautiyal, Ankit K]

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 at intel.com>
>> Cc: Arun R Murthy <arun.r.murthy at intel.com>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: <stable at vger.kernel.org> # v6.10+
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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...

Hmm as per bspec:68925 for LNL : it mentions "This register should not 
be changed while VRR with adaptive sync is enabled on this transcoder."

This line is removed for PTL from which we are having 
always_use_vrr_tg()==true and replaced with: This register should not be 
changed while the refresh rate is changing.

Don't know if it will give issue when we switch from fixed RR case to 
VRR for PTL.



>
> 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.

Yeah this need to be looked into. Will check this.


>
> Also we still seem to be missing EMP_AS_SDP_TL completely.

I have some basic change in pipeline for this, will test and submit for 
review : 
https://patchwork.freedesktop.org/patch/647092/?series=147304&rev=1

For Panel Replay + VRR there seem to be few other things which need to 
be taken care for AS SDP.

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


Thanks Ville for the review.

Regards,

Ankit

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