On Fri, 2017-12-22 at 00:48 +0000, Pandiyan, Dhinakaran wrote:
On Thu, 2017-12-21 at 08:53 +0200, Jani Nikula wrote:
On Wed, 20 Dec 2017, Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com wrote:
Occasionally there are LINK_ADDRESS sideband messages timing out with the Lenovo MST dock + Dell MST monitor(w/ in-built branch) setup I have. These failures lead to the display not coming up on boot. Power cycling the port corresponding to the MST monitor's branch device and resending the message fixes the issue. I am not entirely sure if this is specific to my setup. However, as the power state is toggled conditionally on LINK_ADDRESS timeouts, this should not affect the working cases.
With stuff like this, I always wonder if catering for a failing setup blocks us from improving working setups, because once we add this, we can't regress it. For example, is there a valid scenario where we'd want to fail fast here, instead of retrying?
Link address failure would result in not probing a branch device that already has been detected. I guess the fail fast case will be applicable if the said device was not really present but the parent branch told us otherwise.
The other option is we could check the device ID of the dock and implement this as a quirk.
Btw I found some relevant information in the spec.
"The Message Transaction originator must perform the reply timeout check. If an error to a request causes the system to be in an invalid state (e.g., all nodes failed to delete a virtual channel, it is the responsibility of the Message Transaction originator to return the system to a valid state). The Message Transaction originator is responsible for any retries."
and
"SET_POWER_STATE Must be programmed to 001 (binary) upon power-on reset or an upstream device disconnect. 001 = Set local Sink device and all downstream Sink devices to D0 (normal operation mode)."
I wonder if the dock is missing this step because the monitor seems to work well with a discrete MST hub.
Some nits below.
Cc: Lyude lyude@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 70dcfa58d3c2..e06defcdcf18 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1596,8 +1596,9 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, int len; struct drm_dp_sideband_msg_tx *txmsg; int ret;
- int attempts = 5;
- txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
+retry: txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); if (!txmsg) return;
@@ -1635,9 +1636,17 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, } (*mgr->cbs->hotplug)(mgr); }
- } else if (attempts--) {
You'll end up doing (attempts + 1) attempts, including the first one.
Yeah, that is what I intended to do :) I renamed it from 'retry' to 'attempt' at the last moment, which made it a bit confusing I suppose.
I am stressing testing my setup more to see how well this recovery works and update this patch.
Here's what I got with 266 rounds of reboots. Correct link address at 1st try 180 Retry 1 45 Retry 2 32 Retry 3 0 Retry 4 0 Retry 5 2 Giving up 3 Incorrect link address 4
The retries help about 30% of the cases.
kfree(txmsg);How about memset(txmsg, 0, sizoef(*txmsg)); here and move the goto label down to avoid repeated allocations?
Absolutely.
drm_dp_send_power_updown_phy(mstb->mgr, mstb->port_parent,false);drm_dp_send_power_updown_phy(mstb->mgr, mstb->port_parent,true);DRM_DEBUG_KMS("link address failed %d, retrying\n", ret);Maybe do the debug message before you power down/up?
Ok.
BR, Jani.
goto retry;
DRM_DEBUG_KMS("link address failed %d\n", ret);
DRM_DEBUG_KMS("link address failed %d, giving up\n", ret);}
kfree(txmsg);
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx