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

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

On 2/6/2024 8:21 PM, Lucas De Marchi wrote:
> On Tue, Feb 06, 2024 at 03:41:03PM -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.
>>
>> RFC: this patch sets the minimum to the current GuC version (70.19),
>
> we are still using PVC for development, even if not completely
> supported. We can't update to 70.19 since PVC is not in that version.
> Once we have a firmware in at least drm-firmware repo, then I think
> we can think about the changes here.

My aim here was to discuss putting a lower bound to the GuC FW version, 
not necessarily merging this immediately (hence the RFC tag). If we 
agree to proceed with this approach, I can then sort out the 
requirements, like pushing a new PVC FW for testing.

Daniele

>
>> but that can be moved one way or the other. The main aim here is
>> agreeing to stop supporting very old GuC releases on the newer driver.
>>
>> 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 868208a39829..5e6b27aac495 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -132,15 +132,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))
>> @@ -170,13 +165,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 4714f2c8d2ba..e5bf59616f3d 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -296,36 +296,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)) {
>> +        xe_gt_err(gt, "Unsupported GuC v%u.%u! v70.19 or newer is 
>> required\n",
>> +              release->major, release->minor);
>> +        return -EINVAL;
>>     }
>>
>> +    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);
>> +
>>     uc_fw->private_data_size = css->private_data_size;
>> +
>> +    return 0;
>
> my main concern is not about raising the version requirement, but that
> this is too early. We don't have 70.19 for any platform in
> linux-firmware yet. And we don't have it in drm-firmware for PVC at all.
>
> From the above changes, it doesn't seem we are reducing a lot of code:
>
>     2 files changed, 16 insertions(+), 34 deletions(-)
>
> maybe let's wait a little more before doing that?
>
> Lucas De Marchi
>
>> }
>>
>> int xe_uc_fw_check_version_requirements(struct xe_uc_fw *uc_fw)
>> @@ -424,7 +416,7 @@ static int parse_css_header(struct xe_uc_fw 
>> *uc_fw, const void *fw_data, size_t
>>     release->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, 
>> css->sw_version);
>>
>>     if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>> -        guc_read_css_info(uc_fw, css);
>> +        return guc_read_css_info(uc_fw, css);
>>
>>     return 0;
>> }
>> -- 
>> 2.43.0
>>