Hi Chris, for some reason I didn't receive the review email, so I copied your comments from patchwork and faked this email.

...

static void execlists_dequeue(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists; @@ -1538,6 +1566,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine) }

                    if (__i915_request_submit(rq)) {

•                           /* hsdes: 1809175790 */
  

•                           if ((GRAPHICS_VER(engine->i915) == 12) &&
  

•                               rq->vd_ve_aux_inv &&
  

•                               (engine->class == VIDEO_DECODE_CLASS ||
  

•                                engine->class == VIDEO_ENHANCEMENT_CLASS)) {
  

We do not need the extra checks here; we just do as we are told. We only tell ourselves to apply the fixup when required.

Without checking GRAPHICS_VER, I'm seeing a lot of regressions on older platforms in the CI result. This workaround was only implemented for gen12 (gen12_emit_flush_xcs). Without checking engine->class, I'm seeing boot issues due to GEM_BUG_ON() in aux_inv_reg(). Any rq will go through this code regardless of engine class and gen version, so the checking seems to be necessary.

...

•                                   *rq->vd_ve_aux_inv = i915_mmio_reg_offset
  

Likewise, vd_ve is overspecific, aux_inv_fixup or aux_inv_wa (or wa_aux_iv, fixup_aux_inv).

I wanted to be specific because the workaround was only implemented for vd/ve engines. But I'm ok with your proposal.

...

•                                           (aux_inv_reg(engine));
  

•                                   rq->vd_ve_aux_inv = NULL;
  

Move this to i915_request initialisation so that we only set aux_inv when required, which probably explains the extra defence.

The pointer is currently initialized with 0x5a5a. I set it to NULL in gen12_emit_flush_xcs, otherwise the rq will enter that if-statement with an invalid pointer. I'm not familiar with the code, there seems to be multiple functions allocating the structure. I agree it's better to set it to NULL at initialization, but need some guidance on where is the best place to do so.

...

•                                   rq->execution_mask = engine->mask;
  

•                           }
                            if (!merge) {
                                    *port++ = i915_request_get(last);
                                    last = NULL;
  

diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 28b1f9db5487..69de32e5e15d 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -350,6 +350,8 @@ struct i915_request { struct list_head link; unsigned long delay; } mock;)

•   u32 *vd_ve_aux_inv;
  

Not at the end of the struct; that's where we put things in the dungeon. The selftest struct should be last; I do hope no one has been putting things at random places in the struct without considering the layout and semantics. From the flow, this is akin to batch, capture_list; before emitted_jiffies would be a good spot.

Got it, will change. I thought adding at the end would be safer, thanks for the explanation.

-Chris