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

[Lyude Paul <lyude@redhat.com>]

On Tue, 2018-08-07 at 22:26 +0000, Jan-Marek Glogowski wrote:
> Am August 7, 2018 7:33:12 PM UTC schrieb Lyude Paul <lyude@redhat.com>:
> > On Mon, 2018-08-06 at 12:25 +0200, Jan-Marek Glogowski wrote:
> > > 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")
> > > 
> > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 46
> > 
> > ++++++++++++++++++++++++------------
> > > -----
> > >  1 file changed, 27 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 8e0e14b..7e6f8a5 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)
> > >  {
> > > @@ -4361,10 +4349,12 @@ int intel_dp_retrain_link(struct
> > 
> > intel_encoder
> > > *encoder,
> > >  	if (!connector || connector->base.status !=
> > > connector_status_connected)
> > >  		return 0;
> > > 
> > > -	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
> > > -			       ctx);
> > > -	if (ret)
> > > -		return ret;
> > > +	if (ctx) {
> > > +		ret = drm_modeset_lock
> > > +			(&dev_priv->drm.mode_config.connection_mutex, ctx);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > 
> > ...why are we skipping locking anything if there isn't a
> > drm_modeset_acquire_ctx passed to us? And additionally, why wouldn't
> > there be
> > an acquisition context passed to us? We always need to be holding the
> > connection mutex and the respective CRTC lock whenever we're
> > retraining.
> 
> The original patch added all the locking to intel_dp_retrain_link.
> intel_dp_long_pulse is just called from intel_dp_detect, which takes the
> drm_modeset_lock depending on a valid crtc. I just assumed this is still
> sufficient, so for this case we don't need to acquire the lock again. If
> this is not sufficient, we can forward the ctx, state etc. from
> intel_dp_detect. I didn't check, if these values are the same values that
> intel_dp_retrain_link collects using different calls.

Since intel_dp_retrain_link() is meant to be called from pretty much anywhere
with an acquisition context, you want to assume that at the start of the
function there's no locks being held at all even that isn't currently the case
in the code (likewise, we have plans to call this function from other places
soon). As well, it's perfectly OK to grab a modeset lock twice in a row, as
long as it is only ever dropped once. That's why we made ww_mutex in the first
place!

That being said: we /cannot/ perform link training if the CRTC is not locked,
same for the connector lock. Things will break very badly if we're in the
middle of retraining a DP link if something else tries to change the display
state at the same time. I think the original code did this as well, but had
the locking in different spots. If it didn't then that code was definitely
broken, and was probably why we changed it in the first place.

> 
> There is also the "WARN_ON(!drm_modeset_is_locked(&dev_priv-
> >drm.mode_config.connection_mutex));" in intel_dp_long_pulse, which suggests
> we should also already have the connection_mutex when calling
> intel_dp_retrain_link later.
I wouldn't worry about that one unless you get a warning :).
intel_dp_long_pulse() is pretty much exclusively meant for handling long pulse
HPD events, so it's not really meant to be used outside of that context. That
is just there in case that changes someday in the future, at which point we
can add the appropriate locking.

> 
> All my assumptions are based on reading the original patch and a little bit
> of the surrounding code.
> 
> This patch is just a minimal change, which "works for me". Not sure if all
> the locking should be moved back to the retrain call sites, to make it more
> explicit?
(more comments down below...)
> 
> > > 
> > >  	conn_state = connector->base.state;
> > > 
> > > @@ -4372,9 +4362,11 @@ int intel_dp_retrain_link(struct intel_encoder
> > > *encoder,
> > >  	if (!crtc)
> > >  		return 0;
> > > 
> > > -	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> > > -	if (ret)
> > > -		return ret;
> > > +	if (ctx) {
> > > +		ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > 
> > >  	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > 
> > > @@ -4982,6 +4974,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, NULL);
So, the only valid thing for you to do here is to make it so that we pass ctx
to intel_dp_long_pulse() so that it can grab the locks it needs for retraining
under the context that's being used for the connector probing.

> > >  	}
> > > 
> > >  	/*
> 
> 
-- 
Cheers,
	Lyude Paul

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx