[Intel-gfx] [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Sun Jan 31 22:20:55 PST 2016
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 at intel.com>
>>>> 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 | 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list