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

[Jani Nikula]

On Tue, 30 Nov 2021, "Srivatsa, Anusha" <anusha.srivatsa at intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula at linux.intel.com>
>> Sent: Monday, November 22, 2021 3:28 PM
>> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
>> gfx at lists.freedesktop.org; Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>;
>> Syrjala, Ville <ville.syrjala at intel.com>; Vivi, Rodrigo
>> <rodrigo.vivi at intel.com>; Joonas Lahtinen
>> <joonas.lahtinen at linux.intel.com>
>> Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; Dhanavanthri, Swathi
>> <swathi.dhanavanthri at 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 at 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 at linux.intel.com>
>> > Cc: Swathi Dhanavanthri <swathi.dhanavanthri at intel.com>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at 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?

Yes, that's the whole point. It is confusing that a check that looks
like a platform check is in fact a sub-platform check.

My primary concern here is what it looks like to the reader, and let's
face it, we have so many platforms and developers that everyone isn't
going to automatically remember RPL-S is a sub-platform of ADL-S.


BR,
Jani.


>
>
> 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

-- 
Jani Nikula, Intel Open Source Graphics Center