Re: [PATCH v3] drm/i915/dp: Read link status more times when EQ not done

[Jim Bride <jim.bride@linux.intel.com>]

On Fri, Mar 31, 2017 at 04:25:31PM -0700, Rodrigo Vivi wrote:
> On Mon, Mar 13, 2017 at 1:12 AM, Lee, Shawn C <shawn.c.lee@intel.com> wrote:
> > From: "Lee, Shawn C" <shawn.c.lee@intel.com>
> >
> > Display driver read DPCD register 0x202, 0x203 and 0x204 to identify
> > eDP sink status.If PSR exit is ongoing at eDP sink, and eDP source
> > read these registers at the same time. Panel will report EQ & symbol
> > lock not done. It will cause panel display flicking.
> >
> > Try to read link status more times if eDP EQ not done. Panel side
> > request at least 1000us for fast link train while doing PSR exit.
> > So wait more than 1000us then retrieve sink's status again.
> 
> it is missing a v2 and v3 here with explanations on the changes.
> It was hard to follow the changes.
> 
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99639
> > TEST=Reboot DUT and no flicking on local display at login screen
> >
> > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > Cc: Wei Shun Chen <wei.shun.chang@intel.com>
> > Cc: Gary C Wang <gary.c.wang@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 024798a9c016..d50827a92aa2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4225,15 +4225,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  {
> >         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >         struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > -       u8 link_status[DP_LINK_STATUS_SIZE];
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       u8 link_status[DP_LINK_STATUS_SIZE], retry = 1;
> >
> >         WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >
> > -       if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > -               DRM_ERROR("Failed to get link status\n");
> > -               return;
> > -       }
> > -
> >         if (!intel_encoder->base.crtc)
> >                 return;
> >
> > @@ -4245,13 +4241,31 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >         if (!intel_dp->lane_count)
> >                 return;
> >
> > +       if (is_edp(intel_dp) && dev_priv->psr.enabled)
> > +               retry = 3;
> > +
> >         /* Retrain if Channel EQ or CR not ok */
> > -       if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> > -               DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> > -                             intel_encoder->base.name);
> > +       while ( retry-- ) {
> > +               if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > +                       DRM_ERROR("Failed to get link status\n");
> > +                       return;
> 
> Well, if link status is not ok you return without retrying, so, why is
> this here?
> 
> > +               }
> >
> > -               intel_dp_retrain_link(intel_dp);
> > +               if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
> > +                       return;
> > +
> > +               /*
> > +                * EQ not ok may caused by fast link train while exit PSR active,
> > +                * wait at least 1000 us then read it again.
> > +                */
> > +               if (retry)
> > +                       usleep_range(1000, 1500);
> 
> maybe this retry is randomly just masking the real issue.
> Jim recently found out that on psr enable we are clearing a bit that
> we should never touch by spec.
> I'd try Jim's patch(es) first to see if they solve the issue for you.

Actually, I'd go one better.  If you look at the code, we should
never get to running the actual link status checks due to the
following snippet a few lines up.

       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
                       return;
		       
Basically, we only check the link status if the pipe is not active.
If the pipe isn't active, then PSR should be disabled.  If it's not,
then that's a scarier problem.  I've been tempted to throw a WARN_ON
in intel_dp_start_link_training() to ensure that PSR is off before
we start modifying the link state, because it really messes
with the re-syncing done at PSR exit time (which this patch was
trying to compensate for.)  In any event, assuming the pipe is
disabled, then PSR shouldn't be an issue.

The patch that Rodrigo is referring to is
https://patchwork.freedesktop.org/patch/147942/ and it could
certainly help if on SKL+.

Jim


> 
> >         }
> > +
> > +       DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> > +               intel_encoder->base.name);
> > +
> > +       intel_dp_retrain_link(intel_dp);
> >  }
> >
> >  /*
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx