Re: [Intel-gfx] [PATCH v2 4/9] drm/i915: Eliminate IS_MTL_GRAPHICS_STEP

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 24 Jul 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> +/*
> + * Check whether a GT contains the specific IP version and a stepping within
> + * the specified range [from, until).  The lower stepping bound is inclusive,
> + * the upper bound is exclusive (corresponding to the first hardware stepping
> + * at when the workaround is no longer needed).  E.g.,
> + *
> + *    IS_GT_IP_STEP(GFX, IP_VER(12, 70), A0, B0)
> + *    IS_GT_IP_STEP(MEDIA, IP_VER(13, 00), B1, D0)
> + *    IS_GT_IP_STEP(GFX, IP_VER(12, 71), B1, FOREVER)
> + *
> + * "FOREVER" can be passed as the upper stepping bound for workarounds that
> + * have no upper bound on steppings of the specified IP version.
> + *
> + * Note that media version checks with this macro will only work on platforms
> + * with standalone media design (i.e., media version 13 and higher).
> + */
> +#define IS_GT_IP_STEP(type, gt, ipver, since, until) \
> +	(BUILD_BUG_ON_ZERO(ipver < IP_VER(2, 0)) + \
> +	 (__IS_##type##_GT(gt) && \
> +	  __GT_VER_FULL(gt) == ipver && \
> +	  __GT_STEP(gt) >= STEP_##since && \
> +	  __GT_STEP(gt) <= STEP_##until))
> +

I really dislike the type, since and until arguments here, passing in
something that becomes part of the name of another macro. That's
something we do in some of our macro spaghetti internally, but not as
part of an interface that gets called all over the place.

For function-like macros, I'd generally like to make them such that they
could be converted to regular (static inline) functions without changing
the call sites.

BR,
Jani.


>  #define MEDIA_VER(i915)			(RUNTIME_INFO(i915)->media.ip.ver)
>  #define MEDIA_VER_FULL(i915)		IP_VER(RUNTIME_INFO(i915)->media.ip.ver, \
>  					       RUNTIME_INFO(i915)->media.ip.rel)
> @@ -710,10 +736,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define IS_XEHPSDV_GRAPHICS_STEP(__i915, since, until) \
>  	(IS_XEHPSDV(__i915) && IS_GRAPHICS_STEP(__i915, since, until))
>  
> -#define IS_MTL_GRAPHICS_STEP(__i915, variant, since, until) \
> -	(IS_SUBPLATFORM(__i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_##variant) && \
> -	 IS_GRAPHICS_STEP(__i915, since, until))
> -
>  #define IS_MTL_DISPLAY_STEP(__i915, since, until) \
>  	(IS_METEORLAKE(__i915) && \
>  	 IS_DISPLAY_STEP(__i915, since, until))

-- 
Jani Nikula, Intel Open Source Graphics Center