Re: [Intel-gfx] [PATCH v2 2/8] drm/i915/uc: perma-pin firmwares

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

On 4/28/2023 11:58, Daniele Ceraolo Spurio wrote:
> Now that each FW has its own reserved area, we can keep them always
> pinned and skip the pin/unpin dance on reset. This will make things
> easier for the 2-step HuC authentication, which requires the FW to be
> pinned in GGTT after the xfer is completed.
> Given that we use dummy vmas for the pinning, we do need to explicitly
> re-pin on resume because the automated helper won't cover us.
>
> Signed-off-by: Daniele Ceraolo Spurio<daniele.ceraolospurio@intel.com>
> Cc: Alan Previn<alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_ggtt.c      |  3 ++
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c |  7 ++++-
>   drivers/gpu/drm/i915/gt/uc/intel_guc.c    |  2 +-
>   drivers/gpu/drm/i915/gt/uc/intel_huc.c    |  2 +-
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  8 +++++
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     |  2 ++
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 36 ++++++++++++++++++-----
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  5 +++-
>   8 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 20915edc8bd9..ab71ed11de79 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -1322,6 +1322,9 @@ void i915_ggtt_resume(struct i915_ggtt *ggtt)
>   		ggtt->vm.scratch_range(&ggtt->vm, ggtt->error_capture.start,
>   				       ggtt->error_capture.size);
>   
> +	list_for_each_entry(gt, &ggtt->gt_list, ggtt_link)
> +		intel_uc_resume_mappings(&gt->uc);
> +
>   	ggtt->invalidate(ggtt);
>   
>   	if (flush)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 64bff01026e8..af542e3cb3e9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -80,7 +80,12 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
>   {
>   	struct intel_gt *gt = gsc_uc_to_gt(gsc);
>   
> -	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC);
> +	/*
> +	 * GSC FW needs to be copied to a dedicated memory allocations for
> +	 * loading (see gsc->local), so we don't need to GGTT map the FW image
> +	 * itself into GGTT.
> +	 */
> +	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC, false);
>   	INIT_WORK(&gsc->work, gsc_work);
>   
>   	/* we can arrive here from i915_driver_early_probe for primary
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index c9f20385f6a0..2eb891b270ae 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -164,7 +164,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>   	struct intel_gt *gt = guc_to_gt(guc);
>   	struct drm_i915_private *i915 = gt->i915;
>   
> -	intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC);
> +	intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC, true);
>   	intel_guc_ct_init_early(&guc->ct);
>   	intel_guc_log_init_early(&guc->log);
>   	intel_guc_submission_init_early(guc);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index aefdaa62da99..9721761373fb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -276,7 +276,7 @@ void intel_huc_init_early(struct intel_huc *huc)
>   	struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
>   	struct intel_gt *gt = huc_to_gt(huc);
>   
> -	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);
> +	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, true);
>   
>   	/*
>   	 * we always init the fence as already completed, even if HuC is not
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 996168312340..b6adfda3761e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -697,6 +697,12 @@ void intel_uc_suspend(struct intel_uc *uc)
>   	}
>   }
>   
> +static void __uc_resume_mappings(struct intel_uc *uc)
> +{
> +	intel_uc_fw_resume_mapping(&uc->guc.fw);
> +	intel_uc_fw_resume_mapping(&uc->huc.fw);
> +}
> +
>   static int __uc_resume(struct intel_uc *uc, bool enable_communication)
>   {
>   	struct intel_guc *guc = &uc->guc;
> @@ -764,4 +770,6 @@ static const struct intel_uc_ops uc_ops_on = {
>   
>   	.init_hw = __uc_init_hw,
>   	.fini_hw = __uc_fini_hw,
> +
> +	.resume_mappings = __uc_resume_mappings,
>   };
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 5d0f1bcc381e..c2783e6e752b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -24,6 +24,7 @@ struct intel_uc_ops {
>   	void (*fini)(struct intel_uc *uc);
>   	int (*init_hw)(struct intel_uc *uc);
>   	void (*fini_hw)(struct intel_uc *uc);
> +	void (*resume_mappings)(struct intel_uc *uc);
>   };
>   
>   struct intel_uc {
> @@ -113,6 +114,7 @@ intel_uc_ops_function(init, init, int, 0);
>   intel_uc_ops_function(fini, fini, void, );
>   intel_uc_ops_function(init_hw, init_hw, int, 0);
>   intel_uc_ops_function(fini_hw, fini_hw, void, );
> +intel_uc_ops_function(resume_mappings, resume_mappings, void, );
>   #undef intel_uc_ops_function
>   
>   #endif
> 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 6b71b9febd74..03f0b258aea7 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -422,12 +422,14 @@ static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc
>    * intel_uc_fw_init_early - initialize the uC object and select the firmware
>    * @uc_fw: uC firmware
>    * @type: type of uC
> + * @needs_ggtt_mapping: whether the FW needs to be GGTT mapped for loading
>    *
>    * Initialize the state of our uC object and relevant tracking and select the
>    * firmware to fetch and load.
>    */
>   void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
> -			    enum intel_uc_fw_type type)
> +			    enum intel_uc_fw_type type,
> +			    bool needs_ggtt_mapping)
>   {
>   	struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915;
>   
> @@ -440,6 +442,7 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>   	GEM_BUG_ON(uc_fw->file_selected.path);
>   
>   	uc_fw->type = type;
> +	uc_fw->needs_ggtt_mapping = needs_ggtt_mapping;
>   
>   	if (HAS_GT_UC(i915)) {
>   		__uc_fw_auto_select(i915, uc_fw);
> @@ -699,7 +702,7 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **
>   	if (err)
>   		return err;
>   
> -	if ((*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) {
> +	if (uc_fw->needs_ggtt_mapping && (*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) {
>   		gt_err(gt, "%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
>   		       intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>   		       (*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
> @@ -880,6 +883,9 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw)
>   	struct i915_vma_resource *dummy = &uc_fw->dummy;
>   	u32 pte_flags = 0;
>   
> +	if (!uc_fw->needs_ggtt_mapping)
> +		return;
> +
>   	dummy->start = uc_fw_ggtt_offset(uc_fw);
>   	dummy->node_size = obj->base.size;
>   	dummy->bi.pages = obj->mm.pages;
> @@ -901,11 +907,13 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw)
>   
>   static void uc_fw_unbind_ggtt(struct intel_uc_fw *uc_fw)
>   {
> -	struct drm_i915_gem_object *obj = uc_fw->obj;
>   	struct i915_ggtt *ggtt = __uc_fw_to_gt(uc_fw)->ggtt;
> -	u64 start = uc_fw_ggtt_offset(uc_fw);
> +	struct i915_vma_resource *dummy = &uc_fw->dummy;
I'm confused as to why this was using uc_fw->obj previously? Why was it 
not originally using dummy? And why if that was correct before, why is 
not correct now?

> +
> +	if (!dummy->node_size)
> +		return;
>   
> -	ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size);
> +	ggtt->vm.clear_range(&ggtt->vm, dummy->start, dummy->node_size);
>   }
>   
>   static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
> @@ -922,7 +930,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_ggtt_offset(uc_fw);
> +	offset = uc_fw->dummy.start;
Same question here.

John.

>   	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));
> @@ -996,9 +1004,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
>   		return -ENOEXEC;
>   
>   	/* Call custom loader */
> -	uc_fw_bind_ggtt(uc_fw);
>   	err = uc_fw_xfer(uc_fw, dst_offset, dma_flags);
> -	uc_fw_unbind_ggtt(uc_fw);
>   	if (err)
>   		goto fail;
>   
> @@ -1102,6 +1108,8 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
>   		goto out_unpin;
>   	}
>   
> +	uc_fw_bind_ggtt(uc_fw);
> +
>   	return 0;
>   
>   out_unpin:
> @@ -1112,6 +1120,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
>   
>   void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
>   {
> +	uc_fw_unbind_ggtt(uc_fw);
>   	uc_fw_rsa_data_destroy(uc_fw);
>   
>   	if (i915_gem_object_has_pinned_pages(uc_fw->obj))
> @@ -1120,6 +1129,17 @@ void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
>   	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
>   }
>   
> +void intel_uc_fw_resume_mapping(struct intel_uc_fw *uc_fw)
> +{
> +	if (!intel_uc_fw_is_available(uc_fw))
> +		return;
> +
> +	if (!i915_gem_object_has_pinned_pages(uc_fw->obj))
> +		return;
> +
> +	uc_fw_bind_ggtt(uc_fw);
> +}
> +
>   /**
>    * intel_uc_fw_cleanup_fetch - cleanup uC firmware
>    * @uc_fw: uC firmware
> 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 6ba00e6b3975..26a9d6e0dc00 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -105,6 +105,7 @@ struct intel_uc_fw {
>   	 * threaded as it done during driver load (inherently single threaded)
>   	 * or during a GT reset (mutex guarantees single threaded).
>   	 */
> +	bool needs_ggtt_mapping;
>   	struct i915_vma_resource dummy;
>   	struct i915_vma *rsa_data;
>   
> @@ -282,12 +283,14 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
>   }
>   
>   void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
> -			    enum intel_uc_fw_type type);
> +			    enum intel_uc_fw_type type,
> +			    bool needs_ggtt_mapping);
>   int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw);
>   void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
>   int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 offset, u32 dma_flags);
>   int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
>   void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
> +void intel_uc_fw_resume_mapping(struct intel_uc_fw *uc_fw);
>   size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
>   int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err);
>   void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);