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/[email protected]
===
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
From: Thomas Zimmermann <[email protected]>
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 <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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 <[email protected]>
*/
+#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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Thomas Zimmermann <[email protected]>
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 <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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 <[email protected]>
*/
+#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
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 <[email protected]>
---
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
Hi
Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> From: Thomas Zimmermann <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
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 <[email protected]>
> */
>
> +#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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> From: Thomas Zimmermann <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
This patch isn't great and incomplete, see
https://lore.kernel.org/dri-devel/[email protected]/
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 <[email protected]>
> */
>
> +#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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> This patch isn't great and incomplete, see
>
> https://lore.kernel.org/dri-devel/[email protected]/
>
> 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 <[email protected]>
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: <[email protected]>
Signed-off-by: Thomas Zimmermann <[email protected]>
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Takashi Iwai <[email protected]>
> >
> > This patch isn't great and incomplete, see
> >
> > https://lore.kernel.org/dri-devel/[email protected]/
> >
> > 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 <[email protected]>
> 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: <[email protected]>
> Signed-off-by: Thomas Zimmermann <[email protected]>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
Yeah I think that's the right one. But please check that it still fixes
the bug you're seeing :-)
Reviewed-by: Daniel Vetter <[email protected]>
> ---
> 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