On Thu, 2017-12-21 at 08:53 +0200, Jani Nikula wrote:
> On Wed, 20 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Dave Airlie <airlied@xxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > 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;
> > } else {
> > mstb->link_address_sent = false;
> > - DRM_DEBUG_KMS("link address failed %d\n", ret);
> > + DRM_DEBUG_KMS("link address failed %d, giving up\n", ret);
> > }
> >
> > kfree(txmsg);
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
