Re: [PATCH v7] drm/i915/panelreplay: Panel replay workaround with VRR

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

On Fri, Jun 21, 2024 at 05:55:13AM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Thursday, June 20, 2024 11:06 PM
> > To: Manna, Animesh <animesh.manna@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> > Hogander, Jouni <jouni.hogander@intel.com>; Murthy, Arun R
> > <arun.r.murthy@intel.com>; Golani, Mitulkumar Ajitkumar
> > <mitulkumar.ajitkumar.golani@intel.com>
> > Subject: Re: [PATCH v7] drm/i915/panelreplay: Panel replay workaround with
> > VRR
> > 
> > On Wed, Jun 19, 2024 at 04:01:01PM +0300, Ville Syrjälä wrote:
> > > On Tue, Jun 18, 2024 at 04:52:15PM +0530, Animesh Manna wrote:
> > > > Panel Replay VSC SDP not getting sent when VRR is enabled and W1 and
> > > > W2 are 0. So Program Set Context Latency in
> > > > TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
> > > >
> > > > HSD: 14015406119
> > > >
> > > > v1: Initial version.
> > > > v2: Update timings stored in adjusted_mode struct. [Ville]
> > > > v3: Add WA in compute_config(). [Ville]
> > > > v4:
> > > > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > > > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> > > > v5: Move to crtc_compute_config(). [Ville]
> > > > v6: Restrict DISPLAY_VER till 14. [Mitul]
> > > > v7:
> > > > - Corrected code-comment. [Mitul]
> > > > - dev_priv local variable removed. [Jani]
> > > >
> > > > Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 21
> > > > ++++++++++++++++++++  drivers/gpu/drm/i915/display/intel_display.h |
> > > > 1 +
> > > >  2 files changed, 22 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 7bc4f3de691e..c3ff3a5c5fa3 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -2515,6 +2515,10 @@ static int intel_crtc_compute_config(struct
> > intel_atomic_state *state,
> > > >  		intel_atomic_get_new_crtc_state(state, crtc);
> > > >  	int ret;
> > > >
> > > > +	/* wa_14015401596: display versions 13, 14 */
> > > > +	if (IS_DISPLAY_VER(to_i915(crtc->base.dev), 13, 14))
> > > > +		intel_crtc_vblank_delay(crtc_state);
> > > > +
> > > >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> > > >  	if (ret)
> > > >  		return ret;
> > > > @@ -3924,6 +3928,23 @@ bool intel_crtc_get_pipe_config(struct
> > intel_crtc_state *crtc_state)
> > > >  	return true;
> > > >  }
> > > >
> > > > +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state) {
> > > > +	struct drm_display_mode *adjusted_mode =
> > > > +&crtc_state->hw.adjusted_mode;
> > > > +
> > > > +	/*
> > > > +	 * wa_14015401596 for display versions 13, 14.
> > > > +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> > register
> > > > +	 * to at least a value of 1 when Panel Replay is enabled with VRR.
> > > > +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> > substracting
> > > > +	 * crtc_vdisplay from crtc_vblank_start, so incrementing
> > crtc_vblank_start
> > > > +	 * by 1 if both are equal.
> > > > +	 */
> > > > +	if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
> > > > +	    adjusted_mode->crtc_vblank_start == adjusted_mode-
> > >crtc_vdisplay)
> > > > +		adjusted_mode->crtc_vblank_start += 1; }
> > >
> > > This is probably too late actually. We already used the previous value
> > > to calculate the VRR guardband/pipeline full values, which may or may
> > > not now be incorrect. So NAK for now until someone actually checks how
> > > it all works (I don't recall the details right now).
> > 
> > I double checked this and the guardband/pipeline full values do indeed need
> > to be calculated based on the delayed vblank. So unfortunately this needs to
> > be done before VRR computation, which is a bit annoying if we'd need to
> > tweak this also for HDMI or DSI.
> > But for now we shouldn't actually need other adjustements as I'm going to
> > be doing the DSB stuff without relying on delayed vblank.
> 
> Sure, I will add a change for recalculating guardband.
> No need to change pipeline full value as this workaround is for display version >= 13.

I don't want to see any platform specific hacks for this.
The code must do the right thing on any platform if the
vblank delay gets changed.

> Currently this workaround is only for panel replay, so HDMI and DSI is out of scope.
> As I understood, DSB stuff will be taken care separately, is it ok if I move the adjustment in encoder-compute-config, more specifically in psr-compute-config where we will know about vrr and panel replay is enabled or not and recalculate the guardband ?

Recalculating stuff twice is not great. I suppose we'll just
have to split the VRR computation into two phases, with the
second phase calculating the guardband/pipeline full values.
I guess that could even get called from intel_crtc_compute_config()
so we could keep the vblank delay adjustement there as well.

> 
> Regards,
> Animesh
> 
> > 
> > >
> > > > +
> > > >  int intel_dotclock_calculate(int link_freq,
> > > >  			     const struct intel_link_m_n *m_n)  { diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_display.h
> > > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > > index b0cf6ca70952..f99a24e76608 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > > @@ -428,6 +428,7 @@ bool intel_crtc_is_joiner_primary(const struct
> > > > intel_crtc_state *crtc_state);
> > > >  u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state
> > > > *crtc_state);  struct intel_crtc *intel_primary_crtc(const struct
> > > > intel_crtc_state *crtc_state);  bool
> > > > intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> > > > +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state);
> > > >  bool intel_pipe_config_compare(const struct intel_crtc_state
> > *current_config,
> > > >  			       const struct intel_crtc_state *pipe_config,
> > > >  			       bool fastset);
> > > > --
> > > > 2.29.0
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel