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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Feb 21, 2024 at 08:19:35PM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Tuesday, February 20, 2024 11:12 PM
> > To: Manna, Animesh <animesh.manna@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Hogander,
> > Jouni <jouni.hogander@intel.com>; Murthy, Arun R
> > <arun.r.murthy@intel.com>
> > Subject: Re: [PATCH v3] drm/i915/panelreplay: Panel replay workaround with
> > VRR
> > 
> > On Tue, Feb 20, 2024 at 07:49:19PM +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
> > 
> > Unnecessary mark since the wa_name already is a pointer to the HSD.
> > 
> > >
> > > v1: Initial version.
> > > v2: Update timings stored in adjusted_mode struct. [Ville]
> > > v3: Add WA in compute_config(). [Ville]
> > >
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 217196196e50..eb0fa513cd0f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2948,6 +2948,18 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> > >  	intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
> > >  	intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp,
> > pipe_config,
> > > conn_state);
> > >
> > > +	/*
> > > +	 * WA: HSD-14015406119
> > 
> > this is not the right one. You should use the lineage one and then mark the
> > platforms.
> > 
> > /* wa_14015401596: xe_lpd, xe_hpd */
> > 
> > or perhaps
> > 
> > /* wa_14015401596: display versions: 13, 14 */
> > 
> > and also add a check for the display version with it.
> 
> Sure.
> 
> > 
> > > +	 * 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 (pipe_config->vrr.enable && pipe_config->has_panel_replay &&
> > > +	    adjusted_mode->crtc_vblank_start == adjusted_mode-
> > >crtc_vdisplay)
> > > +		adjusted_mode->crtc_vblank_start += 1;
> > 
> > why to mess with the vblank start instead of going to
> > intel_set_transcoder_timings() and change directly what is getting written to
> > the register when the register gets written?
> 
> I have done in previous version of this patch. But as per review feedback, added now here.
> https://patchwork.freedesktop.org/series/129632/#rev1
> https://patchwork.freedesktop.org/series/129632/#rev2
>  
> > 
> > In case the answer is becasue by then we don't have the vrr.enable or
> > something like that, then we should consider move around when we set that
> > register?
> 
> This was not acceptable in earlier versions. As per feedback instead of atomic-commit need to add in compute-config phase.  
> 
> > 
> > or perhaps create a specific flag? one extra variable, 3 less comment lines...
> 
> The above comment is not clear to me, can you please elaborate more here.

something like:

@intel_dp_compute_config()

+if (pipe_config->vrr.enable && pipe_config->has_panel_replay &&
+	adjusted_mode->crtc_vblank_start == adjusted_mode-crtc_vdisplay)
+	pipe_config->mode_flags = I915_MODE_FLAG_MIN_TRANS_CONTEXT_LATENCY_1

then
@intel_set_transcoder_timings()
+u32 context_latency;

+if (crtc_state->mode_flags & I915_MODE_FLAG_MIN_TRANS_CONTEXT_LATENCY_1)
+   context_latency = 1;
+else
+   crtc_vblank_start - crtc_vdisplay;

-crtc_vblank_start - crtc_vdisplay);
+context_latency);

Ville, thoughts?

> 
> Regards,
> Animesh
> 
> > 
> > > +
> > >  	return 0;
> > >  }
> > >
> > > --
> > > 2.29.0
> > >