From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: <intel-gfx@lists.freedesktop.org>,
John Harrison <John.C.Harrison@intel.com>
Subject: Re: [PATCH] drm/i915/gsc: ARL-H and ARL-U need a newer GSC FW.
Date: Tue, 29 Oct 2024 09:36:02 -0400 [thread overview]
Message-ID: <ZyDkwilYgDTWwr2F@intel.com> (raw)
In-Reply-To: <20241028233132.149745-1-daniele.ceraolospurio@intel.com>
On Mon, Oct 28, 2024 at 04:31:32PM -0700, Daniele Ceraolo Spurio wrote:
> All MTL and ARL SKUs share the same GSC FW, but the newer platforms are
> only supported in newer blobs. In particular, ARL-S is supported
> starting from 102.0.10.1878 (which is already the minimum required
> version for ARL in the code), while ARL-H and ARL-U are supported from
> 102.1.15.1926. Therefore, the driver needs to check which specific ARL
> subplatform its running on when verifying that the GSC FW is new enough
> for it.
>
> Fixes: 2955ae8186c8 ("drm/i915: ARL requires a newer GSC firmware")
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 50 +++++++++++++++--------
> drivers/gpu/drm/i915/i915_drv.h | 8 +++-
> drivers/gpu/drm/i915/intel_device_info.c | 24 ++++++++---
> drivers/gpu/drm/i915/intel_device_info.h | 4 +-
> include/drm/intel/i915_pciids.h | 15 +++++--
> 5 files changed, 72 insertions(+), 29 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 551b0d7974ff..5dc0ccd07636 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -80,6 +80,7 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
> const struct intel_gsc_cpd_header_v2 *cpd_header = NULL;
> const struct intel_gsc_cpd_entry *cpd_entry = NULL;
> const struct intel_gsc_manifest_header *manifest;
> + struct intel_uc_fw_ver min_ver = { 0 };
> size_t min_size = sizeof(*layout);
> int i;
>
> @@ -212,33 +213,46 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
> }
> }
>
> - if (IS_ARROWLAKE(gt->i915)) {
> + /*
> + * ARL SKUs require newer firmwares, but the blob is actually common
> + * across all MTL and ARL SKUs, so we need to do an explicit version check
> + * here rather than using a separate table entry. If a too old version
> + * is found, then just don't use GSC rather than aborting the driver load.
> + * Note that the major number in the GSC FW version is used to indicate
> + * the platform, so we expect it to always be 102 for MTL/ARL binaries.
> + */
> + if (IS_ARROWLAKE_S(gt->i915))
> + min_ver = (struct intel_uc_fw_ver){ 102, 0, 10, 1878 };
> + else if (IS_ARROWLAKE_H(gt->i915) || IS_ARROWLAKE_U(gt->i915))
> + min_ver = (struct intel_uc_fw_ver){ 102, 1, 15, 1926 };
I haven't verified the version number specifically, for that I trust you to double check.
We need this patch. Please help me to remember to propagate to stable branches once this reaches
Linus tree.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> +
> + if (IS_METEORLAKE(gt->i915) && gsc->release.major != 102) {
> + gt_info(gt, "Invalid GSC firmware for MTL/ARL, got %d.%d.%d.%d but need 102.x.x.x",
> + gsc->release.major, gsc->release.minor,
> + gsc->release.patch, gsc->release.build);
> + return -EINVAL;
> + }
> +
> + if (min_ver.major) {
> 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) {
> + if (gsc->release.minor < min_ver.minor) {
> too_old = true;
> - } else if (gsc->release.major == 102) {
> - if (gsc->release.minor == 0) {
> - if (gsc->release.patch < 10) {
> + } else if (gsc->release.minor == min_ver.minor) {
> + if (gsc->release.patch < min_ver.patch) {
> + too_old = true;
> + } else if (gsc->release.patch == min_ver.patch) {
> + if (gsc->release.build < min_ver.build)
> 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",
> + gt_info(gt, "GSC firmware too old for ARL, got %d.%d.%d.%d but need at least %d.%d.%d.%d",
> gsc->release.major, gsc->release.minor,
> - gsc->release.patch, gsc->release.build);
> + gsc->release.patch, gsc->release.build,
> + min_ver.major, min_ver.minor,
> + min_ver.patch, min_ver.build);
> return -EINVAL;
> }
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a66e5bb078cf..b1f2183aa156 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -539,8 +539,12 @@ 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_ARROWLAKE_H(i915) \
> + IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_ARL_H)
> +#define IS_ARROWLAKE_U(i915) \
> + IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_ARL_U)
> +#define IS_ARROWLAKE_S(i915) \
> + IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_ARL_S)
> #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 3c47c625993e..467999249b9a 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -200,8 +200,16 @@ static const u16 subplatform_g12_ids[] = {
> INTEL_DG2_G12_IDS(ID),
> };
>
> -static const u16 subplatform_arl_ids[] = {
> - INTEL_ARL_IDS(ID),
> +static const u16 subplatform_arl_h_ids[] = {
> + INTEL_ARL_H_IDS(ID),
> +};
> +
> +static const u16 subplatform_arl_u_ids[] = {
> + INTEL_ARL_U_IDS(ID),
> +};
> +
> +static const u16 subplatform_arl_s_ids[] = {
> + INTEL_ARL_S_IDS(ID),
> };
>
> static bool find_devid(u16 id, const u16 *p, unsigned int num)
> @@ -261,9 +269,15 @@ 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);
> + } else if (find_devid(devid, subplatform_arl_h_ids,
> + ARRAY_SIZE(subplatform_arl_h_ids))) {
> + mask = BIT(INTEL_SUBPLATFORM_ARL_H);
> + } else if (find_devid(devid, subplatform_arl_u_ids,
> + ARRAY_SIZE(subplatform_arl_u_ids))) {
> + mask = BIT(INTEL_SUBPLATFORM_ARL_U);
> + } else if (find_devid(devid, subplatform_arl_s_ids,
> + ARRAY_SIZE(subplatform_arl_s_ids))) {
> + mask = BIT(INTEL_SUBPLATFORM_ARL_S);
> }
>
> 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 4f4aa4ff9963..ef84eea9ba0b 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -128,7 +128,9 @@ enum intel_platform {
> #define INTEL_SUBPLATFORM_RPLU 2
>
> /* MTL */
> -#define INTEL_SUBPLATFORM_ARL 0
> +#define INTEL_SUBPLATFORM_ARL_H 0
> +#define INTEL_SUBPLATFORM_ARL_U 1
> +#define INTEL_SUBPLATFORM_ARL_S 2
>
> enum intel_ppgtt_type {
> INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
> diff --git a/include/drm/intel/i915_pciids.h b/include/drm/intel/i915_pciids.h
> index 6b92f8c3731b..ae64e8ec1adc 100644
> --- a/include/drm/intel/i915_pciids.h
> +++ b/include/drm/intel/i915_pciids.h
> @@ -765,13 +765,22 @@
> INTEL_ATS_M75_IDS(MACRO__, ## __VA_ARGS__)
>
> /* ARL */
> -#define INTEL_ARL_IDS(MACRO__, ...) \
> - MACRO__(0x7D41, ## __VA_ARGS__), \
> +#define INTEL_ARL_H_IDS(MACRO__, ...) \
> MACRO__(0x7D51, ## __VA_ARGS__), \
> + MACRO__(0x7DD1, ## __VA_ARGS__)
> +
> +#define INTEL_ARL_U_IDS(MACRO__, ...) \
> + MACRO__(0x7D41, ## __VA_ARGS__) \
> +
> +#define INTEL_ARL_S_IDS(MACRO__, ...) \
> MACRO__(0x7D67, ## __VA_ARGS__), \
> - MACRO__(0x7DD1, ## __VA_ARGS__), \
> MACRO__(0xB640, ## __VA_ARGS__)
>
> +#define INTEL_ARL_IDS(MACRO__, ...) \
> + INTEL_ARL_H_IDS(MACRO__, ## __VA_ARGS__), \
> + INTEL_ARL_U_IDS(MACRO__, ## __VA_ARGS__), \
> + INTEL_ARL_S_IDS(MACRO__, ## __VA_ARGS__)
> +
> /* MTL */
> #define INTEL_MTL_IDS(MACRO__, ...) \
> MACRO__(0x7D40, ## __VA_ARGS__), \
> --
> 2.43.0
>
next prev parent reply other threads:[~2024-10-29 13:36 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-28 23:31 [PATCH] drm/i915/gsc: ARL-H and ARL-U need a newer GSC FW Daniele Ceraolo Spurio
2024-10-29 13:36 ` Rodrigo Vivi [this message]
2024-10-29 17:14 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2024-10-29 17:14 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-10-29 17:28 ` ✓ Fi.CI.BAT: success " Patchwork
2024-10-30 1:36 ` ✗ Fi.CI.IGT: failure " Patchwork
2024-11-04 23:02 ` [PATCH] " John Harrison
2024-11-04 23:09 ` Daniele Ceraolo Spurio
2024-11-04 23:34 ` John Harrison
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZyDkwilYgDTWwr2F@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=John.C.Harrison@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.