On Wed, 2024-02-07 at 13:36 +0200, Joonas Lahtinen wrote: > Quoting Tvrtko Ursulin (2024-02-07 10:44:01) > > > > On 06/02/2024 20:51, Souza, Jose wrote: > > > 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? > > > > Note there was review feedback not addressed so do that too please. > > AFAIR incorrect usage of copy item, pad/rsvd/mbz checking and questions > > about padding in general. Last is why I proposed a simplified version > > which is not future extensible and avoids the need for padding. > > Yeah, I don't think there is point an adding an extensible interface as > we're not going to add further FW version queries. This only the > submission interface version we're going to expose: > > * Note that the spec for the CSS header defines this version number > * as 'vf_version' as it was originally intended for virtualisation. > * However, it is applicable to native submission as well. > > If somebody wants to work on the simplified version like Tvrtko > suggested below, I have no objection. We can also remove the reference > to the VF version even if that's used by the header definition. > > But if there are just suggestions but no actual patches floated, then we > should be merging the GETPARAM version with the "VF" word removed. > > We've already discussed on the topic for some months so doing the > minimal changes to fulfill Mesa requirements should be considered a > priority to avoid further delays. This is Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx> Tested-by: José Roberto de Souza <jose.souza@xxxxxxxxx> Here the user-space usage: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25233 > > > > > Regards, > > > > Tvrtko > > > > > > > > > > > > > > > > And note that it is four calls not three. The code below is missing the > > > > branch version number. > > Not even kernel uses the 'build' version anywhere. I don't see how there > could be 'build' version for the VF interface version? It's not supposed > to version a concrete firmware build but the API contract implemented by > the build where patch version should already be incremented for each > fix. > > So adding the build does not seem appropriate as there is no plan to > extend this API any further. > > Regards, Joonas > > > > > > > > > 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@xxxxxxxxxxxxxxx> > > > > > > > > Cc: Kenneth Graunke <kenneth@xxxxxxxxxxxxx> > > > > > > > > Cc: Jose Souza <jose.souza@xxxxxxxxx> > > > > > > > > Cc: Sagar Ghuge <sagar.ghuge@xxxxxxxxx> > > > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > > > > > > > Cc: John Harrison <John.C.Harrison@xxxxxxxxx> > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > > > > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > > > > > > > --- > > > > > > > > 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 */ > > > > > > > > /** > > > > > > > > > > > > >
