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

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

On Friday 15 January 2016 03:37 PM, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote:
>> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
>>> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
>>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>>>> intel_dp_detect() is called for not just detection but
>>>>> during modes enumeration as well. Repeating the whole
>>>>> sequence during each of these calls is wasteful and
>>>>> time consuming.
>>>>> This patch moves probing for panel, DPCD read etc done in
>>>>> intel_dp_detect() to a new function intel_dp_long_pulse().
>>>>> Note that the behavior of intel_dp_detect() is changed to
>>>>> report connected or disconnected depending on whether the
>>>>> EDID is available or not.
>>>>> This change will be required by further patches in the series
>>>>> to avoid performing duplicated DPCD operations on hotplug.
>>>>>
>>>>> v2: Moved a hunk to next patch of the series.
>>>>>       Moved intel_dp_unset_edid to out. (Ander)
>>>>> v3: Rephrased commit message and intel_dp_unset_dp() is called
>>>>>       within intel_dp_set_dp() to free the previous EDID. (Ander)
>>>>>
>>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>>> 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 | 56 +++++++++++++++++++++++++--------
>>>>> -----
>>>>> --
>>>>> -
>>>>>    1 file changed, 35 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index 796e3d3..e3b4208 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp
>>>>> *intel_dp,
>>>>> bool sync);
>>>>>    static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>>>>>    static void vlv_steal_power_sequencer(struct drm_device *dev,
>>>>>    				      enum pipe pipe);
>>>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>>>>>    
>>>>>    static unsigned int intel_dp_unused_lane_mask(int lane_count)
>>>>>    {
>>>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>>>>>    	struct intel_connector *intel_connector = intel_dp
>>>>> ->attached_connector;
>>>>>    	struct edid *edid;
>>>>>    
>>>>> +	intel_dp_unset_edid(intel_dp);
>>>>>    	edid = intel_dp_get_edid(intel_dp);
>>>>>    	intel_connector->detect_edid = edid;
>>>>>    
>>>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>>>>    	intel_dp->has_audio = false;
>>>>>    }
>>>>>    
>>>>> -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;
>>>>> @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
>>>>>    	intel_display_power_get(to_i915(dev), power_domain);
>>>>>    
>>>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>> This deletion is new in this version of the patch. I think we still need
>>>> the
>>>> hunk above, otherwise we might not properly update the encoder type when
>>>> we
>>>> switch from an HDMI sink connected through a level shifter to an MST sink.
>>>>
>>>> Ander
>>>>
>> Encoder type setting for MST is being done in intel_dp_detect(). So,
>> don't find a need to add it here.
> Yes, but that one only covers the case where the device was already previously
> identified as MST. For a device identified as MST by the call to
> intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will
> not be done. Hopefully, Ville's patch that splits the encoder types and makes
> this unnecessary will land soon, but for now just leave the override there.
>
> Ander

Alright.. Moved the overriding of encoder type to be done before probing..

>>>>> -		status = connector_status_disconnected;
>>>>> +	if (ret)
>>>>>    		goto out;
>>> Also, there is no call to intel_dp_unset_edid() for this case, since the
>>> code
>>> will reach the label 'out' with status being connected. So in this case the
>>> return value of intel_dp_detect() will depend on the stale value of
>>> intel_dp->detect_edid.
>>>
>>> Ander
>> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst()
>> check of intel_dp_detect().

Added call to intel_dp_unset_edid() in is_mst() check of intel_dp_detect().

>>>>> -	}
>>>>>    
>>>>>    	/*
>>>>>    	 * Clearing NACK and defer counts to get their exact values
>>>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	}
>>>>>    
>>>>>    out:
>>>>> +	if (status != connector_status_connected)
>>>>> +		intel_dp_unset_edid(intel_dp);
>>>>>    	intel_display_power_put(to_i915(dev), 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