[PATCH 00/12] drm/udl: More fixes

[PATCH 00/12] drm/udl: More fixes

[Takashi Iwai]

Hi,

this patch set contains more fixes for UDL driver, to be applied on
top of my previous patch set [*].  It covers the PM problems,
regressions in the previous patch set, fixes for the stalls on some
systems, as well as more hardening.


Takashi

[*] https://lore.kernel.org/r/20220804075826.27036-1-tiwai@suse.de

===

Takashi Iwai (8):
  Revert "drm/udl: Kill pending URBs at suspend and disconnect"
  drm/udl: Suppress error print for -EPROTO at URB completion
  drm/udl: Increase the default URB list size to 20
  drm/udl: Drop unneeded alignment
  drm/udl: Fix potential URB leaks
  drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
  drm/udl: Don't re-initialize stuff at retrying the URB list allocation
  drm/udl: Sync pending URBs at the end of suspend

Thomas Zimmermann (4):
  drm/udl: Restore display mode on resume
  drm/udl: Add reset_resume
  drm/udl: Enable damage clipping
  drm/udl: Add parameter to set number of URBs

 drivers/gpu/drm/udl/udl_drv.c      | 19 +++++-
 drivers/gpu/drm/udl/udl_drv.h      | 13 +---
 drivers/gpu/drm/udl/udl_main.c     | 97 +++++++++++++++---------------
 drivers/gpu/drm/udl/udl_modeset.c  | 42 ++++---------
 drivers/gpu/drm/udl/udl_transfer.c | 45 ++------------
 5 files changed, 86 insertions(+), 130 deletions(-)

-- 
2.35.3

[PATCH 00/12] drm/udl: More fixes

[Takashi Iwai]

Hi,

this patch set contains more fixes for UDL driver, to be applied on
top of my previous patch set [*].  It covers the PM problems,
regressions in the previous patch set, fixes for the stalls on some
systems, as well as more hardening.


Takashi

[*] https://lore.kernel.org/r/20220804075826.27036-1-tiwai@suse.de

===

Takashi Iwai (8):
  Revert "drm/udl: Kill pending URBs at suspend and disconnect"
  drm/udl: Suppress error print for -EPROTO at URB completion
  drm/udl: Increase the default URB list size to 20
  drm/udl: Drop unneeded alignment
  drm/udl: Fix potential URB leaks
  drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
  drm/udl: Don't re-initialize stuff at retrying the URB list allocation
  drm/udl: Sync pending URBs at the end of suspend

Thomas Zimmermann (4):
  drm/udl: Restore display mode on resume
  drm/udl: Add reset_resume
  drm/udl: Enable damage clipping
  drm/udl: Add parameter to set number of URBs

 drivers/gpu/drm/udl/udl_drv.c      | 19 +++++-
 drivers/gpu/drm/udl/udl_drv.h      | 13 +---
 drivers/gpu/drm/udl/udl_main.c     | 97 +++++++++++++++---------------
 drivers/gpu/drm/udl/udl_modeset.c  | 42 ++++---------
 drivers/gpu/drm/udl/udl_transfer.c | 45 ++------------
 5 files changed, 86 insertions(+), 130 deletions(-)

-- 
2.35.3

[PATCH 01/12] drm/udl: Restore display mode on resume

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Restore the display mode whne resuming from suspend. Currently, the
display remains dark.

On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.

The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..df987644fb5d 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
@@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
 
-	if (!crtc_state->mode_changed)
+	if (!drm_atomic_crtc_needs_modeset(crtc_state))
 		return;
 
 	/* enable display */
-- 
2.35.3

[PATCH 01/12] drm/udl: Restore display mode on resume

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Restore the display mode whne resuming from suspend. Currently, the
display remains dark.

On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.

The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..df987644fb5d 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
@@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
 
-	if (!crtc_state->mode_changed)
+	if (!drm_atomic_crtc_needs_modeset(crtc_state))
 		return;
 
 	/* enable display */
-- 
2.35.3

[PATCH 02/12] drm/udl: Add reset_resume

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Implement the reset_resume callback of struct usb_driver. Set the
standard channel when called.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 5703277c6f52..0ba88e5472a9 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -32,6 +32,16 @@ static int udl_usb_resume(struct usb_interface *interface)
 	return drm_mode_config_helper_resume(dev);
 }
 
+static int udl_usb_reset_resume(struct usb_interface *interface)
+{
+	struct drm_device *dev = usb_get_intfdata(interface);
+	struct udl_device *udl = to_udl(dev);
+
+	udl_select_std_channel(udl);
+
+	return drm_mode_config_helper_resume(dev);
+}
+
 /*
  * FIXME: Dma-buf sharing requires DMA support by the importing device.
  *        This function is a workaround to make USB devices work as well.
@@ -140,6 +150,7 @@ static struct usb_driver udl_driver = {
 	.disconnect = udl_usb_disconnect,
 	.suspend = udl_usb_suspend,
 	.resume = udl_usb_resume,
+	.reset_resume = udl_usb_reset_resume,
 	.id_table = id_table,
 };
 module_usb_driver(udl_driver);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 28aaf75d71cf..37c14b0ff1fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -95,6 +95,7 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 		     u32 byte_offset, u32 device_byte_offset, u32 byte_width);
 
 int udl_drop_usb(struct drm_device *dev);
+int udl_select_std_channel(struct udl_device *udl);
 
 #define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
 #define CMD_WRITE_RL8    "\xAF\x61" /**< 8 bit run length command. */
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index fdafbf8f3c3c..7d1e6bbc165c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -92,7 +92,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
 /*
  * Need to ensure a channel is selected before submitting URBs
  */
-static int udl_select_std_channel(struct udl_device *udl)
+int udl_select_std_channel(struct udl_device *udl)
 {
 	static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
 					 0x1C, 0x88, 0x5E, 0x15,
-- 
2.35.3

[PATCH 02/12] drm/udl: Add reset_resume

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Implement the reset_resume callback of struct usb_driver. Set the
standard channel when called.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 5703277c6f52..0ba88e5472a9 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -32,6 +32,16 @@ static int udl_usb_resume(struct usb_interface *interface)
 	return drm_mode_config_helper_resume(dev);
 }
 
+static int udl_usb_reset_resume(struct usb_interface *interface)
+{
+	struct drm_device *dev = usb_get_intfdata(interface);
+	struct udl_device *udl = to_udl(dev);
+
+	udl_select_std_channel(udl);
+
+	return drm_mode_config_helper_resume(dev);
+}
+
 /*
  * FIXME: Dma-buf sharing requires DMA support by the importing device.
  *        This function is a workaround to make USB devices work as well.
@@ -140,6 +150,7 @@ static struct usb_driver udl_driver = {
 	.disconnect = udl_usb_disconnect,
 	.suspend = udl_usb_suspend,
 	.resume = udl_usb_resume,
+	.reset_resume = udl_usb_reset_resume,
 	.id_table = id_table,
 };
 module_usb_driver(udl_driver);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 28aaf75d71cf..37c14b0ff1fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -95,6 +95,7 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 		     u32 byte_offset, u32 device_byte_offset, u32 byte_width);
 
 int udl_drop_usb(struct drm_device *dev);
+int udl_select_std_channel(struct udl_device *udl);
 
 #define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
 #define CMD_WRITE_RL8    "\xAF\x61" /**< 8 bit run length command. */
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index fdafbf8f3c3c..7d1e6bbc165c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -92,7 +92,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
 /*
  * Need to ensure a channel is selected before submitting URBs
  */
-static int udl_select_std_channel(struct udl_device *udl)
+int udl_select_std_channel(struct udl_device *udl)
 {
 	static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
 					 0x1C, 0x88, 0x5E, 0x15,
-- 
2.35.3

[PATCH 03/12] drm/udl: Enable damage clipping

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Call drm_plane_enable_fb_damage_clips() and give userspace a chance
of minimizing the updated display area.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index df987644fb5d..187aba2d7825 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -484,6 +484,7 @@ int udl_modeset_init(struct drm_device *dev)
 					   format_count, NULL, connector);
 	if (ret)
 		return ret;
+	drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane);
 
 	drm_mode_config_reset(dev);
 
-- 
2.35.3

[PATCH 03/12] drm/udl: Enable damage clipping

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

Call drm_plane_enable_fb_damage_clips() and give userspace a chance
of minimizing the updated display area.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index df987644fb5d..187aba2d7825 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -484,6 +484,7 @@ int udl_modeset_init(struct drm_device *dev)
 					   format_count, NULL, connector);
 	if (ret)
 		return ret;
+	drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane);
 
 	drm_mode_config_reset(dev);
 
-- 
2.35.3

[PATCH 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect"

[Takashi Iwai]

This reverts the recent fix commit
  e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
as it turned out to lead to potential hangup at a disconnection, and
it doesn't help much for suspend/resume problem, either.

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, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 37c14b0ff1fc..5923d2e02bc8 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,7 +39,6 @@ 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;
@@ -85,7 +84,6 @@ 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 7d1e6bbc165c..a9f6b710b254 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_move(&unode->entry, &udl->urbs.list);
+	list_add_tail(&unode->entry, &udl->urbs.list);
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
@@ -180,7 +180,6 @@ 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;
@@ -247,7 +246,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_move(&unode->entry, &udl->urbs.in_flight);
+	list_del_init(&unode->entry);
 	udl->urbs.available--;
 
 unlock:
@@ -281,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,
-					 list_empty(&udl->urbs.in_flight),
+					 udl->urbs.available == udl->urbs.count,
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
 		ret = -ETIMEDOUT;
@@ -289,23 +288,6 @@ 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;
@@ -354,7 +336,6 @@ 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 187aba2d7825..c34d564773a3 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -398,8 +398,6 @@ 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 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect"

[Takashi Iwai]

This reverts the recent fix commit
  e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
as it turned out to lead to potential hangup at a disconnection, and
it doesn't help much for suspend/resume problem, either.

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, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 37c14b0ff1fc..5923d2e02bc8 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,7 +39,6 @@ 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;
@@ -85,7 +84,6 @@ 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 7d1e6bbc165c..a9f6b710b254 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_move(&unode->entry, &udl->urbs.list);
+	list_add_tail(&unode->entry, &udl->urbs.list);
 	udl->urbs.available++;
 	spin_unlock_irqrestore(&udl->urbs.lock, flags);
 
@@ -180,7 +180,6 @@ 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;
@@ -247,7 +246,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_move(&unode->entry, &udl->urbs.in_flight);
+	list_del_init(&unode->entry);
 	udl->urbs.available--;
 
 unlock:
@@ -281,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,
-					 list_empty(&udl->urbs.in_flight),
+					 udl->urbs.available == udl->urbs.count,
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
 		ret = -ETIMEDOUT;
@@ -289,23 +288,6 @@ 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;
@@ -354,7 +336,6 @@ 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 187aba2d7825..c34d564773a3 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -398,8 +398,6 @@ 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 05/12] drm/udl: Suppress error print for -EPROTO at URB completion

[Takashi Iwai]

The driver may receive -EPROTO at the URB completion when the device
gets disconnected, and it's a normal situation.  Suppress the error
print for that, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9f6b710b254..6aed6e0f669c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -126,6 +126,7 @@ void udl_urb_completion(struct urb *urb)
 	if (urb->status) {
 		if (!(urb->status == -ENOENT ||
 		    urb->status == -ECONNRESET ||
+		    urb->status == -EPROTO ||
 		    urb->status == -ESHUTDOWN)) {
 			DRM_ERROR("%s - nonzero write bulk status received: %d\n",
 				__func__, urb->status);
-- 
2.35.3

[PATCH 05/12] drm/udl: Suppress error print for -EPROTO at URB completion

[Takashi Iwai]

The driver may receive -EPROTO at the URB completion when the device
gets disconnected, and it's a normal situation.  Suppress the error
print for that, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9f6b710b254..6aed6e0f669c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -126,6 +126,7 @@ void udl_urb_completion(struct urb *urb)
 	if (urb->status) {
 		if (!(urb->status == -ENOENT ||
 		    urb->status == -ECONNRESET ||
+		    urb->status == -EPROTO ||
 		    urb->status == -ESHUTDOWN)) {
 			DRM_ERROR("%s - nonzero write bulk status received: %d\n",
 				__func__, urb->status);
-- 
2.35.3

[PATCH 06/12] drm/udl: Increase the default URB list size to 20

[Takashi Iwai]

It seems that the current size (4) for the URB list is too small on
some devices, and it resulted in the occasional stalls.  Increase the
default URB list size to 20 for working around it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 6aed6e0f669c..2b7eafd48ec2 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -20,7 +20,7 @@
 #define NR_USB_REQUEST_CHANNEL 0x12
 
 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
-#define WRITES_IN_FLIGHT (4)
+#define WRITES_IN_FLIGHT (20)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
-- 
2.35.3

[PATCH 06/12] drm/udl: Increase the default URB list size to 20

[Takashi Iwai]

It seems that the current size (4) for the URB list is too small on
some devices, and it resulted in the occasional stalls.  Increase the
default URB list size to 20 for working around it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 6aed6e0f669c..2b7eafd48ec2 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -20,7 +20,7 @@
 #define NR_USB_REQUEST_CHANNEL 0x12
 
 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
-#define WRITES_IN_FLIGHT (4)
+#define WRITES_IN_FLIGHT (20)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
-- 
2.35.3

[PATCH 07/12] drm/udl: Add parameter to set number of URBs

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

For further debugging and optimization purpose, allow users to adjust
the number of URBs via a new module parameter, numurbs.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 2b7eafd48ec2..3c97f647883f 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -8,6 +8,8 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <linux/moduleparam.h>
+
 #include <drm/drm.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -23,6 +25,9 @@
 #define WRITES_IN_FLIGHT (20)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
+static uint udl_num_urbs = WRITES_IN_FLIGHT;
+module_param_named(numurbs, udl_num_urbs, uint, 0600);
+
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -294,6 +299,8 @@ int udl_init(struct udl_device *udl)
 	struct drm_device *dev = &udl->drm;
 	int ret = -ENOMEM;
 
+	drm_info(dev, "pre-allocating %d URBs\n", udl_num_urbs);
+
 	DRM_DEBUG("\n");
 
 	udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
@@ -311,7 +318,7 @@ int udl_init(struct udl_device *udl)
 	if (udl_select_std_channel(udl))
 		DRM_ERROR("Selecting channel failed\n");
 
-	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
+	if (!udl_alloc_urb_list(dev, udl_num_urbs, MAX_TRANSFER)) {
 		DRM_ERROR("udl_alloc_urb_list failed\n");
 		goto err;
 	}
-- 
2.35.3

[PATCH 07/12] drm/udl: Add parameter to set number of URBs

[Takashi Iwai]

From: Thomas Zimmermann <tzimmermann@suse.de>

For further debugging and optimization purpose, allow users to adjust
the number of URBs via a new module parameter, numurbs.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 2b7eafd48ec2..3c97f647883f 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -8,6 +8,8 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <linux/moduleparam.h>
+
 #include <drm/drm.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -23,6 +25,9 @@
 #define WRITES_IN_FLIGHT (20)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
 
+static uint udl_num_urbs = WRITES_IN_FLIGHT;
+module_param_named(numurbs, udl_num_urbs, uint, 0600);
+
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -294,6 +299,8 @@ int udl_init(struct udl_device *udl)
 	struct drm_device *dev = &udl->drm;
 	int ret = -ENOMEM;
 
+	drm_info(dev, "pre-allocating %d URBs\n", udl_num_urbs);
+
 	DRM_DEBUG("\n");
 
 	udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
@@ -311,7 +318,7 @@ int udl_init(struct udl_device *udl)
 	if (udl_select_std_channel(udl))
 		DRM_ERROR("Selecting channel failed\n");
 
-	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
+	if (!udl_alloc_urb_list(dev, udl_num_urbs, MAX_TRANSFER)) {
 		DRM_ERROR("udl_alloc_urb_list failed\n");
 		goto err;
 	}
-- 
2.35.3

[PATCH 08/12] drm/udl: Drop unneeded alignment

[Takashi Iwai]

The alignment of damaged area was needed for the original udlfb driver
that tried to trim the superfluous copies between front and backend
buffers and handle data in long int.  It's not the case for udl DRM
driver, hence we can omit the whole unneeded alignment, as well as the
dead code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
 drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
 2 files changed, 8 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c34d564773a3..bca31c890108 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
 	return __ffs(cpp);
 }
 
-static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
-				   int width, int height)
-{
-	int x1, x2;
-
-	if (WARN_ON_ONCE(x < 0) ||
-	    WARN_ON_ONCE(y < 0) ||
-	    WARN_ON_ONCE(width < 0) ||
-	    WARN_ON_ONCE(height < 0))
-		return -EINVAL;
-
-	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
-	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
-
-	clip->x1 = x1;
-	clip->y1 = y;
-	clip->x2 = x2;
-	clip->y2 = y + height;
-
-	return 0;
-}
-
 static int udl_handle_damage(struct drm_framebuffer *fb,
 			     const struct iosys_map *map,
 			     int x, int y, int width, int height)
@@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 	struct drm_rect clip;
 	int log_bpp;
 
+	if (width <= 0 || height <= 0)
+		return 0;
+
 	ret = udl_log_cpp(fb->format->cpp[0]);
 	if (ret < 0)
 		return ret;
 	log_bpp = ret;
 
-	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
-	if (ret)
-		return ret;
-	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
+	clip.x1 = x;
+	clip.y1 = y;
+	clip.x2 = x + width;
+	clip.y2 = y + height;
+	if (clip.x2 > fb->width || clip.y2 > fb->height)
 		return -EINVAL;
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 176ef2a6a731..a431208dda85 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -25,46 +25,6 @@
 #define MIN_RAW_PIX_BYTES	2
 #define MIN_RAW_CMD_BYTES	(RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
 
-/*
- * Trims identical data from front and back of line
- * Sets new front buffer address and width
- * And returns byte count of identical pixels
- * Assumes CPU natural alignment (unsigned long)
- * for back and front buffer ptrs and width
- */
-#if 0
-static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
-{
-	int j, k;
-	const unsigned long *back = (const unsigned long *) bback;
-	const unsigned long *front = (const unsigned long *) *bfront;
-	const int width = *width_bytes / sizeof(unsigned long);
-	int identical = width;
-	int start = width;
-	int end = width;
-
-	for (j = 0; j < width; j++) {
-		if (back[j] != front[j]) {
-			start = j;
-			break;
-		}
-	}
-
-	for (k = width - 1; k > j; k--) {
-		if (back[k] != front[k]) {
-			end = k+1;
-			break;
-		}
-	}
-
-	identical = start + (width - end);
-	*bfront = (u8 *) &front[start];
-	*width_bytes = (end - start) * sizeof(unsigned long);
-
-	return identical * sizeof(unsigned long);
-}
-#endif
-
 static inline u16 pixel32_to_be16(const uint32_t pixel)
 {
 	return (((pixel >> 3) & 0x001f) |
-- 
2.35.3

[PATCH 08/12] drm/udl: Drop unneeded alignment

[Takashi Iwai]

The alignment of damaged area was needed for the original udlfb driver
that tried to trim the superfluous copies between front and backend
buffers and handle data in long int.  It's not the case for udl DRM
driver, hence we can omit the whole unneeded alignment, as well as the
dead code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
 drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
 2 files changed, 8 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c34d564773a3..bca31c890108 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
 	return __ffs(cpp);
 }
 
-static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
-				   int width, int height)
-{
-	int x1, x2;
-
-	if (WARN_ON_ONCE(x < 0) ||
-	    WARN_ON_ONCE(y < 0) ||
-	    WARN_ON_ONCE(width < 0) ||
-	    WARN_ON_ONCE(height < 0))
-		return -EINVAL;
-
-	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
-	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
-
-	clip->x1 = x1;
-	clip->y1 = y;
-	clip->x2 = x2;
-	clip->y2 = y + height;
-
-	return 0;
-}
-
 static int udl_handle_damage(struct drm_framebuffer *fb,
 			     const struct iosys_map *map,
 			     int x, int y, int width, int height)
@@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 	struct drm_rect clip;
 	int log_bpp;
 
+	if (width <= 0 || height <= 0)
+		return 0;
+
 	ret = udl_log_cpp(fb->format->cpp[0]);
 	if (ret < 0)
 		return ret;
 	log_bpp = ret;
 
-	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
-	if (ret)
-		return ret;
-	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
+	clip.x1 = x;
+	clip.y1 = y;
+	clip.x2 = x + width;
+	clip.y2 = y + height;
+	if (clip.x2 > fb->width || clip.y2 > fb->height)
 		return -EINVAL;
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 176ef2a6a731..a431208dda85 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -25,46 +25,6 @@
 #define MIN_RAW_PIX_BYTES	2
 #define MIN_RAW_CMD_BYTES	(RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
 
-/*
- * Trims identical data from front and back of line
- * Sets new front buffer address and width
- * And returns byte count of identical pixels
- * Assumes CPU natural alignment (unsigned long)
- * for back and front buffer ptrs and width
- */
-#if 0
-static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
-{
-	int j, k;
-	const unsigned long *back = (const unsigned long *) bback;
-	const unsigned long *front = (const unsigned long *) *bfront;
-	const int width = *width_bytes / sizeof(unsigned long);
-	int identical = width;
-	int start = width;
-	int end = width;
-
-	for (j = 0; j < width; j++) {
-		if (back[j] != front[j]) {
-			start = j;
-			break;
-		}
-	}
-
-	for (k = width - 1; k > j; k--) {
-		if (back[k] != front[k]) {
-			end = k+1;
-			break;
-		}
-	}
-
-	identical = start + (width - end);
-	*bfront = (u8 *) &front[start];
-	*width_bytes = (end - start) * sizeof(unsigned long);
-
-	return identical * sizeof(unsigned long);
-}
-#endif
-
 static inline u16 pixel32_to_be16(const uint32_t pixel)
 {
 	return (((pixel >> 3) & 0x001f) |
-- 
2.35.3

[PATCH 09/12] drm/udl: Fix potential URB leaks

[Takashi Iwai]

A couple of error handlings forgot to process the URB completion.
Those are both with WARN_ON() so should be visible, but we must fix
them in anyway.

Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c     | 8 +++++---
 drivers/gpu/drm/udl/udl_transfer.c | 5 ++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 3c97f647883f..8bbb4e2861fb 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -265,11 +265,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	struct udl_device *udl = to_udl(dev);
 	int ret;
 
-	if (WARN_ON(len > udl->urbs.size))
-		return -EINVAL;
-
+	if (WARN_ON(len > udl->urbs.size)) {
+		ret = -EINVAL;
+		goto error;
+	}
 	urb->transfer_buffer_length = len; /* set to actual payload len */
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
+ error:
 	if (ret) {
 		udl_urb_completion(urb); /* because no one else will */
 		DRM_ERROR("usb_submit_urb error %x\n", ret);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index a431208dda85..b57844632dbd 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -180,8 +180,11 @@ 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;
 
-	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
+		/* need to finish URB at error from this function */
+		udl_urb_completion(urb);
 		return -EINVAL;
+	}
 
 	line_start = (u8 *) (front + byte_offset);
 	next_pixel = line_start;
-- 
2.35.3

[PATCH 09/12] drm/udl: Fix potential URB leaks

[Takashi Iwai]

A couple of error handlings forgot to process the URB completion.
Those are both with WARN_ON() so should be visible, but we must fix
them in anyway.

Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c     | 8 +++++---
 drivers/gpu/drm/udl/udl_transfer.c | 5 ++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 3c97f647883f..8bbb4e2861fb 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -265,11 +265,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	struct udl_device *udl = to_udl(dev);
 	int ret;
 
-	if (WARN_ON(len > udl->urbs.size))
-		return -EINVAL;
-
+	if (WARN_ON(len > udl->urbs.size)) {
+		ret = -EINVAL;
+		goto error;
+	}
 	urb->transfer_buffer_length = len; /* set to actual payload len */
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
+ error:
 	if (ret) {
 		udl_urb_completion(urb); /* because no one else will */
 		DRM_ERROR("usb_submit_urb error %x\n", ret);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index a431208dda85..b57844632dbd 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -180,8 +180,11 @@ 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;
 
-	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
+		/* need to finish URB at error from this function */
+		udl_urb_completion(urb);
 		return -EINVAL;
+	}
 
 	line_start = (u8 *) (front + byte_offset);
 	next_pixel = line_start;
-- 
2.35.3

[PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Takashi Iwai]

In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.

For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
 drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@ 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_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);
-}
+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);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 8bbb4e2861fb..19dc8317e843 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -28,6 +28,8 @@
 static uint udl_num_urbs = WRITES_IN_FLIGHT;
 module_param_named(numurbs, udl_num_urbs, uint, 0600);
 
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
+
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
 static void udl_free_urb_list(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
-	int count = udl->urbs.count;
 	struct urb_node *unode;
 	struct urb *urb;
 
 	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
 
 	/* keep waiting and freeing, until we've got 'em all */
-	while (count--) {
-		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+	while (udl->urbs.count) {
+		spin_lock_irq(&udl->urbs.lock);
+		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+		udl->urbs.count--;
+		spin_unlock_irq(&udl->urbs.lock);
 		if (WARN_ON(!urb))
 			break;
 		unode = urb->context;
@@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
 		usb_free_urb(urb);
 		kfree(unode);
 	}
-	udl->urbs.count = 0;
+
+	wake_up(&udl->urbs.sleep);
 }
 
 static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	return udl->urbs.count;
 }
 
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
 {
-	struct udl_device *udl = to_udl(dev);
-	struct urb_node *unode = NULL;
+	struct urb_node *unode;
+
+	assert_spin_locked(&udl->urbs.lock);
 
 	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;
+		return NULL;
 	}
 
 	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);
 	return unode ? unode->urb : NULL;
 }
 
+#define GET_URB_TIMEOUT	HZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	struct urb *urb;
+
+	spin_lock_irq(&udl->urbs.lock);
+	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+	spin_unlock_irq(&udl->urbs.lock);
+	return urb;
+}
+
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 {
 	struct udl_device *udl = to_udl(dev);
-- 
2.35.3

[PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Takashi Iwai]

In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.

For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
 drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@ 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_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);
-}
+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);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 8bbb4e2861fb..19dc8317e843 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -28,6 +28,8 @@
 static uint udl_num_urbs = WRITES_IN_FLIGHT;
 module_param_named(numurbs, udl_num_urbs, uint, 0600);
 
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
+
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
 static void udl_free_urb_list(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
-	int count = udl->urbs.count;
 	struct urb_node *unode;
 	struct urb *urb;
 
 	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
 
 	/* keep waiting and freeing, until we've got 'em all */
-	while (count--) {
-		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+	while (udl->urbs.count) {
+		spin_lock_irq(&udl->urbs.lock);
+		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+		udl->urbs.count--;
+		spin_unlock_irq(&udl->urbs.lock);
 		if (WARN_ON(!urb))
 			break;
 		unode = urb->context;
@@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
 		usb_free_urb(urb);
 		kfree(unode);
 	}
-	udl->urbs.count = 0;
+
+	wake_up(&udl->urbs.sleep);
 }
 
 static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	return udl->urbs.count;
 }
 
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
 {
-	struct udl_device *udl = to_udl(dev);
-	struct urb_node *unode = NULL;
+	struct urb_node *unode;
+
+	assert_spin_locked(&udl->urbs.lock);
 
 	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;
+		return NULL;
 	}
 
 	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);
 	return unode ? unode->urb : NULL;
 }
 
+#define GET_URB_TIMEOUT	HZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	struct urb *urb;
+
+	spin_lock_irq(&udl->urbs.lock);
+	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+	spin_unlock_irq(&udl->urbs.lock);
+	return urb;
+}
+
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 {
 	struct udl_device *udl = to_udl(dev);
-- 
2.35.3

[PATCH 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation

[Takashi Iwai]

udl_alloc_urb_list() retires the allocation if there is no enough room
left, and it reinitializes the stuff unnecessarily such as the linked
list head and the waitqueue, which could be harmful.  Those should be
outside the retry loop.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 19dc8317e843..c1f4b6199949 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -187,15 +187,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	struct usb_device *udev = udl_to_usb_device(udl);
 
 	spin_lock_init(&udl->urbs.lock);
-
-retry:
-	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
-
 	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
 	udl->urbs.available = 0;
 
+retry:
+	udl->urbs.size = size;
+
 	while (udl->urbs.count * size < wanted_size) {
 		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
 		if (!unode)
-- 
2.35.3

[PATCH 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation

[Takashi Iwai]

udl_alloc_urb_list() retires the allocation if there is no enough room
left, and it reinitializes the stuff unnecessarily such as the linked
list head and the waitqueue, which could be harmful.  Those should be
outside the retry loop.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 19dc8317e843..c1f4b6199949 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -187,15 +187,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	struct usb_device *udev = udl_to_usb_device(udl);
 
 	spin_lock_init(&udl->urbs.lock);
-
-retry:
-	udl->urbs.size = size;
 	INIT_LIST_HEAD(&udl->urbs.list);
-
 	init_waitqueue_head(&udl->urbs.sleep);
 	udl->urbs.count = 0;
 	udl->urbs.available = 0;
 
+retry:
+	udl->urbs.size = size;
+
 	while (udl->urbs.count * size < wanted_size) {
 		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
 		if (!unode)
-- 
2.35.3

[PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend

[Takashi Iwai]

It's better to perform the sync at the very last of the suspend
instead of the pipe-disable function, so that we can catch all pending
URBs (if any).

While we're at it, drop the error code from udl_sync_pending_urb()
since we basically ignore it; instead, give a clear error message
indicating a problem.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.c     | 8 +++++++-
 drivers/gpu/drm/udl/udl_drv.h     | 2 +-
 drivers/gpu/drm/udl/udl_main.c    | 6 ++----
 drivers/gpu/drm/udl/udl_modeset.c | 2 --
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 0ba88e5472a9..91effdcefb6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
 			   pm_message_t message)
 {
 	struct drm_device *dev = usb_get_intfdata(interface);
+	int ret;
 
-	return drm_mode_config_helper_suspend(dev);
+	ret = drm_mode_config_helper_suspend(dev);
+	if (ret)
+		return ret;
+
+	udl_sync_pending_urbs(dev);
+	return 0;
 }
 
 static int udl_usb_resume(struct usb_interface *interface)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d943684b5bbb..b4cc7cc568c7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev);
 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_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 c1f4b6199949..df92f6518e1c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -294,10 +294,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 }
 
 /* wait until all pending URBs have been processed */
-int udl_sync_pending_urbs(struct drm_device *dev)
+void 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 */
@@ -305,9 +304,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 					 udl->urbs.available == udl->urbs.count,
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
-		ret = -ETIMEDOUT;
+		drm_err(dev, "Timeout for syncing pending URBs\n");
 	spin_unlock_irq(&udl->urbs.lock);
-	return ret;
 }
 
 int udl_init(struct udl_device *udl)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index bca31c890108..9d72288d9967 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -391,8 +391,6 @@ 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 12/12] drm/udl: Sync pending URBs at the end of suspend

[Takashi Iwai]

It's better to perform the sync at the very last of the suspend
instead of the pipe-disable function, so that we can catch all pending
URBs (if any).

While we're at it, drop the error code from udl_sync_pending_urb()
since we basically ignore it; instead, give a clear error message
indicating a problem.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.c     | 8 +++++++-
 drivers/gpu/drm/udl/udl_drv.h     | 2 +-
 drivers/gpu/drm/udl/udl_main.c    | 6 ++----
 drivers/gpu/drm/udl/udl_modeset.c | 2 --
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 0ba88e5472a9..91effdcefb6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
 			   pm_message_t message)
 {
 	struct drm_device *dev = usb_get_intfdata(interface);
+	int ret;
 
-	return drm_mode_config_helper_suspend(dev);
+	ret = drm_mode_config_helper_suspend(dev);
+	if (ret)
+		return ret;
+
+	udl_sync_pending_urbs(dev);
+	return 0;
 }
 
 static int udl_usb_resume(struct usb_interface *interface)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d943684b5bbb..b4cc7cc568c7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev);
 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_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 c1f4b6199949..df92f6518e1c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -294,10 +294,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 }
 
 /* wait until all pending URBs have been processed */
-int udl_sync_pending_urbs(struct drm_device *dev)
+void 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 */
@@ -305,9 +304,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
 					 udl->urbs.available == udl->urbs.count,
 					 udl->urbs.lock,
 					 msecs_to_jiffies(2000)))
-		ret = -ETIMEDOUT;
+		drm_err(dev, "Timeout for syncing pending URBs\n");
 	spin_unlock_irq(&udl->urbs.lock);
-	return ret;
 }
 
 int udl_init(struct udl_device *udl)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index bca31c890108..9d72288d9967 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -391,8 +391,6 @@ 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

Re: [PATCH 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect"

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> This reverts the recent fix commit
>    e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
> as it turned out to lead to potential hangup at a disconnection, and
> it doesn't help much for suspend/resume problem, either.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Acked-by: Thomas Zimmermann <tzimmermann@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, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 37c14b0ff1fc..5923d2e02bc8 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -39,7 +39,6 @@ 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;
> @@ -85,7 +84,6 @@ 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 7d1e6bbc165c..a9f6b710b254 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_move(&unode->entry, &udl->urbs.list);
> +	list_add_tail(&unode->entry, &udl->urbs.list);
>   	udl->urbs.available++;
>   	spin_unlock_irqrestore(&udl->urbs.lock, flags);
>   
> @@ -180,7 +180,6 @@ 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;
> @@ -247,7 +246,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_move(&unode->entry, &udl->urbs.in_flight);
> +	list_del_init(&unode->entry);
>   	udl->urbs.available--;
>   
>   unlock:
> @@ -281,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,
> -					 list_empty(&udl->urbs.in_flight),
> +					 udl->urbs.available == udl->urbs.count,
>   					 udl->urbs.lock,
>   					 msecs_to_jiffies(2000)))
>   		ret = -ETIMEDOUT;
> @@ -289,23 +288,6 @@ 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;
> @@ -354,7 +336,6 @@ 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 187aba2d7825..c34d564773a3 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -398,8 +398,6 @@ 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;

-- 
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 06/12] drm/udl: Increase the default URB list size to 20

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> It seems that the current size (4) for the URB list is too small on
> some devices, and it resulted in the occasional stalls.  Increase the
> default URB list size to 20 for working around it.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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

> ---
>   drivers/gpu/drm/udl/udl_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 6aed6e0f669c..2b7eafd48ec2 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -20,7 +20,7 @@
>   #define NR_USB_REQUEST_CHANNEL 0x12
>   
>   #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
> -#define WRITES_IN_FLIGHT (4)
> +#define WRITES_IN_FLIGHT (20)
>   #define MAX_VENDOR_DESCRIPTOR_SIZE 256
>   
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)

-- 
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 06/12] drm/udl: Increase the default URB list size to 20

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> It seems that the current size (4) for the URB list is too small on
> some devices, and it resulted in the occasional stalls.  Increase the
> default URB list size to 20 for working around it.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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

> ---
>   drivers/gpu/drm/udl/udl_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 6aed6e0f669c..2b7eafd48ec2 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -20,7 +20,7 @@
>   #define NR_USB_REQUEST_CHANNEL 0x12
>   
>   #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
> -#define WRITES_IN_FLIGHT (4)
> +#define WRITES_IN_FLIGHT (20)
>   #define MAX_VENDOR_DESCRIPTOR_SIZE 256
>   
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)

-- 
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 07/12] drm/udl: Add parameter to set number of URBs

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> From: Thomas Zimmermann <tzimmermann@suse.de>
> 
> For further debugging and optimization purpose, allow users to adjust
> the number of URBs via a new module parameter, numurbs.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I made this patch for debugging, but I don't think it should be added to 
the upstream kernel. Please don't merge.

Best regards
Thomas

> ---
>   drivers/gpu/drm/udl/udl_main.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 2b7eafd48ec2..3c97f647883f 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -8,6 +8,8 @@
>    * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
>    */
>   
> +#include <linux/moduleparam.h>
> +
>   #include <drm/drm.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> @@ -23,6 +25,9 @@
>   #define WRITES_IN_FLIGHT (20)
>   #define MAX_VENDOR_DESCRIPTOR_SIZE 256
>   
> +static uint udl_num_urbs = WRITES_IN_FLIGHT;
> +module_param_named(numurbs, udl_num_urbs, uint, 0600);
> +
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)
>   {
>   	struct usb_device *udev = udl_to_usb_device(udl);
> @@ -294,6 +299,8 @@ int udl_init(struct udl_device *udl)
>   	struct drm_device *dev = &udl->drm;
>   	int ret = -ENOMEM;
>   
> +	drm_info(dev, "pre-allocating %d URBs\n", udl_num_urbs);
> +
>   	DRM_DEBUG("\n");
>   
>   	udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
> @@ -311,7 +318,7 @@ int udl_init(struct udl_device *udl)
>   	if (udl_select_std_channel(udl))
>   		DRM_ERROR("Selecting channel failed\n");
>   
> -	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
> +	if (!udl_alloc_urb_list(dev, udl_num_urbs, MAX_TRANSFER)) {
>   		DRM_ERROR("udl_alloc_urb_list failed\n");
>   		goto err;
>   	}

-- 
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 07/12] drm/udl: Add parameter to set number of URBs

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> From: Thomas Zimmermann <tzimmermann@suse.de>
> 
> For further debugging and optimization purpose, allow users to adjust
> the number of URBs via a new module parameter, numurbs.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I made this patch for debugging, but I don't think it should be added to 
the upstream kernel. Please don't merge.

Best regards
Thomas

> ---
>   drivers/gpu/drm/udl/udl_main.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 2b7eafd48ec2..3c97f647883f 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -8,6 +8,8 @@
>    * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
>    */
>   
> +#include <linux/moduleparam.h>
> +
>   #include <drm/drm.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> @@ -23,6 +25,9 @@
>   #define WRITES_IN_FLIGHT (20)
>   #define MAX_VENDOR_DESCRIPTOR_SIZE 256
>   
> +static uint udl_num_urbs = WRITES_IN_FLIGHT;
> +module_param_named(numurbs, udl_num_urbs, uint, 0600);
> +
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)
>   {
>   	struct usb_device *udev = udl_to_usb_device(udl);
> @@ -294,6 +299,8 @@ int udl_init(struct udl_device *udl)
>   	struct drm_device *dev = &udl->drm;
>   	int ret = -ENOMEM;
>   
> +	drm_info(dev, "pre-allocating %d URBs\n", udl_num_urbs);
> +
>   	DRM_DEBUG("\n");
>   
>   	udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
> @@ -311,7 +318,7 @@ int udl_init(struct udl_device *udl)
>   	if (udl_select_std_channel(udl))
>   		DRM_ERROR("Selecting channel failed\n");
>   
> -	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
> +	if (!udl_alloc_urb_list(dev, udl_num_urbs, MAX_TRANSFER)) {
>   		DRM_ERROR("udl_alloc_urb_list failed\n");
>   		goto err;
>   	}

-- 
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 09/12] drm/udl: Fix potential URB leaks

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> A couple of error handlings forgot to process the URB completion.
> Those are both with WARN_ON() so should be visible, but we must fix
> them in anyway.
> 
> Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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

