[Intel-gfx] [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Tue, 2015-11-17 at 11:47 +0530, Shubhangi Shrivastava wrote:
> 
> On Monday 16 November 2015 07:10 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> > > This patch moves probing for panel, DPCD read etc to another
> > > function intel_dp_long_pulse, while intel_dp_detect returns
> > > the status as connected or disconnected depending on
> > > whether the edid is available or not.
> > Why is the change in connector status necessary?
> Since intel_dp_detect is also called in mode enumeration, so all detect 
> related operations are moved to intel_dp_long_pulse to avoid any such 
> operations to be performed during mode enumeration. intel_dp_long_pulse 
> will perform all the panel probing, DPCD read operations etc. which will 
> end with EDID being read. In intel_dp_detect, it is checked if EDID has 
> been read or not. If EDID has been read then the status is returned as 
> connected to allow further operations otherwise the status is returned 
> as disconnected.

So if I understood correctly, the EDID change is only a about doing a better
split of the code. My concern is that drm_get_edid() might fail. Before this
patch that wouldn't affect the connector status, but now a failure to read EDID 
implies that the connector is disconnected. I'm not sure in which scenario this
would happen, so I wanted to know if this change in behavior is intentional.

Ander

> 
> > 
> > > This change will be required by further patches in the series
> > > to avoid performing multiple DPCD operations and removing
> > > their duplication.
> > > 
> > > 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 | 66 ++++++++++++++++++++++++++++++-----
> > > -----
> > > -
> > >   1 file changed, 49 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 1cb1f3f..a0fe827 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
> > >   	intel_display_power_put(to_i915(encoder->base.dev),
> > > power_domain);
> > >   }
> > >   
> > > -static enum drm_connector_status
> > > -intel_dp_detect(struct drm_connector *connector, bool force)
> > > +static void
> > > +intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >   {
> > > +	struct drm_connector *connector = &intel_connector->base;
> > >   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > >   	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > >   	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > @@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   	bool ret;
> > >   	u8 sink_irq_vector;
> > >   
> > > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > -		      connector->base.id, connector->name);
> > > -	intel_dp_unset_edid(intel_dp);
> > > -
> > > -	if (intel_dp->is_mst) {
> > > -		/* MST devices are disconnected from a monitor POV */
> > > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > -		return connector_status_disconnected;
> > > -	}
> > > -
> > >   	power_domain = intel_dp_power_get(intel_dp);
> > >   
> > >   	/* Can't disconnect eDP, but you can close the lid... */
> > > @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   		status = ironlake_dp_detect(intel_dp);
> > >   	else
> > >   		status = g4x_dp_detect(intel_dp);
> > > -	if (status != connector_status_connected)
> > > +	if (status != connector_status_connected) {
> > > +		intel_dp_unset_edid(intel_dp);
> > >   		goto out;
> > > +	}
> > >   
> > >   	intel_dp_probe_oui(intel_dp);
> > >   
> > >   	ret = intel_dp_probe_mst(intel_dp);
> > >   	if (ret) {
> > > -		/* if we are in MST mode then this connector
> > > -		   won't appear connected or have anything with EDID on
> > > it */
> > > +		/*
> > > +		 * if we are in MST mode then this connector
> > > +		 * won't appear connected or have anything with
> > > +		 * EDID on it
> > > +		 */
> > >   		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > >   			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > >   		status = connector_status_disconnected;
> > >   		goto out;
> > Here the function will exit without a call to intel_dp_unset_edid(), which
> > is
> > different from the behavior prior to this patch.
> Moved call to intel_dp_unset_edid() to out, as per next comment.
> > 
> > > +	} else if (connector->status == connector_status_connected) {
> > > +
> > > +		/*
> > > +		 * If display was connected already and is still
> > > connected
> > > +		 * check links status, there has been known issues of
> > > +		 * link loss triggerring long pulse!!!!
> > > +		 */
> > > +		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 out;
> > >   	}
> > The hunk above should be in a separate patch. It also doesn't call
> > intel_dp_unset_edid(). Might be easier to just add
> > 
> > out:
> > 	if (status == connector_status_disconnected)
> > 		intel_dp_unset_edid(intel_dp);
> > 
> > to the end of the function.
> > 
> > Ander
> Done. Moved the hunk to next patch and call to intel_dp_unset_edid to out.
> > 
> > >   	intel_dp_set_edid(intel_dp);
> > > @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   
> > >   out:
> > >   	intel_dp_power_put(intel_dp, power_domain);
> > > -	return status;
> > > +	return;
> > > +}
> > > +
> > > +static enum drm_connector_status
> > > +intel_dp_detect(struct drm_connector *connector, bool force)
> > > +{
> > > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > +	struct intel_connector *intel_connector =
> > > to_intel_connector(connector);
> > > +
> > > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > +		      connector->base.id, connector->name);
> > > +
> > > +	if (intel_dp->is_mst) {
> > > +		/* MST devices are disconnected from a monitor POV */
> > > +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > +		return connector_status_disconnected;
> > > +	}
> > > +
> > > +	intel_dp_long_pulse(intel_dp->attached_connector);
> > > +
> > > +	if (intel_connector->detect_edid)
> > > +		return connector_status_connected;
> > > +	else
> > > +		return connector_status_disconnected;
> > >   }
> > >   
> > >   static void
>