Re: [PATCH v2 1/6] drm/gem: refine drm_gem_objects_lookup

[Steven Price <steven.price@arm.com>]

On 27/09/2019 14:46, Qiang Yu wrote:
> drm_gem_objects_lookup does not use user space bo handles
> directly and left the user to kernel copy work to a new
> interface drm_gem_objects_lookup_user.
> 
> This is for driver like lima which does not pass gem bo
> handles continously in an array in ioctl argument.

Nit: It would be good to mention in the commit message that this adds
drm_gem_objects_lookup_user() as it helps grepping commits. Also if
you're rewriting it - it would be good to point out that you are
_changing_ the behvaviour of drm_gem_objects_lookpu() to not use user
space handles.

> 
> v2:
> 1. add drm_gem_objects_lookup_user
> 2. remove none-zero check as all caller will check before
>    calling this function
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Qiang Yu <yuq825@gmail.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/drm_gem.c               | 57 +++++++++++++++++++------
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  6 +--
>  include/drm/drm_gem.h                   |  4 +-
>  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6854f5867d51..a5e88c3e6d25 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -679,11 +679,11 @@ static int objects_lookup(struct drm_file *filp, u32 *handle, int count,
>  /**
>   * drm_gem_objects_lookup - look up GEM objects from an array of handles
>   * @filp: DRM file private date
> - * @bo_handles: user pointer to array of userspace handle
> + * @bo_handles: array of GEM object handles
>   * @count: size of handle array
>   * @objs_out: returned pointer to array of drm_gem_object pointers
>   *
> - * Takes an array of userspace handles and returns a newly allocated array of
> + * Takes an array of GEM object handles and returns a newly allocated array of
>   * GEM objects.
>   *
>   * For a single handle lookup, use drm_gem_object_lookup().
> @@ -695,26 +695,56 @@ static int objects_lookup(struct drm_file *filp, u32 *handle, int count,
>   * failure. 0 is returned on success.
>   *
>   */
> -int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> +int drm_gem_objects_lookup(struct drm_file *filp, u32 *bo_handles,
>  			   int count, struct drm_gem_object ***objs_out)
>  {
>  	int ret;
> -	u32 *handles;
>  	struct drm_gem_object **objs;
>  
> -	if (!count)
> -		return 0;
> -
>  	objs = kvmalloc_array(count, sizeof(struct drm_gem_object *),
>  			     GFP_KERNEL | __GFP_ZERO);
>  	if (!objs)
>  		return -ENOMEM;
>  
> +	ret = objects_lookup(filp, bo_handles, count, objs);
> +	if (ret)
> +		kvfree(objs);
> +	else
> +		*objs_out = objs;
> +
> +	return ret;
> +
> +}
> +EXPORT_SYMBOL(drm_gem_objects_lookup);
> +
> +/**
> + * drm_gem_objects_lookup_user - look up GEM objects from an array of handles
> + * @filp: DRM file private date
> + * @bo_handles: user pointer to array of userspace handle
> + * @count: size of handle array
> + * @objs_out: returned pointer to array of drm_gem_object pointers
> + *
> + * Takes an array of userspace handles and returns a newly allocated array of
> + * GEM objects.
> + *
> + * For a single handle lookup, use drm_gem_object_lookup().
> + *
> + * Returns:
> + *
> + * @objs filled in with GEM object pointers. Returned GEM objects need to be
> + * released with drm_gem_object_put(). -ENOENT is returned on a lookup
> + * failure. 0 is returned on success.
> + *
> + */
> +int drm_gem_objects_lookup_user(struct drm_file *filp, void __user *bo_handles,
> +				int count, struct drm_gem_object ***objs_out)
> +{
> +	int ret;
> +	u32 *handles;
> +
>  	handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
> -	if (!handles) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!handles)
> +		return -ENOMEM;
>  
>  	if (copy_from_user(handles, bo_handles, count * sizeof(u32))) {
>  		ret = -EFAULT;
> @@ -722,15 +752,14 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
>  		goto out;
>  	}
>  
> -	ret = objects_lookup(filp, handles, count, objs);
> -	*objs_out = objs;
> +	ret = drm_gem_objects_lookup(filp, handles, count, objs_out);
>  
>  out:
>  	kvfree(handles);
>  	return ret;
>  
>  }
> -EXPORT_SYMBOL(drm_gem_objects_lookup);
> +EXPORT_SYMBOL(drm_gem_objects_lookup_user);
>  
>  /**
>   * drm_gem_object_lookup - look up a GEM object from its handle
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index d74442d71048..486ca51d5662 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -130,9 +130,9 @@ panfrost_lookup_bos(struct drm_device *dev,
>  	if (!job->implicit_fences)
>  		return -ENOMEM;
>  
> -	return drm_gem_objects_lookup(file_priv,
> -				      (void __user *)(uintptr_t)args->bo_handles,
> -				      job->bo_count, &job->bos);
> +	return drm_gem_objects_lookup_user(file_priv,
> +					   (void __user *)(uintptr_t)args->bo_handles,
> +					   job->bo_count, &job->bos);
>  }
>  
>  /**
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 6aaba14f5972..354fc8d240e4 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -387,8 +387,10 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj);
>  void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>  		bool dirty, bool accessed);
>  
> -int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> +int drm_gem_objects_lookup(struct drm_file *filp, u32 *bo_handles,
>  			   int count, struct drm_gem_object ***objs_out);
> +int drm_gem_objects_lookup_user(struct drm_file *filp, void __user *bo_handles,
> +				int count, struct drm_gem_object ***objs_out);
>  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
>  long drm_gem_dma_resv_wait(struct drm_file *filep, u32 handle,
>  				    bool wait_all, unsigned long timeout);
> 

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