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

From: Qiang Yu <yuq825@gmail.com>
To: Steven Price <steven.price@arm.com>
Cc: lima@lists.freedesktop.org,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	David Airlie <airlied@linux.ie>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH v2 1/6] drm/gem: refine drm_gem_objects_lookup
Date: Sun, 29 Sep 2019 14:07:57 +0800	[thread overview]
Message-ID: <CAKGbVbvsjbvoNcDbYoO-kEtHYo8q1nL+yNG2fxsdmttw9sQfWA@mail.gmail.com> (raw)
In-Reply-To: <9a0bfc2a-4be4-3d3e-9fe3-6eecc1f8a880@arm.com>

On Fri, Sep 27, 2019 at 9:57 PM Steven Price <steven.price@arm.com> wrote:
>
> 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.
>
OK, I can improve the comments of two patches in v3.

Thanks,
Qiang

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