Re: [Intel-gfx] [PATCH 11/11] drm/i915/pvc: read fuses for link copy engines

[Tvrtko Ursulin]

On 02/05/2022 17:34, Matt Roper wrote:

From: Lucas De Marchi <lucas.demar...@intel.com>

The new Link Copy engines in PVC may be fused off according to the
mslice_mask. Each bit of the MEML3_EN_MASK we read from the
GEN10_MIRROR_FUSE3 register disables a pair of link copy engines.

Bspec: 44483
Cc: Matt Roper <matthew.d.ro...@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demar...@intel.com>
Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
---
  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 28 +++++++++++++++++++++++
  1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c6e93db134b1..d10cdeff5072 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -686,6 +686,33 @@ static void engine_mask_apply_compute_fuses(struct 
intel_gt *gt)
        }
  }

+static void engine_mask_apply_copy_fuses(struct intel_gt *gt)

+{
+       struct drm_i915_private *i915 = gt->i915;
+       struct intel_gt_info *info = &gt->info;
+       unsigned long meml3_mask;
+       u8 quad;

Any hidden reason u8 is the right type here and not unsigned long like bitops 
expect? (Yes I did notice GEN12_MAX_MSLICES only goes to 4 but generally u8 
sucks.)

+
+       meml3_mask = intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3);
+       meml3_mask = REG_FIELD_GET(GEN12_MEML3_EN_MASK, meml3_mask);
+
+       /*
+        * Link Copy engines may be fused off according to meml3_mask. Each
+        * bit is a quad that houses 2 Link Copy and two Sub Copy engines.
+        */
+       for_each_clear_bit(quad, &meml3_mask, GEN12_MAX_MSLICES) {
+               intel_engine_mask_t mask = GENMASK(BCS1 + quad * 2 + 1,
+                                                  BCS1 + quad * 2);

So internally we will be sure BCS1 to BCS9 are link copy engines? I mean enum 
names hardcoded/fixed to function. Should we have a comment to that effect 
somewhere? In intel_engine_types.h maybe?

+
+               if (mask & info->engine_mask) {
+                       drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 1);
+                       drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 2);

Bikeshed - I'd be tempted to decrease the amount of "quad * 2 + 1" by having a 
local variable.

  unsigned int instance = quad * 2 + 1;
  intel_engine_mask_t mask = GENMASK(_BCS(instance + 1), _BCS(instance));

Etc.

Regards,

Tvrtko

+
+                       info->engine_mask &= ~mask;
+               }
+       }
+}
+
  /*
   * Determine which engines are fused off in our particular hardware.
   * Note that we have a catch-22 situation where we need to be able to access
@@ -768,6 +795,7 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt 
*gt)
        GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));

engine_mask_apply_compute_fuses(gt);

+       engine_mask_apply_copy_fuses(gt);

return info->engine_mask;

  }