[PATCH] drm/i915/gsc: ARL-H and ARL-U need a newer GSC FW.
John Harrison
john.c.harrison at intel.com
Mon Nov 4 23:34:24 UTC 2024
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 at intel.com>
>>> Cc: John Harrison <John.C.Harrison at Intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi at 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 };
>>> +
>>> + 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 at Intel.com>
>
> Daniele
>
>>
>> John.
>>
>>> +
>>> /* MTL */
>>> #define INTEL_MTL_IDS(MACRO__, ...) \
>>> MACRO__(0x7D40, ## __VA_ARGS__), \
>>
>
More information about the Intel-gfx
mailing list