Message ID | 20191204132430.16874-4-tzimmermann@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/udl: Prepare damage handler for simple-pipe KMS | expand |
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 >
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; } -
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(-)