public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 04/13] drm/i915: Make semaphore updates more precise
Date: Wed, 30 Apr 2014 14:45:45 +0200	[thread overview]
Message-ID: <20140430124545.GG20800@phenom.ffwll.local> (raw)
In-Reply-To: <1398808360-3674-5-git-send-email-benjamin.widawsky@intel.com>

On Tue, Apr 29, 2014 at 02:52:31PM -0700, 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. This also
> allows us to drop the unnecessary MI_NOOP, so not really 4, 3.
> 
> v2: Use 3 dwords instead of 4 (Ville)
> Do the proper calculation to get the number of dwords to emit (Ville)
> Conditionally set .sync_to when semaphores are enabled (Ville)
> 
> v3: Rebased on VCS2
> Replace hweight_long with hweight32 (Ville)
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v1)
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 173 +++++++++++++++++---------------
>  1 file changed, 90 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e0c7bf2..7aedc0c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -666,24 +666,19 @@ 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 3
>  	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 = hweight32(INTEL_INFO(dev)->ring_mask);
> +	num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
> +#undef MBOX_UPDATE_DWORDS
>  
>  	ret = intel_ring_begin(signaller, num_dwords);
>  	if (ret)
>  		return ret;
> -#undef MBOX_UPDATE_DWORDS
>  
>  	for_each_ring(useless, dev_priv, i) {
>  		u32 mbox_reg = signaller->semaphore.mbox.signal[i];
> @@ -691,15 +686,13 @@ 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);
> -		} 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);
>  		}
>  	}
>  
> +	/* If num_dwords was rounded, make sure the tail pointer is correct */
> +	if (num_rings % 2 == 0)
> +		intel_ring_emit(signaller, MI_NOOP);
> +
>  	return 0;
>  }
>  
> @@ -717,7 +710,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;
>  

The hunks below look like a different patch. Accidental squash while
rebasing?

I've merged patches 1-3 of this series already.
-Daniel

> @@ -1928,24 +1925,27 @@ 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;
> -		ring->semaphore.signal = gen6_signal;
> -		/*
> -		 * The current semaphore is only applied on pre-gen8 platform.
> -		 * And there is no VCS2 ring on the pre-gen8 platform. So the
> -		 * semaphore between RCS and VCS2 is initialized as INVALID.
> -		 * Gen8 will initialize the sema between VCS2 and RCS later.
> -		 */
> -		ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_INVALID;
> -		ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_RV;
> -		ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_RB;
> -		ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_RVE;
> -		ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> -		ring->semaphore.mbox.signal[RCS] = GEN6_NOSYNC;
> -		ring->semaphore.mbox.signal[VCS] = GEN6_VRSYNC;
> -		ring->semaphore.mbox.signal[BCS] = GEN6_BRSYNC;
> -		ring->semaphore.mbox.signal[VECS] = GEN6_VERSYNC;
> -		ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +		if (i915_semaphore_is_enabled(dev)) {
> +			ring->semaphore.sync_to = gen6_ring_sync;
> +			ring->semaphore.signal = gen6_signal;
> +			/*
> +			 * The current semaphore is only applied on pre-gen8
> +			 * platform.  And there is no VCS2 ring on the pre-gen8
> +			 * platform. So the semaphore between RCS and VCS2 is
> +			 * initialized as INVALID.  Gen8 will initialize the
> +			 * sema between VCS2 and RCS later.
> +			 */
> +			ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_INVALID;
> +			ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_RV;
> +			ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_RB;
> +			ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_RVE;
> +			ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> +			ring->semaphore.mbox.signal[RCS] = GEN6_NOSYNC;
> +			ring->semaphore.mbox.signal[VCS] = GEN6_VRSYNC;
> +			ring->semaphore.mbox.signal[BCS] = GEN6_BRSYNC;
> +			ring->semaphore.mbox.signal[VECS] = GEN6_VERSYNC;
> +			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +		}
>  	} else if (IS_GEN5(dev)) {
>  		ring->add_request = pc_render_add_request;
>  		ring->flush = gen4_render_ring_flush;
> @@ -2113,24 +2113,27 @@ int intel_init_bsd_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;
> -		/*
> -		 * The current semaphore is only applied on pre-gen8 platform.
> -		 * And there is no VCS2 ring on the pre-gen8 platform. So the
> -		 * semaphore between VCS and VCS2 is initialized as INVALID.
> -		 * Gen8 will initialize the sema between VCS2 and VCS later.
> -		 */
> -		ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_VR;
> -		ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_INVALID;
> -		ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_VB;
> -		ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_VVE;
> -		ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> -		ring->semaphore.mbox.signal[RCS] = GEN6_RVSYNC;
> -		ring->semaphore.mbox.signal[VCS] = GEN6_NOSYNC;
> -		ring->semaphore.mbox.signal[BCS] = GEN6_BVSYNC;
> -		ring->semaphore.mbox.signal[VECS] = GEN6_VEVSYNC;
> -		ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +		if (i915_semaphore_is_enabled(dev)) {
> +			ring->semaphore.sync_to = gen6_ring_sync;
> +			ring->semaphore.signal = gen6_signal;
> +			/*
> +			 * The current semaphore is only applied on pre-gen8
> +			 * platform.  And there is no VCS2 ring on the pre-gen8
> +			 * platform. So the semaphore between VCS and VCS2 is
> +			 * initialized as INVALID.  Gen8 will initialize the
> +			 * sema between VCS2 and VCS later.
> +			 */
> +			ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_VR;
> +			ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_INVALID;
> +			ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_VB;
> +			ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_VVE;
> +			ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> +			ring->semaphore.mbox.signal[RCS] = GEN6_RVSYNC;
> +			ring->semaphore.mbox.signal[VCS] = GEN6_NOSYNC;
> +			ring->semaphore.mbox.signal[BCS] = GEN6_BVSYNC;
> +			ring->semaphore.mbox.signal[VECS] = GEN6_VEVSYNC;
> +			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +		}
>  	} else {
>  		ring->mmio_base = BSD_RING_BASE;
>  		ring->flush = bsd_ring_flush;
> @@ -2231,24 +2234,26 @@ 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;
> -	ring->semaphore.signal = gen6_signal;
> -	/*
> -	 * The current semaphore is only applied on pre-gen8 platform. And
> -	 * there is no VCS2 ring on the pre-gen8 platform. So the semaphore
> -	 * between BCS and VCS2 is initialized as INVALID.
> -	 * Gen8 will initialize the sema between BCS and VCS2 later.
> -	 */
> -	ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_BR;
> -	ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_BV;
> -	ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_INVALID;
> -	ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_BVE;
> -	ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> -	ring->semaphore.mbox.signal[RCS] = GEN6_RBSYNC;
> -	ring->semaphore.mbox.signal[VCS] = GEN6_VBSYNC;
> -	ring->semaphore.mbox.signal[BCS] = GEN6_NOSYNC;
> -	ring->semaphore.mbox.signal[VECS] = GEN6_VEBSYNC;
> -	ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +	if (i915_semaphore_is_enabled(dev)) {
> +		ring->semaphore.signal = gen6_signal;
> +		ring->semaphore.sync_to = gen6_ring_sync;
> +		/*
> +		 * The current semaphore is only applied on pre-gen8 platform.
> +		 * And there is no VCS2 ring on the pre-gen8 platform. So the
> +		 * semaphore between BCS and VCS2 is initialized as INVALID.
> +		 * Gen8 will initialize the sema between BCS and VCS2 later.
> +		 */
> +		ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_BR;
> +		ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_BV;
> +		ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_INVALID;
> +		ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_BVE;
> +		ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> +		ring->semaphore.mbox.signal[RCS] = GEN6_RBSYNC;
> +		ring->semaphore.mbox.signal[VCS] = GEN6_VBSYNC;
> +		ring->semaphore.mbox.signal[BCS] = GEN6_NOSYNC;
> +		ring->semaphore.mbox.signal[VECS] = GEN6_VEBSYNC;
> +		ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +	}
>  	ring->init = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
> @@ -2281,18 +2286,20 @@ 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;
> -	ring->semaphore.signal = gen6_signal;
> -	ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_VER;
> -	ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_VEV;
> -	ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_VEB;
> -	ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_INVALID;
> -	ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> -	ring->semaphore.mbox.signal[RCS] = GEN6_RVESYNC;
> -	ring->semaphore.mbox.signal[VCS] = GEN6_VVESYNC;
> -	ring->semaphore.mbox.signal[BCS] = GEN6_BVESYNC;
> -	ring->semaphore.mbox.signal[VECS] = GEN6_NOSYNC;
> -	ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +	if (i915_semaphore_is_enabled(dev)) {
> +		ring->semaphore.sync_to = gen6_ring_sync;
> +		ring->semaphore.signal = gen6_signal;
> +		ring->semaphore.mbox.wait[RCS] = MI_SEMAPHORE_SYNC_VER;
> +		ring->semaphore.mbox.wait[VCS] = MI_SEMAPHORE_SYNC_VEV;
> +		ring->semaphore.mbox.wait[BCS] = MI_SEMAPHORE_SYNC_VEB;
> +		ring->semaphore.mbox.wait[VECS] = MI_SEMAPHORE_SYNC_INVALID;
> +		ring->semaphore.mbox.wait[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> +		ring->semaphore.mbox.signal[RCS] = GEN6_RVESYNC;
> +		ring->semaphore.mbox.signal[VCS] = GEN6_VVESYNC;
> +		ring->semaphore.mbox.signal[BCS] = GEN6_BVESYNC;
> +		ring->semaphore.mbox.signal[VECS] = GEN6_NOSYNC;
> +		ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +	}
>  	ring->init = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

  reply	other threads:[~2014-04-30 12:45 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-29 21:52 [PATCH 00/13] [REPOST] BDW Semaphores Ben Widawsky
2014-04-29 21:52 ` [PATCH 01/13] drm/i915: Move semaphore specific ring members to struct Ben Widawsky
2014-04-29 21:52 ` [PATCH 02/13] drm/i915: Virtualize the ringbuffer signal func Ben Widawsky
2014-04-29 21:52 ` [PATCH 03/13] drm/i915: Move ring_begin to signal() 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 [this message]
2014-04-29 21:52 ` [PATCH 05/13] drm/i915: gen specific ring init Ben Widawsky
2014-04-29 21:52 ` [PATCH 06/13] drm/i915/bdw: implement semaphore signal Ben Widawsky
2014-04-29 21:52 ` [PATCH 07/13] drm/i915/bdw: implement semaphore wait Ben Widawsky
2014-04-29 21:52 ` [PATCH 08/13] drm/i915: Implement MI decode for gen8 Ben Widawsky
2014-04-30 11:21   ` Ville Syrjälä
2014-05-07 16:59     ` Ben Widawsky
2014-05-07 17:09       ` Ville Syrjälä
2014-04-29 21:52 ` [PATCH 09/13] drm/i915/bdw: poll semaphores Ben Widawsky
2014-04-30 10:53   ` Ville Syrjälä
2014-04-29 21:52 ` [PATCH 10/13] drm/i915: Extract semaphore error collection Ben Widawsky
2014-04-29 21:52 ` [PATCH 11/13] drm/i915/bdw: collect semaphore error state Ben Widawsky
2014-04-29 21:52 ` [PATCH 12/13] drm/i915: semaphore debugfs Ben Widawsky
2014-05-03  2:23   ` [PATCH 12.1/13] drm/i915: Small semaphore debugfs fixup Ben Widawsky
2014-04-29 21:52 ` [PATCH 13/13] DONT_MERGE drm/i915: FORCE_RESTORE for gen8 semaphores Ben Widawsky
2014-04-30  7:13   ` Chris Wilson
2014-04-30 18:44     ` Ben Widawsky
2014-04-30 19:03       ` Chris Wilson
2014-04-30 19:27         ` Ben Widawsky
2014-04-30 11:35 ` [PATCH 00/13] [REPOST] BDW Semaphores Ville Syrjälä
  -- 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-01-29 19:55 [PATCH 00/13] [REPOST] Broadwell HW semaphores 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
2014-02-11 17:13       ` Ville Syrjälä

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=20140430124545.GG20800@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=benjamin.widawsky@intel.com \
    --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