Re: [PATCH] drm/dp: Power cycle display if LINK_ADDRESS fails.

[Jani Nikula <jani.nikula@intel.com>]

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?

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.

> +		kfree(txmsg);

How about memset(txmsg, 0, sizoef(*txmsg)); here and move the goto label
down to avoid repeated allocations?

> +		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?

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx