[PATCH 1/2] drm/xe: Add uAPI to query GuC firmware submission version

[Souza, Jose]

On Thu, 2024-02-08 at 10:06 -0800, John Harrison wrote:
> On 2/7/2024 10:14, José Roberto de Souza wrote:
> > 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.
> > 
> > This is querying XE_UC_FW_VER_COMPATIBILITY/submission version because
> > that is also supported by VFs, while XE_UC_FW_VER_RELEASE don't.
> > 
> > i915 uAPI: https://patchwork.freedesktop.org/series/129627/
> > Mesa usage: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25233
> > 
> > Cc: John Harrison <John.C.Harrison at Intel.com>
> > Cc: Francois Dugast <francois.dugast at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_query.c | 44 +++++++++++++++++++++++++++++++++++
> >   include/uapi/drm/xe_drm.h     | 31 ++++++++++++++++++++++++
> >   2 files changed, 75 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > index 4f1ab91dbec58..a912ba58eeb8a 100644
> > --- a/drivers/gpu/drm/xe/xe_query.c
> > +++ b/drivers/gpu/drm/xe/xe_query.c
> > @@ -516,6 +516,49 @@ 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;
> > +	struct xe_uc_fw_version *version = NULL;
> > +
> > +	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;
> Should this not also verify the version fields are zero filled on entry?

Other queries don't do that, like drm_xe_query_engine_cycles.cpu_timestamp.
I guess this is enough.

> 
> > +
> > +	switch (resp.uc_type) {
> > +	case XE_QUERY_UC_TYPE_GUC_SUBMISSION: {
> > +		struct xe_guc *guc = &xe->tiles[0].primary_gt->uc.guc;
> > +
> > +		version = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
> > +		break;
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	resp.major_ver = version->major;
> > +	resp.minor_ver = version->minor;
> > +	resp.patch_ver = version->patch;
> > +	resp.branch_ver = 0;
> > +
> > +	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,
> > @@ -525,6 +568,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 50bbea0992d9c..52a69a6f327e0 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -574,6 +574,36 @@ struct drm_xe_query_engine_cycles {
> >   	__u64 cpu_delta;
> >   };
> >   
> > +/**
> > + * 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_SUBMISSION 0
> > +	__u16 uc_type;
> > +
> > +	/** @pad: MBZ */
> > +	__u16 pad;
> > +
> > +	/* @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;
> The branch component should be first rather than last (earlier 
> assignment order should really have branch first as well for 
> consistency). If the branch has changed, that trumps everything else. It 
> is a full reset of major/minor/patch back to 1.0.0.

Will change, thank you.

> 
> John.
> 
> > +
> > +	/** @pad2: MBZ */
> > +	__u32 pad2;
> > +
> > +	/** @reserved: Reserved */
> > +	__u64 reserved;
> > +};
> > +
> >   /**
> >    * struct drm_xe_device_query - Input of &DRM_IOCTL_XE_DEVICE_QUERY - main
> >    * structure to query device information
> > @@ -643,6 +673,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;
> >   
>