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

[Bhadane, Dnyaneshwar]

> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Sent: Thursday, July 13, 2023 5:55 PM
> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane at intel.com>; Jani Nikula
> <jani.nikula at linux.intel.com>; intel-gfx at lists.freedesktop.org; Ursulin,
> Tvrtko <tvrtko.ursulin at intel.com>
> Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; Shankar, Uma
> <uma.shankar at 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 at linux.intel.com>
> >> Sent: Thursday, July 13, 2023 5:26 PM
> >> To: Jani Nikula <jani.nikula at linux.intel.com>; Bhadane, Dnyaneshwar
> >> <dnyaneshwar.bhadane at intel.com>; intel-gfx at lists.freedesktop.org;
> >> Ursulin, Tvrtko <tvrtko.ursulin at 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 at linux.intel.com>
> wrote:
> >>>> On 10/07/2023 14:44, Bhadane, Dnyaneshwar wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane at intel.com>
> >>>>>> Sent: Monday, July 10, 2023 4:28 PM
> >>>>>> To: intel-gfx at lists.freedesktop.org
> >>>>>> Cc: Ursulin, Tvrtko <tvrtko.ursulin at intel.com>;
> >>>>>> jani.nikula at linux.intel.com; Srivatsa, Anusha
> >>>>>> <anusha.srivatsa at intel.com>; Bhadane, Dnyaneshwar
> >>>>>> <dnyaneshwar.bhadane at 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.

Regards, 
Dnyaneshwar.
> 
> Regards,
> 
> Tvrtko