* [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
@ 2024-01-24 8:19 Joonas Lahtinen
2024-01-24 8:43 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Joonas Lahtinen @ 2024-01-24 8:19 UTC (permalink / raw)
To: Intel graphics driver community testing & development
Cc: Paulo Zanoni, Tvrtko Ursulin, Jani Nikula, Kenneth Graunke,
Sagar Ghuge, Rodrigo Vivi
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.
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
+ *
+ * -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 */
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:19 [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version Joonas Lahtinen
@ 2024-01-24 8:43 ` Patchwork
2024-01-24 8:49 ` ✗ Fi.CI.BAT: failure " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2024-01-24 8:43 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add GETPARAM for GuC submission version
URL : https://patchwork.freedesktop.org/series/129122/
State : warning
== Summary ==
Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:19 [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version Joonas Lahtinen
2024-01-24 8:43 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
@ 2024-01-24 8:49 ` Patchwork
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
2024-01-24 13:35 ` Souza, Jose
3 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2024-01-24 8:49 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 10706 bytes --]
== Series Details ==
Series: drm/i915: Add GETPARAM for GuC submission version
URL : https://patchwork.freedesktop.org/series/129122/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_14168 -> Patchwork_129122v1
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_129122v1 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_129122v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/index.html
Participating hosts (38 -> 37)
------------------------------
Additional (1): bat-kbl-2
Missing (2): fi-snb-2520m fi-pnv-d510
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_129122v1:
### IGT changes ###
#### Possible regressions ####
* igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
- fi-tgl-1115g4: [PASS][1] -> [SKIP][2] +5 other tests skip
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
* igt@kms_pipe_crc_basic@hang-read-crc:
- fi-tgl-1115g4: NOTRUN -> [SKIP][3] +6 other tests skip
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_pipe_crc_basic@hang-read-crc.html
#### Warnings ####
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4: [SKIP][4] ([i915#4103]) -> [SKIP][5] +1 other test skip
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_dsc@dsc-basic:
- fi-tgl-1115g4: [SKIP][6] ([i915#3555] / [i915#3840]) -> [SKIP][7]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html
Known issues
------------
Here are the changes found in Patchwork_129122v1 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@fbdev@info:
- bat-kbl-2: NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#1849])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-kbl-2/igt@fbdev@info.html
- fi-tgl-1115g4: [PASS][9] -> [SKIP][10] ([i915#1849] / [i915#2582])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@fbdev@info.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@fbdev@info.html
* igt@fbdev@nullptr:
- fi-tgl-1115g4: [PASS][11] -> [SKIP][12] ([i915#2582]) +3 other tests skip
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@fbdev@nullptr.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@fbdev@nullptr.html
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#4613]) +3 other tests skip
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-apl-guc/igt@gem_lmem_swapping@basic.html
* igt@gem_lmem_swapping@parallel-random-engines:
- bat-kbl-2: NOTRUN -> [SKIP][14] ([fdo#109271]) +35 other tests skip
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html
* igt@gem_lmem_swapping@verify-random:
- bat-mtlp-8: NOTRUN -> [SKIP][15] ([i915#4613]) +3 other tests skip
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html
* igt@i915_pm_rps@basic-api:
- bat-mtlp-8: NOTRUN -> [SKIP][16] ([i915#6621])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@i915_pm_rps@basic-api.html
* igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][17] ([i915#5190])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
* igt@kms_addfb_basic@basic-y-tiled-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][18] ([i915#4212]) +8 other tests skip
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html
* igt@kms_busy@basic:
- fi-tgl-1115g4: NOTRUN -> [SKIP][19] ([i915#4303])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_busy@basic.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][20] ([i915#4213]) +1 other test skip
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
* igt@kms_dsc@dsc-basic:
- bat-mtlp-8: NOTRUN -> [SKIP][21] ([i915#3555] / [i915#3840] / [i915#9159])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_dsc@dsc-basic.html
* igt@kms_flip@basic-plain-flip:
- fi-tgl-1115g4: NOTRUN -> [SKIP][22] ([i915#3637]) +3 other tests skip
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_flip@basic-plain-flip.html
* igt@kms_force_connector_basic@force-load-detect:
- bat-mtlp-8: NOTRUN -> [SKIP][23] ([fdo#109285])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-mtlp-8: NOTRUN -> [SKIP][24] ([i915#5274])
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_frontbuffer_tracking@basic:
- fi-tgl-1115g4: [PASS][25] -> [SKIP][26] ([i915#1849])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_frontbuffer_tracking@basic.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@kms_frontbuffer_tracking@basic.html
* igt@kms_hdmi_inject@inject-audio:
- fi-apl-guc: NOTRUN -> [SKIP][27] ([fdo#109271]) +13 other tests skip
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-apl-guc/igt@kms_hdmi_inject@inject-audio.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-mtlp-8: NOTRUN -> [SKIP][28] ([i915#3555] / [i915#8809])
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-fence-flip:
- fi-tgl-1115g4: [PASS][29] -> [SKIP][30] ([fdo#109295])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@prime_vgem@basic-fence-flip.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/fi-tgl-1115g4/igt@prime_vgem@basic-fence-flip.html
* igt@prime_vgem@basic-fence-mmap:
- bat-mtlp-8: NOTRUN -> [SKIP][31] ([i915#3708] / [i915#4077]) +1 other test skip
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html
* igt@prime_vgem@basic-fence-read:
- bat-mtlp-8: NOTRUN -> [SKIP][32] ([i915#3708]) +2 other tests skip
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html
#### Possible fixes ####
* igt@i915_hangman@error-state-basic:
- bat-mtlp-8: [ABORT][33] ([i915#9414]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/bat-mtlp-8/igt@i915_hangman@error-state-basic.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/bat-mtlp-8/igt@i915_hangman@error-state-basic.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4303]: https://gitlab.freedesktop.org/drm/intel/issues/4303
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
[i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
[i915#9414]: https://gitlab.freedesktop.org/drm/intel/issues/9414
[i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
Build changes
-------------
* Linux: CI_DRM_14168 -> Patchwork_129122v1
CI-20190529: 20190529
CI_DRM_14168: 0496fe20d8ed5e73ed186d293ef67e7bae4658eb @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7690: aa45298ff675abbe6bf8f04ae186e2388c35f03a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_129122v1: 0496fe20d8ed5e73ed186d293ef67e7bae4658eb @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
eed8c4ce7e14 drm/i915: Add GETPARAM for GuC submission version
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129122v1/index.html
[-- Attachment #2: Type: text/html, Size: 12568 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:19 [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version Joonas Lahtinen
2024-01-24 8:43 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
2024-01-24 8:49 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2024-01-24 8:55 ` Tvrtko Ursulin
2024-01-24 19:55 ` John Harrison
` (2 more replies)
2024-01-24 13:35 ` Souza, Jose
3 siblings, 3 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2024-01-24 8:55 UTC (permalink / raw)
To: Joonas Lahtinen,
Intel graphics driver community testing & development
Cc: Paulo Zanoni, Tvrtko Ursulin, Jani Nikula, Kenneth Graunke,
Rodrigo Vivi, Sagar Ghuge
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.
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 */
>
> /**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:19 [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version Joonas Lahtinen
` (2 preceding siblings ...)
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
@ 2024-01-24 13:35 ` Souza, Jose
3 siblings, 0 replies; 15+ messages in thread
From: Souza, Jose @ 2024-01-24 13:35 UTC (permalink / raw)
To: joonas.lahtinen@linux.intel.com, intel-gfx@lists.freedesktop.org
Cc: Zanoni, Paulo R, Ursulin, Tvrtko, Nikula, Jani,
kenneth@whitecape.org, Ghuge, Sagar, Vivi, Rodrigo
On Wed, 2024-01-24 at 10:19 +0200, 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.
>
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 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
> + *
> + * -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 */
>
> /**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
@ 2024-01-24 19:55 ` John Harrison
2024-01-24 22:42 ` Kenneth Graunke
2024-02-01 18:25 ` Souza, Jose
2 siblings, 0 replies; 15+ messages in thread
From: John Harrison @ 2024-01-24 19:55 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen,
Intel graphics driver community testing & development
Cc: Paulo Zanoni, Tvrtko Ursulin, Jani Nikula, Kenneth Graunke,
Rodrigo Vivi
On 1/24/2024 00:55, 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.
I also think that the original version is a cleaner implementation.
>
> 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.
The file version number should not be exposed to UMDs, only the
submission version. The whole purpose of the submission version is to
track user facing changes. There was a very, very, very long discussion
about that to which all parties did eventually agree on using the
submission version.
The outstanding issues were simply a) whether UMDs should be tracking
version numbers and all the complications that arise with branching and
non-linear numbering, b) should it just be exposed as a feature flag
instead and c) this will prevent hangs in certain specific situations
but it won't prevent the system running slowly and not using the full
capabilities of the hardware, for that we need to be making sure that
distros actually update to a firmware release that is not ancient.
>
>> 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? :/
Agreed. There is no SRIOV support in i915 so i915 should not be
mentioning SRIOV specific features.
John.
>
> 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 */
>> /**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
2024-01-24 19:55 ` John Harrison
@ 2024-01-24 22:42 ` Kenneth Graunke
2024-02-01 18:25 ` Souza, Jose
2 siblings, 0 replies; 15+ messages in thread
From: Kenneth Graunke @ 2024-01-24 22:42 UTC (permalink / raw)
To: Joonas Lahtinen,
Intel graphics driver community testing & development,
Tvrtko Ursulin
Cc: Paulo Zanoni, Tvrtko Ursulin, Jani Nikula, Rodrigo Vivi
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
On Wednesday, January 24, 2024 12:55:56 AM PST 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.
Joonas's patch posted here is:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
I'm fine with the approach that Tvrtko linked as well, querying all
three in one ioctl makes some sense. That particular patch looked like
it needed some (trivial) cleaning up before landing, though. Either
approach works for me.
I agree with John, the submission version should be fine for us.
Thanks a lot for taking care of this.
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
2024-01-24 19:55 ` John Harrison
2024-01-24 22:42 ` Kenneth Graunke
@ 2024-02-01 18:25 ` Souza, Jose
2024-02-06 16:33 ` Tvrtko Ursulin
2 siblings, 1 reply; 15+ messages in thread
From: Souza, Jose @ 2024-02-01 18:25 UTC (permalink / raw)
To: joonas.lahtinen@linux.intel.com, tvrtko.ursulin@linux.intel.com,
intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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.
>
> 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 */
> >
> > /**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-01 18:25 ` Souza, Jose
@ 2024-02-06 16:33 ` Tvrtko Ursulin
2024-02-06 20:42 ` John Harrison
0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2024-02-06 16:33 UTC (permalink / raw)
To: Souza, Jose, joonas.lahtinen@linux.intel.com,
intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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:
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 */
>>>
>>> /**
>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-06 16:33 ` Tvrtko Ursulin
@ 2024-02-06 20:42 ` John Harrison
2024-02-06 20:51 ` Souza, Jose
0 siblings, 1 reply; 15+ messages in thread
From: John Harrison @ 2024-02-06 20:42 UTC (permalink / raw)
To: Tvrtko Ursulin, Souza, Jose, joonas.lahtinen@linux.intel.com,
intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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.
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 */
>>>> /**
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-06 20:42 ` John Harrison
@ 2024-02-06 20:51 ` Souza, Jose
2024-02-07 8:44 ` Tvrtko Ursulin
0 siblings, 1 reply; 15+ messages in thread
From: Souza, Jose @ 2024-02-06 20:51 UTC (permalink / raw)
To: Harrison, John C, joonas.lahtinen@linux.intel.com,
tvrtko.ursulin@linux.intel.com, intel-gfx@lists.freedesktop.org,
Balasubrawmanian, Vivaik
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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 */
> > > > > /**
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-06 20:51 ` Souza, Jose
@ 2024-02-07 8:44 ` Tvrtko Ursulin
2024-02-07 11:36 ` Joonas Lahtinen
0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2024-02-07 8:44 UTC (permalink / raw)
To: Souza, Jose, Harrison, John C, joonas.lahtinen@linux.intel.com,
intel-gfx@lists.freedesktop.org, Balasubrawmanian, Vivaik
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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.
Regards,
Tvrtko
>
>
>>
>> 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 */
>>>>>> /**
>>>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-07 8:44 ` Tvrtko Ursulin
@ 2024-02-07 11:36 ` Joonas Lahtinen
2024-02-07 17:58 ` John Harrison
2024-02-07 18:02 ` Souza, Jose
0 siblings, 2 replies; 15+ messages in thread
From: Joonas Lahtinen @ 2024-02-07 11:36 UTC (permalink / raw)
To: Balasubrawmanian, Vivaik, Harrison, John C, Souza, Jose,
intel-gfx@lists.freedesktop.org, Tvrtko Ursulin
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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.
>
> 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@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 */
> >>>>>> /**
> >>>>
> >>
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-07 11:36 ` Joonas Lahtinen
@ 2024-02-07 17:58 ` John Harrison
2024-02-07 18:02 ` Souza, Jose
1 sibling, 0 replies; 15+ messages in thread
From: John Harrison @ 2024-02-07 17:58 UTC (permalink / raw)
To: Joonas Lahtinen, Balasubrawmanian, Vivaik, Souza, Jose,
intel-gfx@lists.freedesktop.org, Tvrtko Ursulin
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
On 2/7/2024 03:36, 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:
The media team have flip flopped multiple times about whether they need
a HuC version query.
>
> * 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.
The original patch was posted to the mailing list many months ago. Why
do you say 'just suggestions but no patches floated'?
>
> 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.
>
>> 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.
I did not say "build" version. There is no build version. I said
"branch" version. And the branch version absolute becomes important if
we ever have to release a bug fix to an LTS branch. So it needs to be
part of the interface from the beginning and the UMDs need to be using
it from the beginning.
John.
>
> 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@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 */
>>>>>>>> /**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
2024-02-07 11:36 ` Joonas Lahtinen
2024-02-07 17:58 ` John Harrison
@ 2024-02-07 18:02 ` Souza, Jose
1 sibling, 0 replies; 15+ messages in thread
From: Souza, Jose @ 2024-02-07 18:02 UTC (permalink / raw)
To: joonas.lahtinen@linux.intel.com, Harrison, John C,
tvrtko.ursulin@linux.intel.com, Balasubrawmanian, Vivaik,
intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko, Nikula, Jani, kenneth@whitecape.org,
Vivi, Rodrigo, Zanoni, Paulo R, Ghuge, Sagar
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@intel.com>
Tested-by: José Roberto de Souza <jose.souza@intel.com>
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@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 */
> > > > > > > > /**
> > > > > >
> > > >
> > >
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-02-07 18:02 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-24 8:19 [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version Joonas Lahtinen
2024-01-24 8:43 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
2024-01-24 8:49 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-01-24 8:55 ` [RFC PATCH] " Tvrtko Ursulin
2024-01-24 19:55 ` John Harrison
2024-01-24 22:42 ` Kenneth Graunke
2024-02-01 18:25 ` Souza, Jose
2024-02-06 16:33 ` Tvrtko Ursulin
2024-02-06 20:42 ` John Harrison
2024-02-06 20:51 ` Souza, Jose
2024-02-07 8:44 ` Tvrtko Ursulin
2024-02-07 11:36 ` Joonas Lahtinen
2024-02-07 17:58 ` John Harrison
2024-02-07 18:02 ` Souza, Jose
2024-01-24 13:35 ` Souza, Jose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.