Re: [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.harri...@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.ursu...@intel.com>
Signed-off-by: John Harrison <john.c.harri...@intel.com>
Signed-off-by: Tomas Winkler <tomas.wink...@intel.com>
Signed-off-by: Matt Roper <matthew.d.ro...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx