On 12/17/14 1:30 PM, Daniel Vetter wrote:
On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote:
2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@xxxxxxxxx>:
Moves the non-MST case out of the if-statement and places it at the beginning
of the function to handle HPD events for SST mode. The reasoning behind this
is to accommodate link status checks for compliance testing. Some test devices
use long pulses to perform test requests so link status must be checked
regardless of the pulse width for the SST operational mode.
Can you please elaborate a little more on what do you see on these
devices? The test spec is very clear about short vs long HPD pulses,
so it's hard to believe a test device would get this wrong. We have
some registers on the PCH that allow us to redefine short vs long
durations. Have you tried to play with them?
More below:
This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch
indicated that updating intel_dp_hot_plug() was not the correct place for
the test handler.
Signed-off-by: Todd Previte<tprevite@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a55ca6..73014d8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
power_domain = intel_display_port_power_domain(intel_encoder);
intel_display_power_get(dev_priv, power_domain);
+ if (!intel_dp->is_mst) {
+ /*
+ * Pulse width doesn't matter for SST mode
+ * Handle the HPD event now
+ */
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+ intel_dp_check_link_status(intel_dp);
The very first thing intel_dp_check_link_status() does is to return in
case "connector->base.status != connected". If we're getting a long
HPD, it doesn't seem make sense to check this field because the status
might be changing due to the long HPD.
I don't think we can unconditionally run SST hpd logic before we've
correctly handled mst mode. It likely screws up the accounting.
Upon further review, it looks like the best solution here is to place
the SST code in the mst_fail case as the 'else' clause. That way SST
mode stuff gets handled regardless and if a need arises in the future
where differentiating between short and long becomes necessary, there's
a place to handle it. This will be in V3.
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
+ ret = false;
+ goto put_power;
+ }
+
if (long_hpd) {
if (HAS_PCH_SPLIT(dev)) {
@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
goto mst_fail;
}
-
- if (!intel_dp->is_mst) {
- /*
- * we'll check the link status via the normal hot plug path later -
- * but for short hpds we should check it now
- */
Just aside: The above comment is outdated and can be remove. This is now
the only place where we handle link retraining. The function could be made
static and dropped from headers, too.
-Daniel
Can't be static and pulled from the header. It's called in intel_ddi.c
as well.
But the comment and the code is gone, in light of the above changes.
- drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
- intel_dp_check_link_status(intel_dp);
- drm_modeset_unlock(&dev->mode_config.connection_mutex);
- }
}
ret = false;
goto put_power;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
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
