[PATCH 0/4] drm/udl: Fix stray URBs and cleanup

[PATCH 0/4] drm/udl: Fix stray URBs and cleanup

[Takashi Iwai]

Hi,

this is a series of fixes for UDL driver to address the leftover URBs
at suspend/resume.  It begins with the simplification of FIFO control
code with the standard wait queue, followed by the handling of pending
URBs, and replace BUG_ON() with WARN_ON() as a cleanup.


Takashi

===

Takashi Iwai (4):
  drm/udl: Replace semaphore with a simple wait queue
  drm/udl: Sync pending URBs at suspend / disconnect
  drm/udl: Kill pending URBs at suspend and disconnect
  drm/udl: Replace BUG_ON() with WARN_ON()

 drivers/gpu/drm/udl/udl_drv.h      |  14 +++-
 drivers/gpu/drm/udl/udl_main.c     | 125 ++++++++++++++---------------
 drivers/gpu/drm/udl/udl_modeset.c  |   4 +
 drivers/gpu/drm/udl/udl_transfer.c |   3 +-
 4 files changed, 79 insertions(+), 67 deletions(-)

-- 
2.35.3

[PATCH 0/4] drm/udl: Fix stray URBs and cleanup

[Takashi Iwai]

Hi,

this is a series of fixes for UDL driver to address the leftover URBs
at suspend/resume.  It begins with the simplification of FIFO control
code with the standard wait queue, followed by the handling of pending
URBs, and replace BUG_ON() with WARN_ON() as a cleanup.


Takashi

===

Takashi Iwai (4):
  drm/udl: Replace semaphore with a simple wait queue
  drm/udl: Sync pending URBs at suspend / disconnect
  drm/udl: Kill pending URBs at suspend and disconnect
  drm/udl: Replace BUG_ON() with WARN_ON()

 drivers/gpu/drm/udl/udl_drv.h      |  14 +++-
 drivers/gpu/drm/udl/udl_main.c     | 125 ++++++++++++++---------------
 drivers/gpu/drm/udl/udl_modeset.c  |   4 +
 drivers/gpu/drm/udl/udl_transfer.c |   3 +-
 4 files changed, 79 insertions(+), 67 deletions(-)

-- 
2.35.3

[PATCH 1/4] drm/udl: Replace semaphore with a simple wait queue

[Takashi Iwai]

UDL driver uses a semaphore for controlling the emptiness of FIFO in a
slightly funky way.  This patch replaces it with a wait queue and
controls the emptiness with the standard wait_event*() macro instead,
which is a more straightforward implementation.

While we are at it, drop the dead code for delayed semaphore down,
too.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h  | 11 +++--
 drivers/gpu/drm/udl/udl_main.c | 84 ++++++++++------------------------
 2 files changed, 31 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index cc16a13316e4..e008686ec738 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -34,14 +34,13 @@ struct udl_device;
 struct urb_node {
 	struct list_head entry;
 	struct udl_device *dev;
-	struct delayed_work release_urb_work;
 	struct urb *urb;
 };
 
 struct urb_list {
 	struct list_head list;
 	spinlock_t lock;
-	struct semaphore limit_sem;
+	wait_queue_head_t sleep;
 	int available;
 	int count;
 	size_t size;
@@ -75,7 +74,13 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
 int udl_modeset_init(struct drm_device *dev);
 struct drm_connector *udl_connector_init(struct drm_device *dev);
 
-struct urb *udl_get_urb(struct drm_device *dev);
+struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
+
+#define GET_URB_TIMEOUT	HZ
+static inline struct urb *udl_get_urb(struct drm_device *dev)
+{
+	return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
+}
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
 void udl_urb_completion(struct urb *urb);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 853f147036f6..67fd41e59b80 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,9 +23,6 @@
 #define WRITES_IN_FLIGHT (4)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
-#define GET_URB_TIMEOUT	HZ
-#define FREE_URB_TIMEOUT (HZ*2)
-
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -119,14 +116,6 @@ static int udl_select_std_channel(struct udl_device *udl)
 	return ret < 0 ? ret : 0;
 }
 
-static void udl_release_urb_work(struct work_struct *work)
-{
-	struct urb_node *unode = container_of(work, struct urb_node,
-					      release_urb_work.work);
-
-	up(&unode->dev->urbs.limit_sem);
-}
-
 void udl_urb_completion(struct urb *urb)
 {
 	struct urb_node *unode = urb->context;
@@ -150,23 +139,13 @@ void udl_urb_completion(struct urb *urb)
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
-#if 0
-	/*
-	 * When using fb_defio, we deadlock if up() is called
-	 * while another is waiting. So queue to another process.
-	 */
-	if (fb_defio)
-		schedule_delayed_work(&unode->release_urb_work, 0);
-	else
-#endif
-		up(&udl->urbs.limit_sem);
+	wake_up(&udl->urbs.sleep);
 }
 
 static void udl_free_urb_list(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
 	int count = udl->urbs.count;
-	struct list_head *node;
 	struct urb_node *unode;
 	struct urb *urb;
 
@@ -174,23 +153,15 @@ static void udl_free_urb_list(struct drm_device *dev)
 
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
-		down(&udl->urbs.limit_sem);
-
-		spin_lock_irq(&udl->urbs.lock);
-
-		node = udl->urbs.list.next; /* have reserved one with sem */
-		list_del_init(node);
-
-		spin_unlock_irq(&udl->urbs.lock);
-
-		unode = list_entry(node, struct urb_node, entry);
-		urb = unode->urb;
-
+		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+		if (WARN_ON(!urb))
+			break;
+		unode = urb->context;
 		/* Free each separately allocated piece */
 		usb_free_coherent(urb->dev, udl->urbs.size,
 				  urb->transfer_buffer, urb->transfer_dma);
 		usb_free_urb(urb);
-		kfree(node);
+		kfree(unode);
 	}
 	udl->urbs.count = 0;
 }
@@ -210,7 +181,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
 
-	sema_init(&udl->urbs.limit_sem, 0);
+	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
 	udl->urbs.available = 0;
 
@@ -220,9 +191,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 			break;
 		unode->dev = udl;
 
-		INIT_DELAYED_WORK(&unode->release_urb_work,
-			  udl_release_urb_work);
-
 		urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!urb) {
 			kfree(unode);
@@ -250,7 +218,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 
 		list_add_tail(&unode->entry, &udl->urbs.list);
 
-		up(&udl->urbs.limit_sem);
 		udl->urbs.count++;
 		udl->urbs.available++;
 	}
@@ -260,36 +227,31 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	return udl->urbs.count;
 }
 
-struct urb *udl_get_urb(struct drm_device *dev)
+struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
 {
 	struct udl_device *udl = to_udl(dev);
-	int ret = 0;
-	struct list_head *entry;
-	struct urb_node *unode;
-	struct urb *urb = NULL;
+	struct urb_node *unode = NULL;
 
-	/* Wait for an in-flight buffer to complete and get re-queued */
-	ret = down_timeout(&udl->urbs.limit_sem, GET_URB_TIMEOUT);
-	if (ret) {
-		DRM_INFO("wait for urb interrupted: %x available: %d\n",
-		       ret, udl->urbs.available);
-		goto error;
-	}
+	if (!udl->urbs.count)
+		return NULL;
 
+	/* Wait for an in-flight buffer to complete and get re-queued */
 	spin_lock_irq(&udl->urbs.lock);
+	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+					 !list_empty(&udl->urbs.list),
+					 udl->urbs.lock, timeout)) {
+		DRM_INFO("wait for urb interrupted: available: %d\n",
+			 udl->urbs.available);
+		goto unlock;
+	}
 
-	BUG_ON(list_empty(&udl->urbs.list)); /* reserved one with limit_sem */
-	entry = udl->urbs.list.next;
-	list_del_init(entry);
+	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
+	list_del_init(&unode->entry);
 	udl->urbs.available--;
 
+unlock:
 	spin_unlock_irq(&udl->urbs.lock);
-
-	unode = list_entry(entry, struct urb_node, entry);
-	urb = unode->urb;
-
-error:
-	return urb;
+	return unode ? unode->urb : NULL;
 }
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
-- 
2.35.3

[PATCH 1/4] drm/udl: Replace semaphore with a simple wait queue

[Takashi Iwai]

UDL driver uses a semaphore for controlling the emptiness of FIFO in a
slightly funky way.  This patch replaces it with a wait queue and
controls the emptiness with the standard wait_event*() macro instead,
which is a more straightforward implementation.

While we are at it, drop the dead code for delayed semaphore down,
too.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h  | 11 +++--
 drivers/gpu/drm/udl/udl_main.c | 84 ++++++++++------------------------
 2 files changed, 31 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index cc16a13316e4..e008686ec738 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -34,14 +34,13 @@ struct udl_device;
 struct urb_node {
 	struct list_head entry;
 	struct udl_device *dev;
-	struct delayed_work release_urb_work;
 	struct urb *urb;
 };
 
 struct urb_list {
 	struct list_head list;
 	spinlock_t lock;
-	struct semaphore limit_sem;
+	wait_queue_head_t sleep;
 	int available;
 	int count;
 	size_t size;
@@ -75,7 +74,13 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
 int udl_modeset_init(struct drm_device *dev);
 struct drm_connector *udl_connector_init(struct drm_device *dev);
 
-struct urb *udl_get_urb(struct drm_device *dev);
+struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
+
+#define GET_URB_TIMEOUT	HZ
+static inline struct urb *udl_get_urb(struct drm_device *dev)
+{
+	return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
+}
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
 void udl_urb_completion(struct urb *urb);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 853f147036f6..67fd41e59b80 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,9 +23,6 @@
 #define WRITES_IN_FLIGHT (4)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
-#define GET_URB_TIMEOUT	HZ
-#define FREE_URB_TIMEOUT (HZ*2)
-
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -119,14 +116,6 @@ static int udl_select_std_channel(struct udl_device *udl)
 	return ret < 0 ? ret : 0;
 }
 
-static void udl_release_urb_work(struct work_struct *work)
-{
-	struct urb_node *unode = container_of(work, struct urb_node,
-					      release_urb_work.work);
-
-	up(&unode->dev->urbs.limit_sem);
-}
-
 void udl_urb_completion(struct urb *urb)
 {
 	struct urb_node *unode = urb->context;
@@ -150,23 +139,13 @@ void udl_urb_completion(struct urb *urb)
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
-#if 0
-	/*
-	 * When using fb_defio, we deadlock if up() is called
-	 * while another is waiting. So queue to another process.
-	 */
-	if (fb_defio)
-		schedule_delayed_work(&unode->release_urb_work, 0);
-	else
-#endif
-		up(&udl->urbs.limit_sem);
+	wake_up(&udl->urbs.sleep);
 }
 
 static void udl_free_urb_list(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
 	int count = udl->urbs.count;
-	struct list_head *node;
 	struct urb_node *unode;
 	struct urb *urb;
 
@@ -174,23 +153,15 @@ static void udl_free_urb_list(struct drm_device *dev)
 
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
-		down(&udl->urbs.limit_sem);
-
-		spin_lock_irq(&udl->urbs.lock);
-
-		node = udl->urbs.list.next; /* have reserved one with sem */
-		list_del_init(node);
-
-		spin_unlock_irq(&udl->urbs.lock);
-
-		unode = list_entry(node, struct urb_node, entry);
-		urb = unode->urb;
-
+		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+		if (WARN_ON(!urb))
+			break;
+		unode = urb->context;
 		/* Free each separately allocated piece */
 		usb_free_coherent(urb->dev, udl->urbs.size,
 				  urb->transfer_buffer, urb->transfer_dma);
 		usb_free_urb(urb);
-		kfree(node);
+		kfree(unode);
 	}
 	udl->urbs.count = 0;
 }
@@ -210,7 +181,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
 
