public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 16/22] drm/i915: Make fb_tracking.lock a spinlock
Date: Thu, 28 Jul 2016 12:08:34 +0200	[thread overview]
Message-ID: <20160728100834.GG6232@phenom.ffwll.local> (raw)
In-Reply-To: <20160728100201.GD6232@phenom.ffwll.local>

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@chris-wilson.co.uk>
> 
> 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@ffwll.ch>

> -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@lists.freedesktop.org
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-07-28 10:08 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-27 11:14 Getting to RCU and exporting fences Chris Wilson
2016-07-27 11:14 ` [PATCH 01/22] drm/i915: Combine loops within i915_gem_evict_something Chris Wilson
2016-07-29  6:17   ` Joonas Lahtinen
2016-07-29  6:31     ` Chris Wilson
2016-07-27 11:14 ` [PATCH 02/22] drm/i915: Remove surplus drm_device parameter to i915_gem_evict_something() Chris Wilson
2016-07-28  8:07   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 03/22] drm/i915: Double check the active status on the batch pool Chris Wilson
2016-07-28  8:14   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 04/22] drm/i915: Remove request retirement before each batch Chris Wilson
2016-07-28  8:32   ` Joonas Lahtinen
2016-07-28  9:32     ` Chris Wilson
2016-07-28  9:53       ` Joonas Lahtinen
2016-07-28  9:54   ` Daniel Vetter
2016-07-28 10:26     ` Chris Wilson
2016-07-28 11:52       ` Daniel Vetter
2016-07-28 12:24         ` Chris Wilson
2016-07-28 14:21           ` Chris Wilson
2016-07-27 11:14 ` [PATCH 05/22] drm/i915: Remove i915_gem_execbuffer_retire_commands() Chris Wilson
2016-07-28  8:46   ` Joonas Lahtinen
2016-07-28  8:55     ` Chris Wilson
2016-07-28  9:54       ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 06/22] drm/i915: Fix up vma alignment to be u64 Chris Wilson
2016-07-28  8:59   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 07/22] drm/i915: Pad GTT views of exec objects up to user specified size Chris Wilson
2016-07-28  9:55   ` Daniel Vetter
2016-07-28 10:33     ` Chris Wilson
2016-07-29  7:59   ` Joonas Lahtinen
2016-07-29  8:08     ` Chris Wilson
2016-07-29  8:55       ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 08/22] drm/i915: Reduce WARN(i915_gem_valid_gtt_space) to a debug-only check Chris Wilson
2016-07-28  9:18   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 09/22] drm/i915: Split insertion/binding of an object into the VM Chris Wilson
2016-07-28  9:25   ` Joonas Lahtinen
2016-07-28  9:34     ` Chris Wilson
2016-07-27 11:14 ` [PATCH 10/22] drm/i915: Record allocated vma size Chris Wilson
2016-07-29  6:53   ` Joonas Lahtinen
2016-07-29  7:18     ` Chris Wilson
2016-07-29 10:19     ` [PATCH] drm/i915: Convert 4096 alignment request to 0 for drm_mm allocations Chris Wilson
2016-07-29 10:28       ` Joonas Lahtinen
2016-07-29 10:38         ` Chris Wilson
2016-07-27 11:14 ` [PATCH 11/22] drm/i915: Wrap vma->pin_count accessors with small inline helpers Chris Wilson
2016-07-29  6:59   ` Joonas Lahtinen
2016-07-29  7:23     ` Chris Wilson
2016-07-27 11:14 ` [PATCH 12/22] drm/i915: Start passing around i915_vma from execbuffer Chris Wilson
2016-07-29  8:23   ` Joonas Lahtinen
2016-08-01  7:34     ` Chris Wilson
2016-07-27 11:14 ` [PATCH 13/22] drm/i915: Combine all i915_vma bitfields into a single set of flags Chris Wilson
2016-07-29  7:30   ` Joonas Lahtinen
2016-07-29  7:44     ` Chris Wilson
2016-07-27 11:14 ` [PATCH 14/22] drm/i915: Make i915_vma_pin() small and inline Chris Wilson
2016-07-28 11:06   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 15/22] drm/i915: Remove highly confusing i915_gem_obj_ggtt_pin() Chris Wilson
2016-07-28 10:38   ` Joonas Lahtinen
2016-07-28 11:36     ` Chris Wilson
2016-07-28 11:53       ` Joonas Lahtinen
2016-07-28 16:12         ` Chris Wilson
2016-07-29  9:10           ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 16/22] drm/i915: Make fb_tracking.lock a spinlock Chris Wilson
2016-07-28 10:02   ` Daniel Vetter
2016-07-28 10:08     ` Daniel Vetter [this message]
2016-07-29  8:25     ` Chris Wilson
2016-07-28 10:19   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 17/22] drm/i915: Use atomics to manipulate obj->frontbuffer_bits Chris Wilson
2016-07-28  9:49   ` Joonas Lahtinen
2016-07-28 10:10     ` Chris Wilson
2016-07-28 10:51       ` Joonas Lahtinen
2016-07-28 10:05   ` Daniel Vetter
2016-07-27 11:14 ` [PATCH 18/22] drm/i915: Use dev_priv consistently through the intel_frontbuffer interface Chris Wilson
2016-07-28  9:36   ` Joonas Lahtinen
2016-07-28 10:06   ` Daniel Vetter
2016-07-27 11:14 ` [PATCH 19/22] drm/i915: Move obj->active:5 to obj->flags Chris Wilson
2016-07-29  7:40   ` Joonas Lahtinen
2016-07-29  8:04     ` Chris Wilson
2016-07-29  8:10       ` Chris Wilson
2016-07-29  9:34         ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 20/22] drm/i915: Move i915_gem_object_wait_rendering() Chris Wilson
2016-07-28  9:37   ` Joonas Lahtinen
2016-07-27 11:14 ` [PATCH 21/22] drm/i915: Enable lockless lookup of request tracking via RCU Chris Wilson
2016-07-28 10:23   ` Daniel Vetter
2016-07-28 20:49     ` Chris Wilson
2016-07-29  8:41       ` Daniel Vetter
2016-07-29  8:49         ` Chris Wilson
2016-07-29  9:43           ` Chris Wilson
2016-07-29  9:45             ` Daniel Vetter
2016-07-27 11:15 ` [PATCH 22/22] drm/i915: Export our request as a dma-buf fence on the reservation object Chris Wilson
2016-07-28 10:32   ` Daniel Vetter
2016-07-28 10:40     ` Chris Wilson
2016-07-28 11:59       ` Daniel Vetter
2016-07-28 12:17         ` Chris Wilson
2016-07-28 12:28           ` Daniel Vetter
2016-07-28 12:45             ` Chris Wilson
2016-07-28 20:14               ` Daniel Vetter
2016-07-28 21:08                 ` Chris Wilson
2016-07-27 11:23 ` ✗ Ro.CI.BAT: failure for series starting with [01/22] drm/i915: Combine loops within i915_gem_evict_something Patchwork
2016-07-29 10:20 ` ✗ Ro.CI.BAT: failure for series starting with [01/22] drm/i915: Combine loops within i915_gem_evict_something (rev2) Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160728100834.GG6232@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox