Quoting Gustavo Sousa (2023-08-21 11:21:55-03:00)
>Quoting Matt Roper (2023-08-14 17:06:38-03:00)
>>Stepping-specific media behavior shouldn't be tied to MTL as a platform,
>>but rather specifically to the Xe_LPM+ IP. Future non-MTL platforms may
>>re-use this IP and will need to follow the exact same logic and apply
>>the same workarounds. IS_MTL_MEDIA_STEP() is dropped in favor of
>>IS_MEDIA_GT_IP_STEP, which checks the media IP version associated with a
>>specific IP and also ensures that we're operating on the media GT, not
>>the primary GT.
>>
>>v2:
>> - Switch to the IS_GT_IP_STEP macro.
>>v3:
>> - Switch back to long-form IS_MEDIA_GT_IP_STEP. (Jani)
>>v4:
>> - Build IS_MEDIA_GT_IP_STEP on top of IS_MEDIA_GT_IP_RANGE and
>> IS_MEDIA_STEP building blocks and name the parameters from/until
>> rather than begin/fixed.. (Jani)
>>
>>Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>>Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>---
>> drivers/gpu/drm/i915/gt/intel_gt.h | 30 +++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/gt/intel_rc6.c | 3 +--
>> drivers/gpu/drm/i915/i915_drv.h | 4 ----
>> drivers/gpu/drm/i915/i915_perf.c | 15 ++++-----------
>> 4 files changed, 35 insertions(+), 17 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
>>index 636fd651b8b2..abe9fe4a3bd9 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_gt.h
>>+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
>>@@ -25,6 +25,20 @@ struct drm_printer;
>> GRAPHICS_VER_FULL((gt)->i915) >= (from) && \
>> GRAPHICS_VER_FULL((gt)->i915) <= (until)))
>>
>>+/*
>>+ * Check that the GT is a media GT and has an IP version within the
>>+ * specified range (inclusive).
>>+ *
>>+ * Only usable on platforms with a standalone media design (i.e., IP version 13
>>+ * and higher).
>>+ */
>>+#define IS_MEDIA_GT_IP_RANGE(gt, from, until) ( \
>>+ BUILD_BUG_ON_ZERO((from) < IP_VER(13, 0)) + \
>>+ BUILD_BUG_ON_ZERO((until) < (from)) + \
>>+ ((gt)->type == GT_MEDIA && \
>>+ MEDIA_VER_FULL((gt)->i915) >= (from) && \
>>+ MEDIA_VER_FULL((gt)->i915) <= (until)))
>>+
>> /*
>> * Check that the GT is a graphics GT with a specific IP version and has
>> * a stepping in the range [begin, fixed). The lower stepping bound is
>>@@ -42,6 +56,22 @@ struct drm_printer;
>> (IS_GFX_GT_IP_RANGE((gt), (ipver), (ipver)) && \
>> IS_GRAPHICS_STEP((gt)->i915, (from), (until))))
>>
>>+/*
>>+ * Check that the GT is a media GT with a specific IP version and has
>>+ * a stepping in the range [begin, fixed). The lower stepping bound is
>>+ * inclusive, the upper bound is exclusive (corresponding to the first hardware
>>+ * stepping at which the workaround is no longer needed).
>>+ * "STEP_FOREVER" can be passed as the upper stepping bound for workarounds
>>+ * that have no "fixed" version for the specified IP version.
>
>I think the same comments from patch #4 apply here as well.
>
>>+ *
>>+ * This macro may only be used to match on platforms that have a standalone
>>+ * media design (i.e., media version 13 or higher).
>>+ */
>>+#define IS_MEDIA_GT_IP_STEP(gt, ipver, from, until) ( \
>>+ BUILD_BUG_ON_ZERO((until) <= (from)) + \
>>+ (IS_MEDIA_GT_IP_RANGE((gt), (ipver), (ipver)) && \
>>+ IS_MEDIA_STEP((gt)->i915, (from), (until))))
>>+
>> #define GT_TRACE(gt, fmt, ...) do { \
>> const struct intel_gt *gt__ __maybe_unused = (gt); \
>> GEM_TRACE("%s " fmt, dev_name(gt__->i915->drm.dev), \
>>diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>index 58bb1c55294c..748b0c695072 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>>+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>@@ -526,8 +526,7 @@ static bool rc6_supported(struct intel_rc6 *rc6)
>> return false;
>> }
>>
>>- if (IS_MTL_MEDIA_STEP(gt->i915, STEP_A0, STEP_B0) &&
>>- gt->type == GT_MEDIA) {
>>+ if (IS_MEDIA_GT_IP_STEP(gt, IP_VER(13, 0), STEP_A0, STEP_B0)) {
>> drm_notice(&i915->drm,
>> "Media RC6 disabled on A step\n");
>> return false;
>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>index e0e0493d6c1f..42a86483c694 100644
>>--- a/drivers/gpu/drm/i915/i915_drv.h
>>+++ b/drivers/gpu/drm/i915/i915_drv.h
>>@@ -662,10 +662,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>> (IS_METEORLAKE(__i915) && \
>> IS_DISPLAY_STEP(__i915, since, until))
>>
>>-#define IS_MTL_MEDIA_STEP(__i915, since, until) \
>>- (IS_METEORLAKE(__i915) && \
>>- IS_MEDIA_STEP(__i915, since, until))
>>-
>> /*
>> * DG2 hardware steppings are a bit unusual. The hardware design was forked to
>> * create three variants (G10, G11, and G12) which each have distinct
>>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>index 04bc1f4a1115..2ef8addb0cfd 100644
>>--- a/drivers/gpu/drm/i915/i915_perf.c
>>+++ b/drivers/gpu/drm/i915/i915_perf.c
>>@@ -4223,7 +4223,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
>> * C6 disable in BIOS. Fail if Media C6 is enabled on steppings where OAM
>> * does not work as expected.
>> */
>>- if (IS_MTL_MEDIA_STEP(props->engine->i915, STEP_A0, STEP_C0) &&
>>+ if (IS_MEDIA_GT_IP_STEP(props->engine->gt, IP_VER(13, 0), STEP_A0, STEP_C0) &&
>> props->engine->oa_group->type == TYPE_OAM &&
>> intel_check_bios_c6_setup(&props->engine->gt->rc6)) {
>> drm_dbg(&perf->i915->drm,
>>@@ -5332,16 +5332,9 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915)
>> * C6 disable in BIOS. If Media C6 is enabled in BIOS, return version 6
>> * to indicate that OA media is not supported.
>> */
>>- if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) {
>>- struct intel_gt *gt;
>>- int i;
>>-
>>- for_each_gt(gt, i915, i) {
>>- if (gt->type == GT_MEDIA &&
>>- intel_check_bios_c6_setup(>->rc6))
>>- return 6;
>>- }
>>- }
>>+ if (IS_MEDIA_GT_IP_STEP(i915->media_gt, IP_VER(13, 0), STEP_A0, STEP_C0) &&
>
>Hm... i915->media_gt could be NULL, right? We should probably add a NULL check in
>IS_MEDIA_GT_IP_STEP().
Actually, in IS_MEDIA_GT_IP_RANGE().
>
>>+ intel_check_bios_c6_setup(&i915->media_gt->rc6))
>>+ return 6;
>>
>> return 7;
>> }
>>--
>>2.41.0
>>