-	sema_init(&udl->urbs.limit_sem, 0);
+	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
 	udl->urbs.available = 0;
 
@@ -220,9 +191,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 			break;
 		unode->dev = udl;
 
-		INIT_DELAYED_WORK(&unode->release_urb_work,
-			  udl_release_urb_work);
-
 		urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!urb) {
 			kfree(unode);
@@ -250,7 +218,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 
 		list_add_tail(&unode->entry, &udl->urbs.list);
 
-		up(&udl->urbs.limit_sem);
 		udl->urbs.count++;
 		udl->urbs.available++;
 	}
@@ -260,36 +227,31 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	return udl->urbs.count;
 }
 
-struct urb *udl_get_urb(struct drm_device *dev)
+struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
 {
 	struct udl_device *udl = to_udl(dev);
-	int ret = 0;
-	struct list_head *entry;
-	struct urb_node *unode;
-	struct urb *urb = NULL;
+	struct urb_node *unode = NULL;
 
-	/* Wait for an in-flight buffer to complete and get re-queued */
-	ret = down_timeout(&udl->urbs.limit_sem, GET_URB_TIMEOUT);
-	if (ret) {
-		DRM_INFO("wait for urb interrupted: %x available: %d\n",
-		       ret, udl->urbs.available);
-		goto error;
-	}
+	if (!udl->urbs.count)
+		return NULL;
 
+	/* Wait for an in-flight buffer to complete and get re-queued */
 	spin_lock_irq(&udl->urbs.lock);
+	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+					 !list_empty(&udl->urbs.list),
+					 udl->urbs.lock, timeout)) {
+		DRM_INFO("wait for urb interrupted: available: %d\n",
+			 udl->urbs.available);
+		goto unlock;
+	}
 
-	BUG_ON(list_empty(&udl->urbs.list)); /* reserved one with limit_sem */
-	entry = udl->urbs.list.next;
-	list_del_init(entry);
+	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
+	list_del_init(&unode->entry);
 	udl->urbs.available--;
 
+unlock:
 	spin_unlock_irq(&udl->urbs.lock);
-
-	unode = list_entry(entry, struct urb_node, entry);
-	urb = unode->urb;
-
-error:
-	return urb;
+	return unode ? unode->urb : NULL;
 }
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
-- 
2.35.3

[PATCH 2/4] drm/udl: Sync pending URBs at suspend / disconnect

[Takashi Iwai]

We need to wait for finishing to process the all URBs after disabling
the pipe; otherwise pending URBs may stray at suspend/resume, leading
to a possible memory corruption in a worst case.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  1 +
 drivers/gpu/drm/udl/udl_main.c    | 17 +++++++++++++++++
 drivers/gpu/drm/udl/udl_modeset.c |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e008686ec738..f01e50c5b7b7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -83,6 +83,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
 }
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
+int udl_sync_pending_urbs(struct drm_device *dev);
 void udl_urb_completion(struct urb *urb);
 
 int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 67fd41e59b80..93615648414b 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -270,6 +270,23 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	return ret;
 }
 
+/* wait until all pending URBs have been processed */
+int udl_sync_pending_urbs(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	int ret = 0;
+
+	spin_lock_irq(&udl->urbs.lock);
+	/* 2 seconds as a sane timeout */
+	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+					 udl->urbs.available == udl->urbs.count,
+					 udl->urbs.lock,
+					 msecs_to_jiffies(2000)))
+		ret = -ETIMEDOUT;
+	spin_unlock_irq(&udl->urbs.lock);
+	return ret;
+}
+
 int udl_init(struct udl_device *udl)
 {
 	struct drm_device *dev = &udl->drm;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index e67c40a48fb4..50025606b6ad 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -408,6 +408,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	buf = udl_dummy_render(buf);
 
 	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+
+	udl_sync_pending_urbs(dev);
 }
 
 static void
-- 
2.35.3

[PATCH 2/4] drm/udl: Sync pending URBs at suspend / disconnect

[Takashi Iwai]

We need to wait for finishing to process the all URBs after disabling
the pipe; otherwise pending URBs may stray at suspend/resume, leading
to a possible memory corruption in a worst case.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  1 +
 drivers/gpu/drm/udl/udl_main.c    | 17 +++++++++++++++++
 drivers/gpu/drm/udl/udl_modeset.c |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e008686ec738..f01e50c5b7b7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -83,6 +83,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
 }
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
+int udl_sync_pending_urbs(struct drm_device *dev);
 void udl_urb_completion(struct urb *urb);
 
 int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 67fd41e59b80..93615648414b 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -270,6 +270,23 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	return ret;
 }
 
+/* wait until all pending URBs have been processed */
+int udl_sync_pending_urbs(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	int ret = 0;
+
+	spin_lock_irq(&udl->urbs.lock);
+	/* 2 seconds as a sane timeout */
+	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+					 udl->urbs.available == udl->urbs.count,
+					 udl->urbs.lock,
+					 msecs_to_jiffies(2000)))
+		ret = -ETIMEDOUT;
+	spin_unlock_irq(&udl->urbs.lock);
+	return ret;
+}
+
 int udl_init(struct udl_device *udl)
 {
 	struct drm_device *dev = &udl->drm;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index e67c40a48fb4..50025606b6ad 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -408,6 +408,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	buf = udl_dummy_render(buf);
 
 	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+
+	udl_sync_pending_urbs(dev);
 }
 
 static void
-- 
2.35.3

[PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

At both suspend and disconnect, we should rather cancel the pending
URBs immediately.  For the suspend case, the display will be turned
off, so it makes no sense to process the rendering.  And for the
disconnect case, the device may be no longer accessible, hence we
shouldn't do any submission.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  2 ++
 drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
 drivers/gpu/drm/udl/udl_modeset.c |  2 ++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index f01e50c5b7b7..28aaf75d71cf 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,6 +39,7 @@ struct urb_node {
 
 struct urb_list {
 	struct list_head list;
+	struct list_head in_flight;
 	spinlock_t lock;
 	wait_queue_head_t sleep;
 	int available;
@@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
 int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_kill_pending_urbs(struct drm_device *dev);
 void udl_urb_completion(struct urb *urb);
 
 int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 93615648414b..47204b7eb10e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
 	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
 
 	spin_lock_irqsave(&udl->urbs.lock, flags);
-	list_add_tail(&unode->entry, &udl->urbs.list);
+	list_move(&unode->entry, &udl->urbs.list);
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
@@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 retry:
 	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
+	INIT_LIST_HEAD(&udl->urbs.in_flight);
 
 	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
@@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
 	}
 
 	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
-	list_del_init(&unode->entry);
+	list_move(&unode->entry, &udl->urbs.in_flight);
 	udl->urbs.available--;
 
 unlock:
@@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 	spin_lock_irq(&udl->urbs.lock);
 	/* 2 seconds as a sane timeout */
 	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-					 udl->urbs.available == udl->urbs.count,
+					 list_empty(&udl->urbs.in_flight),
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
 		ret = -ETIMEDOUT;
@@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 	return ret;
 }
 
+/* kill pending URBs */
+void udl_kill_pending_urbs(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	struct urb_node *unode;
+
+	spin_lock_irq(&udl->urbs.lock);
+	while (!list_empty(&udl->urbs.in_flight)) {
+		unode = list_first_entry(&udl->urbs.in_flight,
+					 struct urb_node, entry);
+		spin_unlock_irq(&udl->urbs.lock);
+		usb_kill_urb(unode->urb);
+		spin_lock_irq(&udl->urbs.lock);
+	}
+	spin_unlock_irq(&udl->urbs.lock);
+}
+
 int udl_init(struct udl_device *udl)
 {
 	struct drm_device *dev = &udl->drm;
@@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
 
+	udl_kill_pending_urbs(dev);
 	udl_free_urb_list(dev);
 	put_device(udl->dmadev);
 	udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 50025606b6ad..169110d8fc2e 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	struct urb *urb;
 	char *buf;
 
+	udl_kill_pending_urbs(dev);
+
 	urb = udl_get_urb(dev);
 	if (!urb)
 		return;
-- 
2.35.3

[PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

At both suspend and disconnect, we should rather cancel the pending
URBs immediately.  For the suspend case, the display will be turned
off, so it makes no sense to process the rendering.  And for the
disconnect case, the device may be no longer accessible, hence we
shouldn't do any submission.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  2 ++
 drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
 drivers/gpu/drm/udl/udl_modeset.c |  2 ++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index f01e50c5b7b7..28aaf75d71cf 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,6 +39,7 @@ struct urb_node {
 
 struct urb_list {
 	struct list_head list;
+	struct list_head in_flight;
 	spinlock_t lock;
 	wait_queue_head_t sleep;
 	int available;
@@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
 int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_kill_pending_urbs(struct drm_device *dev);
 void udl_urb_completion(struct urb *urb);
 
 int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 93615648414b..47204b7eb10e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
 	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
 
 	spin_lock_irqsave(&udl->urbs.lock, flags);
-	list_add_tail(&unode->entry, &udl->urbs.list);
+	list_move(&unode->entry, &udl->urbs.list);
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
@@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 retry:
 	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
+	INIT_LIST_HEAD(&udl->urbs.in_flight);
 
 	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
@@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
 	}
 
 	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
-	list_del_init(&unode->entry);
+	list_move(&unode->entry, &udl->urbs.in_flight);
 	udl->urbs.available--;
 
 unlock:
@@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 	spin_lock_irq(&udl->urbs.lock);
 	/* 2 seconds as a sane timeout */
 	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-					 udl->urbs.available == udl->urbs.count,
+					 list_empty(&udl->urbs.in_flight),
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
 		ret = -ETIMEDOUT;
@@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 	return ret;
 }
 
+/* kill pending URBs */
+void udl_kill_pending_urbs(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	struct urb_node *unode;
+
+	spin_lock_irq(&udl->urbs.lock);
+	while (!list_empty(&udl->urbs.in_flight)) {
+		unode = list_first_entry(&udl->urbs.in_flight,
+					 struct urb_node, entry);
+		spin_unlock_irq(&udl->urbs.lock);
+		usb_kill_urb(unode->urb);
+		spin_lock_irq(&udl->urbs.lock);
+	}
+	spin_unlock_irq(&udl->urbs.lock);
+}
+
 int udl_init(struct udl_device *udl)
 {
 	struct drm_device *dev = &udl->drm;
@@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
 
+	udl_kill_pending_urbs(dev);
 	udl_free_urb_list(dev);
 	put_device(udl->dmadev);
 	udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 50025606b6ad..169110d8fc2e 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	struct urb *urb;
 	char *buf;
 
+	udl_kill_pending_urbs(dev);
+
 	urb = udl_get_urb(dev);
 	if (!urb)
 		return;
-- 
2.35.3

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5074 bytes --]

Hi

Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> At both suspend and disconnect, we should rather cancel the pending
> URBs immediately.  For the suspend case, the display will be turned
> off, so it makes no sense to process the rendering.  And for the
> disconnect case, the device may be no longer accessible, hence we
> shouldn't do any submission.
> 
> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_drv.h     |  2 ++
>   drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
>   drivers/gpu/drm/udl/udl_modeset.c |  2 ++
>   3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index f01e50c5b7b7..28aaf75d71cf 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -39,6 +39,7 @@ struct urb_node {
>   
>   struct urb_list {
>   	struct list_head list;
> +	struct list_head in_flight;
>   	spinlock_t lock;
>   	wait_queue_head_t sleep;
>   	int available;
> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
>   
>   int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
>   int udl_sync_pending_urbs(struct drm_device *dev);
> +void udl_kill_pending_urbs(struct drm_device *dev);
>   void udl_urb_completion(struct urb *urb);
>   
>   int udl_init(struct udl_device *udl);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 93615648414b..47204b7eb10e 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
>   	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
>   
>   	spin_lock_irqsave(&udl->urbs.lock, flags);
> -	list_add_tail(&unode->entry, &udl->urbs.list);
> +	list_move(&unode->entry, &udl->urbs.list);
>   	udl->urbs.available++;
>   	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>   
> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   retry:
>   	udl->urbs.size = size;
>   	INIT_LIST_HEAD(&udl->urbs.list);
> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
>   
>   	init_waitqueue_head(&udl->urbs.sleep);
>   	udl->urbs.count = 0;
> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
>   	}
>   
>   	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
> -	list_del_init(&unode->entry);
> +	list_move(&unode->entry, &udl->urbs.in_flight);
>   	udl->urbs.available--;
>   
>   unlock:
> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>   	spin_lock_irq(&udl->urbs.lock);
>   	/* 2 seconds as a sane timeout */
>   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> -					 udl->urbs.available == udl->urbs.count,
> +					 list_empty(&udl->urbs.in_flight),
>   					 udl->urbs.lock,
>   					 msecs_to_jiffies(2000)))
>   		ret = -ETIMEDOUT;
> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>   	return ret;
>   }
>   
> +/* kill pending URBs */
> +void udl_kill_pending_urbs(struct drm_device *dev)
> +{
> +	struct udl_device *udl = to_udl(dev);
> +	struct urb_node *unode;
> +
> +	spin_lock_irq(&udl->urbs.lock);
> +	while (!list_empty(&udl->urbs.in_flight)) {
> +		unode = list_first_entry(&udl->urbs.in_flight,
> +					 struct urb_node, entry);
> +		spin_unlock_irq(&udl->urbs.lock);
> +		usb_kill_urb(unode->urb);
> +		spin_lock_irq(&udl->urbs.lock);
> +	}
> +	spin_unlock_irq(&udl->urbs.lock);
> +}
> +
>   int udl_init(struct udl_device *udl)
>   {
>   	struct drm_device *dev = &udl->drm;
> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
>   {
>   	struct udl_device *udl = to_udl(dev);
>   
> +	udl_kill_pending_urbs(dev);
>   	udl_free_urb_list(dev);
>   	put_device(udl->dmadev);
>   	udl->dmadev = NULL;
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 50025606b6ad..169110d8fc2e 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>   	struct urb *urb;
>   	char *buf;
>   
> +	udl_kill_pending_urbs(dev);
> +

I already reviewed the patchset, but I have another comment. I think we 
should only kill urbs from within the suspend handler. Same for the call 
to the URB-sync function in patch 2.

This disable function is part of the regular modeset path. It's probably 
not appropriate to outright remove pending URBs here. This can lead to 
failed modesets, which would have succeeded otherwise.

Best regards
Thomas

>   	urb = udl_get_urb(dev);
>   	if (!urb)
>   		return;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 09:13:16 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> > At both suspend and disconnect, we should rather cancel the pending
> > URBs immediately.  For the suspend case, the display will be turned
> > off, so it makes no sense to process the rendering.  And for the
> > disconnect case, the device may be no longer accessible, hence we
> > shouldn't do any submission.
> > 
> > Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >   drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >   drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >   3 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index f01e50c5b7b7..28aaf75d71cf 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -39,6 +39,7 @@ struct urb_node {
> >     struct urb_list {
> >   	struct list_head list;
> > +	struct list_head in_flight;
> >   	spinlock_t lock;
> >   	wait_queue_head_t sleep;
> >   	int available;
> > @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >     int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > size_t len);
> >   int udl_sync_pending_urbs(struct drm_device *dev);
> > +void udl_kill_pending_urbs(struct drm_device *dev);
> >   void udl_urb_completion(struct urb *urb);
> >     int udl_init(struct udl_device *udl);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 93615648414b..47204b7eb10e 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >   	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >     	spin_lock_irqsave(&udl->urbs.lock, flags);
> > -	list_add_tail(&unode->entry, &udl->urbs.list);
> > +	list_move(&unode->entry, &udl->urbs.list);
> >   	udl->urbs.available++;
> >   	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >   @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> > drm_device *dev, int count, size_t size)
> >   retry:
> >   	udl->urbs.size = size;
> >   	INIT_LIST_HEAD(&udl->urbs.list);
> > +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >     	init_waitqueue_head(&udl->urbs.sleep);
> >   	udl->urbs.count = 0;
> > @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >   	}
> >     	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> > entry);
> > -	list_del_init(&unode->entry);
> > +	list_move(&unode->entry, &udl->urbs.in_flight);
> >   	udl->urbs.available--;
> >     unlock:
> > @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >   	spin_lock_irq(&udl->urbs.lock);
> >   	/* 2 seconds as a sane timeout */
> >   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> > -					 udl->urbs.available == udl->urbs.count,
> > +					 list_empty(&udl->urbs.in_flight),
> >   					 udl->urbs.lock,
> >   					 msecs_to_jiffies(2000)))
> >   		ret = -ETIMEDOUT;
> > @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >   	return ret;
> >   }
> >   +/* kill pending URBs */
> > +void udl_kill_pending_urbs(struct drm_device *dev)
> > +{
> > +	struct udl_device *udl = to_udl(dev);
> > +	struct urb_node *unode;
> > +
> > +	spin_lock_irq(&udl->urbs.lock);
> > +	while (!list_empty(&udl->urbs.in_flight)) {
> > +		unode = list_first_entry(&udl->urbs.in_flight,
> > +					 struct urb_node, entry);
> > +		spin_unlock_irq(&udl->urbs.lock);
> > +		usb_kill_urb(unode->urb);
> > +		spin_lock_irq(&udl->urbs.lock);
> > +	}
> > +	spin_unlock_irq(&udl->urbs.lock);
> > +}
> > +
> >   int udl_init(struct udl_device *udl)
> >   {
> >   	struct drm_device *dev = &udl->drm;
> > @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >   {
> >   	struct udl_device *udl = to_udl(dev);
> >   +	udl_kill_pending_urbs(dev);
> >   	udl_free_urb_list(dev);
> >   	put_device(udl->dmadev);
> >   	udl->dmadev = NULL;
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index 50025606b6ad..169110d8fc2e 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >   	struct urb *urb;
> >   	char *buf;
> >   +	udl_kill_pending_urbs(dev);
> > +
> 
> I already reviewed the patchset, but I have another comment. I think
> we should only kill urbs from within the suspend handler. Same for the
> call to the URB-sync function in patch 2.
> 
> This disable function is part of the regular modeset path. It's
> probably not appropriate to outright remove pending URBs here. This
> can lead to failed modesets, which would have succeeded otherwise.

Well, the device shall be turned off right after that point, so the
all pending rendering makes little sense, no?


Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 09:13:16 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> > At both suspend and disconnect, we should rather cancel the pending
> > URBs immediately.  For the suspend case, the display will be turned
> > off, so it makes no sense to process the rendering.  And for the
> > disconnect case, the device may be no longer accessible, hence we
> > shouldn't do any submission.
> > 
> > Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >   drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >   drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >   3 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index f01e50c5b7b7..28aaf75d71cf 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -39,6 +39,7 @@ struct urb_node {
> >     struct urb_list {
> >   	struct list_head list;
> > +	struct list_head in_flight;
> >   	spinlock_t lock;
> >   	wait_queue_head_t sleep;
> >   	int available;
> > @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >     int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > size_t len);
> >   int udl_sync_pending_urbs(struct drm_device *dev);
> > +void udl_kill_pending_urbs(struct drm_device *dev);
> >   void udl_urb_completion(struct urb *urb);
> >     int udl_init(struct udl_device *udl);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 93615648414b..47204b7eb10e 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >   	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >     	spin_lock_irqsave(&udl->urbs.lock, flags);
> > -	list_add_tail(&unode->entry, &udl->urbs.list);
> > +	list_move(&unode->entry, &udl->urbs.list);
> >   	udl->urbs.available++;
> >   	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >   @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> > drm_device *dev, int count, size_t size)
> >   retry:
> >   	udl->urbs.size = size;
> >   	INIT_LIST_HEAD(&udl->urbs.list);
> > +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >     	init_waitqueue_head(&udl->urbs.sleep);
> >   	udl->urbs.count = 0;
> > @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >   	}
> >     	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> > entry);
> > -	list_del_init(&unode->entry);
> > +	list_move(&unode->entry, &udl->urbs.in_flight);
> >   	udl->urbs.available--;
> >     unlock:
> > @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >   	spin_lock_irq(&udl->urbs.lock);
> >   	/* 2 seconds as a sane timeout */
> >   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> > -					 udl->urbs.available == udl->urbs.count,
> > +					 list_empty(&udl->urbs.in_flight),
> >   					 udl->urbs.lock,
> >   					 msecs_to_jiffies(2000)))
> >   		ret = -ETIMEDOUT;
> > @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >   	return ret;
> >   }
> >   +/* kill pending URBs */
> > +void udl_kill_pending_urbs(struct drm_device *dev)
> > +{
> > +	struct udl_device *udl = to_udl(dev);
> > +	struct urb_node *unode;
> > +
> > +	spin_lock_irq(&udl->urbs.lock);
> > +	while (!list_empty(&udl->urbs.in_flight)) {
> > +		unode = list_first_entry(&udl->urbs.in_flight,
> > +					 struct urb_node, entry);
> > +		spin_unlock_irq(&udl->urbs.lock);
> > +		usb_kill_urb(unode->urb);
> > +		spin_lock_irq(&udl->urbs.lock);
> > +	}
> > +	spin_unlock_irq(&udl->urbs.lock);
> > +}
> > +
> >   int udl_init(struct udl_device *udl)
> >   {
> >   	struct drm_device *dev = &udl->drm;
> > @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >   {
> >   	struct udl_device *udl = to_udl(dev);
> >   +	udl_kill_pending_urbs(dev);
> >   	udl_free_urb_list(dev);
> >   	put_device(udl->dmadev);
> >   	udl->dmadev = NULL;
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index 50025606b6ad..169110d8fc2e 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >   	struct urb *urb;
> >   	char *buf;
> >   +	udl_kill_pending_urbs(dev);
> > +
> 
> I already reviewed the patchset, but I have another comment. I think
> we should only kill urbs from within the suspend handler. Same for the
> call to the URB-sync function in patch 2.
> 
> This disable function is part of the regular modeset path. It's
> probably not appropriate to outright remove pending URBs here. This
> can lead to failed modesets, which would have succeeded otherwise.

Well, the device shall be turned off right after that point, so the
all pending rendering makes little sense, no?


Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5770 bytes --]

Hi

Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> On Tue, 09 Aug 2022 09:13:16 +0200,
> Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
>>> At both suspend and disconnect, we should rather cancel the pending
>>> URBs immediately.  For the suspend case, the display will be turned
>>> off, so it makes no sense to process the rendering.  And for the
>>> disconnect case, the device may be no longer accessible, hence we
>>> shouldn't do any submission.
>>>
>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>    drivers/gpu/drm/udl/udl_drv.h     |  2 ++
>>>    drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
>>>    drivers/gpu/drm/udl/udl_modeset.c |  2 ++
>>>    3 files changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>> index f01e50c5b7b7..28aaf75d71cf 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>> @@ -39,6 +39,7 @@ struct urb_node {
>>>      struct urb_list {
>>>    	struct list_head list;
>>> +	struct list_head in_flight;
>>>    	spinlock_t lock;
>>>    	wait_queue_head_t sleep;
>>>    	int available;
>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
>>>      int udl_submit_urb(struct drm_device *dev, struct urb *urb,
>>> size_t len);
>>>    int udl_sync_pending_urbs(struct drm_device *dev);
>>> +void udl_kill_pending_urbs(struct drm_device *dev);
>>>    void udl_urb_completion(struct urb *urb);
>>>      int udl_init(struct udl_device *udl);
>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>>> index 93615648414b..47204b7eb10e 100644
>>> --- a/drivers/gpu/drm/udl/udl_main.c
>>> +++ b/drivers/gpu/drm/udl/udl_main.c
>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
>>>    	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
>>>      	spin_lock_irqsave(&udl->urbs.lock, flags);
>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
>>> +	list_move(&unode->entry, &udl->urbs.list);
>>>    	udl->urbs.available++;
>>>    	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>>>    @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
>>> drm_device *dev, int count, size_t size)
>>>    retry:
>>>    	udl->urbs.size = size;
>>>    	INIT_LIST_HEAD(&udl->urbs.list);
>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
>>>      	init_waitqueue_head(&udl->urbs.sleep);
>>>    	udl->urbs.count = 0;
>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
>>>    	}
>>>      	unode = list_first_entry(&udl->urbs.list, struct urb_node,
>>> entry);
>>> -	list_del_init(&unode->entry);
>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
>>>    	udl->urbs.available--;
>>>      unlock:
>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>    	spin_lock_irq(&udl->urbs.lock);
>>>    	/* 2 seconds as a sane timeout */
>>>    	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>>> -					 udl->urbs.available == udl->urbs.count,
>>> +					 list_empty(&udl->urbs.in_flight),
>>>    					 udl->urbs.lock,
>>>    					 msecs_to_jiffies(2000)))
>>>    		ret = -ETIMEDOUT;
>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>    	return ret;
>>>    }
>>>    +/* kill pending URBs */
>>> +void udl_kill_pending_urbs(struct drm_device *dev)
>>> +{
>>> +	struct udl_device *udl = to_udl(dev);
>>> +	struct urb_node *unode;
>>> +
>>> +	spin_lock_irq(&udl->urbs.lock);
>>> +	while (!list_empty(&udl->urbs.in_flight)) {
>>> +		unode = list_first_entry(&udl->urbs.in_flight,
>>> +					 struct urb_node, entry);
>>> +		spin_unlock_irq(&udl->urbs.lock);
>>> +		usb_kill_urb(unode->urb);
>>> +		spin_lock_irq(&udl->urbs.lock);
>>> +	}
>>> +	spin_unlock_irq(&udl->urbs.lock);
>>> +}
>>> +
>>>    int udl_init(struct udl_device *udl)
>>>    {
>>>    	struct drm_device *dev = &udl->drm;
>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
>>>    {
>>>    	struct udl_device *udl = to_udl(dev);
>>>    +	udl_kill_pending_urbs(dev);
>>>    	udl_free_urb_list(dev);
>>>    	put_device(udl->dmadev);
>>>    	udl->dmadev = NULL;
>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>>> index 50025606b6ad..169110d8fc2e 100644
>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>    	struct urb *urb;
>>>    	char *buf;
>>>    +	udl_kill_pending_urbs(dev);
>>> +
>>
>> I already reviewed the patchset, but I have another comment. I think
>> we should only kill urbs from within the suspend handler. Same for the
>> call to the URB-sync function in patch 2.
>>
>> This disable function is part of the regular modeset path. It's
>> probably not appropriate to outright remove pending URBs here. This
>> can lead to failed modesets, which would have succeeded otherwise.
> 
> Well, the device shall be turned off right after that point, so the
> all pending rendering makes little sense, no?

udl_simple_display_pipe_disable() only disables the display, but not the 
device. The kill operation here could potentially kill some valid 
modeset operation that was still going on. And who knows what the device 
state is after that.

Best regards
Thomas

> 
> 
> Takashi

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 09:41:19 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:13:16 +0200,
> > Thomas Zimmermann wrote:
> >> 
> >> Hi
> >> 
> >> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>> At both suspend and disconnect, we should rather cancel the pending
> >>> URBs immediately.  For the suspend case, the display will be turned
> >>> off, so it makes no sense to process the rendering.  And for the
> >>> disconnect case, the device may be no longer accessible, hence we
> >>> shouldn't do any submission.
> >>> 
> >>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>    drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>    drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>    drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>    3 files changed, 26 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>      struct urb_list {
> >>>    	struct list_head list;
> >>> +	struct list_head in_flight;
> >>>    	spinlock_t lock;
> >>>    	wait_queue_head_t sleep;
> >>>    	int available;
> >>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>      int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>> size_t len);
> >>>    int udl_sync_pending_urbs(struct drm_device *dev);
> >>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>    void udl_urb_completion(struct urb *urb);
> >>>      int udl_init(struct udl_device *udl);
> >>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>> index 93615648414b..47204b7eb10e 100644
> >>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>    	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>      	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>    	udl->urbs.available++;
> >>>    	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>    @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>> drm_device *dev, int count, size_t size)
> >>>    retry:
> >>>    	udl->urbs.size = size;
> >>>    	INIT_LIST_HEAD(&udl->urbs.list);
> >>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>      	init_waitqueue_head(&udl->urbs.sleep);
> >>>    	udl->urbs.count = 0;
> >>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>    	}
> >>>      	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>> entry);
> >>> -	list_del_init(&unode->entry);
> >>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>    	udl->urbs.available--;
> >>>      unlock:
> >>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>    	spin_lock_irq(&udl->urbs.lock);
> >>>    	/* 2 seconds as a sane timeout */
> >>>    	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>> -					 udl->urbs.available == udl->urbs.count,
> >>> +					 list_empty(&udl->urbs.in_flight),
> >>>    					 udl->urbs.lock,
> >>>    					 msecs_to_jiffies(2000)))
> >>>    		ret = -ETIMEDOUT;
> >>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>    	return ret;
> >>>    }
> >>>    +/* kill pending URBs */
> >>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>> +{
> >>> +	struct udl_device *udl = to_udl(dev);
> >>> +	struct urb_node *unode;
> >>> +
> >>> +	spin_lock_irq(&udl->urbs.lock);
> >>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>> +					 struct urb_node, entry);
> >>> +		spin_unlock_irq(&udl->urbs.lock);
> >>> +		usb_kill_urb(unode->urb);
> >>> +		spin_lock_irq(&udl->urbs.lock);
> >>> +	}
> >>> +	spin_unlock_irq(&udl->urbs.lock);
> >>> +}
> >>> +
> >>>    int udl_init(struct udl_device *udl)
> >>>    {
> >>>    	struct drm_device *dev = &udl->drm;
> >>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>    {
> >>>    	struct udl_device *udl = to_udl(dev);
> >>>    +	udl_kill_pending_urbs(dev);
> >>>    	udl_free_urb_list(dev);
> >>>    	put_device(udl->dmadev);
> >>>    	udl->dmadev = NULL;
> >>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>> index 50025606b6ad..169110d8fc2e 100644
> >>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>    	struct urb *urb;
> >>>    	char *buf;
> >>>    +	udl_kill_pending_urbs(dev);
> >>> +
> >> 
> >> I already reviewed the patchset, but I have another comment. I think
> >> we should only kill urbs from within the suspend handler. Same for the
> >> call to the URB-sync function in patch 2.
> >> 
> >> This disable function is part of the regular modeset path. It's
> >> probably not appropriate to outright remove pending URBs here. This
> >> can lead to failed modesets, which would have succeeded otherwise.
> > 
> > Well, the device shall be turned off right after that point, so the
> > all pending rendering makes little sense, no?
> 
> udl_simple_display_pipe_disable() only disables the display, but not
> the device. The kill operation here could potentially kill some valid
> modeset operation that was still going on. And who knows what the
> device state is after that.

But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
command right after the place I've put udl_kill_pending_urbs().  So it
shall blank / turn off the power (of the device, as it has a single
output).  And the URB completion doesn't do any error handling but
just re-links URB chain and wakes up the queue.  So killing a pending
URB would nothing but canceling the in-flight URBs, and there should
be no disturbance to the modeset operation itself, as the screen will
be blanked immediately.

Of course, it's all theory, and if this breaks anything, it should be
corrected :)


thanks,

Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 09:41:19 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:13:16 +0200,
> > Thomas Zimmermann wrote:
> >> 
> >> Hi
> >> 
> >> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>> At both suspend and disconnect, we should rather cancel the pending
> >>> URBs immediately.  For the suspend case, the display will be turned
> >>> off, so it makes no sense to process the rendering.  And for the
> >>> disconnect case, the device may be no longer accessible, hence we
> >>> shouldn't do any submission.
> >>> 
> >>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>    drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>    drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>    drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>    3 files changed, 26 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>      struct urb_list {
> >>>    	struct list_head list;
> >>> +	struct list_head in_flight;
> >>>    	spinlock_t lock;
> >>>    	wait_queue_head_t sleep;
> >>>    	int available;
> >>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>      int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>> size_t len);
> >>>    int udl_sync_pending_urbs(struct drm_device *dev);
> >>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>    void udl_urb_completion(struct urb *urb);
> >>>      int udl_init(struct udl_device *udl);
> >>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>> index 93615648414b..47204b7eb10e 100644
> >>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>    	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>      	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>    	udl->urbs.available++;
> >>>    	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>    @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>> drm_device *dev, int count, size_t size)
> >>>    retry:
> >>>    	udl->urbs.size = size;
> >>>    	INIT_LIST_HEAD(&udl->urbs.list);
> >>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>      	init_waitqueue_head(&udl->urbs.sleep);
> >>>    	udl->urbs.count = 0;
> >>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>    	}
> >>>      	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>> entry);
> >>> -	list_del_init(&unode->entry);
> >>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>    	udl->urbs.available--;
> >>>      unlock:
> >>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>    	spin_lock_irq(&udl->urbs.lock);
> >>>    	/* 2 seconds as a sane timeout */
> >>>    	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>> -					 udl->urbs.available == udl->urbs.count,
> >>> +					 list_empty(&udl->urbs.in_flight),
> >>>    					 udl->urbs.lock,
> >>>    					 msecs_to_jiffies(2000)))
> >>>    		ret = -ETIMEDOUT;
> >>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>    	return ret;
> >>>    }
> >>>    +/* kill pending URBs */
> >>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>> +{
> >>> +	struct udl_device *udl = to_udl(dev);
> >>> +	struct urb_node *unode;
> >>> +
> >>> +	spin_lock_irq(&udl->urbs.lock);
> >>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>> +					 struct urb_node, entry);
> >>> +		spin_unlock_irq(&udl->urbs.lock);
> >>> +		usb_kill_urb(unode->urb);
> >>> +		spin_lock_irq(&udl->urbs.lock);
> >>> +	}
> >>> +	spin_unlock_irq(&udl->urbs.lock);
> >>> +}
> >>> +
> >>>    int udl_init(struct udl_device *udl)
> >>>    {
> >>>    	struct drm_device *dev = &udl->drm;
> >>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>    {
> >>>    	struct udl_device *udl = to_udl(dev);
> >>>    +	udl_kill_pending_urbs(dev);
> >>>    	udl_free_urb_list(dev);
> >>>    	put_device(udl->dmadev);
> >>>    	udl->dmadev = NULL;
> >>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>> index 50025606b6ad..169110d8fc2e 100644
> >>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>    	struct urb *urb;
> >>>    	char *buf;
> >>>    +	udl_kill_pending_urbs(dev);
> >>> +
> >> 
> >> I already reviewed the patchset, but I have another comment. I think
> >> we should only kill urbs from within the suspend handler. Same for the
> >> call to the URB-sync function in patch 2.
> >> 
> >> This disable function is part of the regular modeset path. It's
> >> probably not appropriate to outright remove pending URBs here. This
> >> can lead to failed modesets, which would have succeeded otherwise.
> > 
> > Well, the device shall be turned off right after that point, so the
> > all pending rendering makes little sense, no?
> 
> udl_simple_display_pipe_disable() only disables the display, but not
> the device. The kill operation here could potentially kill some valid
> modeset operation that was still going on. And who knows what the
> device state is after that.

But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
command right after the place I've put udl_kill_pending_urbs().  So it
shall blank / turn off the power (of the device, as it has a single
output).  And the URB completion doesn't do any error handling but
just re-links URB chain and wakes up the queue.  So killing a pending
URB would nothing but canceling the in-flight URBs, and there should
be no disturbance to the modeset operation itself, as the screen will
be blanked immediately.

Of course, it's all theory, and if this breaks anything, it should be
corrected :)


thanks,

Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6937 bytes --]

Hi

Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> On Tue, 09 Aug 2022 09:41:19 +0200,
> Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
>>> On Tue, 09 Aug 2022 09:13:16 +0200,
>>> Thomas Zimmermann wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
>>>>> At both suspend and disconnect, we should rather cancel the pending
>>>>> URBs immediately.  For the suspend case, the display will be turned
>>>>> off, so it makes no sense to process the rendering.  And for the
>>>>> disconnect case, the device may be no longer accessible, hence we
>>>>> shouldn't do any submission.
>>>>>
>>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>>> ---
>>>>>     drivers/gpu/drm/udl/udl_drv.h     |  2 ++
>>>>>     drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
>>>>>     drivers/gpu/drm/udl/udl_modeset.c |  2 ++
>>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>>>> @@ -39,6 +39,7 @@ struct urb_node {
>>>>>       struct urb_list {
>>>>>     	struct list_head list;
>>>>> +	struct list_head in_flight;
>>>>>     	spinlock_t lock;
>>>>>     	wait_queue_head_t sleep;
>>>>>     	int available;
>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
>>>>>       int udl_submit_urb(struct drm_device *dev, struct urb *urb,
>>>>> size_t len);
>>>>>     int udl_sync_pending_urbs(struct drm_device *dev);
>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
>>>>>     void udl_urb_completion(struct urb *urb);
>>>>>       int udl_init(struct udl_device *udl);
>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>>>>> index 93615648414b..47204b7eb10e 100644
>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
>>>>>     	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
>>>>>       	spin_lock_irqsave(&udl->urbs.lock, flags);
>>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
>>>>> +	list_move(&unode->entry, &udl->urbs.list);
>>>>>     	udl->urbs.available++;
>>>>>     	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>>>>>     @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
>>>>> drm_device *dev, int count, size_t size)
>>>>>     retry:
>>>>>     	udl->urbs.size = size;
>>>>>     	INIT_LIST_HEAD(&udl->urbs.list);
>>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
>>>>>       	init_waitqueue_head(&udl->urbs.sleep);
>>>>>     	udl->urbs.count = 0;
>>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
>>>>>     	}
>>>>>       	unode = list_first_entry(&udl->urbs.list, struct urb_node,
>>>>> entry);
>>>>> -	list_del_init(&unode->entry);
>>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
>>>>>     	udl->urbs.available--;
>>>>>       unlock:
>>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>     	spin_lock_irq(&udl->urbs.lock);
>>>>>     	/* 2 seconds as a sane timeout */
>>>>>     	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>>>>> -					 udl->urbs.available == udl->urbs.count,
>>>>> +					 list_empty(&udl->urbs.in_flight),
>>>>>     					 udl->urbs.lock,
>>>>>     					 msecs_to_jiffies(2000)))
>>>>>     		ret = -ETIMEDOUT;
>>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>     	return ret;
>>>>>     }
>>>>>     +/* kill pending URBs */
>>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
>>>>> +{
>>>>> +	struct udl_device *udl = to_udl(dev);
>>>>> +	struct urb_node *unode;
>>>>> +
>>>>> +	spin_lock_irq(&udl->urbs.lock);
>>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
>>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
>>>>> +					 struct urb_node, entry);
>>>>> +		spin_unlock_irq(&udl->urbs.lock);
>>>>> +		usb_kill_urb(unode->urb);
>>>>> +		spin_lock_irq(&udl->urbs.lock);
>>>>> +	}
>>>>> +	spin_unlock_irq(&udl->urbs.lock);
>>>>> +}
>>>>> +
>>>>>     int udl_init(struct udl_device *udl)
>>>>>     {
>>>>>     	struct drm_device *dev = &udl->drm;
>>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
>>>>>     {
>>>>>     	struct udl_device *udl = to_udl(dev);
>>>>>     +	udl_kill_pending_urbs(dev);
>>>>>     	udl_free_urb_list(dev);
>>>>>     	put_device(udl->dmadev);
>>>>>     	udl->dmadev = NULL;
>>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>>>>> index 50025606b6ad..169110d8fc2e 100644
>>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>>>     	struct urb *urb;
>>>>>     	char *buf;
>>>>>     +	udl_kill_pending_urbs(dev);
>>>>> +
>>>>
>>>> I already reviewed the patchset, but I have another comment. I think
>>>> we should only kill urbs from within the suspend handler. Same for the
>>>> call to the URB-sync function in patch 2.
>>>>
>>>> This disable function is part of the regular modeset path. It's
>>>> probably not appropriate to outright remove pending URBs here. This
>>>> can lead to failed modesets, which would have succeeded otherwise.
>>>
>>> Well, the device shall be turned off right after that point, so the
>>> all pending rendering makes little sense, no?
>>
>> udl_simple_display_pipe_disable() only disables the display, but not
>> the device. The kill operation here could potentially kill some valid
>> modeset operation that was still going on. And who knows what the
>> device state is after that.
> 
> But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> command right after the place I've put udl_kill_pending_urbs().  So it
> shall blank / turn off the power (of the device, as it has a single
> output).  And the URB completion doesn't do any error handling but
> just re-links URB chain and wakes up the queue.  So killing a pending
> URB would nothing but canceling the in-flight URBs, and there should
> be no disturbance to the modeset operation itself, as the screen will
> be blanked immediately.

The blank mode is essentially DPMS. It's unrelated to the device's 
display mode.

Best regards
Thomas

> 
> Of course, it's all theory, and if this breaks anything, it should be
> corrected :)
> 
> 
> thanks,
> 
> Takashi

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 11:13:46 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:41:19 +0200,
> > Thomas Zimmermann wrote:
> >> 
> >> Hi
> >> 
> >> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>> Thomas Zimmermann wrote:
> >>>> 
> >>>> Hi
> >>>> 
> >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>> URBs immediately.  For the suspend case, the display will be turned
> >>>>> off, so it makes no sense to process the rendering.  And for the
> >>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>> shouldn't do any submission.
> >>>>> 
> >>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>>>> ---
> >>>>>     drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>>>     drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>>>     drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>>       struct urb_list {
> >>>>>     	struct list_head list;
> >>>>> +	struct list_head in_flight;
> >>>>>     	spinlock_t lock;
> >>>>>     	wait_queue_head_t sleep;
> >>>>>     	int available;
> >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>>>       int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>> size_t len);
> >>>>>     int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>>     void udl_urb_completion(struct urb *urb);
> >>>>>       int udl_init(struct udl_device *udl);
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>>>> index 93615648414b..47204b7eb10e 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>>     	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>>>       	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>>>     	udl->urbs.available++;
> >>>>>     	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>>     @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>> drm_device *dev, int count, size_t size)
> >>>>>     retry:
> >>>>>     	udl->urbs.size = size;
> >>>>>     	INIT_LIST_HEAD(&udl->urbs.list);
> >>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>>>       	init_waitqueue_head(&udl->urbs.sleep);
> >>>>>     	udl->urbs.count = 0;
> >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>>>     	}
> >>>>>       	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>>>> entry);
> >>>>> -	list_del_init(&unode->entry);
> >>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>>>     	udl->urbs.available--;
> >>>>>       unlock:
> >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>     	spin_lock_irq(&udl->urbs.lock);
> >>>>>     	/* 2 seconds as a sane timeout */
> >>>>>     	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>>>> -					 udl->urbs.available == udl->urbs.count,
> >>>>> +					 list_empty(&udl->urbs.in_flight),
> >>>>>     					 udl->urbs.lock,
> >>>>>     					 msecs_to_jiffies(2000)))
> >>>>>     		ret = -ETIMEDOUT;
> >>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>     	return ret;
> >>>>>     }
> >>>>>     +/* kill pending URBs */
> >>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>>>> +{
> >>>>> +	struct udl_device *udl = to_udl(dev);
> >>>>> +	struct urb_node *unode;
> >>>>> +
> >>>>> +	spin_lock_irq(&udl->urbs.lock);
> >>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>>>> +					 struct urb_node, entry);
> >>>>> +		spin_unlock_irq(&udl->urbs.lock);
> >>>>> +		usb_kill_urb(unode->urb);
> >>>>> +		spin_lock_irq(&udl->urbs.lock);
> >>>>> +	}
> >>>>> +	spin_unlock_irq(&udl->urbs.lock);
> >>>>> +}
> >>>>> +
> >>>>>     int udl_init(struct udl_device *udl)
> >>>>>     {
> >>>>>     	struct drm_device *dev = &udl->drm;
> >>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>>>     {
> >>>>>     	struct udl_device *udl = to_udl(dev);
> >>>>>     +	udl_kill_pending_urbs(dev);
> >>>>>     	udl_free_urb_list(dev);
> >>>>>     	put_device(udl->dmadev);
> >>>>>     	udl->dmadev = NULL;
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> index 50025606b6ad..169110d8fc2e 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>>>     	struct urb *urb;
> >>>>>     	char *buf;
> >>>>>     +	udl_kill_pending_urbs(dev);
> >>>>> +
> >>>> 
> >>>> I already reviewed the patchset, but I have another comment. I think
> >>>> we should only kill urbs from within the suspend handler. Same for the
> >>>> call to the URB-sync function in patch 2.
> >>>> 
> >>>> This disable function is part of the regular modeset path. It's
> >>>> probably not appropriate to outright remove pending URBs here. This
> >>>> can lead to failed modesets, which would have succeeded otherwise.
> >>> 
> >>> Well, the device shall be turned off right after that point, so the
> >>> all pending rendering makes little sense, no?
> >> 
> >> udl_simple_display_pipe_disable() only disables the display, but not
> >> the device. The kill operation here could potentially kill some valid
> >> modeset operation that was still going on. And who knows what the
> >> device state is after that.
> > 
> > But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> > command right after the place I've put udl_kill_pending_urbs().  So it
> > shall blank / turn off the power (of the device, as it has a single
> > output).  And the URB completion doesn't do any error handling but
> > just re-links URB chain and wakes up the queue.  So killing a pending
> > URB would nothing but canceling the in-flight URBs, and there should
> > be no disturbance to the modeset operation itself, as the screen will
> > be blanked immediately.
> 
> The blank mode is essentially DPMS. It's unrelated to the device's
> display mode.

The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
discard the whole rendered picture.  And, the counterpart,
udl_simple_display_pipe_enable(), re-initializes the mode fully from
the scratch again.
So what's the point to continue rendering that is immediately cleared
(from the screen and from the device state)?  Killing pending URBs
doesn't influence on the internal (modeset) state of the driver.


Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 11:13:46 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:41:19 +0200,
> > Thomas Zimmermann wrote:
> >> 
> >> Hi
> >> 
> >> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>> Thomas Zimmermann wrote:
> >>>> 
> >>>> Hi
> >>>> 
> >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>> URBs immediately.  For the suspend case, the display will be turned
> >>>>> off, so it makes no sense to process the rendering.  And for the
> >>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>> shouldn't do any submission.
> >>>>> 
> >>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>>>> ---
> >>>>>     drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>>>     drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>>>     drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>>       struct urb_list {
> >>>>>     	struct list_head list;
> >>>>> +	struct list_head in_flight;
> >>>>>     	spinlock_t lock;
> >>>>>     	wait_queue_head_t sleep;
> >>>>>     	int available;
> >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>>>       int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>> size_t len);
> >>>>>     int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>>     void udl_urb_completion(struct urb *urb);
> >>>>>       int udl_init(struct udl_device *udl);
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>>>> index 93615648414b..47204b7eb10e 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>>     	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>>>       	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>>>     	udl->urbs.available++;
> >>>>>     	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>>     @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>> drm_device *dev, int count, size_t size)
> >>>>>     retry:
> >>>>>     	udl->urbs.size = size;
> >>>>>     	INIT_LIST_HEAD(&udl->urbs.list);
> >>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>>>       	init_waitqueue_head(&udl->urbs.sleep);
> >>>>>     	udl->urbs.count = 0;
> >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>>>     	}
> >>>>>       	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>>>> entry);
> >>>>> -	list_del_init(&unode->entry);
> >>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>>>     	udl->urbs.available--;
> >>>>>       unlock:
> >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>     	spin_lock_irq(&udl->urbs.lock);
> >>>>>     	/* 2 seconds as a sane timeout */
> >>>>>     	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>>>> -					 udl->urbs.available == udl->urbs.count,
> >>>>> +					 list_empty(&udl->urbs.in_flight),
> >>>>>     					 udl->urbs.lock,
> >>>>>     					 msecs_to_jiffies(2000)))
> >>>>>     		ret = -ETIMEDOUT;
> >>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>     	return ret;
> >>>>>     }
> >>>>>     +/* kill pending URBs */
> >>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>>>> +{
> >>>>> +	struct udl_device *udl = to_udl(dev);
> >>>>> +	struct urb_node *unode;
> >>>>> +
> >>>>> +	spin_lock_irq(&udl->urbs.lock);
> >>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>>>> +					 struct urb_node, entry);
> >>>>> +		spin_unlock_irq(&udl->urbs.lock);
> >>>>> +		usb_kill_urb(unode->urb);
> >>>>> +		spin_lock_irq(&udl->urbs.lock);
> >>>>> +	}
> >>>>> +	spin_unlock_irq(&udl->urbs.lock);
> >>>>> +}
> >>>>> +
> >>>>>     int udl_init(struct udl_device *udl)
> >>>>>     {
> >>>>>     	struct drm_device *dev = &udl->drm;
> >>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>>>     {
> >>>>>     	struct udl_device *udl = to_udl(dev);
> >>>>>     +	udl_kill_pending_urbs(dev);
> >>>>>     	udl_free_urb_list(dev);
> >>>>>     	put_device(udl->dmadev);
> >>>>>     	udl->dmadev = NULL;
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> index 50025606b6ad..169110d8fc2e 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>>>     	struct urb *urb;
> >>>>>     	char *buf;
> >>>>>     +	udl_kill_pending_urbs(dev);
> >>>>> +
> >>>> 
> >>>> I already reviewed the patchset, but I have another comment. I think
> >>>> we should only kill urbs from within the suspend handler. Same for the
> >>>> call to the URB-sync function in patch 2.
> >>>> 
> >>>> This disable function is part of the regular modeset path. It's
> >>>> probably not appropriate to outright remove pending URBs here. This
> >>>> can lead to failed modesets, which would have succeeded otherwise.
> >>> 
> >>> Well, the device shall be turned off right after that point, so the
> >>> all pending rendering makes little sense, no?
> >> 
> >> udl_simple_display_pipe_disable() only disables the display, but not
> >> the device. The kill operation here could potentially kill some valid
> >> modeset operation that was still going on. And who knows what the
> >> device state is after that.
> > 
> > But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> > command right after the place I've put udl_kill_pending_urbs().  So it
> > shall blank / turn off the power (of the device, as it has a single
> > output).  And the URB completion doesn't do any error handling but
> > just re-links URB chain and wakes up the queue.  So killing a pending
> > URB would nothing but canceling the in-flight URBs, and there should
> > be no disturbance to the modeset operation itself, as the screen will
> > be blanked immediately.
> 
> The blank mode is essentially DPMS. It's unrelated to the device's
> display mode.

The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
discard the whole rendered picture.  And, the counterpart,
udl_simple_display_pipe_enable(), re-initializes the mode fully from
the scratch again.
So what's the point to continue rendering that is immediately cleared
(from the screen and from the device state)?  Killing pending URBs
doesn't influence on the internal (modeset) state of the driver.


Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 09 Aug 2022 11:19:30 +0200,
Takashi Iwai wrote:
> 
> On Tue, 09 Aug 2022 11:13:46 +0200,
> Thomas Zimmermann wrote:
> > 
> > Hi
> > 
> > Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> > > On Tue, 09 Aug 2022 09:41:19 +0200,
> > > Thomas Zimmermann wrote:
> > >> 
> > >> Hi
> > >> 
> > >> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> > >>> On Tue, 09 Aug 2022 09:13:16 +0200,
> > >>> Thomas Zimmermann wrote:
> > >>>> 
> > >>>> Hi
> > >>>> 
> > >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> > >>>>> At both suspend and disconnect, we should rather cancel the pending
> > >>>>> URBs immediately.  For the suspend case, the display will be turned
> > >>>>> off, so it makes no sense to process the rendering.  And for the
> > >>>>> disconnect case, the device may be no longer accessible, hence we
> > >>>>> shouldn't do any submission.
> > >>>>> 
> > >>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > >>>>> ---
> > >>>>>     drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> > >>>>>     drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> > >>>>>     drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> > >>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
> > >>>>> 
> > >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> > >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> @@ -39,6 +39,7 @@ struct urb_node {
> > >>>>>       struct urb_list {
> > >>>>>     	struct list_head list;
> > >>>>> +	struct list_head in_flight;
> > >>>>>     	spinlock_t lock;
> > >>>>>     	wait_queue_head_t sleep;
> > >>>>>     	int available;
> > >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> > >>>>>       int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > >>>>> size_t len);
> > >>>>>     int udl_sync_pending_urbs(struct drm_device *dev);
> > >>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> > >>>>>     void udl_urb_completion(struct urb *urb);
> > >>>>>       int udl_init(struct udl_device *udl);
> > >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > >>>>> index 93615648414b..47204b7eb10e 100644
> > >>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> > >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> > >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> > >>>>>     	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> > >>>>>       	spin_lock_irqsave(&udl->urbs.lock, flags);
> > >>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> > >>>>> +	list_move(&unode->entry, &udl->urbs.list);
> > >>>>>     	udl->urbs.available++;
> > >>>>>     	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> > >>>>>     @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> > >>>>> drm_device *dev, int count, size_t size)
> > >>>>>     retry:
> > >>>>>     	udl->urbs.size = size;
> > >>>>>     	INIT_LIST_HEAD(&udl->urbs.list);
> > >>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> > >>>>>       	init_waitqueue_head(&udl->urbs.sleep);
> > >>>>>     	udl->urbs.count = 0;
> > >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> > >>>>>     	}
> > >>>>>       	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> > >>>>> entry);
> > >>>>> -	list_del_init(&unode->entry);
> > >>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> > >>>>>     	udl->urbs.available--;
> > >>>>>       unlock:
> > >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> > >>>>>     	spin_lock_irq(&udl->urbs.lock);
> > >>>>>     	/* 2 seconds as a sane timeout */
> > >>>>>     	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> > >>>>> -					 udl->urbs.available == udl->urbs.count,
> > >>>>> +					 list_empty(&udl->urbs.in_flight),
> > >>>>>     					 udl->urbs.lock,
> > >>>>>     					 msecs_to_jiffies(2000)))
> > >>>>>     		ret = -ETIMEDOUT;
> > >>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> > >>>>>     	return ret;
> > >>>>>     }
> > >>>>>     +/* kill pending URBs */
> > >>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
> > >>>>> +{
> > >>>>> +	struct udl_device *udl = to_udl(dev);
> > >>>>> +	struct urb_node *unode;
> > >>>>> +
> > >>>>> +	spin_lock_irq(&udl->urbs.lock);
> > >>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
> > >>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
> > >>>>> +					 struct urb_node, entry);
> > >>>>> +		spin_unlock_irq(&udl->urbs.lock);
> > >>>>> +		usb_kill_urb(unode->urb);
> > >>>>> +		spin_lock_irq(&udl->urbs.lock);
> > >>>>> +	}
> > >>>>> +	spin_unlock_irq(&udl->urbs.lock);
> > >>>>> +}
> > >>>>> +
> > >>>>>     int udl_init(struct udl_device *udl)
> > >>>>>     {
> > >>>>>     	struct drm_device *dev = &udl->drm;
> > >>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> > >>>>>     {
> > >>>>>     	struct udl_device *udl = to_udl(dev);
> > >>>>>     +	udl_kill_pending_urbs(dev);
> > >>>>>     	udl_free_urb_list(dev);
> > >>>>>     	put_device(udl->dmadev);
> > >>>>>     	udl->dmadev = NULL;
> > >>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > >>>>> index 50025606b6ad..169110d8fc2e 100644
> > >>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> > >>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > >>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> > >>>>>     	struct urb *urb;
> > >>>>>     	char *buf;
> > >>>>>     +	udl_kill_pending_urbs(dev);
> > >>>>> +
> > >>>> 
> > >>>> I already reviewed the patchset, but I have another comment. I think
> > >>>> we should only kill urbs from within the suspend handler. Same for the
> > >>>> call to the URB-sync function in patch 2.
> > >>>> 
> > >>>> This disable function is part of the regular modeset path. It's
> > >>>> probably not appropriate to outright remove pending URBs here. This
> > >>>> can lead to failed modesets, which would have succeeded otherwise.
> > >>> 
> > >>> Well, the device shall be turned off right after that point, so the
> > >>> all pending rendering makes little sense, no?
> > >> 
> > >> udl_simple_display_pipe_disable() only disables the display, but not
> > >> the device. The kill operation here could potentially kill some valid
> > >> modeset operation that was still going on. And who knows what the
> > >> device state is after that.
> > > 
> > > But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> > > command right after the place I've put udl_kill_pending_urbs().  So it
> > > shall blank / turn off the power (of the device, as it has a single
> > > output).  And the URB completion doesn't do any error handling but
> > > just re-links URB chain and wakes up the queue.  So killing a pending
> > > URB would nothing but canceling the in-flight URBs, and there should
> > > be no disturbance to the modeset operation itself, as the screen will
> > > be blanked immediately.
> > 
> > The blank mode is essentially DPMS. It's unrelated to the device's
> > display mode.
> 
> The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
> discard the whole rendered picture.  And, the counterpart,
> udl_simple_display_pipe_enable(), re-initializes the mode fully from
> the scratch again.
> So what's the point to continue rendering that is immediately cleared
> (from the screen and from the device state)?  Killing pending URBs
> doesn't influence on the internal (modeset) state of the driver.

In anyway, this patchset seems problematic around the disconnection,
and maybe this particular one is no much improvement, better to drop
for now.

I'll resubmit the v2 patch set including your resume fixes later.


thanks,

Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8373 bytes --]

Hi Takashi

Am 16.08.22 um 15:55 schrieb Takashi Iwai:
> On Tue, 09 Aug 2022 11:19:30 +0200,
> Takashi Iwai wrote:
>>
>> On Tue, 09 Aug 2022 11:13:46 +0200,
>> Thomas Zimmermann wrote:
>>>
>>> Hi
>>>
>>> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
>>>> On Tue, 09 Aug 2022 09:41:19 +0200,
>>>> Thomas Zimmermann wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
>>>>>> On Tue, 09 Aug 2022 09:13:16 +0200,
>>>>>> Thomas Zimmermann wrote:
>>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
>>>>>>>> At both suspend and disconnect, we should rather cancel the pending
>>>>>>>> URBs immediately.  For the suspend case, the display will be turned
>>>>>>>> off, so it makes no sense to process the rendering.  And for the
>>>>>>>> disconnect case, the device may be no longer accessible, hence we
>>>>>>>> shouldn't do any submission.
>>>>>>>>
>>>>>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/udl/udl_drv.h     |  2 ++
>>>>>>>>      drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
>>>>>>>>      drivers/gpu/drm/udl/udl_modeset.c |  2 ++
>>>>>>>>      3 files changed, 26 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> @@ -39,6 +39,7 @@ struct urb_node {
>>>>>>>>        struct urb_list {
>>>>>>>>      	struct list_head list;
>>>>>>>> +	struct list_head in_flight;
>>>>>>>>      	spinlock_t lock;
>>>>>>>>      	wait_queue_head_t sleep;
>>>>>>>>      	int available;
>>>>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
>>>>>>>>        int udl_submit_urb(struct drm_device *dev, struct urb *urb,
>>>>>>>> size_t len);
>>>>>>>>      int udl_sync_pending_urbs(struct drm_device *dev);
>>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
>>>>>>>>      void udl_urb_completion(struct urb *urb);
>>>>>>>>        int udl_init(struct udl_device *udl);
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> index 93615648414b..47204b7eb10e 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
>>>>>>>>      	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
>>>>>>>>        	spin_lock_irqsave(&udl->urbs.lock, flags);
>>>>>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
>>>>>>>> +	list_move(&unode->entry, &udl->urbs.list);
>>>>>>>>      	udl->urbs.available++;
>>>>>>>>      	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>>>>>>>>      @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
>>>>>>>> drm_device *dev, int count, size_t size)
>>>>>>>>      retry:
>>>>>>>>      	udl->urbs.size = size;
>>>>>>>>      	INIT_LIST_HEAD(&udl->urbs.list);
>>>>>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
>>>>>>>>        	init_waitqueue_head(&udl->urbs.sleep);
>>>>>>>>      	udl->urbs.count = 0;
>>>>>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
>>>>>>>>      	}
>>>>>>>>        	unode = list_first_entry(&udl->urbs.list, struct urb_node,
>>>>>>>> entry);
>>>>>>>> -	list_del_init(&unode->entry);
>>>>>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
>>>>>>>>      	udl->urbs.available--;
>>>>>>>>        unlock:
>>>>>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>>>>      	spin_lock_irq(&udl->urbs.lock);
>>>>>>>>      	/* 2 seconds as a sane timeout */
>>>>>>>>      	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>>>>>>>> -					 udl->urbs.available == udl->urbs.count,
>>>>>>>> +					 list_empty(&udl->urbs.in_flight),
>>>>>>>>      					 udl->urbs.lock,
>>>>>>>>      					 msecs_to_jiffies(2000)))
>>>>>>>>      		ret = -ETIMEDOUT;
>>>>>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>>>>      	return ret;
>>>>>>>>      }
>>>>>>>>      +/* kill pending URBs */
>>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
>>>>>>>> +{
>>>>>>>> +	struct udl_device *udl = to_udl(dev);
>>>>>>>> +	struct urb_node *unode;
>>>>>>>> +
>>>>>>>> +	spin_lock_irq(&udl->urbs.lock);
>>>>>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
>>>>>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
>>>>>>>> +					 struct urb_node, entry);
>>>>>>>> +		spin_unlock_irq(&udl->urbs.lock);
>>>>>>>> +		usb_kill_urb(unode->urb);
>>>>>>>> +		spin_lock_irq(&udl->urbs.lock);
>>>>>>>> +	}
>>>>>>>> +	spin_unlock_irq(&udl->urbs.lock);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      int udl_init(struct udl_device *udl)
>>>>>>>>      {
>>>>>>>>      	struct drm_device *dev = &udl->drm;
>>>>>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
>>>>>>>>      {
>>>>>>>>      	struct udl_device *udl = to_udl(dev);
>>>>>>>>      +	udl_kill_pending_urbs(dev);
>>>>>>>>      	udl_free_urb_list(dev);
>>>>>>>>      	put_device(udl->dmadev);
>>>>>>>>      	udl->dmadev = NULL;
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> index 50025606b6ad..169110d8fc2e 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>>>>>>      	struct urb *urb;
>>>>>>>>      	char *buf;
>>>>>>>>      +	udl_kill_pending_urbs(dev);
>>>>>>>> +
>>>>>>>
>>>>>>> I already reviewed the patchset, but I have another comment. I think
>>>>>>> we should only kill urbs from within the suspend handler. Same for the
>>>>>>> call to the URB-sync function in patch 2.
>>>>>>>
>>>>>>> This disable function is part of the regular modeset path. It's
>>>>>>> probably not appropriate to outright remove pending URBs here. This
>>>>>>> can lead to failed modesets, which would have succeeded otherwise.
>>>>>>
>>>>>> Well, the device shall be turned off right after that point, so the
>>>>>> all pending rendering makes little sense, no?
>>>>>
>>>>> udl_simple_display_pipe_disable() only disables the display, but not
>>>>> the device. The kill operation here could potentially kill some valid
>>>>> modeset operation that was still going on. And who knows what the
>>>>> device state is after that.
>>>>
>>>> But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
>>>> command right after the place I've put udl_kill_pending_urbs().  So it
>>>> shall blank / turn off the power (of the device, as it has a single
>>>> output).  And the URB completion doesn't do any error handling but
>>>> just re-links URB chain and wakes up the queue.  So killing a pending
>>>> URB would nothing but canceling the in-flight URBs, and there should
>>>> be no disturbance to the modeset operation itself, as the screen will
>>>> be blanked immediately.
>>>
>>> The blank mode is essentially DPMS. It's unrelated to the device's
>>> display mode.
>>
>> The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
>> discard the whole rendered picture.  And, the counterpart,
>> udl_simple_display_pipe_enable(), re-initializes the mode fully from
>> the scratch again.
>> So what's the point to continue rendering that is immediately cleared
>> (from the screen and from the device state)?  Killing pending URBs
>> doesn't influence on the internal (modeset) state of the driver.
> 
> In anyway, this patchset seems problematic around the disconnection,
> and maybe this particular one is no much improvement, better to drop
> for now.
> 
> I'll resubmit the v2 patch set including your resume fixes later.

I already merged the patches before seeing the discussion on the rsp bug 
report. If you submit an update, maybe you can do so with Fixes tags?

Best regards
Thomas

> 
> 
> thanks,
> 
> Takashi

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 16 Aug 2022 16:01:34 +0200,
Thomas Zimmermann wrote:
> 
> Hi Takashi
> 
> Am 16.08.22 um 15:55 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 11:19:30 +0200,
> > Takashi Iwai wrote:
> >> 
> >> On Tue, 09 Aug 2022 11:13:46 +0200,
> >> Thomas Zimmermann wrote:
> >>> 
> >>> Hi
> >>> 
> >>> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> >>>> On Tue, 09 Aug 2022 09:41:19 +0200,
> >>>> Thomas Zimmermann wrote:
> >>>>> 
> >>>>> Hi
> >>>>> 
> >>>>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>>>>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>>>>> Thomas Zimmermann wrote:
> >>>>>>> 
> >>>>>>> Hi
> >>>>>>> 
> >>>>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>>>>> URBs immediately.  For the suspend case, the display will be turned
> >>>>>>>> off, so it makes no sense to process the rendering.  And for the
> >>>>>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>>>>> shouldn't do any submission.
> >>>>>>>> 
> >>>>>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>>>>>>> ---
> >>>>>>>>      drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>>>>>>      drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>>>>>>      drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>>>>>>      3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>>>>>        struct urb_list {
> >>>>>>>>      	struct list_head list;
> >>>>>>>> +	struct list_head in_flight;
> >>>>>>>>      	spinlock_t lock;
> >>>>>>>>      	wait_queue_head_t sleep;
> >>>>>>>>      	int available;
> >>>>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>>>>>>        int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>>>>> size_t len);
> >>>>>>>>      int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>>>>>      void udl_urb_completion(struct urb *urb);
> >>>>>>>>        int udl_init(struct udl_device *udl);
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> index 93615648414b..47204b7eb10e 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>>>>>      	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>>>>>>        	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>>>>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>>>>>>      	udl->urbs.available++;
> >>>>>>>>      	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>>>>>      @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>>>>> drm_device *dev, int count, size_t size)
> >>>>>>>>      retry:
> >>>>>>>>      	udl->urbs.size = size;
> >>>>>>>>      	INIT_LIST_HEAD(&udl->urbs.list);
> >>>>>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>>>>>>        	init_waitqueue_head(&udl->urbs.sleep);
> >>>>>>>>      	udl->urbs.count = 0;
> >>>>>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>>>>>>      	}
> >>>>>>>>        	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>>>>>>> entry);
> >>>>>>>> -	list_del_init(&unode->entry);
> >>>>>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>>>>>>      	udl->urbs.available--;
> >>>>>>>>        unlock:
> >>>>>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>>>>      	spin_lock_irq(&udl->urbs.lock);
> >>>>>>>>      	/* 2 seconds as a sane timeout */
> >>>>>>>>      	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>>>>>>> -					 udl->urbs.available == udl->urbs.count,
> >>>>>>>> +					 list_empty(&udl->urbs.in_flight),
> >>>>>>>>      					 udl->urbs.lock,
> >>>>>>>>      					 msecs_to_jiffies(2000)))
> >>>>>>>>      		ret = -ETIMEDOUT;
> >>>>>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>>>>      	return ret;
> >>>>>>>>      }
> >>>>>>>>      +/* kill pending URBs */
> >>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>>>>>>> +{
> >>>>>>>> +	struct udl_device *udl = to_udl(dev);
> >>>>>>>> +	struct urb_node *unode;
> >>>>>>>> +
> >>>>>>>> +	spin_lock_irq(&udl->urbs.lock);
> >>>>>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>>>>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>>>>>>> +					 struct urb_node, entry);
> >>>>>>>> +		spin_unlock_irq(&udl->urbs.lock);
> >>>>>>>> +		usb_kill_urb(unode->urb);
> >>>>>>>> +		spin_lock_irq(&udl->urbs.lock);
> >>>>>>>> +	}
> >>>>>>>> +	spin_unlock_irq(&udl->urbs.lock);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>      int udl_init(struct udl_device *udl)
> >>>>>>>>      {
> >>>>>>>>      	struct drm_device *dev = &udl->drm;
> >>>>>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>>>>>>      {
> >>>>>>>>      	struct udl_device *udl = to_udl(dev);
> >>>>>>>>      +	udl_kill_pending_urbs(dev);
> >>>>>>>>      	udl_free_urb_list(dev);
> >>>>>>>>      	put_device(udl->dmadev);
> >>>>>>>>      	udl->dmadev = NULL;
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> index 50025606b6ad..169110d8fc2e 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>>>>>>      	struct urb *urb;
> >>>>>>>>      	char *buf;
> >>>>>>>>      +	udl_kill_pending_urbs(dev);
> >>>>>>>> +
> >>>>>>> 
> >>>>>>> I already reviewed the patchset, but I have another comment. I think
> >>>>>>> we should only kill urbs from within the suspend handler. Same for the
> >>>>>>> call to the URB-sync function in patch 2.
> >>>>>>> 
> >>>>>>> This disable function is part of the regular modeset path. It's
> >>>>>>> probably not appropriate to outright remove pending URBs here. This
> >>>>>>> can lead to failed modesets, which would have succeeded otherwise.
> >>>>>> 
> >>>>>> Well, the device shall be turned off right after that point, so the
> >>>>>> all pending rendering makes little sense, no?
> >>>>> 
> >>>>> udl_simple_display_pipe_disable() only disables the display, but not
> >>>>> the device. The kill operation here could potentially kill some valid
> >>>>> modeset operation that was still going on. And who knows what the
> >>>>> device state is after that.
> >>>> 
> >>>> But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> >>>> command right after the place I've put udl_kill_pending_urbs().  So it
> >>>> shall blank / turn off the power (of the device, as it has a single
> >>>> output).  And the URB completion doesn't do any error handling but
> >>>> just re-links URB chain and wakes up the queue.  So killing a pending
> >>>> URB would nothing but canceling the in-flight URBs, and there should
> >>>> be no disturbance to the modeset operation itself, as the screen will
> >>>> be blanked immediately.
> >>> 
> >>> The blank mode is essentially DPMS. It's unrelated to the device's
> >>> display mode.
> >> 
> >> The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
> >> discard the whole rendered picture.  And, the counterpart,
> >> udl_simple_display_pipe_enable(), re-initializes the mode fully from
> >> the scratch again.
> >> So what's the point to continue rendering that is immediately cleared
> >> (from the screen and from the device state)?  Killing pending URBs
> >> doesn't influence on the internal (modeset) state of the driver.
> > 
> > In anyway, this patchset seems problematic around the disconnection,
> > and maybe this particular one is no much improvement, better to drop
> > for now.
> > 
> > I'll resubmit the v2 patch set including your resume fixes later.
> 
> I already merged the patches before seeing the discussion on the rsp
> bug report. If you submit an update, maybe you can do so with Fixes
> tags?

Oh sure, will check then.


Takashi

Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect

[Takashi Iwai]

On Tue, 16 Aug 2022 16:01:34 +0200,
Thomas Zimmermann wrote:
> 
> Hi Takashi
> 
> Am 16.08.22 um 15:55 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 11:19:30 +0200,
> > Takashi Iwai wrote:
> >> 
> >> On Tue, 09 Aug 2022 11:13:46 +0200,
> >> Thomas Zimmermann wrote:
> >>> 
> >>> Hi
> >>> 
> >>> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> >>>> On Tue, 09 Aug 2022 09:41:19 +0200,
> >>>> Thomas Zimmermann wrote:
> >>>>> 
> >>>>> Hi
> >>>>> 
> >>>>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>>>>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>>>>> Thomas Zimmermann wrote:
> >>>>>>> 
> >>>>>>> Hi
> >>>>>>> 
> >>>>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>>>>> URBs immediately.  For the suspend case, the display will be turned
> >>>>>>>> off, so it makes no sense to process the rendering.  And for the
> >>>>>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>>>>> shouldn't do any submission.
> >>>>>>>> 
> >>>>>>>> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>>>>>>> ---
> >>>>>>>>      drivers/gpu/drm/udl/udl_drv.h     |  2 ++
> >>>>>>>>      drivers/gpu/drm/udl/udl_main.c    | 25 ++++++++++++++++++++++---
> >>>>>>>>      drivers/gpu/drm/udl/udl_modeset.c |  2 ++
> >>>>>>>>      3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>>>>>        struct urb_list {
> >>>>>>>>      	struct list_head list;
> >>>>>>>> +	struct list_head in_flight;
> >>>>>>>>      	spinlock_t lock;
> >>>>>>>>      	wait_queue_head_t sleep;
> >>>>>>>>      	int available;
> >>>>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
> >>>>>>>>        int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>>>>> size_t len);
> >>>>>>>>      int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>>>>>      void udl_urb_completion(struct urb *urb);
> >>>>>>>>        int udl_init(struct udl_device *udl);
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> index 93615648414b..47204b7eb10e 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>>>>>      	urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> >>>>>>>>        	spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>>>>> -	list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>>>>> +	list_move(&unode->entry, &udl->urbs.list);
> >>>>>>>>      	udl->urbs.available++;
> >>>>>>>>      	spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>>>>>      @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>>>>> drm_device *dev, int count, size_t size)
> >>>>>>>>      retry:
> >>>>>>>>      	udl->urbs.size = size;
> >>>>>>>>      	INIT_LIST_HEAD(&udl->urbs.list);
> >>>>>>>> +	INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>>>>>>        	init_waitqueue_head(&udl->urbs.sleep);
> >>>>>>>>      	udl->urbs.count = 0;
> >>>>>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> >>>>>>>>      	}
> >>>>>>>>        	unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>>>>>>> entry);
> >>>>>>>> -	list_del_init(&unode->entry);
> >>>>>>>> +	list_move(&unode->entry, &udl->urbs.in_flight);
> >>>>>>>>      	udl->urbs.available--;
> >>>>>>>>        unlock:
> >>>>>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>>>>      	spin_lock_irq(&udl->urbs.lock);
> >>>>>>>>      	/* 2 seconds as a sane timeout */
> >>>>>>>>      	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>>>>>>> -					 udl->urbs.available == udl->urbs.count,
> >>>>>>>> +					 list_empty(&udl->urbs.in_flight),
> >>>>>>>>      					 udl->urbs.lock,
> >>>>>>>>      					 msecs_to_jiffies(2000)))
> >>>>>>>>      		ret = -ETIMEDOUT;
> >>>>>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>>>>>      	return ret;
> >>>>>>>>      }
> >>>>>>>>      +/* kill pending URBs */
> >>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
> >>>>>>>> +{
> >>>>>>>> +	struct udl_device *udl = to_udl(dev);
> >>>>>>>> +	struct urb_node *unode;
> >>>>>>>> +
> >>>>>>>> +	spin_lock_irq(&udl->urbs.lock);
> >>>>>>>> +	while (!list_empty(&udl->urbs.in_flight)) {
> >>>>>>>> +		unode = list_first_entry(&udl->urbs.in_flight,
> >>>>>>>> +					 struct urb_node, entry);
> >>>>>>>> +		spin_unlock_irq(&udl->urbs.lock);
> >>>>>>>> +		usb_kill_urb(unode->urb);
> >>>>>>>> +		spin_lock_irq(&udl->urbs.lock);
> >>>>>>>> +	}
> >>>>>>>> +	spin_unlock_irq(&udl->urbs.lock);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>      int udl_init(struct udl_device *udl)
> >>>>>>>>      {
> >>>>>>>>      	struct drm_device *dev = &udl->drm;
> >>>>>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> >>>>>>>>      {
> >>>>>>>>      	struct udl_device *udl = to_udl(dev);
> >>>>>>>>      +	udl_kill_pending_urbs(dev);
> >>>>>>>>      	udl_free_urb_list(dev);
> >>>>>>>>      	put_device(udl->dmadev);
> >>>>>>>>      	udl->dmadev = NULL;
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> index 50025606b6ad..169110d8fc2e 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >>>>>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>>>>>>>      	struct urb *urb;
> >>>>>>>>      	char *buf;
> >>>>>>>>      +	udl_kill_pending_urbs(dev);
> >>>>>>>> +
> >>>>>>> 
> >>>>>>> I already reviewed the patchset, but I have another comment. I think
> >>>>>>> we should only kill urbs from within the suspend handler. Same for the
> >>>>>>> call to the URB-sync function in patch 2.
> >>>>>>> 
> >>>>>>> This disable function is part of the regular modeset path. It's
> >>>>>>> probably not appropriate to outright remove pending URBs here. This
> >>>>>>> can lead to failed modesets, which would have succeeded otherwise.
> >>>>>> 
> >>>>>> Well, the device shall be turned off right after that point, so the
> >>>>>> all pending rendering makes little sense, no?
> >>>>> 
> >>>>> udl_simple_display_pipe_disable() only disables the display, but not
> >>>>> the device. The kill operation here could potentially kill some valid
> >>>>> modeset operation that was still going on. And who knows what the
> >>>>> device state is after that.
> >>>> 
> >>>> But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
> >>>> command right after the place I've put udl_kill_pending_urbs().  So it
> >>>> shall blank / turn off the power (of the device, as it has a single
> >>>> output).  And the URB completion doesn't do any error handling but
> >>>> just re-links URB chain and wakes up the queue.  So killing a pending
> >>>> URB would nothing but canceling the in-flight URBs, and there should
> >>>> be no disturbance to the modeset operation itself, as the screen will
> >>>> be blanked immediately.
> >>> 
> >>> The blank mode is essentially DPMS. It's unrelated to the device's
> >>> display mode.
> >> 
> >> The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
> >> discard the whole rendered picture.  And, the counterpart,
> >> udl_simple_display_pipe_enable(), re-initializes the mode fully from
> >> the scratch again.
> >> So what's the point to continue rendering that is immediately cleared
> >> (from the screen and from the device state)?  Killing pending URBs
> >> doesn't influence on the internal (modeset) state of the driver.
> > 
> > In anyway, this patchset seems problematic around the disconnection,
> > and maybe this particular one is no much improvement, better to drop
> > for now.
> > 
> > I'll resubmit the v2 patch set including your resume fixes later.
> 
> I already merged the patches before seeing the discussion on the rsp
> bug report. If you submit an update, maybe you can do so with Fixes
> tags?

Oh sure, will check then.


Takashi

[PATCH 4/4] drm/udl: Replace BUG_ON() with WARN_ON()

[Takashi Iwai]

BUG_ON() is a tasteless choice as a sanity check for a driver like UDL
that isn't really a core code.  Replace with WARN_ON() and proper
error handling instead.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c     | 3 ++-
 drivers/gpu/drm/udl/udl_transfer.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 47204b7eb10e..fdafbf8f3c3c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -260,7 +260,8 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	struct udl_device *udl = to_udl(dev);
 	int ret;
 
-	BUG_ON(len > udl->urbs.size);
+	if (WARN_ON(len > udl->urbs.size))
+		return -EINVAL;
 
 	urb->transfer_buffer_length = len; /* set to actual payload len */
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 971927669d6b..176ef2a6a731 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -220,7 +220,8 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 	u8 *cmd = *urb_buf_ptr;
 	u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length;
 
-	BUG_ON(!(log_bpp == 1 || log_bpp == 2));
+	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+		return -EINVAL;
 
 	line_start = (u8 *) (front + byte_offset);
 	next_pixel = line_start;
-- 
2.35.3

[PATCH 4/4] drm/udl: Replace BUG_ON() with WARN_ON()

[Takashi Iwai]

BUG_ON() is a tasteless choice as a sanity check for a driver like UDL
that isn't really a core code.  Replace with WARN_ON() and proper
error handling instead.

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c     | 3 ++-
 drivers/gpu/drm/udl/udl_transfer.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 47204b7eb10e..fdafbf8f3c3c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -260,7 +260,8 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	struct udl_device *udl = to_udl(dev);
 	int ret;
 
-	BUG_ON(len > udl->urbs.size);
+	if (WARN_ON(len > udl->urbs.size))
+		return -EINVAL;
 
 	urb->transfer_buffer_length = len; /* set to actual payload len */
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 971927669d6b..176ef2a6a731 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -220,7 +220,8 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 	u8 *cmd = *urb_buf_ptr;
 	u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length;
 
-	BUG_ON(!(log_bpp == 1 || log_bpp == 2));
+	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+		return -EINVAL;
 
 	line_start = (u8 *) (front + byte_offset);
 	next_pixel = line_start;
-- 
2.35.3

Re: [PATCH 0/4] drm/udl: Fix stray URBs and cleanup

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1299 bytes --]

Hi Takashi,

Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> Hi,
> 
> this is a series of fixes for UDL driver to address the leftover URBs
> at suspend/resume.  It begins with the simplification of FIFO control
> code with the standard wait queue, followed by the handling of pending
> URBs, and replace BUG_ON() with WARN_ON() as a cleanup.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

That's a lot better than what's currently there. If no other reviews 
come in, I'll add the patches next week.

Best regards
Thomas

> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (4):
>    drm/udl: Replace semaphore with a simple wait queue
>    drm/udl: Sync pending URBs at suspend / disconnect
>    drm/udl: Kill pending URBs at suspend and disconnect
>    drm/udl: Replace BUG_ON() with WARN_ON()
> 
>   drivers/gpu/drm/udl/udl_drv.h      |  14 +++-
>   drivers/gpu/drm/udl/udl_main.c     | 125 ++++++++++++++---------------
>   drivers/gpu/drm/udl/udl_modeset.c  |   4 +
>   drivers/gpu/drm/udl/udl_transfer.c |   3 +-
>   4 files changed, 79 insertions(+), 67 deletions(-)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]