[Intel-gfx] [Patch v1] drm/i915: Add uAPI to query micro-controller FW version

[Tvrtko Ursulin]

On 27/09/2023 05:14, Balasubrawmanian, Vivaik 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.

Is the firmware bug making the ccs engines generally useless, or just 
not suitable for this specific Mesa use case?

> 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
> 
> 
> Cc: John Harrison <John.C.Harrison at Intel.com>
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> 
> Cc: José Roberto de Souza <jose.souza at intel.com>
> 
> Signed-off-by: Vivaik Balasubrawmanian <vivaik.balasubrawmanian at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_query.c | 47 +++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h       | 32 +++++++++++++++++++++
>   2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c 
> b/drivers/gpu/drm/i915/i915_query.c
> index 00871ef99792..7f22a49faae7 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -551,6 +551,52 @@ static int query_hwconfig_blob(struct 
> drm_i915_private *i915,
>       return hwconfig->size;
>   }
> 
> +static int
> +query_uc_fw_version(struct drm_i915_private *i915, struct 
> drm_i915_query_item *query)
> +{
> +    struct drm_i915_query_uc_fw_version __user *query_ptr = 
> u64_to_user_ptr(query->data_ptr);
> +    size_t size = sizeof(struct drm_i915_query_uc_fw_version);
> +    struct drm_i915_query_uc_fw_version resp;
> +
> +    if (query->length == 0) {
> +        query->length = size;
> +        return 0;
> +    } else if (query->length != size) {
> +        drm_dbg(&i915->drm,
> +            "Invalid uc_fw_version query item size=%u expected=%zu\n",
> +            query->length,    size);
> +        return -EINVAL;
> +    }
> +
> +    if (copy_from_user(&resp, query_ptr, size))
> +        return -EFAULT;

The above can probably be replaced by using the copy_query_item() helper 
and it would work a bit better even since no reason to reject a buffer 
too large.

> +
> +    if (resp.pad || resp.pad2 || resp.reserved) {
> +        drm_dbg(&i915->drm,
> +            "Invalid input fw version query structure parameters 
> received");
> +        return -EINVAL;
> +    }
> +
> +    switch (resp.uc_type) {
> +    case I915_QUERY_UC_TYPE_GUC: {
> +        struct intel_guc *guc = &i915->gt0.uc.guc;
> +
> +        resp.major_ver = guc->submission_version.major;
> +        resp.minor_ver = guc->submission_version.minor;
> +        resp.patch_ver = guc->submission_version.patch;

Submission version is not the same as fw version, right? So 
DRM_I915_QUERY_UC_FW_VERSION and uapi kerneldoc is misleading.

Name the query type I915_QUERY_UC_TYPE_GUC*_SUBMISSION* and make it clear?

Regards,

Tvrtko

> +        resp.branch_ver = 0;
> +        break;
> +    }
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    if (copy_to_user(query_ptr, &resp, size))
> +        return -EFAULT;
> +
> +    return 0;
> +}
> +
>   static int (* const i915_query_funcs[])(struct drm_i915_private 
> *dev_priv,
>                       struct drm_i915_query_item *query_item) = {
>       query_topology_info,
> @@ -559,6 +605,7 @@ static int (* const i915_query_funcs[])(struct 
> drm_i915_private *dev_priv,
>       query_memregion_info,
>       query_hwconfig_blob,
>       query_geometry_subslices,
> +    query_uc_fw_version,
>   };
> 
>   int i915_query_ioctl(struct drm_device *dev, void *data, struct 
> drm_file *file)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7000e5910a1d..9be241fb77d8 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3013,6 +3013,7 @@ struct drm_i915_query_item {
>        *  - %DRM_I915_QUERY_MEMORY_REGIONS (see struct 
> drm_i915_query_memory_regions)
>        *  - %DRM_I915_QUERY_HWCONFIG_BLOB (see `GuC HWCONFIG blob uAPI`)
>        *  - %DRM_I915_QUERY_GEOMETRY_SUBSLICES (see struct 
> drm_i915_query_topology_info)
> +     *  - %DRM_I915_QUERY_UC_FW_VERSION (see struct 
> drm_i915_query_uc_fw_version)
>        */
>       __u64 query_id;
>   #define DRM_I915_QUERY_TOPOLOGY_INFO        1
> @@ -3021,6 +3022,7 @@ struct drm_i915_query_item {
>   #define DRM_I915_QUERY_MEMORY_REGIONS        4
>   #define DRM_I915_QUERY_HWCONFIG_BLOB        5
>   #define DRM_I915_QUERY_GEOMETRY_SUBSLICES    6
> +#define DRM_I915_QUERY_UC_FW_VERSION        7
>   /* Must be kept compact -- no holes and well documented */
> 
>       /**
> @@ -3213,6 +3215,36 @@ struct drm_i915_query_topology_info {
>       __u8 data[];
>   };
> 
> +/**
> +* struct drm_i915_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_i915_query_uc_fw_version {
> +    /** @uc: The micro-controller type to query firmware version */
> +#define I915_QUERY_UC_TYPE_GUC 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;
> +
> +    /** @pad2: MBZ */
> +    __u32 pad2;
> +
> +    /** @reserved: Reserved */
> +    __u64 reserved;
> +};
> +
>   /**
>    * DOC: Engine Discovery uAPI
>    *
> 
> base-commit: a42554bf0755b80fdfb8e91ca35ae6835bb3534d