[Intel-gfx] [PATCH 1/2] drm/i915: Store the BIT(engine->id) as the engine's mask

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-05 14:44:04)
> 
> On 02/03/2019 10:06, Chris Wilson wrote:
> > In the next patch, we are introducing a broad virtual engine to encompass
> > multiple physical engines, losing the 1:1 nature of BIT(engine->id). To
> > reflect the broader set of engines implied by the virtual instance, lets
> > store the full bitmask.
> > 
> > v2: Use intel_engine_mask_t (s/ring_mask/engine_mask/)
> > v3: Tvrtko voted for moah churn so teach everyone to not mention ring
> > and use $class$instance throughout.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> > -static void gen6_record_semaphore_state(struct intel_engine_cs *engine,
> > -                                     struct drm_i915_error_engine *ee)
> > -{
> > -     struct drm_i915_private *dev_priv = engine->i915;
> > -
> > -     ee->semaphore_mboxes[0] = I915_READ(RING_SYNC_0(engine->mmio_base));
> > -     ee->semaphore_mboxes[1] = I915_READ(RING_SYNC_1(engine->mmio_base));
> > -     if (HAS_VEBOX(dev_priv))
> > -             ee->semaphore_mboxes[2] =
> > -                     I915_READ(RING_SYNC_2(engine->mmio_base));
> > -}
> > -
> >   static void error_record_engine_registers(struct i915_gpu_state *error,
> >                                         struct intel_engine_cs *engine,
> >                                         struct drm_i915_error_engine *ee)
> > @@ -1161,7 +1142,6 @@ static void error_record_engine_registers(struct i915_gpu_state *error,
> >               if (INTEL_GEN(dev_priv) >= 8) {
> >                       ee->fault_reg = I915_READ(GEN8_RING_FAULT_REG);
> >               } else {
> > -                     gen6_record_semaphore_state(engine, ee);
> 
> Wrong patch?

There wasn't much point updating these as they are defunct, so I removed
them instead.

> >   /* Engine class */
> >   
> > @@ -7250,8 +7250,8 @@ enum {
> >   #define  GEN8_GT_VECS_IRQ           (1 << 6)
> >   #define  GEN8_GT_GUC_IRQ            (1 << 5)
> >   #define  GEN8_GT_PM_IRQ                     (1 << 4)
> > -#define  GEN8_GT_VCS2_IRQ            (1 << 3)
> > -#define  GEN8_GT_VCS1_IRQ            (1 << 2)
> > +#define  GEN8_GT_VCS1_IRQ            (1 << 3)
> > +#define  GEN8_GT_VCS0_IRQ            (1 << 2)
> 
> BSpec uses index 1. :( So I think best to leave these alone.

That does get confusing. So we either have confusing code
with VCS1 -> VCS0 and VCS2 -> VCS1, or just live with the difference
with bspec. Code wins (because I read that more often), but we can
comment that "old" bspec refers to these as 1-indexed?

[snip]

> I wasn't suggesting renaming the engine id enums, but on the other hand 
> it is nicer to have VCS0, VCS1,.. instead of VCS, VCS2, VCS3,...
> 
> So ack from me. And since I read it all almost r-b if not the unrelated 
> error state hunk and more importantly reservations about bit definition 
> names from i915_reg.h.

I'll go drop another couple of patches for the grumbles.
-Chris