[Intel-gfx] [PATCH] drm/dp: Power cycle display if LINK_ADDRESS fails.

[Pandiyan, Dhinakaran]

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 at 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 at redhat.com>
> > > Cc: Dave Airlie <airlied at redhat.com>
> > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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;
> > >  	} 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);
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx