intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
@ 2021-07-26 12:03 Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 2/7] lib/i915/gem_mman: add fixed mode to mmap__device_coherent Matthew Auld
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

We need this for discrete.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/i915/gem_mman.c | 37 +++++++++++++++++++++++++++++++++++++
 lib/i915/gem_mman.h |  4 ++++
 2 files changed, 41 insertions(+)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index 4b4f2114..e2514f0c 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
 	return ptr;
 }
 
+#define LOCAL_I915_MMAP_OFFSET_FIXED 4
+
+void *__gem_mmap_offset__fixed(int fd, uint32_t handle, uint64_t offset,
+			       uint64_t size, unsigned prot)
+{
+	return __gem_mmap_offset(fd, handle, offset, size, prot,
+				 LOCAL_I915_MMAP_OFFSET_FIXED);
+}
+
+/**
+ * gem_mmap_offset__fixed: Used to mmap objects on discrete platforms
+ * @fd: open i915 drm file descriptor
+ * @handle: gem buffer object handle
+ * @offset: offset in the gem buffer of the mmap arena
+ * @size: size of the mmap arena
+ * @prot: memory protection bits as used by mmap()
+ *
+ * Like __gem_mmap_offset__fixed() except we assert on failure.
+ *
+ * For discrete the caching attributes for the pages are fixed at allocation
+ * time, and can't be changed. The FIXED mode will simply use the same caching *
+ * mode of the allocated pages. This mode will always be coherent with GPU
+ * access.
+ *
+ * On non-discrete platforms this mode is not supported.
+ *
+ * Returns: A pointer to the created memory mapping
+ */
+void *gem_mmap_offset__fixed(int fd, uint32_t handle, uint64_t offset,
+			   uint64_t size, unsigned prot)
+{
+	void *ptr = __gem_mmap_offset__fixed(fd, handle, offset, size, prot);
+
+	igt_assert(ptr);
+	return ptr;
+}
+
 /**
  * __gem_mmap__cpu_coherent:
  * @fd: open i915 drm file descriptor
diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h
index 5695d2ad..290c997d 100644
--- a/lib/i915/gem_mman.h
+++ b/lib/i915/gem_mman.h
@@ -37,6 +37,8 @@ bool gem_mmap_offset__has_wc(int fd);
 void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
 void *gem_mmap_offset__wc(int fd, uint32_t handle, uint64_t offset,
 			  uint64_t size, unsigned prot);
+void *gem_mmap_offset__fixed(int fd, uint32_t handle, uint64_t offset,
+			     uint64_t size, unsigned prot);
 void *gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
 				uint64_t size, unsigned prot);
 void *gem_mmap__cpu_coherent(int fd, uint32_t handle, uint64_t offset,
@@ -54,6 +56,8 @@ void *__gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
 void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
 void *__gem_mmap_offset__wc(int fd, uint32_t handle, uint64_t offset,
 			    uint64_t size, unsigned prot);
+void *__gem_mmap_offset__fixed(int fd, uint32_t handle, uint64_t offset,
+			       uint64_t size, unsigned prot);
 void *__gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
 				  uint64_t size, unsigned prot);
 void *__gem_mmap_offset(int fd, uint32_t handle, uint64_t offset, uint64_t size,
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 2/7] lib/i915/gem_mman: add fixed mode to mmap__device_coherent
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 3/7] lib/i915/gem_mman: add fixed mode to mmap__cpu_coherent Matthew Auld
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

On discrete we need to fallback to this mode.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/i915/gem_mman.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index e2514f0c..222e8896 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -383,9 +383,10 @@ void *__gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
 				      I915_MMAP_OFFSET_WC);
 	if (!ptr)
 		ptr = __gem_mmap__wc(fd, handle, offset, size, prot);
-
 	if (!ptr)
 		ptr = __gem_mmap__gtt(fd, handle, size, prot);
+	if (!ptr)
+		ptr = __gem_mmap_offset__fixed(fd, handle, offset, size, prot);
 
 	return ptr;
 }
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 3/7] lib/i915/gem_mman: add fixed mode to mmap__cpu_coherent
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 2/7] lib/i915/gem_mman: add fixed mode to mmap__device_coherent Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 4/7] lib/i915/gem_mman: update mmap_offset_types with FIXED Matthew Auld
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

On discrete we only support the new fixed mode.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/i915/gem_mman.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index 222e8896..337d28fb 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -580,6 +580,8 @@ void *gem_mmap__cpu_coherent(int fd, uint32_t handle, uint64_t offset,
 	igt_assert(offset == 0);
 
 	ptr = __gem_mmap__cpu_coherent(fd, handle, offset, size, prot);
+	if (!ptr)
+		ptr = __gem_mmap_offset__fixed(fd, handle, offset, size, prot);
 	igt_assert(ptr);
 
 	return ptr;
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 4/7] lib/i915/gem_mman: update mmap_offset_types with FIXED
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 2/7] lib/i915/gem_mman: add fixed mode to mmap__device_coherent Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 3/7] lib/i915/gem_mman: add fixed mode to mmap__cpu_coherent Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete Matthew Auld
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

We need to also iterate the fixed mode in the tests which rely on this.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/i915/gem_mman.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index 337d28fb..fe4963f0 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -611,6 +611,7 @@ const struct mmap_offset mmap_offset_types[] = {
 	{ "wb", I915_MMAP_OFFSET_WB, I915_GEM_DOMAIN_CPU },
 	{ "wc", I915_MMAP_OFFSET_WC, I915_GEM_DOMAIN_WC },
 	{ "uc", I915_MMAP_OFFSET_UC, I915_GEM_DOMAIN_WC },
+	{ "fixed", LOCAL_I915_MMAP_OFFSET_FIXED, 0},
 	{},
 };
 
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
                   ` (2 preceding siblings ...)
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 4/7] lib/i915/gem_mman: update mmap_offset_types with FIXED Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-27 21:51   ` Dixit, Ashutosh
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 6/7] lib/ioctl_wrappers: update set_domain " Matthew Auld
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

We can no longer just call get_caching or set_domain, and the mmap mode
must be FIXED. This should bring back gem_exec_basic and a few others in
CI on DG1.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/ioctl_wrappers.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 25c5e495..7e27a1b3 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -339,7 +339,18 @@ static void mmap_write(int fd, uint32_t handle, uint64_t offset,
 	if (!length)
 		return;
 
-	if (is_cache_coherent(fd, handle)) {
+	if (gem_has_lmem(fd)) {
+		/*
+		 * set/get_caching and set_domain are no longer supported on
+		 * discrete, also the only mmap mode supportd is FIXED.
+		 */
+		map = gem_mmap_offset__fixed(fd, handle, 0,
+					     offset + length,
+					     PROT_READ | PROT_WRITE);
+		igt_assert_eq(gem_wait(fd, handle, 0), 0);
+	}
+
+	if (!map && is_cache_coherent(fd, handle)) {
 		/* offset arg for mmap functions must be 0 */
 		map = __gem_mmap__cpu_coherent(fd, handle, 0, offset + length,
 					       PROT_READ | PROT_WRITE);
@@ -369,7 +380,17 @@ static void mmap_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint6
 	if (!length)
 		return;
 
-	if (gem_has_llc(fd) || is_cache_coherent(fd, handle)) {
+	if (gem_has_lmem(fd)) {
+		/*
+		 * set/get_caching and set_domain are no longer supported on
+		 * discrete, also the only supported mmap mode is FIXED.
+		 */
+		map = gem_mmap_offset__fixed(fd, handle, 0,
+					     offset + length, PROT_READ);
+		igt_assert_eq(gem_wait(fd, handle, 0), 0);
+	}
+
+	if (!map && (gem_has_llc(fd) || is_cache_coherent(fd, handle))) {
 		/* offset arg for mmap functions must be 0 */
 		map = __gem_mmap__cpu_coherent(fd, handle, 0,
 					       offset + length, PROT_READ);
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 6/7] lib/ioctl_wrappers: update set_domain for discrete
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
                   ` (3 preceding siblings ...)
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 7/7] tests/i915/module_load: update " Matthew Auld
  2021-07-28  2:01 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Dixit, Ashutosh
  6 siblings, 0 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

On discrete set_domain is now gone, instead we just need to add the
wait.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 lib/ioctl_wrappers.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 7e27a1b3..09eb3ce7 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -565,7 +565,12 @@ int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write)
  */
 void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write)
 {
-	igt_assert_eq(__gem_set_domain(fd, handle, read, write), 0);
+	int ret = __gem_set_domain(fd, handle, read, write);
+
+	if (ret == -ENODEV && gem_has_lmem(fd))
+		igt_assert_eq(gem_wait(fd, handle, 0), 0);
+	else
+		igt_assert_eq(ret, 0);
 }
 
 /**
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Intel-gfx] [PATCH i-g-t 7/7] tests/i915/module_load: update for discrete
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
                   ` (4 preceding siblings ...)
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 6/7] lib/ioctl_wrappers: update set_domain " Matthew Auld
@ 2021-07-26 12:03 ` Matthew Auld
  2021-07-28  2:01 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Dixit, Ashutosh
  6 siblings, 0 replies; 13+ messages in thread
From: Matthew Auld @ 2021-07-26 12:03 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter, intel-gfx

The set_caching ioctl is gone for discrete, and now just returns
-ENODEV. Update the gem_sanitycheck to account for that. After this we
should be back to just having the breakage caused by missing reloc
support for the reload testcase.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
---
 tests/i915/i915_module_load.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/i915/i915_module_load.c b/tests/i915/i915_module_load.c
index 98ceb5d8..4b42fe3e 100644
--- a/tests/i915/i915_module_load.c
+++ b/tests/i915/i915_module_load.c
@@ -172,17 +172,22 @@ static void gem_sanitycheck(void)
 {
 	struct drm_i915_gem_caching args = {};
 	int i915 = __drm_open_driver(DRIVER_INTEL);
+	int expected;
 	int err;
 
+	expected = -ENOENT;
+	if (gem_has_lmem(i915))
+		expected = -ENODEV;
+
 	err = 0;
 	if (ioctl(i915, DRM_IOCTL_I915_GEM_SET_CACHING, &args))
 		err = -errno;
-	if (err == -ENOENT)
+	if (err == expected)
 		store_all(i915);
 	errno = 0;
 
 	close(i915);
-	igt_assert_eq(err, -ENOENT);
+	igt_assert_eq(err, expected);
 }
 
 static void
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete Matthew Auld
@ 2021-07-27 21:51   ` Dixit, Ashutosh
  0 siblings, 0 replies; 13+ messages in thread
From: Dixit, Ashutosh @ 2021-07-27 21:51 UTC (permalink / raw)
  To: Matthew Auld; +Cc: igt-dev, Daniel Vetter, intel-gfx

On Mon, 26 Jul 2021 05:03:08 -0700, Matthew Auld wrote:
>
> We can no longer just call get_caching or set_domain, and the mmap mode
> must be FIXED. This should bring back gem_exec_basic and a few others in
> CI on DG1.

We should probably also similarly update mmap_{read, write} in
lib/intel_bufops.c.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
  2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
                   ` (5 preceding siblings ...)
  2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 7/7] tests/i915/module_load: update " Matthew Auld
@ 2021-07-28  2:01 ` Dixit, Ashutosh
  2021-07-28  5:12   ` Dixit, Ashutosh
  2021-07-28  6:08   ` Petri Latvala
  6 siblings, 2 replies; 13+ messages in thread
From: Dixit, Ashutosh @ 2021-07-28  2:01 UTC (permalink / raw)
  To: Matthew Auld; +Cc: igt-dev, intel-gfx

On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote:
>
> diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> index 4b4f2114..e2514f0c 100644
> --- a/lib/i915/gem_mman.c
> +++ b/lib/i915/gem_mman.c
> @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
>	return ptr;
>  }
>
> +#define LOCAL_I915_MMAP_OFFSET_FIXED 4

Cc: @Petri Latvala

This use of LOCAL declarations is more related to the methodology we follow
in IGT rather than this patch. We have seen in the past that such LOCAL's
linger on in the code for months and years till someone decides to clean
them so the question is can we prevent these LOCAL's from getting
introduced in the first place.

One reason for these is that we sync IGT headers with drm-next whereas IGT
is used to test drm-tip. So the delta between the two results in such
LOCAL's as in this case.

My proposal is that even if we don't start sync'ing IGT headers with
drm-tip (instead of drm-next) we allow direct modification of the headers
when needed to avoid introducing such LOCAL's. So in the above case we
would add:

#define I915_MMAP_OFFSET_FIXED 4

to i915_drm.h as part of this patch and then just use
I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this
#define has appeared there, the compile will break and whoever syncs will
need to add this again to i915_drm.h.

What do people think about a scheme such as this? The other, perhaps
better, option of course is to sync the headers directly with drm-tip
(whenever and as often as needed). But the goal in both cases is to avoid
LOCAL's, or other things like #ifndef's distributed throughout multiple
source files which we also do in such cases. A centralized internal header
to contain such declarations might not be so bad. Thanks.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
  2021-07-28  2:01 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Dixit, Ashutosh
@ 2021-07-28  5:12   ` Dixit, Ashutosh
  2021-07-28  6:08   ` Petri Latvala
  1 sibling, 0 replies; 13+ messages in thread
From: Dixit, Ashutosh @ 2021-07-28  5:12 UTC (permalink / raw)
  To: igt-dev, intel-gfx, Petri Latvala

On Tue, 27 Jul 2021 19:01:24 -0700, Dixit, Ashutosh wrote:
>
> On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote:
> >
> > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > index 4b4f2114..e2514f0c 100644
> > --- a/lib/i915/gem_mman.c
> > +++ b/lib/i915/gem_mman.c
> > @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
> >	return ptr;
> >  }
> >
> > +#define LOCAL_I915_MMAP_OFFSET_FIXED 4
>
> Cc: @Petri Latvala
>
> This use of LOCAL declarations is more related to the methodology we follow
> in IGT rather than this patch. We have seen in the past that such LOCAL's
> linger on in the code for months and years till someone decides to clean
> them so the question is can we prevent these LOCAL's from getting
> introduced in the first place.
>
> One reason for these is that we sync IGT headers with drm-next whereas IGT
> is used to test drm-tip. So the delta between the two results in such
> LOCAL's as in this case.
>
> My proposal is that even if we don't start sync'ing IGT headers with
> drm-tip (instead of drm-next) we allow direct modification of the headers
> when needed to avoid introducing such LOCAL's. So in the above case we
> would add:
>
> #define I915_MMAP_OFFSET_FIXED 4
>
> to i915_drm.h as part of this patch and then just use
> I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this
> #define has appeared there, the compile will break and whoever syncs will
> need to add this again to i915_drm.h.
>
> What do people think about a scheme such as this? The other, perhaps
> better, option of course is to sync the headers directly with drm-tip
> (whenever and as often as needed). But the goal in both cases is to avoid
> LOCAL's, or other things like #ifndef's distributed throughout multiple
> source files which we also do in such cases. A centralized internal header
> to contain such declarations might not be so bad. Thanks.

I have submitted a patch based on this last statement here:

https://patchwork.freedesktop.org/series/93096/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
  2021-07-28  2:01 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Dixit, Ashutosh
  2021-07-28  5:12   ` Dixit, Ashutosh
@ 2021-07-28  6:08   ` Petri Latvala
  2021-07-29  1:53     ` Dixit, Ashutosh
  1 sibling, 1 reply; 13+ messages in thread
From: Petri Latvala @ 2021-07-28  6:08 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: igt-dev, intel-gfx, Matthew Auld

On Tue, Jul 27, 2021 at 07:01:24PM -0700, Dixit, Ashutosh wrote:
> On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote:
> >
> > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > index 4b4f2114..e2514f0c 100644
> > --- a/lib/i915/gem_mman.c
> > +++ b/lib/i915/gem_mman.c
> > @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
> >	return ptr;
> >  }
> >
> > +#define LOCAL_I915_MMAP_OFFSET_FIXED 4
> 
> Cc: @Petri Latvala
> 
> This use of LOCAL declarations is more related to the methodology we follow
> in IGT rather than this patch. We have seen in the past that such LOCAL's
> linger on in the code for months and years till someone decides to clean
> them so the question is can we prevent these LOCAL's from getting
> introduced in the first place.
> 
> One reason for these is that we sync IGT headers with drm-next whereas IGT
> is used to test drm-tip. So the delta between the two results in such
> LOCAL's as in this case.
> 
> My proposal is that even if we don't start sync'ing IGT headers with
> drm-tip (instead of drm-next) we allow direct modification of the headers
> when needed to avoid introducing such LOCAL's. So in the above case we
> would add:
> 
> #define I915_MMAP_OFFSET_FIXED 4
> 
> to i915_drm.h as part of this patch and then just use
> I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this
> #define has appeared there, the compile will break and whoever syncs will
> need to add this again to i915_drm.h.

I don't like that kind of a breakage at all. That enforces mandatory
fixups to some poor developer working on unrelated code who doesn't
necessarily know how to even fix it easily.

Of course an argument can be made that it's an i915 token in an i915
header so it will be the i915 people doing it, but for a general case
that's going to cause more harm than it solves problems, I feel.

> What do people think about a scheme such as this? The other, perhaps
> better, option of course is to sync the headers directly with drm-tip
> (whenever and as often as needed). But the goal in both cases is to avoid
> LOCAL's, or other things like #ifndef's distributed throughout multiple
> source files which we also do in such cases. A centralized internal header
> to contain such declarations might not be so bad. Thanks.

A separate manually written header for new tokens that are not yet in
drm-next might be the least bad of all options. Although now that I've
said it, the perfect world would have new tokens done like this:

#ifndef I915_MMAP_OFFSET_FIXED
#define I915_MMAP_OFFSET_FIXED 4
#else
_Static_assert(I915_MMAP_OFFSET_FIXED == 4, "ABI broken, yikes");
#endif

In a different language wrapping all that in a

MAYBE_DECLARE(I915_MMAP_OFFSET_FIXED, 4)

might be easier but with C preprocessor it's a bit more... involved. A
separate build-time script to generate that header maybe? Such a
script could also just completely omit the definition if header copies
already introduce the token.

Recap:

1) We have kernel headers copied into IGT to ensure it builds fine
without latest-and-greatest headers installed on the system.

2) Copies are from drm-next to ensure the next person to copy the
headers doesn't accidentally drop definitions that originate from a
vendor-specific tree. (That same reason is also for why one shouldn't
edit the headers manually)

3) To get to drm-next, the kernel code needs to be tested with IGT
first, so we need new definitions to test that kernel code in some
form.

4) LOCAL_* definitions that are cleaned up later when actual
definitions reach drm-next sounds good in theory but in practice the
cleaning part is often forgotten.



Either way, I think the code using new definitions should use the
intended final names so we should just entirely drop the practice of
declaring anything LOCAL_*. That way the cleanup is limited to one
place.


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
  2021-07-28  6:08   ` Petri Latvala
@ 2021-07-29  1:53     ` Dixit, Ashutosh
  2021-07-29  8:22       ` Petri Latvala
  0 siblings, 1 reply; 13+ messages in thread
From: Dixit, Ashutosh @ 2021-07-29  1:53 UTC (permalink / raw)
  To: Petri Latvala; +Cc: igt-dev, intel-gfx, Matthew Auld

On Tue, 27 Jul 2021 23:08:40 -0700, Petri Latvala wrote:
>
> On Tue, Jul 27, 2021 at 07:01:24PM -0700, Dixit, Ashutosh wrote:
> > On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote:
> > >
> > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > > index 4b4f2114..e2514f0c 100644
> > > --- a/lib/i915/gem_mman.c
> > > +++ b/lib/i915/gem_mman.c
> > > @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
> > >	return ptr;
> > >  }
> > >
> > > +#define LOCAL_I915_MMAP_OFFSET_FIXED 4
> >
> > Cc: @Petri Latvala
> >
> > This use of LOCAL declarations is more related to the methodology we follow
> > in IGT rather than this patch. We have seen in the past that such LOCAL's
> > linger on in the code for months and years till someone decides to clean
> > them so the question is can we prevent these LOCAL's from getting
> > introduced in the first place.
> >
> > One reason for these is that we sync IGT headers with drm-next whereas IGT
> > is used to test drm-tip. So the delta between the two results in such
> > LOCAL's as in this case.
> >
> > My proposal is that even if we don't start sync'ing IGT headers with
> > drm-tip (instead of drm-next) we allow direct modification of the headers
> > when needed to avoid introducing such LOCAL's. So in the above case we
> > would add:
> >
> > #define I915_MMAP_OFFSET_FIXED 4
> >
> > to i915_drm.h as part of this patch and then just use
> > I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this
> > #define has appeared there, the compile will break and whoever syncs will
> > need to add this again to i915_drm.h.
>
> I don't like that kind of a breakage at all. That enforces mandatory
> fixups to some poor developer working on unrelated code who doesn't
> necessarily know how to even fix it easily.
>
> Of course an argument can be made that it's an i915 token in an i915
> header so it will be the i915 people doing it, but for a general case
> that's going to cause more harm than it solves problems, I feel.

OK, let's not change anything with the headers we import for now.

> > What do people think about a scheme such as this? The other, perhaps
> > better, option of course is to sync the headers directly with drm-tip
> > (whenever and as often as needed). But the goal in both cases is to avoid
> > LOCAL's, or other things like #ifndef's distributed throughout multiple
> > source files which we also do in such cases. A centralized internal header
> > to contain such declarations might not be so bad. Thanks.
>
> A separate manually written header for new tokens that are not yet in
> drm-next might be the least bad of all options. Although now that I've
> said it, the perfect world would have new tokens done like this:
>
> #ifndef I915_MMAP_OFFSET_FIXED
> #define I915_MMAP_OFFSET_FIXED 4
> #else
> _Static_assert(I915_MMAP_OFFSET_FIXED == 4, "ABI broken, yikes");
> #endif
>
> In a different language wrapping all that in a
>
> MAYBE_DECLARE(I915_MMAP_OFFSET_FIXED, 4)
>
> might be easier but with C preprocessor it's a bit more... involved. A
> separate build-time script to generate that header maybe? Such a
> script could also just completely omit the definition if header copies
> already introduce the token.

IMO all this wouldn't do much more that what gcc already does. For example,
for this:

#define I915_MMAP_OFFSET_FIXED 4
#define I915_MMAP_OFFSET_FIXED 4

gcc silently ignores the second #define, whereas for:

#define I915_MMAP_OFFSET_FIXED 4
#define I915_MMAP_OFFSET_FIXED 5

gcc will warn that second I915_MMAP_OFFSET_FIXED is redefined.

And gcc will error out on things like struct redeclaration.

> Recap:
>
> 1) We have kernel headers copied into IGT to ensure it builds fine
> without latest-and-greatest headers installed on the system.
>
> 2) Copies are from drm-next to ensure the next person to copy the
> headers doesn't accidentally drop definitions that originate from a
> vendor-specific tree. (That same reason is also for why one shouldn't
> edit the headers manually)
>
> 3) To get to drm-next, the kernel code needs to be tested with IGT
> first, so we need new definitions to test that kernel code in some
> form.

I guess it is possible to test with "Test-with:" and merge the kernel
changes first and the IGT changes later with the correct headers but maybe
it's inconvenient? But don't we merge the kernel changes before IGT?

> 4) LOCAL_* definitions that are cleaned up later when actual
> definitions reach drm-next sounds good in theory but in practice the
> cleaning part is often forgotten.
>
> Either way, I think the code using new definitions should use the
> intended final names so we should just entirely drop the practice of
> declaring anything LOCAL_*. That way the cleanup is limited to one
> place.

In any case, any thoughts about the i915_drm_local.h patch I posted:

https://patchwork.freedesktop.org/series/93096/

I am not asking for any other changes to anything at this this point. I
have also started asking people to not use the LOCAL_ or local_ prefix any
more in code reviews. But I probably still prefer that these declarations
move to a central place such as i915_drm_local.h if possible so it's easier
to clean them up later. Thanks.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode
  2021-07-29  1:53     ` Dixit, Ashutosh
@ 2021-07-29  8:22       ` Petri Latvala
  0 siblings, 0 replies; 13+ messages in thread
From: Petri Latvala @ 2021-07-29  8:22 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: igt-dev, intel-gfx, Matthew Auld

On Wed, Jul 28, 2021 at 06:53:50PM -0700, Dixit, Ashutosh wrote:
> On Tue, 27 Jul 2021 23:08:40 -0700, Petri Latvala wrote:
> >
> > On Tue, Jul 27, 2021 at 07:01:24PM -0700, Dixit, Ashutosh wrote:
> > > On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote:
> > > >
> > > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > > > index 4b4f2114..e2514f0c 100644
> > > > --- a/lib/i915/gem_mman.c
> > > > +++ b/lib/i915/gem_mman.c
> > > > @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset,
> > > >	return ptr;
> > > >  }
> > > >
> > > > +#define LOCAL_I915_MMAP_OFFSET_FIXED 4
> > >
> > > Cc: @Petri Latvala
> > >
> > > This use of LOCAL declarations is more related to the methodology we follow
> > > in IGT rather than this patch. We have seen in the past that such LOCAL's
> > > linger on in the code for months and years till someone decides to clean
> > > them so the question is can we prevent these LOCAL's from getting
> > > introduced in the first place.
> > >
> > > One reason for these is that we sync IGT headers with drm-next whereas IGT
> > > is used to test drm-tip. So the delta between the two results in such
> > > LOCAL's as in this case.
> > >
> > > My proposal is that even if we don't start sync'ing IGT headers with
> > > drm-tip (instead of drm-next) we allow direct modification of the headers
> > > when needed to avoid introducing such LOCAL's. So in the above case we
> > > would add:
> > >
> > > #define I915_MMAP_OFFSET_FIXED 4
> > >
> > > to i915_drm.h as part of this patch and then just use
> > > I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this
> > > #define has appeared there, the compile will break and whoever syncs will
> > > need to add this again to i915_drm.h.
> >
> > I don't like that kind of a breakage at all. That enforces mandatory
> > fixups to some poor developer working on unrelated code who doesn't
> > necessarily know how to even fix it easily.
> >
> > Of course an argument can be made that it's an i915 token in an i915
> > header so it will be the i915 people doing it, but for a general case
> > that's going to cause more harm than it solves problems, I feel.
> 
> OK, let's not change anything with the headers we import for now.
> 
> > > What do people think about a scheme such as this? The other, perhaps
> > > better, option of course is to sync the headers directly with drm-tip
> > > (whenever and as often as needed). But the goal in both cases is to avoid
> > > LOCAL's, or other things like #ifndef's distributed throughout multiple
> > > source files which we also do in such cases. A centralized internal header
> > > to contain such declarations might not be so bad. Thanks.
> >
> > A separate manually written header for new tokens that are not yet in
> > drm-next might be the least bad of all options. Although now that I've
> > said it, the perfect world would have new tokens done like this:
> >
> > #ifndef I915_MMAP_OFFSET_FIXED
> > #define I915_MMAP_OFFSET_FIXED 4
> > #else
> > _Static_assert(I915_MMAP_OFFSET_FIXED == 4, "ABI broken, yikes");
> > #endif
> >
> > In a different language wrapping all that in a
> >
> > MAYBE_DECLARE(I915_MMAP_OFFSET_FIXED, 4)
> >
> > might be easier but with C preprocessor it's a bit more... involved. A
> > separate build-time script to generate that header maybe? Such a
> > script could also just completely omit the definition if header copies
> > already introduce the token.
> 
> IMO all this wouldn't do much more that what gcc already does. For example,
> for this:
> 
> #define I915_MMAP_OFFSET_FIXED 4
> #define I915_MMAP_OFFSET_FIXED 4
> 
> gcc silently ignores the second #define, whereas for:
> 
> #define I915_MMAP_OFFSET_FIXED 4
> #define I915_MMAP_OFFSET_FIXED 5
> 
> gcc will warn that second I915_MMAP_OFFSET_FIXED is redefined.
> 
> And gcc will error out on things like struct redeclaration.

Ah that's good then!

(For the record, C99 6.10.3 states this is even standard C behaviour)


> 
> > Recap:
> >
> > 1) We have kernel headers copied into IGT to ensure it builds fine
> > without latest-and-greatest headers installed on the system.
> >
> > 2) Copies are from drm-next to ensure the next person to copy the
> > headers doesn't accidentally drop definitions that originate from a
> > vendor-specific tree. (That same reason is also for why one shouldn't
> > edit the headers manually)
> >
> > 3) To get to drm-next, the kernel code needs to be tested with IGT
> > first, so we need new definitions to test that kernel code in some
> > form.
> 
> I guess it is possible to test with "Test-with:" and merge the kernel
> changes first and the IGT changes later with the correct headers but maybe
> it's inconvenient? But don't we merge the kernel changes before IGT?

Merge rules for kernel vs. userspace is like that, yes. But IGT
doesn't count as userspace for those rules and merging order can be
more relaxed.


> 
> > 4) LOCAL_* definitions that are cleaned up later when actual
> > definitions reach drm-next sounds good in theory but in practice the
> > cleaning part is often forgotten.
> >
> > Either way, I think the code using new definitions should use the
> > intended final names so we should just entirely drop the practice of
> > declaring anything LOCAL_*. That way the cleanup is limited to one
> > place.
> 
> In any case, any thoughts about the i915_drm_local.h patch I posted:
> 
> https://patchwork.freedesktop.org/series/93096/
> 
> I am not asking for any other changes to anything at this this point. I
> have also started asking people to not use the LOCAL_ or local_ prefix any
> more in code reviews. But I probably still prefer that these declarations
> move to a central place such as i915_drm_local.h if possible so it's easier
> to clean them up later. Thanks.

Going to reply on that patch in a bit.


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-07-29  8:20 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 12:03 [Intel-gfx] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Matthew Auld
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 2/7] lib/i915/gem_mman: add fixed mode to mmap__device_coherent Matthew Auld
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 3/7] lib/i915/gem_mman: add fixed mode to mmap__cpu_coherent Matthew Auld
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 4/7] lib/i915/gem_mman: update mmap_offset_types with FIXED Matthew Auld
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 5/7] lib/ioctl_wrappers: update mmap_{read, write} for discrete Matthew Auld
2021-07-27 21:51   ` Dixit, Ashutosh
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 6/7] lib/ioctl_wrappers: update set_domain " Matthew Auld
2021-07-26 12:03 ` [Intel-gfx] [PATCH i-g-t 7/7] tests/i915/module_load: update " Matthew Auld
2021-07-28  2:01 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode Dixit, Ashutosh
2021-07-28  5:12   ` Dixit, Ashutosh
2021-07-28  6:08   ` Petri Latvala
2021-07-29  1:53     ` Dixit, Ashutosh
2021-07-29  8:22       ` Petri Latvala

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).