* [PATCH 2/3] drm: Only bump object-reference count when adding first handle
2016-01-04 10:10 [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
@ 2016-01-04 10:11 ` Chris Wilson
2016-01-04 15:29 ` Ville Syrjälä
2016-01-04 10:11 ` [PATCH 3/3] drm: Use a normal idr allocation for the obj->name Chris Wilson
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-01-04 10:11 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
We only need a single reference count for all handles (i.e. non-zero
obj->handle_count) and so can trim a few atomic operations by only
taking the reference on the first handle and dropping it after the last.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/drm_gem.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index a08176debc0e..ad955d7c99fd 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -220,6 +220,9 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
static void
drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
{
+ struct drm_device *dev = obj->dev;
+ bool final = false;
+
if (WARN_ON(obj->handle_count == 0))
return;
@@ -229,14 +232,16 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
* checked for a name
*/
- mutex_lock(&obj->dev->object_name_lock);
+ mutex_lock(&dev->object_name_lock);
if (--obj->handle_count == 0) {
drm_gem_object_handle_free(obj);
drm_gem_object_exported_dma_buf_free(obj);
+ final = true;
}
- mutex_unlock(&obj->dev->object_name_lock);
+ mutex_unlock(&dev->object_name_lock);
- drm_gem_object_unreference_unlocked(obj);
+ if (final)
+ drm_gem_object_unreference_unlocked(obj);
}
/**
@@ -329,6 +334,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
int ret;
WARN_ON(!mutex_is_locked(&dev->object_name_lock));
+ if (obj->handle_count++ == 0)
+ drm_gem_object_reference(obj);
/*
* Get the user-visible handle using idr. Preload and perform
@@ -338,10 +345,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
spin_lock(&file_priv->table_lock);
ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
- drm_gem_object_reference(obj);
- obj->handle_count++;
+
spin_unlock(&file_priv->table_lock);
idr_preload_end();
+
mutex_unlock(&dev->object_name_lock);
if (ret < 0)
goto err_unref;
--
2.6.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] drm: Only bump object-reference count when adding first handle
2016-01-04 10:11 ` [PATCH 2/3] drm: Only bump object-reference count when adding first handle Chris Wilson
@ 2016-01-04 15:29 ` Ville Syrjälä
0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2016-01-04 15:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 10:11:00AM +0000, Chris Wilson wrote:
> We only need a single reference count for all handles (i.e. non-zero
> obj->handle_count) and so can trim a few atomic operations by only
> taking the reference on the first handle and dropping it after the last.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/drm_gem.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a08176debc0e..ad955d7c99fd 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -220,6 +220,9 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
> static void
> drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
> {
> + struct drm_device *dev = obj->dev;
> + bool final = false;
> +
> if (WARN_ON(obj->handle_count == 0))
> return;
>
> @@ -229,14 +232,16 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
> * checked for a name
> */
>
> - mutex_lock(&obj->dev->object_name_lock);
> + mutex_lock(&dev->object_name_lock);
> if (--obj->handle_count == 0) {
> drm_gem_object_handle_free(obj);
> drm_gem_object_exported_dma_buf_free(obj);
> + final = true;
> }
> - mutex_unlock(&obj->dev->object_name_lock);
> + mutex_unlock(&dev->object_name_lock);
>
> - drm_gem_object_unreference_unlocked(obj);
> + if (final)
> + drm_gem_object_unreference_unlocked(obj);
> }
>
> /**
> @@ -329,6 +334,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> int ret;
>
> WARN_ON(!mutex_is_locked(&dev->object_name_lock));
> + if (obj->handle_count++ == 0)
> + drm_gem_object_reference(obj);
Hmm. Yeah, handle_count is protected by object_name_lock, so this looks
better here rather than within the spinlocked section.
Patch looks sane:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> /*
> * Get the user-visible handle using idr. Preload and perform
> @@ -338,10 +345,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> spin_lock(&file_priv->table_lock);
>
> ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
> - drm_gem_object_reference(obj);
> - obj->handle_count++;
> +
> spin_unlock(&file_priv->table_lock);
> idr_preload_end();
> +
> mutex_unlock(&dev->object_name_lock);
> if (ret < 0)
> goto err_unref;
> --
> 2.6.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm: Use a normal idr allocation for the obj->name
2016-01-04 10:10 [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
2016-01-04 10:11 ` [PATCH 2/3] drm: Only bump object-reference count when adding first handle Chris Wilson
@ 2016-01-04 10:11 ` Chris Wilson
2016-01-04 15:31 ` Ville Syrjälä
2016-01-04 10:18 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-01-04 10:11 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Unlike the handle, the name table uses a sleeping mutex rather than a
spinlock. The allocation is in a normal context, and we can use the
simpler sleeping gfp_t, rather than have to take from the atomic
reserves.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/drm_gem.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ad955d7c99fd..1b0c2c127072 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -642,7 +642,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
return -ENOENT;
mutex_lock(&dev->object_name_lock);
- idr_preload(GFP_KERNEL);
/* prevent races with concurrent gem_close. */
if (obj->handle_count == 0) {
ret = -ENOENT;
@@ -650,7 +649,7 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
}
if (!obj->name) {
- ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT);
+ ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_KERNEL);
if (ret < 0)
goto err;
@@ -661,7 +660,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
ret = 0;
err:
- idr_preload_end();
mutex_unlock(&dev->object_name_lock);
drm_gem_object_unreference_unlocked(obj);
return ret;
--
2.6.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] drm: Use a normal idr allocation for the obj->name
2016-01-04 10:11 ` [PATCH 3/3] drm: Use a normal idr allocation for the obj->name Chris Wilson
@ 2016-01-04 15:31 ` Ville Syrjälä
2016-01-05 8:00 ` Daniel Vetter
0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2016-01-04 15:31 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 10:11:01AM +0000, Chris Wilson wrote:
> Unlike the handle, the name table uses a sleeping mutex rather than a
> spinlock. The allocation is in a normal context, and we can use the
> simpler sleeping gfp_t, rather than have to take from the atomic
> reserves.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_gem.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ad955d7c99fd..1b0c2c127072 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -642,7 +642,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> return -ENOENT;
>
> mutex_lock(&dev->object_name_lock);
> - idr_preload(GFP_KERNEL);
> /* prevent races with concurrent gem_close. */
> if (obj->handle_count == 0) {
> ret = -ENOENT;
> @@ -650,7 +649,7 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> }
>
> if (!obj->name) {
> - ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT);
> + ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_KERNEL);
> if (ret < 0)
> goto err;
>
> @@ -661,7 +660,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> ret = 0;
>
> err:
> - idr_preload_end();
> mutex_unlock(&dev->object_name_lock);
> drm_gem_object_unreference_unlocked(obj);
> return ret;
> --
> 2.6.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] drm: Use a normal idr allocation for the obj->name
2016-01-04 15:31 ` Ville Syrjälä
@ 2016-01-05 8:00 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2016-01-05 8:00 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 05:31:14PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:11:01AM +0000, Chris Wilson wrote:
> > Unlike the handle, the name table uses a sleeping mutex rather than a
> > spinlock. The allocation is in a normal context, and we can use the
> > simpler sleeping gfp_t, rather than have to take from the atomic
> > reserves.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
All 3 merged to drm-misc, thanks for patches&review.
-Daniel
>
> > ---
> > drivers/gpu/drm/drm_gem.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index ad955d7c99fd..1b0c2c127072 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -642,7 +642,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> > return -ENOENT;
> >
> > mutex_lock(&dev->object_name_lock);
> > - idr_preload(GFP_KERNEL);
> > /* prevent races with concurrent gem_close. */
> > if (obj->handle_count == 0) {
> > ret = -ENOENT;
> > @@ -650,7 +649,7 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> > }
> >
> > if (!obj->name) {
> > - ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT);
> > + ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_KERNEL);
> > if (ret < 0)
> > goto err;
> >
> > @@ -661,7 +660,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> > ret = 0;
> >
> > err:
> > - idr_preload_end();
> > mutex_unlock(&dev->object_name_lock);
> > drm_gem_object_unreference_unlocked(obj);
> > return ret;
> > --
> > 2.6.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation
2016-01-04 10:10 [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
2016-01-04 10:11 ` [PATCH 2/3] drm: Only bump object-reference count when adding first handle Chris Wilson
2016-01-04 10:11 ` [PATCH 3/3] drm: Use a normal idr allocation for the obj->name Chris Wilson
@ 2016-01-04 10:18 ` Chris Wilson
2016-01-04 15:33 ` Ville Syrjälä
2016-01-04 10:49 ` ✗ failure: Fi.CI.BAT Patchwork
2016-01-04 15:24 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Ville Syrjälä
4 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-01-04 10:18 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> The current error path for failure when establishing a handle for a GEM
> object is unbalance, e.g. we call object_close() without calling first
> object_open(). Use the typical onion structure to only undo what has
> been set up prior to the error.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e10bba4468b..a08176debc0e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> spin_unlock(&file_priv->table_lock);
> idr_preload_end();
> mutex_unlock(&dev->object_name_lock);
> - if (ret < 0) {
> - drm_gem_object_handle_unreference_unlocked(obj);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_unref;
> +
> *handlep = ret;
>
> ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> - if (ret) {
> - drm_gem_handle_delete(file_priv, *handlep);
> - return ret;
> - }
> + if (ret)
> + goto err_remove;
>
> if (dev->driver->gem_open_object) {
> ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret) {
> - drm_gem_handle_delete(file_priv, *handlep);
> - return ret;
> - }
> + if (ret)
> + goto err_revoke;
> }
>
> return 0;
> +
> +err_revoke:
> + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> +err_remove:
> + spin_lock(&file_priv->table_lock);
> + idr_remove(&file_priv->object_idr, *handlep);
For bonus points, we could *handlep = 0; here.
> + spin_unlock(&file_priv->table_lock);
> +err_unref:
> + drm_gem_object_handle_unreference_unlocked(obj);
> + return ret;
> }
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation
2016-01-04 10:18 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
@ 2016-01-04 15:33 ` Ville Syrjälä
2016-01-04 15:37 ` Chris Wilson
0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2016-01-04 15:33 UTC (permalink / raw)
To: Chris Wilson, dri-devel, intel-gfx
On Mon, Jan 04, 2016 at 10:18:50AM +0000, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > The current error path for failure when establishing a handle for a GEM
> > object is unbalance, e.g. we call object_close() without calling first
> > object_open(). Use the typical onion structure to only undo what has
> > been set up prior to the error.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e10bba4468b..a08176debc0e 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > spin_unlock(&file_priv->table_lock);
> > idr_preload_end();
> > mutex_unlock(&dev->object_name_lock);
> > - if (ret < 0) {
> > - drm_gem_object_handle_unreference_unlocked(obj);
> > - return ret;
> > - }
> > + if (ret < 0)
> > + goto err_unref;
> > +
> > *handlep = ret;
> >
> > ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> > - if (ret) {
> > - drm_gem_handle_delete(file_priv, *handlep);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_remove;
> >
> > if (dev->driver->gem_open_object) {
> > ret = dev->driver->gem_open_object(obj, file_priv);
> > - if (ret) {
> > - drm_gem_handle_delete(file_priv, *handlep);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_revoke;
> > }
> >
> > return 0;
> > +
> > +err_revoke:
> > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > +err_remove:
> > + spin_lock(&file_priv->table_lock);
> > + idr_remove(&file_priv->object_idr, *handlep);
>
> For bonus points, we could *handlep = 0; here.
For these sort of things I usually prefer to not clobber any
return parameters unless the whole operation suceeds.
>
> > + spin_unlock(&file_priv->table_lock);
> > +err_unref:
> > + drm_gem_object_handle_unreference_unlocked(obj);
> > + return ret;
> > }
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation
2016-01-04 15:33 ` Ville Syrjälä
@ 2016-01-04 15:37 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2016-01-04 15:37 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 05:33:45PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:18:50AM +0000, Chris Wilson wrote:
> > On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > > The current error path for failure when establishing a handle for a GEM
> > > object is unbalance, e.g. we call object_close() without calling first
> > > object_open(). Use the typical onion structure to only undo what has
> > > been set up prior to the error.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> > > 1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 2e10bba4468b..a08176debc0e 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > > spin_unlock(&file_priv->table_lock);
> > > idr_preload_end();
> > > mutex_unlock(&dev->object_name_lock);
> > > - if (ret < 0) {
> > > - drm_gem_object_handle_unreference_unlocked(obj);
> > > - return ret;
> > > - }
> > > + if (ret < 0)
> > > + goto err_unref;
> > > +
> > > *handlep = ret;
> > >
> > > ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> > > - if (ret) {
> > > - drm_gem_handle_delete(file_priv, *handlep);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + goto err_remove;
> > >
> > > if (dev->driver->gem_open_object) {
> > > ret = dev->driver->gem_open_object(obj, file_priv);
> > > - if (ret) {
> > > - drm_gem_handle_delete(file_priv, *handlep);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + goto err_revoke;
> > > }
> > >
> > > return 0;
> > > +
> > > +err_revoke:
> > > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > > +err_remove:
> > > + spin_lock(&file_priv->table_lock);
> > > + idr_remove(&file_priv->object_idr, *handlep);
> >
> > For bonus points, we could *handlep = 0; here.
>
> For these sort of things I usually prefer to not clobber any
> return parameters unless the whole operation suceeds.
True, resetting handlep back to the INVALID_HANDLE value was just a step
in the right direction ;-)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ failure: Fi.CI.BAT
2016-01-04 10:10 [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
` (2 preceding siblings ...)
2016-01-04 10:18 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
@ 2016-01-04 10:49 ` Patchwork
2016-01-04 15:24 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Ville Syrjälä
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-01-04 10:49 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Summary ==
Built on c1e9dc2dcb577438a6350c7f1cb36ba8ad0e1dfd drm-intel-nightly: 2016y-01m-04d-09h-35m-16s UTC integration manifest
Test gem_basic:
Subgroup create-close:
pass -> DMESG-WARN (skl-i7k-2)
Test gem_cpu_reloc:
Subgroup basic:
pass -> DMESG-FAIL (skl-i7k-2)
Test gem_ctx_param_basic:
Subgroup basic:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup invalid-param-set:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup non-root-set-no-zeromap:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup root-set-no-zeromap-disabled:
pass -> DMESG-WARN (skl-i7k-2)
Test gem_mmap:
Subgroup basic:
pass -> DMESG-WARN (skl-i7k-2)
Test gem_mmap_gtt:
Subgroup basic-read:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup basic-write:
pass -> DMESG-WARN (skl-i7k-2)
Test kms_addfb_basic:
Subgroup addfb25-modifier-no-flag:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup addfb25-x-tiled-mismatch:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup bad-pitch-1024:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup bad-pitch-63:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup bad-pitch-999:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup clobberred-modifier:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup too-high:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup too-wide:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup unused-offsets:
pass -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
Subgroup basic-plain-flip:
pass -> DMESG-FAIL (skl-i7k-2)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (skl-i7k-2)
Subgroup read-crc-pipe-b:
dmesg-warn -> PASS (snb-dellxps)
Subgroup read-crc-pipe-b-frame-sequence:
pass -> DMESG-FAIL (skl-i7k-2)
Test kms_setmode:
Subgroup basic-clone-single-crtc:
pass -> DMESG-WARN (snb-dellxps)
Test prime_self_import:
Subgroup basic-with_two_bos:
pass -> DMESG-WARN (skl-i7k-2)
bdw-nuci7 total:132 pass:122 dwarn:1 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:124 dwarn:2 dfail:0 fail:0 skip:6
bsw-nuc-2 total:135 pass:114 dwarn:1 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
hsw-brixbox total:135 pass:126 dwarn:2 dfail:0 fail:0 skip:7
hsw-gt2 total:135 pass:130 dwarn:1 dfail:0 fail:0 skip:4
hsw-xps12 total:132 pass:125 dwarn:3 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:99 dwarn:1 dfail:0 fail:0 skip:35
ivb-t430s total:135 pass:127 dwarn:2 dfail:0 fail:0 skip:6
skl-i5k-2 total:135 pass:124 dwarn:3 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:101 dwarn:22 dfail:3 fail:0 skip:8
snb-dellxps total:135 pass:121 dwarn:2 dfail:0 fail:0 skip:12
snb-x220t total:135 pass:121 dwarn:2 dfail:0 fail:1 skip:11
Results at /archive/results/CI_IGT_test/Patchwork_1063/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation
2016-01-04 10:10 [PATCH 1/3] drm: Balance error path for GEM handle allocation Chris Wilson
` (3 preceding siblings ...)
2016-01-04 10:49 ` ✗ failure: Fi.CI.BAT Patchwork
@ 2016-01-04 15:24 ` Ville Syrjälä
2016-01-04 15:37 ` Ville Syrjälä
4 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2016-01-04 15:24 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> The current error path for failure when establishing a handle for a GEM
> object is unbalance, e.g. we call object_close() without calling first
> object_open(). Use the typical onion structure to only undo what has
> been set up prior to the error.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e10bba4468b..a08176debc0e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> spin_unlock(&file_priv->table_lock);
> idr_preload_end();
> mutex_unlock(&dev->object_name_lock);
> - if (ret < 0) {
> - drm_gem_object_handle_unreference_unlocked(obj);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_unref;
> +
> *handlep = ret;
>
> ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> - if (ret) {
> - drm_gem_handle_delete(file_priv, *handlep);
> - return ret;
> - }
> + if (ret)
> + goto err_remove;
>
> if (dev->driver->gem_open_object) {
> ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret) {
> - drm_gem_handle_delete(file_priv, *handlep);
> - return ret;
> - }
> + if (ret)
> + goto err_revoke;
> }
>
> return 0;
> +
> +err_revoke:
> + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> +err_remove:
> + spin_lock(&file_priv->table_lock);
> + idr_remove(&file_priv->object_idr, *handlep);
> + spin_unlock(&file_priv->table_lock);
> +err_unref:
> + drm_gem_object_handle_unreference_unlocked(obj);
> + return ret;
First I misread this as drm_gem_object_unreference_unlocked()
and though we'd leak the handle_count++, but it's
drm_gem_object_handle_unreference_unlocked() which does the
handle_count-- we need.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> }
>
> /**
> --
> 2.6.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation
2016-01-04 15:24 ` [PATCH 1/3] drm: Balance error path for GEM handle allocation Ville Syrjälä
@ 2016-01-04 15:37 ` Ville Syrjälä
0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2016-01-04 15:37 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, dri-devel
On Mon, Jan 04, 2016 at 05:24:11PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > The current error path for failure when establishing a handle for a GEM
> > object is unbalance, e.g. we call object_close() without calling first
> > object_open(). Use the typical onion structure to only undo what has
> > been set up prior to the error.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e10bba4468b..a08176debc0e 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > spin_unlock(&file_priv->table_lock);
> > idr_preload_end();
> > mutex_unlock(&dev->object_name_lock);
> > - if (ret < 0) {
> > - drm_gem_object_handle_unreference_unlocked(obj);
> > - return ret;
> > - }
> > + if (ret < 0)
> > + goto err_unref;
> > +
> > *handlep = ret;
> >
> > ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> > - if (ret) {
> > - drm_gem_handle_delete(file_priv, *handlep);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_remove;
> >
> > if (dev->driver->gem_open_object) {
> > ret = dev->driver->gem_open_object(obj, file_priv);
> > - if (ret) {
> > - drm_gem_handle_delete(file_priv, *handlep);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_revoke;
> > }
> >
> > return 0;
> > +
> > +err_revoke:
> > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > +err_remove:
> > + spin_lock(&file_priv->table_lock);
> > + idr_remove(&file_priv->object_idr, *handlep);
> > + spin_unlock(&file_priv->table_lock);
> > +err_unref:
> > + drm_gem_object_handle_unreference_unlocked(obj);
> > + return ret;
>
> First I misread this as drm_gem_object_unreference_unlocked()
> and though we'd leak the handle_count++, but it's
> drm_gem_object_handle_unreference_unlocked() which does the
> handle_count-- we need.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
BTW while looking at the code I spotted that drm_gem_handle_delete()
contains an open coded copy of drm_gem_object_release_handle(). Might
make sense to eliminate that duplication.
>
> > }
> >
> > /**
> > --
> > 2.6.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread