Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>]

On Tuesday 17 November 2015 02:52 PM, Ander Conselvan De Oliveira wrote:
> 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
Yes, this is intentional. Intel_dp_detect is supposed to be called after 
the long pulse has been handled. If EDID is not present, that means 
either long pulse was never received or there is no panel connected.

>
>>>> 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@intel.com>
>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx