Re: [Intel-xe] [PATCH 04/12] drm/xe/gsc: Introduce GSC FW

[John Harrison <john.c.harrison@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10829 bytes --]

On 11/7/2023 15:32, Daniele Ceraolo Spurio wrote:
> On 11/7/2023 3:26 PM, John Harrison wrote:
>> On 10/27/2023 15:29, Daniele Ceraolo Spurio wrote:
>>> Add the basic definitions and init function. Same as HuC, GSC is only
>>> supported on the media GT on MTL and newer platforms.
>>> Note that the GSC requires submission resources which can't be 
>>> allocated
>>> during init (because we don't have the hwconfig yet), so it can't be
>>> marked as loadable at the end of the init function. The allocation of
>>> those resources will come in the patch that makes use of them to load
>>> the FW.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/Makefile         |  1 +
>>>   drivers/gpu/drm/xe/xe_gsc.c         | 50 
>>> +++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_gsc.h         | 13 ++++++++
>>>   drivers/gpu/drm/xe/xe_gsc_types.h   | 19 +++++++++++
>>>   drivers/gpu/drm/xe/xe_uc.c          |  9 ++++--
>>>   drivers/gpu/drm/xe/xe_uc_fw.c       | 21 ++++++++++--
>>>   drivers/gpu/drm/xe/xe_uc_fw.h       |  2 ++
>>>   drivers/gpu/drm/xe/xe_uc_fw_types.h |  5 +--
>>>   drivers/gpu/drm/xe/xe_uc_types.h    |  3 ++
>>>   9 files changed, 116 insertions(+), 7 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/xe/xe_gsc.c
>>>   create mode 100644 drivers/gpu/drm/xe/xe_gsc.h
>>>   create mode 100644 drivers/gpu/drm/xe/xe_gsc_types.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index cee57681732d..474b6044d054 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -57,6 +57,7 @@ xe-y += xe_bb.o \
>>>       xe_exec_queue.o \
>>>       xe_force_wake.o \
>>>       xe_ggtt.o \
>>> +    xe_gsc.o \
>>>       xe_gt.o \
>>>       xe_gt_clock.o \
>>>       xe_gt_debugfs.o \
>>> diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
>>> new file mode 100644
>>> index 000000000000..3f709577d73b
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_gsc.c
>>> @@ -0,0 +1,50 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#include "xe_gsc.h"
>>> +
>>> +#include "xe_device.h"
>>> +#include "xe_gt.h"
>>> +#include "xe_gt_printk.h"
>>> +#include "xe_uc_fw.h"
>>> +
>>> +static struct xe_gt *
>>> +gsc_to_gt(struct xe_gsc *gsc)
>>> +{
>>> +    return container_of(gsc, struct xe_gt, uc.gsc);
>>> +}
>>> +
>>> +int xe_gsc_init(struct xe_gsc *gsc)
>>> +{
>>> +    struct xe_gt *gt = gsc_to_gt(gsc);
>>> +    struct xe_tile *tile = gt_to_tile(gt);
>>> +    int ret;
>>> +
>>> +    gsc->fw.type = XE_UC_FW_TYPE_GSC;
>>> +
>>> +    /* The GSC uC is only available on the media GT */
>>> +    if (tile->media_gt && (gt != tile->media_gt)) {
>>> +        xe_uc_fw_change_status(&gsc->fw, 
>>> XE_UC_FIRMWARE_NOT_SUPPORTED);
>>> +        return 0;
>>> +    }
>>> +
>>> +    /*
>>> +     * GSC can be "not supported" where GuC is instead supported, 
>>> so we
>>> +     * don't want to fail if xe_uc_fw_init() returns an error due 
>>> to that.
>> Not following this comment. Can you explain more?
>
> xe_uc_fw_init() is going to fail if the GSC FW is not defined for the 
> platform. The uC switch is global for all FWs, so either all FW init 
> functions are called or none of them; since we do have to handle the 
> case where GuC is supported and GSC is not, xe_gsc_init() needs to not 
> fail in that scenario. We do still have to call xe_uc_fw_init() as 
> that's where the FW selection is (and therefore where we find out if 
> it is defined or not), so the solution is to check the enablement 
> status first before checking the return code.
>
Maybe go with this instead?

    Some platforms can have GuC (and HuC?) but not GSC. That would cause
    xe_uc_fw_init(gsc) to return a "not supported" failure code and
    abort all firmware loading. So check for GSC being enabled before
    propagating the failure back up. That way the higher level will keep
    going and load GuC/HuC as appropriate.



>>
>>> +     * To avoid this problem, check the FW status before the return 
>>> code.
>>> +     */
>>> +    ret = xe_uc_fw_init(&gsc->fw);
>>> +    if (!xe_uc_fw_is_enabled(&gsc->fw))
>>> +        return 0;
>>> +    else if (ret)
>>> +        goto out;
>>> +
>>> +    return 0;
>>> +
>>> +out:
>>> +    xe_gt_err(gt, "GSC init failed with %d", ret);
>>> +    return ret;
>>> +}
>>> +
>>> diff --git a/drivers/gpu/drm/xe/xe_gsc.h b/drivers/gpu/drm/xe/xe_gsc.h
>>> new file mode 100644
>>> index 000000000000..baa7f21f4204
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_gsc.h
>>> @@ -0,0 +1,13 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _XE_GSC_H_
>>> +#define _XE_GSC_H_
>>> +
>>> +#include "xe_gsc_types.h"
>>> +
>>> +int xe_gsc_init(struct xe_gsc *gsc);
>>> +
>>> +#endif
>>> diff --git a/drivers/gpu/drm/xe/xe_gsc_types.h 
>>> b/drivers/gpu/drm/xe/xe_gsc_types.h
>>> new file mode 100644
>>> index 000000000000..135f156e3736
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_gsc_types.h
>>> @@ -0,0 +1,19 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _XE_GSC_TYPES_H_
>>> +#define _XE_GSC_TYPES_H_
>>> +
>>> +#include "xe_uc_fw_types.h"
>>> +
>>> +/**
>>> + * struct xe_gsc - GSC
>>> + */
>>> +struct xe_gsc {
>>> +    /** @fw: Generic uC firmware management */
>>> +    struct xe_uc_fw fw;
>>> +};
>>> +
>>> +#endif
>>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>>> index 784f53c5f282..b67154c78dff 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc.c
>>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>>> @@ -6,6 +6,7 @@
>>>   #include "xe_uc.h"
>>>     #include "xe_device.h"
>>> +#include "xe_gsc.h"
>>>   #include "xe_gt.h"
>>>   #include "xe_guc.h"
>>>   #include "xe_guc_pc.h"
>>> @@ -32,8 +33,8 @@ int xe_uc_init(struct xe_uc *uc)
>>>       int ret;
>>>         /*
>>> -     * We call the GuC/HuC init functions even if GuC submission is 
>>> off to
>>> -     * correctly move our tracking of the FW state to "disabled".
>>> +     * We call the GuC/HuC/GSC init functions even if GuC 
>>> submission is off
>>> +     * to correctly move our tracking of the FW state to "disabled".
>>>        */
>>>         ret = xe_guc_init(&uc->guc);
>>> @@ -44,6 +45,10 @@ int xe_uc_init(struct xe_uc *uc)
>>>       if (ret)
>>>           goto err;
>>>   +    ret = xe_gsc_init(&uc->gsc);
>>> +    if (ret)
>>> +        goto err;
>>> +
>>>       if (!xe_device_uc_enabled(uc_to_xe(uc)))
>>>           return 0;
>>>   diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c 
>>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>>> index 1f7dac394a1d..af3e5cba606f 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>>> @@ -158,11 +158,18 @@ XE_HUC_FIRMWARE_DEFS(XE_UC_MODULE_FIRMWARE,
>>>   static struct xe_gt *
>>>   __uc_fw_to_gt(struct xe_uc_fw *uc_fw, enum xe_uc_fw_type type)
>>>   {
>>> -    if (type == XE_UC_FW_TYPE_GUC)
>>> +    XE_WARN_ON(type >= XE_UC_FW_NUM_TYPES);
>>> +
>>> +    switch (type) {
>>> +    case XE_UC_FW_TYPE_GUC:
>>>           return container_of(uc_fw, struct xe_gt, uc.guc.fw);
>>> +    case XE_UC_FW_TYPE_HUC:
>>> +        return container_of(uc_fw, struct xe_gt, uc.huc.fw);
>>> +    case XE_UC_FW_TYPE_GSC:
>>> +        return container_of(uc_fw, struct xe_gt, uc.gsc.fw);
>>> +    }
>>>   -    XE_WARN_ON(type != XE_UC_FW_TYPE_HUC);
>>> -    return container_of(uc_fw, struct xe_gt, uc.huc.fw);
>>> +    return NULL;
>>>   }
>>>     static struct xe_gt *uc_fw_to_gt(struct xe_uc_fw *uc_fw)
>>> @@ -197,6 +204,14 @@ uc_fw_auto_select(struct xe_device *xe, struct 
>>> xe_uc_fw *uc_fw)
>>>       u32 count;
>>>       int i;
>>>   +    /*
>>> +     * GSC FW support is still not fully in place, so we're not 
>>> defining
>>> +     * the FW blob yet because we don't want the driver to attempt 
>>> to load
>>> +     * it until we're ready for it.
>>> +     */
>>> +    if (uc_fw->type == XE_UC_FW_TYPE_GSC)
>>> +        return;
>>> +
>>>       xe_assert(xe, uc_fw->type < ARRAY_SIZE(blobs_all));
>>>       entries = blobs_all[uc_fw->type].entries;
>>>       count = blobs_all[uc_fw->type].count;
>>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.h 
>>> b/drivers/gpu/drm/xe/xe_uc_fw.h
>>> index 1d1a0c156cdf..d4682b0276e2 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_fw.h
>>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.h
>>> @@ -96,6 +96,8 @@ static inline const char *xe_uc_fw_type_repr(enum 
>>> xe_uc_fw_type type)
>>>           return "GuC";
>>>       case XE_UC_FW_TYPE_HUC:
>>>           return "HuC";
>>> +    case XE_UC_FW_TYPE_GSC:
>>> +        return "GSC";
>>>       }
>>>       return "uC";
>>>   }
>>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_types.h 
>>> b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> index e4774c560e67..239256bfdb07 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> @@ -55,9 +55,10 @@ enum xe_uc_fw_status {
>>>     enum xe_uc_fw_type {
>>>       XE_UC_FW_TYPE_GUC = 0,
>>> -    XE_UC_FW_TYPE_HUC
>>> +    XE_UC_FW_TYPE_HUC,
>>> +    XE_UC_FW_TYPE_GSC
>>>   };
>>> -#define XE_UC_FW_NUM_TYPES 2
>>> +#define XE_UC_FW_NUM_TYPES 3
>> Why is this not just a sentinel entry of the enum?
>
> No idea, I'm just updating the value. It is the same in i915.
Seems like a good opportunity to fix it.

John.

>
> Daniele
>
>>
>> John.
>>
>>>     /**
>>>    * struct xe_uc_fw_version - Version for XE micro controller firmware
>>> diff --git a/drivers/gpu/drm/xe/xe_uc_types.h 
>>> b/drivers/gpu/drm/xe/xe_uc_types.h
>>> index 49bef6498b85..9924e4484866 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_uc_types.h
>>> @@ -6,6 +6,7 @@
>>>   #ifndef _XE_UC_TYPES_H_
>>>   #define _XE_UC_TYPES_H_
>>>   +#include "xe_gsc_types.h"
>>>   #include "xe_guc_types.h"
>>>   #include "xe_huc_types.h"
>>>   #include "xe_wopcm_types.h"
>>> @@ -18,6 +19,8 @@ struct xe_uc {
>>>       struct xe_guc guc;
>>>       /** @huc: HuC */
>>>       struct xe_huc huc;
>>> +    /** @gsc: Graphics Security Controller */
>>> +    struct xe_gsc gsc;
>>>       /** @wopcm: WOPCM */
>>>       struct xe_wopcm wopcm;
>>>   };
>>
>

[-- Attachment #2: Type: text/html, Size: 20068 bytes --]