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

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

[Ankit Nautiyal]

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;
 }
 
 static bool cmrr_params_changed(const struct intel_crtc_state *old_crtc_state,
-- 
2.45.2

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

[Ville Syrjälä]

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

Re: [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@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...

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

Re: [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@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