Re: [PATCH 00/17] Broadwell HW semaphores

[Ben Widawsky <ben@bwidawsk.net>]

On Sat, Dec 14, 2013 at 09:16:51PM +0000, Chris Wilson wrote:
> 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

That's not new to the series.

> 
> #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

Well, the other half is not currently gen specific, and I liked keeping
it that way.


In either case, the fix on top is quite simple. Patches welcome :-)

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center