Re: [PATCH 16/22] drm/i915: Make fb_tracking.lock a spinlock

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

 



On Thu, Jul 28, 2016 at 12:02:01PM +0200, Daniel Vetter wrote:
> On Wed, Jul 27, 2016 at 12:14:54PM +0100, Chris Wilson wrote:
> > We only need a very lightweight mechanism here as the locking is only
> > used for co-ordinating a bitfield.
> > 
> > v2: Move the cheap unlikely tests into the caller
> > 
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> 
> I think the code shuffling in here badly breaks the kerneldoc. Best fix
> would be to extract a small header for frontbuffer tracking and pull that
> into the kernel doc. Much less preferred is to explicitly pull in the
> kerneldoc function-by-function (but that tends to be rather fragile when
> someone adds something new).

fwiw the change itself looks correct and makes sense, so with
intel_frontbuffer.h extracted and the kerneldoc include directives in
gpu/drm-i915.rst fixed up this is:

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

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  2 +-
> >  drivers/gpu/drm/i915/i915_gem.c          |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h         | 29 ++++++++++++++---
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 54 ++++++++++++++------------------
> >  4 files changed, 51 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3a68e604ad10..a24d31e3e014 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1669,7 +1669,7 @@ struct intel_pipe_crc {
> >  };
> >  
> >  struct i915_frontbuffer_tracking {
> > -	struct mutex lock;
> > +	spinlock_t lock;
> >  
> >  	/*
> >  	 * Tracking bits for delayed frontbuffer flushing du to gpu activity or
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 1fb958dcc749..7db0808f6961 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4447,7 +4447,7 @@ i915_gem_load_init(struct drm_device *dev)
> >  
> >  	dev_priv->mm.interruptible = true;
> >  
> > -	mutex_init(&dev_priv->fb_tracking.lock);
> > +	spin_lock_init(&dev_priv->fb_tracking.lock);
> >  }
> >  
> >  void i915_gem_load_cleanup(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e74d851868c5..01056ce8d461 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1135,8 +1135,6 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> >  
> >  /* intel_frontbuffer.c */
> > -void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > -			     enum fb_op_origin origin);
> >  void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> >  				    unsigned frontbuffer_bits);
> >  void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > @@ -1147,8 +1145,31 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
> >  				   unsigned int height,
> >  				   uint32_t pixel_format,
> >  				   uint64_t fb_format_modifier);
> > -void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> > -			enum fb_op_origin origin);
> > +
> > +void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > +			       enum fb_op_origin origin);
> > +static inline void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > +					   enum fb_op_origin origin)
> > +{
> > +	if (!obj->frontbuffer_bits)
> > +		return;
> > +
> > +	__intel_fb_obj_invalidate(obj, origin);
> > +}
> > +
> > +void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > +			  bool retire,
> > +			  enum fb_op_origin origin);
> > +static inline void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > +				      bool retire,
> > +				      enum fb_op_origin origin)
> > +{
> > +	if (!obj->frontbuffer_bits)
> > +		return;
> > +
> > +	__intel_fb_obj_flush(obj, retire, origin);
> > +}
> > +
> >  u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> >  			      uint64_t fb_modifier, uint32_t pixel_format);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index ac85357010b4..a38ccfe4894a 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -76,24 +76,19 @@
> >   * until the rendering completes or a flip on this frontbuffer plane is
> >   * scheduled.
> >   */
> > -void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > -			     enum fb_op_origin origin)
> > +void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > +			       enum fb_op_origin origin)
> >  {
> >  	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 (!obj->frontbuffer_bits)
> > -		return;
> > -
> >  	if (origin == ORIGIN_CS) {
> > -		mutex_lock(&dev_priv->fb_tracking.lock);
> > -		dev_priv->fb_tracking.busy_bits
> > -			|= obj->frontbuffer_bits;
> > -		dev_priv->fb_tracking.flip_bits
> > -			&= ~obj->frontbuffer_bits;
> > -		mutex_unlock(&dev_priv->fb_tracking.lock);
> > +		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;
> > +		spin_unlock(&dev_priv->fb_tracking.lock);
> >  	}
> >  
> >  	intel_psr_invalidate(dev, obj->frontbuffer_bits);
> > @@ -120,11 +115,11 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  	/* Delay flushing when rings are still busy.*/
> > -	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	spin_lock(&dev_priv->fb_tracking.lock);
> >  	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
> > -	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +	spin_unlock(&dev_priv->fb_tracking.lock);
> >  
> > -	if (!frontbuffer_bits)
> > +	if (frontbuffer_bits == 0)
> >  		return;
> >  
> >  	intel_edp_drrs_flush(dev, frontbuffer_bits);
> > @@ -142,8 +137,9 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
> >   * completed and frontbuffer caching can be started again. If @retire is true
> >   * then any delayed flushes will be unblocked.
> >   */
> > -void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > -			bool retire, enum fb_op_origin origin)
> > +void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > +			  bool retire,
> > +			  enum fb_op_origin origin)
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -151,21 +147,18 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> >  
> >  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >  
> > -	if (!obj->frontbuffer_bits)
> > -		return;
> > -
> >  	frontbuffer_bits = obj->frontbuffer_bits;
> >  
> >  	if (retire) {
> > -		mutex_lock(&dev_priv->fb_tracking.lock);
> > +		spin_lock(&dev_priv->fb_tracking.lock);
> >  		/* Filter out new bits since rendering started. */
> >  		frontbuffer_bits &= dev_priv->fb_tracking.busy_bits;
> > -
> >  		dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > -		mutex_unlock(&dev_priv->fb_tracking.lock);
> > +		spin_unlock(&dev_priv->fb_tracking.lock);
> >  	}
> >  
> > -	intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
> > +	if (frontbuffer_bits)
> > +		intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
> >  }
> >  
> >  /**
> > @@ -185,11 +178,11 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > -	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	spin_lock(&dev_priv->fb_tracking.lock);
> >  	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> >  	/* Remove stale busy bits due to the old buffer. */
> >  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > -	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +	spin_unlock(&dev_priv->fb_tracking.lock);
> >  
> >  	intel_psr_single_frame_update(dev, frontbuffer_bits);
> >  }
> > @@ -209,13 +202,14 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > -	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	spin_lock(&dev_priv->fb_tracking.lock);
> >  	/* Mask any cancelled flips. */
> >  	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> >  	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > -	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +	spin_unlock(&dev_priv->fb_tracking.lock);
> >  
> > -	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> > +	if (frontbuffer_bits)
> > +		intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> >  }
> >  
> >  /**
> > @@ -234,10 +228,10 @@ void intel_frontbuffer_flip(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > -	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	spin_lock(&dev_priv->fb_tracking.lock);
> >  	/* Remove stale busy bits due to the old buffer. */
> >  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > -	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +	spin_unlock(&dev_priv->fb_tracking.lock);
> >  
> >  	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> >  }
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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