Re: [PATCH] drm/xe/guc: Don't support older GuC 70.x releases

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 3/4/2024 8:38 AM, Lucas De Marchi wrote:
> On Mon, Mar 04, 2024 at 08:26:16AM -0800, Daniele Ceraolo Spurio wrote:
>> Supporting older GuC versions comes with baggage, both on the coding
>> side (due to interfaces only being available from a certain version
>> onwards) and on the testing side (due to having to make sure the driver
>> works as expected with older GuCs).
>> Since all of our Xe platform are still under force probe, we haven't
>> committed to support any specific GuC version and we therefore don't
>> need to support the older once, which means that we can force a bottom
>> limit to what GuC we accept. This allows us to remove any conditional
>> statements based on older GuC versions and also to approach newer
>> additions knowing that we'll never attempt to load something older
>> than our minimum requirement.
>>
>> As an initial value, the minimum expected version is set to 70.19,
>> which is the version currently in the firmware table, but the
>> expectation is that this will be bumbed every time we update the
>> table, until we remove the force probe.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc.c   | 14 ++------------
>> drivers/gpu/drm/xe/xe_uc_fw.c | 36 ++++++++++++++---------------------
>> 2 files changed, 16 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 0d2a2dd13f11..ba2ca6895917 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -133,15 +133,10 @@ static u32 guc_ctl_ads_flags(struct xe_guc *guc)
>>     return flags;
>> }
>>
>> -#define GUC_VER(maj, min, pat)    (((maj) << 16) | ((min) << 8) | 
>> (pat))
>> -
>> static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>> {
>>     struct xe_device *xe = guc_to_xe(guc);
>>     struct xe_gt *gt = guc_to_gt(guc);
>> -    struct xe_uc_fw *uc_fw = &guc->fw;
>> -    struct xe_uc_fw_version *version = 
>> &uc_fw->versions.found[XE_UC_FW_VER_RELEASE];
>> -
>>     u32 flags = 0;
>>
>>     if (XE_WA(gt, 22012773006))
>> @@ -171,13 +166,8 @@ static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>>     if (XE_WA(gt, 1509372804))
>>         flags |= GUC_WA_RENDER_RST_RC6_EXIT;
>>
>> -    if (XE_WA(gt, 14018913170)) {
>> -        if (GUC_VER(version->major, version->minor, version->patch) 
>> >= GUC_VER(70, 7, 0))
>> -            flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
>> -        else
>> -            drm_dbg(&xe->drm, "Skip WA 14018913170: GUC version 
>> expected >= 70.7.0, found %u.%u.%u\n",
>> -                version->major, version->minor, version->patch);
>> -    }
>> +    if (XE_WA(gt, 14018913170))
>> +        flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
>>
>>     return flags;
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c 
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index 7751d6ba6ce0..fa06b57d2af5 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -297,36 +297,28 @@ static void uc_fw_fini(struct drm_device *drm, 
>> void *arg)
>>     xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_SELECTED);
>> }
>>
>> -static void guc_read_css_info(struct xe_uc_fw *uc_fw, struct 
>> uc_css_header *css)
>> +static int guc_read_css_info(struct xe_uc_fw *uc_fw, struct 
>> uc_css_header *css)
>> {
>>     struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>>     struct xe_uc_fw_version *release = 
>> &uc_fw->versions.found[XE_UC_FW_VER_RELEASE];
>>     struct xe_uc_fw_version *compatibility = 
>> &uc_fw->versions.found[XE_UC_FW_VER_COMPATIBILITY];
>>
>>     xe_gt_assert(gt, uc_fw->type == XE_UC_FW_TYPE_GUC);
>> -    xe_gt_assert(gt, release->major >= 70);
>> -
>> -    if (release->major > 70 || release->minor >= 6) {
>> -        /* v70.6.0 adds CSS header support */
>> -        compatibility->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR,
>> -                         css->submission_version);
>> -        compatibility->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> -                         css->submission_version);
>> -        compatibility->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> -                         css->submission_version);
>> -    } else if (release->minor >= 3) {
>> -        /* v70.3.0 introduced v1.1.0 */
>> -        compatibility->major = 1;
>> -        compatibility->minor = 1;
>> -        compatibility->patch = 0;
>> -    } else {
>> -        /* v70.0.0 introduced v1.0.0 */
>> -        compatibility->major = 1;
>> -        compatibility->minor = 0;
>> -        compatibility->patch = 0;
>> +
>> +    /* We don't support GuC releases older than 70.19 */
>> +    if (release->major < 70 || (release->major == 70 && 
>> release->minor < 19)) {
>
> there's slight chance of this getting out of sync with the firmware 
> versions we
> define. What about adding a define with minimum version just above
> XE_GUC_FIRMWARE_DEFS ?

I thought about that, but I was worried that'd make it easier to 
accidentally update it after the force_probe has been removed, while 
updating it here would have to be more deliberate. I guess I could put a 
big warning comment above the define for safety.

John, as the one who most often does the GuC updates, any preference here?

>
> Anyway,
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
> for this patch or to carry over to a v2.
>

Thanks!
Daniele

> thanks
> Lucas De Marchi