Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

From: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
To: Ander Conselvan De Oliveira <conselvan2@gmail.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
Date: Fri, 29 Jan 2016 14:31:50 +0530	[thread overview]
Message-ID: <56AB2A7E.6030403@intel.com> (raw)
In-Reply-To: <1453814570.2695.18.camel@gmail.com>

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@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 | 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.

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.

>   
>>   	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);
>>   

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