[Nouveau] [PATCH i-g-t 0/7] lib: Miscellaneous cleanups from nouveau work

[Nouveau] [PATCH i-g-t 0/7] lib: Miscellaneous cleanups from nouveau work

[Lyude]

From: Lyude Paul <lyude@redhat.com>

These are just a couple of small fixes that didn't seem important
enough to stick in their own patch series, but were various issues with
igt that I found along the way during my recent nouveau igt work.

Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>

Lyude Paul (7):
  lib/drmtest: Use igt_assert_eq() for do_or_die()
  lib/drmtest: And use do_or_die() in do_ioctl()
  lib/drmtest: Remove i915 refs in do_ioctl*() docs
  lib/igt_fb: Remove domain from igt_fb
  lib/debugfs: Fix igt_crc_t docs to mention has_valid_frame
  lib/igt_aux: Print name of debug var in igt_debug_wait_for_keypress()
  lib/debugfs: Fix function name in igt_pipe_crc_get_for_frame() docs

 lib/drmtest.h     | 8 ++++----
 lib/igt_aux.c     | 2 +-
 lib/igt_debugfs.c | 2 +-
 lib/igt_debugfs.h | 3 ++-
 lib/igt_fb.c      | 3 ---
 lib/igt_fb.h      | 2 --
 6 files changed, 8 insertions(+), 12 deletions(-)

-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 1/7] lib/drmtest: Use igt_assert_eq() for do_or_die()

[Lyude]

From: Lyude Paul <lyude@redhat.com>

Noticed this while working on some nouveau tests, if we use igt_assert_eq()
here we'll output both the expected and returned value instead of just the
expected value.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/drmtest.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/drmtest.h b/lib/drmtest.h
index d393dbf1..789452ea 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -113,7 +113,7 @@ bool is_vc4_device(int fd);
  * that in any failure case the return value is non-zero and a precise error is
  * logged into errno. Uses igt_assert() internally.
  */
-#define do_or_die(x) igt_assert((x) == 0)
+#define do_or_die(x) igt_assert_eq((x), 0)
 
 /**
  * do_ioctl:
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [igt-dev] [PATCH i-g-t 1/7] lib/drmtest: Use igt_assert_eq() for do_or_die()

[Petri Latvala]

On Wed, Mar 17, 2021 at 06:49:43PM -0400, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> Noticed this while working on some nouveau tests, if we use igt_assert_eq()
> here we'll output both the expected and returned value instead of just the
> expected value.

igt_assert_eq stuffs x into an int so there's one difference.

Hmm, but generally do_or_die is used with ioctl() or something that
wraps it and it will be an int.


> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Martin Peres <martin.peres@free.fr>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Jeremy Cline <jcline@redhat.com>
> ---
>  lib/drmtest.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index d393dbf1..789452ea 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -113,7 +113,7 @@ bool is_vc4_device(int fd);
>   * that in any failure case the return value is non-zero and a precise error is
>   * logged into errno. Uses igt_assert() internally.

A minor cosmetic change to this comment (igt_assert usage) is welcome.



Reviewed-by: Petri Latvala <petri.latvala@intel.com>


>   */
> -#define do_or_die(x) igt_assert((x) == 0)
> +#define do_or_die(x) igt_assert_eq((x), 0)
>  
>  /**
>   * do_ioctl:
> -- 
> 2.29.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 2/7] lib/drmtest: And use do_or_die() in do_ioctl()

[Lyude]

From: Lyude Paul <lyude@redhat.com>

No need to use igt_assert_eq() here now that do_or_die() uses
igt_assert_eq()

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/drmtest.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/drmtest.h b/lib/drmtest.h
index 789452ea..c4b15a26 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -125,7 +125,7 @@ bool is_vc4_device(int fd);
  * successfully executed.
  */
 #define do_ioctl(fd, ioc, ioc_data) do { \
-	igt_assert_eq(igt_ioctl((fd), (ioc), (ioc_data)), 0); \
+	do_or_die(igt_ioctl((fd), (ioc), (ioc_data))); \
 	errno = 0; \
 } while (0)
 
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 3/7] lib/drmtest: Remove i915 refs in do_ioctl*() docs

[Lyude]

From: Lyude Paul <lyude@redhat.com>

These aren't i915 specific, looks like some historical junk.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/drmtest.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/drmtest.h b/lib/drmtest.h
index c4b15a26..b933f76a 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -117,7 +117,7 @@ bool is_vc4_device(int fd);
 
 /**
  * do_ioctl:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
  * @ioc: ioctl op definition from drm headers
  * @ioc_data: data pointer for the ioctl operation
  *
@@ -131,7 +131,7 @@ bool is_vc4_device(int fd);
 
 /**
  * do_ioctl_err:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
  * @ioc: ioctl op definition from drm headers
  * @ioc_data: data pointer for the ioctl operation
  * @err: value to expect in errno
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 4/7] lib/igt_fb: Remove domain from igt_fb

[Lyude]

From: Lyude Paul <lyude@redhat.com>

It doesn't look like that this is used by i915 for anything these days, so
let's just remove this while we're at it.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/igt_fb.c | 3 ---
 lib/igt_fb.h | 2 --
 2 files changed, 5 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index f0fcd1a7..2b478887 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2491,7 +2491,6 @@ static void create_cairo_surface__gpu(int fd, struct igt_fb *fb)
 						    cairo_format,
 						    fb->width, fb->height,
 						    blit->linear.fb.strides[0]);
-	fb->domain = I915_GEM_DOMAIN_GTT;
 
 	cairo_surface_set_user_data(fb->cairo_surface,
 				    (cairo_user_data_key_t *)create_cairo_surface__gpu,
@@ -2571,8 +2570,6 @@ static void create_cairo_surface__gtt(int fd, struct igt_fb *fb)
 		      "Unable to create a cairo surface: %s\n",
 		      cairo_status_to_string(cairo_surface_status(fb->cairo_surface)));
 
-	fb->domain = I915_GEM_DOMAIN_GTT;
-
 	cairo_surface_set_user_data(fb->cairo_surface,
 				    (cairo_user_data_key_t *)create_cairo_surface__gtt,
 				    fb, destroy_cairo_surface__gtt);
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 0d64898a..e4f8b2b1 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -64,7 +64,6 @@ struct buf_ops;
  * @modifier: tiling mode as a DRM framebuffer modifier
  * @size: size in bytes of the underlying backing storage
  * @cairo_surface: optionally attached cairo drawing surface
- * @domain: current domain for cache flushing tracking on i915.ko
  * @num_planes: Amount of planes on this fb. >1 for planar formats.
  * @strides: line stride for each plane in bytes
  * @offsets: Offset for each plane in bytes.
@@ -87,7 +86,6 @@ typedef struct igt_fb {
 	uint64_t modifier;
 	uint64_t size;
 	cairo_surface_t *cairo_surface;
-	unsigned int domain;
 	unsigned int num_planes;
 	uint32_t strides[4];
 	uint32_t offsets[4];
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [igt-dev] [PATCH i-g-t 4/7] lib/igt_fb: Remove domain from igt_fb

[Ville Syrjälä]

On Wed, Mar 17, 2021 at 06:49:46PM -0400, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> It doesn't look like that this is used by i915 for anything these days, so
> let's just remove this while we're at it.

I wa scurious why this is here so dug around a bit.

It was added in
commit 18d0b1efb875 ("lib/igt_fb: Fix domain tracking for GTT cairo surfaces")

then got broken again by
commit 89b3ffe02c2a ("lib: Map dumb buffers")

and later fixed again with
commit 3bd68529c464 ("lib/igt_fb: Add missing set_domain calls")

So yeah, not needed anymore.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Martin Peres <martin.peres@free.fr>
> Cc: Jeremy Cline <jcline@redhat.com>
> ---
>  lib/igt_fb.c | 3 ---
>  lib/igt_fb.h | 2 --
>  2 files changed, 5 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index f0fcd1a7..2b478887 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2491,7 +2491,6 @@ static void create_cairo_surface__gpu(int fd, struct igt_fb *fb)
>  						    cairo_format,
>  						    fb->width, fb->height,
>  						    blit->linear.fb.strides[0]);
> -	fb->domain = I915_GEM_DOMAIN_GTT;
>  
>  	cairo_surface_set_user_data(fb->cairo_surface,
>  				    (cairo_user_data_key_t *)create_cairo_surface__gpu,
> @@ -2571,8 +2570,6 @@ static void create_cairo_surface__gtt(int fd, struct igt_fb *fb)
>  		      "Unable to create a cairo surface: %s\n",
>  		      cairo_status_to_string(cairo_surface_status(fb->cairo_surface)));
>  
> -	fb->domain = I915_GEM_DOMAIN_GTT;
> -
>  	cairo_surface_set_user_data(fb->cairo_surface,
>  				    (cairo_user_data_key_t *)create_cairo_surface__gtt,
>  				    fb, destroy_cairo_surface__gtt);
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 0d64898a..e4f8b2b1 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -64,7 +64,6 @@ struct buf_ops;
>   * @modifier: tiling mode as a DRM framebuffer modifier
>   * @size: size in bytes of the underlying backing storage
>   * @cairo_surface: optionally attached cairo drawing surface
> - * @domain: current domain for cache flushing tracking on i915.ko
>   * @num_planes: Amount of planes on this fb. >1 for planar formats.
>   * @strides: line stride for each plane in bytes
>   * @offsets: Offset for each plane in bytes.
> @@ -87,7 +86,6 @@ typedef struct igt_fb {
>  	uint64_t modifier;
>  	uint64_t size;
>  	cairo_surface_t *cairo_surface;
> -	unsigned int domain;
>  	unsigned int num_planes;
>  	uint32_t strides[4];
>  	uint32_t offsets[4];
> -- 
> 2.29.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 5/7] lib/debugfs: Fix igt_crc_t docs to mention has_valid_frame

[Lyude]

From: Lyude Paul <lyude@redhat.com>

Looks like this never got updated when has_valid_frame got added.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/igt_debugfs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index d43ba6c6..97cbaa1c 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -70,10 +70,11 @@ typedef struct _igt_pipe_crc igt_pipe_crc_t;
 /**
  * igt_crc_t:
  * @frame: frame number of the capture CRC
+ * @has_valid_frame: whether this CRC entry has a valid frame counter
  * @n_words: internal field, don't access
  * @crc: internal field, don't access
  *
- * Pipe CRC value. All other members than @frame are private and should not be
+ * Pipe CRC value. All other members than @frame and @has_valid_frame are private and should not be
  * inspected by testcases.
  */
 typedef struct {
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 6/7] lib/igt_aux: Print name of debug var in igt_debug_wait_for_keypress()

[Lyude]

From: Lyude Paul <lyude@redhat.com>

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/igt_aux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 1217f5e8..0ab51124 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -997,7 +997,7 @@ void igt_debug_wait_for_keypress(const char *var)
 	    !strstr(igt_interactive_debug, "all"))
 		return;
 
-	igt_info("Press any key to continue ...\n");
+	igt_info("Hit interactive debug var %s, press any key to continue...\n", var);
 
 	tcgetattr ( STDIN_FILENO, &oldt );
 	newt = oldt;
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH i-g-t 7/7] lib/debugfs: Fix function name in igt_pipe_crc_get_for_frame() docs

[Lyude]

From: Lyude Paul <lyude@redhat.com>

Looks like this got broken in ab405a405

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Jeremy Cline <jcline@redhat.com>
---
 lib/igt_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 2f58519a..a9f1cf40 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -972,7 +972,7 @@ void igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
 }
 
 /**
- * igt_pipe_crc_get_current:
+ * igt_pipe_crc_get_for_frame:
  * @drm_fd: Pointer to drm fd for vblank counter
  * @pipe_crc: pipe CRC object
  * @vblank: frame counter value we're looking for
-- 
2.29.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [igt-dev] [PATCH i-g-t 0/7] lib: Miscellaneous cleanups from nouveau work

[Martin Peres]

On 18/03/2021 00:49, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> These are just a couple of small fixes that didn't seem important
> enough to stick in their own patch series, but were various issues with
> igt that I found along the way during my recent nouveau igt work.
> 
> Cc: Martin Peres <martin.peres@free.fr>
> Cc: Jeremy Cline <jcline@redhat.com>

This series looks sensible to me, and do not seem to have affected 
Intel's results.

I'll refrain from giving a full R-b because I lack the historical 
context for some of these changes, but it seems safe to me:

Acked-by: Martin Peres <martin.peres@mupuf.org>

> 
> Lyude Paul (7):
>    lib/drmtest: Use igt_assert_eq() for do_or_die()
>    lib/drmtest: And use do_or_die() in do_ioctl()
>    lib/drmtest: Remove i915 refs in do_ioctl*() docs
>    lib/igt_fb: Remove domain from igt_fb
>    lib/debugfs: Fix igt_crc_t docs to mention has_valid_frame
>    lib/igt_aux: Print name of debug var in igt_debug_wait_for_keypress()
>    lib/debugfs: Fix function name in igt_pipe_crc_get_for_frame() docs
> 
>   lib/drmtest.h     | 8 ++++----
>   lib/igt_aux.c     | 2 +-
>   lib/igt_debugfs.c | 2 +-
>   lib/igt_debugfs.h | 3 ++-
>   lib/igt_fb.c      | 3 ---
>   lib/igt_fb.h      | 2 --
>   6 files changed, 8 insertions(+), 12 deletions(-)
> 
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [igt-dev] [PATCH i-g-t 0/7] lib: Miscellaneous cleanups from nouveau work

[Petri Latvala]

On Wed, Mar 17, 2021 at 06:49:42PM -0400, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> These are just a couple of small fixes that didn't seem important
> enough to stick in their own patch series, but were various issues with
> igt that I found along the way during my recent nouveau igt work.
> 
> Cc: Martin Peres <martin.peres@free.fr>
> Cc: Jeremy Cline <jcline@redhat.com>
> 
> Lyude Paul (7):
>   lib/drmtest: Use igt_assert_eq() for do_or_die()
>   lib/drmtest: And use do_or_die() in do_ioctl()
>   lib/drmtest: Remove i915 refs in do_ioctl*() docs
>   lib/igt_fb: Remove domain from igt_fb
>   lib/debugfs: Fix igt_crc_t docs to mention has_valid_frame
>   lib/igt_aux: Print name of debug var in igt_debug_wait_for_keypress()
>   lib/debugfs: Fix function name in igt_pipe_crc_get_for_frame() docs
> 
>  lib/drmtest.h     | 8 ++++----
>  lib/igt_aux.c     | 2 +-
>  lib/igt_debugfs.c | 2 +-
>  lib/igt_debugfs.h | 3 ++-
>  lib/igt_fb.c      | 3 ---
>  lib/igt_fb.h      | 2 --
>  6 files changed, 8 insertions(+), 12 deletions(-)
> 


One minor comment on one patch, otherwise LGTM.

Series is
Reviewed-by: Petri Latvala <petri.latvala@intel.com>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau