[Intel-xe] [PATCH 04/12] drm/xe/gsc: Introduce GSC FW
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Tue Nov 7 23:59:22 UTC 2023
On 11/7/2023 3:52 PM, John Harrison wrote:
> 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 at intel.com>
>>>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>>>> Cc: John Harrison <John.C.Harrison at 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.
>
HuC can also be "not supported", but apart from that I'll switch to the
comment you suggested.
Daniele
>
>
>>>
>>>> + * 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;
>>>> };
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20231107/c42c3525/attachment-0001.htm>
More information about the Intel-xe
mailing list