[Intel-gfx] [PATCH v2 4/8] drm/i915/huc: Load GSC-enabled HuC via DMA xfer if the fuse says so
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Sat May 20 00:05:46 UTC 2023
On 5/19/2023 11:03 AM, John Harrison wrote:
> On 4/28/2023 11:58, Daniele Ceraolo Spurio wrote:
>> In the previous patch we extracted the offset of the legacy-style HuC
>> binary located within the GSC-enabled blob, so now we can use that to
>> load the HuC via DMA if the fuse is set that way.
>> Note that we now need to differentiate between "GSC-enabled binary" and
>> "loaded by GSC", so the former case has been renamed to "MEU binary" for
>> clarity, while the latter is now based on the fuse instead of the binary
>> format. This way, all the legacy load paths are automatically taken
>> (including the auth by GuC) without having to implement further code
>> changes.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 27 ++++++++++++++---------
>> drivers/gpu/drm/i915/gt/uc/intel_huc.h | 4 +++-
>> drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +-
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 14 ++++++------
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 +-
>> 5 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> index 062ff914b274..c189ede4ef55 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> @@ -298,31 +298,38 @@ void intel_huc_init_early(struct intel_huc *huc)
>> static int check_huc_loading_mode(struct intel_huc *huc)
>> {
>> struct intel_gt *gt = huc_to_gt(huc);
>> - bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc);
>> - bool hw_uses_gsc = false;
>> + bool fw_is_meu = huc->fw.is_meu_binary;
>> /*
>> * The fuse for HuC load via GSC is only valid on platforms
>> that have
>> * GuC deprivilege.
>> */
>> if (HAS_GUC_DEPRIVILEGE(gt->i915))
>> - hw_uses_gsc = intel_uncore_read(gt->uncore,
>> GUC_SHIM_CONTROL2) &
>> - GSC_LOADS_HUC;
>> + huc->loaded_via_gsc = intel_uncore_read(gt->uncore,
>> GUC_SHIM_CONTROL2) &
>> + GSC_LOADS_HUC;
>> - if (fw_needs_gsc != hw_uses_gsc) {
>> - huc_err(huc, "mismatch between FW (%s) and HW (%s) load
>> modes\n",
>> - HUC_LOAD_MODE_STRING(fw_needs_gsc),
>> HUC_LOAD_MODE_STRING(hw_uses_gsc));
>> + if (huc->loaded_via_gsc && !fw_is_meu) {
>> + huc_err(huc, "HW requires a MEU blob, but we found a legacy
>> one\n");
>> return -ENOEXEC;
>> }
>> - /* make sure we can access the GSC via the mei driver if we
>> need it */
>> + /*
>> + * Newer meu blobs contain the old FW structure inside. If we found
>> + * that, we can use it to load the legacy way.
>> + */
>> + if (!huc->loaded_via_gsc && fw_is_meu &&
>> !huc->fw.dma_start_offset) {
>> + huc_err(huc," HW in legacy mode, but we have an incompatible
>> meu blob\n");
> Leading space in the message? MEU or meu?
As mentioned in the reply on the previous patch, I'm going to drop the
meu tag.
>
>> + return -ENOEXEC;
>> + }
>> +
>> + /* make sure we can access the GSC if we need it */
>> if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) &&
>> IS_ENABLED(CONFIG_INTEL_MEI_GSC)) &&
>> - fw_needs_gsc) {
>> + !HAS_ENGINE(gt, GSC0) && huc->loaded_via_gsc) {
> Should that be || !HAS_ENGINE ?
No. The config check is for DG2, while the engine check is for MTL+. We
need one of the two to be true, not both, so we only fail if both are false.
>
>> huc_info(huc, "can't load due to missing MEI modules\n");
> 'missing MEI modules or GSC engine'?
I'll update it to "missing requirements" or something like that.
>
>> return -EIO;
>> }
>> - huc_dbg(huc, "loaded by GSC = %s\n", str_yes_no(fw_needs_gsc));
>> + huc_dbg(huc, "loaded by GSC = %s\n",
>> str_yes_no(huc->loaded_via_gsc));
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> index db555b3c1f56..345e1b9aa062 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> @@ -39,6 +39,8 @@ struct intel_huc {
>> struct notifier_block nb;
>> enum intel_huc_delayed_load_status status;
>> } delayed_load;
>> +
>> + bool loaded_via_gsc;
>> };
>> int intel_huc_sanitize(struct intel_huc *huc);
>> @@ -73,7 +75,7 @@ static inline bool intel_huc_is_used(struct
>> intel_huc *huc)
>> static inline bool intel_huc_is_loaded_by_gsc(const struct
>> intel_huc *huc)
>> {
>> - return huc->fw.loaded_via_gsc;
>> + return huc->loaded_via_gsc;
>> }
>> static inline bool intel_huc_wait_required(struct intel_huc *huc)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> index f1c973e1c676..88ad2c322c4a 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> @@ -34,7 +34,7 @@ int intel_huc_fw_get_binary_info(struct intel_uc_fw
>> *huc_fw, const void *data, s
>> size_t min_size = sizeof(*header);
>> int i;
>> - if (!huc_fw->loaded_via_gsc) {
>> + if (!huc_fw->is_meu_binary) {
>> huc_err(huc, "Invalid FW type MEU parsing!\n");
>> return -EINVAL;
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index da6fcfe1d80a..3338dd45e78b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -180,7 +180,7 @@ struct __packed uc_fw_blob {
>> u8 major;
>> u8 minor;
>> u8 patch;
>> - bool loaded_via_gsc;
>> + bool is_meu_binary;
>> };
>> #define UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>> @@ -189,9 +189,9 @@ struct __packed uc_fw_blob {
>> .patch = patch_, \
>> .path = path_,
>> -#define UC_FW_BLOB_NEW(major_, minor_, patch_, gsc_, path_) \
>> +#define UC_FW_BLOB_NEW(major_, minor_, patch_, meu_, path_) \
>> { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>> - .legacy = false, .loaded_via_gsc = gsc_ }
>> + .legacy = false, .is_meu_binary = meu_ }
> Should we be changing the filename suffix to be 'meu' instead of 'gsc'?
The dg2 filename already used gsc and I don't want to change that. As
already mentioned, I'll just drop the meu tag.
Daniele
>
> John.
>
>> #define UC_FW_BLOB_OLD(major_, minor_, patch_, path_) \
>> { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>> @@ -296,7 +296,7 @@ __uc_fw_auto_select(struct drm_i915_private
>> *i915, struct intel_uc_fw *uc_fw)
>> uc_fw->file_wanted.path = blob->path;
>> uc_fw->file_wanted.ver.major = blob->major;
>> uc_fw->file_wanted.ver.minor = blob->minor;
>> - uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
>> + uc_fw->is_meu_binary = blob->is_meu_binary;
>> found = true;
>> break;
>> }
>> @@ -680,7 +680,7 @@ static int check_fw_header(struct intel_gt *gt,
>> if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
>> return 0;
>> - if (uc_fw->loaded_via_gsc)
>> + if (uc_fw->is_meu_binary)
>> err = check_gsc_manifest(gt, fw, uc_fw);
>> else
>> err = check_ccs_header(gt, fw, uc_fw);
>> @@ -929,7 +929,7 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw,
>> u32 dst_offset, u32 dma_flags)
>> intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>> /* Set the source address for the uCode */
>> - offset = uc_fw->dummy.start;
>> + offset = uc_fw->dummy.start + uc_fw->dma_start_offset;
>> GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
>> intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW,
>> lower_32_bits(offset));
>> intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH,
>> upper_32_bits(offset));
>> @@ -1168,7 +1168,7 @@ size_t intel_uc_fw_copy_rsa(struct intel_uc_fw
>> *uc_fw, void *dst, u32 max_len)
>> {
>> struct intel_memory_region *mr = uc_fw->obj->mm.region;
>> u32 size = min_t(u32, uc_fw->rsa_size, max_len);
>> - u32 offset = sizeof(struct uc_css_header) + uc_fw->ucode_size;
>> + u32 offset = uc_fw->dma_start_offset + sizeof(struct
>> uc_css_header) + uc_fw->ucode_size;
>> struct sgt_iter iter;
>> size_t count = 0;
>> int idx;
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> index 2691bb6bde48..8f2306627332 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> @@ -115,7 +115,7 @@ struct intel_uc_fw {
>> u32 dma_start_offset;
>> - bool loaded_via_gsc;
>> + bool is_meu_binary;
>> };
>> /*
>
More information about the dri-devel
mailing list