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

From: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 4/6] drm/i915/dp: Do not grab crtc modeset lock in intel_dp_detect()
Date: Mon, 01 Oct 2018 15:01:18 -0700	[thread overview]
Message-ID: <582e9d724154736dc40bdc04e56d70067ff01bc9.camel@intel.com> (raw)
In-Reply-To: <20181001201006.GP9144@intel.com>

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.

-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