Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/dp: Get TC link reference during DP detection

[Imre Deak <imre.deak@intel.com>]

On Mon, Mar 30, 2020 at 11:27:06PM +0300, Souza, Jose wrote:
> On Sat, 2020-03-28 at 12:26 +0200, Imre Deak wrote:
> > On Tue, Mar 24, 2020 at 01:14:29PM -0700, José Roberto de Souza
> > wrote:
> > > As now the cost to lock and use a TC port is higher due the
> > > implementation of the TCCOLD sequences it is worty to hold a
> > > reference of the TC port to avoid all this locking at every
> > > aux transaction part of the DisplayPort detection.
> > 
> > The problem with locking the port for detection is that it would
> > block modesets on the port, which we should avoid. By blocking
> > tc-cold
> 
> It will not block modesets on the port, intel_tc_port_get_link and
> intel_tc_port_put_link gets locks tc_lock, increment or decrement
> tc_link_refcount and then unlock,

The effect of holding a link_refcount is that it's not possible to
update the TC port mode and select the correct TBT/MG PLL for the mode.
This will only be possible in the middle of the modeset sequence where
an active mode is first disabled on the port and that's the place we
don't want to wait for the detect sequence to finish.

So only an active mode should hold a link_refcount, so that it's
guaranteed that a modeset can update the TC port mode to its current
state.

> it would only avoid the TC cold sequences over and over.

Yes, but that would be also avoided by disabling the AUX power well only
with a delay.

>
> > whenever enabling an AUX power well you would avoid the overhead of
> > the
> > PCODE requests for each AUX transfer, since the AUX power refs are
> > dropped asynchronously with a delay.
> 
> Left comments on your proposal in patch 1.
> 
> > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 7f1a4e55cda1..6fbf3beee544 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -6041,6 +6041,7 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  	struct intel_dp *intel_dp =
> > > intel_attached_dp(to_intel_connector(connector));
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >  	struct intel_encoder *encoder = &dig_port->base;
> > > +	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> > >  	enum drm_connector_status status;
> > >  
> > >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> > > @@ -6049,12 +6050,17 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  		    !drm_modeset_is_locked(&dev_priv-
> > > >drm.mode_config.connection_mutex));
> > >  
> > >  	/* Can't disconnect eDP */
> > > -	if (intel_dp_is_edp(intel_dp))
> > > +	if (intel_dp_is_edp(intel_dp)) {
> > >  		status = edp_detect(intel_dp);
> > > -	else if (intel_digital_port_connected(encoder))
> > > -		status = intel_dp_detect_dpcd(intel_dp);
> > > -	else
> > > -		status = connector_status_disconnected;
> > > +	} else {
> > > +		if (intel_phy_is_tc(dev_priv, phy))
> > > +			intel_tc_port_get_link(dig_port, 1);
> > > +
> > > +		if (intel_digital_port_connected(encoder))
> > > +			status = intel_dp_detect_dpcd(intel_dp);
> > > +		else
> > > +			status = connector_status_disconnected;
> > > +	}
> > >  
> > >  	if (status == connector_status_disconnected) {
> > >  		memset(&intel_dp->compliance, 0, sizeof(intel_dp-
> > > >compliance));
> > > @@ -6132,6 +6138,9 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  	if (status != connector_status_connected && !intel_dp->is_mst)
> > >  		intel_dp_unset_edid(intel_dp);
> > >  
> > > +	if (intel_phy_is_tc(dev_priv, phy))
> > > +		intel_tc_port_put_link(dig_port);
> > > +
> > >  	/*
> > >  	 * Make sure the refs for power wells enabled during detect are
> > >  	 * dropped to avoid a new detect cycle triggered by HPD
> > > polling.
> > > -- 
> > > 2.26.0
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx