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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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
> :)

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@xxxxxxxxxxxxxxx>

Thanks.
-Chris

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux