[PATCH i-g-t,v1 0/4] L3 bank mask

[Lucas De Marchi]

On Wed, Apr 17, 2024 at 02:50:45PM GMT, Francois Dugast wrote:
>Align the XeKMD header with the current version and add the L3 bank mask
>topology query proposed in this KMD series:
>https://patchwork.freedesktop.org/series/132075/
>
>This series first includes some patches to align the XeKMD header which
>have already been reviewed but not yet applied, from this other series:
>https://patchwork.freedesktop.org/series/131816
>
>Francois Dugast (4):
>  drm-uapi/xe: Align header with drm-xe-next
>  drm-uapi/xe: Define topology types as indexes rather than masks
>  drm-uapi/xe: Expose the L3 bank mask

no, I don't think we want to merge these as is in this series. It can be
used for premerge testing, but this is just reintroducing the same
problem fixed in patch 1.

I applied patch 1 locally and then did a diff with the current
drm-xe-next:

diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h
index 4353595a4..0b709b374 100644
--- a/include/drm-uapi/xe_drm.h
+++ b/include/drm-uapi/xe_drm.h
@@ -518,9 +518,9 @@ struct drm_xe_query_topology_mask {
         /** @gt_id: GT ID the mask is associated with */
         __u16 gt_id;

-#define DRM_XE_TOPO_DSS_GEOMETRY       (1 << 0)
-#define DRM_XE_TOPO_DSS_COMPUTE                (1 << 1)
-#define DRM_XE_TOPO_EU_PER_DSS         (1 << 2)
+#define DRM_XE_TOPO_DSS_GEOMETRY       1
+#define DRM_XE_TOPO_DSS_COMPUTE                2
+#define DRM_XE_TOPO_EU_PER_DSS         4
         /** @type: type of mask */
         __u16 type;

@@ -871,10 +871,12 @@ struct drm_xe_vm_destroy {
   *  - %DRM_XE_VM_BIND_OP_PREFETCH
   *
   * and the @flags can be:
- *  - %DRM_XE_VM_BIND_FLAG_READONLY
- *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - Valid on a faulting VM only, do the
+ *  - %DRM_XE_VM_BIND_FLAG_READONLY - Setup the page tables as read-only
+ *    to ensure write protection
+ *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - On a faulting VM, do the
   *    MAP operation immediately rather than deferring the MAP to the page
- *    fault handler.
+ *    fault handler. This is implied on a non-faulting VM as there is no
+ *    fault handler to defer to.
   *  - %DRM_XE_VM_BIND_FLAG_NULL - When the NULL flag is set, the page
   *    tables are setup with a special bit which indicates writes are
   *    dropped and all reads return zero. In the future, the NULL flags
@@ -1085,19 +1087,6 @@ struct drm_xe_exec_queue_create {
  #define DRM_XE_EXEC_QUEUE_EXTENSION_SET_PROPERTY               0
  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY              0
  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE             1
-#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PREEMPTION_TIMEOUT    2
-#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_JOB_TIMEOUT           4
-#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_TRIGGER           5
-#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_NOTIFY            6
-#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_GRANULARITY       7
-/* Monitor 128KB contiguous region with 4K sub-granularity */
-#define     DRM_XE_ACC_GRANULARITY_128K                                0
-/* Monitor 2MB contiguous region with 64KB sub-granularity */
-#define     DRM_XE_ACC_GRANULARITY_2M                          1
-/* Monitor 16MB contiguous region with 512KB sub-granularity */
-#define     DRM_XE_ACC_GRANULARITY_16M                         2
-/* Monitor 64MB contiguous region with 2M sub-granularity */
-#define     DRM_XE_ACC_GRANULARITY_64M                         3

         /** @extensions: Pointer to the first extension struct, if any */

I seems we don't need patch 1 and 2. We only need the commit used to be
closer to drm-xe-next.

include/drm-uapi/xe_drm.h should always and only be updated by copying
from kernel from specific commits documented in the commit message.

WIP changes while the kernel side is not merged should be moved to
another header. Traditionally we used LOCAL_* defines for that. But that
comes with its own problems as it's more work to  go find them all and
drop when we sync the headers.

What about doing this?

Add include/drm-uapi/local/ to -I in build system *before* the non-local
one. Something like:

meson.build:
	inc = include_directories('include', 'include/drm-uapi/local' 'include/drm-uapi', 'include/linux-uapi', 'lib', 'lib/stubs/syscalls', '.')

Example content for
include/drm-uapi/local/drm_xe.h:
	#pragma once
	#include_next <xe_drm.h>

	/*
	 * Local definitions - delta with WIP UAPI while the upstream defines
	 * are not yet merged. These are supposed to conflict whenever the
	 * upstream header is updated and break the build. That is intended.
	 * Commit updating the upstream header should garbage collect these
	 * local definitions.
	 */

	#ifdef DRM_XE_TOPO_L3_BANK
	#error "Already defined by upstream - remove"
	#define DRM_XE_TOPO_L3_BANK		3
	#endif

The ifdef + error could be optional as it's harmless (and allowed by
compiler) to redefine it multiple times to the same value.

Thoughts?

If we don't want to decide on this and you only want to unblock this
test for the kernel addition, then maybe just do the same LOCAL_
approach currently being used. See e.g. lib/i915/i915_drm_local.h

Lucas De Marchi

>  lib/xe/xe_query: Add L3 bank mask test
>
> include/drm-uapi/xe_drm.h | 27 +++-------------
> tests/intel/xe_query.c    | 66 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 71 insertions(+), 22 deletions(-)
>
>-- 
>2.34.1
>