[Intel-gfx] [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> 
> On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > Current DP detection has DPCD operations split across
> > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > duplicates as well. Also intel_dp_detect is called
> > > during modes enumeration as well which will result
> > > in multiple dpcd operations. So this patch tries
> > > to solve both these by bringing all DPCD operations
> > > in one single function and make intel_dp_detect
> > > use existing values instead of repeating same steps.
> > > 
> > > v2: Pulled in a hunk from last patch of the series to
> > >      this patch. (Ander)
> > > v3: Added MST hotplug handling. (Ander)
> > > 
> > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu at intel.com>
> > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
> > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----------
> > > -----
> > > -
> > >   1 file changed, 44 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 8969ff9..82ee18d 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > [...]
> > 
> > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   		return connector_status_disconnected;
> > >   	}
> > >   
> > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > +	if (force)
> > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > I didn't notice this at first, but force is not the right thing to check for
> > here. It is basically intended to avoid doing load detection (see
> > intel_get_load_detect_pipe()) on automated polling. But we still have to try
> > detection here when force = false, otherwise this will cause a regression.
> > 
> > If you plug in a DP device while suspended, that device won't get detected,
> > since we don't get an HPD for it. Previously, the call do intel_dp_detect()
> > with
> > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would
> > cause a full detection.
> > 
> > To avoid the repeated DPCD operations, I think we need a more explicit
> > mechanism
> > to signal that we already handled the long pulse via the HPD handler. In
> > intel_dp_hpd_pulse() we could set a flag that tells we just handled a long
> > pulse
> > for the given port. The call to intel_dp_long_pulse() in intel_dp_detect()
> > would
> > then be dependent on that flag.
> > 
> > For that reason I have to retract my R-b from this patch.
> > 
> > Ander
> 
> Call to intel_dp_detect() from get_modes is with force set to true while 
> from resume the call is with force set to false.. It should be in the 
> opposite manner as get_modes should not require full detection whereas 
> resume should. So, this needs to be cleaned up there. After merge of 
> these patches, will look into cleaning up that part of the code.

That really depends on what the force parameter is supposed to mean. The
documentation states that "force is set to false whilst polling, true when
checking the connector due to a user request". A look through git history shows
the parameter was added to reduce time wasted doing load detection (doing a mode
set in order to check if there is a device connected) for hardware that needs it
(commit 7b334fcb45b7).

As far as I can tell, across all the drm drivers, that parameter is only used to
avoid doing load detection.

Another thing to consider is that the driver may switch to polling if it detects
an HPD storm. When the detect calls come from polling, the code in this patch
will simply avoid any detection.


> Moreover, intel_dp_detect() will be called from 
> drm_helper_hpd_irq_event() in polling scenarios only (when 
> DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like 
> this code here, doesn't really create a regression for realtime scenarios.

I don't know what you mean by realtime scenarios, but the regression is very
real. Using a kernel with your patches applied, suspend while there is no DP
monitor attached, attach the monitor while suspended and then wake up. Notice
how the connector state doesn't change. You can check the i915_display_info file
in debugfs, for instance.


Ander

> >   
> > >   	if (intel_connector->detect_edid)
> > >   		return connector_status_connected;
> > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port, bool long_hpd)
> > >   		/* indicate that we need to restart link training */
> > >   		intel_dp->train_set_valid = false;
> > >   
> > > -		if (!intel_digital_port_connected(dev_priv,
> > > intel_dig_port))
> > > -			goto mst_fail;
> > > -
> > > -		if (!intel_dp_get_dpcd(intel_dp)) {
> > > -			goto mst_fail;
> > > -		}
> > > -
> > > -		intel_dp_probe_oui(intel_dp);
> > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > +		if (intel_dp->is_mst)
> > > +			ret = IRQ_HANDLED;
> > > +		goto put_power;
> > >   
> > > -		if (!intel_dp_probe_mst(intel_dp)) {
> > > -			drm_modeset_lock(&dev
> > > ->mode_config.connection_mutex,
> > > NULL);
> > > -			intel_dp_check_link_status(intel_dp);
> > > -			drm_modeset_unlock(&dev
> > > ->mode_config.connection_mutex);
> > > -			goto mst_fail;
> > > -		}
> > >   	} else {
> > >   		if (intel_dp->is_mst) {
> > > -			if (intel_dp_check_mst_status(intel_dp) == 
> > > -EINVAL)
> > > -				goto mst_fail;
> > > +			if (intel_dp_check_mst_status(intel_dp) == 
> > > -EINVAL) {
> > > +				/*
> > > +				 * If we were in MST mode, and device is
> > > not
> > > +				 * there, get out of MST mode
> > > +				 */
> > > +				DRM_DEBUG_KMS("MST device may have
> > > disappeared %d vs %d\n",
> > > +					intel_dp->is_mst, intel_dp
> > > ->mst_mgr.mst_state);
> > > +				intel_dp->is_mst = false;
> > > +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> > > ->mst_mgr,
> > > +								intel_dp
> > > ->is_mst);
> > > +				goto put_power;
> > > +			}
> > >   		}
> > >   
> > >   		if (!intel_dp->is_mst) {
> > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port, bool long_hpd)
> > >   
> > >   	ret = IRQ_HANDLED;
> > >   
> > > -	goto put_power;
> > > -mst_fail:
> > > -	/* if we were in MST mode, and device is not there get out of MST
> > > mode */
> > > -	if (intel_dp->is_mst) {
> > > -		DRM_DEBUG_KMS("MST device may have disappeared %d vs
> > > %d\n",
> > > intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> > > -		intel_dp->is_mst = false;
> > > -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > intel_dp
> > > ->is_mst);
> > > -	}
> > >   put_power:
> > >   	intel_display_power_put(dev_priv, power_domain);
> > >   
>