Re: [PATCH 4/7] drm/cma: Introduce drm_gem_cma_dumb_create_internal()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 05, 2014 at 02:25:16PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This function is similar to drm_gem_cma_dumb_create() but targetted at
> kernel internal users so that they can override the pitch and size
> requirements of the dumb buffer.
> 
> It is important to make this difference because the IOCTL says that the
> pitch and size fields are to be considered outputs and therefore should
> not be used in computations of the framebuffer size. Internal users may
> still want to use this code to avoid duplication and at the same time
> pass on additional, driver-specific restrictions on the pitch and size.
> 
> While at it, convert the R-Car DU driver, the single user that overrides
> the pitch, to use the new internal helper.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Some comments about the kerneldoc, with that address (needs the alignment
with the previous patch essentially) this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c  | 33 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 +-
>  include/drm/drm_gem_cma_helper.h      |  5 +++++
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 55306be1f8f7..c55986566f0c 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -189,7 +189,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>  
>  /**
> - * drm_gem_cma_dumb_create - create a dumb buffer object
> + * drm_gem_cma_dumb_create_internal - create a dumb buffer object
>   * @file_priv: DRM file-private structure to create the dumb buffer for
>   * @drm: DRM device
>   * @args: IOCTL data
> @@ -199,12 +199,12 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);

Kerneldoc should be update to mention what exactly this is useful for, and
how it differs from the drm_gem_cma_dumb_create().
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -			    struct drm_device *drm,
> -			    struct drm_mode_create_dumb *args)
> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> +				     struct drm_device *drm,
> +				     struct drm_mode_create_dumb *args)
>  {
> +	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  	struct drm_gem_cma_object *cma_obj;
> -	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  
>  	if (args->pitch < min_pitch)
>  		args->pitch = min_pitch;
> @@ -216,6 +216,29 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  						 &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
> +
> +/**
> + * drm_gem_cma_dumb_create - create a dumb buffer object
> + * @file_priv: DRM file-private structure to create the dumb buffer for
> + * @drm: DRM device
> + * @args: IOCTL data

Same here (and raised in the previous kerneldoc patch already), this needs
some comments on what it's useful for and what's the difference with the
_internal variant.

> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +
> +	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +	args->size = args->pitch * args->height;
> +
> +	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> +						 &args->handle);
> +	return PTR_ERR_OR_ZERO(cma_obj);
> +}
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>  
>  /**
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 6c24ad7d03ef..5329491e32c3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -128,7 +128,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  
>  	args->pitch = roundup(max(args->pitch, min_pitch), align);
>  
> -	return drm_gem_cma_dumb_create(file, dev, args);
> +	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
>  
>  static struct drm_framebuffer *
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 873d4eb7f125..acd6af8a8e67 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -30,6 +30,11 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>  
>  /* create memory region for DRM framebuffer */
> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> +				     struct drm_device *drm,
> +				     struct drm_mode_create_dumb *args);
> +
> +/* create memory region for DRM framebuffer */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  			    struct drm_device *drm,
>  			    struct drm_mode_create_dumb *args);
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel