* [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path
@ 2017-02-28 16:22 Chris Wilson
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Chris Wilson @ 2017-02-28 16:22 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
2017-02-28 16:22 [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Chris Wilson
@ 2017-02-28 16:22 ` Chris Wilson
2017-02-28 16:31 ` Chris Wilson
2017-03-01 13:09 ` Ville Syrjälä
2017-02-28 20:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Patchwork
2017-03-01 13:00 ` [PATCH 1/2] " Ville Syrjälä
2 siblings, 2 replies; 7+ messages in thread
From: Chris Wilson @ 2017-02-28 16:22 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
@ 2017-02-28 16:31 ` Chris Wilson
2017-03-01 13:09 ` Ville Syrjälä
1 sibling, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-02-28 16:31 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
2017-02-28 16:31 ` Chris Wilson
@ 2017-03-01 13:09 ` Ville Syrjälä
2017-03-01 15:47 ` Chris Wilson
1 sibling, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2017-03-01 13:09 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
2017-03-01 13:09 ` Ville Syrjälä
@ 2017-03-01 15:47 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-03-01 15:47 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path
2017-02-28 16:22 [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Chris Wilson
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
@ 2017-02-28 20:24 ` Patchwork
2017-03-01 13:00 ` [PATCH 1/2] " Ville Syrjälä
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2017-02-28 20:24 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path
2017-02-28 16:22 [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Chris Wilson
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
2017-02-28 20:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Patchwork
@ 2017-03-01 13:00 ` Ville Syrjälä
2 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2017-03-01 13:00 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-01 15:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-28 16:22 [PATCH 1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Chris Wilson
2017-02-28 16:22 ` [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications Chris Wilson
2017-02-28 16:31 ` Chris Wilson
2017-03-01 13:09 ` Ville Syrjälä
2017-03-01 15:47 ` Chris Wilson
2017-02-28 20:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix all intel_framebuffer_init failures to take the error path Patchwork
2017-03-01 13:00 ` [PATCH 1/2] " Ville Syrjälä
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.