> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Monday, November 22, 2021 3:28 PM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>;
> Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Vivi, Rodrigo
> <rodrigo.vivi@xxxxxxxxx>; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Dhanavanthri, Swathi
> <swathi.dhanavanthri@xxxxxxxxx>
> Subject: Re: [v2 3/3] drm/i915/rpl-s: Enable guc submission by default
>
> On Fri, 19 Nov 2021, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> wrote:
> > Though, RPL-S is defined as subplatform of ADL-S, unlike ADL-S, it has
> > GuC submission by default.
> >
> > v2: Remove extra parenthesis (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Swathi Dhanavanthri <swathi.dhanavanthri@xxxxxxxxx>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > index 2fef3b0bbe95..6aa843a1c25f 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > @@ -35,7 +35,7 @@ static void uc_expand_default_options(struct intel_uc
> *uc)
> > }
> >
> > /* Intermediate platforms are HuC authentication only */
> > - if (IS_ALDERLAKE_S(i915)) {
> > + if (IS_ALDERLAKE_S(i915) && !IS_RAPTORLAKE_S(i915)) {
>
> I know I looked through the previous version, but I only realized this now.
> The above just feels wrong. Like, if it's ADL-S it obviously can't be RPL-S, so
> why the check.
>
> We've had this type of thing before when IS_VALLEYVIEW() used to mean
> VLV || CHV, and you'd have these really confusing checks:
>
> if (IS_VALLEYVIEW() && !IS_CHERRYVIEW())
>
> We had to change that later on, and it was pretty annoying.
>
> I'm really sorry I didn't spot this before, but I firmly believe adding a platform
> check macro IS_RAPTORLAKE_S() as a subplatform check is the wrong thing
> to do.
>
> I think there are maybe three options:
>
> 1) Add RPL-S as a full blown platform of its own. Convert
> IS_ALDERLAKE_S() checks to IS_ALDERLAKE_S() || IS_RAPTORLAKE_S(). If
> we think there's going to be more differences than just the guc
> submission, this is the way to go.
No. there is nothing else different between the 2 platforms.
> 2) Add RPL-S as a subplatform of ADL-S like here, but then don't add a
> platform macro IS_RAPTORLAKE_S(). Make the check something that
> conveys the subplatform idea. See all the users of IS_SUBPLATFORM()
> in i915_drv.h; for example IS_DG2_G10(). It's obvious it's a DG2 but
> subtype G10. So maybe IS_ADLS_RPLS(), I don't know.
I am trying to understand what this will serve. The above check will change from
(IS_ALDERLAKE_S(i915) && !IS_RAPTORLAKE_S(i915) to (IS_ALDERLAKE_S(i915) && !IS_ADLS_RPLS(i915). Agreed it will make the fact that RPLS is subplatform of ADLS a lot clear. Is that what you are suggesting?
Anusha
> 3) Add RPL-S PCI IDs as ADL-S with separate device info, but add a
> feature flag for the guc submission default. Then RPL-S does not
> exist as a platform or subplatform in code, rather as ADL-S, but the
> difference is recorded via flags.
>
>
> BR,
> Jani.
>
>
>
>
> > i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
> > return;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
