Re: [PATCHv5 02/34] drm/gem-fb-helper: Allow drivers to allocate struct drm_framebuffer on their own

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

Hi Andrzej:
On Tue, Dec 17, 2019 at 03:49:48PM +0100, Andrzej Pietrasiewicz wrote:
> Prepare tools for drivers which need to allocate a struct drm_framebuffer
> (or a container of struct drm_framebuffer) explicitly, before calling
> helpers. In such a case we need new helpers which omit allocating the
> struct drm_framebuffer and this patch provides them. Consequently, they
> are used also inside the helpers themselves.
> 
> The interested drivers will likely need to be able to perform object
> lookups and size checks in separate invocations and this patch provides
> that as well. Helpers themselves are updated, too.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 216 ++++++++++++++-----
>  include/drm/drm_gem_framebuffer_helper.h     |  17 ++
>  2 files changed, 181 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index b9bcd310ca2d..b3494f6b66bb 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -54,6 +54,69 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
>  
> +/**
> + * drm_gem_fb_init_with_funcs() - Initialize an already allocated framebuffer
> + * @fb: Framebuffer
> + * @dev: DRM device
> + * @mode_cmd: Metadata from the userspace framebuffer creation request
> + * @obj: GEM objects to be assigned to the framebuffer
> + * @num_planes: number of planes
> + * @funcs: vtable to be used for the framebuffer object
> + *
> + * This variant of the function allows passing a custom vtable.
> + *
> + * Returns:
> + * 0 on success or a negative error code
> + */
> +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb,
> +			       struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd,
> +			       struct drm_gem_object **obj,
> +			       unsigned int num_planes,
> +			       const struct drm_framebuffer_funcs *funcs)
> +{
> +	int ret, i;
> +
> +	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
> +
> +	for (i = 0; i < num_planes; i++)
> +		fb->obj[i] = obj[i];
> +
> +	ret = drm_framebuffer_init(dev, fb, funcs);
> +	if (ret)
> +		DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs);
> +
> +static const struct drm_framebuffer_funcs drm_gem_fb_funcs = {
> +	.destroy	= drm_gem_fb_destroy,
> +	.create_handle	= drm_gem_fb_create_handle,
> +};
> +
> +/**
> + * drm_gem_fb_init() - Initialize an already allocated framebuffer
> + * @fb: Framebuffer
> + * @dev: DRM device
> + * @mode_cmd: Metadata from the userspace framebuffer creation request
> + * @obj: GEM objects to be assigned to the framebuffer
> + * @num_planes: number of planes
> + *
> + * This variant of the function uses a default vtable.
> + *
> + * Returns:
> + * 0 on success or a negative error code
> + */
> +int drm_gem_fb_init(struct drm_framebuffer *fb,
> +		    struct drm_device *dev,
> +		    const struct drm_mode_fb_cmd2 *mode_cmd,
> +		    struct drm_gem_object **obj, unsigned int num_planes)
> +{
> +	return drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, &drm_gem_fb_funcs);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_init);
> +
>  static struct drm_framebuffer *
>  drm_gem_fb_alloc(struct drm_device *dev,
>  		 const struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -61,21 +124,14 @@ drm_gem_fb_alloc(struct drm_device *dev,
>  		 const struct drm_framebuffer_funcs *funcs)
>  {
>  	struct drm_framebuffer *fb;
> -	int ret, i;
> +	int ret;
>  
>  	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>  	if (!fb)
>  		return ERR_PTR(-ENOMEM);
>  
> -	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
> -
> -	for (i = 0; i < num_planes; i++)
> -		fb->obj[i] = obj[i];
> -
> -	ret = drm_framebuffer_init(dev, fb, funcs);
> +	ret = drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, funcs);
>  	if (ret) {
> -		DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n",
> -			      ret);
>  		kfree(fb);
>  		return ERR_PTR(ret);
>  	}
> @@ -144,59 +200,29 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>  			     const struct drm_mode_fb_cmd2 *mode_cmd,
>  			     const struct drm_framebuffer_funcs *funcs)
>  {
> -	const struct drm_format_info *info;
>  	struct drm_gem_object *objs[4];
>  	struct drm_framebuffer *fb;
> -	int ret, i;
> -
> -	info = drm_get_format_info(dev, mode_cmd);
> -	if (!info)
> -		return ERR_PTR(-EINVAL);
> -
> -	for (i = 0; i < info->num_planes; i++) {
> -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> +	int ret, num_planes;
>  
> -		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> -		if (!objs[i]) {
> -			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> -			ret = -ENOENT;
> -			goto err_gem_object_put;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i]
> -			 + drm_format_info_min_pitch(info, i, width)
> -			 + mode_cmd->offsets[i];
> +	ret = drm_gem_fb_lookup(dev, file, mode_cmd, objs);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +	num_planes = ret;
>  
> -		if (objs[i]->size < min_size) {
> -			drm_gem_object_put_unlocked(objs[i]);
> -			ret = -EINVAL;
> -			goto err_gem_object_put;
> -		}
> -	}
> +	ret = drm_gem_fb_size_check(dev, mode_cmd, objs);
> +	if (ret)
> +		fb = ERR_PTR(ret);
> +	else
> +		fb = drm_gem_fb_alloc(dev, mode_cmd, objs, num_planes, funcs);
>  
> -	fb = drm_gem_fb_alloc(dev, mode_cmd, objs, i, funcs);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_put;
> -	}
> +	if (IS_ERR(fb))
> +		for (num_planes--; num_planes >= 0; num_planes--)
> +			drm_gem_object_put_unlocked(objs[num_planes]);
>  
>  	return fb;
> -
> -err_gem_object_put:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -
> -	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_funcs);
>  
> -static const struct drm_framebuffer_funcs drm_gem_fb_funcs = {
> -	.destroy	= drm_gem_fb_destroy,
> -	.create_handle	= drm_gem_fb_create_handle,
> -};
> -
>  /**
>   * drm_gem_fb_create() - Helper function for the
>   *                       &drm_mode_config_funcs.fb_create callback
> @@ -228,6 +254,92 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_create);
>  
> +/**
> + * drm_gem_fb_lookup() - Helper function for use in
> + *			 &drm_mode_config_funcs.fb_create implementations
> + * @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 can be used to look up the objects for all planes.
> + * In case an error is returned all the objects are put by the
> + * function before returning.
> + *
> + * Returns:
> + * Number of planes on success or a negative error code on failure.
> + */
> +int drm_gem_fb_lookup(struct drm_device *dev,

[nit-pick] How about name it to drm_gem_fb_objs_lookup() ?

> +		      struct drm_file *file,
> +		      const struct drm_mode_fb_cmd2 *mode_cmd,
> +		      struct drm_gem_object **objs)
> +{
> +	const struct drm_format_info *info;
> +	int ret, i;
> +
> +	info = drm_get_format_info(dev, mode_cmd);
> +	if (!info)
> +		return -EINVAL;
> +
> +	for (i = 0; i < info->num_planes; i++) {
> +		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> +		if (!objs[i]) {
> +			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> +			ret = -ENOENT;
> +			goto err_gem_object_put;
> +		}
> +	}
> +
> +	return i;
> +
> +err_gem_object_put:
> +	for (i--; i >= 0; i--)
> +		drm_gem_object_put_unlocked(objs[i]);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_lookup);
> +
> +/**
> + * drm_gem_fb_size_check() - Helper function for use in
> + *			     &drm_mode_config_funcs.fb_create implementations
> + * @dev: DRM device
> + * @mode_cmd: Metadata from the userspace framebuffer creation request
> + *
> + * This function can be used to verify buffer sizes for all planes.
> + * It is caller's responsibility to put the objects on failure.
> + *
> + * Returns:
> + * Zero on success or a negative error code on failure.
> + */
> +int drm_gem_fb_size_check(struct drm_device *dev,
> +			  const struct drm_mode_fb_cmd2 *mode_cmd,
> +			  struct drm_gem_object **objs)
> +{
> +	const struct drm_format_info *info;
> +	int i;
> +
> +	info = drm_get_format_info(dev, mode_cmd);
> +	if (!info)
> +		return -EINVAL;
> +
> +	for (i = 0; i < info->num_planes; i++) {
> +		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> +		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> +		unsigned int min_size;
> +
> +		min_size = (height - 1) * mode_cmd->pitches[i]
> +			 + drm_format_info_min_pitch(info, i, width)
> +			 + mode_cmd->offsets[i];
> +
> +		if (objs[i]->size < min_size)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_size_check);
> +
>  static const struct drm_framebuffer_funcs drm_gem_fb_funcs_dirtyfb = {
>  	.destroy	= drm_gem_fb_destroy,
>  	.create_handle	= drm_gem_fb_create_handle,
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index d9f13fd25b0a..c85d4b152e91 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -14,10 +14,27 @@ struct drm_simple_display_pipe;
>  
>  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>  					  unsigned int plane);
> +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb,
> +			       struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd,
> +			       struct drm_gem_object **obj,
> +			       unsigned int num_planes,
> +			       const struct drm_framebuffer_funcs *funcs);
> +int drm_gem_fb_init(struct drm_framebuffer *fb,
> +		    struct drm_device *dev,
> +		    const struct drm_mode_fb_cmd2 *mode_cmd,
> +		    struct drm_gem_object **obj, unsigned int num_planes);
>  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
>  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
>  			     unsigned int *handle);
>  
> +int drm_gem_fb_lookup(struct drm_device *dev,
> +		      struct drm_file *file,
> +		      const struct drm_mode_fb_cmd2 *mode_cmd,
> +		      struct drm_gem_object **objs);
> +int drm_gem_fb_size_check(struct drm_device *dev,
> +			  const struct drm_mode_fb_cmd2 *mode_cmd,
> +			  struct drm_gem_object **objs);
>  struct drm_framebuffer *
>  drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>  			     const struct drm_mode_fb_cmd2 *mode_cmd,
Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

James.

> -- 
> 2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel