Hi,
this is a revised 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.
Takashi
===
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 (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 (3):
drm/udl: Restore display mode on resume
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 | 95 +++++++++++++++---------------
drivers/gpu/drm/udl/udl_modeset.c | 36 ++---------
drivers/gpu/drm/udl/udl_transfer.c | 45 ++------------
5 files changed, 75 insertions(+), 133 deletions(-)
--
2.35.3
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()")
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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 2b7eafd48ec2..de28eeff3155 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -260,11 +260,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
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 <[email protected]>
---
drivers/gpu/drm/udl/udl_modeset.c | 28 +--------------------
drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
2 files changed, 1 insertion(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c34d564773a3..9896c16c74f5 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)
@@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
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))
- return -EINVAL;
+ drm_rect_init(&clip, x, y, width, height);
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
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
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 | 44 +++++++++++++++++++++++-----------
2 files changed, 31 insertions(+), 21 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..df7ebe1fdc90 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);
@@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb)
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
- wake_up(&udl->urbs.sleep);
+ wake_up_all(&udl->urbs.sleep);
}
static void udl_free_urb_list(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int count = udl->urbs.count;
struct 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(&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
Hi
Am 06.09.22 um 09:39 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 | 44 +++++++++++++++++++++++-----------
> 2 files changed, 31 insertions(+), 21 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..df7ebe1fdc90 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);
> @@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb)
> udl->urbs.available++;
> spin_unlock_irqrestore(&udl->urbs.lock, flags);
>
> - wake_up(&udl->urbs.sleep);
> + wake_up_all(&udl->urbs.sleep);
> }
>
> static void udl_free_urb_list(struct drm_device *dev)
> {
> struct udl_device *udl = to_udl(dev);
> - int count = udl->urbs.count;
> struct 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(&udl->urbs.sleep);
I meant that we should maybe do a wake_up_all() here and not in the
completion handler udl_urb_completion().
In any case
Acked-by: Thomas Zimmermann <[email protected]>
> }
>
> 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);
--
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 06.09.22 um 09:39 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 <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
with an entirely optional comment below.
> ---
> drivers/gpu/drm/udl/udl_modeset.c | 28 +--------------------
> drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
> 2 files changed, 1 insertion(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c34d564773a3..9896c16c74f5 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)
> @@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> 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))
> - return -EINVAL;
> + drm_rect_init(&clip, x, y, width, height);
The clip rectangle could be passed directly by the caller, which would
simplify the update function. But that's really just nitpicking.
Best regards
Thomas
>
> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> if (ret)
> 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
On Wed, 07 Sep 2022 09:29:37 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 06.09.22 um 09:39 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 <[email protected]>
>
> Acked-by: Thomas Zimmermann <[email protected]>
>
> with an entirely optional comment below.
>
> > ---
> > drivers/gpu/drm/udl/udl_modeset.c | 28 +--------------------
> > drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
> > 2 files changed, 1 insertion(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..9896c16c74f5 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)
> > @@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> > 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))
> > - return -EINVAL;
> > + drm_rect_init(&clip, x, y, width, height);
>
> The clip rectangle could be passed directly by the caller, which would
> simplify the update function. But that's really just nitpicking.
OK, will add a patch to do this, too :)
thanks,
Takashi