Re: [PATCH 1/4] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 09, 2019 at 06:49:53PM +0100, Noralf Trønnes wrote:
> This adds a .fb_create helper that sets the .dirty callback to
> drm_atomic_helper_dirtyfb().
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 47 +++++++++++++++++---
>  include/drm/drm_gem_framebuffer_helper.h     |  3 ++
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index acb466d25afc..4edd0261e507 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -17,6 +17,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_uapi.h>
> +#include <drm/drm_damage_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
> @@ -136,10 +137,9 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle);
>   * @mode_cmd: Metadata from the userspace framebuffer creation request
>   * @funcs: vtable to be used for the new framebuffer object
>   *
> - * This can be used to set &drm_framebuffer_funcs for drivers that need the
> - * &drm_framebuffer_funcs.dirty callback. Use drm_gem_fb_create() if you don't
> - * need to change &drm_framebuffer_funcs.
> - * The function does buffer size validation.
> + * This function can be used to set &drm_framebuffer_funcs for drivers that need
> + * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to
> + * change &drm_framebuffer_funcs. The function does buffer size validation.
>   *
>   * Returns:
>   * Pointer to a &drm_framebuffer on success or an error pointer on failure.
> @@ -215,8 +215,8 @@ static const struct drm_framebuffer_funcs drm_gem_fb_funcs = {
>   *
>   * If your hardware has special alignment or pitch requirements these should be
>   * checked before calling this function. The function does buffer size
> - * validation. Use drm_gem_fb_create_with_funcs() if you need to set
> - * &drm_framebuffer_funcs.dirty.
> + * validation. Use drm_gem_fb_create_with_dirty() if you need framebuffer
> + * flushing.
>   *
>   * Drivers can use this as their &drm_mode_config_funcs.fb_create callback.
>   * The ADDFB2 IOCTL calls into this callback.
> @@ -233,6 +233,41 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_create);
>  
> +static const struct drm_framebuffer_funcs drm_gem_fb_funcs_dirtyfb = {
> +	.destroy	= drm_gem_fb_destroy,
> +	.create_handle	= drm_gem_fb_create_handle,
> +	.dirty		= drm_atomic_helper_dirtyfb,
> +};
> +
> +/**
> + * drm_gem_fb_create_with_dirty() - Helper function for the
> + *                       &drm_mode_config_funcs.fb_create callback
> + * @dev: DRM device
> + * @file: DRM file that holds the GEM handle(s) backing the framebuffer
> + * @mode_cmd: Metadata from the userspace framebuffer creation request
> + *
> + * This function creates a new framebuffer object described by
> + * &drm_mode_fb_cmd2. This description includes handles for the buffer(s)
> + * backing the framebuffer. drm_atomic_helper_dirtyfb() is used for the dirty
> + * callback giving framebuffer flushing through the atomic machinery. Use
> + * drm_gem_fb_create() if you don't need the dirty callback.
> + * The function does buffer size validation.
> + *
> + * Drivers can use this as their &drm_mode_config_funcs.fb_create callback.
> + * The ADDFB2 IOCTL calls into this callback.

I think would be good to add:

"Drivers should also call drm_plane_enable_fb_damage_clips() on all planes
to enable userspace to use damage clips also with the ATOMIC IOCTL."

It's a bit annoying that drivers have to make that separate call too (so
some risk for accidents), but I didn't see a good way to avoid that.

With the documentation nit, on the entire series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + * Returns:
> + * Pointer to a &drm_framebuffer on success or an error pointer on failure.
> + */
> +struct drm_framebuffer *
> +drm_gem_fb_create_with_dirty(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,
> +					    &drm_gem_fb_funcs_dirtyfb);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
> +
>  /**
>   * drm_gem_fb_prepare_fb() - Prepare a GEM backed framebuffer
>   * @plane: Plane
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index a38de7eb55b4..7f307e834eef 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -25,6 +25,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>  struct drm_framebuffer *
>  drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>  		  const struct drm_mode_fb_cmd2 *mode_cmd);
> +struct drm_framebuffer *
> +drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
> +			     const struct drm_mode_fb_cmd2 *mode_cmd);
>  
>  int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>  			  struct drm_plane_state *state);
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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