Re: [PATCH] drm/i915/gsc: ARL-H and ARL-U need a newer GSC FW.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/4/2024 15:09, Daniele Ceraolo Spurio wrote:
On 11/4/2024 3:02 PM, John Harrison wrote:
On 10/28/2024 16:31, 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@xxxxxxxxx>
Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
---
  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 };
+
+    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__)
Is there any point in defining this? I'm not seeing it being used anywhere. There is no longer a generic 'IS_ARROWLAKE()' macro. So does seem to be worth defining a generic ARL id list.

AFAICS it is still used in i915_pci.c and display/intel_display_device.c, where we assign the MTL capabilities to the ARL device.
Oops, yes. Missed those. Okay, looks good to me.

Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>


Daniele


John.

+
  /* MTL */
  #define INTEL_MTL_IDS(MACRO__, ...) \
      MACRO__(0x7D40, ## __VA_ARGS__), \






[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux