[PATCH 1/3] drm: Balance error path for GEM handle allocation

[PATCH 1/3] drm: Balance error path for GEM handle allocation

[Chris Wilson]

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;
 }
 
 /**
-- 
2.6.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Only bump object-reference count when adding first handle

[Chris Wilson]

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

[PATCH 3/3] drm: Use a normal idr allocation for the obj->name

[Chris Wilson]

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

Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation

[Chris Wilson]

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

✗ failure: Fi.CI.BAT

[Patchwork]

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

Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation

[Ville Syrjälä]

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

Re: [PATCH 2/3] drm: Only bump object-reference count when adding first handle

[Ville Syrjälä]

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

Re: [PATCH 3/3] drm: Use a normal idr allocation for the obj->name

[Ville Syrjälä]

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

Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation

[Ville Syrjälä]

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

Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation

[Ville Syrjälä]

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

Re: [PATCH 1/3] drm: Balance error path for GEM handle allocation

[Chris Wilson]

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

Re: [PATCH 3/3] drm: Use a normal idr allocation for the obj->name

[Daniel Vetter]

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