Hi,
this is another respin of patch set for cleaning up and fixes for UDL
driver [*]. It covers the PM problems, regressions in the previous
patch set, fixes for the stalls on some systems, as well as more
hardening.
thanks,
Takashi
[*] v2: https://lore.kernel.org/r/[email protected]
===
v2->v3:
- More fix on Restore-on-display-mode patch, suggested by Daniel
- Yet more fix for ubs.count check patch, suggested by Thomas
- Another patch for passing rectangle directly, suggested by Thomas
- Put more Acks from Daniel and Thomas
v1->v2: cleanups as suggested by Thomas
- Drop numurbs parameter patch
- Clean up / simplify clipping patch
- Code cleanup and changes for urb management patch
- Put Acks on some given patches
===
Takashi Iwai (10):
drm/udl: Restore display mode on resume
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: Pass rectangle directly to udl_handle_damage()
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 (2):
drm/udl: Add reset_resume
drm/udl: Enable damage clipping
drivers/gpu/drm/udl/udl_drv.c | 19 +++++-
drivers/gpu/drm/udl/udl_drv.h | 13 +----
drivers/gpu/drm/udl/udl_main.c | 93 +++++++++++++++---------------
drivers/gpu/drm/udl/udl_modeset.c | 54 ++++-------------
drivers/gpu/drm/udl/udl_transfer.c | 45 ++-------------
5 files changed, 80 insertions(+), 144 deletions(-)
--
2.35.3
Just for some code simplification.
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c9b837ac26a7..0142fc6a478a 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
- int x, int y, int width, int height)
+ struct drm_rect *clip)
{
struct drm_device *dev = fb->dev;
void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
int i, ret;
char *cmd;
struct urb *urb;
- struct drm_rect clip;
int log_bpp;
ret = udl_log_cpp(fb->format->cpp[0]);
@@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- drm_rect_init(&clip, x, y, width, height);
-
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
return ret;
@@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
}
cmd = urb->transfer_buffer;
- for (i = clip.y1; i < clip.y2; i++) {
+ for (i = clip->y1; i < clip->y2; i++) {
const int line_offset = fb->pitches[0] * i;
- const int byte_offset = line_offset + (clip.x1 << log_bpp);
- const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
- const int byte_width = (clip.x2 - clip.x1) << log_bpp;
+ const int byte_offset = line_offset + (clip->x1 << log_bpp);
+ const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
+ const int byte_width = (clip->x2 - clip->x1) << log_bpp;
ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
&cmd, byte_offset, dev_byte_offset,
byte_width);
@@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
struct udl_device *udl = to_udl(dev);
struct drm_display_mode *mode = &crtc_state->mode;
struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+ struct drm_rect clip;
char *buf;
char *wrptr;
int color_depth = UDL_COLOR_DEPTH_16BPP;
@@ -354,7 +352,8 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
udl->mode_buf_len = wrptr - buf;
- udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
+ drm_rect_init(&clip, 0, 0, fb->width, fb->height);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
/* enable display */
udl_crtc_write_mode_to_hw(crtc);
@@ -396,8 +395,7 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
return;
if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
- udl_handle_damage(fb, &shadow_plane_state->data[0], rect.x1, rect.y1,
- rect.x2 - rect.x1, rect.y2 - rect.y1);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
}
static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
--
2.35.3
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.
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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
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().
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/gpu/drm/udl/udl_drv.h | 8 +------
drivers/gpu/drm/udl/udl_main.c | 42 +++++++++++++++++++++++-----------
2 files changed, 30 insertions(+), 20 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 de28eeff3155..16aa4a655e7f 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,6 +23,8 @@
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
+static struct urb *udl_get_urb_locked(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);
@@ -146,15 +148,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_locked(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(&udl->urbs.lock);
if (WARN_ON(!urb))
break;
unode = urb->context;
@@ -164,7 +168,8 @@ static void udl_free_urb_list(struct drm_device *dev)
usb_free_urb(urb);
kfree(unode);
}
- udl->urbs.count = 0;
+
+ wake_up_all(&udl->urbs.sleep);
}
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -228,33 +233,44 @@ 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_locked(struct udl_device *udl, long timeout)
{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode = NULL;
+ struct urb_node *unode;
- if (!udl->urbs.count)
- return NULL;
+ assert_spin_locked(&udl->urbs.lock);
/* 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,
+ !udl->urbs.count ||
!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;
}
+ if (!udl->urbs.count)
+ 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_locked(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
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.
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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 829edb60a987..061cb88c08a2 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -290,10 +290,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 */
@@ -301,9 +300,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 0142fc6a478a..d4f409f6d533 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -378,8 +378,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
On Thu, 08 Sep 2022 14:47:52 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 08.09.22 um 11:51 schrieb Takashi Iwai:
> > Just for some code simplification.
> >
> > Suggested-by: Thomas Zimmermann <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> With my comments fixed, you can add
>
> Acked-by: Thomas Zimmermann <[email protected]>
>
> > ---
> > drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index c9b837ac26a7..0142fc6a478a 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
> > static int udl_handle_damage(struct drm_framebuffer *fb,
> > const struct iosys_map *map,
> > - int x, int y, int width, int height)
> > + struct drm_rect *clip)
>
> Should probably be declared const.
>
> > {
> > struct drm_device *dev = fb->dev;
> > void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
> > int i, ret;
> > char *cmd;
> > struct urb *urb;
> > - struct drm_rect clip;
> > int log_bpp;
> > ret = udl_log_cpp(fb->format->cpp[0]);
> > @@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> > return ret;
> > log_bpp = ret;
> > - drm_rect_init(&clip, x, y, width, height);
> > -
> > ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> > if (ret)
> > return ret;
> > @@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> > }
> > cmd = urb->transfer_buffer;
> > - for (i = clip.y1; i < clip.y2; i++) {
> > + for (i = clip->y1; i < clip->y2; i++) {
> > const int line_offset = fb->pitches[0] * i;
> > - const int byte_offset = line_offset + (clip.x1 << log_bpp);
> > - const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
> > - const int byte_width = (clip.x2 - clip.x1) << log_bpp;
> > + const int byte_offset = line_offset + (clip->x1 << log_bpp);
> > + const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
> > + const int byte_width = (clip->x2 - clip->x1) << log_bpp;
>
> Please use drm_rect_width(clip) instead. Somehow there's already too
> much code that open-codes this.
>
> > ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> > &cmd, byte_offset, dev_byte_offset,
> > byte_width);
> > @@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> > struct udl_device *udl = to_udl(dev);
> > struct drm_display_mode *mode = &crtc_state->mode;
> > struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> > + struct drm_rect clip;
>
> Better do a static init with DRM_RECT_INIT(0, 0, fb->width,
> fb->height) and remove the other init call below.
OK, below is the revised patch.
Do you want me a full respin for v4?
Takashi
-- 8< --
From: Takashi Iwai <[email protected]>
Subject: [PATCH] drm/udl: Pass rectangle directly to udl_handle_damage()
Just for some code simplification.
Suggested-by: Thomas Zimmermann <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/gpu/drm/udl/udl_modeset.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c9b837ac26a7..d5e20bf144bc 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
- int x, int y, int width, int height)
+ const struct drm_rect *clip)
{
struct drm_device *dev = fb->dev;
void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
int i, ret;
char *cmd;
struct urb *urb;
- struct drm_rect clip;
int log_bpp;
ret = udl_log_cpp(fb->format->cpp[0]);
@@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- drm_rect_init(&clip, x, y, width, height);
-
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
return ret;
@@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
}
cmd = urb->transfer_buffer;
- for (i = clip.y1; i < clip.y2; i++) {
+ for (i = clip->y1; i < clip->y2; i++) {
const int line_offset = fb->pitches[0] * i;
- const int byte_offset = line_offset + (clip.x1 << log_bpp);
- const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
- const int byte_width = (clip.x2 - clip.x1) << log_bpp;
+ const int byte_offset = line_offset + (clip->x1 << log_bpp);
+ const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
+ const int byte_width = drm_rect_width(clip) << log_bpp;
ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
&cmd, byte_offset, dev_byte_offset,
byte_width);
@@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
struct udl_device *udl = to_udl(dev);
struct drm_display_mode *mode = &crtc_state->mode;
struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+ struct drm_rect clip = DRM_RECT_INIT(0, 0, fb->width, fb->height);
char *buf;
char *wrptr;
int color_depth = UDL_COLOR_DEPTH_16BPP;
@@ -354,7 +352,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
udl->mode_buf_len = wrptr - buf;
- udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
/* enable display */
udl_crtc_write_mode_to_hw(crtc);
@@ -396,8 +394,7 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
return;
if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
- udl_handle_damage(fb, &shadow_plane_state->data[0], rect.x1, rect.y1,
- rect.x2 - rect.x1, rect.y2 - rect.y1);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
}
static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
--
2.35.3
Hi
Am 08.09.22 um 11:51 schrieb Takashi Iwai:
> Just for some code simplification.
>
> Suggested-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
With my comments fixed, you can add
Acked-by: Thomas Zimmermann <[email protected]>
> ---
> drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c9b837ac26a7..0142fc6a478a 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
>
> static int udl_handle_damage(struct drm_framebuffer *fb,
> const struct iosys_map *map,
> - int x, int y, int width, int height)
> + struct drm_rect *clip)
Should probably be declared const.
> {
> struct drm_device *dev = fb->dev;
> void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
> int i, ret;
> char *cmd;
> struct urb *urb;
> - struct drm_rect clip;
> int log_bpp;
>
> ret = udl_log_cpp(fb->format->cpp[0]);
> @@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> return ret;
> log_bpp = ret;
>
> - drm_rect_init(&clip, x, y, width, height);
> -
> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> if (ret)
> return ret;
> @@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> }
> cmd = urb->transfer_buffer;
>
> - for (i = clip.y1; i < clip.y2; i++) {
> + for (i = clip->y1; i < clip->y2; i++) {
> const int line_offset = fb->pitches[0] * i;
> - const int byte_offset = line_offset + (clip.x1 << log_bpp);
> - const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
> - const int byte_width = (clip.x2 - clip.x1) << log_bpp;
> + const int byte_offset = line_offset + (clip->x1 << log_bpp);
> + const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
> + const int byte_width = (clip->x2 - clip->x1) << log_bpp;
Please use drm_rect_width(clip) instead. Somehow there's already too
much code that open-codes this.
> ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> &cmd, byte_offset, dev_byte_offset,
> byte_width);
> @@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> struct udl_device *udl = to_udl(dev);
> struct drm_display_mode *mode = &crtc_state->mode;
> struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> + struct drm_rect clip;
Better do a static init with DRM_RECT_INIT(0, 0, fb->width, fb->height)
and remove the other init call below.
Best regards
Thomas
> char *buf;
> char *wrptr;
> int color_depth = UDL_COLOR_DEPTH_16BPP;
> @@ -354,7 +352,8 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>
> udl->mode_buf_len = wrptr - buf;
>
> - udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
> + drm_rect_init(&clip, 0, 0, fb->width, fb->height);
> + udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
>
> /* enable display */
> udl_crtc_write_mode_to_hw(crtc);
> @@ -396,8 +395,7 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> return;
>
> if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
> - udl_handle_damage(fb, &shadow_plane_state->data[0], rect.x1, rect.y1,
> - rect.x2 - rect.x1, rect.y2 - rect.y1);
> + udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
> }
>
> static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
--
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 08.09.22 um 14:54 schrieb Takashi Iwai:
> On Thu, 08 Sep 2022 14:47:52 +0200,
> Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 08.09.22 um 11:51 schrieb Takashi Iwai:
>>> Just for some code simplification.
>>>
>>> Suggested-by: Thomas Zimmermann <[email protected]>
>>> Signed-off-by: Takashi Iwai <[email protected]>
>>
>> With my comments fixed, you can add
>>
>> Acked-by: Thomas Zimmermann <[email protected]>
>>
>>> ---
>>> drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++-----------
>>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>>> index c9b837ac26a7..0142fc6a478a 100644
>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>>> @@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
>>> static int udl_handle_damage(struct drm_framebuffer *fb,
>>> const struct iosys_map *map,
>>> - int x, int y, int width, int height)
>>> + struct drm_rect *clip)
>>
>> Should probably be declared const.
>>
>>> {
>>> struct drm_device *dev = fb->dev;
>>> void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
>>> int i, ret;
>>> char *cmd;
>>> struct urb *urb;
>>> - struct drm_rect clip;
>>> int log_bpp;
>>> ret = udl_log_cpp(fb->format->cpp[0]);
>>> @@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>>> return ret;
>>> log_bpp = ret;
>>> - drm_rect_init(&clip, x, y, width, height);
>>> -
>>> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>> if (ret)
>>> return ret;
>>> @@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>>> }
>>> cmd = urb->transfer_buffer;
>>> - for (i = clip.y1; i < clip.y2; i++) {
>>> + for (i = clip->y1; i < clip->y2; i++) {
>>> const int line_offset = fb->pitches[0] * i;
>>> - const int byte_offset = line_offset + (clip.x1 << log_bpp);
>>> - const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
>>> - const int byte_width = (clip.x2 - clip.x1) << log_bpp;
>>> + const int byte_offset = line_offset + (clip->x1 << log_bpp);
>>> + const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
>>> + const int byte_width = (clip->x2 - clip->x1) << log_bpp;
>>
>> Please use drm_rect_width(clip) instead. Somehow there's already too
>> much code that open-codes this.
>>
>>> ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
>>> &cmd, byte_offset, dev_byte_offset,
>>> byte_width);
>>> @@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>>> struct udl_device *udl = to_udl(dev);
>>> struct drm_display_mode *mode = &crtc_state->mode;
>>> struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>> + struct drm_rect clip;
>>
>> Better do a static init with DRM_RECT_INIT(0, 0, fb->width,
>> fb->height) and remove the other init call below.
>
> OK, below is the revised patch.
>
> Do you want me a full respin for v4?
The patch looks good to me. I don't think a full v4 would be necessary
unless another major change is requested.
Best regards
Thomas
>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <[email protected]>
> Subject: [PATCH] drm/udl: Pass rectangle directly to udl_handle_damage()
>
> Just for some code simplification.
>
> Suggested-by: Thomas Zimmermann <[email protected]>
> Acked-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> drivers/gpu/drm/udl/udl_modeset.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c9b837ac26a7..d5e20bf144bc 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
>
> static int udl_handle_damage(struct drm_framebuffer *fb,
> const struct iosys_map *map,
> - int x, int y, int width, int height)
> + const struct drm_rect *clip)
> {
> struct drm_device *dev = fb->dev;
> void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
> int i, ret;
> char *cmd;
> struct urb *urb;
> - struct drm_rect clip;
> int log_bpp;
>
> ret = udl_log_cpp(fb->format->cpp[0]);
> @@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> return ret;
> log_bpp = ret;
>
> - drm_rect_init(&clip, x, y, width, height);
> -
> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> if (ret)
> return ret;
> @@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> }
> cmd = urb->transfer_buffer;
>
> - for (i = clip.y1; i < clip.y2; i++) {
> + for (i = clip->y1; i < clip->y2; i++) {
> const int line_offset = fb->pitches[0] * i;
> - const int byte_offset = line_offset + (clip.x1 << log_bpp);
> - const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
> - const int byte_width = (clip.x2 - clip.x1) << log_bpp;
> + const int byte_offset = line_offset + (clip->x1 << log_bpp);
> + const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
> + const int byte_width = drm_rect_width(clip) << log_bpp;
> ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> &cmd, byte_offset, dev_byte_offset,
> byte_width);
> @@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> struct udl_device *udl = to_udl(dev);
> struct drm_display_mode *mode = &crtc_state->mode;
> struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> + struct drm_rect clip = DRM_RECT_INIT(0, 0, fb->width, fb->height);
> char *buf;
> char *wrptr;
> int color_depth = UDL_COLOR_DEPTH_16BPP;
> @@ -354,7 +352,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>
> udl->mode_buf_len = wrptr - buf;
>
> - udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
> + udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
>
> /* enable display */
> udl_crtc_write_mode_to_hw(crtc);
> @@ -396,8 +394,7 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> return;
>
> if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
> - udl_handle_damage(fb, &shadow_plane_state->data[0], rect.x1, rect.y1,
> - rect.x2 - rect.x1, rect.y2 - rect.y1);
> + udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
> }
>
> static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
--
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,
I've meanwhile merged the patchset, including the one updated patch and
the missing r-b.
Best regards
Thomas
Am 08.09.22 um 11:51 schrieb Takashi Iwai:
> Hi,
>
> this is another respin of patch set for cleaning up and fixes for UDL
> driver [*]. It covers the PM problems, regressions in the previous
> patch set, fixes for the stalls on some systems, as well as more
> hardening.
>
>
> thanks,
>
> Takashi
>
> [*] v2: https://lore.kernel.org/r/[email protected]
>
> ===
>
> v2->v3:
> - More fix on Restore-on-display-mode patch, suggested by Daniel
> - Yet more fix for ubs.count check patch, suggested by Thomas
> - Another patch for passing rectangle directly, suggested by Thomas
> - Put more Acks from Daniel and Thomas
>
> v1->v2: cleanups as suggested by Thomas
> - Drop numurbs parameter patch
> - Clean up / simplify clipping patch
> - Code cleanup and changes for urb management patch
> - Put Acks on some given patches
>
> ===
>
> Takashi Iwai (10):
> drm/udl: Restore display mode on resume
> 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: Pass rectangle directly to udl_handle_damage()
> 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 (2):
> drm/udl: Add reset_resume
> drm/udl: Enable damage clipping
>
> drivers/gpu/drm/udl/udl_drv.c | 19 +++++-
> drivers/gpu/drm/udl/udl_drv.h | 13 +----
> drivers/gpu/drm/udl/udl_main.c | 93 +++++++++++++++---------------
> drivers/gpu/drm/udl/udl_modeset.c | 54 ++++-------------
> drivers/gpu/drm/udl/udl_transfer.c | 45 ++-------------
> 5 files changed, 80 insertions(+), 144 deletions(-)
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Mon, 12 Sep 2022 09:06:47 +0200,
Thomas Zimmermann wrote:
>
> Hi,
>
> I've meanwhile merged the patchset, including the one updated patch
> and the missing r-b.
Great, thanks!
Takashi