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; }
/**
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;
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
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;
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);@@ -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
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);@@ -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
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;
}
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
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
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
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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Ville Syrjälä