Udl's damage-handling code requires clean up before switching the driver to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables and statistics. Patches 3 to 5 reorganizes the damage handler to be more readable. Patch 6 moves dma-buf begin/end calls into the damage handler, so it will run during page-flip andmodeset operations.
The patchset has been tested by running fbdev console emulation, X11, and Weston with a DisplayLink adapter.
Thomas Zimmermann (7): drm/udl: Remove unused statistics counters drm/udl: Don't track number of identical and sent pixels per line drm/udl: Vmap framebuffer after all tests succeeded in damage handling drm/udl: Move clip-rectangle code out of udl_handle_damage() drm/udl: Move log-cpp code out of udl_damage_handler() drm/udl: Begin/end access to imported buffers in damage-handler drm/udl: Remove field lost_pixels from struct udl_device
drivers/gpu/drm/udl/udl_drv.h | 8 +- drivers/gpu/drm/udl/udl_fb.c | 129 +++++++++++++++-------------- drivers/gpu/drm/udl/udl_main.c | 3 - drivers/gpu/drm/udl/udl_transfer.c | 12 ++- 4 files changed, 72 insertions(+), 80 deletions(-)
-- 2.23.0
None of the udl driver's statistics counters is used anywhere. Remove them.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_drv.h | 4 ---- drivers/gpu/drm/udl/udl_fb.c | 14 +------------- 2 files changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 66cbe04f832a..42426407c318 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -66,10 +66,6 @@ struct udl_device {
char mode_buf[1024]; uint32_t mode_buf_len; - atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */ - atomic_t bytes_identical; /* saved effort with backbuffer comparison */ - atomic_t bytes_sent; /* to usb, after compression including overhead */ - atomic_t cpu_kcycles_used; /* transpired during pixel processing */ };
#define to_udl(x) container_of(x, struct udl_device, drm) diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index c1996ac73a1f..bc033779f6e4 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -68,7 +68,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, struct udl_device *udl = to_udl(dev); int i, ret; char *cmd; - cycles_t start_cycles, end_cycles; int bytes_sent = 0; int bytes_identical = 0; struct urb *urb; @@ -105,8 +104,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, goto err_drm_gem_shmem_vunmap; }
- start_cycles = get_cycles(); - urb = udl_get_urb(dev); if (!urb) goto out; @@ -120,7 +117,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, &cmd, byte_offset, dev_byte_offset, width << log_bpp, &bytes_identical, &bytes_sent)) - goto error; + goto out; }
if (cmd > (char *) urb->transfer_buffer) { @@ -134,15 +131,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } else udl_urb_completion(urb);
-error: - atomic_add(bytes_sent, &udl->bytes_sent); - atomic_add(bytes_identical, &udl->bytes_identical); - atomic_add((width * height) << log_bpp, &udl->bytes_rendered); - end_cycles = get_cycles(); - atomic_add(((unsigned int) ((end_cycles - start_cycles) - >> 10)), /* Kcycles */ - &udl->cpu_kcycles_used); - out: drm_gem_shmem_vunmap(fb->obj[0], vaddr);
A call to udl_render_hline() returns the number of identical and sent pixels. None of these values is used. Remove the parameters.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_drv.h | 3 +-- drivers/gpu/drm/udl/udl_fb.c | 6 +----- drivers/gpu/drm/udl/udl_transfer.c | 4 +--- 3 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 42426407c318..d732c9e47812 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -93,8 +93,7 @@ udl_fb_user_fb_create(struct drm_device *dev,
int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, const char *front, char **urb_buf_ptr, - u32 byte_offset, u32 device_byte_offset, u32 byte_width, - int *ident_ptr, int *sent_ptr); + u32 byte_offset, u32 device_byte_offset, u32 byte_width);
struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, size_t size); diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index bc033779f6e4..ed6d9476b25b 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -68,8 +68,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, struct udl_device *udl = to_udl(dev); int i, ret; char *cmd; - int bytes_sent = 0; - int bytes_identical = 0; struct urb *urb; int aligned_x; int log_bpp; @@ -115,8 +113,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, const int dev_byte_offset = (fb->width * i + x) << log_bpp; if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, &cmd, byte_offset, dev_byte_offset, - width << log_bpp, - &bytes_identical, &bytes_sent)) + width << log_bpp)) goto out; }
@@ -127,7 +124,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, *cmd++ = 0xAF; len = cmd - (char *) urb->transfer_buffer; ret = udl_submit_urb(dev, urb, len); - bytes_sent += len; } else udl_urb_completion(urb);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c index 1973a4c1e358..686358d1f669 100644 --- a/drivers/gpu/drm/udl/udl_transfer.c +++ b/drivers/gpu/drm/udl/udl_transfer.c @@ -212,8 +212,7 @@ static void udl_compress_hline16( int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, const char *front, char **urb_buf_ptr, u32 byte_offset, u32 device_byte_offset, - u32 byte_width, - int *ident_ptr, int *sent_ptr) + u32 byte_width) { const u8 *line_start, *line_end, *next_pixel; u32 base16 = 0 + (device_byte_offset >> log_bpp) * 2; @@ -237,7 +236,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, int len = cmd - (u8 *) urb->transfer_buffer; if (udl_submit_urb(dev, urb, len)) return 1; /* lost pixels is set */ - *sent_ptr += len; urb = udl_get_urb(dev); if (!urb) return 1; /* lost_pixels is set */
We now do the fast tests before the potentially expensive vmap operation.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_fb.c | 19 +++++++------------ drivers/gpu/drm/udl/udl_transfer.c | 1 - 2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index ed6d9476b25b..dd7ba7f63214 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } spin_unlock(&udl->active_fb_16_lock);
- vaddr = drm_gem_shmem_vmap(fb->obj[0]); - if (IS_ERR(vaddr)) { - DRM_ERROR("failed to vmap fb\n"); - return 0; - } - aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long)); width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long)); x = aligned_x; @@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, if ((width <= 0) || (x + width > fb->width) || (y + height > fb->height)) { - ret = -EINVAL; - goto err_drm_gem_shmem_vunmap; + return -EINVAL; + } + + vaddr = drm_gem_shmem_vmap(fb->obj[0]); + if (IS_ERR(vaddr)) { + DRM_ERROR("failed to vmap fb\n"); + return 0; }
urb = udl_get_urb(dev); @@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, drm_gem_shmem_vunmap(fb->obj[0], vaddr);
return 0; - -err_drm_gem_shmem_vunmap: - drm_gem_shmem_vunmap(fb->obj[0], vaddr); - return ret; }
static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c index 686358d1f669..5fae48723286 100644 --- a/drivers/gpu/drm/udl/udl_transfer.c +++ b/drivers/gpu/drm/udl/udl_transfer.c @@ -249,4 +249,3 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
return 0; } -
Hi Thomas.
I did a casual browse of the patches. Looks like some nice cleanup.
On Wed, Dec 04, 2019 at 02:24:26PM +0100, Thomas Zimmermann wrote:
We now do the fast tests before the potentially expensive vmap operation.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/udl/udl_fb.c | 19 +++++++------------ drivers/gpu/drm/udl/udl_transfer.c | 1 - 2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index ed6d9476b25b..dd7ba7f63214 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } spin_unlock(&udl->active_fb_16_lock);
- vaddr = drm_gem_shmem_vmap(fb->obj[0]);
- if (IS_ERR(vaddr)) {
DRM_ERROR("failed to vmap fb\n");return 0;- }
- aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long)); width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long)); x = aligned_x;
@@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, if ((width <= 0) || (x + width > fb->width) || (y + height > fb->height)) {
ret = -EINVAL;goto err_drm_gem_shmem_vunmap;
return -EINVAL;}
vaddr = drm_gem_shmem_vmap(fb->obj[0]);
if (IS_ERR(vaddr)) {
DRM_ERROR("failed to vmap fb\n");return 0;}
urb = udl_get_urb(dev);
@@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, drm_gem_shmem_vunmap(fb->obj[0], vaddr);
return 0;
-err_drm_gem_shmem_vunmap:
- drm_gem_shmem_vunmap(fb->obj[0], vaddr);
- return ret;
This label is reintroduced two patches later. Is this on purpose it is gone and reintroduced?
Sam
Hi Sam
Am 04.12.19 um 15:25 schrieb Sam Ravnborg:
Hi Thomas.
I did a casual browse of the patches. Looks like some nice cleanup.
On Wed, Dec 04, 2019 at 02:24:26PM +0100, Thomas Zimmermann wrote:
We now do the fast tests before the potentially expensive vmap operation.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/udl/udl_fb.c | 19 +++++++------------ drivers/gpu/drm/udl/udl_transfer.c | 1 - 2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index ed6d9476b25b..dd7ba7f63214 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } spin_unlock(&udl->active_fb_16_lock);
- vaddr = drm_gem_shmem_vmap(fb->obj[0]);
- if (IS_ERR(vaddr)) {
DRM_ERROR("failed to vmap fb\n");return 0;- }
- aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long)); width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long)); x = aligned_x;
@@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, if ((width <= 0) || (x + width > fb->width) || (y + height > fb->height)) {
ret = -EINVAL;goto err_drm_gem_shmem_vunmap;
return -EINVAL;}
vaddr = drm_gem_shmem_vmap(fb->obj[0]);
if (IS_ERR(vaddr)) {
DRM_ERROR("failed to vmap fb\n");return 0;}
urb = udl_get_urb(dev);
@@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, drm_gem_shmem_vunmap(fb->obj[0], vaddr);
return 0;
-err_drm_gem_shmem_vunmap:
- drm_gem_shmem_vunmap(fb->obj[0], vaddr);
- return ret;
This label is reintroduced two patches later. Is this on purpose it is gone and reintroduced?
This label is for rollback during error handling. The label in [6/7] is called out_drm_gem_shmem_vunmap and it's part of the regular flow.
Admittedly, it's a bit unfortunate. OTOH the alternative is to add dma-buf calls before moving around the vmap call. That seems like the worse option.
Best regards Thomas
Sam _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Computing the clip rectable in a separate helper function makes the damage-handler code more readable.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_fb.c | 48 ++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index dd7ba7f63214..cc2a09a995b8 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -20,9 +20,6 @@
#include "udl_drv.h"
-#define DL_ALIGN_UP(x, a) ALIGN(x, a) -#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a) - /** Read the red component (0..255) of a 32 bpp colour. */ #define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
@@ -61,6 +58,28 @@ static uint16_t rgb16(uint32_t col) } #endif
+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; +} + int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, int width, int height) { @@ -69,7 +88,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, int i, ret; char *cmd; struct urb *urb; - int aligned_x; + struct drm_rect clip; int log_bpp; void *vaddr;
@@ -85,15 +104,11 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } spin_unlock(&udl->active_fb_16_lock);
- aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long)); - width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long)); - x = aligned_x; - - if ((width <= 0) || - (x + width > fb->width) || - (y + height > fb->height)) { + 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; - }
vaddr = drm_gem_shmem_vmap(fb->obj[0]); if (IS_ERR(vaddr)) { @@ -106,13 +121,14 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, goto out; cmd = urb->transfer_buffer;
- for (i = y; i < y + height ; i++) { + for (i = clip.y1; i < clip.y2; i++) { const int line_offset = fb->pitches[0] * i; - const int byte_offset = line_offset + (x << log_bpp); - const int dev_byte_offset = (fb->width * i + x) << 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; if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, &cmd, byte_offset, dev_byte_offset, - width << log_bpp)) + byte_width)) goto out; }
Computing the cpp value's logarithm in a separate helper function makes the damage-handler code more readable.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_fb.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index cc2a09a995b8..482786eeea6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -58,6 +58,13 @@ static uint16_t rgb16(uint32_t col) } #endif
+static long udl_log_cpp(unsigned int cpp) +{ + if (WARN_ON(!is_power_of_2(cpp))) + return -EINVAL; + return __ffs(cpp); +} + static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y, int width, int height) { @@ -92,11 +99,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, int log_bpp; void *vaddr;
- if (WARN_ON(!is_power_of_2(fb->format->cpp[0]))) - return -EINVAL; - - log_bpp = __ffs(fb->format->cpp[0]); - spin_lock(&udl->active_fb_16_lock); if (udl->active_fb_16 != fb) { spin_unlock(&udl->active_fb_16_lock); @@ -104,6 +106,11 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } spin_unlock(&udl->active_fb_16_lock);
+ 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;
The damage-handler code now invokes dma_buf_{begin,end}_access() for imported buffers. These calls were missing from the page-flip and modesetting code paths.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_fb.c | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 482786eeea6c..7d184ff96a1f 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -92,6 +92,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, { struct drm_device *dev = fb->dev; struct udl_device *udl = to_udl(dev); + struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; int i, ret; char *cmd; struct urb *urb; @@ -117,15 +118,22 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, else if ((clip.x2 > fb->width) || (clip.y2 > fb->height)) return -EINVAL;
+ if (import_attach) { + ret = dma_buf_begin_cpu_access(import_attach->dmabuf, + DMA_FROM_DEVICE); + if (ret) + return ret; + } + vaddr = drm_gem_shmem_vmap(fb->obj[0]); if (IS_ERR(vaddr)) { DRM_ERROR("failed to vmap fb\n"); - return 0; + goto out_dma_buf_end_cpu_access; }
urb = udl_get_urb(dev); if (!urb) - goto out; + goto out_drm_gem_shmem_vunmap; cmd = urb->transfer_buffer;
for (i = clip.y1; i < clip.y2; i++) { @@ -136,7 +144,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, &cmd, byte_offset, dev_byte_offset, byte_width)) - goto out; + goto out_drm_gem_shmem_vunmap; }
if (cmd > (char *) urb->transfer_buffer) { @@ -149,10 +157,16 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } else udl_urb_completion(urb);
-out: + ret = 0; + +out_drm_gem_shmem_vunmap: drm_gem_shmem_vunmap(fb->obj[0], vaddr); +out_dma_buf_end_cpu_access: + if (import_attach) + ret = dma_buf_end_cpu_access(import_attach->dmabuf, + DMA_FROM_DEVICE);
- return 0; + return ret; }
static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, @@ -162,7 +176,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, unsigned num_clips) { struct udl_device *udl = fb->dev->dev_private; - struct dma_buf_attachment *import_attach; int i; int ret = 0;
@@ -175,15 +188,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, } spin_unlock(&udl->active_fb_16_lock);
- import_attach = fb->obj[0]->import_attach; - - if (import_attach) { - ret = dma_buf_begin_cpu_access(import_attach->dmabuf, - DMA_FROM_DEVICE); - if (ret) - goto unlock; - } - for (i = 0; i < num_clips; i++) { ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1, clips[i].x2 - clips[i].x1, @@ -192,10 +196,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, break; }
- if (import_attach) - ret = dma_buf_end_cpu_access(import_attach->dmabuf, - DMA_FROM_DEVICE); - unlock: drm_modeset_unlock_all(fb->dev);
On Wed, 4 Dec 2019 at 13:24, Thomas Zimmermann tzimmermann@suse.de wrote:
The damage-handler code now invokes dma_buf_{begin,end}_access() for imported buffers. These calls were missing from the page-flip and modesetting code paths.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/udl/udl_fb.c | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 482786eeea6c..7d184ff96a1f 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -92,6 +92,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, { struct drm_device *dev = fb->dev; struct udl_device *udl = to_udl(dev);
struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; int i, ret; char *cmd; struct urb *urb;@@ -117,15 +118,22 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, else if ((clip.x2 > fb->width) || (clip.y2 > fb->height)) return -EINVAL;
if (import_attach) {ret = dma_buf_begin_cpu_access(import_attach->dmabuf,DMA_FROM_DEVICE);if (ret)return ret;}vaddr = drm_gem_shmem_vmap(fb->obj[0]); if (IS_ERR(vaddr)) { DRM_ERROR("failed to vmap fb\n");
return 0;
goto out_dma_buf_end_cpu_access; } urb = udl_get_urb(dev); if (!urb)
goto out;
goto out_drm_gem_shmem_vunmap; cmd = urb->transfer_buffer; for (i = clip.y1; i < clip.y2; i++) {@@ -136,7 +144,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, &cmd, byte_offset, dev_byte_offset, byte_width))
goto out;
goto out_drm_gem_shmem_vunmap; } if (cmd > (char *) urb->transfer_buffer) {@@ -149,10 +157,16 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, } else udl_urb_completion(urb);
-out:
ret = 0;+out_drm_gem_shmem_vunmap: drm_gem_shmem_vunmap(fb->obj[0], vaddr); +out_dma_buf_end_cpu_access:
if (import_attach)ret = dma_buf_end_cpu_access(import_attach->dmabuf,DMA_FROM_DEVICE);
return 0;
return ret;
Since you're touching the end_cpu_access call, we might as well address the following bug. Namely: Even though we get a failure from udl_render_hline() or otherwise, the function return "OK" when end_cpu_access() is successful.
If you really want to, one can keep it separate (+ CC stable), although it's fine to squash it here with a small note in the commit message.
HTH Emil
The field lost_pixels in struct udl_device was supposed to signal an error during USB transfers of the framebuffer data. The driver would have to schedule a re-transfer at a later point. This code was never implemented. Remove lost_pixels and return regular error codes instead.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_fb.c | 7 ++++--- drivers/gpu/drm/udl/udl_main.c | 3 --- drivers/gpu/drm/udl/udl_transfer.c | 7 ++++--- 4 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index d732c9e47812..4de00bddef39 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -62,7 +62,6 @@ struct udl_device { int sku_pixel_limit;
struct urb_list urbs; - atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
char mode_buf[1024]; uint32_t mode_buf_len; diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 7d184ff96a1f..3c77e8e5e417 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -141,9 +141,10 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, 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; - if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, - &cmd, byte_offset, dev_byte_offset, - byte_width)) + ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr, + &cmd, byte_offset, dev_byte_offset, + byte_width); + if (ret) goto out_drm_gem_shmem_vunmap; }
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index a23218fc7d8e..ff3e98666e8c 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -140,7 +140,6 @@ void udl_urb_completion(struct urb *urb) urb->status == -ESHUTDOWN)) { DRM_ERROR("%s - nonzero write bulk status received: %d\n", __func__, urb->status); - atomic_set(&udl->lost_pixels, 1); } }
@@ -271,7 +270,6 @@ struct urb *udl_get_urb(struct drm_device *dev) /* Wait for an in-flight buffer to complete and get re-queued */ ret = down_timeout(&udl->urbs.limit_sem, GET_URB_TIMEOUT); if (ret) { - atomic_set(&udl->lost_pixels, 1); DRM_INFO("wait for urb interrupted: %x available: %d\n", ret, udl->urbs.available); goto error; @@ -304,7 +302,6 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len) ret = usb_submit_urb(urb, GFP_ATOMIC); if (ret) { udl_urb_completion(urb); /* because no one else will */ - atomic_set(&udl->lost_pixels, 1); DRM_ERROR("usb_submit_urb error %x\n", ret); } return ret; diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c index 5fae48723286..971927669d6b 100644 --- a/drivers/gpu/drm/udl/udl_transfer.c +++ b/drivers/gpu/drm/udl/udl_transfer.c @@ -234,11 +234,12 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
if (cmd >= cmd_end) { int len = cmd - (u8 *) urb->transfer_buffer; - if (udl_submit_urb(dev, urb, len)) - return 1; /* lost pixels is set */ + int ret = udl_submit_urb(dev, urb, len); + if (ret) + return ret; urb = udl_get_urb(dev); if (!urb) - return 1; /* lost_pixels is set */ + return -EAGAIN; *urb_ptr = urb; cmd = urb->transfer_buffer; cmd_end = &cmd[urb->transfer_buffer_length];
On Wed, Dec 04, 2019 at 02:24:23PM +0100, Thomas Zimmermann wrote:
Udl's damage-handling code requires clean up before switching the driver to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables and statistics. Patches 3 to 5 reorganizes the damage handler to be more readable. Patch 6 moves dma-buf begin/end calls into the damage handler, so it will run during page-flip andmodeset operations.
Nice cleanups.
Acked-by: Gerd Hoffmann kraxel@redhat.com
cheers, Gerd
On Wed, 4 Dec 2019 at 13:24, Thomas Zimmermann tzimmermann@suse.de wrote:
Udl's damage-handling code requires clean up before switching the driver to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables and statistics. Patches 3 to 5 reorganizes the damage handler to be more readable. Patch 6 moves dma-buf begin/end calls into the damage handler, so it will run during page-flip andmodeset operations.
The patchset has been tested by running fbdev console emulation, X11, and Weston with a DisplayLink adapter.
Thomas Zimmermann (7): drm/udl: Remove unused statistics counters drm/udl: Don't track number of identical and sent pixels per line drm/udl: Vmap framebuffer after all tests succeeded in damage handling drm/udl: Move clip-rectangle code out of udl_handle_damage() drm/udl: Move log-cpp code out of udl_damage_handler() drm/udl: Begin/end access to imported buffers in damage-handler drm/udl: Remove field lost_pixels from struct udl_device
There's a bugfix request in 6/7.
Regardless if you squash it in or choose to keep it separate, the series is: Reviewed-by: Emil Velikov emil.velikov@collabora.com
Thanks Emil
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Gerd Hoffmann -
Sam Ravnborg -
Thomas Zimmermann