Re: [PATCH 5/7] drm/udl: Convert to drm_atomic_helper_dirtyfb()

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel