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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

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.

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