Re: [PATCH] drm/i915/psr: WA for panels stating bad link status after PSR is enabled

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Mon, 2024-10-28 at 14:24 +0200, Jani Nikula wrote:
> On Mon, 28 Oct 2024, Jouni Högander <jouni.hogander@intel.com> wrote:
> > We are currently seeing unexpected link trainings with several
> > different
> > eDP panels. These are caused by these panels stating bad link
> > status in
> > their dpcd registers. This can be observed by doing following test:
> > 
> > 1. Boot up without Xe module loaded
> > 
> > 2. Load Xe module with PSR disabled:
> >     $ modprobe xe  enable_psr=0
> > 
> > 3. Read panel link status register
> >     $ dpcd_reg read --offset 0x200e --count=1
> >     0x200e:  00
> > 
> > 4. Enable PSR, sleep for 2 seconds and disable PSR again:
> > 
> >     $ echo 0x1 > /sys/kernel/debug/dri/0/i915_edp_psr_debug
> >     $ echo "-1" >
> > /sys/kernel/debug/dri/0000:00:02.0/xe_params/enable_psr
> >     $ echo 0x0 > /sys/kernel/debug/dri/0/i915_edp_psr_debug
> >     $ sleep 2
> >     $ cat /sys/kernel/debug/dri/0/i915_edp_psr_status | grep status
> >     $ echo 0x1 > /sys/kernel/debug/dri/0/i915_edp_psr_debug
> >     Source PSR/PanelReplay status: DEEP_SLEEP [0x80310030]
> > 
> > 5. Now read panel link status registers again:
> >     $ dpcd_reg read --offset 0x200e --count=1
> >     0x200e:  80
> > 
> > Workaround this by not trusting link status registers after PSR is
> > enabled
> > until first short pulse interrupt is received.
> 
> Which eDP/DPCD version are we talking about?

This is at least since eDP 1.4.

> 
> Could this be related to AUX-less ALPM?

Panels we are seeing this are not using AUX-less ALPM.

> 
> Some nitpicks below, less important.
> 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 39
> > +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_psr.h      |  1 +
> >  4 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 2bb1fa64da2f..f0b7d7262961 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1618,6 +1618,8 @@ struct intel_psr {
> >         u32 dc3co_exit_delay;
> >         struct delayed_work dc3co_work;
> >         u8 entry_setup_frames;
> > +
> > +       bool link_ok;
> >  };
> >  
> >  struct intel_dp {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index b036c6521659..efaaadfb12ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5007,7 +5007,8 @@ intel_dp_needs_link_retrain(struct intel_dp
> > *intel_dp)
> >                 return true;
> >  
> >         /* Retrain if link not ok */
> > -       return !intel_dp_link_ok(intel_dp, link_status);
> > +       return !(intel_dp_link_ok(intel_dp, link_status) ||
> > +                intel_psr_link_ok(intel_dp));
> 
> Nitpick, in general I think "not A and not B" reads better than "not
> (A
> or B)" because saying the parens aloud is kind of clumsy. Reading
> code
> aloud (well, in my head anyway) is one of my main tests for
> readability.

yes, I agree on this. I was just thinking this too much as want it to
accept intel_dp_link_ok result if it's true. I will change it.
 
> 
> >  }
> >  
> >  bool intel_dp_has_connector(struct intel_dp *intel_dp,
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 992543f508c1..0cd7388389e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2009,6 +2009,15 @@ static void intel_psr_enable_locked(struct
> > intel_dp *intel_dp,
> >         intel_dp->psr.enabled = true;
> >         intel_dp->psr.paused = false;
> >  
> > +       /*
> > +        * Link_ok is sticky and set here on PSR enable. We can
> > assume link
> > +        * training is complete as we never continue to PSR enable
> > with
> > +        * untrained link. Link_ok is kept as set until first short
> > pulse
> > +        * interrupt. This is targeted to workaround panels stating
> > bad link
> > +        * after PSR is enabled.
> > +        */
> > +       intel_dp->psr.link_ok = true;
> > +
> >         intel_psr_activate(intel_dp);
> >  }
> >  
> > @@ -3458,6 +3467,9 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp)
> >  
> >         mutex_lock(&psr->lock);
> >  
> > +       /* Let's clear possibly set link_ok flag here */
> 
> That's kind of a translation of C into English. If you need the
> comment,
> then maybe state the why instead of the what?

You are right. I didn't succeed stating link_ok might not be set at all
but we are still clearing it here. Anyways useless comment. I will drop
it.

BR,

Jouni Högander

> 
> > +       psr->link_ok = false;
> > +
> >         if (!psr->enabled)
> >                 goto exit;
> >  
> > @@ -3517,6 +3529,33 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp)
> >         return ret;
> >  }
> >  
> > +/**
> > + * intel_psr_link_ok - return psr->link_ok
> > + * @intel_dp: struct intel_dp
> > + *
> > + * We are seeing unexpected link re-trainings with some panels.
> > This is caused
> > + * by panel stating bad link status after PSR is enabled. Code
> > checking link
> > + * status can call this to ensure it can ignore bad link status
> > stated by the
> > + * panel I.e. if panel is stating bad link and intel_psr_link_ok
> > is stating link
> > + * is ok caller should rely on latter.
> > + *
> > + * Return value of link_ok
> > + */
> > +bool intel_psr_link_ok(struct intel_dp *intel_dp)
> > +{
> > +       bool ret;
> > +
> > +       if ((!CAN_PSR(intel_dp) && !CAN_PANEL_REPLAY(intel_dp)) ||
> > +           !intel_dp_is_edp(intel_dp))
> > +               return false;
> > +
> > +       mutex_lock(&intel_dp->psr.lock);
> > +       ret = intel_dp->psr.link_ok;
> > +       mutex_unlock(&intel_dp->psr.lock);
> > +
> > +       return ret;
> > +}
> > +
> >  /**
> >   * intel_psr_lock - grab PSR lock
> >   * @crtc_state: the crtc state
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 5f26f61f82aa..956be263c09e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -59,6 +59,7 @@ void intel_psr2_program_trans_man_trk_ctl(const
> > struct intel_crtc_state *crtc_st
> >  void intel_psr_pause(struct intel_dp *intel_dp);
> >  void intel_psr_resume(struct intel_dp *intel_dp);
> >  bool intel_psr_needs_block_dc_vblank(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_psr_link_ok(struct intel_dp *intel_dp);
> >  
> >  void intel_psr_lock(const struct intel_crtc_state *crtc_state);
> >  void intel_psr_unlock(const struct intel_crtc_state *crtc_state);
>