[Intel-xe] [RFC v1 04/17] drm/xe: Add uAPI to query micro-controler firmware version

[John Harrison]

On 10/11/2023 06:59, Francois Dugast wrote:
> From: José Roberto de Souza <jose.souza at intel.com>
>
> Due to a bug in GuC firmware, Mesa can't enable by default the usage of
> compute engines in DG2 and newer.
>
> A new GuC firmware fixed the issue but until now there was no way
> for Mesa to know if KMD was running with the fixed GuC version or not,
> so this uAPI is required.
>
> It may be expanded in future to query other firmware versions too.
>
> More information: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23661
> Mesa usage: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25233
>
> v2:
> - changed to submission version
> - added branch version to be future proof
> - checking if pads and reserved are zero
>
> v3:
> - add braces around case XE_QUERY_UC_TYPE_GUC to make CI happy
>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_query.c | 41 +++++++++++++++++++++++++++++++++++
>   include/uapi/drm/xe_drm.h     | 31 ++++++++++++++++++++++++++
>   2 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 10b9878ec95a..efbcf2ef436e 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -498,6 +498,46 @@ static int query_gt_topology(struct xe_device *xe,
>   	return 0;
>   }
>   
> +static int
> +query_uc_fw_version(struct xe_device *xe, struct drm_xe_device_query *query)
> +{
> +	struct drm_xe_query_uc_fw_version __user *query_ptr = u64_to_user_ptr(query->data);
> +	size_t size = sizeof(struct drm_xe_query_uc_fw_version);
> +	struct drm_xe_query_uc_fw_version resp;
> +
> +	if (query->size == 0) {
> +		query->size = size;
> +		return 0;
> +	} else if (XE_IOCTL_DBG(xe, query->size != size)) {
> +		return -EINVAL;
> +	}
> +
> +	if (copy_from_user(&resp, query_ptr, size))
> +		return -EFAULT;
> +
> +	if (XE_IOCTL_DBG(xe, resp.pad || resp.pad2 || resp.reserved))
> +		return -EINVAL;
> +
> +	switch (resp.uc_type) {
> +	case XE_QUERY_UC_TYPE_GUC: {
> +		struct xe_guc *guc = &xe->tiles[0].primary_gt->uc.guc;
> +
> +		resp.major_ver = guc->submission_state.version.major;
> +		resp.minor_ver = guc->submission_state.version.minor;
> +		resp.patch_ver = guc->submission_state.version.patch;
> +		resp.branch_ver = 0;
> +		break;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (copy_to_user(query_ptr, &resp, size))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>   static int (* const xe_query_funcs[])(struct xe_device *xe,
>   				      struct drm_xe_device_query *query) = {
>   	query_engines,
> @@ -507,6 +547,7 @@ static int (* const xe_query_funcs[])(struct xe_device *xe,
>   	query_hwconfig,
>   	query_gt_topology,
>   	query_engine_cycles,
> +	query_uc_fw_version,
>   };
>   
>   int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 47d2d0468339..f7ca52aba79b 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -466,6 +466,36 @@ struct drm_xe_query_topology_mask {
>   	__u8 mask[];
>   };
>   
> +/**
> + * struct drm_xe_query_uc_fw_version - query a micro-controller firmware version
> + *
> + * Given a uc_type this will return the major, minor, patch and branch version
> + * of the micro-controller firmware.
> + */
> +struct drm_xe_query_uc_fw_version {
> +	/** @uc: The micro-controller type to query firmware version */
> +#define XE_QUERY_UC_TYPE_GUC 0
I thought it was agreed to use TYPE_GUC_SUBMISSION to differentiate from 
the file version which may need to be added at some point (e.g. for 
reporting status in a control panel or something).

> +	__u16 uc_type;
> +
> +	/** @pad: MBZ */
> +	__u16 pad;
And wasn't there a comment about just using a u32 for the type? What is 
the advantage to splitting it into two u16 values?

John.

> +
> +	/* @major_ver: major uc fw version */
> +	__u32 major_ver;
> +	/* @minor_ver: minor uc fw version */
> +	__u32 minor_ver;
> +	/* @patch_ver: patch uc fw version */
> +	__u32 patch_ver;
> +	/* @branch_ver: branch uc fw version */
> +	__u32 branch_ver;
> +
> +	/** @pad2: MBZ */
> +	__u32 pad2;
> +
> +	/** @reserved: Reserved */
> +	__u64 reserved;
> +};
> +
>   /**
>    * struct drm_xe_device_query - main structure to query device information
>    *
> @@ -518,6 +548,7 @@ struct drm_xe_device_query {
>   #define DRM_XE_DEVICE_QUERY_HWCONFIG		4
>   #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY		5
>   #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
> +#define DRM_XE_DEVICE_QUERY_UC_FW_VERSION	7
>   	/** @query: The type of data to query */
>   	__u32 query;
>