[PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path

[PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path

[Chris Wilson]

No more direct return -EINVAL as we have to unwind the
obj->framebuffer_references.

Fixes: 24dbf51a5517 ("drm/i915: struct_mutex is not required for allocating the framebuffer")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 10e68dfb7260..77936ddd860a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14383,7 +14383,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	if (INTEL_INFO(dev_priv)->gen < 4 &&
 	    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
 		DRM_DEBUG("tiling_mode must match fb modifier exactly on gen2/3\n");
-		return -EINVAL;
+		goto err;
 	}
 
 	stride_alignment = intel_fb_stride_alignment(dev_priv,
@@ -14428,7 +14428,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		if (INTEL_GEN(dev_priv) > 3) {
 			DRM_DEBUG("unsupported pixel format: %s\n",
 			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			return -EINVAL;
+			goto err;
 		}
 		break;
 	case DRM_FORMAT_ABGR8888:
@@ -14436,7 +14436,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		    INTEL_GEN(dev_priv) < 9) {
 			DRM_DEBUG("unsupported pixel format: %s\n",
 			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			return -EINVAL;
+			goto err;
 		}
 		break;
 	case DRM_FORMAT_XBGR8888:
@@ -14445,14 +14445,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		if (INTEL_GEN(dev_priv) < 4) {
 			DRM_DEBUG("unsupported pixel format: %s\n",
 			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			return -EINVAL;
+			goto err;
 		}
 		break;
 	case DRM_FORMAT_ABGR2101010:
 		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
 			DRM_DEBUG("unsupported pixel format: %s\n",
 			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			return -EINVAL;
+			goto err;
 		}
 		break;
 	case DRM_FORMAT_YUYV:
@@ -14462,13 +14462,13 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		if (INTEL_GEN(dev_priv) < 5) {
 			DRM_DEBUG("unsupported pixel format: %s\n",
 			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			return -EINVAL;
+			goto err;
 		}
 		break;
 	default:
 		DRM_DEBUG("unsupported pixel format: %s\n",
 		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-		return -EINVAL;
+		goto err;
 	}
 
 	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
@@ -14481,7 +14481,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 
 	ret = intel_fill_fb_info(dev_priv, &intel_fb->base);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = drm_framebuffer_init(obj->base.dev,
 				   &intel_fb->base,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications

[Chris Wilson]

Reintroduce a lock around tiling vs framebuffer creation to prevent
modification of the obj->tiling_and_stride whilst the framebuffer is
being created. Rather than use struct_mutex once again, use the
per-object lock - this will also be required in future to prevent
changing the tiling whilst submitting rendering.

Fixes: 24dbf51a5517 ("drm/i915: struct_mutex is not required for allocating the framebuffer")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_object.h   | 18 +++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c   |  9 ++++++++-
 drivers/gpu/drm/i915/intel_display.c     | 25 ++++++++++++++++---------
 4 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index ad1bc0b1a0c2..8c02c8ec2a3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -169,7 +169,7 @@ struct drm_i915_gem_object {
 	struct reservation_object *resv;
 
 	/** References from framebuffers, locks out tiling changes. */
-	atomic_t framebuffer_references;
+	unsigned int framebuffer_references;
 
 	/** Record of address bit 17 of each page at last unbind. */
 	unsigned long *bit_17;
@@ -263,6 +263,16 @@ extern void drm_gem_object_unreference(struct drm_gem_object *);
 __deprecated
 extern void drm_gem_object_unreference_unlocked(struct drm_gem_object *);
 
+static inline void i915_gem_object_lock(struct drm_i915_gem_object *obj)
+{
+	reservation_object_lock(obj->resv, NULL);
+}
+
+static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
+{
+	reservation_object_unlock(obj->resv);
+}
+
 static inline bool
 i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
 {
@@ -303,6 +313,12 @@ i915_gem_object_clear_active_reference(struct drm_i915_gem_object *obj)
 
 void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj);
 
+static inline bool
+i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
+{
+	return READ_ONCE(obj->framebuffer_references);
+}
+
 static inline unsigned int
 i915_gem_object_get_tiling(struct drm_i915_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 7e3bb48e043e..630697001b38 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -210,7 +210,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 
 			if (!(flags & I915_SHRINK_ACTIVE) &&
 			    (i915_gem_object_is_active(obj) ||
-			     atomic_read(&obj->framebuffer_references)))
+			     i915_gem_object_is_framebuffer(obj)))
 				continue;
 
 			if (!can_release_pages(obj))
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index c1d669e32f41..ad5e05f6b836 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -238,7 +238,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
 	if ((tiling | stride) == obj->tiling_and_stride)
 		return 0;
 
-	if (atomic_read(&obj->framebuffer_references))
+	if (i915_gem_object_is_framebuffer(obj))
 		return -EBUSY;
 
 	/* We need to rebind the object if its current allocation
@@ -258,6 +258,12 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
 	if (err)
 		return err;
 
+	i915_gem_object_lock(obj);
+	if (i915_gem_object_is_framebuffer(obj)) {
+		i915_gem_object_unlock(obj);
+		return -EBUSY;
+	}
+
 	/* If the memory has unknown (i.e. varying) swizzling, we pin the
 	 * pages to prevent them being swapped out and causing corruption
 	 * due to the change in swizzling.
@@ -294,6 +300,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
 	}
 
 	obj->tiling_and_stride = tiling | stride;
+	i915_gem_object_unlock(obj);
 
 	/* Force the fence to be reacquired for GTT access */
 	i915_gem_release_mmap(obj);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77936ddd860a..62a1e628e399 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14255,7 +14255,10 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 
 	drm_framebuffer_cleanup(fb);
 
-	WARN_ON(atomic_dec_return(&intel_fb->obj->framebuffer_references) < 0);
+	i915_gem_object_lock(intel_fb->obj);
+	WARN_ON(!intel_fb->obj->framebuffer_references--);
+	i915_gem_object_unlock(intel_fb->obj);
+
 	i915_gem_object_put(intel_fb->obj);
 
 	kfree(intel_fb);
@@ -14332,12 +14335,16 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 				  struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
-	unsigned int tiling = i915_gem_object_get_tiling(obj);
-	u32 pitch_limit, stride_alignment;
 	struct drm_format_name_buf format_name;
+	u32 pitch_limit, stride_alignment;
+	unsigned int tiling, stride;
 	int ret = -EINVAL;
 
-	atomic_inc(&obj->framebuffer_references);
+	i915_gem_object_lock(obj);
+	obj->framebuffer_references++;
+	tiling = i915_gem_object_get_tiling(obj);
+	stride = i915_gem_object_get_stride(obj);
+	i915_gem_object_unlock(obj);
 
 	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
 		/*
@@ -14409,11 +14416,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	 * If there's a fence, enforce that
 	 * the fb pitch and fence stride match.
 	 */
-	if (tiling != I915_TILING_NONE &&
-	    mode_cmd->pitches[0] != i915_gem_object_get_stride(obj)) {
+	if (tiling != I915_TILING_NONE && mode_cmd->pitches[0] !=  stride) {
 		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
-			  mode_cmd->pitches[0],
-			  i915_gem_object_get_stride(obj));
+			  mode_cmd->pitches[0], stride);
 		goto err;
 	}
 
@@ -14494,7 +14499,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return 0;
 
 err:
-	atomic_dec(&obj->framebuffer_references);
+	i915_gem_object_lock(obj);
+	obj->framebuffer_references--;
+	i915_gem_object_unlock(obj);
 	return ret;
 }
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications

[Chris Wilson]

On Tue, Feb 28, 2017 at 04:22:33PM +0000, Chris Wilson wrote:
> Reintroduce a lock around tiling vs framebuffer creation to prevent
> modification of the obj->tiling_and_stride whilst the framebuffer is
> being created. Rather than use struct_mutex once again, use the
> per-object lock - this will also be required in future to prevent
> changing the tiling whilst submitting rendering.

It also means that we can't create a framebuffer whilst submitting
rendering to/from the object (on this or other device). However, the
gen2/gen3 fence requirement means that we will want some serialisation
between set_tiling_ioctl and execbuffer_ioctl - and so the ww_mutex
still seems apt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path
URL   : https://patchwork.freedesktop.org/series/20409/
State : success

== Summary ==

Series 20409v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20409/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#100004

fdo#100004 https://bugs.freedesktop.org/show_bug.cgi?id=100004

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-n2820     total:278  pass:246  dwarn:1   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

5d37006b578e38562382215e8782cfced9c992ce drm-tip: 2017y-02m-28d-16h-27m-13s UTC integration manifest
fc05620 drm/i915: Fix all intel_framebuffer_init failures to take the error path

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4009/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 04:22:32PM +0000, Chris Wilson wrote:
> No more direct return -EINVAL as we have to unwind the
> obj->framebuffer_references.
> 
> Fixes: 24dbf51a5517 ("drm/i915: struct_mutex is not required for allocating the framebuffer")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10e68dfb7260..77936ddd860a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14383,7 +14383,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	if (INTEL_INFO(dev_priv)->gen < 4 &&
>  	    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
>  		DRM_DEBUG("tiling_mode must match fb modifier exactly on gen2/3\n");
> -		return -EINVAL;
> +		goto err;
>  	}
>  
>  	stride_alignment = intel_fb_stride_alignment(dev_priv,
> @@ -14428,7 +14428,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		if (INTEL_GEN(dev_priv) > 3) {
>  			DRM_DEBUG("unsupported pixel format: %s\n",
>  			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			return -EINVAL;
> +			goto err;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR8888:
> @@ -14436,7 +14436,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		    INTEL_GEN(dev_priv) < 9) {
>  			DRM_DEBUG("unsupported pixel format: %s\n",
>  			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			return -EINVAL;
> +			goto err;
>  		}
>  		break;
>  	case DRM_FORMAT_XBGR8888:
> @@ -14445,14 +14445,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		if (INTEL_GEN(dev_priv) < 4) {
>  			DRM_DEBUG("unsupported pixel format: %s\n",
>  			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			return -EINVAL;
> +			goto err;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR2101010:
>  		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>  			DRM_DEBUG("unsupported pixel format: %s\n",
>  			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			return -EINVAL;
> +			goto err;
>  		}
>  		break;
>  	case DRM_FORMAT_YUYV:
> @@ -14462,13 +14462,13 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		if (INTEL_GEN(dev_priv) < 5) {
>  			DRM_DEBUG("unsupported pixel format: %s\n",
>  			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			return -EINVAL;
> +			goto err;
>  		}
>  		break;
>  	default:
>  		DRM_DEBUG("unsupported pixel format: %s\n",
>  		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -		return -EINVAL;
> +		goto err;
>  	}
>  
>  	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
> @@ -14481,7 +14481,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  
>  	ret = intel_fill_fb_info(dev_priv, &intel_fb->base);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	ret = drm_framebuffer_init(obj->base.dev,
>  				   &intel_fb->base,
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 04:22:33PM +0000, Chris Wilson wrote:
> Reintroduce a lock around tiling vs framebuffer creation to prevent
> modification of the obj->tiling_and_stride whilst the framebuffer is
> being created. Rather than use struct_mutex once again, use the
> per-object lock - this will also be required in future to prevent
> changing the tiling whilst submitting rendering.
> 
> Fixes: 24dbf51a5517 ("drm/i915: struct_mutex is not required for allocating the framebuffer")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_object.h   | 18 +++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>  drivers/gpu/drm/i915/i915_gem_tiling.c   |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_display.c     | 25 ++++++++++++++++---------
>  4 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> index ad1bc0b1a0c2..8c02c8ec2a3b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -169,7 +169,7 @@ struct drm_i915_gem_object {
>  	struct reservation_object *resv;
>  
>  	/** References from framebuffers, locks out tiling changes. */
> -	atomic_t framebuffer_references;
> +	unsigned int framebuffer_references;
>  
>  	/** Record of address bit 17 of each page at last unbind. */
>  	unsigned long *bit_17;
> @@ -263,6 +263,16 @@ extern void drm_gem_object_unreference(struct drm_gem_object *);
>  __deprecated
>  extern void drm_gem_object_unreference_unlocked(struct drm_gem_object *);
>  
> +static inline void i915_gem_object_lock(struct drm_i915_gem_object *obj)
> +{
> +	reservation_object_lock(obj->resv, NULL);
> +}
> +
> +static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
> +{
> +	reservation_object_unlock(obj->resv);
> +}
> +
>  static inline bool
>  i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
>  {
> @@ -303,6 +313,12 @@ i915_gem_object_clear_active_reference(struct drm_i915_gem_object *obj)
>  
>  void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj);
>  
> +static inline bool
> +i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
> +{
> +	return READ_ONCE(obj->framebuffer_references);
> +}
> +
>  static inline unsigned int
>  i915_gem_object_get_tiling(struct drm_i915_gem_object *obj)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 7e3bb48e043e..630697001b38 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -210,7 +210,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  
>  			if (!(flags & I915_SHRINK_ACTIVE) &&
>  			    (i915_gem_object_is_active(obj) ||
> -			     atomic_read(&obj->framebuffer_references)))
> +			     i915_gem_object_is_framebuffer(obj)))
>  				continue;
>  
>  			if (!can_release_pages(obj))
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index c1d669e32f41..ad5e05f6b836 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -238,7 +238,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
>  	if ((tiling | stride) == obj->tiling_and_stride)
>  		return 0;
>  
> -	if (atomic_read(&obj->framebuffer_references))
> +	if (i915_gem_object_is_framebuffer(obj))
>  		return -EBUSY;
>  
>  	/* We need to rebind the object if its current allocation
> @@ -258,6 +258,12 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
>  	if (err)
>  		return err;
>  
> +	i915_gem_object_lock(obj);
> +	if (i915_gem_object_is_framebuffer(obj)) {
> +		i915_gem_object_unlock(obj);
> +		return -EBUSY;
> +	}
> +
>  	/* If the memory has unknown (i.e. varying) swizzling, we pin the
>  	 * pages to prevent them being swapped out and causing corruption
>  	 * due to the change in swizzling.
> @@ -294,6 +300,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
>  	}
>  
>  	obj->tiling_and_stride = tiling | stride;
> +	i915_gem_object_unlock(obj);
>  
>  	/* Force the fence to be reacquired for GTT access */
>  	i915_gem_release_mmap(obj);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 77936ddd860a..62a1e628e399 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14255,7 +14255,10 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  
>  	drm_framebuffer_cleanup(fb);
>  
> -	WARN_ON(atomic_dec_return(&intel_fb->obj->framebuffer_references) < 0);
> +	i915_gem_object_lock(intel_fb->obj);
> +	WARN_ON(!intel_fb->obj->framebuffer_references--);
> +	i915_gem_object_unlock(intel_fb->obj);
> +
>  	i915_gem_object_put(intel_fb->obj);
>  
>  	kfree(intel_fb);
> @@ -14332,12 +14335,16 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> -	unsigned int tiling = i915_gem_object_get_tiling(obj);
> -	u32 pitch_limit, stride_alignment;
>  	struct drm_format_name_buf format_name;
> +	u32 pitch_limit, stride_alignment;
> +	unsigned int tiling, stride;
>  	int ret = -EINVAL;
>  
> -	atomic_inc(&obj->framebuffer_references);
> +	i915_gem_object_lock(obj);
> +	obj->framebuffer_references++;
> +	tiling = i915_gem_object_get_tiling(obj);
> +	stride = i915_gem_object_get_stride(obj);
> +	i915_gem_object_unlock(obj);

I can't say I'm really up to date on the object locking stuff, but
from the display POV this looks all right to me. So good enough for me
:)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>  		/*
> @@ -14409,11 +14416,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	 * If there's a fence, enforce that
>  	 * the fb pitch and fence stride match.
>  	 */
> -	if (tiling != I915_TILING_NONE &&
> -	    mode_cmd->pitches[0] != i915_gem_object_get_stride(obj)) {
> +	if (tiling != I915_TILING_NONE && mode_cmd->pitches[0] !=  stride) {
>  		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
> -			  mode_cmd->pitches[0],
> -			  i915_gem_object_get_stride(obj));
> +			  mode_cmd->pitches[0], stride);
>  		goto err;
>  	}
>  
> @@ -14494,7 +14499,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	return 0;
>  
>  err:
> -	atomic_dec(&obj->framebuffer_references);
> +	i915_gem_object_lock(obj);
> +	obj->framebuffer_references--;
> +	i915_gem_object_unlock(obj);
>  	return ret;
>  }
>  
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications

[Chris Wilson]

On Wed, Mar 01, 2017 at 03:09:36PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 28, 2017 at 04:22:33PM +0000, Chris Wilson wrote:
> > Reintroduce a lock around tiling vs framebuffer creation to prevent
> > modification of the obj->tiling_and_stride whilst the framebuffer is
> > being created. Rather than use struct_mutex once again, use the
> > per-object lock - this will also be required in future to prevent
> > changing the tiling whilst submitting rendering.
> > 
> > Fixes: 24dbf51a5517 ("drm/i915: struct_mutex is not required for allocating the framebuffer")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_object.h   | 18 +++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
> >  drivers/gpu/drm/i915/i915_gem_tiling.c   |  9 ++++++++-
> >  drivers/gpu/drm/i915/intel_display.c     | 25 ++++++++++++++++---------
> >  4 files changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> > index ad1bc0b1a0c2..8c02c8ec2a3b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> > @@ -169,7 +169,7 @@ struct drm_i915_gem_object {
> >  	struct reservation_object *resv;
> >  
> >  	/** References from framebuffers, locks out tiling changes. */
> > -	atomic_t framebuffer_references;
> > +	unsigned int framebuffer_references;
> >  
> >  	/** Record of address bit 17 of each page at last unbind. */
> >  	unsigned long *bit_17;
> > @@ -263,6 +263,16 @@ extern void drm_gem_object_unreference(struct drm_gem_object *);
> >  __deprecated
> >  extern void drm_gem_object_unreference_unlocked(struct drm_gem_object *);
> >  
> > +static inline void i915_gem_object_lock(struct drm_i915_gem_object *obj)
> > +{
> > +	reservation_object_lock(obj->resv, NULL);
> > +}
> > +
> > +static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
> > +{
> > +	reservation_object_unlock(obj->resv);
> > +}
> > +
> >  static inline bool
> >  i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> >  {
> > @@ -303,6 +313,12 @@ i915_gem_object_clear_active_reference(struct drm_i915_gem_object *obj)
> >  
> >  void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj);
> >  
> > +static inline bool
> > +i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
> > +{
> > +	return READ_ONCE(obj->framebuffer_references);
> > +}
> > +
> >  static inline unsigned int
> >  i915_gem_object_get_tiling(struct drm_i915_gem_object *obj)
> >  {
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index 7e3bb48e043e..630697001b38 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -210,7 +210,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
> >  
> >  			if (!(flags & I915_SHRINK_ACTIVE) &&
> >  			    (i915_gem_object_is_active(obj) ||
> > -			     atomic_read(&obj->framebuffer_references)))
> > +			     i915_gem_object_is_framebuffer(obj)))
> >  				continue;
> >  
> >  			if (!can_release_pages(obj))
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index c1d669e32f41..ad5e05f6b836 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -238,7 +238,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
> >  	if ((tiling | stride) == obj->tiling_and_stride)
> >  		return 0;
> >  
> > -	if (atomic_read(&obj->framebuffer_references))
> > +	if (i915_gem_object_is_framebuffer(obj))
> >  		return -EBUSY;
> >  
> >  	/* We need to rebind the object if its current allocation
> > @@ -258,6 +258,12 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
> >  	if (err)
> >  		return err;
> >  
> > +	i915_gem_object_lock(obj);
> > +	if (i915_gem_object_is_framebuffer(obj)) {
> > +		i915_gem_object_unlock(obj);
> > +		return -EBUSY;
> > +	}
> > +
> >  	/* If the memory has unknown (i.e. varying) swizzling, we pin the
> >  	 * pages to prevent them being swapped out and causing corruption
> >  	 * due to the change in swizzling.
> > @@ -294,6 +300,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
> >  	}
> >  
> >  	obj->tiling_and_stride = tiling | stride;
> > +	i915_gem_object_unlock(obj);
> >  
> >  	/* Force the fence to be reacquired for GTT access */
> >  	i915_gem_release_mmap(obj);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 77936ddd860a..62a1e628e399 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14255,7 +14255,10 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >  
> >  	drm_framebuffer_cleanup(fb);
> >  
> > -	WARN_ON(atomic_dec_return(&intel_fb->obj->framebuffer_references) < 0);
> > +	i915_gem_object_lock(intel_fb->obj);
> > +	WARN_ON(!intel_fb->obj->framebuffer_references--);
> > +	i915_gem_object_unlock(intel_fb->obj);
> > +
> >  	i915_gem_object_put(intel_fb->obj);
> >  
> >  	kfree(intel_fb);
> > @@ -14332,12 +14335,16 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >  				  struct drm_mode_fb_cmd2 *mode_cmd)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > -	unsigned int tiling = i915_gem_object_get_tiling(obj);
> > -	u32 pitch_limit, stride_alignment;
> >  	struct drm_format_name_buf format_name;
> > +	u32 pitch_limit, stride_alignment;
> > +	unsigned int tiling, stride;
> >  	int ret = -EINVAL;
> >  
> > -	atomic_inc(&obj->framebuffer_references);
> > +	i915_gem_object_lock(obj);
> > +	obj->framebuffer_references++;
> > +	tiling = i915_gem_object_get_tiling(obj);
> > +	stride = i915_gem_object_get_stride(obj);
> > +	i915_gem_object_unlock(obj);
> 
> I can't say I'm really up to date on the object locking stuff, but
> from the display POV this looks all right to me. So good enough for me
> :)

It'll do for the moment. The whole fence and vma pinning needs a
dramatic overhaul to allow greater concurrency, and a mix of per-object,
per-GTT and per-global resource mutexes. :|
 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx