[Intel-gfx] [PATCH 00/17] Broadwell HW semaphores

[Chris Wilson]

On Sat, Dec 14, 2013 at 11:39:27AM -0800, Ben Widawsky wrote:
> 
> On Sat, Dec 14, 2013 at 09:08:47AM +0000, Chris Wilson wrote:
> > On Fri, Dec 13, 2013 at 08:15:48PM -0800, Ben Widawsky wrote:
> > > Ben Widawsky (17):
> > >   drm/i915: Reorder/respace MI instruction definition
> > >   drm/i915: Don't emit mbox updates without semaphores
> > >   drm/i915: Move semaphore specific ring members to struct
> > >   drm/i915: Virtualize the ringbuffer signal func
> > >   drm/i915: Move ring_begin to signal()
> > >   drm/i915: Make semaphore updates more precise
> > >   drm/i915: gen specific semaphore info
> > >   drm/i915: Create for_all_rings
> > >   drm/i915: init ring->id early
> > >   drm/i915/bdw: implement semaphore signal
> > >   drm/i915/bdw: implement semaphore wait
> > >   drm/i915: FORCE_RESTORE for gen8 semaphores
> > >   drm/i915/bdw: poll semaphores
> > >   drm/i915: Extract semaphore error collection
> > >   drm/i915/bdw: collect semaphore error state
> > >   drm/i915: unleash semaphores on gen8
> > >   drm/i915: semaphore debugfs
> > 
> > By the end, don't you use a mix of tables and formula for writing the
> > offsets for the wait/signal commands? Looks very inconsistent when there
> > is a very simple routine for generating the appropriate semaphore slot
> > given (waiter, signaller).
> > -Chris
> > 
> 
> Excluding debugfs, which was left intentionally that way...
> 
> The only inconsistency, I think was in the error state capture,
> right? That was a last minute addition probably after I should have quit
> for the day. Do you see something other than error state (and debugfs)?
> I'll discuss the error state stuff with on the patch itself.

The error state and signaller use the table, but the waiter uses the
offset calculation. And debugfs does its own compact thing (which is
ideal for neatly formating the table.).

perhaps

#define GEN8_SEMAPHORE(signaller, waiter) \
(i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
(signaller * I915_NUM_RINGS  + waiter) * SEQNO_SIZE)

#define GEN8_WAIT_OFFSET(from) GEN8_SEMAPHORE(from, ring->id)
#define GEN8_SIGNAL_OFFSET(to) GEN8_SEMAPHORE(ring->id, to)

would make it clearer what I mean.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre