Re: [PATCH v4 5/8] drm/xe/guc: Bump minimum required GuC version to v70.36.0

[Riana Tauro <riana.tauro@intel.com>]

Hi John

On 1/31/2025 1:34 AM, John Harrison wrote:
> On 1/29/2025 02:16, Riana Tauro wrote:
>> The VF API version for this release is 1.17.1
>>
>> Bump the minimum required version to v70.36.0 to support
>> engine activity.
> We can only bump the minimum recommended version, not the required version.
I took the reference of 70.29.2 patch for this.
> 
>>
>> Suggested-by: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_uc_fw.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/ 
>> xe_uc_fw.c
>> index 18e06ee9e23f..d9ff285c5d1d 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -108,17 +108,17 @@ struct fw_blobs_by_type {
>>   #define XE_GUC_FIRMWARE_DEFS(fw_def, mmp_ver, major_ver)            \
>>       fw_def(PANTHERLAKE,    mmp_ver(xe,    guc,    ptl,    70, 38, 
>> 1))    \
>> -    fw_def(BATTLEMAGE,    major_ver(xe,    guc,    bmg,    70, 29, 
>> 2))    \
>> -    fw_def(LUNARLAKE,    major_ver(xe,    guc,    lnl,    70, 29, 
>> 2))    \
>> -    fw_def(METEORLAKE,    major_ver(i915,    guc,    mtl,    70, 29, 
>> 2))    \
>> -    fw_def(PVC,        mmp_ver(xe,    guc,    pvc,    70, 29, 2))    \
>> -    fw_def(DG2,        major_ver(i915,    guc,    dg2,    70, 29, 
>> 2))    \
>> -    fw_def(DG1,        major_ver(i915,    guc,    dg1,    70, 29, 
>> 2))    \
>> -    fw_def(ALDERLAKE_N,    major_ver(i915,    guc,    tgl,    70, 29, 
>> 2))    \
>> -    fw_def(ALDERLAKE_P,    major_ver(i915,    guc,    adlp,    70, 
>> 29, 2))    \
>> -    fw_def(ALDERLAKE_S,    major_ver(i915,    guc,    tgl,    70, 29, 
>> 2))    \
>> -    fw_def(ROCKETLAKE,    major_ver(i915,    guc,    tgl,    70, 29, 
>> 2))    \
>> -    fw_def(TIGERLAKE,    major_ver(i915,    guc,    tgl,    70, 29, 2))
>> +    fw_def(BATTLEMAGE,    major_ver(xe,    guc,    bmg,    70, 36, 
>> 0))    \
>> +    fw_def(LUNARLAKE,    major_ver(xe,    guc,    lnl,    70, 36, 
>> 0))    \
>> +    fw_def(METEORLAKE,    major_ver(i915,    guc,    mtl,    70, 36, 
>> 0))    \
>> +    fw_def(PVC,        mmp_ver(xe,    guc,    pvc,    70, 36, 0))    \
>> +    fw_def(DG2,        major_ver(i915,    guc,    dg2,    70, 36, 
>> 0))    \
>> +    fw_def(DG1,        major_ver(i915,    guc,    dg1,    70, 36, 
>> 0))    \
>> +    fw_def(ALDERLAKE_N,    major_ver(i915,    guc,    tgl,    70, 36, 
>> 0))    \
>> +    fw_def(ALDERLAKE_P,    major_ver(i915,    guc,    adlp,    70, 
>> 36, 0))    \
>> +    fw_def(ALDERLAKE_S,    major_ver(i915,    guc,    tgl,    70, 36, 
>> 0))    \
>> +    fw_def(ROCKETLAKE,    major_ver(i915,    guc,    tgl,    70, 36, 
>> 0))    \
>> +    fw_def(TIGERLAKE,    major_ver(i915,    guc,    tgl,    70, 36, 0))
> This part is technically fine, but note that we have just discovered an 
> issue with recent GuC releases which means we need to wait for a bug fix 
> before updating.
I added this as per your suggestion in the last rev. Should i drop this 
from the series till the guc release with the bug fix comes in?
> 
> Also note that the purpose of the minor/patch version numbers in this 
> table is just to provide a notice to the user that they should update. 
> So after the update to 70.36.0, if a user boots a LNL with 70.29.2, they 
> will get a line in dmesg saying "please update". But the driver will 
> still load and run with no negative effects.
> 
>>   #define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)        \
>>       fw_def(PANTHERLAKE,    mmp_ver(xe,    huc,        ptl, 10, 2, 
>> 1))    \
>> @@ -320,9 +320,9 @@ static int guc_read_css_info(struct xe_uc_fw 
>> *uc_fw, struct uc_css_header *css)
>>       xe_gt_assert(gt, uc_fw->type == XE_UC_FW_TYPE_GUC);
>> -    /* We don't support GuC releases older than 70.29.2 */
>> -    if (MAKE_GUC_VER_STRUCT(*release) < MAKE_GUC_VER(70, 29, 2)) {
>> -        xe_gt_err(gt, "Unsupported GuC v%u.%u.%u! v70.29.2 or newer 
>> is required\n",
>> +    /* We don't support GuC releases older than 70.36.0 */
>> +    if (MAKE_GUC_VER_STRUCT(*release) < MAKE_GUC_VER(70, 36, 0)) {
>> +        xe_gt_err(gt, "Unsupported GuC v%u.%u.%u! v70.36.0 or newer 
>> is required\n",
> This is definitely not allowed.
> 
> Once a GuC has been released for a given platform, it must be supported 
> forever on that platform. Which means that LNL and BMG must forever be 
> able to run on 70.29.2.
> 
> If we really need to, we can add a per platform variant of this check 
> for new platforms. E.g. PTL must be at least 70.39.42 or whatever. But 
> we can't ever change the Xe global base line version. And there isn't 
> much point in adding a new platform baseline because we just don't push 
> the firmware upstream for new platforms until we are ready to do an 
> official release. So there simply aren't any other versions available to 
> warrant a baseline version check.
> 
> Rather than bumping the baseline here, individual features must check 
> for a suitable GuC version. Which is what you have in patch #3 - the 
> check for GUC_VER(1.14.1). That ensures that the new feature will not 
> try to run if a new enough GuC is not available on the user's system.

Sorry about that. I took the reference of the patch 70.29.2. Looks like 
that was intended for baseline update.

I will drop the baseline update.

Thanks
Riana
> 
> John.
> 
>>                 release->major, release->minor, release->patch);
>>           return -EINVAL;
>>       }
>