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, "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.


BR,
Jani.




>
> Regards, 
> Dnyaneshwar.
>> 
>> Regards,
>> 
>> Tvrtko

-- 
Jani Nikula, Intel Open Source Graphics Center