On Mon, Aug 05, 2024 at 02:22:13PM -0700, Daniele Ceraolo Spurio wrote:
>
>
> On 8/1/2024 8:10 PM, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > ARL and MTL share a single GSC firmware blob. However, ARL requires a
> > newer version of it.
> >
> > So add differentiate of the PCI ids for ARL from MTL and create ARL as
> > a sub-platform of MTL. That way, all the existing workarounds and such
> > still treat ARL as MTL exactly as before. However, now the GSC code
> > can check for ARL and do an extra version check on the firmware before
> > committing to it.
> >
> > Also, the version extraction code has various ways of failing but the
> > return code was being ignore and so the firmware load would attempt to
> > continue anyway. Fix that by propagating the return code to the next
> > level out.
> >
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> This needs a fixes tag. AFAICS we already had the ARL-S pci ID in the kernel
> by the time we removed the MTL force probe, so the best fix tag is probably:
>
> Fixes: 213c43676beb ("drm/i915/mtl: Remove the 'force_probe' requirement for
> Meteor Lake")
yes, we do need this Fixes tag.
Please let's also ensure we propagate that to stable.
Cc: stable@xxxxxxxxxxxxxxx # v6.7+
I even wondered if we should have the removal of the pci id in a separate
patch that is easily propagated. So, please check this option.
I mean, if this applies cleanly to 6.7 or if it is easy to backport it
is okay a single patch..
>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 31 +++++++++++++++++++++++
> > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 10 ++++++--
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_device_info.c | 7 +++++
> > drivers/gpu/drm/i915/intel_device_info.h | 3 +++
> > include/drm/intel/i915_pciids.h | 11 +++++---
> > 6 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > index 3b69bc6616bd3..551b0d7974ff1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > @@ -212,6 +212,37 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
> > }
> > }
> > + if (IS_ARROWLAKE(gt->i915)) {
> > + bool too_old = false;
> > +
> > + /*
> > + * ARL requires a newer firmware than MTL did (102.0.10.1878) but the
> > + * firmware is actually common. So, need to do an explicit version check
> > + * here rather than using a separate table entry. And if the older
> > + * MTL-only version is found, then just don't use GSC rather than aborting
> > + * the driver load.
> > + */
> > + if (gsc->release.major < 102) {
> > + too_old = true;
>
> nit: the 102 major number just indicates that it is a MTL/ARL image, so
> unless the binary is corrupted (which should be caught by the checks higher
> up this function) it should be guaranteed that this matches. You could
> probably skip checking for it (below as well).
>
> Apart from this nit the patch LGTM, so with the fixes tag added:
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> However, given that this is a non-backward compatible change that we'd be
> propagating as a fix, please get a maintainer ack as well. IMO there should
> be no problem since this is only breaking for ARL and that platform hasn't
> been publicly released yet, but I'd still prefer a maintainer to confirm.
We really need to be careful with this for the next platforms.
If we have this risk we need to start splitting better the PCI IDs
into the different platforms and removing force_probe individually.
But for now, let's move with this.
Not only because the platforms is not at the shelves yet, but because
it is GSC, not GuC and because it looks like the platform would
be broken anyway with new firmware. So it is not a regression, but
a fix really needed for the older platforms.
Acked-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> Daniele
>
> > + } else if (gsc->release.major == 102) {
> > + if (gsc->release.minor == 0) {
> > + if (gsc->release.patch < 10) {
> > + too_old = true;
> > + } else if (gsc->release.patch == 10) {
> > + if (gsc->release.build < 1878)
> > + too_old = true;
> > + }
> > + }
> > + }
> > +
> > + if (too_old) {
> > + gt_info(gt, "GSC firmware too old for ARL, got %d.%d.%d.%d but need at least 102.0.10.1878",
> > + gsc->release.major, gsc->release.minor,
> > + gsc->release.patch, gsc->release.build);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > index d80278eb45d73..ec33ad942115a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > @@ -698,12 +698,18 @@ static int check_gsc_manifest(struct intel_gt *gt,
> > const struct firmware *fw,
> > struct intel_uc_fw *uc_fw)
> > {
> > + int ret;
> > +
> > switch (uc_fw->type) {
> > case INTEL_UC_FW_TYPE_HUC:
> > - intel_huc_fw_get_binary_info(uc_fw, fw->data, fw->size);
> > + ret = intel_huc_fw_get_binary_info(uc_fw, fw->data, fw->size);
> > + if (ret)
> > + return ret;
> > break;
> > case INTEL_UC_FW_TYPE_GSC:
> > - intel_gsc_fw_get_binary_info(uc_fw, fw->data, fw->size);
> > + ret = intel_gsc_fw_get_binary_info(uc_fw, fw->data, fw->size);
> > + if (ret)
> > + return ret;
> > break;
> > default:
> > MISSING_CASE(uc_fw->type);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 02f28a6170c39..17561b53648e2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -546,6 +546,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > #define IS_LUNARLAKE(i915) (0 && i915)
> > #define IS_BATTLEMAGE(i915) (0 && i915)
> > +#define IS_ARROWLAKE(i915) \
> > + IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_ARL)
> > #define IS_DG2_G10(i915) \
> > IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G10)
> > #define IS_DG2_G11(i915) \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index d26de37719a72..eede5417cb3fe 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -203,6 +203,10 @@ static const u16 subplatform_g12_ids[] = {
> > INTEL_DG2_G12_IDS(ID),
> > };
> > +static const u16 subplatform_arl_ids[] = {
> > + INTEL_ARL_IDS(ID),
> > +};
> > +
> > static bool find_devid(u16 id, const u16 *p, unsigned int num)
> > {
> > for (; num; num--, p++) {
> > @@ -260,6 +264,9 @@ static void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> > } else if (find_devid(devid, subplatform_g12_ids,
> > ARRAY_SIZE(subplatform_g12_ids))) {
> > mask = BIT(INTEL_SUBPLATFORM_G12);
> > + } else if (find_devid(devid, subplatform_arl_ids,
> > + ARRAY_SIZE(subplatform_arl_ids))) {
> > + mask = BIT(INTEL_SUBPLATFORM_ARL);
> > }
> > GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_MASK);
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> > index d1a2abc7e5139..df73ef94615dd 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -127,6 +127,9 @@ enum intel_platform {
> > #define INTEL_SUBPLATFORM_N 1
> > #define INTEL_SUBPLATFORM_RPLU 2
> > +/* MTL */
> > +#define INTEL_SUBPLATFORM_ARL 0
> > +
> > enum intel_ppgtt_type {
> > INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
> > INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
> > diff --git a/include/drm/intel/i915_pciids.h b/include/drm/intel/i915_pciids.h
> > index b21374f76df23..2bf03ebfcf73d 100644
> > --- a/include/drm/intel/i915_pciids.h
> > +++ b/include/drm/intel/i915_pciids.h
> > @@ -772,15 +772,18 @@
> > INTEL_ATS_M75_IDS(MACRO__, ## __VA_ARGS__)
> > /* MTL */
> > +#define INTEL_ARL_IDS(MACRO__, ...) \
> > + MACRO__(0x7D41, ## __VA_ARGS__), \
> > + MACRO__(0x7D51, ## __VA_ARGS__), \
> > + MACRO__(0x7D67, ## __VA_ARGS__), \
> > + MACRO__(0x7DD1, ## __VA_ARGS__)
> > +
> > #define INTEL_MTL_IDS(MACRO__, ...) \
> > + INTEL_ARL_IDS(MACRO__, ## __VA_ARGS__), \
> > MACRO__(0x7D40, ## __VA_ARGS__), \
> > - MACRO__(0x7D41, ## __VA_ARGS__), \
> > MACRO__(0x7D45, ## __VA_ARGS__), \
> > - MACRO__(0x7D51, ## __VA_ARGS__), \
> > MACRO__(0x7D55, ## __VA_ARGS__), \
> > MACRO__(0x7D60, ## __VA_ARGS__), \
> > - MACRO__(0x7D67, ## __VA_ARGS__), \
> > - MACRO__(0x7DD1, ## __VA_ARGS__), \
> > MACRO__(0x7DD5, ## __VA_ARGS__)
> > /* LNL */
>
