Re: [PATCH RESEND] drm/udl: fail fast on stalled URB pool

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 28.10.26 um 07:13 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> A stalled device never returns its URBs, so every allocation waited
> out the full timeout and stalled the display stack on each operation.
> Flag the stall and fail fast until an URB comes back; give damage a
> short timeout and keep the long one for modeset.

I'm not comfortable with merging this change. It's nigh impossible to 
test.  It could also break a working set up if there's some temporary 
contention on the USB.

Best regards
Thomas


>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>   drivers/gpu/drm/udl/udl_drv.h      | 14 +++++++++++++-
>   drivers/gpu/drm/udl/udl_main.c     | 30 +++++++++++++++++++++++++-----
>   drivers/gpu/drm/udl/udl_modeset.c  |  6 +++---
>   drivers/gpu/drm/udl/udl_transfer.c |  2 +-
>   4 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 145bb95ccc48..43cff97e12f7 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -11,6 +11,7 @@
>   #ifndef UDL_DRV_H
>   #define UDL_DRV_H
>   
> +#include <linux/jiffies.h>
>   #include <linux/mm_types.h>
>   #include <linux/usb.h>
>   
> @@ -46,6 +47,7 @@ struct urb_list {
>   	int available;
>   	int count;
>   	size_t size;
> +	bool wedged;
>   };
>   
>   struct udl_device {
> @@ -72,7 +74,17 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
>   int udl_modeset_init(struct udl_device *udl);
>   struct drm_connector *udl_connector_init(struct drm_device *dev);
>   
> -struct urb *udl_get_urb(struct udl_device *udl);
> +/*
> + * How long to wait for a free URB before giving up.  The damage path runs
> + * on every atomic update, so keep its timeout short: a stalled device never
> + * returns its URBs and the wait would otherwise stall the whole display
> + * stack on every operation.  Modeset is low-frequency and worth waiting
> + * longer for.  Once the pool is flagged wedged, neither path blocks at all.
> + */
> +#define UDL_GET_URB_TIMEOUT_DAMAGE	msecs_to_jiffies(100)
> +#define UDL_GET_URB_TIMEOUT_MODESET	msecs_to_jiffies(2000)
> +
> +struct urb *udl_get_urb(struct udl_device *udl, long timeout);
>   
>   int udl_submit_urb(struct udl_device *udl, struct urb *urb, size_t len);
>   void udl_sync_pending_urbs(struct udl_device *udl);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 17950fe3a0ec..b92cd5300030 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -169,6 +169,7 @@ void udl_urb_completion(struct urb *urb)
>   	spin_lock_irqsave(&udl->urbs.lock, flags);
>   	list_add_tail(&unode->entry, &udl->urbs.list);
>   	udl->urbs.available++;
> +	udl->urbs.wedged = false;
>   	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>   
>   	wake_up(&udl->urbs.sleep);
> @@ -213,6 +214,7 @@ static int udl_alloc_urb_list(struct udl_device *udl, int count, size_t size)
>   	init_waitqueue_head(&udl->urbs.sleep);
>   	udl->urbs.count = 0;
>   	udl->urbs.available = 0;
> +	udl->urbs.wedged = false;
>   
>   retry:
>   	udl->urbs.size = size;
> @@ -265,16 +267,34 @@ static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
>   
>   	assert_spin_locked(&udl->urbs.lock);
>   
> +	/* A device that stopped completing transfers will never wake us up:
> +	 * fail fast instead of stalling callers for the full timeout on every
> +	 * call.  Allocations for teardown (MAX_SCHEDULE_TIMEOUT) still wait,
> +	 * since the USB core returns all URBs on disconnect.
> +	 */
> +	if (udl->urbs.wedged && timeout != MAX_SCHEDULE_TIMEOUT) {
> +		if (list_empty(&udl->urbs.list))
> +			return NULL;
> +		goto take;
> +	}
> +
>   	/* Wait for an in-flight buffer to complete and get re-queued */
>   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>   					 !udl->urbs.count ||
>   					 !list_empty(&udl->urbs.list),
>   					 udl->urbs.lock, timeout)) {
> -		DRM_INFO("wait for urb interrupted: available: %d\n",
> -			 udl->urbs.available);
> +		if (timeout == MAX_SCHEDULE_TIMEOUT) {
> +			DRM_INFO("wait for urb interrupted: available: %d\n",
> +				 udl->urbs.available);
> +		} else {
> +			udl->urbs.wedged = true;
> +			drm_warn_ratelimited(&udl->drm,
> +				"no URB returned within %u ms (available %d); device stalled, dropping frames\n",
> +				jiffies_to_msecs(timeout), udl->urbs.available);
> +		}
>   		return NULL;
>   	}
> -
> +take:
>   	if (!udl->urbs.count)
>   		return NULL;
>   
> @@ -285,12 +305,12 @@ static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
>   	return unode->urb;
>   }
>   
> -struct urb *udl_get_urb(struct udl_device *udl)
> +struct urb *udl_get_urb(struct udl_device *udl, long timeout)
>   {
>   	struct urb *urb;
>   
>   	spin_lock_irq(&udl->urbs.lock);
> -	urb = udl_get_urb_locked(udl, HZ * 2);
> +	urb = udl_get_urb_locked(udl, timeout);
>   	spin_unlock_irq(&udl->urbs.lock);
>   	return urb;
>   }
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 289711035b67..c907846048f8 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -218,7 +218,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>   		return ret;
>   	log_bpp = ret;
>   
> -	urb = udl_get_urb(udl);
> +	urb = udl_get_urb(udl, UDL_GET_URB_TIMEOUT_DAMAGE);
>   	if (!urb)
>   		return -ENOMEM;
>   	cmd = urb->transfer_buffer;
> @@ -342,7 +342,7 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
>   	if (!drm_dev_enter(dev, &idx))
>   		return;
>   
> -	urb = udl_get_urb(udl);
> +	urb = udl_get_urb(udl, UDL_GET_URB_TIMEOUT_MODESET);
>   	if (!urb) {
>   		drm_err_ratelimited(dev, "get urb failed when enabling crtc\n");
>   		goto out;
> @@ -377,7 +377,7 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
>   	if (!drm_dev_enter(dev, &idx))
>   		return;
>   
> -	urb = udl_get_urb(udl);
> +	urb = udl_get_urb(udl, UDL_GET_URB_TIMEOUT_MODESET);
>   	if (!urb)
>   		goto out;
>   
> diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
> index 7d670b3a5293..bfff17bba58d 100644
> --- a/drivers/gpu/drm/udl/udl_transfer.c
> +++ b/drivers/gpu/drm/udl/udl_transfer.c
> @@ -202,7 +202,7 @@ int udl_render_hline(struct udl_device *udl, int log_bpp, struct urb **urb_ptr,
>   			int ret = udl_submit_urb(udl, urb, len);
>   			if (ret)
>   				return ret;
> -			urb = udl_get_urb(udl);
> +			urb = udl_get_urb(udl, UDL_GET_URB_TIMEOUT_DAMAGE);
>   			if (!urb)
>   				return -EAGAIN;
>   			*urb_ptr = urb;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)