Re: [Intel-gfx] [v2 3/3] drm/i915/rpl-s: Enable guc submission by default

["Srivatsa, Anusha" <anusha.srivatsa@intel.com>]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Monday, November 22, 2021 3:28 PM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> gfx@lists.freedesktop.org; Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>;
> Syrjala, Ville <ville.syrjala@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>
> Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Dhanavanthri, Swathi
> <swathi.dhanavanthri@intel.com>
> Subject: Re: [v2 3/3] drm/i915/rpl-s: Enable guc submission by default
> 
> On Fri, 19 Nov 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> 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@linux.intel.com>
> > Cc: Swathi Dhanavanthri <swathi.dhanavanthri@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  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