[Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Francois Dugast]

This series addresses some feedback on xe_drm.h provided here:
https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html

It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
likely break CI.

v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)

Francois Dugast (5):
  drm/xe: Use SPDX-License-Identifier instead of license text
  drm/xe: Group engine related structs
  drm/xe: Move defines before relevant fields
  drm/xe: Document usage of struct drm_xe_device_query
  drm/xe: Fix some formatting issues in uAPI

 include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
 1 file changed, 90 insertions(+), 81 deletions(-)

-- 
2.34.1

[Intel-xe] [PATCH v2 1/5] drm/xe: Use SPDX-License-Identifier instead of license text

[Francois Dugast]

Replace the license text with its SPDX-License-Identifier for
quick identification of the license and consistency with the
rest of the driver.

Reported-by: Oded Gabbay <ogabbay@kernel.org>
Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 include/uapi/drm/xe_drm.h | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index b0b80aae3ee8..ac3cfc1fb4a5 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -1,26 +1,6 @@
+/* SPDX-License-Identifier: MIT */
 /*
- * Copyright 2021 Intel Corporation. All Rights Reserved.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the
- * "Software"), to deal in the Software without restriction, including
- * without limitation the rights to use, copy, modify, merge, publish,
- * distribute, sub license, and/or sell copies of the Software, and to
- * permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
- * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
- * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
- * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
- * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
- * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
- *
+ * Copyright © 2023 Intel Corporation
  */
 
 #ifndef _UAPI_XE_DRM_H_
-- 
2.34.1

[Intel-xe] [PATCH v2 2/5] drm/xe: Group engine related structs

[Francois Dugast]

Move the definition of drm_xe_engine_class_instance to group it with
other engine related structs and to follow the ioctls order.

Reported-by: Oded Gabbay <ogabbay@kernel.org>
Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 include/uapi/drm/xe_drm.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index ac3cfc1fb4a5..5d34b570a305 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -116,24 +116,6 @@ struct xe_user_extension {
 #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
 #define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
 
-struct drm_xe_engine_class_instance {
-	__u16 engine_class;
-
-#define DRM_XE_ENGINE_CLASS_RENDER		0
-#define DRM_XE_ENGINE_CLASS_COPY		1
-#define DRM_XE_ENGINE_CLASS_VIDEO_DECODE	2
-#define DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE	3
-#define DRM_XE_ENGINE_CLASS_COMPUTE		4
-	/*
-	 * Kernel only class (not actual hardware engine class). Used for
-	 * creating ordered queues of VM bind operations.
-	 */
-#define DRM_XE_ENGINE_CLASS_VM_BIND		5
-
-	__u16 engine_instance;
-	__u16 gt_id;
-};
-
 #define XE_MEM_REGION_CLASS_SYSMEM	0
 #define XE_MEM_REGION_CLASS_VRAM	1
 
@@ -518,6 +500,24 @@ struct drm_xe_engine_set_property {
 	__u64 reserved[2];
 };
 
+struct drm_xe_engine_class_instance {
+	__u16 engine_class;
+
+#define DRM_XE_ENGINE_CLASS_RENDER		0
+#define DRM_XE_ENGINE_CLASS_COPY		1
+#define DRM_XE_ENGINE_CLASS_VIDEO_DECODE	2
+#define DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE	3
+#define DRM_XE_ENGINE_CLASS_COMPUTE		4
+	/*
+	 * Kernel only class (not actual hardware engine class). Used for
+	 * creating ordered queues of VM bind operations.
+	 */
+#define DRM_XE_ENGINE_CLASS_VM_BIND		5
+
+	__u16 engine_instance;
+	__u16 gt_id;
+};
+
 struct drm_xe_engine_create {
 	/** @extensions: Pointer to the first extension struct, if any */
 #define XE_ENGINE_EXTENSION_SET_PROPERTY               0
-- 
2.34.1

[Intel-xe] [PATCH v2 3/5] drm/xe: Move defines before relevant fields

[Francois Dugast]

Align on same rule in the whole file: defines then doc then relevant
field.

Reported-by: Oded Gabbay <ogabbay@kernel.org>
Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 include/uapi/drm/xe_drm.h | 62 ++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 5d34b570a305..2eea80bf0e06 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -138,6 +138,7 @@ struct drm_xe_query_mem_usage {
 struct drm_xe_query_config {
 	__u32 num_params;
 	__u32 pad;
+
 #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
 #define XE_QUERY_CONFIG_FLAGS			1
 	#define XE_QUERY_CONFIG_FLAGS_HAS_VRAM		(0x1 << 0)
@@ -179,11 +180,11 @@ struct drm_xe_query_topology_mask {
 	/** @gt_id: GT ID the mask is associated with */
 	__u16 gt_id;
 
-	/** @type: type of mask */
-	__u16 type;
 #define XE_TOPO_DSS_GEOMETRY	(1 << 0)
 #define XE_TOPO_DSS_COMPUTE	(1 << 1)
 #define XE_TOPO_EU_PER_DSS	(1 << 2)
+	/** @type: type of mask */
+	__u16 type;
 
 	/** @num_bytes: number of bytes in requested mask */
 	__u32 num_bytes;
@@ -196,15 +197,14 @@ struct drm_xe_device_query {
 	/** @extensions: Pointer to the first extension struct, if any */
 	__u64 extensions;
 
-	/** @query: The type of data to query */
-	__u32 query;
-
 #define DRM_XE_DEVICE_QUERY_ENGINES	0
 #define DRM_XE_DEVICE_QUERY_MEM_USAGE	1
 #define DRM_XE_DEVICE_QUERY_CONFIG	2
 #define DRM_XE_DEVICE_QUERY_GTS		3
 #define DRM_XE_DEVICE_QUERY_HWCONFIG	4
 #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY	5
+	/** @query: The type of data to query */
+	__u32 query;
 
 	/** @size: Size of the queried data */
 	__u32 size;
@@ -227,12 +227,12 @@ struct drm_xe_gem_create {
 	 */
 	__u64 size;
 
+#define XE_GEM_CREATE_FLAG_DEFER_BACKING	(0x1 << 24)
+#define XE_GEM_CREATE_FLAG_SCANOUT		(0x1 << 25)
 	/**
 	 * @flags: Flags, currently a mask of memory instances of where BO can
 	 * be placed
 	 */
-#define XE_GEM_CREATE_FLAG_DEFER_BACKING	(0x1 << 24)
-#define XE_GEM_CREATE_FLAG_SCANOUT		(0x1 << 25)
 	__u32 flags;
 
 	/**
@@ -293,8 +293,8 @@ struct drm_xe_ext_vm_set_property {
 	/** @base: base user extension */
 	struct xe_user_extension base;
 
-	/** @property: property to set */
 #define XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS		0
+	/** @property: property to set */
 	__u32 property;
 
 	/** @value: property value */
@@ -305,17 +305,17 @@ struct drm_xe_ext_vm_set_property {
 };
 
 struct drm_xe_vm_create {
-	/** @extensions: Pointer to the first extension struct, if any */
+
 #define XE_VM_EXTENSION_SET_PROPERTY	0
+	/** @extensions: Pointer to the first extension struct, if any */
 	__u64 extensions;
 
-	/** @flags: Flags */
-	__u32 flags;
-
 #define DRM_XE_VM_CREATE_SCRATCH_PAGE	(0x1 << 0)
 #define DRM_XE_VM_CREATE_COMPUTE_MODE	(0x1 << 1)
 #define DRM_XE_VM_CREATE_ASYNC_BIND_OPS	(0x1 << 2)
 #define DRM_XE_VM_CREATE_FAULT_MODE	(0x1 << 3)
+	/** @flags: Flags */
+	__u32 flags;
 
 	/** @vm_id: Returned VM ID */
 	__u32 vm_id;
@@ -365,12 +365,6 @@ struct drm_xe_vm_bind_op {
 	 */
 	__u64 gt_mask;
 
-	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
-	__u32 op;
-
-	/** @mem_region: Memory region to prefetch VMA to, instance not a mask */
-	__u32 region;
-
 #define XE_VM_BIND_OP_MAP		0x0
 #define XE_VM_BIND_OP_UNMAP		0x1
 #define XE_VM_BIND_OP_MAP_USERPTR	0x2
@@ -409,6 +403,11 @@ struct drm_xe_vm_bind_op {
 	 * than differing the MAP to the page fault handler.
 	 */
 #define XE_VM_BIND_FLAG_IMMEDIATE	(0x1 << 18)
+	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
+	__u32 op;
+
+	/** @mem_region: Memory region to prefetch VMA to, instance not a mask */
+	__u32 region;
 
 	/** @reserved: Reserved */
 	__u64 reserved[2];
@@ -475,7 +474,6 @@ struct drm_xe_engine_set_property {
 	/** @engine_id: Engine ID */
 	__u32 engine_id;
 
-	/** @property: property to set */
 #define XE_ENGINE_SET_PROPERTY_PRIORITY			0
 #define XE_ENGINE_SET_PROPERTY_TIMESLICE		1
 #define XE_ENGINE_SET_PROPERTY_PREEMPTION_TIMEOUT	2
@@ -491,6 +489,7 @@ struct drm_xe_engine_set_property {
 #define XE_ENGINE_SET_PROPERTY_ACC_TRIGGER		6
 #define XE_ENGINE_SET_PROPERTY_ACC_NOTIFY		7
 #define XE_ENGINE_SET_PROPERTY_ACC_GRANULARITY		8
+	/** @property: property to set */
 	__u32 property;
 
 	/** @value: property value */
@@ -501,7 +500,6 @@ struct drm_xe_engine_set_property {
 };
 
 struct drm_xe_engine_class_instance {
-	__u16 engine_class;
 
 #define DRM_XE_ENGINE_CLASS_RENDER		0
 #define DRM_XE_ENGINE_CLASS_COPY		1
@@ -513,14 +511,16 @@ struct drm_xe_engine_class_instance {
 	 * creating ordered queues of VM bind operations.
 	 */
 #define DRM_XE_ENGINE_CLASS_VM_BIND		5
+	__u16 engine_class;
 
 	__u16 engine_instance;
 	__u16 gt_id;
 };
 
 struct drm_xe_engine_create {
-	/** @extensions: Pointer to the first extension struct, if any */
+
 #define XE_ENGINE_EXTENSION_SET_PROPERTY               0
+	/** @extensions: Pointer to the first extension struct, if any */
 	__u64 extensions;
 
 	/** @width: submission width (number BB per exec) for this engine */
@@ -558,8 +558,8 @@ struct drm_xe_engine_get_property {
 	/** @engine_id: Engine ID */
 	__u32 engine_id;
 
-	/** @property: property to get */
 #define XE_ENGINE_GET_PROPERTY_BAN			0
+	/** @property: property to get */
 	__u32 property;
 
 	/** @value: property value */
@@ -584,13 +584,12 @@ struct drm_xe_sync {
 	/** @extensions: Pointer to the first extension struct, if any */
 	__u64 extensions;
 
-	__u32 flags;
-
 #define DRM_XE_SYNC_SYNCOBJ		0x0
 #define DRM_XE_SYNC_TIMELINE_SYNCOBJ	0x1
 #define DRM_XE_SYNC_DMA_BUF		0x2
 #define DRM_XE_SYNC_USER_FENCE		0x3
 #define DRM_XE_SYNC_SIGNAL		0x10
+	__u32 flags;
 
 	union {
 		__u32 handle;
@@ -646,8 +645,6 @@ struct drm_xe_mmio {
 
 	__u32 addr;
 
-	__u32 flags;
-
 #define DRM_XE_MMIO_8BIT	0x0
 #define DRM_XE_MMIO_16BIT	0x1
 #define DRM_XE_MMIO_32BIT	0x2
@@ -655,6 +652,7 @@ struct drm_xe_mmio {
 #define DRM_XE_MMIO_BITS_MASK	0x3
 #define DRM_XE_MMIO_READ	0x4
 #define DRM_XE_MMIO_WRITE	0x8
+	__u32 flags;
 
 	__u64 value;
 
@@ -685,27 +683,32 @@ struct drm_xe_wait_user_fence {
 		 */
 		__u64 vm_id;
 	};
-	/** @op: wait operation (type of comparison) */
+
 #define DRM_XE_UFENCE_WAIT_EQ	0
 #define DRM_XE_UFENCE_WAIT_NEQ	1
 #define DRM_XE_UFENCE_WAIT_GT	2
 #define DRM_XE_UFENCE_WAIT_GTE	3
 #define DRM_XE_UFENCE_WAIT_LT	4
 #define DRM_XE_UFENCE_WAIT_LTE	5
+	/** @op: wait operation (type of comparison) */
 	__u16 op;
-	/** @flags: wait flags */
+
 #define DRM_XE_UFENCE_WAIT_SOFT_OP	(1 << 0)	/* e.g. Wait on VM bind */
 #define DRM_XE_UFENCE_WAIT_ABSTIME	(1 << 1)
 #define DRM_XE_UFENCE_WAIT_VM_ERROR	(1 << 2)
+	/** @flags: wait flags */
 	__u16 flags;
+
 	/** @value: compare value */
 	__u64 value;
-	/** @mask: comparison mask */
+
 #define DRM_XE_UFENCE_WAIT_U8		0xffu
 #define DRM_XE_UFENCE_WAIT_U16		0xffffu
 #define DRM_XE_UFENCE_WAIT_U32		0xffffffffu
 #define DRM_XE_UFENCE_WAIT_U64		0xffffffffffffffffu
+	/** @mask: comparison mask */
 	__u64 mask;
+
 	/** @timeout: how long to wait before bailing, value in jiffies */
 	__s64 timeout;
 	/**
@@ -770,7 +773,6 @@ struct drm_xe_vm_madvise {
 #define		DRM_XE_VMA_PRIORITY_HIGH	2	/* Must be elevated user */
 	/* Pin the VMA in memory, must be elevated user */
 #define DRM_XE_VM_MADVISE_PIN			6
-
 	/** @property: property to set */
 	__u32 property;
 
-- 
2.34.1

[Intel-xe] [PATCH v2 4/5] drm/xe: Document usage of struct drm_xe_device_query

[Francois Dugast]

Explain how to use struct drm_xe_device_query, in particular the behavior
for size.

Reported-by: Oded Gabbay <ogabbay@kernel.org>
Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 include/uapi/drm/xe_drm.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 2eea80bf0e06..e29399de148c 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -120,7 +120,10 @@ struct xe_user_extension {
 #define XE_MEM_REGION_CLASS_VRAM	1
 
 struct drm_xe_query_mem_usage {
+	/** @num_params: number of memory regions returned in regions */
 	__u32 num_regions;
+
+	/** @pad: MBZ */
 	__u32 pad;
 
 	struct drm_xe_query_mem_region {
@@ -136,7 +139,10 @@ struct drm_xe_query_mem_usage {
 };
 
 struct drm_xe_query_config {
+	/** @num_params: number of parameters returned in info */
 	__u32 num_params;
+
+	/** @pad: MBZ */
 	__u32 pad;
 
 #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
@@ -149,11 +155,15 @@ struct drm_xe_query_config {
 #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
 #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
 #define XE_QUERY_CONFIG_NUM_PARAM		XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
+	/** @info: array containing the config info below  */
 	__u64 info[];
 };
 
 struct drm_xe_query_gts {
+	/** @num_gt: number of GTs returned in gts */
 	__u32 num_gt;
+
+	/** @pad: MBZ */
 	__u32 pad;
 
 	/*
@@ -193,6 +203,23 @@ struct drm_xe_query_topology_mask {
 	__u8 mask[];
 };
 
+/**
+ * struct drm_xe_device_query - main structure to query device information
+ *
+ * If size is set to 0, the driver fills it with the required size for the
+ * requested type of data to query. If size is equal to the required size,
+ * the queried information is copied into data.
+ *
+ * A typical usage from user space is:
+ * - create a struct drm_xe_device_query with size = 0 and query = one of
+ *   the types DRM_XE_DEVICE_QUERY_*, for example DRM_XE_DEVICE_QUERY_ENGINES
+ * - call DRM_IOCTL_XE_DEVICE_QUERY
+ * - create a pointer to a query struct matching the query type selected
+ *   previously, for example struct drm_xe_engine_class_instance *
+ * - allocate memory of the size returned in drm_xe_device_query
+ * - call DRM_IOCTL_XE_DEVICE_QUERY
+ * - read the queried information in data
+ */
 struct drm_xe_device_query {
 	/** @extensions: Pointer to the first extension struct, if any */
 	__u64 extensions;
-- 
2.34.1

[Intel-xe] [PATCH v2 5/5] drm/xe: Fix some formatting issues in uAPI

[Francois Dugast]

Reported-by: Oded Gabbay <ogabbay@kernel.org>
Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 include/uapi/drm/xe_drm.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index e29399de148c..667cd60db3d5 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -105,16 +105,16 @@ struct xe_user_extension {
 #define DRM_IOCTL_XE_GEM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_CREATE, struct drm_xe_gem_create)
 #define DRM_IOCTL_XE_GEM_MMAP_OFFSET		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_MMAP_OFFSET, struct drm_xe_gem_mmap_offset)
 #define DRM_IOCTL_XE_VM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_VM_CREATE, struct drm_xe_vm_create)
-#define DRM_IOCTL_XE_VM_DESTROY			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
-#define DRM_IOCTL_XE_VM_BIND			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
+#define DRM_IOCTL_XE_VM_DESTROY			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
+#define DRM_IOCTL_XE_VM_BIND			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
 #define DRM_IOCTL_XE_ENGINE_CREATE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_CREATE, struct drm_xe_engine_create)
 #define DRM_IOCTL_XE_ENGINE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_GET_PROPERTY, struct drm_xe_engine_get_property)
-#define DRM_IOCTL_XE_ENGINE_DESTROY		DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
-#define DRM_IOCTL_XE_EXEC			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
+#define DRM_IOCTL_XE_ENGINE_DESTROY		 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
+#define DRM_IOCTL_XE_EXEC			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
 #define DRM_IOCTL_XE_MMIO			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_MMIO, struct drm_xe_mmio)
-#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
+#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
 #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
-#define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
+#define DRM_IOCTL_XE_VM_MADVISE			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
 
 #define XE_MEM_REGION_CLASS_SYSMEM	0
 #define XE_MEM_REGION_CLASS_VRAM	1
@@ -154,7 +154,7 @@ struct drm_xe_query_config {
 #define XE_QUERY_CONFIG_GT_COUNT		4
 #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
 #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
-#define XE_QUERY_CONFIG_NUM_PARAM		XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
+#define XE_QUERY_CONFIG_NUM_PARAM		(XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1)
 	/** @info: array containing the config info below  */
 	__u64 info[];
 };
@@ -411,8 +411,8 @@ struct drm_xe_vm_bind_op {
 	 * If this flag is clear and the IOCTL doesn't return an error, in
 	 * practice the bind op is good and will complete.
 	 *
-	 * If this flag is set and doesn't return return an error, the bind op
-	 * can still fail and recovery is needed. If configured, the bind op that
+	 * If this flag is set and doesn't return an error, the bind op can
+	 * still fail and recovery is needed. If configured, the bind op that
 	 * caused the error will be captured in drm_xe_vm_bind_op_error_capture.
 	 * Once the user sees the error (via a ufence +
 	 * XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS), it should free memory
@@ -651,9 +651,9 @@ struct drm_xe_exec {
 	__u64 syncs;
 
 	/**
-	  * @address: address of batch buffer if num_batch_buffer == 1 or an
-	  * array of batch buffer addresses
-	  */
+	 * @address: address of batch buffer if num_batch_buffer == 1 or an
+	 * array of batch buffer addresses
+	 */
 	__u64 address;
 
 	/**
-- 
2.34.1

[Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: uAPI cleanup (rev2)

[Patchwork]

== Series Details ==

Series: drm/xe: uAPI cleanup (rev2)
URL   : https://patchwork.freedesktop.org/series/118447/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: 90a956e56 drm/xe: Rename GPU offset helper to reflect true usage
=== git am output follows ===
error: patch failed: include/uapi/drm/xe_drm.h:293
error: include/uapi/drm/xe_drm.h: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/xe: Use SPDX-License-Identifier instead of license text
Applying: drm/xe: Group engine related structs
Applying: drm/xe: Move defines before relevant fields
Patch failed at 0003 drm/xe: Move defines before relevant fields
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Lucas De Marchi]

+Oded

On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
>This series addresses some feedback on xe_drm.h provided here:
>https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>
>It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
>likely break CI.

As I said on the previous reply, this is not going to fly. Now we have a
patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
*no recorded reviews*, doing things that already conflict with other
fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
the mmio refactor, the oob WAs, the probe refactor, etc etc.

Also, there are other fixes queued as fixups, which is still the process
being used in drm-xe-next. It's also where CI is running. Due to display
being moved up in the branch it's basically the only sane thing to do.

I think we need we are shooting ourselves on the foot and this review
process needs to be fixed / agreed upon before we create a mess out of
it.

Lucas De Marchi

>
>v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
>
>Francois Dugast (5):
>  drm/xe: Use SPDX-License-Identifier instead of license text
>  drm/xe: Group engine related structs
>  drm/xe: Move defines before relevant fields
>  drm/xe: Document usage of struct drm_xe_device_query
>  drm/xe: Fix some formatting issues in uAPI
>
> include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
> 1 file changed, 90 insertions(+), 81 deletions(-)
>
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Francois Dugast]

On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
> +Oded
> 
> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
> > This series addresses some feedback on xe_drm.h provided here:
> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> > 
> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
> > likely break CI.
> 
> As I said on the previous reply, this is not going to fly. Now we have a
> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
> *no recorded reviews*, doing things that already conflict with other
> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
> the mmio refactor, the oob WAs, the probe refactor, etc etc.
> 
> Also, there are other fixes queued as fixups, which is still the process
> being used in drm-xe-next. It's also where CI is running. Due to display
> being moved up in the branch it's basically the only sane thing to do.
> 
> I think we need we are shooting ourselves on the foot and this review
> process needs to be fixed / agreed upon before we create a mess out of
> it.
> 
> Lucas De Marchi

Yes I understand the concern. The intention is to address Oded's feedback on the
revision that was reviewed, so that the next review iteration is manageable and
not based on a moving target, which could lead to and endless effort.

Maybe I created confusion by sharing on the mailing list but at this stage the
idea is not to get this series merged, only to get Oded's validation that it
fixes the issues that he reported. Yes this means more work will be required to
rebase and create fixups later for drm-xe-next but it seems acceptable for the
work that has been done so far.

Do you see a better way that would still satisfy the "not moving target" goal?

Thanks,
Francois

> 
> > 
> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
> > 
> > Francois Dugast (5):
> >  drm/xe: Use SPDX-License-Identifier instead of license text
> >  drm/xe: Group engine related structs
> >  drm/xe: Move defines before relevant fields
> >  drm/xe: Document usage of struct drm_xe_device_query
> >  drm/xe: Fix some formatting issues in uAPI
> > 
> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
> > 1 file changed, 90 insertions(+), 81 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Oded Gabbay]

On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
> >> +Oded
> >>
> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
> >> > This series addresses some feedback on xe_drm.h provided here:
> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> >> >
> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
> >> > likely break CI.
> >>
> >> As I said on the previous reply, this is not going to fly. Now we have a
> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
> >> *no recorded reviews*, doing things that already conflict with other
> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
> >>
> >> Also, there are other fixes queued as fixups, which is still the process
> >> being used in drm-xe-next. It's also where CI is running. Due to display
> >> being moved up in the branch it's basically the only sane thing to do.
> >>
> >> I think we need we are shooting ourselves on the foot and this review
> >> process needs to be fixed / agreed upon before we create a mess out of
> >> it.
> >>
> >> Lucas De Marchi
> >
> >Yes I understand the concern. The intention is to address Oded's feedback on the
> >revision that was reviewed, so that the next review iteration is manageable and
> >not based on a moving target, which could lead to and endless effort.
>
> and then what are we going to do next? rebase the rest of drm-xe-next on top?
> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
> What about the changes that are already there changing the commits that
> are on xe-rev?
>
> >
> >Maybe I created confusion by sharing on the mailing list but at this stage the
>
> no, that was the right part of it. Reviews on the patches happen on this
> mailing list. The wrong part IMO was to push it as a branch before it
> got the necessary review and before we know how we are going to
> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
> really work for the reasons mentioned above.
>
> >idea is not to get this series merged, only to get Oded's validation that it
> >fixes the issues that he reported. Yes this means more work will be required to
> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
> >work that has been done so far.
> >
> >Do you see a better way that would still satisfy the "not moving target" goal?
>
> 0) Ignore the "not moving target" goal

Hi Lucas,
I would like to say a few things.

The first point is that the current xe-rev1 branch was something that
Rodrigo and I set-up. If you feel this is not a good starting point
for the review, let's talk and decide on a new starting point. Even
though I have already invested multiple hours in reviewing it, I can
understand the point you are making and I'm willing to re-start the
review with a new starting point.

The second point is once we make a decision regarding the code
baseline (keep using the old or moving to the new), I want *all* of us
to agree that newer patches that are sent to the mailing list will not
be part of the initial Xe upstreamed code. There are mainly two
reasons for that:

1. There is a long-standing process of upstreaming a new driver to the
Linux kernel and that process assumes a code baseline that doesn't
change between revisions, except for addressing code review comments.
As a person who has undergone this a couple of times from the side
being reviewed I would like to follow this same process here as well
:)

2. The Xe driver is one big driver, with a large team behind it that
continuously sends patches. The code keeps getting re-factored and
acquires new features at a high rate. If every revision of the review
the baseline code will change, the changes from the previous revision
will be too large to review independently. I will have to review the
entire driver again... Not to mention how will I be able to easily
verify that my comments were addressed properly (btw, I also wrote it
here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).

Therefore, I think we need to agree on some method that won't burn me
out and cause me to tell Dave & Daniel I'm giving up on what they
asked me to do. One way is to follow the rule above, but if you still
disagree with this, I'm open to hearing other suggestions on how to
enable a proper review in a timely manner that won't cause me to pull
my hair out.

In that context, I feel your "Ignore the not moving target goal"
statement to Francois was inappropriate. We need to have an agreement,
you can't decide that on your own.

Thanks,
Oded




>
> 1) The review should be on very a recent rev, not something old with a
>     lot of commits on top. That doesn't scale and there will be another 100
>     patches before the next review round.
>
>         $ tag=xe-rev1-2023-05-02
>         $ s=$(git log -1 --format=%s $tag)
>         $ commit=$(git log --grep "$s" -n1 --format=%H)
>         $ git rev-list $commit..HEAD | wc -l
>         208
>
>     Then as outcome of the review we have more patches to land, rebase
>     the rest, etc, etc.  This is a much worse "moving target" and IMO
>     it's worse for both reviewers and committers.
>
> 2) The xe-revX should have the same baseline. That makes for a really
>     simple way to check "what changed since I last looked". This can be
>     checked on the overall diff with `git diff` or on the amended fixes
>     with git range-diff.
>
>     Currently drm-xe-next is on drm-tip due to the display integration.
>     Being on drm-next is important for merging the branch upstream, but
>     not so much for these kind of reviews
>
> 3) Not merge things in the xe-revX branch that is not already
>     in drm-xe-next.  Ideally if there are things that really need to
>     be changed back in the xe-rev branch, it should be a fixup on
>     drm-xe-next that is then brought back to the xe-rev branch.
>
>
> Lucas De Marchi
>
> >
> >Thanks,
> >Francois
> >
> >>
> >> >
> >> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
> >> >
> >> > Francois Dugast (5):
> >> >  drm/xe: Use SPDX-License-Identifier instead of license text
> >> >  drm/xe: Group engine related structs
> >> >  drm/xe: Move defines before relevant fields
> >> >  drm/xe: Document usage of struct drm_xe_device_query
> >> >  drm/xe: Fix some formatting issues in uAPI
> >> >
> >> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
> >> > 1 file changed, 90 insertions(+), 81 deletions(-)
> >> >
> >> > --
> >> > 2.34.1
> >> >

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Lucas De Marchi]

On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
>On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
><lucas.demarchi@intel.com> wrote:
>>
>> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
>> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
>> >> +Oded
>> >>
>> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
>> >> > This series addresses some feedback on xe_drm.h provided here:
>> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>> >> >
>> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
>> >> > likely break CI.
>> >>
>> >> As I said on the previous reply, this is not going to fly. Now we have a
>> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
>> >> *no recorded reviews*, doing things that already conflict with other
>> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
>> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
>> >>
>> >> Also, there are other fixes queued as fixups, which is still the process
>> >> being used in drm-xe-next. It's also where CI is running. Due to display
>> >> being moved up in the branch it's basically the only sane thing to do.
>> >>
>> >> I think we need we are shooting ourselves on the foot and this review
>> >> process needs to be fixed / agreed upon before we create a mess out of
>> >> it.
>> >>
>> >> Lucas De Marchi
>> >
>> >Yes I understand the concern. The intention is to address Oded's feedback on the
>> >revision that was reviewed, so that the next review iteration is manageable and
>> >not based on a moving target, which could lead to and endless effort.
>>
>> and then what are we going to do next? rebase the rest of drm-xe-next on top?
>> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
>> What about the changes that are already there changing the commits that
>> are on xe-rev?
>>
>> >
>> >Maybe I created confusion by sharing on the mailing list but at this stage the
>>
>> no, that was the right part of it. Reviews on the patches happen on this
>> mailing list. The wrong part IMO was to push it as a branch before it
>> got the necessary review and before we know how we are going to
>> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
>> really work for the reasons mentioned above.
>>
>> >idea is not to get this series merged, only to get Oded's validation that it
>> >fixes the issues that he reported. Yes this means more work will be required to
>> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
>> >work that has been done so far.
>> >
>> >Do you see a better way that would still satisfy the "not moving target" goal?
>>
>> 0) Ignore the "not moving target" goal
>
>Hi Lucas,
>I would like to say a few things.
>
>The first point is that the current xe-rev1 branch was something that
>Rodrigo and I set-up. If you feel this is not a good starting point
>for the review, let's talk and decide on a new starting point. Even
>though I have already invested multiple hours in reviewing it, I can
>understand the point you are making and I'm willing to re-start the
>review with a new starting point.
>
>The second point is once we make a decision regarding the code
>baseline (keep using the old or moving to the new), I want *all* of us
>to agree that newer patches that are sent to the mailing list will not
>be part of the initial Xe upstreamed code. There are mainly two
>reasons for that:
>
>1. There is a long-standing process of upstreaming a new driver to the
>Linux kernel and that process assumes a code baseline that doesn't
>change between revisions, except for addressing code review comments.
>As a person who has undergone this a couple of times from the side
>being reviewed I would like to follow this same process here as well
>:)
>
>2. The Xe driver is one big driver, with a large team behind it that
>continuously sends patches. The code keeps getting re-factored and
>acquires new features at a high rate. If every revision of the review
>the baseline code will change, the changes from the previous revision
>will be too large to review independently. I will have to review the
>entire driver again... Not to mention how will I be able to easily
>verify that my comments were addressed properly (btw, I also wrote it
>here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).

Let me use a few examples:

	22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")

Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
Intel GPUs" as part of the original and then have a Fix commit leaving a
few hundreds of broken commits?  On other "first driver submissions"
I've seen that would be frowned upon and it would be expected that this
is fixed inplace instead of leaving broken commits behind.

	a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")

Same thing: a nasty memory corruption lurking there. Do we leave it
broken?  With the previous (current(?)) process we were actually adding
these fixups that are later moved back in the git history so things like
this are never part of the upstream code. Looking at xe-rev2 branch
being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
Remove unused define") more important that deserves being brought back
to the original submission?

Another one that comes to mind: the engine vs context name that you
raised on the other review and I pointed out to the history: would it be
something to be changed on xe-rev branch and invalidate the other things
on top?  Or would we better change this in drm-xe-next?

 From what I talked with Rodrigo before, we'd actually discuss and agree
about things that needed to be changed/fixed, particularly on the design
and interface of the driver (with drm subsystem and with userspace).
Line by line review with non-critical things would then become gitlab
issues, like when Matt Roper went through a similar line by line
review and Rodrigo created a bunch of gitlab issues that are being
solved in drm-xe-next.

>Therefore, I think we need to agree on some method that won't burn me
>out and cause me to tell Dave & Daniel I'm giving up on what they
>asked me to do. One way is to follow the rule above, but if you still
>disagree with this, I'm open to hearing other suggestions on how to
>enable a proper review in a timely manner that won't cause me to pull
>my hair out.
>
>In that context, I feel your "Ignore the not moving target goal"
>statement to Francois was inappropriate. We need to have an agreement,
>you can't decide that on your own.

What am I deciding on my own and why was that inappropriate?  I went
back to read again what I wrote to check if it was badly phrased, but I
don't see it that way. He asked my opinion on a different method, which
you are also asking when you say "I'm open to hearing other suggestions
on how to enable a proper review in a timely manner that won't cause me
to pull my hair out". If you hear and disagree, I'm totally fine with
it, but asking for it and then saying it's inappropriate to disagree
with the premise seems odd.

I went on and explained *why*, saying that the moving base is already
there and IMO a better method would be to accept the moving base, with a
different review method. And if you look at the proposal below, I'm
actually removing the "moving base" factor between what's being reviewed
and where the rest of development is happening, CI is running, other
review comments are being handled, etc.

What I'm currently seeing and I don't agree with:

- Commits are being pushed to a xe-revN branch bypassing code review

- Since CI is running on drm-xe-next, it's also bypassing CI. I was
   part of enough upstream submissions to know our ability to break
   things when handling code review comments, which is only noticed later
   when the rest is rebased on top and CI is running again

- drm-xe-next and xe-revN are not on the same baseline. If they were,
   then comparing both would be trivial and we'd probably not have
   this discussion. Question here on your process: currently xe-rev
   is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
   Is your intention that fixes (to only your review commens) are
   accumulated  on xe-rev* and then propose this to be merged to drm-next
   when that is finished, without any rebase?

- changes to "the original submission" that from my pov some are good,
   some would better be discussed and some that are not critical
   taking precedence over others that should really be there. And that
   is happening because there is this disconnect between drm-xe-next and
   xe-rev as per above.

- the disconnect will only get worse as we are on 1/6 batch of review
   comments so I don't see the process scaling well.

Lucas De Marchi

>
>Thanks,
>Oded
>
>
>
>
>>
>> 1) The review should be on very a recent rev, not something old with a
>>     lot of commits on top. That doesn't scale and there will be another 100
>>     patches before the next review round.
>>
>>         $ tag=xe-rev1-2023-05-02
>>         $ s=$(git log -1 --format=%s $tag)
>>         $ commit=$(git log --grep "$s" -n1 --format=%H)
>>         $ git rev-list $commit..HEAD | wc -l
>>         208
>>
>>     Then as outcome of the review we have more patches to land, rebase
>>     the rest, etc, etc.  This is a much worse "moving target" and IMO
>>     it's worse for both reviewers and committers.
>>
>> 2) The xe-revX should have the same baseline. That makes for a really
>>     simple way to check "what changed since I last looked". This can be
>>     checked on the overall diff with `git diff` or on the amended fixes
>>     with git range-diff.
>>
>>     Currently drm-xe-next is on drm-tip due to the display integration.
>>     Being on drm-next is important for merging the branch upstream, but
>>     not so much for these kind of reviews
>>
>> 3) Not merge things in the xe-revX branch that is not already
>>     in drm-xe-next.  Ideally if there are things that really need to
>>     be changed back in the xe-rev branch, it should be a fixup on
>>     drm-xe-next that is then brought back to the xe-rev branch.
>>
>>
>> Lucas De Marchi
>>
>> >
>> >Thanks,
>> >Francois
>> >
>> >>
>> >> >
>> >> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
>> >> >
>> >> > Francois Dugast (5):
>> >> >  drm/xe: Use SPDX-License-Identifier instead of license text
>> >> >  drm/xe: Group engine related structs
>> >> >  drm/xe: Move defines before relevant fields
>> >> >  drm/xe: Document usage of struct drm_xe_device_query
>> >> >  drm/xe: Fix some formatting issues in uAPI
>> >> >
>> >> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
>> >> > 1 file changed, 90 insertions(+), 81 deletions(-)
>> >> >
>> >> > --
>> >> > 2.34.1
>> >> >

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Oded Gabbay]

On Thu, Jun 1, 2023 at 7:48 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
> >On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
> ><lucas.demarchi@intel.com> wrote:
> >>
> >> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
> >> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
> >> >> +Oded
> >> >>
> >> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
> >> >> > This series addresses some feedback on xe_drm.h provided here:
> >> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> >> >> >
> >> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
> >> >> > likely break CI.
> >> >>
> >> >> As I said on the previous reply, this is not going to fly. Now we have a
> >> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
> >> >> *no recorded reviews*, doing things that already conflict with other
> >> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
> >> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
> >> >>
> >> >> Also, there are other fixes queued as fixups, which is still the process
> >> >> being used in drm-xe-next. It's also where CI is running. Due to display
> >> >> being moved up in the branch it's basically the only sane thing to do.
> >> >>
> >> >> I think we need we are shooting ourselves on the foot and this review
> >> >> process needs to be fixed / agreed upon before we create a mess out of
> >> >> it.
> >> >>
> >> >> Lucas De Marchi
> >> >
> >> >Yes I understand the concern. The intention is to address Oded's feedback on the
> >> >revision that was reviewed, so that the next review iteration is manageable and
> >> >not based on a moving target, which could lead to and endless effort.
> >>
> >> and then what are we going to do next? rebase the rest of drm-xe-next on top?
> >> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
> >> What about the changes that are already there changing the commits that
> >> are on xe-rev?
> >>
> >> >
> >> >Maybe I created confusion by sharing on the mailing list but at this stage the
> >>
> >> no, that was the right part of it. Reviews on the patches happen on this
> >> mailing list. The wrong part IMO was to push it as a branch before it
> >> got the necessary review and before we know how we are going to
> >> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
> >> really work for the reasons mentioned above.
> >>
> >> >idea is not to get this series merged, only to get Oded's validation that it
> >> >fixes the issues that he reported. Yes this means more work will be required to
> >> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
> >> >work that has been done so far.
> >> >
> >> >Do you see a better way that would still satisfy the "not moving target" goal?
> >>
> >> 0) Ignore the "not moving target" goal
> >
> >Hi Lucas,
> >I would like to say a few things.
> >
> >The first point is that the current xe-rev1 branch was something that
> >Rodrigo and I set-up. If you feel this is not a good starting point
> >for the review, let's talk and decide on a new starting point. Even
> >though I have already invested multiple hours in reviewing it, I can
> >understand the point you are making and I'm willing to re-start the
> >review with a new starting point.
> >
> >The second point is once we make a decision regarding the code
> >baseline (keep using the old or moving to the new), I want *all* of us
> >to agree that newer patches that are sent to the mailing list will not
> >be part of the initial Xe upstreamed code. There are mainly two
> >reasons for that:
> >
> >1. There is a long-standing process of upstreaming a new driver to the
> >Linux kernel and that process assumes a code baseline that doesn't
> >change between revisions, except for addressing code review comments.
> >As a person who has undergone this a couple of times from the side
> >being reviewed I would like to follow this same process here as well
> >:)
> >
> >2. The Xe driver is one big driver, with a large team behind it that
> >continuously sends patches. The code keeps getting re-factored and
> >acquires new features at a high rate. If every revision of the review
> >the baseline code will change, the changes from the previous revision
> >will be too large to review independently. I will have to review the
> >entire driver again... Not to mention how will I be able to easily
> >verify that my comments were addressed properly (btw, I also wrote it
> >here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).
>
> Let me use a few examples:
>
>         22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>
> Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
> Intel GPUs" as part of the original and then have a Fix commit leaving a
> few hundreds of broken commits?  On other "first driver submissions"
> I've seen that would be frowned upon and it would be expected that this
> is fixed inplace instead of leaving broken commits behind.
>
>         a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>
> Same thing: a nasty memory corruption lurking there. Do we leave it
> broken?  With the previous (current(?)) process we were actually adding
> these fixups that are later moved back in the git history so things like
> this are never part of the upstream code. Looking at xe-rev2 branch
> being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
> Remove unused define") more important that deserves being brought back
> to the original submission?
I'm sure there are multiple additional bugs/broken code paths that we
don't know about right now, will be part of the original submission
and will be fixed only in a couple of months or further than that.
Therefore, I don't think this is a critical issue.
But in any case, no one said we need to have the original submission
sent upstream, without additional patches after it in the same merge
cycle.

What is critical imo, is to have confidence that this driver is not
doing something horrendously wrong, like abstracting entire kernel
layers, or putting up a broken uapi, or incorrectly using kernel API,
or whatever.

>
> Another one that comes to mind: the engine vs context name that you
> raised on the other review and I pointed out to the history: would it be
> something to be changed on xe-rev branch and invalidate the other things
> on top?  Or would we better change this in drm-xe-next?

I agree the changes between rev1 and rev2 are often quite significant.
And with all your other input, I think we can consider the following:
1. Halting the review for now.
2. You will prepare a rev2 branch with all the changes we agreed upon
+ updated code to make it more aligned with drm-xe-next.
3. I'll restart the review on rev2, while we agree to keep the
drm-xe-next mostly aligned with the review branch until the review is
over.

>
>  From what I talked with Rodrigo before, we'd actually discuss and agree
> about things that needed to be changed/fixed, particularly on the design
> and interface of the driver (with drm subsystem and with userspace).
> Line by line review with non-critical things would then become gitlab
> issues, like when Matt Roper went through a similar line by line
> review and Rodrigo created a bunch of gitlab issues that are being
> solved in drm-xe-next.
>
> >Therefore, I think we need to agree on some method that won't burn me
> >out and cause me to tell Dave & Daniel I'm giving up on what they
> >asked me to do. One way is to follow the rule above, but if you still
> >disagree with this, I'm open to hearing other suggestions on how to
> >enable a proper review in a timely manner that won't cause me to pull
> >my hair out.
> >
> >In that context, I feel your "Ignore the not moving target goal"
> >statement to Francois was inappropriate. We need to have an agreement,
> >you can't decide that on your own.
>
> What am I deciding on my own and why was that inappropriate?  I went
> back to read again what I wrote to check if it was badly phrased, but I
> don't see it that way. He asked my opinion on a different method, which
> you are also asking when you say "I'm open to hearing other suggestions
> on how to enable a proper review in a timely manner that won't cause me
> to pull my hair out". If you hear and disagree, I'm totally fine with
> it, but asking for it and then saying it's inappropriate to disagree
> with the premise seems odd.
I read it as something else but if you meant it only as a suggestion,
then I take my words back.
>
> I went on and explained *why*, saying that the moving base is already
> there and IMO a better method would be to accept the moving base, with a
> different review method. And if you look at the proposal below, I'm
> actually removing the "moving base" factor between what's being reviewed
> and where the rest of development is happening, CI is running, other
> review comments are being handled, etc.
>
> What I'm currently seeing and I don't agree with:
>
> - Commits are being pushed to a xe-revN branch bypassing code review
Correct, this should not happen. That's why Francois sent them to the
ml and I thought it was sufficient.
>
> - Since CI is running on drm-xe-next, it's also bypassing CI. I was
>    part of enough upstream submissions to know our ability to break
>    things when handling code review comments, which is only noticed later
>    when the rest is rebased on top and CI is running again
I agree, but unless you can trigger CI on specific branch, I think
your only option is to run CI on drm-xe-next after it is rebased over
the final xe-rev branch (after the review is over).
>
> - drm-xe-next and xe-revN are not on the same baseline. If they were,
>    then comparing both would be trivial and we'd probably not have
>    this discussion. Question here on your process: currently xe-rev
>    is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
>    Is your intention that fixes (to only your review commens) are
>    accumulated  on xe-rev* and then propose this to be merged to drm-next
>    when that is finished, without any rebase?
I thought you would rebase drm-next-xe over the final xe-rev branch.
Then just make sure all those commits on top are passing CI after the rebase.
>
> - changes to "the original submission" that from my pov some are good,
>    some would better be discussed and some that are not critical
>    taking precedence over others that should really be there. And that
>    is happening because there is this disconnect between drm-xe-next and
>    xe-rev as per above.
That's why I don't mind if for xe-rev2, we make it aligned with a
newer baseline that matches drm-xe-next.
I'll continue the review on that branch when it's done.
Then, after the review is over, rebase drm-xe-next over xe-revLast,
which shouldn't be too hard hopefully.

wdyt ?
Thanks,
Oded
>
> - the disconnect will only get worse as we are on 1/6 batch of review
>    comments so I don't see the process scaling well.
>
> Lucas De Marchi
>
> >
> >Thanks,
> >Oded
> >
> >
> >
> >
> >>
> >> 1) The review should be on very a recent rev, not something old with a
> >>     lot of commits on top. That doesn't scale and there will be another 100
> >>     patches before the next review round.
> >>
> >>         $ tag=xe-rev1-2023-05-02
> >>         $ s=$(git log -1 --format=%s $tag)
> >>         $ commit=$(git log --grep "$s" -n1 --format=%H)
> >>         $ git rev-list $commit..HEAD | wc -l
> >>         208
> >>
> >>     Then as outcome of the review we have more patches to land, rebase
> >>     the rest, etc, etc.  This is a much worse "moving target" and IMO
> >>     it's worse for both reviewers and committers.
> >>
> >> 2) The xe-revX should have the same baseline. That makes for a really
> >>     simple way to check "what changed since I last looked". This can be
> >>     checked on the overall diff with `git diff` or on the amended fixes
> >>     with git range-diff.
> >>
> >>     Currently drm-xe-next is on drm-tip due to the display integration.
> >>     Being on drm-next is important for merging the branch upstream, but
> >>     not so much for these kind of reviews
> >>
> >> 3) Not merge things in the xe-revX branch that is not already
> >>     in drm-xe-next.  Ideally if there are things that really need to
> >>     be changed back in the xe-rev branch, it should be a fixup on
> >>     drm-xe-next that is then brought back to the xe-rev branch.
> >>
> >>
> >> Lucas De Marchi
> >>
> >> >
> >> >Thanks,
> >> >Francois
> >> >
> >> >>
> >> >> >
> >> >> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
> >> >> >
> >> >> > Francois Dugast (5):
> >> >> >  drm/xe: Use SPDX-License-Identifier instead of license text
> >> >> >  drm/xe: Group engine related structs
> >> >> >  drm/xe: Move defines before relevant fields
> >> >> >  drm/xe: Document usage of struct drm_xe_device_query
> >> >> >  drm/xe: Fix some formatting issues in uAPI
> >> >> >
> >> >> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
> >> >> > 1 file changed, 90 insertions(+), 81 deletions(-)
> >> >> >
> >> >> > --
> >> >> > 2.34.1
> >> >> >

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Lucas De Marchi]

On Mon, Jun 05, 2023 at 10:33:57AM +0300, Oded Gabbay wrote:
>On Thu, Jun 1, 2023 at 7:48 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
>> >On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
>> ><lucas.demarchi@intel.com> wrote:
>> >>
>> >> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
>> >> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
>> >> >> +Oded
>> >> >>
>> >> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
>> >> >> > This series addresses some feedback on xe_drm.h provided here:
>> >> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>> >> >> >
>> >> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
>> >> >> > likely break CI.
>> >> >>
>> >> >> As I said on the previous reply, this is not going to fly. Now we have a
>> >> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
>> >> >> *no recorded reviews*, doing things that already conflict with other
>> >> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
>> >> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
>> >> >>
>> >> >> Also, there are other fixes queued as fixups, which is still the process
>> >> >> being used in drm-xe-next. It's also where CI is running. Due to display
>> >> >> being moved up in the branch it's basically the only sane thing to do.
>> >> >>
>> >> >> I think we need we are shooting ourselves on the foot and this review
>> >> >> process needs to be fixed / agreed upon before we create a mess out of
>> >> >> it.
>> >> >>
>> >> >> Lucas De Marchi
>> >> >
>> >> >Yes I understand the concern. The intention is to address Oded's feedback on the
>> >> >revision that was reviewed, so that the next review iteration is manageable and
>> >> >not based on a moving target, which could lead to and endless effort.
>> >>
>> >> and then what are we going to do next? rebase the rest of drm-xe-next on top?
>> >> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
>> >> What about the changes that are already there changing the commits that
>> >> are on xe-rev?
>> >>
>> >> >
>> >> >Maybe I created confusion by sharing on the mailing list but at this stage the
>> >>
>> >> no, that was the right part of it. Reviews on the patches happen on this
>> >> mailing list. The wrong part IMO was to push it as a branch before it
>> >> got the necessary review and before we know how we are going to
>> >> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
>> >> really work for the reasons mentioned above.
>> >>
>> >> >idea is not to get this series merged, only to get Oded's validation that it
>> >> >fixes the issues that he reported. Yes this means more work will be required to
>> >> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
>> >> >work that has been done so far.
>> >> >
>> >> >Do you see a better way that would still satisfy the "not moving target" goal?
>> >>
>> >> 0) Ignore the "not moving target" goal
>> >
>> >Hi Lucas,
>> >I would like to say a few things.
>> >
>> >The first point is that the current xe-rev1 branch was something that
>> >Rodrigo and I set-up. If you feel this is not a good starting point
>> >for the review, let's talk and decide on a new starting point. Even
>> >though I have already invested multiple hours in reviewing it, I can
>> >understand the point you are making and I'm willing to re-start the
>> >review with a new starting point.
>> >
>> >The second point is once we make a decision regarding the code
>> >baseline (keep using the old or moving to the new), I want *all* of us
>> >to agree that newer patches that are sent to the mailing list will not
>> >be part of the initial Xe upstreamed code. There are mainly two
>> >reasons for that:
>> >
>> >1. There is a long-standing process of upstreaming a new driver to the
>> >Linux kernel and that process assumes a code baseline that doesn't
>> >change between revisions, except for addressing code review comments.
>> >As a person who has undergone this a couple of times from the side
>> >being reviewed I would like to follow this same process here as well
>> >:)
>> >
>> >2. The Xe driver is one big driver, with a large team behind it that
>> >continuously sends patches. The code keeps getting re-factored and
>> >acquires new features at a high rate. If every revision of the review
>> >the baseline code will change, the changes from the previous revision
>> >will be too large to review independently. I will have to review the
>> >entire driver again... Not to mention how will I be able to easily
>> >verify that my comments were addressed properly (btw, I also wrote it
>> >here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).
>>
>> Let me use a few examples:
>>
>>         22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>>
>> Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
>> Intel GPUs" as part of the original and then have a Fix commit leaving a
>> few hundreds of broken commits?  On other "first driver submissions"
>> I've seen that would be frowned upon and it would be expected that this
>> is fixed inplace instead of leaving broken commits behind.
>>
>>         a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>>
>> Same thing: a nasty memory corruption lurking there. Do we leave it
>> broken?  With the previous (current(?)) process we were actually adding
>> these fixups that are later moved back in the git history so things like
>> this are never part of the upstream code. Looking at xe-rev2 branch
>> being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
>> Remove unused define") more important that deserves being brought back
>> to the original submission?
>I'm sure there are multiple additional bugs/broken code paths that we
>don't know about right now, will be part of the original submission
>and will be fixed only in a couple of months or further than that.
>Therefore, I don't think this is a critical issue.
>But in any case, no one said we need to have the original submission
>sent upstream, without additional patches after it in the same merge
>cycle.
>
>What is critical imo, is to have confidence that this driver is not
>doing something horrendously wrong, like abstracting entire kernel
>layers, or putting up a broken uapi, or incorrectly using kernel API,
>or whatever.
>
>>
>> Another one that comes to mind: the engine vs context name that you
>> raised on the other review and I pointed out to the history: would it be
>> something to be changed on xe-rev branch and invalidate the other things
>> on top?  Or would we better change this in drm-xe-next?
>
>I agree the changes between rev1 and rev2 are often quite significant.
>And with all your other input, I think we can consider the following:
>1. Halting the review for now.
>2. You will prepare a rev2 branch with all the changes we agreed upon
>+ updated code to make it more aligned with drm-xe-next.
>3. I'll restart the review on rev2, while we agree to keep the
>drm-xe-next mostly aligned with the review branch until the review is
>over.

mostly agree, but need some clarifications below.

>
>>
>>  From what I talked with Rodrigo before, we'd actually discuss and agree
>> about things that needed to be changed/fixed, particularly on the design
>> and interface of the driver (with drm subsystem and with userspace).
>> Line by line review with non-critical things would then become gitlab
>> issues, like when Matt Roper went through a similar line by line
>> review and Rodrigo created a bunch of gitlab issues that are being
>> solved in drm-xe-next.
>>
>> >Therefore, I think we need to agree on some method that won't burn me
>> >out and cause me to tell Dave & Daniel I'm giving up on what they
>> >asked me to do. One way is to follow the rule above, but if you still
>> >disagree with this, I'm open to hearing other suggestions on how to
>> >enable a proper review in a timely manner that won't cause me to pull
>> >my hair out.
>> >
>> >In that context, I feel your "Ignore the not moving target goal"
>> >statement to Francois was inappropriate. We need to have an agreement,
>> >you can't decide that on your own.
>>
>> What am I deciding on my own and why was that inappropriate?  I went
>> back to read again what I wrote to check if it was badly phrased, but I
>> don't see it that way. He asked my opinion on a different method, which
>> you are also asking when you say "I'm open to hearing other suggestions
>> on how to enable a proper review in a timely manner that won't cause me
>> to pull my hair out". If you hear and disagree, I'm totally fine with
>> it, but asking for it and then saying it's inappropriate to disagree
>> with the premise seems odd.
>I read it as something else but if you meant it only as a suggestion,
>then I take my words back.
>>
>> I went on and explained *why*, saying that the moving base is already
>> there and IMO a better method would be to accept the moving base, with a
>> different review method. And if you look at the proposal below, I'm
>> actually removing the "moving base" factor between what's being reviewed
>> and where the rest of development is happening, CI is running, other
>> review comments are being handled, etc.
>>
>> What I'm currently seeing and I don't agree with:
>>
>> - Commits are being pushed to a xe-revN branch bypassing code review
>Correct, this should not happen. That's why Francois sent them to the
>ml and I thought it was sufficient.
>>
>> - Since CI is running on drm-xe-next, it's also bypassing CI. I was
>>    part of enough upstream submissions to know our ability to break
>>    things when handling code review comments, which is only noticed later
>>    when the rest is rebased on top and CI is running again
>I agree, but unless you can trigger CI on specific branch, I think
>your only option is to run CI on drm-xe-next after it is rebased over
>the final xe-rev branch (after the review is over).
>>
>> - drm-xe-next and xe-revN are not on the same baseline. If they were,
>>    then comparing both would be trivial and we'd probably not have
>>    this discussion. Question here on your process: currently xe-rev
>>    is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
>>    Is your intention that fixes (to only your review commens) are
>>    accumulated  on xe-rev* and then propose this to be merged to drm-next
>>    when that is finished, without any rebase?
>I thought you would rebase drm-next-xe over the final xe-rev branch.
>Then just make sure all those commits on top are passing CI after the rebase.
>>
>> - changes to "the original submission" that from my pov some are good,
>>    some would better be discussed and some that are not critical
>>    taking precedence over others that should really be there. And that
>>    is happening because there is this disconnect between drm-xe-next and
>>    xe-rev as per above.
>That's why I don't mind if for xe-rev2, we make it aligned with a
>newer baseline that matches drm-xe-next.
>I'll continue the review on that branch when it's done.
>Then, after the review is over, rebase drm-xe-next over xe-revLast,
>which shouldn't be too hard hopefully.
>
>wdyt ?

Let me first start saying that when I saw your 1/6 in your second batch
I thought it was 1 out of 6 and you would send each of them on
Thursdays, leading to 6 weeks from a state that is already 250 behind
drm-xe-next. I was pointed out today that that was actually the date.
My bad.

To move forward I think we need to clarify and agree on a few things:

1. How are you splitting the code review? I know you are reviewing "the
final state" rather than patch by patch. But what I'm looking for is to
know if you split the driver in 2, 3, 10 parts/files and we should
expect 2, 3, 10 batches of reviews or what? Depending on if we are
looking at 2 weeks, 2 month or a year, we may need to adapt the workflow
differently. If it's 1 week we could simply halt any change at all.
If it's 2 months, then we may have some problems to figure out the
workflow

2. I will quote what you said:

>What is critical imo, is to have confidence that this driver is not
>doing something horrendously wrong, like abstracting entire kernel
>layers, or putting up a broken uapi, or incorrectly using kernel API,
>or whatever.

I completely agree with that. Isn't this the original ask from drm
maintainers? However reading the review comments, the critical ones
related to the above paragraph seem to be swamped with other
cleanup-related. Some of those cleanups already happened or we moved in
another direction  with drm-xe-next.

Considering that as the critical part, what about the changes that come
due to others' feedback that also fall into that category?  Examples:
the uapi break we needed to do so we are not completely imcompatible
with 32b applications?  Or for example other patches changing jiffies
to nsec in the uapi. Wouldn't it be sensible to include those in the
next xe-revN?

With that in mind, what about this?

1. Halting the review for now.

2. I will tag the current drm-xe-next, just before the point
    in which we add display. This would give the same baseline between
    drm-xe-next and xe-rev2 tag.

3. We don't modify anymore the code below that point. If there are some
    critical changes that we'd rather have squashed below that point,
    then we just accumulate on top of that tag. So from your point of
    view, the next xe-rev3 tag won't have those changes together with
    others

4. No patch is merged/pushed directly to a xe-revX branch - xe-revX
    would be actually tags. Instead, the patch is based on drm-xe-next
    to get CI passing and having a solution that is agreed upon.
    When you're done with xe-rev2 and we send a xe-rev3, those patches can
    be moved so you actually get those changes first/only. Again, depending
    on the timeline we are talking about, it may be simply done by
    halting any other change. If it's a long time, then we may need to
    rebase and move commits around before giving you a xe-rev3 tag.

5. For the questions you made in the review comments, do you want devs
    to answer inline like I've been doing? Or to create gitlab issues like
    Rodrigo suggested when this process started?

6. (only if we are talking a long timeline, to be agreed on a case by
    case basis):  for intrusive changes that don't fall into the
    critical classification you gave above, may we agree on handling
    those on top of drm-xe-next, without moving them before other
    things?

thanks
Lucas De Marchi

>Thanks,
>Oded
>>
>> - the disconnect will only get worse as we are on 1/6 batch of review
>>    comments so I don't see the process scaling well.
>>
>> Lucas De Marchi
>>
>> >
>> >Thanks,
>> >Oded
>> >
>> >
>> >
>> >
>> >>
>> >> 1) The review should be on very a recent rev, not something old with a
>> >>     lot of commits on top. That doesn't scale and there will be another 100
>> >>     patches before the next review round.
>> >>
>> >>         $ tag=xe-rev1-2023-05-02
>> >>         $ s=$(git log -1 --format=%s $tag)
>> >>         $ commit=$(git log --grep "$s" -n1 --format=%H)
>> >>         $ git rev-list $commit..HEAD | wc -l
>> >>         208
>> >>
>> >>     Then as outcome of the review we have more patches to land, rebase
>> >>     the rest, etc, etc.  This is a much worse "moving target" and IMO
>> >>     it's worse for both reviewers and committers.
>> >>
>> >> 2) The xe-revX should have the same baseline. That makes for a really
>> >>     simple way to check "what changed since I last looked". This can be
>> >>     checked on the overall diff with `git diff` or on the amended fixes
>> >>     with git range-diff.
>> >>
>> >>     Currently drm-xe-next is on drm-tip due to the display integration.
>> >>     Being on drm-next is important for merging the branch upstream, but
>> >>     not so much for these kind of reviews
>> >>
>> >> 3) Not merge things in the xe-revX branch that is not already
>> >>     in drm-xe-next.  Ideally if there are things that really need to
>> >>     be changed back in the xe-rev branch, it should be a fixup on
>> >>     drm-xe-next that is then brought back to the xe-rev branch.
>> >>
>> >>
>> >> Lucas De Marchi
>> >>
>> >> >
>> >> >Thanks,
>> >> >Francois
>> >> >
>> >> >>
>> >> >> >
>> >> >> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
>> >> >> >
>> >> >> > Francois Dugast (5):
>> >> >> >  drm/xe: Use SPDX-License-Identifier instead of license text
>> >> >> >  drm/xe: Group engine related structs
>> >> >> >  drm/xe: Move defines before relevant fields
>> >> >> >  drm/xe: Document usage of struct drm_xe_device_query
>> >> >> >  drm/xe: Fix some formatting issues in uAPI
>> >> >> >
>> >> >> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
>> >> >> > 1 file changed, 90 insertions(+), 81 deletions(-)
>> >> >> >
>> >> >> > --
>> >> >> > 2.34.1
>> >> >> >

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Oded Gabbay]

On Mon, Jun 5, 2023 at 11:09 PM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Mon, Jun 05, 2023 at 10:33:57AM +0300, Oded Gabbay wrote:
> >On Thu, Jun 1, 2023 at 7:48 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >>
> >> On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
> >> >On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
> >> ><lucas.demarchi@intel.com> wrote:
> >> >>
> >> >> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
> >> >> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
> >> >> >> +Oded
> >> >> >>
> >> >> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
> >> >> >> > This series addresses some feedback on xe_drm.h provided here:
> >> >> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> >> >> >> >
> >> >> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
> >> >> >> > likely break CI.
> >> >> >>
> >> >> >> As I said on the previous reply, this is not going to fly. Now we have a
> >> >> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
> >> >> >> *no recorded reviews*, doing things that already conflict with other
> >> >> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
> >> >> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
> >> >> >>
> >> >> >> Also, there are other fixes queued as fixups, which is still the process
> >> >> >> being used in drm-xe-next. It's also where CI is running. Due to display
> >> >> >> being moved up in the branch it's basically the only sane thing to do.
> >> >> >>
> >> >> >> I think we need we are shooting ourselves on the foot and this review
> >> >> >> process needs to be fixed / agreed upon before we create a mess out of
> >> >> >> it.
> >> >> >>
> >> >> >> Lucas De Marchi
> >> >> >
> >> >> >Yes I understand the concern. The intention is to address Oded's feedback on the
> >> >> >revision that was reviewed, so that the next review iteration is manageable and
> >> >> >not based on a moving target, which could lead to and endless effort.
> >> >>
> >> >> and then what are we going to do next? rebase the rest of drm-xe-next on top?
> >> >> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
> >> >> What about the changes that are already there changing the commits that
> >> >> are on xe-rev?
> >> >>
> >> >> >
> >> >> >Maybe I created confusion by sharing on the mailing list but at this stage the
> >> >>
> >> >> no, that was the right part of it. Reviews on the patches happen on this
> >> >> mailing list. The wrong part IMO was to push it as a branch before it
> >> >> got the necessary review and before we know how we are going to
> >> >> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
> >> >> really work for the reasons mentioned above.
> >> >>
> >> >> >idea is not to get this series merged, only to get Oded's validation that it
> >> >> >fixes the issues that he reported. Yes this means more work will be required to
> >> >> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
> >> >> >work that has been done so far.
> >> >> >
> >> >> >Do you see a better way that would still satisfy the "not moving target" goal?
> >> >>
> >> >> 0) Ignore the "not moving target" goal
> >> >
> >> >Hi Lucas,
> >> >I would like to say a few things.
> >> >
> >> >The first point is that the current xe-rev1 branch was something that
> >> >Rodrigo and I set-up. If you feel this is not a good starting point
> >> >for the review, let's talk and decide on a new starting point. Even
> >> >though I have already invested multiple hours in reviewing it, I can
> >> >understand the point you are making and I'm willing to re-start the
> >> >review with a new starting point.
> >> >
> >> >The second point is once we make a decision regarding the code
> >> >baseline (keep using the old or moving to the new), I want *all* of us
> >> >to agree that newer patches that are sent to the mailing list will not
> >> >be part of the initial Xe upstreamed code. There are mainly two
> >> >reasons for that:
> >> >
> >> >1. There is a long-standing process of upstreaming a new driver to the
> >> >Linux kernel and that process assumes a code baseline that doesn't
> >> >change between revisions, except for addressing code review comments.
> >> >As a person who has undergone this a couple of times from the side
> >> >being reviewed I would like to follow this same process here as well
> >> >:)
> >> >
> >> >2. The Xe driver is one big driver, with a large team behind it that
> >> >continuously sends patches. The code keeps getting re-factored and
> >> >acquires new features at a high rate. If every revision of the review
> >> >the baseline code will change, the changes from the previous revision
> >> >will be too large to review independently. I will have to review the
> >> >entire driver again... Not to mention how will I be able to easily
> >> >verify that my comments were addressed properly (btw, I also wrote it
> >> >here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).
> >>
> >> Let me use a few examples:
> >>
> >>         22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
> >>
> >> Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
> >> Intel GPUs" as part of the original and then have a Fix commit leaving a
> >> few hundreds of broken commits?  On other "first driver submissions"
> >> I've seen that would be frowned upon and it would be expected that this
> >> is fixed inplace instead of leaving broken commits behind.
> >>
> >>         a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
> >>
> >> Same thing: a nasty memory corruption lurking there. Do we leave it
> >> broken?  With the previous (current(?)) process we were actually adding
> >> these fixups that are later moved back in the git history so things like
> >> this are never part of the upstream code. Looking at xe-rev2 branch
> >> being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
> >> Remove unused define") more important that deserves being brought back
> >> to the original submission?
> >I'm sure there are multiple additional bugs/broken code paths that we
> >don't know about right now, will be part of the original submission
> >and will be fixed only in a couple of months or further than that.
> >Therefore, I don't think this is a critical issue.
> >But in any case, no one said we need to have the original submission
> >sent upstream, without additional patches after it in the same merge
> >cycle.
> >
> >What is critical imo, is to have confidence that this driver is not
> >doing something horrendously wrong, like abstracting entire kernel
> >layers, or putting up a broken uapi, or incorrectly using kernel API,
> >or whatever.
> >
> >>
> >> Another one that comes to mind: the engine vs context name that you
> >> raised on the other review and I pointed out to the history: would it be
> >> something to be changed on xe-rev branch and invalidate the other things
> >> on top?  Or would we better change this in drm-xe-next?
> >
> >I agree the changes between rev1 and rev2 are often quite significant.
> >And with all your other input, I think we can consider the following:
> >1. Halting the review for now.
> >2. You will prepare a rev2 branch with all the changes we agreed upon
> >+ updated code to make it more aligned with drm-xe-next.
> >3. I'll restart the review on rev2, while we agree to keep the
> >drm-xe-next mostly aligned with the review branch until the review is
> >over.
>
> mostly agree, but need some clarifications below.
>
> >
> >>
> >>  From what I talked with Rodrigo before, we'd actually discuss and agree
> >> about things that needed to be changed/fixed, particularly on the design
> >> and interface of the driver (with drm subsystem and with userspace).
> >> Line by line review with non-critical things would then become gitlab
> >> issues, like when Matt Roper went through a similar line by line
> >> review and Rodrigo created a bunch of gitlab issues that are being
> >> solved in drm-xe-next.
> >>
> >> >Therefore, I think we need to agree on some method that won't burn me
> >> >out and cause me to tell Dave & Daniel I'm giving up on what they
> >> >asked me to do. One way is to follow the rule above, but if you still
> >> >disagree with this, I'm open to hearing other suggestions on how to
> >> >enable a proper review in a timely manner that won't cause me to pull
> >> >my hair out.
> >> >
> >> >In that context, I feel your "Ignore the not moving target goal"
> >> >statement to Francois was inappropriate. We need to have an agreement,
> >> >you can't decide that on your own.
> >>
> >> What am I deciding on my own and why was that inappropriate?  I went
> >> back to read again what I wrote to check if it was badly phrased, but I
> >> don't see it that way. He asked my opinion on a different method, which
> >> you are also asking when you say "I'm open to hearing other suggestions
> >> on how to enable a proper review in a timely manner that won't cause me
> >> to pull my hair out". If you hear and disagree, I'm totally fine with
> >> it, but asking for it and then saying it's inappropriate to disagree
> >> with the premise seems odd.
> >I read it as something else but if you meant it only as a suggestion,
> >then I take my words back.
> >>
> >> I went on and explained *why*, saying that the moving base is already
> >> there and IMO a better method would be to accept the moving base, with a
> >> different review method. And if you look at the proposal below, I'm
> >> actually removing the "moving base" factor between what's being reviewed
> >> and where the rest of development is happening, CI is running, other
> >> review comments are being handled, etc.
> >>
> >> What I'm currently seeing and I don't agree with:
> >>
> >> - Commits are being pushed to a xe-revN branch bypassing code review
> >Correct, this should not happen. That's why Francois sent them to the
> >ml and I thought it was sufficient.
> >>
> >> - Since CI is running on drm-xe-next, it's also bypassing CI. I was
> >>    part of enough upstream submissions to know our ability to break
> >>    things when handling code review comments, which is only noticed later
> >>    when the rest is rebased on top and CI is running again
> >I agree, but unless you can trigger CI on specific branch, I think
> >your only option is to run CI on drm-xe-next after it is rebased over
> >the final xe-rev branch (after the review is over).
> >>
> >> - drm-xe-next and xe-revN are not on the same baseline. If they were,
> >>    then comparing both would be trivial and we'd probably not have
> >>    this discussion. Question here on your process: currently xe-rev
> >>    is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
> >>    Is your intention that fixes (to only your review commens) are
> >>    accumulated  on xe-rev* and then propose this to be merged to drm-next
> >>    when that is finished, without any rebase?
> >I thought you would rebase drm-next-xe over the final xe-rev branch.
> >Then just make sure all those commits on top are passing CI after the rebase.
> >>
> >> - changes to "the original submission" that from my pov some are good,
> >>    some would better be discussed and some that are not critical
> >>    taking precedence over others that should really be there. And that
> >>    is happening because there is this disconnect between drm-xe-next and
> >>    xe-rev as per above.
> >That's why I don't mind if for xe-rev2, we make it aligned with a
> >newer baseline that matches drm-xe-next.
> >I'll continue the review on that branch when it's done.
> >Then, after the review is over, rebase drm-xe-next over xe-revLast,
> >which shouldn't be too hard hopefully.
> >
> >wdyt ?
>
> Let me first start saying that when I saw your 1/6 in your second batch
> I thought it was 1 out of 6 and you would send each of them on
> Thursdays, leading to 6 weeks from a state that is already 250 behind
> drm-xe-next. I was pointed out today that that was actually the date.
> My bad.
>
> To move forward I think we need to clarify and agree on a few things:
>
> 1. How are you splitting the code review? I know you are reviewing "the
> final state" rather than patch by patch. But what I'm looking for is to
> know if you split the driver in 2, 3, 10 parts/files and we should
> expect 2, 3, 10 batches of reviews or what? Depending on if we are
> looking at 2 weeks, 2 month or a year, we may need to adapt the workflow
> differently. If it's 1 week we could simply halt any change at all.
> If it's 2 months, then we may have some problems to figure out the
> workflow
I think we can finish this in 3-4 weeks (3-4 sets of additional comments).
>
> 2. I will quote what you said:
>
> >What is critical imo, is to have confidence that this driver is not
> >doing something horrendously wrong, like abstracting entire kernel
> >layers, or putting up a broken uapi, or incorrectly using kernel API,
> >or whatever.
>
> I completely agree with that. Isn't this the original ask from drm
> maintainers? However reading the review comments, the critical ones
> related to the above paragraph seem to be swamped with other
> cleanup-related. Some of those cleanups already happened or we moved in
> another direction  with drm-xe-next.
Yes, but I thought that if I'm already passing over the code, why
ignore these issues...
I was already documenting errors so I preferred to mention everything.
>
> Considering that as the critical part, what about the changes that come
> due to others' feedback that also fall into that category?  Examples:
> the uapi break we needed to do so we are not completely imcompatible
> with 32b applications?  Or for example other patches changing jiffies
> to nsec in the uapi. Wouldn't it be sensible to include those in the
> next xe-revN?
Yes, but again, I'm pretty sure you will continue to figure out stuff
as we go along, and we don't want to re-start the review each time.
Let's set a new baseline, do a concentrated 3-4 weeks review on it. We
will fix all the issues in that review and that will be good enough.
Then, any patches above it will go through the regular upstream flow.
>
> With that in mind, what about this?
>
> 1. Halting the review for now.
>
> 2. I will tag the current drm-xe-next, just before the point
>     in which we add display. This would give the same baseline between
>     drm-xe-next and xe-rev2 tag.
>
> 3. We don't modify anymore the code below that point. If there are some
>     critical changes that we'd rather have squashed below that point,
>     then we just accumulate on top of that tag. So from your point of
>     view, the next xe-rev3 tag won't have those changes together with
>     others
>
> 4. No patch is merged/pushed directly to a xe-revX branch - xe-revX
>     would be actually tags. Instead, the patch is based on drm-xe-next
>     to get CI passing and having a solution that is agreed upon.
>     When you're done with xe-rev2 and we send a xe-rev3, those patches can
>     be moved so you actually get those changes first/only. Again, depending
>     on the timeline we are talking about, it may be simply done by
>     halting any other change. If it's a long time, then we may need to
>     rebase and move commits around before giving you a xe-rev3 tag.
>
Sounds like a good plan.

> 5. For the questions you made in the review comments, do you want devs
>     to answer inline like I've been doing? Or to create gitlab issues like
>     Rodrigo suggested when this process started?
I think inline is fine. I think we should open gitlab issues for
points raising from this review only for extreme cases where they
can't be fixed now for some reason.
>
> 6. (only if we are talking a long timeline, to be agreed on a case by
>     case basis):  for intrusive changes that don't fall into the
>     critical classification you gave above, may we agree on handling
>     those on top of drm-xe-next, without moving them before other
>     things?
I think 3-4 weeks is not a long time, so I hope this point can be
disregarded right now.
Thanks,
Oded
>
> thanks
> Lucas De Marchi
>
> >Thanks,
> >Oded
> >>
> >> - the disconnect will only get worse as we are on 1/6 batch of review
> >>    comments so I don't see the process scaling well.
> >>
> >> Lucas De Marchi
> >>
> >> >
> >> >Thanks,
> >> >Oded
> >> >
> >> >
> >> >
> >> >
> >> >>
> >> >> 1) The review should be on very a recent rev, not something old with a
> >> >>     lot of commits on top. That doesn't scale and there will be another 100
> >> >>     patches before the next review round.
> >> >>
> >> >>         $ tag=xe-rev1-2023-05-02
> >> >>         $ s=$(git log -1 --format=%s $tag)
> >> >>         $ commit=$(git log --grep "$s" -n1 --format=%H)
> >> >>         $ git rev-list $commit..HEAD | wc -l
> >> >>         208
> >> >>
> >> >>     Then as outcome of the review we have more patches to land, rebase
> >> >>     the rest, etc, etc.  This is a much worse "moving target" and IMO
> >> >>     it's worse for both reviewers and committers.
> >> >>
> >> >> 2) The xe-revX should have the same baseline. That makes for a really
> >> >>     simple way to check "what changed since I last looked". This can be
> >> >>     checked on the overall diff with `git diff` or on the amended fixes
> >> >>     with git range-diff.
> >> >>
> >> >>     Currently drm-xe-next is on drm-tip due to the display integration.
> >> >>     Being on drm-next is important for merging the branch upstream, but
> >> >>     not so much for these kind of reviews
> >> >>
> >> >> 3) Not merge things in the xe-revX branch that is not already
> >> >>     in drm-xe-next.  Ideally if there are things that really need to
> >> >>     be changed back in the xe-rev branch, it should be a fixup on
> >> >>     drm-xe-next that is then brought back to the xe-rev branch.
> >> >>
> >> >>
> >> >> Lucas De Marchi
> >> >>
> >> >> >
> >> >> >Thanks,
> >> >> >Francois
> >> >> >
> >> >> >>
> >> >> >> >
> >> >> >> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
> >> >> >> >
> >> >> >> > Francois Dugast (5):
> >> >> >> >  drm/xe: Use SPDX-License-Identifier instead of license text
> >> >> >> >  drm/xe: Group engine related structs
> >> >> >> >  drm/xe: Move defines before relevant fields
> >> >> >> >  drm/xe: Document usage of struct drm_xe_device_query
> >> >> >> >  drm/xe: Fix some formatting issues in uAPI
> >> >> >> >
> >> >> >> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
> >> >> >> > 1 file changed, 90 insertions(+), 81 deletions(-)
> >> >> >> >
> >> >> >> > --
> >> >> >> > 2.34.1
> >> >> >> >

Re: [Intel-xe] [PATCH v2 1/5] drm/xe: Use SPDX-License-Identifier instead of license text

[Lucas De Marchi]

On Wed, May 31, 2023 at 03:23:34PM +0000, Francois Dugast wrote:
>Replace the license text with its SPDX-License-Identifier for
>quick identification of the license and consistency with the
>rest of the driver.
>
>Reported-by: Oded Gabbay <ogabbay@kernel.org>
>Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>Signed-off-by: Francois Dugast <francois.dugast@intel.com>

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>---
> include/uapi/drm/xe_drm.h | 24 ++----------------------
> 1 file changed, 2 insertions(+), 22 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index b0b80aae3ee8..ac3cfc1fb4a5 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -1,26 +1,6 @@
>+/* SPDX-License-Identifier: MIT */
> /*
>- * Copyright 2021 Intel Corporation. All Rights Reserved.
>- *
>- * Permission is hereby granted, free of charge, to any person obtaining a
>- * copy of this software and associated documentation files (the
>- * "Software"), to deal in the Software without restriction, including
>- * without limitation the rights to use, copy, modify, merge, publish,
>- * distribute, sub license, and/or sell copies of the Software, and to
>- * permit persons to whom the Software is furnished to do so, subject to
>- * the following conditions:
>- *
>- * The above copyright notice and this permission notice (including the
>- * next paragraph) shall be included in all copies or substantial portions
>- * of the Software.
>- *
>- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>- * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>- * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
>- * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>- * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>- * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>- *
>+ * Copyright © 2023 Intel Corporation
>  */
>
> #ifndef _UAPI_XE_DRM_H_
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 2/5] drm/xe: Group engine related structs

[Lucas De Marchi]

On Wed, May 31, 2023 at 03:23:35PM +0000, Francois Dugast wrote:
>Move the definition of drm_xe_engine_class_instance to group it with
>other engine related structs and to follow the ioctls order.
>
>Reported-by: Oded Gabbay <ogabbay@kernel.org>
>Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>Signed-off-by: Francois Dugast <francois.dugast@intel.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>---
> include/uapi/drm/xe_drm.h | 36 ++++++++++++++++++------------------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index ac3cfc1fb4a5..5d34b570a305 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -116,24 +116,6 @@ struct xe_user_extension {
> #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> #define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
>
>-struct drm_xe_engine_class_instance {
>-	__u16 engine_class;
>-
>-#define DRM_XE_ENGINE_CLASS_RENDER		0
>-#define DRM_XE_ENGINE_CLASS_COPY		1
>-#define DRM_XE_ENGINE_CLASS_VIDEO_DECODE	2
>-#define DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE	3
>-#define DRM_XE_ENGINE_CLASS_COMPUTE		4
>-	/*
>-	 * Kernel only class (not actual hardware engine class). Used for
>-	 * creating ordered queues of VM bind operations.
>-	 */
>-#define DRM_XE_ENGINE_CLASS_VM_BIND		5
>-
>-	__u16 engine_instance;
>-	__u16 gt_id;
>-};
>-
> #define XE_MEM_REGION_CLASS_SYSMEM	0
> #define XE_MEM_REGION_CLASS_VRAM	1
>
>@@ -518,6 +500,24 @@ struct drm_xe_engine_set_property {
> 	__u64 reserved[2];
> };
>
>+struct drm_xe_engine_class_instance {
>+	__u16 engine_class;
>+
>+#define DRM_XE_ENGINE_CLASS_RENDER		0
>+#define DRM_XE_ENGINE_CLASS_COPY		1
>+#define DRM_XE_ENGINE_CLASS_VIDEO_DECODE	2
>+#define DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE	3
>+#define DRM_XE_ENGINE_CLASS_COMPUTE		4
>+	/*
>+	 * Kernel only class (not actual hardware engine class). Used for
>+	 * creating ordered queues of VM bind operations.
>+	 */
>+#define DRM_XE_ENGINE_CLASS_VM_BIND		5
>+
>+	__u16 engine_instance;
>+	__u16 gt_id;
>+};
>+
> struct drm_xe_engine_create {
> 	/** @extensions: Pointer to the first extension struct, if any */
> #define XE_ENGINE_EXTENSION_SET_PROPERTY               0
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 3/5] drm/xe: Move defines before relevant fields

[Lucas De Marchi]

On Wed, May 31, 2023 at 03:23:36PM +0000, Francois Dugast wrote:
>Align on same rule in the whole file: defines then doc then relevant
>field.
>
>Reported-by: Oded Gabbay <ogabbay@kernel.org>
>Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>---
> include/uapi/drm/xe_drm.h | 62 ++++++++++++++++++++-------------------
> 1 file changed, 32 insertions(+), 30 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index 5d34b570a305..2eea80bf0e06 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -138,6 +138,7 @@ struct drm_xe_query_mem_usage {
> struct drm_xe_query_config {
> 	__u32 num_params;
> 	__u32 pad;
>+
> #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
> #define XE_QUERY_CONFIG_FLAGS			1
> 	#define XE_QUERY_CONFIG_FLAGS_HAS_VRAM		(0x1 << 0)
>@@ -179,11 +180,11 @@ struct drm_xe_query_topology_mask {
> 	/** @gt_id: GT ID the mask is associated with */
> 	__u16 gt_id;
>
>-	/** @type: type of mask */
>-	__u16 type;
> #define XE_TOPO_DSS_GEOMETRY	(1 << 0)
> #define XE_TOPO_DSS_COMPUTE	(1 << 1)
> #define XE_TOPO_EU_PER_DSS	(1 << 2)
>+	/** @type: type of mask */
>+	__u16 type;
>
> 	/** @num_bytes: number of bytes in requested mask */
> 	__u32 num_bytes;
>@@ -196,15 +197,14 @@ struct drm_xe_device_query {
> 	/** @extensions: Pointer to the first extension struct, if any */
> 	__u64 extensions;
>
>-	/** @query: The type of data to query */
>-	__u32 query;
>-
> #define DRM_XE_DEVICE_QUERY_ENGINES	0
> #define DRM_XE_DEVICE_QUERY_MEM_USAGE	1
> #define DRM_XE_DEVICE_QUERY_CONFIG	2
> #define DRM_XE_DEVICE_QUERY_GTS		3
> #define DRM_XE_DEVICE_QUERY_HWCONFIG	4
> #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY	5
>+	/** @query: The type of data to query */
>+	__u32 query;
>
> 	/** @size: Size of the queried data */
> 	__u32 size;
>@@ -227,12 +227,12 @@ struct drm_xe_gem_create {
> 	 */
> 	__u64 size;
>
>+#define XE_GEM_CREATE_FLAG_DEFER_BACKING	(0x1 << 24)
>+#define XE_GEM_CREATE_FLAG_SCANOUT		(0x1 << 25)
> 	/**
> 	 * @flags: Flags, currently a mask of memory instances of where BO can
> 	 * be placed
> 	 */
>-#define XE_GEM_CREATE_FLAG_DEFER_BACKING	(0x1 << 24)
>-#define XE_GEM_CREATE_FLAG_SCANOUT		(0x1 << 25)
> 	__u32 flags;
>
> 	/**
>@@ -293,8 +293,8 @@ struct drm_xe_ext_vm_set_property {
> 	/** @base: base user extension */
> 	struct xe_user_extension base;
>
>-	/** @property: property to set */
> #define XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS		0
>+	/** @property: property to set */
> 	__u32 property;
>
> 	/** @value: property value */
>@@ -305,17 +305,17 @@ struct drm_xe_ext_vm_set_property {
> };
>
> struct drm_xe_vm_create {
>-	/** @extensions: Pointer to the first extension struct, if any */
>+
> #define XE_VM_EXTENSION_SET_PROPERTY	0
>+	/** @extensions: Pointer to the first extension struct, if any */
> 	__u64 extensions;
>
>-	/** @flags: Flags */
>-	__u32 flags;
>-
> #define DRM_XE_VM_CREATE_SCRATCH_PAGE	(0x1 << 0)
> #define DRM_XE_VM_CREATE_COMPUTE_MODE	(0x1 << 1)
> #define DRM_XE_VM_CREATE_ASYNC_BIND_OPS	(0x1 << 2)
> #define DRM_XE_VM_CREATE_FAULT_MODE	(0x1 << 3)
>+	/** @flags: Flags */
>+	__u32 flags;

not for this patch, but we are missing a _FLAG_ in this to be consistent
with the rest.

>
> 	/** @vm_id: Returned VM ID */
> 	__u32 vm_id;
>@@ -365,12 +365,6 @@ struct drm_xe_vm_bind_op {
> 	 */
> 	__u64 gt_mask;
>
>-	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
>-	__u32 op;
>-
>-	/** @mem_region: Memory region to prefetch VMA to, instance not a mask */
>-	__u32 region;
>-
> #define XE_VM_BIND_OP_MAP		0x0
> #define XE_VM_BIND_OP_UNMAP		0x1
> #define XE_VM_BIND_OP_MAP_USERPTR	0x2
>@@ -409,6 +403,11 @@ struct drm_xe_vm_bind_op {
> 	 * than differing the MAP to the page fault handler.
> 	 */
> #define XE_VM_BIND_FLAG_IMMEDIATE	(0x1 << 18)
>+	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */

any idea why we mix them together like this? can't we simply have it
like this?

	__u16 op;
	__u16 flags;

>+	__u32 op;
>+
>+	/** @mem_region: Memory region to prefetch VMA to, instance not a mask */
>+	__u32 region;
>
> 	/** @reserved: Reserved */
> 	__u64 reserved[2];
>@@ -475,7 +474,6 @@ struct drm_xe_engine_set_property {
> 	/** @engine_id: Engine ID */
> 	__u32 engine_id;
>
>-	/** @property: property to set */
> #define XE_ENGINE_SET_PROPERTY_PRIORITY			0
> #define XE_ENGINE_SET_PROPERTY_TIMESLICE		1
> #define XE_ENGINE_SET_PROPERTY_PREEMPTION_TIMEOUT	2
>@@ -491,6 +489,7 @@ struct drm_xe_engine_set_property {
> #define XE_ENGINE_SET_PROPERTY_ACC_TRIGGER		6
> #define XE_ENGINE_SET_PROPERTY_ACC_NOTIFY		7
> #define XE_ENGINE_SET_PROPERTY_ACC_GRANULARITY		8
>+	/** @property: property to set */
> 	__u32 property;
>
> 	/** @value: property value */
>@@ -501,7 +500,6 @@ struct drm_xe_engine_set_property {
> };
>
> struct drm_xe_engine_class_instance {
>-	__u16 engine_class;
>
> #define DRM_XE_ENGINE_CLASS_RENDER		0
> #define DRM_XE_ENGINE_CLASS_COPY		1
>@@ -513,14 +511,16 @@ struct drm_xe_engine_class_instance {
> 	 * creating ordered queues of VM bind operations.
> 	 */
> #define DRM_XE_ENGINE_CLASS_VM_BIND		5
>+	__u16 engine_class;
>
> 	__u16 engine_instance;
> 	__u16 gt_id;
> };
>
> struct drm_xe_engine_create {
>-	/** @extensions: Pointer to the first extension struct, if any */
>+

here and in other places you are now leaving a newline that shouldn't be
there

> #define XE_ENGINE_EXTENSION_SET_PROPERTY               0
>+	/** @extensions: Pointer to the first extension struct, if any */
> 	__u64 extensions;
>
> 	/** @width: submission width (number BB per exec) for this engine */
>@@ -558,8 +558,8 @@ struct drm_xe_engine_get_property {
> 	/** @engine_id: Engine ID */
> 	__u32 engine_id;
>
>-	/** @property: property to get */
> #define XE_ENGINE_GET_PROPERTY_BAN			0
>+	/** @property: property to get */
> 	__u32 property;
>
> 	/** @value: property value */
>@@ -584,13 +584,12 @@ struct drm_xe_sync {
> 	/** @extensions: Pointer to the first extension struct, if any */
> 	__u64 extensions;
>
>-	__u32 flags;
>-
> #define DRM_XE_SYNC_SYNCOBJ		0x0
> #define DRM_XE_SYNC_TIMELINE_SYNCOBJ	0x1
> #define DRM_XE_SYNC_DMA_BUF		0x2
> #define DRM_XE_SYNC_USER_FENCE		0x3
> #define DRM_XE_SYNC_SIGNAL		0x10
>+	__u32 flags;
>
> 	union {
> 		__u32 handle;
>@@ -646,8 +645,6 @@ struct drm_xe_mmio {
>
> 	__u32 addr;
>
>-	__u32 flags;
>-
> #define DRM_XE_MMIO_8BIT	0x0
> #define DRM_XE_MMIO_16BIT	0x1
> #define DRM_XE_MMIO_32BIT	0x2
>@@ -655,6 +652,7 @@ struct drm_xe_mmio {
> #define DRM_XE_MMIO_BITS_MASK	0x3
> #define DRM_XE_MMIO_READ	0x4
> #define DRM_XE_MMIO_WRITE	0x8
>+	__u32 flags;
>
> 	__u64 value;
>
>@@ -685,27 +683,32 @@ struct drm_xe_wait_user_fence {
> 		 */
> 		__u64 vm_id;
> 	};
>-	/** @op: wait operation (type of comparison) */
>+
> #define DRM_XE_UFENCE_WAIT_EQ	0
> #define DRM_XE_UFENCE_WAIT_NEQ	1
> #define DRM_XE_UFENCE_WAIT_GT	2
> #define DRM_XE_UFENCE_WAIT_GTE	3
> #define DRM_XE_UFENCE_WAIT_LT	4
> #define DRM_XE_UFENCE_WAIT_LTE	5
>+	/** @op: wait operation (type of comparison) */
> 	__u16 op;
>-	/** @flags: wait flags */
>+
> #define DRM_XE_UFENCE_WAIT_SOFT_OP	(1 << 0)	/* e.g. Wait on VM bind */
> #define DRM_XE_UFENCE_WAIT_ABSTIME	(1 << 1)
> #define DRM_XE_UFENCE_WAIT_VM_ERROR	(1 << 2)
>+	/** @flags: wait flags */
> 	__u16 flags;
>+
> 	/** @value: compare value */
> 	__u64 value;
>-	/** @mask: comparison mask */
>+
> #define DRM_XE_UFENCE_WAIT_U8		0xffu
> #define DRM_XE_UFENCE_WAIT_U16		0xffffu
> #define DRM_XE_UFENCE_WAIT_U32		0xffffffffu
> #define DRM_XE_UFENCE_WAIT_U64		0xffffffffffffffffu
>+	/** @mask: comparison mask */
> 	__u64 mask;
>+
> 	/** @timeout: how long to wait before bailing, value in jiffies */
> 	__s64 timeout;
> 	/**
>@@ -770,7 +773,6 @@ struct drm_xe_vm_madvise {
> #define		DRM_XE_VMA_PRIORITY_HIGH	2	/* Must be elevated user */
> 	/* Pin the VMA in memory, must be elevated user */
> #define DRM_XE_VM_MADVISE_PIN			6
>-
> 	/** @property: property to set */
> 	__u32 property;

apparently does what it says and lgtm, but will need a rebase to double
check.

Lucas De Marchi

>
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 5/5] drm/xe: Fix some formatting issues in uAPI

[Lucas De Marchi]

On Wed, May 31, 2023 at 03:23:38PM +0000, Francois Dugast wrote:
>Reported-by: Oded Gabbay <ogabbay@kernel.org>
>Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>Signed-off-by: Francois Dugast <francois.dugast@intel.com>

no commit should be without commit message... need to spell out what
"some formatting issues" are, like the spacing, alignment, and repeated
words. With that and a rebase,

	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Not sure about the "Link:" usage here as it seems to be unique to these
patches.

Lucas De Marchi

>---
> include/uapi/drm/xe_drm.h | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index e29399de148c..667cd60db3d5 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -105,16 +105,16 @@ struct xe_user_extension {
> #define DRM_IOCTL_XE_GEM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_CREATE, struct drm_xe_gem_create)
> #define DRM_IOCTL_XE_GEM_MMAP_OFFSET		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_MMAP_OFFSET, struct drm_xe_gem_mmap_offset)
> #define DRM_IOCTL_XE_VM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_VM_CREATE, struct drm_xe_vm_create)
>-#define DRM_IOCTL_XE_VM_DESTROY			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
>-#define DRM_IOCTL_XE_VM_BIND			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
>+#define DRM_IOCTL_XE_VM_DESTROY			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
>+#define DRM_IOCTL_XE_VM_BIND			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
> #define DRM_IOCTL_XE_ENGINE_CREATE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_CREATE, struct drm_xe_engine_create)
> #define DRM_IOCTL_XE_ENGINE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_GET_PROPERTY, struct drm_xe_engine_get_property)
>-#define DRM_IOCTL_XE_ENGINE_DESTROY		DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
>-#define DRM_IOCTL_XE_EXEC			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
>+#define DRM_IOCTL_XE_ENGINE_DESTROY		 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
>+#define DRM_IOCTL_XE_EXEC			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> #define DRM_IOCTL_XE_MMIO			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_MMIO, struct drm_xe_mmio)
>-#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
>+#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
> #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>-#define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
>+#define DRM_IOCTL_XE_VM_MADVISE			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
>
> #define XE_MEM_REGION_CLASS_SYSMEM	0
> #define XE_MEM_REGION_CLASS_VRAM	1
>@@ -154,7 +154,7 @@ struct drm_xe_query_config {
> #define XE_QUERY_CONFIG_GT_COUNT		4
> #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
> #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
>-#define XE_QUERY_CONFIG_NUM_PARAM		XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
>+#define XE_QUERY_CONFIG_NUM_PARAM		(XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1)
> 	/** @info: array containing the config info below  */
> 	__u64 info[];
> };
>@@ -411,8 +411,8 @@ struct drm_xe_vm_bind_op {
> 	 * If this flag is clear and the IOCTL doesn't return an error, in
> 	 * practice the bind op is good and will complete.
> 	 *
>-	 * If this flag is set and doesn't return return an error, the bind op
>-	 * can still fail and recovery is needed. If configured, the bind op that
>+	 * If this flag is set and doesn't return an error, the bind op can
>+	 * still fail and recovery is needed. If configured, the bind op that
> 	 * caused the error will be captured in drm_xe_vm_bind_op_error_capture.
> 	 * Once the user sees the error (via a ufence +
> 	 * XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS), it should free memory
>@@ -651,9 +651,9 @@ struct drm_xe_exec {
> 	__u64 syncs;
>
> 	/**
>-	  * @address: address of batch buffer if num_batch_buffer == 1 or an
>-	  * array of batch buffer addresses
>-	  */
>+	 * @address: address of batch buffer if num_batch_buffer == 1 or an
>+	 * array of batch buffer addresses
>+	 */
> 	__u64 address;
>
> 	/**
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 4/5] drm/xe: Document usage of struct drm_xe_device_query

[Lucas De Marchi]

On Wed, May 31, 2023 at 03:23:37PM +0000, Francois Dugast wrote:
>Explain how to use struct drm_xe_device_query, in particular the behavior
>for size.

While at it, also add some missing kernel-doc in related structs.

>
>Reported-by: Oded Gabbay <ogabbay@kernel.org>
>Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>---
> include/uapi/drm/xe_drm.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index 2eea80bf0e06..e29399de148c 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -120,7 +120,10 @@ struct xe_user_extension {
> #define XE_MEM_REGION_CLASS_VRAM	1
>
> struct drm_xe_query_mem_usage {
>+	/** @num_params: number of memory regions returned in regions */
> 	__u32 num_regions;
>+
>+	/** @pad: MBZ */
> 	__u32 pad;
>
> 	struct drm_xe_query_mem_region {
>@@ -136,7 +139,10 @@ struct drm_xe_query_mem_usage {
> };
>
> struct drm_xe_query_config {
>+	/** @num_params: number of parameters returned in info */
> 	__u32 num_params;
>+
>+	/** @pad: MBZ */
> 	__u32 pad;
>
> #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
>@@ -149,11 +155,15 @@ struct drm_xe_query_config {
> #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
> #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
> #define XE_QUERY_CONFIG_NUM_PARAM		XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
>+	/** @info: array containing the config info below  */
> 	__u64 info[];
> };
>
> struct drm_xe_query_gts {
>+	/** @num_gt: number of GTs returned in gts */
> 	__u32 num_gt;
>+
>+	/** @pad: MBZ */
> 	__u32 pad;
>
> 	/*
>@@ -193,6 +203,23 @@ struct drm_xe_query_topology_mask {
> 	__u8 mask[];
> };
>
>+/**
>+ * struct drm_xe_device_query - main structure to query device information
>+ *
>+ * If size is set to 0, the driver fills it with the required size for the
>+ * requested type of data to query. If size is equal to the required size,
>+ * the queried information is copied into data.
>+ *
>+ * A typical usage from user space is:
>+ * - create a struct drm_xe_device_query with size = 0 and query = one of
>+ *   the types DRM_XE_DEVICE_QUERY_*, for example DRM_XE_DEVICE_QUERY_ENGINES
>+ * - call DRM_IOCTL_XE_DEVICE_QUERY
>+ * - create a pointer to a query struct matching the query type selected
>+ *   previously, for example struct drm_xe_engine_class_instance *
>+ * - allocate memory of the size returned in drm_xe_device_query
>+ * - call DRM_IOCTL_XE_DEVICE_QUERY
>+ * - read the queried information in data

looks ok, but may be better as a code snippet?

Lucas De Marchi

>+ */
> struct drm_xe_device_query {
> 	/** @extensions: Pointer to the first extension struct, if any */
> 	__u64 extensions;
>-- 
>2.34.1
>

Re: [Intel-xe] [PATCH v2 5/5] drm/xe: Fix some formatting issues in uAPI

[Francois Dugast]

On Tue, Jun 06, 2023 at 10:26:39PM -0700, Lucas De Marchi wrote:
> On Wed, May 31, 2023 at 03:23:38PM +0000, Francois Dugast wrote:
> > Reported-by: Oded Gabbay <ogabbay@kernel.org>
> > Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> 
> no commit should be without commit message... need to spell out what
> "some formatting issues" are, like the spacing, alignment, and repeated
> words. With that and a rebase,
> 
> 	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks, will do.

> 
> Not sure about the "Link:" usage here as it seems to be unique to these
> patches.

It seems it is used already, even to link to freedesktop archives:

	git log --after=2022-01-01 | grep "Link: https://lists.freedesktop.org"

The intention is to link patches with the review batches but I am fine
dropping it and keeping track in a separate list.

Francois

> 
> Lucas De Marchi
> 
> > ---
> > include/uapi/drm/xe_drm.h | 24 ++++++++++++------------
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index e29399de148c..667cd60db3d5 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -105,16 +105,16 @@ struct xe_user_extension {
> > #define DRM_IOCTL_XE_GEM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_CREATE, struct drm_xe_gem_create)
> > #define DRM_IOCTL_XE_GEM_MMAP_OFFSET		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_GEM_MMAP_OFFSET, struct drm_xe_gem_mmap_offset)
> > #define DRM_IOCTL_XE_VM_CREATE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_VM_CREATE, struct drm_xe_vm_create)
> > -#define DRM_IOCTL_XE_VM_DESTROY			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
> > -#define DRM_IOCTL_XE_VM_BIND			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
> > +#define DRM_IOCTL_XE_VM_DESTROY			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_DESTROY, struct drm_xe_vm_destroy)
> > +#define DRM_IOCTL_XE_VM_BIND			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_BIND, struct drm_xe_vm_bind)
> > #define DRM_IOCTL_XE_ENGINE_CREATE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_CREATE, struct drm_xe_engine_create)
> > #define DRM_IOCTL_XE_ENGINE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_GET_PROPERTY, struct drm_xe_engine_get_property)
> > -#define DRM_IOCTL_XE_ENGINE_DESTROY		DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
> > -#define DRM_IOCTL_XE_EXEC			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> > +#define DRM_IOCTL_XE_ENGINE_DESTROY		 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_DESTROY, struct drm_xe_engine_destroy)
> > +#define DRM_IOCTL_XE_EXEC			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> > #define DRM_IOCTL_XE_MMIO			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_MMIO, struct drm_xe_mmio)
> > -#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
> > +#define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
> > #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> > -#define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
> > +#define DRM_IOCTL_XE_VM_MADVISE			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
> > 
> > #define XE_MEM_REGION_CLASS_SYSMEM	0
> > #define XE_MEM_REGION_CLASS_VRAM	1
> > @@ -154,7 +154,7 @@ struct drm_xe_query_config {
> > #define XE_QUERY_CONFIG_GT_COUNT		4
> > #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
> > #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
> > -#define XE_QUERY_CONFIG_NUM_PARAM		XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
> > +#define XE_QUERY_CONFIG_NUM_PARAM		(XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1)
> > 	/** @info: array containing the config info below  */
> > 	__u64 info[];
> > };
> > @@ -411,8 +411,8 @@ struct drm_xe_vm_bind_op {
> > 	 * If this flag is clear and the IOCTL doesn't return an error, in
> > 	 * practice the bind op is good and will complete.
> > 	 *
> > -	 * If this flag is set and doesn't return return an error, the bind op
> > -	 * can still fail and recovery is needed. If configured, the bind op that
> > +	 * If this flag is set and doesn't return an error, the bind op can
> > +	 * still fail and recovery is needed. If configured, the bind op that
> > 	 * caused the error will be captured in drm_xe_vm_bind_op_error_capture.
> > 	 * Once the user sees the error (via a ufence +
> > 	 * XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS), it should free memory
> > @@ -651,9 +651,9 @@ struct drm_xe_exec {
> > 	__u64 syncs;
> > 
> > 	/**
> > -	  * @address: address of batch buffer if num_batch_buffer == 1 or an
> > -	  * array of batch buffer addresses
> > -	  */
> > +	 * @address: address of batch buffer if num_batch_buffer == 1 or an
> > +	 * array of batch buffer addresses
> > +	 */
> > 	__u64 address;
> > 
> > 	/**
> > -- 
> > 2.34.1
> > 

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Lucas De Marchi]

On Tue, Jun 06, 2023 at 01:58:43PM +0300, Oded Gabbay wrote:
>On Mon, Jun 5, 2023 at 11:09 PM Lucas De Marchi
><lucas.demarchi@intel.com> wrote:
>>
>> On Mon, Jun 05, 2023 at 10:33:57AM +0300, Oded Gabbay wrote:
>> >On Thu, Jun 1, 2023 at 7:48 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >>
>> >> On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
>> >> >On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
>> >> ><lucas.demarchi@intel.com> wrote:
>> >> >>
>> >> >> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
>> >> >> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
>> >> >> >> +Oded
>> >> >> >>
>> >> >> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
>> >> >> >> > This series addresses some feedback on xe_drm.h provided here:
>> >> >> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>> >> >> >> >
>> >> >> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
>> >> >> >> > likely break CI.
>> >> >> >>
>> >> >> >> As I said on the previous reply, this is not going to fly. Now we have a
>> >> >> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
>> >> >> >> *no recorded reviews*, doing things that already conflict with other
>> >> >> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
>> >> >> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
>> >> >> >>
>> >> >> >> Also, there are other fixes queued as fixups, which is still the process
>> >> >> >> being used in drm-xe-next. It's also where CI is running. Due to display
>> >> >> >> being moved up in the branch it's basically the only sane thing to do.
>> >> >> >>
>> >> >> >> I think we need we are shooting ourselves on the foot and this review
>> >> >> >> process needs to be fixed / agreed upon before we create a mess out of
>> >> >> >> it.
>> >> >> >>
>> >> >> >> Lucas De Marchi
>> >> >> >
>> >> >> >Yes I understand the concern. The intention is to address Oded's feedback on the
>> >> >> >revision that was reviewed, so that the next review iteration is manageable and
>> >> >> >not based on a moving target, which could lead to and endless effort.
>> >> >>
>> >> >> and then what are we going to do next? rebase the rest of drm-xe-next on top?
>> >> >> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
>> >> >> What about the changes that are already there changing the commits that
>> >> >> are on xe-rev?
>> >> >>
>> >> >> >
>> >> >> >Maybe I created confusion by sharing on the mailing list but at this stage the
>> >> >>
>> >> >> no, that was the right part of it. Reviews on the patches happen on this
>> >> >> mailing list. The wrong part IMO was to push it as a branch before it
>> >> >> got the necessary review and before we know how we are going to
>> >> >> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
>> >> >> really work for the reasons mentioned above.
>> >> >>
>> >> >> >idea is not to get this series merged, only to get Oded's validation that it
>> >> >> >fixes the issues that he reported. Yes this means more work will be required to
>> >> >> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
>> >> >> >work that has been done so far.
>> >> >> >
>> >> >> >Do you see a better way that would still satisfy the "not moving target" goal?
>> >> >>
>> >> >> 0) Ignore the "not moving target" goal
>> >> >
>> >> >Hi Lucas,
>> >> >I would like to say a few things.
>> >> >
>> >> >The first point is that the current xe-rev1 branch was something that
>> >> >Rodrigo and I set-up. If you feel this is not a good starting point
>> >> >for the review, let's talk and decide on a new starting point. Even
>> >> >though I have already invested multiple hours in reviewing it, I can
>> >> >understand the point you are making and I'm willing to re-start the
>> >> >review with a new starting point.
>> >> >
>> >> >The second point is once we make a decision regarding the code
>> >> >baseline (keep using the old or moving to the new), I want *all* of us
>> >> >to agree that newer patches that are sent to the mailing list will not
>> >> >be part of the initial Xe upstreamed code. There are mainly two
>> >> >reasons for that:
>> >> >
>> >> >1. There is a long-standing process of upstreaming a new driver to the
>> >> >Linux kernel and that process assumes a code baseline that doesn't
>> >> >change between revisions, except for addressing code review comments.
>> >> >As a person who has undergone this a couple of times from the side
>> >> >being reviewed I would like to follow this same process here as well
>> >> >:)
>> >> >
>> >> >2. The Xe driver is one big driver, with a large team behind it that
>> >> >continuously sends patches. The code keeps getting re-factored and
>> >> >acquires new features at a high rate. If every revision of the review
>> >> >the baseline code will change, the changes from the previous revision
>> >> >will be too large to review independently. I will have to review the
>> >> >entire driver again... Not to mention how will I be able to easily
>> >> >verify that my comments were addressed properly (btw, I also wrote it
>> >> >here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).
>> >>
>> >> Let me use a few examples:
>> >>
>> >>         22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>> >>
>> >> Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
>> >> Intel GPUs" as part of the original and then have a Fix commit leaving a
>> >> few hundreds of broken commits?  On other "first driver submissions"
>> >> I've seen that would be frowned upon and it would be expected that this
>> >> is fixed inplace instead of leaving broken commits behind.
>> >>
>> >>         a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
>> >>
>> >> Same thing: a nasty memory corruption lurking there. Do we leave it
>> >> broken?  With the previous (current(?)) process we were actually adding
>> >> these fixups that are later moved back in the git history so things like
>> >> this are never part of the upstream code. Looking at xe-rev2 branch
>> >> being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
>> >> Remove unused define") more important that deserves being brought back
>> >> to the original submission?
>> >I'm sure there are multiple additional bugs/broken code paths that we
>> >don't know about right now, will be part of the original submission
>> >and will be fixed only in a couple of months or further than that.
>> >Therefore, I don't think this is a critical issue.
>> >But in any case, no one said we need to have the original submission
>> >sent upstream, without additional patches after it in the same merge
>> >cycle.
>> >
>> >What is critical imo, is to have confidence that this driver is not
>> >doing something horrendously wrong, like abstracting entire kernel
>> >layers, or putting up a broken uapi, or incorrectly using kernel API,
>> >or whatever.
>> >
>> >>
>> >> Another one that comes to mind: the engine vs context name that you
>> >> raised on the other review and I pointed out to the history: would it be
>> >> something to be changed on xe-rev branch and invalidate the other things
>> >> on top?  Or would we better change this in drm-xe-next?
>> >
>> >I agree the changes between rev1 and rev2 are often quite significant.
>> >And with all your other input, I think we can consider the following:
>> >1. Halting the review for now.
>> >2. You will prepare a rev2 branch with all the changes we agreed upon
>> >+ updated code to make it more aligned with drm-xe-next.
>> >3. I'll restart the review on rev2, while we agree to keep the
>> >drm-xe-next mostly aligned with the review branch until the review is
>> >over.
>>
>> mostly agree, but need some clarifications below.
>>
>> >
>> >>
>> >>  From what I talked with Rodrigo before, we'd actually discuss and agree
>> >> about things that needed to be changed/fixed, particularly on the design
>> >> and interface of the driver (with drm subsystem and with userspace).
>> >> Line by line review with non-critical things would then become gitlab
>> >> issues, like when Matt Roper went through a similar line by line
>> >> review and Rodrigo created a bunch of gitlab issues that are being
>> >> solved in drm-xe-next.
>> >>
>> >> >Therefore, I think we need to agree on some method that won't burn me
>> >> >out and cause me to tell Dave & Daniel I'm giving up on what they
>> >> >asked me to do. One way is to follow the rule above, but if you still
>> >> >disagree with this, I'm open to hearing other suggestions on how to
>> >> >enable a proper review in a timely manner that won't cause me to pull
>> >> >my hair out.
>> >> >
>> >> >In that context, I feel your "Ignore the not moving target goal"
>> >> >statement to Francois was inappropriate. We need to have an agreement,
>> >> >you can't decide that on your own.
>> >>
>> >> What am I deciding on my own and why was that inappropriate?  I went
>> >> back to read again what I wrote to check if it was badly phrased, but I
>> >> don't see it that way. He asked my opinion on a different method, which
>> >> you are also asking when you say "I'm open to hearing other suggestions
>> >> on how to enable a proper review in a timely manner that won't cause me
>> >> to pull my hair out". If you hear and disagree, I'm totally fine with
>> >> it, but asking for it and then saying it's inappropriate to disagree
>> >> with the premise seems odd.
>> >I read it as something else but if you meant it only as a suggestion,
>> >then I take my words back.
>> >>
>> >> I went on and explained *why*, saying that the moving base is already
>> >> there and IMO a better method would be to accept the moving base, with a
>> >> different review method. And if you look at the proposal below, I'm
>> >> actually removing the "moving base" factor between what's being reviewed
>> >> and where the rest of development is happening, CI is running, other
>> >> review comments are being handled, etc.
>> >>
>> >> What I'm currently seeing and I don't agree with:
>> >>
>> >> - Commits are being pushed to a xe-revN branch bypassing code review
>> >Correct, this should not happen. That's why Francois sent them to the
>> >ml and I thought it was sufficient.
>> >>
>> >> - Since CI is running on drm-xe-next, it's also bypassing CI. I was
>> >>    part of enough upstream submissions to know our ability to break
>> >>    things when handling code review comments, which is only noticed later
>> >>    when the rest is rebased on top and CI is running again
>> >I agree, but unless you can trigger CI on specific branch, I think
>> >your only option is to run CI on drm-xe-next after it is rebased over
>> >the final xe-rev branch (after the review is over).
>> >>
>> >> - drm-xe-next and xe-revN are not on the same baseline. If they were,
>> >>    then comparing both would be trivial and we'd probably not have
>> >>    this discussion. Question here on your process: currently xe-rev
>> >>    is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
>> >>    Is your intention that fixes (to only your review commens) are
>> >>    accumulated  on xe-rev* and then propose this to be merged to drm-next
>> >>    when that is finished, without any rebase?
>> >I thought you would rebase drm-next-xe over the final xe-rev branch.
>> >Then just make sure all those commits on top are passing CI after the rebase.
>> >>
>> >> - changes to "the original submission" that from my pov some are good,
>> >>    some would better be discussed and some that are not critical
>> >>    taking precedence over others that should really be there. And that
>> >>    is happening because there is this disconnect between drm-xe-next and
>> >>    xe-rev as per above.
>> >That's why I don't mind if for xe-rev2, we make it aligned with a
>> >newer baseline that matches drm-xe-next.
>> >I'll continue the review on that branch when it's done.
>> >Then, after the review is over, rebase drm-xe-next over xe-revLast,
>> >which shouldn't be too hard hopefully.
>> >
>> >wdyt ?
>>
>> Let me first start saying that when I saw your 1/6 in your second batch
>> I thought it was 1 out of 6 and you would send each of them on
>> Thursdays, leading to 6 weeks from a state that is already 250 behind
>> drm-xe-next. I was pointed out today that that was actually the date.
>> My bad.
>>
>> To move forward I think we need to clarify and agree on a few things:
>>
>> 1. How are you splitting the code review? I know you are reviewing "the
>> final state" rather than patch by patch. But what I'm looking for is to
>> know if you split the driver in 2, 3, 10 parts/files and we should
>> expect 2, 3, 10 batches of reviews or what? Depending on if we are
>> looking at 2 weeks, 2 month or a year, we may need to adapt the workflow
>> differently. If it's 1 week we could simply halt any change at all.
>> If it's 2 months, then we may have some problems to figure out the
>> workflow
>I think we can finish this in 3-4 weeks (3-4 sets of additional comments).
>>
>> 2. I will quote what you said:
>>
>> >What is critical imo, is to have confidence that this driver is not
>> >doing something horrendously wrong, like abstracting entire kernel
>> >layers, or putting up a broken uapi, or incorrectly using kernel API,
>> >or whatever.
>>
>> I completely agree with that. Isn't this the original ask from drm
>> maintainers? However reading the review comments, the critical ones
>> related to the above paragraph seem to be swamped with other
>> cleanup-related. Some of those cleanups already happened or we moved in
>> another direction  with drm-xe-next.
>Yes, but I thought that if I'm already passing over the code, why
>ignore these issues...
>I was already documenting errors so I preferred to mention everything.
>>
>> Considering that as the critical part, what about the changes that come
>> due to others' feedback that also fall into that category?  Examples:
>> the uapi break we needed to do so we are not completely imcompatible
>> with 32b applications?  Or for example other patches changing jiffies
>> to nsec in the uapi. Wouldn't it be sensible to include those in the
>> next xe-revN?
>Yes, but again, I'm pretty sure you will continue to figure out stuff
>as we go along, and we don't want to re-start the review each time.
>Let's set a new baseline, do a concentrated 3-4 weeks review on it. We
>will fix all the issues in that review and that will be good enough.
>Then, any patches above it will go through the regular upstream flow.
>>
>> With that in mind, what about this?
>>
>> 1. Halting the review for now.
>>
>> 2. I will tag the current drm-xe-next, just before the point
>>     in which we add display. This would give the same baseline between
>>     drm-xe-next and xe-rev2 tag.
>>
>> 3. We don't modify anymore the code below that point. If there are some
>>     critical changes that we'd rather have squashed below that point,
>>     then we just accumulate on top of that tag. So from your point of
>>     view, the next xe-rev3 tag won't have those changes together with
>>     others
>>
>> 4. No patch is merged/pushed directly to a xe-revX branch - xe-revX
>>     would be actually tags. Instead, the patch is based on drm-xe-next
>>     to get CI passing and having a solution that is agreed upon.
>>     When you're done with xe-rev2 and we send a xe-rev3, those patches can
>>     be moved so you actually get those changes first/only. Again, depending
>>     on the timeline we are talking about, it may be simply done by
>>     halting any other change. If it's a long time, then we may need to
>>     rebase and move commits around before giving you a xe-rev3 tag.
>>
>Sounds like a good plan.
>
>> 5. For the questions you made in the review comments, do you want devs
>>     to answer inline like I've been doing? Or to create gitlab issues like
>>     Rodrigo suggested when this process started?
>I think inline is fine. I think we should open gitlab issues for
>points raising from this review only for extreme cases where they
>can't be fixed now for some reason.
>>
>> 6. (only if we are talking a long timeline, to be agreed on a case by
>>     case basis):  for intrusive changes that don't fall into the
>>     critical classification you gave above, may we agree on handling
>>     those on top of drm-xe-next, without moving them before other
>>     things?
>I think 3-4 weeks is not a long time, so I hope this point can be
>disregarded right now.

Ok, let's do that then. A new tag is pushed which should contain
everything we have until now except display. Please take a look at
https://gitlab.freedesktop.org/drm/xe/kernel.git xe-rev2

thanks
Lucas De Marchi

Re: [Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Oded Gabbay]

On Wed, Jun 7, 2023 at 10:42 PM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Tue, Jun 06, 2023 at 01:58:43PM +0300, Oded Gabbay wrote:
> >On Mon, Jun 5, 2023 at 11:09 PM Lucas De Marchi
> ><lucas.demarchi@intel.com> wrote:
> >>
> >> On Mon, Jun 05, 2023 at 10:33:57AM +0300, Oded Gabbay wrote:
> >> >On Thu, Jun 1, 2023 at 7:48 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> >>
> >> >> On Thu, Jun 01, 2023 at 01:21:06PM +0300, Oded Gabbay wrote:
> >> >> >On Wed, May 31, 2023 at 11:29 PM Lucas De Marchi
> >> >> ><lucas.demarchi@intel.com> wrote:
> >> >> >>
> >> >> >> On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
> >> >> >> >On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
> >> >> >> >> +Oded
> >> >> >> >>
> >> >> >> >> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
> >> >> >> >> > This series addresses some feedback on xe_drm.h provided here:
> >> >> >> >> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> >> >> >> >> >
> >> >> >> >> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
> >> >> >> >> > likely break CI.
> >> >> >> >>
> >> >> >> >> As I said on the previous reply, this is not going to fly. Now we have a
> >> >> >> >> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
> >> >> >> >> *no recorded reviews*, doing things that already conflict with other
> >> >> >> >> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
> >> >> >> >> the mmio refactor, the oob WAs, the probe refactor, etc etc.
> >> >> >> >>
> >> >> >> >> Also, there are other fixes queued as fixups, which is still the process
> >> >> >> >> being used in drm-xe-next. It's also where CI is running. Due to display
> >> >> >> >> being moved up in the branch it's basically the only sane thing to do.
> >> >> >> >>
> >> >> >> >> I think we need we are shooting ourselves on the foot and this review
> >> >> >> >> process needs to be fixed / agreed upon before we create a mess out of
> >> >> >> >> it.
> >> >> >> >>
> >> >> >> >> Lucas De Marchi
> >> >> >> >
> >> >> >> >Yes I understand the concern. The intention is to address Oded's feedback on the
> >> >> >> >revision that was reviewed, so that the next review iteration is manageable and
> >> >> >> >not based on a moving target, which could lead to and endless effort.
> >> >> >>
> >> >> >> and then what are we going to do next? rebase the rest of drm-xe-next on top?
> >> >> >> How do we guarantee the fixes on top of xe-rev is not breaking stuff?
> >> >> >> What about the changes that are already there changing the commits that
> >> >> >> are on xe-rev?
> >> >> >>
> >> >> >> >
> >> >> >> >Maybe I created confusion by sharing on the mailing list but at this stage the
> >> >> >>
> >> >> >> no, that was the right part of it. Reviews on the patches happen on this
> >> >> >> mailing list. The wrong part IMO was to push it as a branch before it
> >> >> >> got the necessary review and before we know how we are going to
> >> >> >> reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
> >> >> >> really work for the reasons mentioned above.
> >> >> >>
> >> >> >> >idea is not to get this series merged, only to get Oded's validation that it
> >> >> >> >fixes the issues that he reported. Yes this means more work will be required to
> >> >> >> >rebase and create fixups later for drm-xe-next but it seems acceptable for the
> >> >> >> >work that has been done so far.
> >> >> >> >
> >> >> >> >Do you see a better way that would still satisfy the "not moving target" goal?
> >> >> >>
> >> >> >> 0) Ignore the "not moving target" goal
> >> >> >
> >> >> >Hi Lucas,
> >> >> >I would like to say a few things.
> >> >> >
> >> >> >The first point is that the current xe-rev1 branch was something that
> >> >> >Rodrigo and I set-up. If you feel this is not a good starting point
> >> >> >for the review, let's talk and decide on a new starting point. Even
> >> >> >though I have already invested multiple hours in reviewing it, I can
> >> >> >understand the point you are making and I'm willing to re-start the
> >> >> >review with a new starting point.
> >> >> >
> >> >> >The second point is once we make a decision regarding the code
> >> >> >baseline (keep using the old or moving to the new), I want *all* of us
> >> >> >to agree that newer patches that are sent to the mailing list will not
> >> >> >be part of the initial Xe upstreamed code. There are mainly two
> >> >> >reasons for that:
> >> >> >
> >> >> >1. There is a long-standing process of upstreaming a new driver to the
> >> >> >Linux kernel and that process assumes a code baseline that doesn't
> >> >> >change between revisions, except for addressing code review comments.
> >> >> >As a person who has undergone this a couple of times from the side
> >> >> >being reviewed I would like to follow this same process here as well
> >> >> >:)
> >> >> >
> >> >> >2. The Xe driver is one big driver, with a large team behind it that
> >> >> >continuously sends patches. The code keeps getting re-factored and
> >> >> >acquires new features at a high rate. If every revision of the review
> >> >> >the baseline code will change, the changes from the previous revision
> >> >> >will be too large to review independently. I will have to review the
> >> >> >entire driver again... Not to mention how will I be able to easily
> >> >> >verify that my comments were addressed properly (btw, I also wrote it
> >> >> >here https://lore.kernel.org/intel-xe/CAFCwf124UMR3VQ=Tr6mT_eTRQi9ssqZqEH4jys1bGggHJjboLw@mail.gmail.com/).
> >> >>
> >> >> Let me use a few examples:
> >> >>
> >> >>         22ad50d184c0 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
> >> >>
> >> >> Do we want to leave the commit "drm/xe: Introduce a new DRM driver for
> >> >> Intel GPUs" as part of the original and then have a Fix commit leaving a
> >> >> few hundreds of broken commits?  On other "first driver submissions"
> >> >> I've seen that would be frowned upon and it would be expected that this
> >> >> is fixed inplace instead of leaving broken commits behind.
> >> >>
> >> >>         a00a92181ab1 ("fixup! drm/xe: Introduce a new DRM driver for Intel GPUs")
> >> >>
> >> >> Same thing: a nasty memory corruption lurking there. Do we leave it
> >> >> broken?  With the previous (current(?)) process we were actually adding
> >> >> these fixups that are later moved back in the git history so things like
> >> >> this are never part of the upstream code. Looking at xe-rev2 branch
> >> >> being pushed without any code review, why is 83ac6e02bf5e ("drm/xe:
> >> >> Remove unused define") more important that deserves being brought back
> >> >> to the original submission?
> >> >I'm sure there are multiple additional bugs/broken code paths that we
> >> >don't know about right now, will be part of the original submission
> >> >and will be fixed only in a couple of months or further than that.
> >> >Therefore, I don't think this is a critical issue.
> >> >But in any case, no one said we need to have the original submission
> >> >sent upstream, without additional patches after it in the same merge
> >> >cycle.
> >> >
> >> >What is critical imo, is to have confidence that this driver is not
> >> >doing something horrendously wrong, like abstracting entire kernel
> >> >layers, or putting up a broken uapi, or incorrectly using kernel API,
> >> >or whatever.
> >> >
> >> >>
> >> >> Another one that comes to mind: the engine vs context name that you
> >> >> raised on the other review and I pointed out to the history: would it be
> >> >> something to be changed on xe-rev branch and invalidate the other things
> >> >> on top?  Or would we better change this in drm-xe-next?
> >> >
> >> >I agree the changes between rev1 and rev2 are often quite significant.
> >> >And with all your other input, I think we can consider the following:
> >> >1. Halting the review for now.
> >> >2. You will prepare a rev2 branch with all the changes we agreed upon
> >> >+ updated code to make it more aligned with drm-xe-next.
> >> >3. I'll restart the review on rev2, while we agree to keep the
> >> >drm-xe-next mostly aligned with the review branch until the review is
> >> >over.
> >>
> >> mostly agree, but need some clarifications below.
> >>
> >> >
> >> >>
> >> >>  From what I talked with Rodrigo before, we'd actually discuss and agree
> >> >> about things that needed to be changed/fixed, particularly on the design
> >> >> and interface of the driver (with drm subsystem and with userspace).
> >> >> Line by line review with non-critical things would then become gitlab
> >> >> issues, like when Matt Roper went through a similar line by line
> >> >> review and Rodrigo created a bunch of gitlab issues that are being
> >> >> solved in drm-xe-next.
> >> >>
> >> >> >Therefore, I think we need to agree on some method that won't burn me
> >> >> >out and cause me to tell Dave & Daniel I'm giving up on what they
> >> >> >asked me to do. One way is to follow the rule above, but if you still
> >> >> >disagree with this, I'm open to hearing other suggestions on how to
> >> >> >enable a proper review in a timely manner that won't cause me to pull
> >> >> >my hair out.
> >> >> >
> >> >> >In that context, I feel your "Ignore the not moving target goal"
> >> >> >statement to Francois was inappropriate. We need to have an agreement,
> >> >> >you can't decide that on your own.
> >> >>
> >> >> What am I deciding on my own and why was that inappropriate?  I went
> >> >> back to read again what I wrote to check if it was badly phrased, but I
> >> >> don't see it that way. He asked my opinion on a different method, which
> >> >> you are also asking when you say "I'm open to hearing other suggestions
> >> >> on how to enable a proper review in a timely manner that won't cause me
> >> >> to pull my hair out". If you hear and disagree, I'm totally fine with
> >> >> it, but asking for it and then saying it's inappropriate to disagree
> >> >> with the premise seems odd.
> >> >I read it as something else but if you meant it only as a suggestion,
> >> >then I take my words back.
> >> >>
> >> >> I went on and explained *why*, saying that the moving base is already
> >> >> there and IMO a better method would be to accept the moving base, with a
> >> >> different review method. And if you look at the proposal below, I'm
> >> >> actually removing the "moving base" factor between what's being reviewed
> >> >> and where the rest of development is happening, CI is running, other
> >> >> review comments are being handled, etc.
> >> >>
> >> >> What I'm currently seeing and I don't agree with:
> >> >>
> >> >> - Commits are being pushed to a xe-revN branch bypassing code review
> >> >Correct, this should not happen. That's why Francois sent them to the
> >> >ml and I thought it was sufficient.
> >> >>
> >> >> - Since CI is running on drm-xe-next, it's also bypassing CI. I was
> >> >>    part of enough upstream submissions to know our ability to break
> >> >>    things when handling code review comments, which is only noticed later
> >> >>    when the rest is rebased on top and CI is running again
> >> >I agree, but unless you can trigger CI on specific branch, I think
> >> >your only option is to run CI on drm-xe-next after it is rebased over
> >> >the final xe-rev branch (after the review is over).
> >> >>
> >> >> - drm-xe-next and xe-revN are not on the same baseline. If they were,
> >> >>    then comparing both would be trivial and we'd probably not have
> >> >>    this discussion. Question here on your process: currently xe-rev
> >> >>    is on top of drm-next-2023-04-27, which is on top of v6.3-rc4.
> >> >>    Is your intention that fixes (to only your review commens) are
> >> >>    accumulated  on xe-rev* and then propose this to be merged to drm-next
> >> >>    when that is finished, without any rebase?
> >> >I thought you would rebase drm-next-xe over the final xe-rev branch.
> >> >Then just make sure all those commits on top are passing CI after the rebase.
> >> >>
> >> >> - changes to "the original submission" that from my pov some are good,
> >> >>    some would better be discussed and some that are not critical
> >> >>    taking precedence over others that should really be there. And that
> >> >>    is happening because there is this disconnect between drm-xe-next and
> >> >>    xe-rev as per above.
> >> >That's why I don't mind if for xe-rev2, we make it aligned with a
> >> >newer baseline that matches drm-xe-next.
> >> >I'll continue the review on that branch when it's done.
> >> >Then, after the review is over, rebase drm-xe-next over xe-revLast,
> >> >which shouldn't be too hard hopefully.
> >> >
> >> >wdyt ?
> >>
> >> Let me first start saying that when I saw your 1/6 in your second batch
> >> I thought it was 1 out of 6 and you would send each of them on
> >> Thursdays, leading to 6 weeks from a state that is already 250 behind
> >> drm-xe-next. I was pointed out today that that was actually the date.
> >> My bad.
> >>
> >> To move forward I think we need to clarify and agree on a few things:
> >>
> >> 1. How are you splitting the code review? I know you are reviewing "the
> >> final state" rather than patch by patch. But what I'm looking for is to
> >> know if you split the driver in 2, 3, 10 parts/files and we should
> >> expect 2, 3, 10 batches of reviews or what? Depending on if we are
> >> looking at 2 weeks, 2 month or a year, we may need to adapt the workflow
> >> differently. If it's 1 week we could simply halt any change at all.
> >> If it's 2 months, then we may have some problems to figure out the
> >> workflow
> >I think we can finish this in 3-4 weeks (3-4 sets of additional comments).
> >>
> >> 2. I will quote what you said:
> >>
> >> >What is critical imo, is to have confidence that this driver is not
> >> >doing something horrendously wrong, like abstracting entire kernel
> >> >layers, or putting up a broken uapi, or incorrectly using kernel API,
> >> >or whatever.
> >>
> >> I completely agree with that. Isn't this the original ask from drm
> >> maintainers? However reading the review comments, the critical ones
> >> related to the above paragraph seem to be swamped with other
> >> cleanup-related. Some of those cleanups already happened or we moved in
> >> another direction  with drm-xe-next.
> >Yes, but I thought that if I'm already passing over the code, why
> >ignore these issues...
> >I was already documenting errors so I preferred to mention everything.
> >>
> >> Considering that as the critical part, what about the changes that come
> >> due to others' feedback that also fall into that category?  Examples:
> >> the uapi break we needed to do so we are not completely imcompatible
> >> with 32b applications?  Or for example other patches changing jiffies
> >> to nsec in the uapi. Wouldn't it be sensible to include those in the
> >> next xe-revN?
> >Yes, but again, I'm pretty sure you will continue to figure out stuff
> >as we go along, and we don't want to re-start the review each time.
> >Let's set a new baseline, do a concentrated 3-4 weeks review on it. We
> >will fix all the issues in that review and that will be good enough.
> >Then, any patches above it will go through the regular upstream flow.
> >>
> >> With that in mind, what about this?
> >>
> >> 1. Halting the review for now.
> >>
> >> 2. I will tag the current drm-xe-next, just before the point
> >>     in which we add display. This would give the same baseline between
> >>     drm-xe-next and xe-rev2 tag.
> >>
> >> 3. We don't modify anymore the code below that point. If there are some
> >>     critical changes that we'd rather have squashed below that point,
> >>     then we just accumulate on top of that tag. So from your point of
> >>     view, the next xe-rev3 tag won't have those changes together with
> >>     others
> >>
> >> 4. No patch is merged/pushed directly to a xe-revX branch - xe-revX
> >>     would be actually tags. Instead, the patch is based on drm-xe-next
> >>     to get CI passing and having a solution that is agreed upon.
> >>     When you're done with xe-rev2 and we send a xe-rev3, those patches can
> >>     be moved so you actually get those changes first/only. Again, depending
> >>     on the timeline we are talking about, it may be simply done by
> >>     halting any other change. If it's a long time, then we may need to
> >>     rebase and move commits around before giving you a xe-rev3 tag.
> >>
> >Sounds like a good plan.
> >
> >> 5. For the questions you made in the review comments, do you want devs
> >>     to answer inline like I've been doing? Or to create gitlab issues like
> >>     Rodrigo suggested when this process started?
> >I think inline is fine. I think we should open gitlab issues for
> >points raising from this review only for extreme cases where they
> >can't be fixed now for some reason.
> >>
> >> 6. (only if we are talking a long timeline, to be agreed on a case by
> >>     case basis):  for intrusive changes that don't fall into the
> >>     critical classification you gave above, may we agree on handling
> >>     those on top of drm-xe-next, without moving them before other
> >>     things?
> >I think 3-4 weeks is not a long time, so I hope this point can be
> >disregarded right now.
>
> Ok, let's do that then. A new tag is pushed which should contain
> everything we have until now except display. Please take a look at
> https://gitlab.freedesktop.org/drm/xe/kernel.git xe-rev2
>
> thanks
> Lucas De Marchi
Thanks,
Will look into that.
Oded