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

[Ville Syrjälä]

On Wed, Apr 10, 2024 at 07:41:42AM +0000, Hogander, Jouni wrote:
> On Wed, 2024-04-10 at 07:35 +0000, Manna, Animesh wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Hogander, Jouni <jouni.hogander at intel.com>
> > > Sent: Wednesday, April 10, 2024 12:54 PM
> > > To: Manna, Animesh <animesh.manna at intel.com>; intel-
> > > gfx at lists.freedesktop.org
> > > Cc: ville.syrjala at linux.intel.com; Murthy, Arun R
> > > <arun.r.murthy at intel.com>
> > > Subject: Re: [PATCH v4] drm/i915/panelreplay: Panel replay
> > > workaround with
> > > VRR
> > > 
> > > On Thu, 2024-03-28 at 10:13 +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]
> > > > 
> > > > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 17
> > > > +++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> > > >  drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
> > > >  3 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 00ac65a14029..7f5c42a14196 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -3840,6 +3840,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.
> > > > +        * 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; }
> > > > +
> > > 
> > > Do you have some specific reason to have this in intel_display.c?
> > > How about
> > > move it to intel_psr.c? You could also use more descriptive name.
> > > Current name somehow made me think it is some generic function to
> > > calculate vblank delay. It is actually only for this workaround.
> > 
> > Thanks for review.
> > As per feedback from rev3 I have added in intel_display.c. Going
> > forward all vbalnk related adjustment will be added into this
> > function.
> > https://patchwork.freedesktop.org/series/129632/#rev3
> > I can move to intel_psr.c if the current version is not acceptable.
> 
> Ok, thank you for providing the context. If it's intended use is
> generic then I think calling it shouldn't happen from
> intel_psr_compute_config. Maybe Ville can comment where it should be
> called from.

It's a potential chicken vs. egg situation.

The best place would be somewhere after .compute_config() since
we need this this for DSB in general. I think I have it in
intel_crtc_compute_config() in my current WIP DSB plane update
branch.

The problem with PSR is that it seems to want to use it
before that. The question no one has yet answered is
whether PSR actually wants to consult the undelayed
transcoder vblank or the delayed vblank. I'm think the
former is more likely (with PSR being a transcoder level
feature), in which case the chicken vs. egg sitation
will simply disappear. But someone will need to confirm
that.

The other potential chicken vs. egg looks to be VRR on
setups where TRANS_SET_CONTEXT_LATENCY isn't a thing.
The question there is how the vblank delay is configured
when using VRR. I'm pretty sure I did figure this out
already, but can't remember right now what the conclusion
was. So that needs to be double checked as well.

I was also pondering whether we should try to leave
adjusted_mode.crtc_vblank_start alone and instead just
reflect the delayed vblank in pipe_mode.crtc_vblank_start?
That might be useful if there are other places later on
in the atomic_check/etc. that needs to know the position
of the undelayed vblank. But that would have big implications
to eg. the vblank timestamping code, so probably not entirely
feasible.

-- 
Ville Syrjälä
Intel