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
next prev parent 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.