> ---
>   drivers/gpu/drm/udl/udl_main.c     | 8 +++++---
>   drivers/gpu/drm/udl/udl_transfer.c | 5 ++++-
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 3c97f647883f..8bbb4e2861fb 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -265,11 +265,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
>   	struct udl_device *udl = to_udl(dev);
>   	int ret;
>   
> -	if (WARN_ON(len > udl->urbs.size))
> -		return -EINVAL;
> -
> +	if (WARN_ON(len > udl->urbs.size)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
>   	urb->transfer_buffer_length = len; /* set to actual payload len */
>   	ret = usb_submit_urb(urb, GFP_ATOMIC);
> + error:
>   	if (ret) {
>   		udl_urb_completion(urb); /* because no one else will */
>   		DRM_ERROR("usb_submit_urb error %x\n", ret);
> diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
> index a431208dda85..b57844632dbd 100644
> --- a/drivers/gpu/drm/udl/udl_transfer.c
> +++ b/drivers/gpu/drm/udl/udl_transfer.c
> @@ -180,8 +180,11 @@ 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;
>   
> -	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
> +	if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
> +		/* need to finish URB at error from this function */
> +		udl_urb_completion(urb);
>   		return -EINVAL;
> +	}
>   
>   	line_start = (u8 *) (front + byte_offset);
>   	next_pixel = line_start;

-- 
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 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> In the current design, udl_get_urb() may be called asynchronously
> during the driver freeing its URL list via udl_free_urb_list().
> The problem is that the sync is determined by comparing the urbs.count
> and urbs.available fields, while we clear urbs.count field only once
> after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> the state becomes inconsistent.
> 
> For fixing this inconsistency and also for hardening the locking
> scheme, this patch does a slight refactoring of the code around
> udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> the same spinlock at extracting a URB from the list in
> udl_free_url_list().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
>   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
>   2 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 5923d2e02bc8..d943684b5bbb 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -74,13 +74,7 @@ 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_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);
> -}
> +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);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 8bbb4e2861fb..19dc8317e843 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -28,6 +28,8 @@
>   static uint udl_num_urbs = WRITES_IN_FLIGHT;
>   module_param_named(numurbs, udl_num_urbs, uint, 0600);
>   
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
> +
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)
>   {
>   	struct usb_device *udev = udl_to_usb_device(udl);
> @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
>   static void udl_free_urb_list(struct drm_device *dev)
>   {
>   	struct udl_device *udl = to_udl(dev);
> -	int count = udl->urbs.count;
>   	struct urb_node *unode;
>   	struct urb *urb;
>   
>   	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
>   
>   	/* keep waiting and freeing, until we've got 'em all */
> -	while (count--) {
> -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> +	while (udl->urbs.count) {
> +		spin_lock_irq(&udl->urbs.lock);
> +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> +		udl->urbs.count--;
> +		spin_unlock_irq(&udl->urbs.lock);
>   		if (WARN_ON(!urb))
>   			break;
>   		unode = urb->context;
> @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
>   		usb_free_urb(urb);
>   		kfree(unode);
>   	}
> -	udl->urbs.count = 0;
> +
> +	wake_up(&udl->urbs.sleep);

There's just one waiter, but it's the shutdown code. Maybe wake_up_all() 
would more clearly communicate the intention.

>   }
>   
>   static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   	return udl->urbs.count;
>   }
>   
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)

I think in DRM, the correct name for this function would be 
udl_get_urb_locked().

>   {
> -	struct udl_device *udl = to_udl(dev);
> -	struct urb_node *unode = NULL;
> +	struct urb_node *unode;
> +
> +	assert_spin_locked(&udl->urbs.lock);
>   
>   	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),

The urb-free function will wake up this code, but the urb list should be 
empty then. Should we update the condition to something like 
'udl->urbs.count && !list_empty()' ?

Best regards
Thomas

>   					 udl->urbs.lock, timeout)) {
>   		DRM_INFO("wait for urb interrupted: available: %d\n",
>   			 udl->urbs.available);
> -		goto unlock;
> +		return NULL;
>   	}
>   
>   	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);
>   	return unode ? unode->urb : NULL;
>   }
>   
> +#define GET_URB_TIMEOUT	HZ
> +struct urb *udl_get_urb(struct drm_device *dev)
> +{
> +	struct udl_device *udl = to_udl(dev);
> +	struct urb *urb;
> +
> +	spin_lock_irq(&udl->urbs.lock);
> +	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
> +	spin_unlock_irq(&udl->urbs.lock);
> +	return urb;
> +}
> +
>   int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
>   {
>   	struct udl_device *udl = to_udl(dev);

-- 
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 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> In the current design, udl_get_urb() may be called asynchronously
> during the driver freeing its URL list via udl_free_urb_list().
> The problem is that the sync is determined by comparing the urbs.count
> and urbs.available fields, while we clear urbs.count field only once
> after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> the state becomes inconsistent.
> 
> For fixing this inconsistency and also for hardening the locking
> scheme, this patch does a slight refactoring of the code around
> udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> the same spinlock at extracting a URB from the list in
> udl_free_url_list().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
>   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
>   2 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 5923d2e02bc8..d943684b5bbb 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -74,13 +74,7 @@ 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_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);
> -}
> +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);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 8bbb4e2861fb..19dc8317e843 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -28,6 +28,8 @@
>   static uint udl_num_urbs = WRITES_IN_FLIGHT;
>   module_param_named(numurbs, udl_num_urbs, uint, 0600);
>   
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
> +
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)
>   {
>   	struct usb_device *udev = udl_to_usb_device(udl);
> @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
>   static void udl_free_urb_list(struct drm_device *dev)
>   {
>   	struct udl_device *udl = to_udl(dev);
> -	int count = udl->urbs.count;
>   	struct urb_node *unode;
>   	struct urb *urb;
>   
>   	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
>   
>   	/* keep waiting and freeing, until we've got 'em all */
> -	while (count--) {
> -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> +	while (udl->urbs.count) {
> +		spin_lock_irq(&udl->urbs.lock);
> +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> +		udl->urbs.count--;
> +		spin_unlock_irq(&udl->urbs.lock);
>   		if (WARN_ON(!urb))
>   			break;
>   		unode = urb->context;
> @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
>   		usb_free_urb(urb);
>   		kfree(unode);
>   	}
> -	udl->urbs.count = 0;
> +
> +	wake_up(&udl->urbs.sleep);

There's just one waiter, but it's the shutdown code. Maybe wake_up_all() 
would more clearly communicate the intention.

>   }
>   
>   static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   	return udl->urbs.count;
>   }
>   
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)

I think in DRM, the correct name for this function would be 
udl_get_urb_locked().

>   {
> -	struct udl_device *udl = to_udl(dev);
> -	struct urb_node *unode = NULL;
> +	struct urb_node *unode;
> +
> +	assert_spin_locked(&udl->urbs.lock);
>   
>   	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),

The urb-free function will wake up this code, but the urb list should be 
empty then. Should we update the condition to something like 
'udl->urbs.count && !list_empty()' ?

Best regards
Thomas

>   					 udl->urbs.lock, timeout)) {
>   		DRM_INFO("wait for urb interrupted: available: %d\n",
>   			 udl->urbs.available);
> -		goto unlock;
> +		return NULL;
>   	}
>   
>   	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);
>   	return unode ? unode->urb : NULL;
>   }
>   
> +#define GET_URB_TIMEOUT	HZ
> +struct urb *udl_get_urb(struct drm_device *dev)
> +{
> +	struct udl_device *udl = to_udl(dev);
> +	struct urb *urb;
> +
> +	spin_lock_irq(&udl->urbs.lock);
> +	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
> +	spin_unlock_irq(&udl->urbs.lock);
> +	return urb;
> +}
> +
>   int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
>   {
>   	struct udl_device *udl = to_udl(dev);

-- 
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 08/12] drm/udl: Drop unneeded alignment

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> The alignment of damaged area was needed for the original udlfb driver
> that tried to trim the superfluous copies between front and backend
> buffers and handle data in long int.  It's not the case for udl DRM
> driver, hence we can omit the whole unneeded alignment, as well as the
> dead code.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
>   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
>   2 files changed, 8 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c34d564773a3..bca31c890108 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
>   	return __ffs(cpp);
>   }
>   
> -static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
> -				   int width, int height)
> -{
> -	int x1, x2;
> -
> -	if (WARN_ON_ONCE(x < 0) ||
> -	    WARN_ON_ONCE(y < 0) ||
> -	    WARN_ON_ONCE(width < 0) ||
> -	    WARN_ON_ONCE(height < 0))
> -		return -EINVAL;
> -
> -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> -
> -	clip->x1 = x1;
> -	clip->y1 = y;
> -	clip->x2 = x2;
> -	clip->y2 = y + height;
> -
> -	return 0;
> -}
> -
>   static int udl_handle_damage(struct drm_framebuffer *fb,
>   			     const struct iosys_map *map,
>   			     int x, int y, int width, int height)
> @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>   	struct drm_rect clip;
>   	int log_bpp;
>   
> +	if (width <= 0 || height <= 0)
> +		return 0;
> +

That shouldn't happen.

>   	ret = udl_log_cpp(fb->format->cpp[0]);
>   	if (ret < 0)
>   		return ret;
>   	log_bpp = ret;
>   
> -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> -	if (ret)
> -		return ret;
> -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> +	clip.x1 = x;
> +	clip.y1 = y;
> +	clip.x2 = x + width;
> +	clip.y2 = y + height;

drm_rect_init() please.

> +	if (clip.x2 > fb->width || clip.y2 > fb->height)

That's another thing that should not happen. The damage clips in the 
plane state is what you what to copy. The DRM helpers ensure that these 
various plane, fb and clip coordinates add up.

Best regards
Thomas

>   		return -EINVAL;
>   
>   	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
> index 176ef2a6a731..a431208dda85 100644
> --- a/drivers/gpu/drm/udl/udl_transfer.c
> +++ b/drivers/gpu/drm/udl/udl_transfer.c
> @@ -25,46 +25,6 @@
>   #define MIN_RAW_PIX_BYTES	2
>   #define MIN_RAW_CMD_BYTES	(RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
>   
> -/*
> - * Trims identical data from front and back of line
> - * Sets new front buffer address and width
> - * And returns byte count of identical pixels
> - * Assumes CPU natural alignment (unsigned long)
> - * for back and front buffer ptrs and width
> - */
> -#if 0
> -static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
> -{
> -	int j, k;
> -	const unsigned long *back = (const unsigned long *) bback;
> -	const unsigned long *front = (const unsigned long *) *bfront;
> -	const int width = *width_bytes / sizeof(unsigned long);
> -	int identical = width;
> -	int start = width;
> -	int end = width;
> -
> -	for (j = 0; j < width; j++) {
> -		if (back[j] != front[j]) {
> -			start = j;
> -			break;
> -		}
> -	}
> -
> -	for (k = width - 1; k > j; k--) {
> -		if (back[k] != front[k]) {
> -			end = k+1;
> -			break;
> -		}
> -	}
> -
> -	identical = start + (width - end);
> -	*bfront = (u8 *) &front[start];
> -	*width_bytes = (end - start) * sizeof(unsigned long);
> -
> -	return identical * sizeof(unsigned long);
> -}
> -#endif
> -
>   static inline u16 pixel32_to_be16(const uint32_t pixel)
>   {
>   	return (((pixel >> 3) & 0x001f) |

-- 
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 08/12] drm/udl: Drop unneeded alignment

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> The alignment of damaged area was needed for the original udlfb driver
> that tried to trim the superfluous copies between front and backend
> buffers and handle data in long int.  It's not the case for udl DRM
> driver, hence we can omit the whole unneeded alignment, as well as the
> dead code.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
>   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
>   2 files changed, 8 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c34d564773a3..bca31c890108 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
>   	return __ffs(cpp);
>   }
>   
> -static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
> -				   int width, int height)
> -{
> -	int x1, x2;
> -
> -	if (WARN_ON_ONCE(x < 0) ||
> -	    WARN_ON_ONCE(y < 0) ||
> -	    WARN_ON_ONCE(width < 0) ||
> -	    WARN_ON_ONCE(height < 0))
> -		return -EINVAL;
> -
> -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> -
> -	clip->x1 = x1;
> -	clip->y1 = y;
> -	clip->x2 = x2;
> -	clip->y2 = y + height;
> -
> -	return 0;
> -}
> -
>   static int udl_handle_damage(struct drm_framebuffer *fb,
>   			     const struct iosys_map *map,
>   			     int x, int y, int width, int height)
> @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>   	struct drm_rect clip;
>   	int log_bpp;
>   
> +	if (width <= 0 || height <= 0)
> +		return 0;
> +

That shouldn't happen.

>   	ret = udl_log_cpp(fb->format->cpp[0]);
>   	if (ret < 0)
>   		return ret;
>   	log_bpp = ret;
>   
> -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> -	if (ret)
> -		return ret;
> -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> +	clip.x1 = x;
> +	clip.y1 = y;
> +	clip.x2 = x + width;
> +	clip.y2 = y + height;

drm_rect_init() please.

> +	if (clip.x2 > fb->width || clip.y2 > fb->height)

That's another thing that should not happen. The damage clips in the 
plane state is what you what to copy. The DRM helpers ensure that these 
various plane, fb and clip coordinates add up.

Best regards
Thomas

>   		return -EINVAL;
>   
>   	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
> index 176ef2a6a731..a431208dda85 100644
> --- a/drivers/gpu/drm/udl/udl_transfer.c
> +++ b/drivers/gpu/drm/udl/udl_transfer.c
> @@ -25,46 +25,6 @@
>   #define MIN_RAW_PIX_BYTES	2
>   #define MIN_RAW_CMD_BYTES	(RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
>   
> -/*
> - * Trims identical data from front and back of line
> - * Sets new front buffer address and width
> - * And returns byte count of identical pixels
> - * Assumes CPU natural alignment (unsigned long)
> - * for back and front buffer ptrs and width
> - */
> -#if 0
> -static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
> -{
> -	int j, k;
> -	const unsigned long *back = (const unsigned long *) bback;
> -	const unsigned long *front = (const unsigned long *) *bfront;
> -	const int width = *width_bytes / sizeof(unsigned long);
> -	int identical = width;
> -	int start = width;
> -	int end = width;
> -
> -	for (j = 0; j < width; j++) {
> -		if (back[j] != front[j]) {
> -			start = j;
> -			break;
> -		}
> -	}
> -
> -	for (k = width - 1; k > j; k--) {
> -		if (back[k] != front[k]) {
> -			end = k+1;
> -			break;
> -		}
> -	}
> -
> -	identical = start + (width - end);
> -	*bfront = (u8 *) &front[start];
> -	*width_bytes = (end - start) * sizeof(unsigned long);
> -
> -	return identical * sizeof(unsigned long);
> -}
> -#endif
> -
>   static inline u16 pixel32_to_be16(const uint32_t pixel)
>   {
>   	return (((pixel >> 3) & 0x001f) |

-- 
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 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation

[Thomas Zimmermann]

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



Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> udl_alloc_urb_list() retires the allocation if there is no enough room
> left, and it reinitializes the stuff unnecessarily such as the linked
> list head and the waitqueue, which could be harmful.  Those should be
> outside the retry loop.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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

> ---
>   drivers/gpu/drm/udl/udl_main.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 19dc8317e843..c1f4b6199949 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -187,15 +187,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   	struct usb_device *udev = udl_to_usb_device(udl);
>   
>   	spin_lock_init(&udl->urbs.lock);
> -
> -retry:
> -	udl->urbs.size = size;
>   	INIT_LIST_HEAD(&udl->urbs.list);
> -
>   	init_waitqueue_head(&udl->urbs.sleep);
>   	udl->urbs.count = 0;
>   	udl->urbs.available = 0;
>   
> +retry:
> +	udl->urbs.size = size;
> +
>   	while (udl->urbs.count * size < wanted_size) {
>   		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
>   		if (!unode)

-- 
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 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation

[Thomas Zimmermann]

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



Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> udl_alloc_urb_list() retires the allocation if there is no enough room
> left, and it reinitializes the stuff unnecessarily such as the linked
> list head and the waitqueue, which could be harmful.  Those should be
> outside the retry loop.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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

> ---
>   drivers/gpu/drm/udl/udl_main.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 19dc8317e843..c1f4b6199949 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -187,15 +187,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   	struct usb_device *udev = udl_to_usb_device(udl);
>   
>   	spin_lock_init(&udl->urbs.lock);
> -
> -retry:
> -	udl->urbs.size = size;
>   	INIT_LIST_HEAD(&udl->urbs.list);
> -
>   	init_waitqueue_head(&udl->urbs.sleep);
>   	udl->urbs.count = 0;
>   	udl->urbs.available = 0;
>   
> +retry:
> +	udl->urbs.size = size;
> +
>   	while (udl->urbs.count * size < wanted_size) {
>   		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
>   		if (!unode)

-- 
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 12/12] drm/udl: Sync pending URBs at the end of suspend

[Thomas Zimmermann]

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

Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> It's better to perform the sync at the very last of the suspend
> instead of the pipe-disable function, so that we can catch all pending
> URBs (if any).
> 
> While we're at it, drop the error code from udl_sync_pending_urb()
> since we basically ignore it; instead, give a clear error message
> indicating a problem.

But if we fail, shouldn't we report that error to the caller of the 
suspend function?

Best regards
Thomas

> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_drv.c     | 8 +++++++-
>   drivers/gpu/drm/udl/udl_drv.h     | 2 +-
>   drivers/gpu/drm/udl/udl_main.c    | 6 ++----
>   drivers/gpu/drm/udl/udl_modeset.c | 2 --
>   4 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 0ba88e5472a9..91effdcefb6d 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
>   			   pm_message_t message)
>   {
>   	struct drm_device *dev = usb_get_intfdata(interface);
> +	int ret;
>   
> -	return drm_mode_config_helper_suspend(dev);
> +	ret = drm_mode_config_helper_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	udl_sync_pending_urbs(dev);
> +	return 0;
>   }
>   
>   static int udl_usb_resume(struct usb_interface *interface)
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index d943684b5bbb..b4cc7cc568c7 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev);
>   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_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 c1f4b6199949..df92f6518e1c 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -294,10 +294,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
>   }
>   
>   /* wait until all pending URBs have been processed */
> -int udl_sync_pending_urbs(struct drm_device *dev)
> +void 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 */
> @@ -305,9 +304,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>   					 udl->urbs.available == udl->urbs.count,
>   					 udl->urbs.lock,
>   					 msecs_to_jiffies(2000)))
> -		ret = -ETIMEDOUT;
> +		drm_err(dev, "Timeout for syncing pending URBs\n");
>   	spin_unlock_irq(&udl->urbs.lock);
> -	return ret;
>   }
>   
>   int udl_init(struct udl_device *udl)
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index bca31c890108..9d72288d9967 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -391,8 +391,6 @@ 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

-- 
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 08/12] drm/udl: Drop unneeded alignment

[Takashi Iwai]

On Mon, 05 Sep 2022 10:40:58 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > The alignment of damaged area was needed for the original udlfb driver
> > that tried to trim the superfluous copies between front and backend
> > buffers and handle data in long int.  It's not the case for udl DRM
> > driver, hence we can omit the whole unneeded alignment, as well as the
> > dead code.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
> >   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
> >   2 files changed, 8 insertions(+), 66 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..bca31c890108 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
> >   	return __ffs(cpp);
> >   }
> >   -static int udl_aligned_damage_clip(struct drm_rect *clip, int x,
> > int y,
> > -				   int width, int height)
> > -{
> > -	int x1, x2;
> > -
> > -	if (WARN_ON_ONCE(x < 0) ||
> > -	    WARN_ON_ONCE(y < 0) ||
> > -	    WARN_ON_ONCE(width < 0) ||
> > -	    WARN_ON_ONCE(height < 0))
> > -		return -EINVAL;
> > -
> > -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> > -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> > -
> > -	clip->x1 = x1;
> > -	clip->y1 = y;
> > -	clip->x2 = x2;
> > -	clip->y2 = y + height;
> > -
> > -	return 0;
> > -}
> > -
> >   static int udl_handle_damage(struct drm_framebuffer *fb,
> >   			     const struct iosys_map *map,
> >   			     int x, int y, int width, int height)
> > @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> >   	struct drm_rect clip;
> >   	int log_bpp;
> >   +	if (width <= 0 || height <= 0)
> > +		return 0;
> > +
> 
> That shouldn't happen.
> 
> >   	ret = udl_log_cpp(fb->format->cpp[0]);
> >   	if (ret < 0)
> >   		return ret;
> >   	log_bpp = ret;
> >   -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> > -	if (ret)
> > -		return ret;
> > -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > +	clip.x1 = x;
> > +	clip.y1 = y;
> > +	clip.x2 = x + width;
> > +	clip.y2 = y + height;
> 
> drm_rect_init() please.
> 
> > +	if (clip.x2 > fb->width || clip.y2 > fb->height)
> 
> That's another thing that should not happen. The damage clips in the
> plane state is what you what to copy. The DRM helpers ensure that
> these various plane, fb and clip coordinates add up.

OK, then we can drop those clip size checks completely.
Will do that in v2 patch.


thanks,

Takashi

Re: [PATCH 08/12] drm/udl: Drop unneeded alignment

[Takashi Iwai]

On Mon, 05 Sep 2022 10:40:58 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > The alignment of damaged area was needed for the original udlfb driver
> > that tried to trim the superfluous copies between front and backend
> > buffers and handle data in long int.  It's not the case for udl DRM
> > driver, hence we can omit the whole unneeded alignment, as well as the
> > dead code.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
> >   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
> >   2 files changed, 8 insertions(+), 66 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..bca31c890108 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
> >   	return __ffs(cpp);
> >   }
> >   -static int udl_aligned_damage_clip(struct drm_rect *clip, int x,
> > int y,
> > -				   int width, int height)
> > -{
> > -	int x1, x2;
> > -
> > -	if (WARN_ON_ONCE(x < 0) ||
> > -	    WARN_ON_ONCE(y < 0) ||
> > -	    WARN_ON_ONCE(width < 0) ||
> > -	    WARN_ON_ONCE(height < 0))
> > -		return -EINVAL;
> > -
> > -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> > -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> > -
> > -	clip->x1 = x1;
> > -	clip->y1 = y;
> > -	clip->x2 = x2;
> > -	clip->y2 = y + height;
> > -
> > -	return 0;
> > -}
> > -
> >   static int udl_handle_damage(struct drm_framebuffer *fb,
> >   			     const struct iosys_map *map,
> >   			     int x, int y, int width, int height)
> > @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> >   	struct drm_rect clip;
> >   	int log_bpp;
> >   +	if (width <= 0 || height <= 0)
> > +		return 0;
> > +
> 
> That shouldn't happen.
> 
> >   	ret = udl_log_cpp(fb->format->cpp[0]);
> >   	if (ret < 0)
> >   		return ret;
> >   	log_bpp = ret;
> >   -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> > -	if (ret)
> > -		return ret;
> > -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > +	clip.x1 = x;
> > +	clip.y1 = y;
> > +	clip.x2 = x + width;
> > +	clip.y2 = y + height;
> 
> drm_rect_init() please.
> 
> > +	if (clip.x2 > fb->width || clip.y2 > fb->height)
> 
> That's another thing that should not happen. The damage clips in the
> plane state is what you what to copy. The DRM helpers ensure that
> these various plane, fb and clip coordinates add up.

OK, then we can drop those clip size checks completely.
Will do that in v2 patch.


thanks,

Takashi

Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Takashi Iwai]

On Mon, 05 Sep 2022 10:32:55 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > In the current design, udl_get_urb() may be called asynchronously
> > during the driver freeing its URL list via udl_free_urb_list().
> > The problem is that the sync is determined by comparing the urbs.count
> > and urbs.available fields, while we clear urbs.count field only once
> > after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> > the state becomes inconsistent.
> > 
> > For fixing this inconsistency and also for hardening the locking
> > scheme, this patch does a slight refactoring of the code around
> > udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> > the same spinlock at extracting a URB from the list in
> > udl_free_url_list().
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
> >   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
> >   2 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 5923d2e02bc8..d943684b5bbb 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -74,13 +74,7 @@ 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_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);
> > -}
> > +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);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 8bbb4e2861fb..19dc8317e843 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -28,6 +28,8 @@
> >   static uint udl_num_urbs = WRITES_IN_FLIGHT;
> >   module_param_named(numurbs, udl_num_urbs, uint, 0600);
> >   +static struct urb *__udl_get_urb(struct udl_device *udl, long
> > timeout);
> > +
> >   static int udl_parse_vendor_descriptor(struct udl_device *udl)
> >   {
> >   	struct usb_device *udev = udl_to_usb_device(udl);
> > @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
> >   static void udl_free_urb_list(struct drm_device *dev)
> >   {
> >   	struct udl_device *udl = to_udl(dev);
> > -	int count = udl->urbs.count;
> >   	struct urb_node *unode;
> >   	struct urb *urb;
> >     	DRM_DEBUG("Waiting for completes and freeing all render
> > urbs\n");
> >     	/* keep waiting and freeing, until we've got 'em all */
> > -	while (count--) {
> > -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> > +	while (udl->urbs.count) {
> > +		spin_lock_irq(&udl->urbs.lock);
> > +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > +		udl->urbs.count--;
> > +		spin_unlock_irq(&udl->urbs.lock);
> >   		if (WARN_ON(!urb))
> >   			break;
> >   		unode = urb->context;
> > @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
> >   		usb_free_urb(urb);
> >   		kfree(unode);
> >   	}
> > -	udl->urbs.count = 0;
> > +
> > +	wake_up(&udl->urbs.sleep);
> 
> There's just one waiter, but it's the shutdown code. Maybe
> wake_up_all() would more clearly communicate the intention.

OK.

> >   }
> >     static int udl_alloc_urb_list(struct drm_device *dev, int count,
> > size_t size)
> > @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> >   	return udl->urbs.count;
> >   }
> >   -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout)
> > +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
> 
> I think in DRM, the correct name for this function would be
> udl_get_urb_locked().

OK.

> >   {
> > -	struct udl_device *udl = to_udl(dev);
> > -	struct urb_node *unode = NULL;
> > +	struct urb_node *unode;
> > +
> > +	assert_spin_locked(&udl->urbs.lock);
> >     	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),
> 
> The urb-free function will wake up this code, but the urb list should
> be empty then. Should we update the condition to something like
> 'udl->urbs.count && !list_empty()' ?

This must not happen as this function itself is the only one who takes
out the element from the list.  But it's OK to add the zero check
there just to be sure...


thanks,

Takashi

Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

[Takashi Iwai]

On Mon, 05 Sep 2022 10:32:55 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > In the current design, udl_get_urb() may be called asynchronously
> > during the driver freeing its URL list via udl_free_urb_list().
> > The problem is that the sync is determined by comparing the urbs.count
> > and urbs.available fields, while we clear urbs.count field only once
> > after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> > the state becomes inconsistent.
> > 
> > For fixing this inconsistency and also for hardening the locking
> > scheme, this patch does a slight refactoring of the code around
> > udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> > the same spinlock at extracting a URB from the list in
> > udl_free_url_list().
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
> >   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
> >   2 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 5923d2e02bc8..d943684b5bbb 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -74,13 +74,7 @@ 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_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);
> > -}
> > +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);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 8bbb4e2861fb..19dc8317e843 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -28,6 +28,8 @@
> >   static uint udl_num_urbs = WRITES_IN_FLIGHT;
> >   module_param_named(numurbs, udl_num_urbs, uint, 0600);
> >   +static struct urb *__udl_get_urb(struct udl_device *udl, long
> > timeout);
> > +
> >   static int udl_parse_vendor_descriptor(struct udl_device *udl)
> >   {
> >   	struct usb_device *udev = udl_to_usb_device(udl);
> > @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
> >   static void udl_free_urb_list(struct drm_device *dev)
> >   {
> >   	struct udl_device *udl = to_udl(dev);
> > -	int count = udl->urbs.count;
> >   	struct urb_node *unode;
> >   	struct urb *urb;
> >     	DRM_DEBUG("Waiting for completes and freeing all render
> > urbs\n");
> >     	/* keep waiting and freeing, until we've got 'em all */
> > -	while (count--) {
> > -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> > +	while (udl->urbs.count) {
> > +		spin_lock_irq(&udl->urbs.lock);
> > +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > +		udl->urbs.count--;
> > +		spin_unlock_irq(&udl->urbs.lock);
> >   		if (WARN_ON(!urb))
> >   			break;
> >   		unode = urb->context;
> > @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
> >   		usb_free_urb(urb);
> >   		kfree(unode);
> >   	}
> > -	udl->urbs.count = 0;
> > +
> > +	wake_up(&udl->urbs.sleep);
> 
> There's just one waiter, but it's the shutdown code. Maybe
> wake_up_all() would more clearly communicate the intention.

OK.

> >   }
> >     static int udl_alloc_urb_list(struct drm_device *dev, int count,
> > size_t size)
> > @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> >   	return udl->urbs.count;
> >   }
> >   -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout)
> > +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
> 
> I think in DRM, the correct name for this function would be
> udl_get_urb_locked().

OK.

> >   {
> > -	struct udl_device *udl = to_udl(dev);
> > -	struct urb_node *unode = NULL;
> > +	struct urb_node *unode;
> > +
> > +	assert_spin_locked(&udl->urbs.lock);
> >     	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),
> 
> The urb-free function will wake up this code, but the urb list should
> be empty then. Should we update the condition to something like
> 'udl->urbs.count && !list_empty()' ?

This must not happen as this function itself is the only one who takes
out the element from the list.  But it's OK to add the zero check
there just to be sure...


thanks,

Takashi

Re: [PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend

[Takashi Iwai]

On Mon, 05 Sep 2022 10:44:25 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > It's better to perform the sync at the very last of the suspend
> > instead of the pipe-disable function, so that we can catch all pending
> > URBs (if any).
> > 
> > While we're at it, drop the error code from udl_sync_pending_urb()
> > since we basically ignore it; instead, give a clear error message
> > indicating a problem.
> 
> But if we fail, shouldn't we report that error to the caller of the
> suspend function?

It's an open question.  We may fail the suspend, but OTOH, the sync
error is likely nothing we can recover from at any time later, either;
that is, even if we return an error and abort the suspend, it wouldn't
help so much from the practical POV.  So for now -- and just for this
URL device handling -- I'm inclined to continue suspending.  But if
anyone has more strong argument against it, I'm all ears.


thanks,

Takashi

Re: [PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend

[Takashi Iwai]

On Mon, 05 Sep 2022 10:44:25 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > It's better to perform the sync at the very last of the suspend
> > instead of the pipe-disable function, so that we can catch all pending
> > URBs (if any).
> > 
> > While we're at it, drop the error code from udl_sync_pending_urb()
> > since we basically ignore it; instead, give a clear error message
> > indicating a problem.
> 
> But if we fail, shouldn't we report that error to the caller of the
> suspend function?

It's an open question.  We may fail the suspend, but OTOH, the sync
error is likely nothing we can recover from at any time later, either;
that is, even if we return an error and abort the suspend, it wouldn't
help so much from the practical POV.  So for now -- and just for this
URL device handling -- I'm inclined to continue suspending.  But if
anyone has more strong argument against it, I'm all ears.


thanks,

Takashi

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Daniel Vetter]

On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> From: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Restore the display mode whne resuming from suspend. Currently, the
> display remains dark.
> 
> On resume, the CRTC's mode does not change, but the 'active' flag
> changes to 'true'. Taking this into account when considering a mode
> switch restores the display mode.
> 
> The bug is reproducable by using Gnome with udl and observing the
> adapter's suspend/resume behavior.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

This patch isn't great and incomplete, see

https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/

You need cc: stable and fixes: 997d33c35618 and actually just remove the
entire check :-)
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 169110d8fc2e..df987644fb5d 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
>   */
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_damage_helper.h>
> @@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
>  
> -	if (!crtc_state->mode_changed)
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>  		return;
>  
>  	/* enable display */
> -- 
> 2.35.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Daniel Vetter]

