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@xxxxxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
