Re: [PATCH 6/7] drm/udl: Begin/end access to imported buffers in damage-handler

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