On 4/14/15 12:00 PM, Paulo Zanoni wrote:
2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
On 4/14/15 4:29 AM, Paulo Zanoni wrote:
2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
Update the hot plug function to handle the SST case. Instead of placing
the SST case within the long/short pulse block, it is now handled after
determining that MST mode is not in use. This way, the topology
management
layer can handle any MST-related operations while SST operations are
still
correctly handled afterwards.
This patch also corrects the problem of SST mode only being handled in
the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing
purposes
both short and long pulses are used by the different tests, thus both
cases
need to be addressed for SST.
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.
For the SST case, the main stream is disabled for long HPD pulses as this
generally indicates either a connect/disconnect event or link failure.
For
a number of case in compliance testing, the source is required to disable
the main link upon detection of a long HPD.
V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing
V4:
- Added a disable of the main stream (DP transport) for the long pulse
case
for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
V6:
- Reformatted a comment
Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 77b6b15..ba2da44 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4572,29 +4572,26 @@ 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
- */
-
drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
- intel_dp_check_link_status(intel_dp);
-
drm_modeset_unlock(&dev->mode_config.connection_mutex);
- }
}
Shouldn't the code be moved to exactly this spot instead of after the
put_power label? Why would we want to call check_link_status in case
we goto mst_fail? In case there is a valid reason, maybe it would be
better to do a big reorganization of this function because it's going
to start looking very weird - or at least rename the labels.
No because then you don't get long pulses, only short ones.
No, what I mean is:
if (long_hpd) {
... code ...
} else {
... code ....
}
if (!intel_dp->is_mst) {
drm_modeset_lock(...)
... code ...
}
mst_fail:
... code ...
The other problem I point is: imagine we're SST and we get a long_hpd.
Then we run ibx_digital_port_connected(), and since the monitor is
disconnected we "goto mst_fail". We'll end up running
intel_dp_check_link_status() before returning, but we really shouldn't
run it since we know the monitor is disconnected.
I see what you're saying, however under normal operation for SST (when
connected and everything is working) the code will hit this line:
if (!intel_dp_probe_mst(intel_dp))
And proceed to the mst_fail block, thus skipping that block of code
entirely and missing the SST handler. The result is a missed long pulse
for the SST case.
Your second point has validity though. This can be addressed with a
"connected" flag just before the if (long_pulse) statement:
connected = intel_dp_digital_port_connected(intel_dp);
if (!connected)
goto mst_fail;
The pulse handler for the most part can then be skipped, since the
device is gone. In mst_fail, the MST topology manager is updated though,
so that still has to happen. With the SST pulse handler in put_power, it
can simply fall through now and hit the updated if-statement there which is:
if (!intel_dp->is_mst && connected) {
... code ...
}
And all should be well for a disconnected device as well as normal
operational modes of SST and MST.
Oh the intel_dp_digital_port_connected() function is just the
encapsulation of this code from the long_pulse segment:
if (HAS_PCH_SPLIT(dev)) {
if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
goto mst_fail;
} else {
if (g4x_digital_port_connected(dev, intel_dig_port) != 1)
goto mst_fail;
}
And it's been added to the updated patch.
The put_power
case is where this belongs, unless you want to duplicate code in both the
long_pulse and the else clause. There is a separate mst_check_link_status
call so this one is specific to SST mode. There is also a check to make sure
it doesn't get called when MST is active and MST has hit a failure mode, so
that is a non-issue.
Also, for the long_hpd case, I see that check_link_status() will redo
some of the stuff we already did on this function, such as get_dpcd().
And if you follow my advice on patch 2, you will end up having even
more repeated code. I think you could try to do a careful analysis
here to make sure we're not calling stuff twice here, especially since
some of those operations are potentially slow.
I see a couple places where the code is duplicated, specifically the
connection check (which I encapsulated in a function and I'll likely roll
forward into this one since it makes things more clear) and the DPCD read in
the long pulse case. I removed the code in check_link_status for both of
these things and it still passes compliance. Good catch Paulo. This has been
fixed and tested and will be in the updated patch posted shortly.
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) {
+ /* if we were in MST mode, and device is not there get
out of MST mode */
I don't see the need for changes such as the one above - I saw similar
cases in other patches you submitted. I often use git blame on
comments in order to be able to see the whole context of the change,
and a simple change like this makes it harder to blame. Also, you're
not even fixing the 80 column problem here. And I do prefer the
comment on top of the if statement.
This is just an artifact of moving things around, as it likely was in the
other patches. The only reason I will move comments is to clarify what they
pertain to if code is moving around it. It's back where it belongs now so it
doesn't even show up in the patch. Fixed for the next version.
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:
+ /* SST mode - handle short/long pulses here */
+ if (!intel_dp->is_mst) {
+ 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 = IRQ_HANDLED;
+ }
intel_display_power_put(dev_priv, power_domain);
return ret;
--
1.9.1
_______________________________________________
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
