Re: [Intel-gfx] [v3] drm/i915/mtl: s/MTL/METEORLAKE for platform/subplatform defines

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

On Thu, 13 Jul 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 13 Jul 2023, "Bhadane, Dnyaneshwar" <dnyaneshwar.bhadane@intel.com> wrote:
>>> -----Original Message-----
>>> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Sent: Thursday, July 13, 2023 5:55 PM
>>> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Jani Nikula
>>> <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org; Ursulin,
>>> Tvrtko <tvrtko.ursulin@intel.com>
>>> Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Shankar, Uma
>>> <uma.shankar@intel.com>
>>> Subject: Re: [Intel-gfx] [v3] drm/i915/mtl: s/MTL/METEORLAKE for
>>> platform/subplatform defines
>>> 
>>> 
>>> On 13/07/2023 13:12, Bhadane, Dnyaneshwar wrote:
>>> >
>>> >
>>> >> -----Original Message-----
>>> >> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> >> Sent: Thursday, July 13, 2023 5:26 PM
>>> >> To: Jani Nikula <jani.nikula@linux.intel.com>; Bhadane, Dnyaneshwar
>>> >> <dnyaneshwar.bhadane@intel.com>; intel-gfx@lists.freedesktop.org;
>>> >> Ursulin, Tvrtko <tvrtko.ursulin@intel.com>
>>> >> Subject: Re: [Intel-gfx] [v3] drm/i915/mtl: s/MTL/METEORLAKE for
>>> >> platform/subplatform defines
>>> >>
>>> >>
>>> >> On 13/07/2023 10:39, Jani Nikula wrote:
>>> >>> On Thu, 13 Jul 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> wrote:
>>> >>>> On 10/07/2023 14:44, Bhadane, Dnyaneshwar wrote:
>>> >>>>>> -----Original Message-----
>>> >>>>>> From: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>
>>> >>>>>> Sent: Monday, July 10, 2023 4:28 PM
>>> >>>>>> To: intel-gfx@lists.freedesktop.org
>>> >>>>>> Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>;
>>> >>>>>> jani.nikula@linux.intel.com; Srivatsa, Anusha
>>> >>>>>> <anusha.srivatsa@intel.com>; Bhadane, Dnyaneshwar
>>> >>>>>> <dnyaneshwar.bhadane@intel.com>
>>> >>>>>> Subject: [v3] drm/i915/mtl: s/MTL/METEORLAKE for
>>> >>>>>> platform/subplatform defines
>>> >>>>>>
>>> >>>>>> Follow consistent naming convention. Replace MTL with
>>> METEORLAKE.
>>> >>>>>> Added defines that are replacing IS_MTL_GRAPHICS_STEP with
>>> >>>>>> IS_METEORLAKE_P_GRAPHICS_STEP and
>>> >> IS_METEORLAKE_M_GRAPHICS_STEP.
>>> >>>>>> Also replaced IS_METEORLAKE_MEDIA_STEP instead of
>>> >> IS_MTL_MEDIA_STEP
>>> >>>>>> and IS_METEORLAKE_DISPLAY_STEP instead of
>>> IS_MTL_DISPLAY_STEP.
>>> >>>>>>
>>> >>>>> Hi Tvrtko,
>>> >>>>> Could you please give the feedback on this ? or suggestion
>>> >>>>> regarding the
>>> >> approach.
>>> >>>>
>>> >>>> It's a step in the right direction I just wish we could do all
>>> >>>> churning in one go.
>>> >>>>
>>> >>>> Have you captured IS_CFL and IS_CML in the series? ICL? HSW? Any
>>> >>>> other I am missing?
>>> >>>>
>>> >>>> What have we concluded on Jani's suggestion to split it all to
>>> >>>> IS_<platform> && IS_<subsys>?
>>> >>>
>>> >>> IS_<platform> && IS_<step> is what I was after.
>>> >>
>>> >> Yeah I mistyped. I liked that to so would get my ack.
>>> >>
>>> >>>> If you have a) captured all IS_<tla> and b) Jani acks the series
>>> >>>> too, I guess go ahead.
>>> >>>>
>>> >>>> Hm.. what have we concluded to do with IS_JASPERLAKE_EHL?
>>> >>>
>>> >>> For sure it can't be *that*. It's JSL *or* EHL. Not subplatform.
>>> >>
>>> >> IS_ELKHARTLAKE would indeed work and platform/subplatform can be
>>> >> hidden implementation detail.
>>> >>
>>> >>>> P.S.
>>> >>>> I still think these suck though:
>>> >>>>
>>> >>>> 	if (IS_METEORLAKE_M_GRAPHICS_STEP(i915, STEP_A0, STEP_B0) ||
>>> >>>> 	    IS_METEORLAKE_P_GRAPHICS_STEP(i915, STEP_A0, STEP_B0))
>>> >>>
>>> >>> I still find it appealing to a) go towards shorter acronyms instead
>>> >>> of long names, and b) to separate platform and stepping checks
>>> >>> because they're orthogonal. They're only bundled together for
>>> >>> historical reasons, and to keep the conditions shorter.
>>> >>>
>>> >>> The above could be:
>>> >>>
>>> >>> 	if (IS_MTL(i915) && IS_GRAPHICS_STEP(i915, STEP_A0, STEP_B0))
>>> >>
>>> >> I'd be super pleased with that.
>>> >
>>> > Could we use the above suggestion for MTL variants for P/M? also
>>> replacing MTL with METEORLAKE.
>>> >
>>> > Using the format:  IS_FULL_PLATFORM_NAME &&
>>> IS_GRAPHICS_STEP(i915, STEP_A0, STEP_B0).
>>> >
>>> > It will change to :
>>> > 	For M:	IS_METEORLAKE_M(i915) && IS_GRAPHICS_STEP(i915,
>>> STEP_A0, STEP_B0)
>>> > 	For P:	IS_METEORLAKE_P(i915) && IS_GRAPHICS_STEP(i915,
>>> STEP_A0, STEP_B0)
>>> 
>>> You could, but you'd only get a meh from me. :) Why you'd insist to keep the
>>> two checks? Are we expecting IS_METEROLAKE_<X> at some point?
>>
>> For example FILE PATH:  drivers/gpu/drm/i915/gt/intel_workarounds.c
>>
>> Multiple occurrences of IS_MTL_GRAPHICS_STEP(i915, M/P, STEP_B0, STEP_FOREVER)
>> Where P and M are passed explicitly. That why we can not check IS_METEORLAKE()
>> as single check.
>> IS_MTL_GRAPHICS_STEP(i915, M, STEP_B0, STEP_FOREVER) ||
>> 	    IS_MTL_GRAPHICS_STEP(i915, P, STEP_B0, STEP_FOREVER))
>>
>> The IS_GRAPHICS_STEP is generic macro and used by other platforms also. 
>> On changing the IS_GRAPHICS_STEP  only  for MTL variants is lead to affect the other 
>> platform. The IS_METEORLAKE_P(i915) or IS_METEORLAKE_M(i915) solves the problem. 
>> to differentiate the MTL platform variant.
>
> I've been trying to say all along that we've abstracted the stepping
> checks, and we no longer need macros that capture *both* the platform
> and the step ranges. They're orthogonal.
>
> If the stepping ranges to check are the same, you don't need to separate
> between MTL subplatforms. They'll both match MTL. They'll both match the
> stepping ranges, and you can use the generic stepping check.
>
> You'll need to do if
> 	((IS_MTL_M() && IS_GRAPHICS_STEP(a,b)) ||
> 	  IS_MTL_P() && IS_GRAPHICS_STEP(c,d))
>
> if a != c && b != d.

There are some cases where only _M or _P is needed, but then you'd use
the relevant subplatform check.

BR,
Jani.

>
>
> BR,
> Jani.
>
>
>
>
>>
>> Regards, 
>> Dnyaneshwar.
>>> 
>>> Regards,
>>> 
>>> Tvrtko

-- 
Jani Nikula, Intel Open Source Graphics Center