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

Thu Mar 28 05:46:03 UTC 2024

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Thursday, February 22, 2024 2:55 AM
> To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Cc: Manna, Animesh <animesh.manna at intel.com>; intel-
> gfx at lists.freedesktop.org; Hogander, Jouni <jouni.hogander at intel.com>;
> Murthy, Arun R <arun.r.murthy at intel.com>
> Subject: Re: [PATCH v3] drm/i915/panelreplay: Panel replay workaround with
> VRR
> 
> On Wed, Feb 21, 2024 at 11:08:18PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 21, 2024 at 03:58:48PM -0500, Rodrigo Vivi wrote:
> > > On Wed, Feb 21, 2024 at 08:19:35PM +0000, Manna, Animesh wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > > > Sent: Tuesday, February 20, 2024 11:12 PM
> > > > > To: Manna, Animesh <animesh.manna at intel.com>
> > > > > Cc: intel-gfx at lists.freedesktop.org;
> > > > > ville.syrjala at linux.intel.com; Hogander, Jouni
> > > > > <jouni.hogander at intel.com>; Murthy, Arun R
> > > > > <arun.r.murthy at 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 at 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?
> >
> > I think what we need is a intel_crtc_vblank_delay() or somesuch thing
> > that accounts for all the things (eg. this w/a, dsb execution latency
> > when we start to use dsb for double buffered registers, etc).
> > And it should probably be called from some central place so that it
> > works for all output types. intel_crtc_compute_config() comes to mind,
> > but I guess we need to first ponder whether there are bits of code
> > being executed prior to that that would need to know the actual
> > vblank_start...
> 
> PSR2 itself might have a chicken vs. egg issue here. Wehether we can enable
> it not depends on the length of the vblank. Whether that means the full
> vblank or the pipe's shrunken vblank I don't know. Someone needs to figure
> that out.

Added a centralized intel_crtc_vblank_delay() in v4 but as this WA is specific for panel replay which will be supported by DP/EDP
so called from encoder-compute-config() instead of crtc-compute-config. 

Regards,
Animesh
> 
> --
> Ville Syrjälä
> Intel