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.
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.
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);