| Message ID | 20191126134707.22385-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/udl: Convert to simple-pipe helpers and clean up | expand |
On Tue, Nov 26, 2019 at 02:47:05PM +0100, Thomas Zimmermann wrote: > The infrastruture for atomic modesetting allows us to use the generic > code for dirty-FB and damage handling. Switch over udl and remove the > driver's implementation. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/udl/udl_drv.h | 5 --- > drivers/gpu/drm/udl/udl_fb.c | 67 ------------------------------- > drivers/gpu/drm/udl/udl_modeset.c | 11 +++-- > 3 files changed, 8 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > index 77b57d6abd23..c6fd5c08f5fc 100644 > --- a/drivers/gpu/drm/udl/udl_drv.h > +++ b/drivers/gpu/drm/udl/udl_drv.h > @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb); > int udl_init(struct udl_device *udl); > void udl_fini(struct drm_device *dev); > > -struct drm_framebuffer * > -udl_fb_user_fb_create(struct drm_device *dev, > - struct drm_file *file, > - const struct drm_mode_fb_cmd2 *mode_cmd); > - > 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, > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index c1996ac73a1f..ed01ebaaf468 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -9,14 +9,9 @@ > */ > > #include <linux/moduleparam.h> > -#include <linux/dma-buf.h> > > -#include <drm/drm_crtc_helper.h> > -#include <drm/drm_drv.h> > #include <drm/drm_fourcc.h> > -#include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_gem_shmem_helper.h> > -#include <drm/drm_modeset_helper.h> > > #include "udl_drv.h" > > @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > drm_gem_shmem_vunmap(fb->obj[0], vaddr); > return ret; > } > - > -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > - struct drm_file *file, > - unsigned flags, unsigned color, > - struct drm_clip_rect *clips, > - unsigned num_clips) > -{ > - struct udl_device *udl = fb->dev->dev_private; > - struct dma_buf_attachment *import_attach; > - int i; > - int ret = 0; > - > - drm_modeset_lock_all(fb->dev); > - > - spin_lock(&udl->active_fb_16_lock); > - if (udl->active_fb_16 != fb) { > - spin_unlock(&udl->active_fb_16_lock); > - goto unlock; > - } > - 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, > - clips[i].y2 - clips[i].y1); > - if (ret) > - break; > - } > - > - if (import_attach) > - ret = dma_buf_end_cpu_access(import_attach->dmabuf, > - DMA_FROM_DEVICE); > - > - unlock: > - drm_modeset_unlock_all(fb->dev); > - > - return ret; > -} > - > -static const struct drm_framebuffer_funcs udlfb_funcs = { > - .destroy = drm_gem_fb_destroy, > - .create_handle = drm_gem_fb_create_handle, > - .dirty = udl_user_framebuffer_dirty, > -}; > - > -struct drm_framebuffer * > -udl_fb_user_fb_create(struct drm_device *dev, > - struct drm_file *file, > - const struct drm_mode_fb_cmd2 *mode_cmd) > -{ > - return drm_gem_fb_create_with_funcs(dev, file, mode_cmd, > - &udlfb_funcs); > -} > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > index 1b5a46a91cb4..aade61ad097b 100644 > --- a/drivers/gpu/drm/udl/udl_modeset.c > +++ b/drivers/gpu/drm/udl/udl_modeset.c > @@ -11,6 +11,7 @@ > > #include <drm/drm_atomic_helper.h> > #include <drm/drm_crtc_helper.h> > +#include <drm/drm_damage_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_modeset_helper_vtables.h> > #include <drm/drm_vblank.h> > @@ -364,7 +365,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, > { > struct drm_device *dev = pipe->crtc.dev; > struct udl_device *udl = dev->dev_private; > - struct drm_framebuffer *fb = pipe->plane.state->fb; > + struct drm_plane_state *state = pipe->plane.state; > + struct drm_framebuffer *fb = state->fb; > + struct drm_rect rect; > > spin_lock(&udl->active_fb_16_lock); > udl->active_fb_16 = fb; > @@ -373,7 +376,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, > if (!fb) > return; > > - udl_handle_damage(fb, 0, 0, fb->width, fb->height); > + if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect)) > + udl_handle_damage(fb, rect.x1, rect.y1, rect.x2 - rect.x1, > + rect.y2 - rect.y1); Please mention in the commit message that you've put the optimized damage upload into the pipe_update function here. With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Aside, would be neat to roll out the damage property for udl. But I'm not sure whether it's been wired to any generic kms userspace yet (and which) ... Worst case could just test it with the igts we have. Cheers, Daniel > } > > static const > @@ -391,7 +396,7 @@ struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = { > */ > > static const struct drm_mode_config_funcs udl_mode_funcs = { > - .fb_create = udl_fb_user_fb_create, > + .fb_create = drm_gem_fb_create_with_dirty, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = drm_atomic_helper_commit, > }; > -- > 2.23.0 >
Hi Thomas, On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > The infrastruture for atomic modesetting allows us to use the generic > code for dirty-FB and damage handling. Switch over udl and remove the > driver's implementation. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/udl/udl_drv.h | 5 --- > drivers/gpu/drm/udl/udl_fb.c | 67 ------------------------------- > drivers/gpu/drm/udl/udl_modeset.c | 11 +++-- > 3 files changed, 8 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > index 77b57d6abd23..c6fd5c08f5fc 100644 > --- a/drivers/gpu/drm/udl/udl_drv.h > +++ b/drivers/gpu/drm/udl/udl_drv.h > @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb); > int udl_init(struct udl_device *udl); > void udl_fini(struct drm_device *dev); > > -struct drm_framebuffer * > -udl_fb_user_fb_create(struct drm_device *dev, > - struct drm_file *file, > - const struct drm_mode_fb_cmd2 *mode_cmd); > - > 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, > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index c1996ac73a1f..ed01ebaaf468 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -9,14 +9,9 @@ > */ > > #include <linux/moduleparam.h> > -#include <linux/dma-buf.h> > > -#include <drm/drm_crtc_helper.h> > -#include <drm/drm_drv.h> > #include <drm/drm_fourcc.h> > -#include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_gem_shmem_helper.h> > -#include <drm/drm_modeset_helper.h> > > #include "udl_drv.h" > > @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > drm_gem_shmem_vunmap(fb->obj[0], vaddr); > return ret; > } > - > -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > - struct drm_file *file, > - unsigned flags, unsigned color, > - struct drm_clip_rect *clips, > - unsigned num_clips) > -{ > - struct udl_device *udl = fb->dev->dev_private; > - struct dma_buf_attachment *import_attach; > - int i; > - int ret = 0; > - > - drm_modeset_lock_all(fb->dev); > - > - spin_lock(&udl->active_fb_16_lock); > - if (udl->active_fb_16 != fb) { > - spin_unlock(&udl->active_fb_16_lock); > - goto unlock; > - } > - 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, > - clips[i].y2 - clips[i].y1); > - if (ret) > - break; > - } > - > - if (import_attach) > - ret = dma_buf_end_cpu_access(import_attach->dmabuf, > - DMA_FROM_DEVICE); > - I cannot quite see how the existing atomic helper handles dma buf imports. There are no dma_buf_begin_cpu_access/dma_buf_end_cpu_access calls around. From a quick look, imports don't seem to be prohibited in the helper. Am I missing something? Alternatively can you add a brief note in the commit message. Thanks -Emil
On Fri, Nov 29, 2019 at 06:04:02PM +0000, Emil Velikov wrote: > Hi Thomas, > > On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > The infrastruture for atomic modesetting allows us to use the generic > > code for dirty-FB and damage handling. Switch over udl and remove the > > driver's implementation. > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > --- > > drivers/gpu/drm/udl/udl_drv.h | 5 --- > > drivers/gpu/drm/udl/udl_fb.c | 67 ------------------------------- > > drivers/gpu/drm/udl/udl_modeset.c | 11 +++-- > > 3 files changed, 8 insertions(+), 75 deletions(-) > > > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > > index 77b57d6abd23..c6fd5c08f5fc 100644 > > --- a/drivers/gpu/drm/udl/udl_drv.h > > +++ b/drivers/gpu/drm/udl/udl_drv.h > > @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb); > > int udl_init(struct udl_device *udl); > > void udl_fini(struct drm_device *dev); > > > > -struct drm_framebuffer * > > -udl_fb_user_fb_create(struct drm_device *dev, > > - struct drm_file *file, > > - const struct drm_mode_fb_cmd2 *mode_cmd); > > - > > 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, > > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > > index c1996ac73a1f..ed01ebaaf468 100644 > > --- a/drivers/gpu/drm/udl/udl_fb.c > > +++ b/drivers/gpu/drm/udl/udl_fb.c > > @@ -9,14 +9,9 @@ > > */ > > > > #include <linux/moduleparam.h> > > -#include <linux/dma-buf.h> > > > > -#include <drm/drm_crtc_helper.h> > > -#include <drm/drm_drv.h> > > #include <drm/drm_fourcc.h> > > -#include <drm/drm_gem_framebuffer_helper.h> > > #include <drm/drm_gem_shmem_helper.h> > > -#include <drm/drm_modeset_helper.h> > > > > #include "udl_drv.h" > > > > @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > > drm_gem_shmem_vunmap(fb->obj[0], vaddr); > > return ret; > > } > > - > > -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, > > - struct drm_file *file, > > - unsigned flags, unsigned color, > > - struct drm_clip_rect *clips, > > - unsigned num_clips) > > -{ > > - struct udl_device *udl = fb->dev->dev_private; > > - struct dma_buf_attachment *import_attach; > > - int i; > > - int ret = 0; > > - > > - drm_modeset_lock_all(fb->dev); > > - > > - spin_lock(&udl->active_fb_16_lock); > > - if (udl->active_fb_16 != fb) { > > - spin_unlock(&udl->active_fb_16_lock); > > - goto unlock; > > - } > > - 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, > > - clips[i].y2 - clips[i].y1); > > - if (ret) > > - break; > > - } > > - > > - if (import_attach) > > - ret = dma_buf_end_cpu_access(import_attach->dmabuf, > > - DMA_FROM_DEVICE); > > - > I cannot quite see how the existing atomic helper handles dma buf imports. > There are no dma_buf_begin_cpu_access/dma_buf_end_cpu_access calls around. > > From a quick look, imports don't seem to be prohibited in the helper. > > Am I missing something? Alternatively can you add a brief note in the > commit message. Great catch, that's indeeded missing (and it was also missing from the existing code except for the dirty path here). I think a prep patch to move the begin/end_cpu_access into udl_handle_damage, before this entire series here, would be best. -Daniel > > Thanks > -Emil
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 77b57d6abd23..c6fd5c08f5fc 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb); int udl_init(struct udl_device *udl); void udl_fini(struct drm_device *dev); -struct drm_framebuffer * -udl_fb_user_fb_create(struct drm_device *dev, - struct drm_file *file, - const struct drm_mode_fb_cmd2 *mode_cmd); - 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, diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index c1996ac73a1f..ed01ebaaf468 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -9,14 +9,9 @@ */ #include <linux/moduleparam.h> -#include <linux/dma-buf.h> -#include <drm/drm_crtc_helper.h> -#include <drm/drm_drv.h> #include <drm/drm_fourcc.h> -#include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> -#include <drm/drm_modeset_helper.h> #include "udl_drv.h" @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, drm_gem_shmem_vunmap(fb->obj[0], vaddr); return ret; } - -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, - struct drm_file *file, - unsigned flags, unsigned color, - struct drm_clip_rect *clips, - unsigned num_clips) -{ - struct udl_device *udl = fb->dev->dev_private; - struct dma_buf_attachment *import_attach; - int i; - int ret = 0; - - drm_modeset_lock_all(fb->dev); - - spin_lock(&udl->active_fb_16_lock); - if (udl->active_fb_16 != fb) { - spin_unlock(&udl->active_fb_16_lock); - goto unlock; - } - 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, - clips[i].y2 - clips[i].y1); - if (ret) - break; - } - - if (import_attach) - ret = dma_buf_end_cpu_access(import_attach->dmabuf, - DMA_FROM_DEVICE); - - unlock: - drm_modeset_unlock_all(fb->dev); - - return ret; -} - -static const struct drm_framebuffer_funcs udlfb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, - .dirty = udl_user_framebuffer_dirty, -}; - -struct drm_framebuffer * -udl_fb_user_fb_create(struct drm_device *dev, - struct drm_file *file, - const struct drm_mode_fb_cmd2 *mode_cmd) -{ - return drm_gem_fb_create_with_funcs(dev, file, mode_cmd, - &udlfb_funcs); -} diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index 1b5a46a91cb4..aade61ad097b 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -11,6 +11,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_damage_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_vblank.h> @@ -364,7 +365,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, { struct drm_device *dev = pipe->crtc.dev; struct udl_device *udl = dev->dev_private; - struct drm_framebuffer *fb = pipe->plane.state->fb; + struct drm_plane_state *state = pipe->plane.state; + struct drm_framebuffer *fb = state->fb; + struct drm_rect rect; spin_lock(&udl->active_fb_16_lock); udl->active_fb_16 = fb; @@ -373,7 +376,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, if (!fb) return; - udl_handle_damage(fb, 0, 0, fb->width, fb->height); + if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect)) + udl_handle_damage(fb, rect.x1, rect.y1, rect.x2 - rect.x1, + rect.y2 - rect.y1); } static const @@ -391,7 +396,7 @@ struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = { */ static const struct drm_mode_config_funcs udl_mode_funcs = { - .fb_create = udl_fb_user_fb_create, + .fb_create = drm_gem_fb_create_with_dirty, .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, };
The infrastruture for atomic modesetting allows us to use the generic code for dirty-FB and damage handling. Switch over udl and remove the driver's implementation. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/udl/udl_drv.h | 5 --- drivers/gpu/drm/udl/udl_fb.c | 67 ------------------------------- drivers/gpu/drm/udl/udl_modeset.c | 11 +++-- 3 files changed, 8 insertions(+), 75 deletions(-)