[Intel-gfx] [PATCH v2 06/50] drm/i915/xehp: Extra media engines - Part 1 (engine definitions)

[Lucas De Marchi]

On Tue, Jul 20, 2021 at 04:40:52PM -0700, John Harrison wrote:
>On 7/20/2021 16:03, Lucas De Marchi wrote:
>>On Tue, Jul 13, 2021 at 08:14:56PM -0700, Matt Roper wrote:
>>>From: John Harrison <John.C.Harrison at Intel.com>
>>>
>>>Xe_HP can have a lot of extra media engines. This patch adds the basic
>>>definitions for them.
>>>
>>>v2:
>>>- Re-order intel_gt_info and intel_device_info slightly to avoid
>>>  unnecessary padding now that we've increased the size of
>>>  intel_engine_mask_t.  (Tvrtko)
>>>
>>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>>>Signed-off-by: Tomas Winkler <tomas.winkler at intel.com>
>>>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>>---
>>>drivers/gpu/drm/i915/gt/gen8_engine_cs.c     |  7 ++-
>>>drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 50 ++++++++++++++++++++
>>>drivers/gpu/drm/i915/gt/intel_engine_types.h | 14 ++++--
>>>drivers/gpu/drm/i915/gt/intel_gt_types.h     |  5 +-
>>>drivers/gpu/drm/i915/i915_reg.h              |  6 +++
>>>drivers/gpu/drm/i915/intel_device_info.h     |  3 +-
>>>6 files changed, 74 insertions(+), 11 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
>>>b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>>index 87b06572fd2e..35edc55720f4 100644
>>>--- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>>+++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>>@@ -279,7 +279,7 @@ int gen12_emit_flush_xcs(struct i915_request 
>>>*rq, u32 mode)
>>>    if (mode & EMIT_INVALIDATE)
>>>        aux_inv = rq->engine->mask & ~BIT(BCS0);
>>>    if (aux_inv)
>>>-        cmd += 2 * hweight8(aux_inv) + 2;
>>>+        cmd += 2 * hweight32(aux_inv) + 2;
>>>
>>>    cs = intel_ring_begin(rq, cmd);
>>>    if (IS_ERR(cs))
>>>@@ -313,9 +313,8 @@ int gen12_emit_flush_xcs(struct i915_request 
>>>*rq, u32 mode)
>>>        struct intel_engine_cs *engine;
>>>        unsigned int tmp;
>>>
>>>-        *cs++ = MI_LOAD_REGISTER_IMM(hweight8(aux_inv));
>>>-        for_each_engine_masked(engine, rq->engine->gt,
>>>-                       aux_inv, tmp) {
>>>+        *cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv));
>>>+        for_each_engine_masked(engine, rq->engine->gt, aux_inv, tmp) {
>>>            *cs++ = i915_mmio_reg_offset(aux_inv_reg(engine));
>>>            *cs++ = AUX_INV;
>>>        }
>>>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>index 3f8013612a08..6c2cb1400c8c 100644
>>>--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>@@ -104,6 +104,38 @@ static const struct engine_info intel_engines[] = {
>>>            { .graphics_ver = 11, .base = GEN11_BSD4_RING_BASE }
>>>        },
>>>    },
>>>+    [VCS4] = {
>>>+        .hw_id = 0, /* not used in GEN12+, see MI_SEMAPHORE_SIGNAL */
>>
>>I may be misreading this, but hw_id is only used by
>>RING_FAULT_REG() which is not actually used since
>>gen8... they are using GEN8_RING_FAULT_REG().
>>
>>I'm having a hard time to understand what this comment "see
>>MI_SEMAPHORE_SIGNAL" actually means.
>I vaguely recall something about being told the hw_id field was used 
>in semaphore messages from one engine to another. I.e. if engine X is 
>blocked on a semaphore that is signalled by engine Y then the MI_ 
>instruction executed on Y to do the signal needs to specify X as the 
>target. Whereas, on newer hardware this requirement was no longer 
>applicable because MI_SEMAPHORE_SIGNAL uses memory mailboxes instead 
>of directed engine messages. Maybe that information was wrong or maybe 
>that code has since been removed or reworked?
>
>
>>
>>
>>I'd just remove all these `.hw_id = 0, ...` together with the comment
>>since it will be zero-initiliazed.
>Yeah, the reason for explicitly setting it to zero was to avoid 
>confusion over whether it had just been forgotten or not. I.e. to say 
>'we know semaphores used to use this field but honest guv, we didn't 
>forget to add it, it's just that newer hardware doesn't need it'.


makes sense... I just sent a patch series and Cc'ed you all
(https://patchwork.freedesktop.org/series/92797/) actually removing
hw_id. I have that feeling I'm missing something, but we can try to
simplify.

thanks
Lucas De Marchi