Re: [PATCH V1 5/7] accel/amdxdna: Add query firmware version

[Lizhi Hou <lizhi.hou@xxxxxxx>]

On 12/4/24 14:24, Mario Limonciello wrote:
> On 12/4/2024 15:37, Lizhi Hou wrote:
> > Enhance GET_INFO ioctl to support retrieving firmware version.
> >
> > Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxx>
> > ---
> >   drivers/accel/amdxdna/aie2_pci.c | 20 ++++++++++++++++++++
> >   include/uapi/drm/amdxdna_accel.h | 19 +++++++++++++++++++
> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/accel/amdxdna/aie2_pci.c 
> > b/drivers/accel/amdxdna/aie2_pci.c
> > index 1c8170325837..83abd16ade11 100644
> > --- a/drivers/accel/amdxdna/aie2_pci.c
> > +++ b/drivers/accel/amdxdna/aie2_pci.c
> > @@ -640,6 +640,23 @@ static int aie2_get_aie_version(struct 
> > amdxdna_client *client,
> >       return 0;
> >   }
> >   +static int aie2_get_firmware_version(struct amdxdna_client *client,
> > +                     struct amdxdna_drm_get_info *args)
> > +{
> > +    struct amdxdna_drm_query_firmware_version version;
> > +    struct amdxdna_dev *xdna = client->xdna;
> > +
> > +    version.major = xdna->fw_ver.major;
> > +    version.minor = xdna->fw_ver.minor;
> > +    version.patch = xdna->fw_ver.sub;
> > +    version.build = xdna->fw_ver.build;
> > +
> > +    if (copy_to_user(u64_to_user_ptr(args->buffer), &version, 
> > sizeof(version)))
> > +        return -EFAULT;
> > +
> > +    return 0;
> > +}
> > +
> >   static int aie2_get_clock_metadata(struct amdxdna_client *client,
> >                      struct amdxdna_drm_get_info *args)
> >   {
> > @@ -752,6 +769,9 @@ static int aie2_get_info(struct amdxdna_client 
> > *client, struct amdxdna_drm_get_i
> >       case DRM_AMDXDNA_QUERY_HW_CONTEXTS:
> >           ret = aie2_get_hwctx_status(client, args);
> >           break;
> > +    case DRM_AMDXDNA_QUERY_FIRMWARE_VERSION:
> > +        ret = aie2_get_firmware_version(client, args);
> > +        break;
> >       default:
> >           XDNA_ERR(xdna, "Not supported request parameter %u", 
> > args->param);
> >           ret = -EOPNOTSUPP;
> > diff --git a/include/uapi/drm/amdxdna_accel.h 
> > b/include/uapi/drm/amdxdna_accel.h
> > index af12af8bd699..ea86c57beb92 100644
> > --- a/include/uapi/drm/amdxdna_accel.h
> > +++ b/include/uapi/drm/amdxdna_accel.h
> > @@ -375,6 +375,20 @@ struct amdxdna_drm_query_hwctx {
> >       __u64 errors;
> >   };
> >   +/**
> > + * struct amdxdna_drm_query_firmware_version - Query the firmware 
> > version
> > + * @major: The major version number
> > + * @minor: The minor version number
> > + * @patch: The patch level version number
> > + * @build: The build ID
> > + */
> > +struct amdxdna_drm_query_firmware_version {
> > +    __u32 major; /* out */
> > +    __u32 minor; /* out */
> > +    __u32 patch; /* out */
> > +    __u32 build; /* out */
> > +};
> > +
> >   enum amdxdna_drm_get_param {
> >       DRM_AMDXDNA_QUERY_AIE_STATUS,
> >       DRM_AMDXDNA_QUERY_AIE_METADATA,
> > @@ -382,6 +396,11 @@ enum amdxdna_drm_get_param {
> >       DRM_AMDXDNA_QUERY_CLOCK_METADATA,
> >       DRM_AMDXDNA_QUERY_SENSORS,
> >       DRM_AMDXDNA_QUERY_HW_CONTEXTS,
> > +    DRM_AMDXDNA_READ_AIE_MEM,
> > +    DRM_AMDXDNA_READ_AIE_REG,
> > +    DRM_AMDXDNA_QUERY_FIRMWARE_VERSION,
> > +    DRM_AMDXDNA_GET_POWER_MODE,
> > +    DRM_AMDXDNA_QUERY_TELEMETRY,
>
> This is more than DRM_AMDXDNA_QUERY_FIRMWARE_VERSION.
>
> The other ones should go with other patches.
> Like DRM_AMDXDNA_GET_POWER_MODE should be in patch 6.
Ok.
> I didn't see DRM_AMDXDNA_READ_AIE_MEM, DRM_AMDXDNA_READ_AIE_REG, or
> DRM_AMDXDNA_QUERY_TELEMETRY used in this series, are they just 
> placeholders?  Maybe a different patch for the placeholders?

Yes. These are placeholders. I can remove them.


Thanks,

Lizhi

> >       DRM_AMDXDNA_NUM_GET_PARAM,
> >   };