Hello Jose,
Thank you for your comments. Please see my responses below and check
the new version I have sent.
On Tue, 2022-11-29 at 19:46 +0200, Jouni Högander wrote:
> On Tue, 2022-11-29 at 14:00 +0000, Souza, Jose wrote:
> > On Tue, 2022-11-29 at 09:51 +0200, Jouni Högander wrote:
> > > Currently we are observing occasionally display flickering or
> > > complete
> > > freeze. This is narrowed down to be caused by single full frame
> > > update
> > > (SFF).
> > >
> > > SFF bit after it's written gets cleared by HW in subsequent
> > > vblank
> > > i.e. when the update is sent to the panel. SFF bit is required to
> > > be
> > > written together with partial frame update (PFU) bit. After the
> > > SFF
> > > bit gets cleared by the HW psr2 man trk ctl register still
> > > contains
> > > PFU bit. If there is subsequent update for any reason we will end
> > > up
> > > having selective update/fetch configuration where start line is 0
> > > and
> > > end line is 0.
> > >
> >
> > How did you get this information(start and end line 0)?
>
> If you consider what is written into psr2 man trk ctl register in
> case
> of full frame update in intel_psr2_program_trans_man_trk_ctl and in
> _psr_flush_handle:
>
> SFF = 1
> PFU = 1
> Start line = 0
> End line = 0
>
> On next vblank SFF is cleared by the hw. After that we have:
>
> PFU = 1
> Start line = 0
> End line = 0
>
> which is basically selective update with start and endline as 0. If
> there is an update with this configration we are observing
> freeze/flicker. I can use CFF instead of SFF as a workaround. I also
> checked that configuring start and end lines as a full frame is also
> fixing the issue. I choose to come out with the first one.
>
> >
> > > Also selective fetch configuration for the planes is
> > > not properly performed. This seems to be causing problems with
> > > some
> > > panels.
> > >
> > > Using CFF without SFF doesn't work either because it may happen
> > > that
> > > psr2 man track ctl register is overwritten by next update before
> > > vblank triggers sending the update. This is causing problems to
> > > psr_invalidate/flush. Using CFF and SFF together solves the
> > > problems
> > > as SFF is cleared only by HW in subsequent vblank.
> >
> > This looks dangerous, have you checked with HW engineers if setting
> > both could cause any issue?
>
> Yes, this make sense, I will check this.
I got confirmation from HW folks:
"There are no restrictions on setting both bits simultaneously (HW
simply OR's the two bits together)."
>
> > At the SFF write you could get what is the current vblank counter
> > and
> > properly handle future PSR2_MAN_TRK_CTL writes.
>
> Is vblank counter updated with that granularity? E.g. if you have
> psr_invalidate/psr_flush sequence there is an update, but vblank
> interrupts are not enabled? In legacy cursor update there is also an
> update without vblank interrupts getting enabled. How vblank counter
> works when vblank interrupt is disabled?
>
I did experiments with vblank counter. It could be actually possible to
to use it, but to my opinion new strategy keeping CFF bit enabled
unless there is a selective update is much simpler. It solves this
problem, takes care of current flickering/freeze issue and also takes
care of HSD 14014971508.
> >
> > >
> > > Fix the flickering/freeze issue by adding continuous full frame
> > > with
> > > single full frame update and switch to partial frame update only
> > > when
> > > selective update area is properly calculated and configured.
> > >
> > > This is also workaround for HSD 14014971508
> >
> > Please use versions in your patches, you had 2 patches in the
> > previous approach with the same subject but no versioning and no
> > information about what
> > changed between versions.
>
> First one was sent to trybot and then to intel-gfx. In this one I
> changed the subject. So I was thinking it's ok to leave out
> versioning.
> I will add versioning when sending again.
>
> >
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > >
> > > Reported-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
> > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 19 ++++++++++--------
> > > -
> > > 1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 5b678916e6db..88388201684e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1510,7 +1510,8 @@ static void
> > > psr_force_hw_tracking_exit(struct
> > > intel_dp *intel_dp)
> > > PSR2_MAN_TRK_CTL(intel_dp-
> > > > psr.transcoder),
> > >
> > > man_trk_ctl_enable_bit_get(dev_priv)
> > > >
> > >
> > > man_trk_ctl_partial_frame_bit_get(dev_priv) |
> > > -
> > > man_trk_ctl_single_full_frame_bit_get(dev_priv));
> > > +
> > > man_trk_ctl_single_full_frame_bit_get(dev_priv) |
> > > +
> > > man_trk_ctl_continuos_full_frame(dev_priv));
> > >
> > > /*
> > > * Display WA #0884: skl+
> > > @@ -1624,11 +1625,8 @@ static void psr2_man_trk_ctl_calc(struct
> > > intel_crtc_state *crtc_state,
> > > val |= man_trk_ctl_partial_frame_bit_get(dev_priv);
> > >
> > > if (full_update) {
> > > - /*
> > > - * Not applying Wa_14014971508:adlp as we do not
> > > support the
> > > - * feature that requires this workaround.
> > > - */
> > > val |=
> > > man_trk_ctl_single_full_frame_bit_get(dev_priv);
> > > + val |=
> > > man_trk_ctl_continuos_full_frame(dev_priv);
> > > goto exit;
> > > }
> > >
> > > @@ -2307,12 +2305,15 @@ static void _psr_flush_handle(struct
> > > intel_dp *intel_dp)
> > > /* can we turn CFF off? */
> > > if (intel_dp->psr.busy_frontbuffer_bits
> > > ==
> > > 0) {
> > > u32 val =
> > > man_trk_ctl_enable_bit_get(dev_priv) |
> > > -
> > > man_trk_ctl_partial_frame_bit_get(dev_priv) |
> > > -
> > > man_trk_ctl_single_full_frame_bit_get(dev_priv);
> > > + man_trk_ctl_partial_frame
> > > _b
> > > it_get(dev_priv) |
> > > + man_trk_ctl_single_full_f
> > > ra
> > > me_bit_get(dev_priv) |
> > > + man_trk_ctl_continuos_ful
> > > l_
> > > frame(dev_priv);
> >
> > style.
> >
> > >
> > > /*
> > > - * turn continuous full frame off
> > > and do a single
> > > - * full frame
> > > + * turn continuous full frame off
> > > and do a single full frame. Still
> > > + * keep cff bit enabled as we
> > > don't
> > > have proper SU configuration in
> > > + * case update is sent for any
> > > reason after sff bit gets cleared by
> > > + * the HW on next vblank.
> >
> >
> > turn off and keep bit enabled?! makes no sense this comment.
I tried to make it more clear. Please check new version.
> >
> > > */
> > > intel_de_write(dev_priv,
> > > PSR2_MAN_TRK_CTL(intel_dp->psr.transcoder),
> > > val);
> >
>
