Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2024-02-06 at 12:42 -0800, John Harrison wrote:
> On 2/6/2024 08:33, Tvrtko Ursulin wrote:
> > On 01/02/2024 18:25, Souza, Jose wrote:
> > > On Wed, 2024-01-24 at 08:55 +0000, Tvrtko Ursulin wrote:
> > > > On 24/01/2024 08:19, Joonas Lahtinen wrote:
> > > > > Add reporting of the GuC submissio/VF interface version via GETPARAM
> > > > > properties. Mesa intends to use this information to check for old
> > > > > firmware versions with known bugs before enabling features like async
> > > > > compute.
> > > > 
> > > > There was
> > > > https://patchwork.freedesktop.org/patch/560704/?series=124592&rev=1
> > > > which does everything in one go so would be my preference.
> > > 
> > > IMO Joonas version brings less burden to be maintained(no new struct).
> > > But both versions works, please just get into some agreement so we 
> > > can move this forward.
> > 
> > So I would really prefer the query. Simplified version would do like 
> > the compile tested only:
> Vivaik's patch is definitely preferred. It is much cleaner to make one 
> single call than having to make four separate calls. It is also 
> extensible to other firmwares if required. The only blockage against it 
> was whether it was a good thing to report at all. If that blockage is no 
> longer valid then we should just merge the patch that has already been 
> discussed, polished, fixed, etc. rather than starting the whole process 
> from scratch.

Agreed.

Vivaik can you please rebase and send it again?


> 
> And note that it is four calls not three. The code below is missing the 
> branch version number.
> 
> John.
> 
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_query.c 
> > b/drivers/gpu/drm/i915/i915_query.c
> > index 00871ef99792..999687f6a3d4 100644
> > --- a/drivers/gpu/drm/i915/i915_query.c
> > +++ b/drivers/gpu/drm/i915/i915_query.c
> > @@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct 
> > drm_i915_private *i915,
> >         return hwconfig->size;
> >  }
> > 
> > +static int
> > +query_guc_submission_version(struct drm_i915_private *i915,
> > +                            struct drm_i915_query_item *query)
> > +{
> > +       struct drm_i915_query_guc_submission_version __user *query_ptr =
> > + u64_to_user_ptr(query->data_ptr);
> > +       struct drm_i915_query_guc_submission_version ver;
> > +       struct intel_guc *guc = &to_gt(i915)->uc.guc;
> > +       const size_t size = sizeof(ver);
> > +       int ret;
> > +
> > +       if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
> > +               return -ENODEV;
> > +
> > +       ret = copy_query_item(&ver, size, size, query);
> > +       if (ret != 0)
> > +               return ret;
> > +
> > +       if (ver.major || ver.minor || ver.patch)
> > +               return -EINVAL;
> > +
> > +       ver.major = guc->submission_version.major;
> > +       ver.minor = guc->submission_version.minor;
> > +       ver.patch = guc->submission_version.patch;
> > +
> > +       if (copy_to_user(query_ptr, &ver, 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 +590,7 @@ static int (* const i915_query_funcs[])(struct 
> > drm_i915_private *dev_priv,
> >         query_memregion_info,
> >         query_hwconfig_blob,
> >         query_geometry_subslices,
> > +       query_guc_submission_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 550c496ce76d..d80d9b5e1eda 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -3038,6 +3038,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_GUC_SUBMISSION_VERSION (see struct 
> > drm_i915_query_guc_submission_version)
> >          */
> >         __u64 query_id;
> >  #define DRM_I915_QUERY_TOPOLOGY_INFO           1
> > @@ -3046,6 +3047,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_GUC_SUBMISSION_VERSION  7
> >  /* Must be kept compact -- no holes and well documented */
> > 
> >         /**
> > @@ -3591,6 +3593,15 @@ struct drm_i915_query_memory_regions {
> >         struct drm_i915_memory_region_info regions[];
> >  };
> > 
> > +/**
> > +* struct drm_i915_query_guc_submission_version - query GuC submission 
> > interface version
> > +*/
> > +struct drm_i915_query_guc_submission_version {
> > +       __u64 major;
> > +       __u64 minor;
> > +       __u64 patch;
> > +};
> > +
> >  /**
> >   * DOC: GuC HWCONFIG blob uAPI
> >   *
> > 
> > It is not that much bigger that the triple get param and IMO nicer.
> > 
> > But if there is no motivation to do it properly then feel free to 
> > proceed with this, I will not block it.
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > P.S.
> > Probably still make sure to remove the reference to SR-IOV.
> > 
> > > 
> > > > 
> > > > During the time of that patch there was discussion whether firmware
> > > > version or submission version was better. I vaguely remember someone
> > > > raised an issue with the latter. Adding John in case he remembers.
> > > > 
> > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > > > Cc: Jose Souza <jose.souza@intel.com>
> > > > > Cc: Sagar Ghuge <sagar.ghuge@intel.com>
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Cc: John Harrison <John.C.Harrison@Intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/i915_getparam.c | 12 ++++++++++++
> > > > >    include/uapi/drm/i915_drm.h          | 13 +++++++++++++
> > > > >    2 files changed, 25 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c 
> > > > > b/drivers/gpu/drm/i915/i915_getparam.c
> > > > > index 5c3fec63cb4c1..f176372debc54 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > > > > @@ -113,6 +113,18 @@ int i915_getparam_ioctl(struct drm_device 
> > > > > *dev, void *data,
> > > > >            if (value < 0)
> > > > >                return value;
> > > > >            break;
> > > > > +    case I915_PARAM_GUC_SUBMISSION_VERSION_MAJOR:
> > > > > +    case I915_PARAM_GUC_SUBMISSION_VERSION_MINOR:
> > > > > +    case I915_PARAM_GUC_SUBMISSION_VERSION_PATCH:
> > > > > +        if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
> > > > > +            return -ENODEV;
> > > > > +        if (param->param == I915_PARAM_GUC_SUBMISSION_VERSION_MAJOR)
> > > > > +            value = to_gt(i915)->uc.guc.submission_version.major;
> > > > > +        else if (param->param == 
> > > > > I915_PARAM_GUC_SUBMISSION_VERSION_MINOR)
> > > > > +            value = to_gt(i915)->uc.guc.submission_version.minor;
> > > > > +        else
> > > > > +            value = to_gt(i915)->uc.guc.submission_version.patch;
> > > > > +        break;
> > > > >        case I915_PARAM_MMAP_GTT_VERSION:
> > > > >            /* Though we've started our numbering from 1, and so 
> > > > > class all
> > > > >             * earlier versions as 0, in effect their value is 
> > > > > undefined as
> > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > > > index fd4f9574d177a..7d5a47f182542 100644
> > > > > --- a/include/uapi/drm/i915_drm.h
> > > > > +++ b/include/uapi/drm/i915_drm.h
> > > > > @@ -806,6 +806,19 @@ typedef struct drm_i915_irq_wait {
> > > > >     */
> > > > >    #define I915_PARAM_PXP_STATUS         58
> > > > >    +/*
> > > > > + * Query for the GuC submission/VF interface version number
> > > > 
> > > > What is this VF you speak of? :/
> > > > 
> > > > Regards,
> > > > 
> > > > Tvrtko
> > > > 
> > > > > + *
> > > > > + * -ENODEV is returned if GuC submission is not used
> > > > > + *
> > > > > + * On success, returns the respective GuC submission/VF interface 
> > > > > major,
> > > > > + * minor or patch version as per the requested parameter.
> > > > > + *
> > > > > + */
> > > > > +#define I915_PARAM_GUC_SUBMISSION_VERSION_MAJOR 59
> > > > > +#define I915_PARAM_GUC_SUBMISSION_VERSION_MINOR 60
> > > > > +#define I915_PARAM_GUC_SUBMISSION_VERSION_PATCH 61
> > > > > +
> > > > >    /* Must be kept compact -- no holes and well documented */
> > > > >       /**
> > > 
>