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