Re: [PATCH 17/22] drm/i915: Use atomics to manipulate obj->frontbuffer_bits

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 27, 2016 at 12:14:55PM +0100, Chris Wilson wrote:
> The individual bits inside obj->frontbuffer_bits are protected by each
> plane->mutex, but the whole bitfield may be accessed by multiple KMS
> operations simultaneously and so the RMW need to be under atomics.
> However, for updating the single field we do not need to mandate that it
> be under the struct_mutex, one more step towards its removal as the de
> facto BKL.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  6 ++++--
>  drivers/gpu/drm/i915/i915_drv.h          |  4 +---
>  drivers/gpu/drm/i915/i915_gem.c          | 18 +++++++++++-------
>  drivers/gpu/drm/i915/intel_display.c     | 18 ++++++------------
>  drivers/gpu/drm/i915/intel_drv.h         | 20 ++++++++++++++------
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 23 +++++++++--------------
>  6 files changed, 45 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fcfa9ca6b50a..10a346237795 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -138,6 +138,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>  	struct intel_engine_cs *engine;
>  	struct i915_vma *vma;
> +	unsigned int frontbuffer_bits;
>  	int pin_count = 0;
>  	enum intel_engine_id id;
>  
> @@ -204,8 +205,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  	if (engine)
>  		seq_printf(m, " (%s)", engine->name);
>  
> -	if (obj->frontbuffer_bits)
> -		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
> +	frontbuffer_bits = atomic_read(&obj->frontbuffer_bits);
> +	if (frontbuffer_bits)
> +		seq_printf(m, " (frontbuffer: 0x%03x)", frontbuffer_bits);
>  }
>  
>  static int i915_gem_object_list_info(struct seq_file *m, void *data)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a24d31e3e014..b6b9a1f78238 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2127,8 +2127,6 @@ struct drm_i915_gem_object_ops {
>   */
>  #define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
>  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> -#define INTEL_FRONTBUFFER_BITS \
> -	(INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES)
>  #define INTEL_FRONTBUFFER_PRIMARY(pipe) \
>  	(1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  #define INTEL_FRONTBUFFER_CURSOR(pipe) \
> @@ -2216,7 +2214,7 @@ struct drm_i915_gem_object {
>  	unsigned int cache_level:3;
>  	unsigned int cache_dirty:1;
>  
> -	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> +	atomic_t frontbuffer_bits;
>  
>  	unsigned int has_wc_mmap;
>  	/** Count of VMA actually bound by this object */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7db0808f6961..bc5bc5ccdde0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4031,7 +4031,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  	if (obj->stolen)
>  		i915_gem_object_unpin_pages(obj);
>  
> -	WARN_ON(obj->frontbuffer_bits);
> +	WARN_ON(atomic_read(&obj->frontbuffer_bits));
>  
>  	if (obj->pages && obj->madv == I915_MADV_WILLNEED &&
>  	    dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES &&
> @@ -4549,16 +4549,20 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
>  		       struct drm_i915_gem_object *new,
>  		       unsigned frontbuffer_bits)
>  {
> +	/* Control of individual bits within the bitfield are guarded by
> +	 * the owning plane->mutex, i.e. we can never see concurrent
> +	 * manipulation of individual bits. But since the bitfield as a whole
> +	 * is updated using RMW, we need to use atomics in order to update
> +	 * the bits.
> +	 */
>  	if (old) {
> -		WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
> -		WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits));
> -		old->frontbuffer_bits &= ~frontbuffer_bits;
> +		WARN_ON(!(atomic_read(&old->frontbuffer_bits) & frontbuffer_bits));
> +		atomic_andnot(frontbuffer_bits, &old->frontbuffer_bits);
>  	}
>  
>  	if (new) {
> -		WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex));
> -		WARN_ON(new->frontbuffer_bits & frontbuffer_bits);
> -		new->frontbuffer_bits |= frontbuffer_bits;
> +		WARN_ON(atomic_read(&new->frontbuffer_bits) & frontbuffer_bits);
> +		atomic_or(frontbuffer_bits, &new->frontbuffer_bits);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed2069c56036..1c70f68328b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2600,7 +2600,8 @@ valid_fb:
>  	primary->fb = primary->state->fb = fb;
>  	primary->crtc = primary->state->crtc = &intel_crtc->base;
>  	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
> -	obj->frontbuffer_bits |= to_intel_plane(primary)->frontbuffer_bit;
> +	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
> +		  &obj->frontbuffer_bits);
>  }
>  
>  static void i9xx_update_primary_plane(struct drm_plane *primary,
> @@ -13807,19 +13808,12 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>  {
>  	struct drm_plane_state *old_plane_state;
>  	struct drm_plane *plane;
> -	struct drm_i915_gem_object *obj, *old_obj;
> -	struct intel_plane *intel_plane;
>  	int i;
>  
> -	mutex_lock(&state->dev->struct_mutex);
> -	for_each_plane_in_state(state, plane, old_plane_state, i) {
> -		obj = intel_fb_obj(plane->state->fb);
> -		old_obj = intel_fb_obj(old_plane_state->fb);
> -		intel_plane = to_intel_plane(plane);
> -
> -		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> -	}
> -	mutex_unlock(&state->dev->struct_mutex);
> +	for_each_plane_in_state(state, plane, old_plane_state, i)
> +		i915_gem_track_fb(intel_fb_obj(old_plane_state->fb),
> +				  intel_fb_obj(plane->state->fb),
> +				  to_intel_plane(plane)->frontbuffer_bit);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 01056ce8d461..5294039cf238 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1147,27 +1147,35 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
>  				   uint64_t fb_format_modifier);
>  
>  void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> -			       enum fb_op_origin origin);
> +			       enum fb_op_origin origin,
> +			       unsigned int frontbuffer_bits);
>  static inline void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>  					   enum fb_op_origin origin)
>  {
> -	if (!obj->frontbuffer_bits)
> +	unsigned int frontbuffer_bits;
> +
> +	frontbuffer_bits = atomic_read(&obj->frontbuffer_bits);
> +	if (!frontbuffer_bits)
>  		return;
>  
> -	__intel_fb_obj_invalidate(obj, origin);
> +	__intel_fb_obj_invalidate(obj, origin, frontbuffer_bits);
>  }
>  
>  void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>  			  bool retire,
> -			  enum fb_op_origin origin);
> +			  enum fb_op_origin origin,
> +			  unsigned int frontbuffer_tibst);
>  static inline void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>  				      bool retire,
>  				      enum fb_op_origin origin)
>  {
> -	if (!obj->frontbuffer_bits)
> +	unsigned int frontbuffer_bits;
> +
> +	frontbuffer_bits = atomic_read(&obj->frontbuffer_bits);
> +	if (!frontbuffer_bits)
>  		return;
>  
> -	__intel_fb_obj_flush(obj, retire, origin);
> +	__intel_fb_obj_flush(obj, retire, origin, frontbuffer_bits);
>  }
>  
>  u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index a38ccfe4894a..636324da21c2 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -77,23 +77,22 @@
>   * scheduled.
>   */
>  void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> -			       enum fb_op_origin origin)
> +			       enum fb_op_origin origin,
> +			       unsigned int frontbuffer_bits)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
>  	if (origin == ORIGIN_CS) {
>  		spin_lock(&dev_priv->fb_tracking.lock);
> -		dev_priv->fb_tracking.busy_bits |= obj->frontbuffer_bits;
> -		dev_priv->fb_tracking.flip_bits &= ~obj->frontbuffer_bits;
> +		dev_priv->fb_tracking.busy_bits |= frontbuffer_bits;
> +		dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
>  		spin_unlock(&dev_priv->fb_tracking.lock);
>  	}
>  
> -	intel_psr_invalidate(dev, obj->frontbuffer_bits);
> -	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
> -	intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
> +	intel_psr_invalidate(dev, frontbuffer_bits);
> +	intel_edp_drrs_invalidate(dev, frontbuffer_bits);
> +	intel_fbc_invalidate(dev_priv, frontbuffer_bits, origin);
>  }
>  
>  /**
> @@ -139,15 +138,11 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
>   */
>  void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>  			  bool retire,
> -			  enum fb_op_origin origin)
> +			  enum fb_op_origin origin,
> +			  unsigned int frontbuffer_bits)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	unsigned frontbuffer_bits;
> -
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> -	frontbuffer_bits = obj->frontbuffer_bits;
>  
>  	if (retire) {
>  		spin_lock(&dev_priv->fb_tracking.lock);
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx