Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
On Wed, Mar 01, 2017 at 03:09:36PM +0200, Ville Syrjälä wrote: > On Tue, Feb 28, 2017 at 04:22:33PM +, 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 > > Cc: Ville Syrjälä > > --- > > 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/i
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
On Tue, Feb 28, 2017 at 04:22:33PM +, 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 > Cc: Ville Syrjälä > --- > 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)
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
On Tue, Feb 28, 2017 at 04:22:33PM +, 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
[Intel-gfx] [PATCH 2/2] drm/i915: Prevent concurrent tiling/framebuffer modifications
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 Cc: Ville Syrjälä --- 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 +