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@xxxxxxxxx>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@xxxxxxxxx>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
