Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Rodrigo Vivi]

On Mon, Aug 27, 2018 at 01:39:02PM -0400, Lyude Paul wrote:
> On Mon, 2018-08-27 at 11:43 +0300, Jani Nikula wrote:
> > On Sat, 25 Aug 2018, Lyude Paul <lyude@xxxxxxxxxx> wrote:
> > > From: Jan-Marek Glogowski <glogow@xxxxxxxxxx>
> > > 
> > > This re-applies the workaround for "some DP sinks, [which] are a
> > > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > > quality check unconditionally during long pulse").
> > > It makes the secondary AOC E2460P monitor connected via DP to an
> > > acer Veriton N4640G usable again.
> > > 
> > > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > > DP link retraining into the ->post_hotplug() hook")
> > > 
> > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > ->post_hotplug() hook")
> > > [Cleaned up commit message, added stable cc]
> > > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> > > Signed-off-by: Jan-Marek Glogowski <glogow@xxxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > ---
> > > Resending this to update patchwork; will push in a little bit
> > 
> > Is there a bugzilla? Reference to a list discussion? Something with a
> > dmesg where someone can actually verify this is the right fix?
> This patch has actually been on the list for a while now-I have had mdnavare
> take a look at it as well (they said it looked fine with the only change being
> in regards to the comment), and it'd been on the list for a while already.
> 
> > 
> > IMO needs an ack from Ville too. He should be in Cc: in the first place
> > as the author of the regressing commit.
> > 
> > 'dim fixes c85d200e8321' gives you the output:
> aaah-I had thought it was just for generating the Fixes line, I will be more
> careful about that in the future

Hi Lyude,

I understand that this patch was a while on the mailing list and no
proper attention, but besides the commit message information that
dim is generating I have a feeling now that this patch wasn't quite
ready yet and shouldn't be rushed in like it was.

Since you modified few versions ideally it would be good to have extra eyes
and the mandatory "Reviewed-by" there.

Also it is a good idea to give few days for different timezones to
comment out (after the re-sping I mean)

And finally based on the discussions it seems that there is still
some work to get done or a different way to handle this other
this back and forth of reapply-revert-reapply...

I just noticed it when preparing drm-intel-fixes pull request.

Since I'm not 100% confident this patch should be here I'm removing it
from this week's pull request.

But probably adding it next week if Jani and Ville give and ack
or indication that it is right or at least he best we could do now.

thanks,
Rodrigo.


> > 
> > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > ->post_hotplug() hook")
> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: <stable@xxxxxxxxxxxxxxx> # v4.17+
> > 
> > BR,
> > Jani.
> > 
> > > 
> > >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index b3f6f04c3c7d..db8515171270 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > > *intel_dp)
> > >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > >  }
> > >  
> > > -/*
> > > - * If display is now connected check links status,
> > > - * there has been known issues of link loss triggering
> > > - * long pulse.
> > > - *
> > > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > - * weird HPD ping pong during modesets. So we can apparently
> > > - * end up with HPD going low during a modeset, and then
> > > - * going back up soon after. And once that happens we must
> > > - * retrain the link to get a picture. That's in case no
> > > - * userspace component reacted to intermittent HPD dip.
> > > - */
> > >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> > >  			  struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > >  }
> > >  
> > >  static int
> > > -intel_dp_long_pulse(struct intel_connector *connector)
> > > +intel_dp_long_pulse(struct intel_connector *connector,
> > > +		    struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > > *connector)
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > +	} else {
> > > +		/*
> > > +		 * If display is now connected check links status,
> > > +		 * there has been known issues of link loss triggering
> > > +		 * long pulse.
> > > +		 *
> > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > +		 * weird HPD ping pong during modesets. So we can apparently
> > > +		 * end up with HPD going low during a modeset, and then
> > > +		 * going back up soon after. And once that happens we must
> > > +		 * retrain the link to get a picture. That's in case no
> > > +		 * userspace component reacted to intermittent HPD dip.
> > > +		 */
> > > +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
> > > >base;
> > > +
> > > +		intel_dp_retrain_link(encoder, ctx);
> > >  	}
> > >  
> > >  	/*
> > > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> > >  				return ret;
> > >  		}
> > >  
> > > -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > +		status = intel_dp_long_pulse(intel_dp->attached_connector,
> > > ctx);
> > >  	}
> > >  
> > >  	intel_dp->detect_done = false;
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx