| Message ID | 20230814200632.56105-16-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce MTL-specific platform checks | expand |
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@linux.intel.com> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >--- > 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(). >+ intel_check_bios_c6_setup(&i915->media_gt->rc6)) >+ return 6; > > return 7; > } >-- >2.41.0 >
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@linux.intel.com> >>Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >>--- >> 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 >>
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. + * + * 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) && + intel_check_bios_c6_setup(&i915->media_gt->rc6)) + return 6; return 7; }
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@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- 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(-)