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

["Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>]

On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> 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@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.
> 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.
>
hmm i think this discussion will prolong for a while :)
how about we call intel_dp_long_pulse() always for now.
this will be a compromise to not break any of the existing code
but will still result in getting a clean detection code, which
will can then be improved upon in the next iteration ?
i.e post the change it should look like. i.e skip this change alone

	intel_dp_long_pulse(intel_dp->attached_connector);


regards,
Sivakumar
>> 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);
>>>>    
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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