Re: [PATCH v3 4/6] drm/i915/dp: Do not grab crtc modeset lock in intel_dp_detect()

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Oct 01, 2018 at 03:01:18PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-01 at 23:10 +0300, Ville Syrjälä wrote:
> > On Mon, Oct 01, 2018 at 12:48:28PM -0700, Rodrigo Vivi wrote:
> > > On Tue, Sep 25, 2018 at 07:54:22PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > A crtc modeset lock was added for link retraining but
> > > > intel_dp_retrain_link() knows to take the necessary locks since
> > > > commit c85d200e8321 ("drm/i915: Move SST DP link retraining into
> > > > the
> > > > ->post_hotplug() hook")
> > > > v2: Drop AUX power domain reference in the early return path
> > > > 
> > > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into
> > > > the ->post_hotplug() hook")
> > > 
> > > This patch failed on cherry-pick for drm-intel-fixes targeting
> > > 4.19.
> > > 
> > > If it still makes sense to have it there please provide a
> > > backported version
> > > to drm-intel-fixes or let me know if it has dependency on other
> > > patches?
> > 
> > This is more of an optimization to not disturb page flips and whatnot
> > so much with detect. The double lock is otherwise harmless due to
> > ww_mutex.
> > 
> Yeah, let's not backport this.

cool, thanks.

> 
> -DK
> 
> > > 
> > > Thanks,
> > > Rodrigo.
> > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 24 ++++++++++--------------
> > > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 09229fc66dec..ef28cc0f122c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5079,8 +5079,15 @@ intel_dp_long_pulse(struct intel_connector
> > > > *connector,
> > > >  	 * Some external monitors do not signal loss of link
> > > > synchronization
> > > >  	 * with an IRQ_HPD, so force a link status check.
> > > >  	 */
> > > > -	if (!intel_dp_is_edp(intel_dp))
> > > > -		intel_dp_retrain_link(encoder, ctx);
> > > > +	if (!intel_dp_is_edp(intel_dp)) {
> > > > +		int ret;
> > > > +
> > > > +		ret = intel_dp_retrain_link(encoder, ctx);
> > > > +		if (ret) {
> > > > +			intel_display_power_put(dev_priv, intel_dp-
> > > > >aux_power_domain);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Clearing NACK and defer counts to get their exact values
> > > > @@ -5130,19 +5137,8 @@ intel_dp_detect(struct drm_connector
> > > > *connector,
> > > >  		      connector->base.id, connector->name);
> > > >  
> > > >  	/* If full detect is not performed yet, do a full detect */
> > > > -	if (!intel_dp->detect_done) {
> > > > -		struct drm_crtc *crtc;
> > > > -		int ret;
> > > > -
> > > > -		crtc = connector->state->crtc;
> > > > -		if (crtc) {
> > > > -			ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > > -			if (ret)
> > > > -				return ret;
> > > > -		}
> > > > -
> > > > +	if (!intel_dp->detect_done)
> > > >  		status = intel_dp_long_pulse(intel_dp-
> > > > >attached_connector, ctx);
> > > > -	}
> > > >  
> > > >  	intel_dp->detect_done = false;
> > > >  
> > > > -- 
> > > > 2.14.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx