[PATCH 2/3] drm/xe/vf: Store the GuC FW info in guc->fw
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu May 8 21:26:06 UTC 2025
Hi Daniele,
On 08.04.2025 23:31, Daniele Ceraolo Spurio wrote:
> The GuC compatibility version that we read from the CSS header in
> native/PF and the GuC VF version that we get when a VF handshakes with
> the GuC are the same version number, so we should store it into the same
> structure. This makes all the checks based on the compatibility version
> automatically work for VFs without having to copy the value over.
>
> For completion, also copy the wanted version and set the path to a known
> string to indicate that the FW is PF-loaded. This way all the info will
> be coherent when dumped from debugfs.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Lukasz Laguna <lukasz.laguna at intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 42 ++++++++++++++++++++-
> drivers/gpu/drm/xe/xe_gt_sriov_vf.h | 4 ++
> drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h | 2 +
> drivers/gpu/drm/xe/xe_uc_fw.c | 46 +++++++++++++++++------
> 4 files changed, 81 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 68714e607b10..a1c025d7e6a9 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -173,6 +173,9 @@ static int vf_handshake_with_guc(struct xe_gt *gt)
> } else {
> vf_wanted_guc_version(gt, &wanted);
> xe_gt_assert(gt, wanted.major != GUC_VERSION_MAJOR_ANY);
> +
> + /* First time we handshake, so record the minimum wanted */
> + gt->sriov.vf.wanted_guc_version = wanted;
> }
>
> err = guc_action_match_version(guc, &wanted, guc_version);
> @@ -558,6 +561,40 @@ u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt)
> return gt->sriov.vf.self_config.num_ctxs;
> }
>
it looks that below function is put between
xe_gt_sriov_vf_guc_ids
xe_gt_sriov_vf_lmem
which serve different purposes are both are related
a place just after xe_gt_sriov_vf_bootstrap() seems better
> +/**
> + * xe_gt_sriov_vf_guc_versions - Minimum required and found GuC ABI versions
> + * @gt: the &xe_gt
> + * @min: pointer to the xe_uc_fw_version to be filled with the min version
> + * @guc_version: pointer to the xe_uc_fw_version to be filled with the found version
as the "min version" is just "min"
then "found version" can be
s/guc_version/found
> + *
> + * This function is for VF use only.
> + *
> + * Return: 0 if the versions are available, an errno value if the bootstrap
> + * failed
> + */
> +int xe_gt_sriov_vf_guc_versions(struct xe_gt *gt,
> + struct xe_uc_fw_version *min,
> + struct xe_uc_fw_version *guc_version)
> +{
> + xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
asserts should be after local vars list
> + int err;
> +
> + if (!gt->sriov.vf.guc_version.major) {
> + xe_guc_comm_init_early(>->uc.guc);
> + err = xe_gt_sriov_vf_bootstrap(gt);
hmm, this looks like we are trying to hide that we somehow missed to do
proper init/bootstrap before calling this function
so maybe call ordering is just wrong ?
then either we can assert that at this point we should know the outcome
of the handshake or return an error due to failed previous handshake
> + if (err)
> + return err;
> + }
> +
> + if (min)
> + *min = gt->sriov.vf.wanted_guc_version;
> +
> + if (guc_version)
> + *guc_version = gt->sriov.vf.guc_version;
> +
> + return 0;
> +}
> +
> /**
> * xe_gt_sriov_vf_lmem - VF LMEM configuration.
> * @gt: the &xe_gt
> @@ -1095,6 +1132,7 @@ void xe_gt_sriov_vf_print_runtime(struct xe_gt *gt, struct drm_printer *p)
> void xe_gt_sriov_vf_print_version(struct xe_gt *gt, struct drm_printer *p)
> {
> struct xe_uc_fw_version *guc_version = >->sriov.vf.guc_version;
> + struct xe_uc_fw_version *wanted = >->sriov.vf.wanted_guc_version;
> struct xe_gt_sriov_vf_relay_version *pf_version = >->sriov.vf.pf_version;
> struct xe_uc_fw_version ver;
>
> @@ -1105,8 +1143,8 @@ void xe_gt_sriov_vf_print_version(struct xe_gt *gt, struct drm_printer *p)
> vf_minimum_guc_version(gt, &ver);
> drm_printf(p, "\tbase:\t%u.%u.%u.*\n", ver.branch, ver.major, ver.minor);
>
> - vf_wanted_guc_version(gt, &ver);
> - drm_printf(p, "\twanted:\t%u.%u.%u.*\n", ver.branch, ver.major, ver.minor);
> + drm_printf(p, "\twanted:\t%u.%u.%u.*\n",
> + wanted->branch, wanted->major, wanted->minor);
>
> drm_printf(p, "\thandshake:\t%u.%u.%u.%u\n",
> guc_version->branch, guc_version->major,
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> index ba6c5d74e326..32ac8fa0d5fb 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> @@ -11,6 +11,7 @@
> struct drm_printer;
> struct xe_gt;
> struct xe_reg;
> +struct xe_uc_fw_version;
>
> int xe_gt_sriov_vf_reset(struct xe_gt *gt);
> int xe_gt_sriov_vf_bootstrap(struct xe_gt *gt);
> @@ -23,6 +24,9 @@ void xe_gt_sriov_vf_migrated_event_handler(struct xe_gt *gt);
>
> u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt);
> u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
> +int xe_gt_sriov_vf_guc_versions(struct xe_gt *gt,
> + struct xe_uc_fw_version *min,
> + struct xe_uc_fw_version *actual);
> u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt);
> u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg);
> void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val);
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> index f1b5e523ddf0..da97e3351a13 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> @@ -58,6 +58,8 @@ struct xe_gt_sriov_vf_runtime {
> * struct xe_gt_sriov_vf - GT level VF virtualization data.
> */
> struct xe_gt_sriov_vf {
> + /** @wanted_guc_version: minimum wanted GuC ABI version. */
> + struct xe_uc_fw_version wanted_guc_version;
> /** @guc_version: negotiated GuC ABI version. */
> struct xe_uc_fw_version guc_version;
> /** @self_config: resource configurations. */
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> index 70d7b542f665..d913440fabf1 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> @@ -16,6 +16,7 @@
> #include "xe_gsc.h"
> #include "xe_gt.h"
> #include "xe_gt_printk.h"
> +#include "xe_gt_sriov_vf.h"
> #include "xe_guc.h"
> #include "xe_map.h"
> #include "xe_mmio.h"
> @@ -665,6 +666,38 @@ do { \
> ver_->major, ver_->minor, ver_->patch); \
> } while (0)
>
> +static int uc_fw_vf_override(struct xe_uc_fw *uc_fw)
> +{
> + struct xe_uc_fw_version *wanted = &uc_fw->versions.wanted;
> + struct xe_uc_fw_version *compat = &uc_fw->versions.found[XE_UC_FW_VER_COMPATIBILITY];
> + int err;
rev-xmas-order?
> +
> + /* Only GuC/HuC are supported */
> + if (uc_fw->type != XE_UC_FW_TYPE_GUC && uc_fw->type != XE_UC_FW_TYPE_HUC)
> + uc_fw->path = NULL;
> +
> + /* VF will support only firmwares that driver can autoselect */
> + xe_uc_fw_change_status(uc_fw, uc_fw->path ?
> + XE_UC_FIRMWARE_PRELOADED :
> + XE_UC_FIRMWARE_NOT_SUPPORTED);
> +
> + if (!xe_uc_fw_is_supported(uc_fw))
> + return 0;
> +
> + /* PF is doing the loading, so we don't need a path on the VF */
> + uc_fw->path = "Loaded by PF";
> +
> + /* The GuC versions are set up during the VF bootstrap */
> + if (uc_fw->type == XE_UC_FW_TYPE_GUC) {
> + uc_fw->versions.wanted_type = XE_UC_FW_VER_COMPATIBILITY;
> + err = xe_gt_sriov_vf_guc_versions(uc_fw_to_gt(uc_fw), wanted, compat);
> + if (err)
> + return err;
on this error, shouldn't we change status to:
XE_UC_FIRMWARE_ERROR, /* invalid format or version */
> + }
shouldn't we set uc_fw->path and uc_fw->versions.wanted_type here just
before claiming success?
> +
> + return 0;
> +}
> +
> static int uc_fw_request(struct xe_uc_fw *uc_fw, const struct firmware **firmware_p)
> {
> struct xe_device *xe = uc_fw_to_xe(uc_fw);
> @@ -683,17 +716,8 @@ static int uc_fw_request(struct xe_uc_fw *uc_fw, const struct firmware **firmwar
>
> uc_fw_auto_select(xe, uc_fw);
>
> - if (IS_SRIOV_VF(xe)) {
> - /* Only GuC/HuC are supported */
> - if (uc_fw->type != XE_UC_FW_TYPE_GUC &&
> - uc_fw->type != XE_UC_FW_TYPE_HUC)
> - uc_fw->path = NULL;
> - /* VF will support only firmwares that driver can autoselect */
> - xe_uc_fw_change_status(uc_fw, uc_fw->path ?
> - XE_UC_FIRMWARE_PRELOADED :
> - XE_UC_FIRMWARE_NOT_SUPPORTED);
> - return 0;
> - }
> + if (IS_SRIOV_VF(xe))
> + return uc_fw_vf_override(uc_fw);
>
> uc_fw_override(uc_fw);
>
More information about the Intel-xe
mailing list