[Intel-gfx] [PATCH 1/9] drm/i915: Remove HW semaphores for gen7 inter-engine synchronisation

[Chris Wilson]

Quoting Mika Kuoppala (2018-12-28 11:37:32)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > The writing is on the wall for the existence of a single execution queue
> > along each engine, and as a consequence we will not be able to track
> > dependencies along the HW queue itself, i.e. we will not be able to use
> > HW semaphores on gen7 as they use a global set of registers (and unlike
> > gen8+ we can not effectively target memory to keep per-context seqno and
> > dependencies).
> >
> > On the positive side, when we implement request reordering for gen7 we
> > could also not presume a simple execution queue and would also require
> > removing the current semaphore generation code. So this bring us another
> > step closer to request reordering!
> >
> > The negative side is that using interrupts to drive inter-engine
> > synchronisation is much slower (4us -> 15us to do a nop on each of the 3
> > engines on ivb). This is much better than it at the time of introducing
> > the HW semaphores and equally importantly userspace weaned itself of
> 
> s/of/off ?
> 
> > intermixing dependent BLT/RENDER operations (the prime culprit was glyph
> > rendering in UXA). So while we regress the microbenchmarks it should not
> > impact the user.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c     |  19 +--
> >  drivers/gpu/drm/i915/i915_drv.c         |   2 +-
> >  drivers/gpu/drm/i915/i915_drv.h         |   3 -
> >  drivers/gpu/drm/i915/i915_gem.c         |   4 +-
> >  drivers/gpu/drm/i915/i915_request.c     | 126 +---------------
> >  drivers/gpu/drm/i915/i915_timeline.h    |   8 -
> >  drivers/gpu/drm/i915/intel_engine_cs.c  |  29 +---
> >  drivers/gpu/drm/i915/intel_hangcheck.c  | 155 --------------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 187 +-----------------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  56 +------
> >  10 files changed, 15 insertions(+), 574 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 77486a614614..b0bdf3c0d776 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1041,21 +1041,7 @@ static const struct file_operations i915_error_state_fops = {
> >  static int
> >  i915_next_seqno_set(void *data, u64 val)
> >  {
> > -     struct drm_i915_private *dev_priv = data;
> > -     struct drm_device *dev = &dev_priv->drm;
> > -     int ret;
> > -
> > -     ret = mutex_lock_interruptible(&dev->struct_mutex);
> > -     if (ret)
> > -             return ret;
> > -
> > -     intel_runtime_pm_get(dev_priv);
> > -     ret = i915_gem_set_global_seqno(dev, val);
> > -     intel_runtime_pm_put(dev_priv);
> > -
> > -     mutex_unlock(&dev->struct_mutex);
> > -
> > -     return ret;
> > +     return val ? 0 : -EINVAL;
> 
> This begs to meet it's maker soon.

Yeah, I was tempted to just remove the debugfs, but was feeling lazy
enough to leave reviewing igt to a second patch.

The only user I see is gem_exec_whisper who is ignoring errors here. So
looks safe. Safer still to do it afterwards.

> I didn't find anything else in this patch, so looks
> ok and what a clump of compilated code we can get rid off!
> Code that we have dragged along with no small amount of
> suffering. I am in favour.

I predict short-lived relief. The light at the end of this tunnel goes
choo-choo. :|
-Chris