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

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