Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
Rob Herring writes:
> On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt wrote:
>>
>> Chris Wilson writes:
>>
>> > Quoting Daniel Vetter (2019-04-01 14:06:48)
>> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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. :/
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt wrote:
>
> Chris Wilson writes:
>
> > Quoting Daniel Vetter (2019-04-01 14:06:48)
> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
Rob Herring writes: > On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter wrote: >> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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 >> > Cc: Maxime Ripard >> > Cc: Sean Paul >> > Cc: David Airlie >> > Cc: Daniel Vetter >> > Signed-off-by: Rob Herring >> > --- >> > 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 :) signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter wrote: > > On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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 > > Cc: Maxime Ripard > > Cc: Sean Paul > > Cc: David Airlie > > Cc: Daniel Vetter > > Signed-off-by: Rob Herring > > --- > > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
Chris Wilson writes:
> Quoting Daniel Vetter (2019-04-01 14:06:48)
>> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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.
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
Quoting Daniel Vetter (2019-04-01 14:06:48)
> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
On Mon, Apr 1, 2019 at 9:47 AM Rob Herring 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
> Cc: Maxime Ripard
> Cc: Sean Paul
> Cc: David Airlie
> Cc: Daniel Vetter
> Signed-off-by: Rob Herring
> ---
> 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
> +{
> + 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
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
Cc: Maxime Ripard
Cc: Sean Paul
Cc: David Airlie
Cc: Daniel Vetter
Signed-off-by: Rob Herring
---
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.
+ */
+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);
--
2.19.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
