From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 01/18] drm/i915: Comments for semaphore clarification Date: Tue, 7 May 2013 09:51:16 -0700 Message-ID: <20130507165116.GA28358@bwidawsk.net> References: <1367110769-1306-1-git-send-email-ben@bwidawsk.net> <1367110769-1306-2-git-send-email-ben@bwidawsk.net> <20130507135406.GE26612@strange.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.localdomain (unknown [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id DAB78E626A for ; Tue, 7 May 2013 09:48:40 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130507135406.GE26612@strange.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Damien Lespiau Cc: Intel-GFX List-Id: intel-gfx@lists.freedesktop.org On Tue, May 07, 2013 at 02:54:06PM +0100, Damien Lespiau wrote: > On Sat, Apr 27, 2013 at 05:59:12PM -0700, Ben Widawsky wrote: > > Semaphores are tied very closely to the rings in the GPU. Trivial patch > > adds comments to the existing code so that when we add new rings we can > > include comments there as well. It also helps distinguish the ring to > > semaphore mailbox interactions by using the ringname in the semaphore > > data structures. > > > > This patch should have no functional impact. > > > > A subset of this patch was: > > Reviewed-by: Chris Wilson > > Signed-off-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++------ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 18 +++++++++--------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > > 3 files changed, 16 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 4d66898..767aa32 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -267,12 +267,12 @@ > > #define MI_SEMAPHORE_UPDATE (1<<21) > > #define MI_SEMAPHORE_COMPARE (1<<20) > > #define MI_SEMAPHORE_REGISTER (1<<18) > > -#define MI_SEMAPHORE_SYNC_RV (2<<16) > > -#define MI_SEMAPHORE_SYNC_RB (0<<16) > > -#define MI_SEMAPHORE_SYNC_VR (0<<16) > > -#define MI_SEMAPHORE_SYNC_VB (2<<16) > > -#define MI_SEMAPHORE_SYNC_BR (2<<16) > > -#define MI_SEMAPHORE_SYNC_BV (0<<16) > > +#define MI_SEMAPHORE_SYNC_RB (0<<16) /* RCS wait for BCS (BRSYNC) */ > > +#define MI_SEMAPHORE_SYNC_RV (2<<16) /* RCS wait for VCS (VRSYNC) */ > > +#define MI_SEMAPHORE_SYNC_VR (0<<16) /* VCS wait for RCS (RVSYNC) */ > > +#define MI_SEMAPHORE_SYNC_VB (2<<16) /* VCS wait for BCS (BVSYNC) */ > > +#define MI_SEMAPHORE_SYNC_BV (0<<16) /* BCS wait for VCS (VBSYNC) */ > > +#define MI_SEMAPHORE_SYNC_BR (2<<16) /* BCS wait for RCS (RBSYNC) */ > > Huum aren't the register names inverted in the comment? RCS waiting for > BCS would be RBSYNC? (Register read by RCS, written by BCS) > > -- > Damien Admittedly this code is quite confusing. So I won't go past, I *think* The current comments are correct. Using the example of the render waiting on the blitter: gen6_ring_sync(render, blitter, seqno) iender emits a wait on: blitter->semaphore_register[RCS] (MI_SEMAPHORE_SYNC_BR) gen6_add_request(blitter) update_mboxes(GEN6_RBSYNC) update_mboxes(GEN6_VBSYNC) So in this case: MI_SEMAPHORE_SYNC_BR <==> GEN6_RBSYNC > > > #define MI_SEMAPHORE_SYNC_INVALID (1<<0) > > /* > > * 3D instructions used by the kernel > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index 1d5d613..38751a7 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -1666,9 +1666,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev) > > ring->get_seqno = gen6_ring_get_seqno; > > ring->set_seqno = ring_set_seqno; > > ring->sync_to = gen6_ring_sync; > > - ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_INVALID; > > - ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_RV; > > - ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_RB; > > + ring->semaphore_register[RCS] = MI_SEMAPHORE_SYNC_INVALID; > > + ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_RV; > > + ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_RB; > > ring->signal_mbox[0] = GEN6_VRSYNC; > > ring->signal_mbox[1] = GEN6_BRSYNC; > > } else if (IS_GEN5(dev)) { > > @@ -1825,9 +1825,9 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev) > > ring->irq_put = gen6_ring_put_irq; > > ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer; > > ring->sync_to = gen6_ring_sync; > > - ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_VR; > > - ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_INVALID; > > - ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_VB; > > + ring->semaphore_register[RCS] = MI_SEMAPHORE_SYNC_VR; > > + ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_INVALID; > > + ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_VB; > > ring->signal_mbox[0] = GEN6_RVSYNC; > > ring->signal_mbox[1] = GEN6_BVSYNC; > > } else { > > @@ -1871,9 +1871,9 @@ 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->sync_to = gen6_ring_sync; > > - ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_BR; > > - ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_BV; > > - ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_INVALID; > > + ring->semaphore_register[RCS] = MI_SEMAPHORE_SYNC_BR; > > + ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_BV; > > + ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_INVALID; > > ring->signal_mbox[0] = GEN6_RBSYNC; > > ring->signal_mbox[1] = GEN6_VBSYNC; > > ring->init = init_ring_common; > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index d66208c..785df13 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -102,7 +102,7 @@ struct intel_ring_buffer { > > struct intel_ring_buffer *to, > > u32 seqno); > > > > - u32 semaphore_register[3]; /*our mbox written by others */ > > + u32 semaphore_register[I915_NUM_RINGS]; /*our mbox written by others */ > > u32 signal_mbox[2]; /* mboxes this ring signals to */ > > /** > > * List of objects currently involved in rendering from the > > -- > > 1.8.2.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center