public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Ben Widawsky <ben@bwidawsk.net>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH 04/13] drm/i915: Make semaphore updates more precise
Date: Tue, 11 Feb 2014 08:08:27 -0800	[thread overview]
Message-ID: <20140211160824.GA10233@bwidawsk.net> (raw)
In-Reply-To: <20140130112542.GI9454@intel.com>

On Thu, Jan 30, 2014 at 01:25:42PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 29, 2014 at 11:55:24AM -0800, Ben Widawsky wrote:
> > With the ring mask we now have an easy way to know the number of rings
> > in the system, and therefore can accurately predict the number of dwords
> > to emit for semaphore signalling. This was not possible (easily)
> > previously.
> > 
> > There should be no functional impact, simply fewer instructions emitted.
> > 
> > While we're here, simply do the round up to 2 instead of the fancier
> > rounding we did before, which rounding up per mbox, ie 4.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 43 +++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 70f7190..97789ff 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -635,24 +635,20 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
> >  static int gen6_signal(struct intel_ring_buffer *signaller,
> >  		       unsigned int num_dwords)
> >  {
> > +#define MBOX_UPDATE_DWORDS 4
> >  	struct drm_device *dev = signaller->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_ring_buffer *useless;
> > -	int i, ret;
> > +	int i, ret, num_rings;
> >  
> > -	/* NB: In order to be able to do semaphore MBOX updates for varying
> > -	 * number of rings, it's easiest if we round up each individual update
> > -	 * to a multiple of 2 (since ring updates must always be a multiple of
> > -	 * 2) even though the actual update only requires 3 dwords.
> > -	 */
> > -#define MBOX_UPDATE_DWORDS 4
> > -	if (i915_semaphore_is_enabled(dev))
> > -		num_dwords += ((I915_NUM_RINGS-1) * MBOX_UPDATE_DWORDS);
> > +	num_rings = hweight_long(INTEL_INFO(dev)->ring_mask);
> > +	num_dwords = round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
> 
> num_dwords +=
> 

Hmm. I think I may have submitted the wrong patch here since I was
pretty certain Chris had caught this before. Anyway, thanks.

> Also round_up() is useless since it's already a multiple of 4. Or did
> you mean to change it to emit only 3 dwords per mbox?

Yep, I meant to change to 3, thanks. Double on the, maybe this was the
wrong patch.

> 
> > +#undef MBOX_UPDATE_DWORDS
> >  
> > -	ret = intel_ring_begin(signaller, num_dwords);
> > +	/* XXX: + 4 for the caller */
> > +	ret = intel_ring_begin(signaller, num_dwords + 4);
> 
> The += earlier gets rid of the +4 here.
> 
> >  	if (ret)
> >  		return ret;
> > -#undef MBOX_UPDATE_DWORDS
> >  
> >  	for_each_ring(useless, dev_priv, i) {
> >  		u32 mbox_reg = signaller->semaphore.signal_mbox[i];
> > @@ -661,14 +657,11 @@ static int gen6_signal(struct intel_ring_buffer *signaller,
> >  			intel_ring_emit(signaller, mbox_reg);
> >  			intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
> >  			intel_ring_emit(signaller, MI_NOOP);
> > -		} else {
> > -			intel_ring_emit(signaller, MI_NOOP);
> > -			intel_ring_emit(signaller, MI_NOOP);
> > -			intel_ring_emit(signaller, MI_NOOP);
> > -			intel_ring_emit(signaller, MI_NOOP);
> >  		}
> >  	}
> >  
> > +	WARN_ON(i != num_rings);
> 
> So we're not expecting dev_priv->ring[] to be sparsely populated ever?

I'll never say "ever." Currently however it is unexpected. I suppose it
would be nicer to hit the WARN sooner in the init path, but I'm not sure
how offended you are by this. The code is still slightly muddy with
regards to rings on the HW vs. enabled rings - but it's a separate issue
which is slowly getting better.

In either case, there is a bug here because for_each_ring will be >
num_rings on SNB and IVB.

> 
> > +
> >  	return 0;
> >  }
> >  
> > @@ -686,7 +679,11 @@ gen6_add_request(struct intel_ring_buffer *ring)
> >  {
> >  	int ret;
> >  
> > -	ret = ring->semaphore.signal(ring, 4);
> > +	if (ring->semaphore.signal)
> > +		ret = ring->semaphore.signal(ring, 4);
> > +	else
> > +		ret = intel_ring_begin(ring, 4);
> > +
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1881,7 +1878,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> >  		ring->get_seqno = gen6_ring_get_seqno;
> >  		ring->set_seqno = ring_set_seqno;
> >  		ring->semaphore.sync_to = gen6_ring_sync;
> > -		ring->semaphore.signal = gen6_signal;
> > +		if (i915_semaphore_is_enabled(dev))
> > +			ring->semaphore.signal = gen6_signal;
> 
> I guess we could also set .sync_to conditionally, but doesn't really
> matter since we won't call it anyway w/o semaphores enabled.
> 

I prefer that. Not sure why I didn't do it in the first place.

> >  		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_INVALID;
> >  		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_RV;
> >  		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_RB;
> > @@ -2058,7 +2056,8 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
> >  				gen6_ring_dispatch_execbuffer;
> >  		}
> >  		ring->semaphore.sync_to = gen6_ring_sync;
> > -		ring->semaphore.signal = gen6_signal;
> > +		if (i915_semaphore_is_enabled(dev))
> > +			ring->semaphore.signal = gen6_signal;
> >  		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VR;
> >  		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_INVALID;
> >  		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VB;
> > @@ -2116,7 +2115,8 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
> >  		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
> >  	}
> >  	ring->semaphore.sync_to = gen6_ring_sync;
> > -	ring->semaphore.signal = gen6_signal;
> > +	if (i915_semaphore_is_enabled(dev))
> > +		ring->semaphore.signal = gen6_signal;
> >  	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_BR;
> >  	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_BV;
> >  	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_INVALID;
> > @@ -2158,7 +2158,8 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
> >  		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
> >  	}
> >  	ring->semaphore.sync_to = gen6_ring_sync;
> > -	ring->semaphore.signal = gen6_signal;
> > +	if (i915_semaphore_is_enabled(dev))
> > +		ring->semaphore.signal = gen6_signal;
> >  	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VER;
> >  	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_VEV;
> >  	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VEB;
> -- 
> Ville Syrjälä
> Intel OTC