On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> From: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Restore the display mode whne resuming from suspend. Currently, the
> display remains dark.
> 
> On resume, the CRTC's mode does not change, but the 'active' flag
> changes to 'true'. Taking this into account when considering a mode
> switch restores the display mode.
> 
> The bug is reproducable by using Gnome with udl and observing the
> adapter's suspend/resume behavior.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

This patch isn't great and incomplete, see

https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/

You need cc: stable and fixes: 997d33c35618 and actually just remove the
entire check :-)
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 169110d8fc2e..df987644fb5d 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
>   */
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_damage_helper.h>
> @@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
>  
> -	if (!crtc_state->mode_changed)
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>  		return;
>  
>  	/* enable display */
> -- 
> 2.35.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Takashi Iwai]

On Tue, 06 Sep 2022 22:06:55 +0200,
Daniel Vetter wrote:
> 
> On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > From: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > Restore the display mode whne resuming from suspend. Currently, the
> > display remains dark.
> > 
> > On resume, the CRTC's mode does not change, but the 'active' flag
> > changes to 'true'. Taking this into account when considering a mode
> > switch restores the display mode.
> > 
> > The bug is reproducable by using Gnome with udl and observing the
> > adapter's suspend/resume behavior.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> This patch isn't great and incomplete, see
> 
> https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
> 
> You need cc: stable and fixes: 997d33c35618 and actually just remove the
> entire check :-)

OK, then is something like below?

I already submitted v2 yesterday (as I overlooked your reply), so I'll
respin v3 with this (and your ack) if that's OK.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/udl: Restore display mode on resume

Restore the display mode whne resuming from suspend. Currently, the
display remains dark.

On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.

The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.

Actually, the whole check added in udl_simple_display_pipe_enable()
about the crtc_state->mode_changed was bogus.  We should drop the
whole check and always apply the mode change in this function.

[ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
  suggestion ]

Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable functions")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..34ce5b43c5db 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
 
-	if (!crtc_state->mode_changed)
-		return;
-
 	/* enable display */
 	udl_crtc_write_mode_to_hw(crtc);
 }
-- 
2.35.3

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Takashi Iwai]

On Tue, 06 Sep 2022 22:06:55 +0200,
Daniel Vetter wrote:
> 
> On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > From: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > Restore the display mode whne resuming from suspend. Currently, the
> > display remains dark.
> > 
> > On resume, the CRTC's mode does not change, but the 'active' flag
> > changes to 'true'. Taking this into account when considering a mode
> > switch restores the display mode.
> > 
> > The bug is reproducable by using Gnome with udl and observing the
> > adapter's suspend/resume behavior.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> This patch isn't great and incomplete, see
> 
> https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
> 
> You need cc: stable and fixes: 997d33c35618 and actually just remove the
> entire check :-)

OK, then is something like below?

I already submitted v2 yesterday (as I overlooked your reply), so I'll
respin v3 with this (and your ack) if that's OK.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/udl: Restore display mode on resume

Restore the display mode whne resuming from suspend. Currently, the
display remains dark.

On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.

The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.

Actually, the whole check added in udl_simple_display_pipe_enable()
about the crtc_state->mode_changed was bogus.  We should drop the
whole check and always apply the mode change in this function.

[ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
  suggestion ]

Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable functions")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..34ce5b43c5db 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
 
-	if (!crtc_state->mode_changed)
-		return;
-
 	/* enable display */
 	udl_crtc_write_mode_to_hw(crtc);
 }
-- 
2.35.3

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Daniel Vetter]

On Wed, Sep 07, 2022 at 07:51:05AM +0200, Takashi Iwai wrote:
> On Tue, 06 Sep 2022 22:06:55 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > > From: Thomas Zimmermann <tzimmermann@suse.de>
> > > 
> > > Restore the display mode whne resuming from suspend. Currently, the
> > > display remains dark.
> > > 
> > > On resume, the CRTC's mode does not change, but the 'active' flag
> > > changes to 'true'. Taking this into account when considering a mode
> > > switch restores the display mode.
> > > 
> > > The bug is reproducable by using Gnome with udl and observing the
> > > adapter's suspend/resume behavior.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > This patch isn't great and incomplete, see
> > 
> > https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
> > 
> > You need cc: stable and fixes: 997d33c35618 and actually just remove the
> > entire check :-)
> 
> OK, then is something like below?
> 
> I already submitted v2 yesterday (as I overlooked your reply), so I'll
> respin v3 with this (and your ack) if that's OK.
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] drm/udl: Restore display mode on resume
> 
> Restore the display mode whne resuming from suspend. Currently, the
> display remains dark.
> 
> On resume, the CRTC's mode does not change, but the 'active' flag
> changes to 'true'. Taking this into account when considering a mode
> switch restores the display mode.
> 
> The bug is reproducable by using Gnome with udl and observing the
> adapter's suspend/resume behavior.
> 
> Actually, the whole check added in udl_simple_display_pipe_enable()
> about the crtc_state->mode_changed was bogus.  We should drop the
> whole check and always apply the mode change in this function.
> 
> [ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
>   suggestion ]
> 
> Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable functions")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Yeah I think that's the right one. But please check that it still fixes
the bug you're seeing :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 169110d8fc2e..34ce5b43c5db 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
>  
> -	if (!crtc_state->mode_changed)
> -		return;
> -
>  	/* enable display */
>  	udl_crtc_write_mode_to_hw(crtc);
>  }
> -- 
> 2.35.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Daniel Vetter]

On Wed, Sep 07, 2022 at 07:51:05AM +0200, Takashi Iwai wrote:
> On Tue, 06 Sep 2022 22:06:55 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > > From: Thomas Zimmermann <tzimmermann@suse.de>
> > > 
> > > Restore the display mode whne resuming from suspend. Currently, the
> > > display remains dark.
> > > 
> > > On resume, the CRTC's mode does not change, but the 'active' flag
> > > changes to 'true'. Taking this into account when considering a mode
> > > switch restores the display mode.
> > > 
> > > The bug is reproducable by using Gnome with udl and observing the
> > > adapter's suspend/resume behavior.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > This patch isn't great and incomplete, see
> > 
> > https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
> > 
> > You need cc: stable and fixes: 997d33c35618 and actually just remove the
> > entire check :-)
> 
> OK, then is something like below?
> 
> I already submitted v2 yesterday (as I overlooked your reply), so I'll
> respin v3 with this (and your ack) if that's OK.
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] drm/udl: Restore display mode on resume
> 
> Restore the display mode whne resuming from suspend. Currently, the
> display remains dark.
> 
> On resume, the CRTC's mode does not change, but the 'active' flag
> changes to 'true'. Taking this into account when considering a mode
> switch restores the display mode.
> 
> The bug is reproducable by using Gnome with udl and observing the
> adapter's suspend/resume behavior.
> 
> Actually, the whole check added in udl_simple_display_pipe_enable()
> about the crtc_state->mode_changed was bogus.  We should drop the
> whole check and always apply the mode change in this function.
> 
> [ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
>   suggestion ]
> 
> Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable functions")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Yeah I think that's the right one. But please check that it still fixes
the bug you're seeing :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 169110d8fc2e..34ce5b43c5db 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
>  
> -	if (!crtc_state->mode_changed)
> -		return;
> -
>  	/* enable display */
>  	udl_crtc_write_mode_to_hw(crtc);
>  }
> -- 
> 2.35.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch