| Message ID | 20190401074730.12241-3-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial Panfrost driver | expand |
On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: > > Similar to the single handle drm_gem_object_lookup(), > drm_gem_objects_lookup() takes an array of handles and returns an array > of GEM objects. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Sean Paul <sean@poorly.run> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++-------- > include/drm/drm_gem.h | 2 ++ > 2 files changed, 39 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 388b3742e562..5c9bff45e5e1 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, > } > EXPORT_SYMBOL(drm_gem_put_pages); > > +/** > + * drm_gem_objects_lookup - look up GEM objects from an array of handles > + * @filp: DRM file private date > + * @handle: pointer to array of userspace handle > + * @count: size of handle array > + * @objs: pointer to array of drm_gem_object pointers > + * > + * Returns: > + * > + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup > + * failure. 0 is returned on success. Bonus points for adding references between the array and normal lookup functions to guide people around. Also a comment that the buffers need to be released with drm_gem_object_put(). > + */ > +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count, > + struct drm_gem_object **objs) With a pointer to a pointer I'd expect this function to do the allocation, but it doesn't. Normal pointer is enough to pass an array. Also maybe make the handle pointer const, so it's clear that it's an input parameter. With the bikesheds addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > +{ > + int i, ret = 0; > + struct drm_gem_object *obj; > + > + spin_lock(&filp->table_lock); > + > + for (i = 0; i < count; i++) { > + /* Check if we currently have a reference on the object */ > + obj = idr_find(&filp->object_idr, handle[i]); > + if (!obj) { > + ret = -ENOENT; > + break; > + } > + drm_gem_object_get(obj); > + objs[i] = obj; > + } > + spin_unlock(&filp->table_lock); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_gem_objects_lookup); > + > /** > * drm_gem_object_lookup - look up a GEM object from its handle > * @filp: DRM file private date > @@ -678,15 +714,7 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle) > { > struct drm_gem_object *obj; > > - spin_lock(&filp->table_lock); > - > - /* Check if we currently have a reference on the object */ > - obj = idr_find(&filp->object_idr, handle); > - if (obj) > - drm_gem_object_get(obj); > - > - spin_unlock(&filp->table_lock); > - > + drm_gem_objects_lookup(filp, &handle, 1, &obj); > return obj; > } > EXPORT_SYMBOL(drm_gem_object_lookup); > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 2955aaab3dca..5404225e0194 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -381,6 +381,8 @@ 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, u32 *handle, int count, > + struct drm_gem_object **objs); > struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle); > long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle, > bool wait_all, unsigned long timeout); > -- > 2.19.1 >
Quoting Daniel Vetter (2019-04-01 14:06:48) > On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: > > +{ > > + int i, ret = 0; > > + struct drm_gem_object *obj; > > + > > + spin_lock(&filp->table_lock); > > + > > + for (i = 0; i < count; i++) { > > + /* Check if we currently have a reference on the object */ > > + obj = idr_find(&filp->object_idr, handle[i]); > > + if (!obj) { > > + ret = -ENOENT; Unwind previous drm_gem_object_get(), the caller has no idea how many were processed before the error. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Daniel Vetter (2019-04-01 14:06:48) >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: >> > +{ >> > + int i, ret = 0; >> > + struct drm_gem_object *obj; >> > + >> > + spin_lock(&filp->table_lock); >> > + >> > + for (i = 0; i < count; i++) { >> > + /* Check if we currently have a reference on the object */ >> > + obj = idr_find(&filp->object_idr, handle[i]); >> > + if (!obj) { >> > + ret = -ENOENT; > > Unwind previous drm_gem_object_get(), the caller has no idea how many > were processed before the error. I had the same thought, but the pattern we have is that you're loading into a refcounted struct that will free the BOs when you're done, anyway.
On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: > > > > Similar to the single handle drm_gem_object_lookup(), > > drm_gem_objects_lookup() takes an array of handles and returns an array > > of GEM objects. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++-------- > > include/drm/drm_gem.h | 2 ++ > > 2 files changed, 39 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 388b3742e562..5c9bff45e5e1 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, > > } > > EXPORT_SYMBOL(drm_gem_put_pages); > > > > +/** > > + * drm_gem_objects_lookup - look up GEM objects from an array of handles > > + * @filp: DRM file private date > > + * @handle: pointer to array of userspace handle > > + * @count: size of handle array > > + * @objs: pointer to array of drm_gem_object pointers > > + * > > + * Returns: > > + * > > + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup > > + * failure. 0 is returned on success. > > Bonus points for adding references between the array and normal lookup > functions to guide people around. Also a comment that the buffers need > to be released with drm_gem_object_put(). > > > + */ > > +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count, > > + struct drm_gem_object **objs) > > With a pointer to a pointer I'd expect this function to do the > allocation, but it doesn't. Normal pointer is enough to pass an array. > Also maybe make the handle pointer > const, so it's clear that it's an input parameter. I thought about doing the allocation here, but then I couldn't implement the single drm_gem_object_lookup() using this function. Maybe I can refactor in 3 functions making this one a static function. Rob
Rob Herring <robh@kernel.org> writes: > On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter <daniel@ffwll.ch> wrote: >> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: >> > >> > Similar to the single handle drm_gem_object_lookup(), >> > drm_gem_objects_lookup() takes an array of handles and returns an array >> > of GEM objects. >> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> >> > Cc: Sean Paul <sean@poorly.run> >> > Cc: David Airlie <airlied@linux.ie> >> > Cc: Daniel Vetter <daniel@ffwll.ch> >> > Signed-off-by: Rob Herring <robh@kernel.org> >> > --- >> > drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++-------- >> > include/drm/drm_gem.h | 2 ++ >> > 2 files changed, 39 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> > index 388b3742e562..5c9bff45e5e1 100644 >> > --- a/drivers/gpu/drm/drm_gem.c >> > +++ b/drivers/gpu/drm/drm_gem.c >> > @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, >> > } >> > EXPORT_SYMBOL(drm_gem_put_pages); >> > >> > +/** >> > + * drm_gem_objects_lookup - look up GEM objects from an array of handles >> > + * @filp: DRM file private date >> > + * @handle: pointer to array of userspace handle >> > + * @count: size of handle array >> > + * @objs: pointer to array of drm_gem_object pointers >> > + * >> > + * Returns: >> > + * >> > + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup >> > + * failure. 0 is returned on success. >> >> Bonus points for adding references between the array and normal lookup >> functions to guide people around. Also a comment that the buffers need >> to be released with drm_gem_object_put(). >> >> > + */ >> > +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count, >> > + struct drm_gem_object **objs) >> >> With a pointer to a pointer I'd expect this function to do the >> allocation, but it doesn't. Normal pointer is enough to pass an array. >> Also maybe make the handle pointer >> const, so it's clear that it's an input parameter. > > I thought about doing the allocation here, but then I couldn't > implement the single drm_gem_object_lookup() using this function. > Maybe I can refactor in 3 functions making this one a static function. FWIW, I was thinking for v3d that I'd probably add a helper that took in the user's pointer to handles :)
On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt <eric@anholt.net> wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Daniel Vetter (2019-04-01 14:06:48) > >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: > >> > +{ > >> > + int i, ret = 0; > >> > + struct drm_gem_object *obj; > >> > + > >> > + spin_lock(&filp->table_lock); > >> > + > >> > + for (i = 0; i < count; i++) { > >> > + /* Check if we currently have a reference on the object */ > >> > + obj = idr_find(&filp->object_idr, handle[i]); > >> > + if (!obj) { > >> > + ret = -ENOENT; > > > > Unwind previous drm_gem_object_get(), the caller has no idea how many > > were processed before the error. > > I had the same thought, but the pattern we have is that you're loading > into a refcounted struct that will free the BOs when you're done, > anyway. The real bug here is if allocation of the array fails. The BO array may be NULL when the count is not. So this V3D cleanup hunk: for (i = 0; i < exec->bo_count; i++) drm_gem_object_put_unlocked(&exec->bo[i]->base.base); kvfree(exec->bo); Needs to be wrapped with 'if (exec->bo)'. We have a similar problem with fence arrays too. Thanks to Steven for noticing this copied code in panfrost. Rob
Rob Herring <robh@kernel.org> writes: > On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt <eric@anholt.net> wrote: >> >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > Quoting Daniel Vetter (2019-04-01 14:06:48) >> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote: >> >> > +{ >> >> > + int i, ret = 0; >> >> > + struct drm_gem_object *obj; >> >> > + >> >> > + spin_lock(&filp->table_lock); >> >> > + >> >> > + for (i = 0; i < count; i++) { >> >> > + /* Check if we currently have a reference on the object */ >> >> > + obj = idr_find(&filp->object_idr, handle[i]); >> >> > + if (!obj) { >> >> > + ret = -ENOENT; >> > >> > Unwind previous drm_gem_object_get(), the caller has no idea how many >> > were processed before the error. >> >> I had the same thought, but the pattern we have is that you're loading >> into a refcounted struct that will free the BOs when you're done, >> anyway. > > The real bug here is if allocation of the array fails. The BO array > may be NULL when the count is not. So this V3D cleanup hunk: > > for (i = 0; i < exec->bo_count; i++) > drm_gem_object_put_unlocked(&exec->bo[i]->base.base); > kvfree(exec->bo); > > Needs to be wrapped with 'if (exec->bo)'. We have a similar problem > with fence arrays too. Yeah, seems legit. Not really going to write new patches when I've got month-old critical patches I can't get acked, though. :/
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 388b3742e562..5c9bff45e5e1 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, } EXPORT_SYMBOL(drm_gem_put_pages); +/** + * drm_gem_objects_lookup - look up GEM objects from an array of handles + * @filp: DRM file private date + * @handle: pointer to array of userspace handle + * @count: size of handle array + * @objs: pointer to array of drm_gem_object pointers + * + * Returns: + * + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup + * failure. 0 is returned on success. + */ +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count, + struct drm_gem_object **objs) +{ + int i, ret = 0; + struct drm_gem_object *obj; + + spin_lock(&filp->table_lock); + + for (i = 0; i < count; i++) { + /* Check if we currently have a reference on the object */ + obj = idr_find(&filp->object_idr, handle[i]); + if (!obj) { + ret = -ENOENT; + break; + } + drm_gem_object_get(obj); + objs[i] = obj; + } + spin_unlock(&filp->table_lock); + + return ret; +} +EXPORT_SYMBOL(drm_gem_objects_lookup); + /** * drm_gem_object_lookup - look up a GEM object from its handle * @filp: DRM file private date @@ -678,15 +714,7 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle) { struct drm_gem_object *obj; - spin_lock(&filp->table_lock); - - /* Check if we currently have a reference on the object */ - obj = idr_find(&filp->object_idr, handle); - if (obj) - drm_gem_object_get(obj); - - spin_unlock(&filp->table_lock); - + drm_gem_objects_lookup(filp, &handle, 1, &obj); return obj; } EXPORT_SYMBOL(drm_gem_object_lookup); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 2955aaab3dca..5404225e0194 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -381,6 +381,8 @@ 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, u32 *handle, int count, + struct drm_gem_object **objs); struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle); long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle, bool wait_all, unsigned long timeout);
Similar to the single handle drm_gem_object_lookup(), drm_gem_objects_lookup() takes an array of handles and returns an array of GEM objects. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Sean Paul <sean@poorly.run> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++-------- include/drm/drm_gem.h | 2 ++ 2 files changed, 39 insertions(+), 9 deletions(-)