FYI: here is the diff I am using:

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 97789ff..3bec0f5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -635,18 +635,18 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
 static int gen6_signal(struct intel_ring_buffer *signaller,
 		       unsigned int num_dwords)
 {
-#define MBOX_UPDATE_DWORDS 4
+#define MBOX_UPDATE_DWORDS 3
 	struct drm_device *dev = signaller->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *useless;
 	int i, ret, num_rings;
 
 	num_rings = hweight_long(INTEL_INFO(dev)->ring_mask);
-	num_dwords = round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
+	num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
 #undef MBOX_UPDATE_DWORDS
 
 	/* XXX: + 4 for the caller */
-	ret = intel_ring_begin(signaller, num_dwords + 4);
+	ret = intel_ring_begin(signaller, num_dwords);
 	if (ret)
 		return ret;
 
@@ -656,7 +656,6 @@ static int gen6_signal(struct intel_ring_buffer *signaller,
 			intel_ring_emit(signaller, MI_LOAD_REGISTER_IMM(1));
 			intel_ring_emit(signaller, mbox_reg);
 			intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
-			intel_ring_emit(signaller, MI_NOOP);
 		}
 	}
 
@@ -1877,9 +1876,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
 		ring->get_seqno = gen6_ring_get_seqno;
 		ring->set_seqno = ring_set_seqno;
-		ring->semaphore.sync_to = gen6_ring_sync;
-		if (i915_semaphore_is_enabled(dev))
+		if (i915_semaphore_is_enabled(dev)) {
+			ring->semaphore.sync_to = gen6_ring_sync;
 			ring->semaphore.signal = gen6_signal;
+		}
 		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_INVALID;
 		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_RV;
 		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_RB;
@@ -2055,9 +2055,10 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 			ring->dispatch_execbuffer =
 				gen6_ring_dispatch_execbuffer;
 		}
-		ring->semaphore.sync_to = gen6_ring_sync;
-		if (i915_semaphore_is_enabled(dev))
+		if (i915_semaphore_is_enabled(dev)) {
+			ring->semaphore.sync_to = gen6_ring_sync;
 			ring->semaphore.signal = gen6_signal;
+		}
 		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VR;
 		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_INVALID;
 		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VB;
@@ -2114,9 +2115,10 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 		ring->irq_put = gen6_ring_put_irq;
 		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
 	}
-	ring->semaphore.sync_to = gen6_ring_sync;
-	if (i915_semaphore_is_enabled(dev))
+	if (i915_semaphore_is_enabled(dev)) {
 		ring->semaphore.signal = gen6_signal;
+		ring->semaphore.sync_to = gen6_ring_sync;
+	}
 	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_BR;
 	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_BV;
 	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_INVALID;
