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