On Thu, 13 Jul 2023, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Thu, 13 Jul 2023, "Bhadane, Dnyaneshwar" <dnyaneshwar.bhadane@xxxxxxxxx> wrote: >>> -----Original Message----- >>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> >>> Sent: Thursday, July 13, 2023 5:55 PM >>> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@xxxxxxxxx>; Jani Nikula >>> <jani.nikula@xxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ursulin, >>> Tvrtko <tvrtko.ursulin@xxxxxxxxx> >>> Cc: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Shankar, Uma >>> <uma.shankar@xxxxxxxxx> >>> Subject: Re: [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@xxxxxxxxxxxxxxx> >>> >> Sent: Thursday, July 13, 2023 5:26 PM >>> >> To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>; Bhadane, Dnyaneshwar >>> >> <dnyaneshwar.bhadane@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >>> >> Ursulin, Tvrtko <tvrtko.ursulin@xxxxxxxxx> >>> >> Subject: Re: [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@xxxxxxxxxxxxxxx> >>> wrote: >>> >>>> On 10/07/2023 14:44, Bhadane, Dnyaneshwar wrote: >>> >>>>>> -----Original Message----- >>> >>>>>> From: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@xxxxxxxxx> >>> >>>>>> Sent: Monday, July 10, 2023 4:28 PM >>> >>>>>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> >>>>>> Cc: Ursulin, Tvrtko <tvrtko.ursulin@xxxxxxxxx>; >>> >>>>>> jani.nikula@xxxxxxxxxxxxxxx; Srivatsa, Anusha >>> >>>>>> <anusha.srivatsa@xxxxxxxxx>; Bhadane, Dnyaneshwar >>> >>>>>> <dnyaneshwar.bhadane@xxxxxxxxx> >>> >>>>>> 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