@@ -2157,9 +2159,10 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 		ring->irq_put = hsw_vebox_put_irq;
 		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
 	}
-	ring->semaphore.sync_to = gen6_ring_sync;
-	if (i915_semaphore_is_enabled(dev))
+	if (i915_semaphore_is_enabled(dev)) {
+		ring->semaphore.sync_to = gen6_ring_sync;
 		ring->semaphore.signal = gen6_signal;
+	}
 	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VER;
 	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_VEV;
 	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VEB;

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2014-02-11 16:08 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-29 19:55 [PATCH 00/13] [REPOST] Broadwell HW semaphores Ben Widawsky
2014-01-29 19:55 ` [PATCH 01/13] drm/i915: Move semaphore specific ring members to struct Ben Widawsky
2014-01-29 19:55 ` [PATCH 02/13] drm/i915: Virtualize the ringbuffer signal func Ben Widawsky
2014-01-29 19:55 ` [PATCH 03/13] drm/i915: Move ring_begin to signal() Ben Widawsky
2014-01-29 19:55 ` [PATCH 04/13] drm/i915: Make semaphore updates more precise Ben Widawsky
2014-01-30 11:25   ` Ville Syrjälä
2014-02-11 16:08     ` Ben Widawsky [this message]
2014-02-11 17:13       ` Ville Syrjälä
2014-02-11 20:20   ` [PATCH] [v2] " Ben Widawsky
2014-02-11 20:53     ` Ville Syrjälä
2014-02-11 21:50       ` Ben Widawsky
2014-01-29 19:55 ` [PATCH 05/13] drm/i915: gen specific ring init Ben Widawsky
2014-01-29 19:55 ` [PATCH 06/13] drm/i915/bdw: implement semaphore signal Ben Widawsky
2014-01-30 12:38   ` Ville Syrjälä
2014-01-30 12:46     ` Chris Wilson
2014-01-30 13:18       ` Daniel Vetter
2014-01-30 13:25         ` Chris Wilson
2014-01-30 13:35         ` Chris Wilson
2014-02-11 21:48           ` Ben Widawsky
2014-02-11 22:23             ` Chris Wilson
2014-02-11 22:25               ` Ben Widawsky
2014-02-11 22:28                 ` Chris Wilson
2014-02-11 22:11     ` Ben Widawsky
2014-02-11 22:22       ` Ben Widawsky
2014-02-11 23:01         ` Ben Widawsky
2014-02-12  9:29           ` Ville Syrjälä
2014-01-29 19:55 ` [PATCH 07/13] drm/i915/bdw: implement semaphore wait Ben Widawsky
2014-01-30 12:48   ` Ville Syrjälä
2014-01-29 19:55 ` [PATCH 08/13] drm/i915: FORCE_RESTORE for gen8 semaphores Ben Widawsky
2014-01-29 19:55 ` [PATCH 09/13] drm/i915/bdw: poll semaphores Ben Widawsky
2014-01-30 13:26   ` Ville Syrjälä
2014-01-29 19:55 ` [PATCH 10/13] drm/i915: Extract semaphore error collection Ben Widawsky
2014-01-29 19:55 ` [PATCH 11/13] drm/i915/bdw: collect semaphore error state Ben Widawsky
2014-01-30 14:53   ` Ville Syrjälä
2014-01-30 14:58     ` Chris Wilson
2014-02-12  0:19       ` Ben Widawsky
2014-02-12  0:23     ` Ben Widawsky
2014-01-29 19:55 ` [PATCH 12/13] drm/i915: unleash semaphores on gen8 Ben Widawsky
2014-01-29 19:55 ` [PATCH 13/13] drm/i915: semaphore debugfs Ben Widawsky
  -- strict thread matches above, loose matches on Subject: below --
2014-02-20  6:19 [PATCH 01/13] drm/i915: Move semaphore specific ring members to struct Ben Widawsky
2014-02-20  6:19 ` [PATCH 04/13] drm/i915: Make semaphore updates more precise Ben Widawsky
2014-02-24 13:09   ` Ville Syrjälä
2014-04-29 21:52 [PATCH 00/13] [REPOST] BDW Semaphores Ben Widawsky
2014-04-29 21:52 ` [PATCH 04/13] drm/i915: Make semaphore updates more precise Ben Widawsky
2014-04-30 12:45   ` Daniel Vetter

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=20140211160824.GA10233@bwidawsk.net \
    --to=ben@bwidawsk.net \
    --cc=benjamin.widawsky@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=ville.syrjala@linux.intel.com \
    /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