| Message ID | 20250404080540.2059511-1-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/vrr: Add vrr.vsync_{start, end} in vrr_params_changed | expand |
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
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
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
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,
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(-)