[Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Feb 6 19:09:39 UTC 2020
On Tue, 04 Feb 2020 00:28:37 +0100, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> We are quite trigger happy in cleaning up the firmware blobs, as we do
> so from several error/fini paths in GuC/HuC/uC code. We do have the
> __uc_cleanup_firmwares cleanup function, which unwinds
> __uc_fetch_firmwares and is already called both from the error path of
> gem_init and from gem_driver_release, so let's stop cleaning up from
> all the other paths.
>
> The fact that we're not cleaning the firmware immediately means that
> we can't consider firmware availability as an indication of
> initialization success. A "READY_TO_LOAD" status has been added to
what about s/READY_TO_LOAD/LOADABLE ?
> indicate that the initialization was successful, to be used to
> selectively load HuC only if HuC init has completed (HuC init failure
> is not considered a fatal error).
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 10 ++++------
> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 3 ++-
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +-
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 7 +++++--
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 18 +++++++++++++++---
> 5 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 97b9c71a6fd4..2d05de570bdf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -333,7 +333,7 @@ int intel_guc_init(struct intel_guc *guc)
> ret = intel_uc_fw_init(&guc->fw);
> if (ret)
> - goto err_fetch;
> + goto out;
> ret = intel_guc_log_create(&guc->log);
> if (ret)
> @@ -364,6 +364,8 @@ int intel_guc_init(struct intel_guc *guc)
> /* We need to notify the guc whenever we change the GGTT */
> i915_ggtt_enable_guc(gt->ggtt);
> + intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
> +
> return 0;
> err_ct:
> @@ -374,8 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
> intel_guc_log_destroy(&guc->log);
> err_fw:
> intel_uc_fw_fini(&guc->fw);
> -err_fetch:
> - intel_uc_fw_cleanup_fetch(&guc->fw);
> +out:
> i915_probe_error(gt->i915, "failed with %d\n", ret);
> return ret;
> }
> @@ -397,9 +398,6 @@ void intel_guc_fini(struct intel_guc *guc)
> intel_guc_ads_destroy(guc);
> intel_guc_log_destroy(&guc->log);
> intel_uc_fw_fini(&guc->fw);
> - intel_uc_fw_cleanup_fetch(&guc->fw);
> -
> - intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_DISABLED);
> }
> /*
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 5f448d0e360b..2e4f4a8e791e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -121,12 +121,13 @@ int intel_huc_init(struct intel_huc *huc)
> if (err)
> goto out_fini;
> + intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
> +
> return 0;
> out_fini:
> intel_uc_fw_fini(&huc->fw);
> out:
> - intel_uc_fw_cleanup_fetch(&huc->fw);
> i915_probe_error(i915, "failed with %d\n", err);
> return err;
> }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index a119b7776973..4c55b1285cbc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -417,7 +417,7 @@ static int __uc_init_hw(struct intel_uc *uc)
> GEM_BUG_ON(!intel_uc_supports_guc(uc));
> GEM_BUG_ON(!intel_uc_wants_guc(uc));
> - if (!intel_uc_fw_is_available(&guc->fw)) {
> + if (!intel_uc_fw_is_ready_to_load(&guc->fw)) {
> ret = __uc_check_hw(uc) ||
> intel_uc_fw_is_overridden(&guc->fw) ||
> intel_uc_wants_guc_submission(uc) ?
> 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 c9c094a73399..3759569ec000 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -501,7 +501,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> u32 dst_offset, u32 dma_flags)
> if (err)
> return err;
> - if (!intel_uc_fw_is_available(uc_fw))
> + if (!intel_uc_fw_is_ready_to_load(uc_fw))
> return -ENOEXEC;
> /* Call custom loader */
> @@ -544,7 +544,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
> {
> - intel_uc_fw_cleanup_fetch(uc_fw);
> + if (i915_gem_object_has_pinned_pages(uc_fw->obj))
> + i915_gem_object_unpin_pages(uc_fw->obj);
> +
> + intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
> }
> /**
> 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 1f30543d0d2d..ba3c362aeca2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -29,8 +29,11 @@ struct intel_gt;
> * | | SELECTED |
> * +------------+- / | \ -+
> * | | MISSING <--/ | \--> ERROR |
> - * | fetch | | |
> - * | | /------> AVAILABLE <---<-----------\ |
> + * | fetch | V |
> + * | | AVAILABLE |
> + * | | | |
as we change status during "init" phase, we should also mark it here
+------------+- -+
| init | |
> + * | | V |
> + * | | /----> READY TO LOAD <---<---------\ |
> * +------------+- \ / \ \ \ -+
> * | | FAIL <--< \--> TRANSFERRED \ |
> * | upload | \ / \ / |
> @@ -46,6 +49,7 @@ enum intel_uc_fw_status {
> INTEL_UC_FIRMWARE_MISSING, /* blob not found on the system */
> INTEL_UC_FIRMWARE_ERROR, /* invalid format or version */
> INTEL_UC_FIRMWARE_AVAILABLE, /* blob found and copied in mem */
> + INTEL_UC_FIRMWARE_READY_TO_LOAD, /* all fw-required objects are ready
> */
> INTEL_UC_FIRMWARE_FAIL, /* failed to xfer or init/auth the fw */
> INTEL_UC_FIRMWARE_TRANSFERRED, /* dma xfer done */
> INTEL_UC_FIRMWARE_RUNNING /* init/auth done */
> @@ -115,6 +119,8 @@ const char *intel_uc_fw_status_repr(enum
> intel_uc_fw_status status)
> return "ERROR";
> case INTEL_UC_FIRMWARE_AVAILABLE:
> return "AVAILABLE";
> + case INTEL_UC_FIRMWARE_READY_TO_LOAD:
> + return "READY TO LOAD";
> case INTEL_UC_FIRMWARE_FAIL:
> return "FAIL";
> case INTEL_UC_FIRMWARE_TRANSFERRED:
> @@ -143,6 +149,7 @@ static inline int intel_uc_fw_status_to_error(enum
> intel_uc_fw_status status)
> case INTEL_UC_FIRMWARE_SELECTED:
> return -ESTALE;
> case INTEL_UC_FIRMWARE_AVAILABLE:
> + case INTEL_UC_FIRMWARE_READY_TO_LOAD:
> case INTEL_UC_FIRMWARE_TRANSFERRED:
> case INTEL_UC_FIRMWARE_RUNNING:
> return 0;
> @@ -184,6 +191,11 @@ static inline bool intel_uc_fw_is_available(struct
> intel_uc_fw *uc_fw)
> return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_AVAILABLE;
> }
> +static inline bool intel_uc_fw_is_ready_to_load(struct intel_uc_fw
> *uc_fw)
> +{
> + return __intel_uc_fw_status(uc_fw) == INTEL_UC_FIRMWARE_READY_TO_LOAD;
> +}
> +
> static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
> {
> return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_TRANSFERRED;
> @@ -202,7 +214,7 @@ static inline bool intel_uc_fw_is_overridden(const
> struct intel_uc_fw *uc_fw)
> static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
> {
> if (intel_uc_fw_is_loaded(uc_fw))
> - intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
> + intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
> }
> static inline u32 __intel_uc_fw_get_upload_size(struct intel_uc_fw
> *uc_fw)
in the future we should never try to hide intermediate states,
with above nits,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
More information about the Intel-gfx
mailing list