[igt-dev] [PATCH i-g-t 0/9] Chamelium VC4 plane testing, with T-tiled mode

[igt-dev] [PATCH i-g-t 0/9] Chamelium VC4 plane testing, with T-tiled mode

[Paul Kocialkowski]

This series introduces the required plumbing for allocating buffers in
the VC4 T-tiled mode and converting to that mode from XR24 linear
buffers.

With that in place, a new Chamelium test is introduced to perform some
testing of planes with randomized properties. VC4-specific features such
as T-tiled mode, bandwidth limitation and overrun detection are also
tested.

It should be noted that the VC4 driver apaprently does not recover well
from underruns with the Chamelium, leading to "flip_done timed out"
errors in subsequent runs (until a reboot).

This series is based on Maxime Ripard's series:
  igt: chamelium: Test YUV buffers using the Chamelium

Paul Kocialkowski (9):
  chamelium: Pass dimensions instead of mode to pattern generation
    helper
  chamelium: Pass the pattern block size as argument to helpers
  lib: drmtest: Add helpers to check and require the VC4 driver
  lib/igt_fb: Add checks on i915 for i915-specific tiled buffer
    allocation
  lib/igt_fb: Add support for allocating T-tiled VC4 buffers
  lib/igt_vc4: Add helpers for converting linear to T-tiled XRGB buffers
  lib/igt_fb: Add a stride-provisioned fashion of igt_fb_convert
  lib/igt_fb: Add a helper to retreive the plane bpp for a given format
  chamelium: Add a CRC-based display test for randomized planes

 lib/drmtest.c         |  10 ++
 lib/drmtest.h         |   2 +
 lib/igt_fb.c          |  87 ++++++++++-
 lib/igt_fb.h          |   4 +
 lib/igt_vc4.c         |  96 ++++++++++++
 lib/igt_vc4.h         |   4 +
 tests/kms_chamelium.c | 352 ++++++++++++++++++++++++++++++++++++++++--
 7 files changed, 537 insertions(+), 18 deletions(-)

-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 1/9] chamelium: Pass dimensions instead of mode to pattern generation helper

[Paul Kocialkowski]

In order to reuse the pattern generation helper for overlay planes,
let's provide the pattern generation helper with the explicit dimensions
instead of the mode (that only applies to the primary plane).

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 tests/kms_chamelium.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 8a9f6bfe..b8cab927 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -504,7 +504,7 @@ static void chamelium_paint_xr24_pattern(uint32_t *data,
 			*(data + i * stride / 4 + j) = colors[((j / 64) + (i / 64)) % 5];
 }
 
-static int chamelium_get_pattern_fb(data_t *data, drmModeModeInfo *mode,
+static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t height,
 				    uint32_t fourcc, struct igt_fb *fb)
 {
 	int fb_id;
@@ -512,15 +512,14 @@ static int chamelium_get_pattern_fb(data_t *data, drmModeModeInfo *mode,
 
 	igt_assert(fourcc == DRM_FORMAT_XRGB8888);
 
-	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-			      fourcc, LOCAL_DRM_FORMAT_MOD_NONE, fb);
+	fb_id = igt_create_fb(data->drm_fd, width, height, fourcc,
+			      LOCAL_DRM_FORMAT_MOD_NONE, fb);
 	igt_assert(fb_id > 0);
 
 	ptr = igt_fb_map_buffer(fb->fd, fb);
 	igt_assert(ptr);
 
-	chamelium_paint_xr24_pattern(ptr, mode->hdisplay, mode->vdisplay,
-				     fb->strides[0]);
+	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
 	igt_fb_unmap_buffer(fb, ptr);
 
 	return fb_id;
@@ -541,7 +540,7 @@ static void do_test_display(data_t *data, struct chamelium_port *port,
 	int i, fb_id, captured_frame_count;
 	int frame_id;
 
-	fb_id = chamelium_get_pattern_fb(data, mode,
+	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
 					 DRM_FORMAT_XRGB8888, &fb);
 	igt_assert(fb_id > 0);
 
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/9] chamelium: Pass dimensions instead of mode to pattern generation helper

[Lyude Paul]

Are there some patches missing here? This doesn't seem to apply cleanly to
master, and it looks like this is relying on some changes to
chamelium_paint_xr24_pattern() that aren't in the tree

(otherwise, this patch lgtm, Reviewed-by: Lyude Paul <lyude@redhat.com>)

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> In order to reuse the pattern generation helper for overlay planes,
> let's provide the pattern generation helper with the explicit dimensions
> instead of the mode (that only applies to the primary plane).
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  tests/kms_chamelium.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 8a9f6bfe..b8cab927 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -504,7 +504,7 @@ static void chamelium_paint_xr24_pattern(uint32_t *data,
>  			*(data + i * stride / 4 + j) = colors[((j / 64) + (i /
> 64)) % 5];
>  }
>  
> -static int chamelium_get_pattern_fb(data_t *data, drmModeModeInfo *mode,
> +static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t
> height,
>  				    uint32_t fourcc, struct igt_fb *fb)
>  {
>  	int fb_id;
> @@ -512,15 +512,14 @@ static int chamelium_get_pattern_fb(data_t *data,
> drmModeModeInfo *mode,
>  
>  	igt_assert(fourcc == DRM_FORMAT_XRGB8888);
>  
> -	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> -			      fourcc, LOCAL_DRM_FORMAT_MOD_NONE, fb);
> +	fb_id = igt_create_fb(data->drm_fd, width, height, fourcc,
> +			      LOCAL_DRM_FORMAT_MOD_NONE, fb);
>  	igt_assert(fb_id > 0);
>  
>  	ptr = igt_fb_map_buffer(fb->fd, fb);
>  	igt_assert(ptr);
>  
> -	chamelium_paint_xr24_pattern(ptr, mode->hdisplay, mode->vdisplay,
> -				     fb->strides[0]);
> +	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
>  	igt_fb_unmap_buffer(fb, ptr);
>  
>  	return fb_id;
> @@ -541,7 +540,7 @@ static void do_test_display(data_t *data, struct
> chamelium_port *port,
>  	int i, fb_id, captured_frame_count;
>  	int frame_id;
>  
> -	fb_id = chamelium_get_pattern_fb(data, mode,
> +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
>  					 DRM_FORMAT_XRGB8888, &fb);
>  	igt_assert(fb_id > 0);
>  
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/9] chamelium: Pass dimensions instead of mode to pattern generation helper

[Paul Kocialkowski]

Hi,

On Thu, 2018-12-06 at 16:43 -0500, Lyude Paul wrote:
> Are there some patches missing here? This doesn't seem to apply cleanly to
> master, and it looks like this is relying on some changes to
> chamelium_paint_xr24_pattern() that aren't in the tree

Yes, there are definitely missing patches in this series! It's
mentionned in the cover letter, where it says: 

This series is based on Maxime Ripard's series:
  igt: chamelium: Test YUV buffers using the Chamelium

which was not merged yet. Maybe I should better include the patches I
need in my series next time?

> (otherwise, this patch lgtm, Reviewed-by: Lyude Paul <lyude@redhat.com>)

Thanks for reviewing the series!

Cheers,

Paul

> On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> > In order to reuse the pattern generation helper for overlay planes,
> > let's provide the pattern generation helper with the explicit dimensions
> > instead of the mode (that only applies to the primary plane).
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  tests/kms_chamelium.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 8a9f6bfe..b8cab927 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -504,7 +504,7 @@ static void chamelium_paint_xr24_pattern(uint32_t *data,
> >  			*(data + i * stride / 4 + j) = colors[((j / 64) + (i /
> > 64)) % 5];
> >  }
> >  
> > -static int chamelium_get_pattern_fb(data_t *data, drmModeModeInfo *mode,
> > +static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t
> > height,
> >  				    uint32_t fourcc, struct igt_fb *fb)
> >  {
> >  	int fb_id;
> > @@ -512,15 +512,14 @@ static int chamelium_get_pattern_fb(data_t *data,
> > drmModeModeInfo *mode,
> >  
> >  	igt_assert(fourcc == DRM_FORMAT_XRGB8888);
> >  
> > -	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > -			      fourcc, LOCAL_DRM_FORMAT_MOD_NONE, fb);
> > +	fb_id = igt_create_fb(data->drm_fd, width, height, fourcc,
> > +			      LOCAL_DRM_FORMAT_MOD_NONE, fb);
> >  	igt_assert(fb_id > 0);
> >  
> >  	ptr = igt_fb_map_buffer(fb->fd, fb);
> >  	igt_assert(ptr);
> >  
> > -	chamelium_paint_xr24_pattern(ptr, mode->hdisplay, mode->vdisplay,
> > -				     fb->strides[0]);
> > +	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
> >  	igt_fb_unmap_buffer(fb, ptr);
> >  
> >  	return fb_id;
> > @@ -541,7 +540,7 @@ static void do_test_display(data_t *data, struct
> > chamelium_port *port,
> >  	int i, fb_id, captured_frame_count;
> >  	int frame_id;
> >  
> > -	fb_id = chamelium_get_pattern_fb(data, mode,
> > +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> >  					 DRM_FORMAT_XRGB8888, &fb);
> >  	igt_assert(fb_id > 0);
> >  
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/9] chamelium: Pass dimensions instead of mode to pattern generation helper

[Lyude Paul]

On Fri, 2018-12-07 at 08:37 +0100, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu, 2018-12-06 at 16:43 -0500, Lyude Paul wrote:
> > Are there some patches missing here? This doesn't seem to apply cleanly to
> > master, and it looks like this is relying on some changes to
> > chamelium_paint_xr24_pattern() that aren't in the tree
> 
> Yes, there are definitely missing patches in this series! It's
> mentionned in the cover letter, where it says: 
> 
> This series is based on Maxime Ripard's series:
>   igt: chamelium: Test YUV buffers using the Chamelium
> 
> which was not merged yet. Maybe I should better include the patches I
> need in my series next time?

Whoops! My fault for missing that.

Also, does Maxime have push privileges with igt? I just want to make sure you
don't have a bunch of work on the ML that has gotten reviewed by someone but
hasn't gotten pushed anywhere
> 
> > (otherwise, this patch lgtm, Reviewed-by: Lyude Paul <lyude@redhat.com>)
> 
> Thanks for reviewing the series!
> 
> Cheers,
> 
> Paul
> 
> > On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> > > In order to reuse the pattern generation helper for overlay planes,
> > > let's provide the pattern generation helper with the explicit dimensions
> > > instead of the mode (that only applies to the primary plane).
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  tests/kms_chamelium.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > index 8a9f6bfe..b8cab927 100644
> > > --- a/tests/kms_chamelium.c
> > > +++ b/tests/kms_chamelium.c
> > > @@ -504,7 +504,7 @@ static void chamelium_paint_xr24_pattern(uint32_t
> > > *data,
> > >  			*(data + i * stride / 4 + j) = colors[((j / 64) + (i /
> > > 64)) % 5];
> > >  }
> > >  
> > > -static int chamelium_get_pattern_fb(data_t *data, drmModeModeInfo
> > > *mode,
> > > +static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t
> > > height,
> > >  				    uint32_t fourcc, struct igt_fb *fb)
> > >  {
> > >  	int fb_id;
> > > @@ -512,15 +512,14 @@ static int chamelium_get_pattern_fb(data_t *data,
> > > drmModeModeInfo *mode,
> > >  
> > >  	igt_assert(fourcc == DRM_FORMAT_XRGB8888);
> > >  
> > > -	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > > -			      fourcc, LOCAL_DRM_FORMAT_MOD_NONE, fb);
> > > +	fb_id = igt_create_fb(data->drm_fd, width, height, fourcc,
> > > +			      LOCAL_DRM_FORMAT_MOD_NONE, fb);
> > >  	igt_assert(fb_id > 0);
> > >  
> > >  	ptr = igt_fb_map_buffer(fb->fd, fb);
> > >  	igt_assert(ptr);
> > >  
> > > -	chamelium_paint_xr24_pattern(ptr, mode->hdisplay, mode->vdisplay,
> > > -				     fb->strides[0]);
> > > +	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
> > >  	igt_fb_unmap_buffer(fb, ptr);
> > >  
> > >  	return fb_id;
> > > @@ -541,7 +540,7 @@ static void do_test_display(data_t *data, struct
> > > chamelium_port *port,
> > >  	int i, fb_id, captured_frame_count;
> > >  	int frame_id;
> > >  
> > > -	fb_id = chamelium_get_pattern_fb(data, mode,
> > > +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> > >  					 DRM_FORMAT_XRGB8888, &fb);
> > >  	igt_assert(fb_id > 0);
> > >  
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/9] chamelium: Pass the pattern block size as argument to helpers

[Paul Kocialkowski]

This adds a new block size argument to the pattern generation helpers so
that different sizes of blocks can be used.

In the future, this allows us to use different block sizes when testing
overlay planes, making it visually explicit what is part of the main
plane and what is part of the overlay plane.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 tests/kms_chamelium.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index b8cab927..7d95a8bc 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -490,7 +490,7 @@ enable_output(data_t *data,
 
 static void chamelium_paint_xr24_pattern(uint32_t *data,
 					 size_t width, size_t height,
-					 size_t stride)
+					 size_t stride, size_t block_size)
 {
 	uint32_t colors[] = { 0xff000000,
 			      0xffff0000,
@@ -501,11 +501,12 @@ static void chamelium_paint_xr24_pattern(uint32_t *data,
 
 	for (i = 0; i < height; i++)
 		for (j = 0; j < width; j++)
-			*(data + i * stride / 4 + j) = colors[((j / 64) + (i / 64)) % 5];
+			*(data + i * stride / 4 + j) = colors[((j / block_size) + (i / block_size)) % 5];
 }
 
 static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t height,
-				    uint32_t fourcc, struct igt_fb *fb)
+				    uint32_t fourcc, size_t block_size,
+				    struct igt_fb *fb)
 {
 	int fb_id;
 	void *ptr;
@@ -519,7 +520,8 @@ static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t height,
 	ptr = igt_fb_map_buffer(fb->fd, fb);
 	igt_assert(ptr);
 
-	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
+	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0],
+				     block_size);
 	igt_fb_unmap_buffer(fb, ptr);
 
 	return fb_id;
@@ -541,7 +543,7 @@ static void do_test_display(data_t *data, struct chamelium_port *port,
 	int frame_id;
 
 	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
-					 DRM_FORMAT_XRGB8888, &fb);
+					 DRM_FORMAT_XRGB8888, 64, &fb);
 	igt_assert(fb_id > 0);
 
 	frame_id = igt_fb_convert(&frame_fb, &fb, fourcc);
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/9] chamelium: Pass the pattern block size as argument to helpers

[Lyude Paul]

With the missing patch issues I mentioned on the first patch in this series
fixed

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> This adds a new block size argument to the pattern generation helpers so
> that different sizes of blocks can be used.
> 
> In the future, this allows us to use different block sizes when testing
> overlay planes, making it visually explicit what is part of the main
> plane and what is part of the overlay plane.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  tests/kms_chamelium.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index b8cab927..7d95a8bc 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -490,7 +490,7 @@ enable_output(data_t *data,
>  
>  static void chamelium_paint_xr24_pattern(uint32_t *data,
>  					 size_t width, size_t height,
> -					 size_t stride)
> +					 size_t stride, size_t block_size)
>  {
>  	uint32_t colors[] = { 0xff000000,
>  			      0xffff0000,
> @@ -501,11 +501,12 @@ static void chamelium_paint_xr24_pattern(uint32_t
> *data,
>  
>  	for (i = 0; i < height; i++)
>  		for (j = 0; j < width; j++)
> -			*(data + i * stride / 4 + j) = colors[((j / 64) + (i /
> 64)) % 5];
> +			*(data + i * stride / 4 + j) = colors[((j /
> block_size) + (i / block_size)) % 5];
>  }
>  
>  static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t
> height,
> -				    uint32_t fourcc, struct igt_fb *fb)
> +				    uint32_t fourcc, size_t block_size,
> +				    struct igt_fb *fb)
>  {
>  	int fb_id;
>  	void *ptr;
> @@ -519,7 +520,8 @@ static int chamelium_get_pattern_fb(data_t *data, size_t
> width, size_t height,
>  	ptr = igt_fb_map_buffer(fb->fd, fb);
>  	igt_assert(ptr);
>  
> -	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0]);
> +	chamelium_paint_xr24_pattern(ptr, width, height, fb->strides[0],
> +				     block_size);
>  	igt_fb_unmap_buffer(fb, ptr);
>  
>  	return fb_id;
> @@ -541,7 +543,7 @@ static void do_test_display(data_t *data, struct
> chamelium_port *port,
>  	int frame_id;
>  
>  	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> -					 DRM_FORMAT_XRGB8888, &fb);
> +					 DRM_FORMAT_XRGB8888, 64, &fb);
>  	igt_assert(fb_id > 0);
>  
>  	frame_id = igt_fb_convert(&frame_fb, &fb, fourcc);
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 3/9] lib: drmtest: Add helpers to check and require the VC4 driver

[Paul Kocialkowski]

In order to add support for features specific to the VC4 driver, add
helpers for checking and requiring the driver like it's done for the
i915 driver.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/drmtest.c | 10 ++++++++++
 lib/drmtest.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index d2aa1c19..53ea7598 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -105,6 +105,11 @@ bool is_i915_device(int fd)
 	return __is_device(fd, "i915");
 }
 
+bool is_vc4_device(int fd)
+{
+	return __is_device(fd, "vc4");
+}
+
 static bool has_known_intel_chipset(int fd)
 {
 	struct drm_i915_getparam gp;
@@ -444,3 +449,8 @@ void igt_require_intel(int fd)
 {
 	igt_require(is_i915_device(fd) && has_known_intel_chipset(fd));
 }
+
+void igt_require_vc4(int fd)
+{
+	igt_require(is_vc4_device(fd));
+}
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 96ee517e..1d11bff9 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -78,8 +78,10 @@ int __drm_open_driver(int chipset);
 void gem_quiescent_gpu(int fd);
 
 void igt_require_intel(int fd);
+void igt_require_vc4(int fd);
 
 bool is_i915_device(int fd);
+bool is_vc4_device(int fd);
 
 /**
  * do_or_die:
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/9] lib: drmtest: Add helpers to check and require the VC4 driver

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> In order to add support for features specific to the VC4 driver, add
> helpers for checking and requiring the driver like it's done for the
> i915 driver.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/drmtest.c | 10 ++++++++++
>  lib/drmtest.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index d2aa1c19..53ea7598 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -105,6 +105,11 @@ bool is_i915_device(int fd)
>  	return __is_device(fd, "i915");
>  }
>  
> +bool is_vc4_device(int fd)
> +{
> +	return __is_device(fd, "vc4");
> +}
> +
>  static bool has_known_intel_chipset(int fd)
>  {
>  	struct drm_i915_getparam gp;
> @@ -444,3 +449,8 @@ void igt_require_intel(int fd)
>  {
>  	igt_require(is_i915_device(fd) && has_known_intel_chipset(fd));
>  }
> +
> +void igt_require_vc4(int fd)
> +{
> +	igt_require(is_vc4_device(fd));
> +}
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index 96ee517e..1d11bff9 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -78,8 +78,10 @@ int __drm_open_driver(int chipset);
>  void gem_quiescent_gpu(int fd);
>  
>  void igt_require_intel(int fd);
> +void igt_require_vc4(int fd);
>  
>  bool is_i915_device(int fd);
> +bool is_vc4_device(int fd);
>  
>  /**
>   * do_or_die:
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 4/9] lib/igt_fb: Add checks on i915 for i915-specific tiled buffer allocation

[Paul Kocialkowski]

The code path for allocating tiled buffers has a few i915-specific bits
without checks for the i915 driver. Add these missing checks.

For the map_bo function, initially define the return pointer to
MAP_FAILED and assert that it's not MAP_FAILED when no mapping function
was found, in order to provide an understandable error when it occurs.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_fb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index f2e6c89f..ccdf07d0 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -298,6 +298,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
 		(fb->plane_bpp[plane] / 8);
 
 	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
+	    is_i915_device(fb->fd) &&
 	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
 		uint32_t stride;
 
@@ -326,6 +327,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
 static uint64_t calc_plane_size(struct igt_fb *fb, int plane)
 {
 	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
+	    is_i915_device(fb->fd) &&
 	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
 		uint64_t min_size = (uint64_t) fb->strides[plane] *
 			fb->plane_height[plane];
@@ -1466,7 +1468,7 @@ static void destroy_cairo_surface__gtt(void *arg)
 
 static void *map_bo(int fd, struct igt_fb *fb)
 {
-	void *ptr;
+	void *ptr = MAP_FAILED;
 
 	if (is_i915_device(fd))
 		gem_set_domain(fd, fb->gem_handle,
@@ -1475,9 +1477,11 @@ static void *map_bo(int fd, struct igt_fb *fb)
 	if (fb->is_dumb)
 		ptr = kmstest_dumb_map_buffer(fd, fb->gem_handle, fb->size,
 					      PROT_READ | PROT_WRITE);
-	else
+	else if (is_i915_device(fd))
 		ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
 				    PROT_READ | PROT_WRITE);
+	else
+		igt_assert(ptr != MAP_FAILED);
 
 	return ptr;
 }
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/9] lib/igt_fb: Add checks on i915 for i915-specific tiled buffer allocation

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 669 bytes --]

On Thu, Dec 06, 2018 at 03:11:27PM +0100, Paul Kocialkowski wrote:
> The code path for allocating tiled buffers has a few i915-specific bits
> without checks for the i915 driver. Add these missing checks.
> 
> For the map_bo function, initially define the return pointer to
> MAP_FAILED and assert that it's not MAP_FAILED when no mapping function
> was found, in order to provide an understandable error when it occurs.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

You're doing two things in that patch, it should be two patches.

maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/9] lib/igt_fb: Add checks on i915 for i915-specific tiled buffer allocation

[Ville Syrjälä]

On Thu, Dec 06, 2018 at 03:11:27PM +0100, Paul Kocialkowski wrote:
> The code path for allocating tiled buffers has a few i915-specific bits
> without checks for the i915 driver. Add these missing checks.

The else branch also has a bit of i915 specifics in it:

void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
                          unsigned *width_ret, unsigned *height_ret)
{
        switch (tiling) {
        case LOCAL_DRM_FORMAT_MOD_NONE:
                *width_ret = 64;
                *height_ret = 1;
                break;

But I'm not sure if there is hardware that wouldn't be happy
with 64 byte stride alignment.

> 
> For the map_bo function, initially define the return pointer to
> MAP_FAILED and assert that it's not MAP_FAILED when no mapping function
> was found, in order to provide an understandable error when it occurs.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_fb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index f2e6c89f..ccdf07d0 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -298,6 +298,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
>  		(fb->plane_bpp[plane] / 8);
>  
>  	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
> +	    is_i915_device(fb->fd) &&
>  	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
>  		uint32_t stride;
>  
> @@ -326,6 +327,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
>  static uint64_t calc_plane_size(struct igt_fb *fb, int plane)
>  {
>  	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
> +	    is_i915_device(fb->fd) &&
>  	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
>  		uint64_t min_size = (uint64_t) fb->strides[plane] *
>  			fb->plane_height[plane];
> @@ -1466,7 +1468,7 @@ static void destroy_cairo_surface__gtt(void *arg)
>  
>  static void *map_bo(int fd, struct igt_fb *fb)
>  {
> -	void *ptr;
> +	void *ptr = MAP_FAILED;
>  
>  	if (is_i915_device(fd))
>  		gem_set_domain(fd, fb->gem_handle,
> @@ -1475,9 +1477,11 @@ static void *map_bo(int fd, struct igt_fb *fb)
>  	if (fb->is_dumb)
>  		ptr = kmstest_dumb_map_buffer(fd, fb->gem_handle, fb->size,
>  					      PROT_READ | PROT_WRITE);
> -	else
> +	else if (is_i915_device(fd))
>  		ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
>  				    PROT_READ | PROT_WRITE);
> +	else
> +		igt_assert(ptr != MAP_FAILED);

igt_assert(0)?

>  
>  	return ptr;
>  }
> -- 
> 2.19.2

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

Re: [igt-dev] [PATCH i-g-t 4/9] lib/igt_fb: Add checks on i915 for i915-specific tiled buffer allocation

[Paul Kocialkowski]

Hi,

On Mon, 2018-12-10 at 18:30 +0200, Ville Syrjälä wrote:
> On Thu, Dec 06, 2018 at 03:11:27PM +0100, Paul Kocialkowski wrote:
> > The code path for allocating tiled buffers has a few i915-specific bits
> > without checks for the i915 driver. Add these missing checks.
> 
> The else branch also has a bit of i915 specifics in it:
> 
> void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
>                           unsigned *width_ret, unsigned *height_ret)
> {
>         switch (tiling) {
>         case LOCAL_DRM_FORMAT_MOD_NONE:
>                 *width_ret = 64;
>                 *height_ret = 1;
>                 break;
> 
> But I'm not sure if there is hardware that wouldn't be happy
> with 64 byte stride alignment.

Right, that wasn't causing any problem so I kept it untouched.

We could totally change that to align to a lower value (I think we need
at least 4 bytes for stride alignment to make pixman happy with using
the buffer).

If you think that change is necessary, I could make it a follow-up
patch or integrate it in the next revision.

Cheers,

Paul

> > For the map_bo function, initially define the return pointer to
> > MAP_FAILED and assert that it's not MAP_FAILED when no mapping function
> > was found, in order to provide an understandable error when it occurs.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  lib/igt_fb.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index f2e6c89f..ccdf07d0 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -298,6 +298,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
> >  		(fb->plane_bpp[plane] / 8);
> >  
> >  	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
> > +	    is_i915_device(fb->fd) &&
> >  	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
> >  		uint32_t stride;
> >  
> > @@ -326,6 +327,7 @@ static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
> >  static uint64_t calc_plane_size(struct igt_fb *fb, int plane)
> >  {
> >  	if (fb->tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
> > +	    is_i915_device(fb->fd) &&
> >  	    intel_gen(intel_get_drm_devid(fb->fd)) <= 3) {
> >  		uint64_t min_size = (uint64_t) fb->strides[plane] *
> >  			fb->plane_height[plane];
> > @@ -1466,7 +1468,7 @@ static void destroy_cairo_surface__gtt(void *arg)
> >  
> >  static void *map_bo(int fd, struct igt_fb *fb)
> >  {
> > -	void *ptr;
> > +	void *ptr = MAP_FAILED;
> >  
> >  	if (is_i915_device(fd))
> >  		gem_set_domain(fd, fb->gem_handle,
> > @@ -1475,9 +1477,11 @@ static void *map_bo(int fd, struct igt_fb *fb)
> >  	if (fb->is_dumb)
> >  		ptr = kmstest_dumb_map_buffer(fd, fb->gem_handle, fb->size,
> >  					      PROT_READ | PROT_WRITE);
> > -	else
> > +	else if (is_i915_device(fd))
> >  		ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
> >  				    PROT_READ | PROT_WRITE);
> > +	else
> > +		igt_assert(ptr != MAP_FAILED);
> 
> igt_assert(0)?
> 
> >  
> >  	return ptr;
> >  }
> > -- 
> > 2.19.2
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 5/9] lib/igt_fb: Add support for allocating T-tiled VC4 buffers

[Paul Kocialkowski]

This introduces the required bits for allocating buffers with a T-tiled
disposition, that is specific to the VC4. It includes calculating the
top-tile width and creating a buffer object with the VC4-specific
helper. The tiling flag is set to the buffer object so that this can
be reused for GPU tests if needed later.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_fb.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index ccdf07d0..61250b0e 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -37,6 +37,7 @@
 #include "igt_fb.h"
 #include "igt_kms.h"
 #include "igt_matrix.h"
+#include "igt_vc4.h"
 #include "igt_x86.h"
 #include "ioctl_wrappers.h"
 #include "intel_batchbuffer.h"
@@ -179,6 +180,9 @@ static const struct format_desc_struct *lookup_drm_format(uint32_t drm_format)
 void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
 			  unsigned *width_ret, unsigned *height_ret)
 {
+	if (is_vc4_device(fd))
+		tiling = fourcc_mod_broadcom_mod(tiling);
+
 	switch (tiling) {
 	case LOCAL_DRM_FORMAT_MOD_NONE:
 		*width_ret = 64;
@@ -228,6 +232,17 @@ void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
 			igt_assert(false);
 		}
 		break;
+	case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
+		igt_require_vc4(fd);
+		switch (fb_bpp) {
+		case 32:
+			*width_ret = 32 * fb_bpp / 8;
+			*height_ret = 32;
+			break;
+		default:
+			igt_assert(false);
+		}
+		break;
 	default:
 		igt_assert(false);
 	}
@@ -568,6 +583,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
 
 		if (is_i915_device(fd)) {
 			return i915_create_gem_for_fb(fb);
+		} else if (is_vc4_device(fd)) {
+			fb->gem_handle = igt_vc4_create_bo(fd, fb->size);
+
+			if (fb->tiling == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
+				igt_vc4_set_tiling(fd, fb->gem_handle, fb->tiling);
+
+			return fb->gem_handle;
 		} else {
 			bool driver_has_gem_api = false;
 
@@ -1480,6 +1502,9 @@ static void *map_bo(int fd, struct igt_fb *fb)
 	else if (is_i915_device(fd))
 		ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
 				    PROT_READ | PROT_WRITE);
+	else if (is_vc4_device(fd))
+		ptr = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size,
+				      PROT_READ | PROT_WRITE);
 	else
 		igt_assert(ptr != MAP_FAILED);
 
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 6/9] lib/igt_vc4: Add helpers for converting linear to T-tiled XRGB buffers

[Paul Kocialkowski]

In order to integrate testing of T-tiled buffers, the easiest path is to
generate patterns (with the already-existing functions) in linear
buffers and convert them to T-tiled subsequently.

Add helpers to do that conversion, with a first helper that returns the
memory offset for a given position in a T-tiled buffer and a second
helper that uses it for converting between framebuffers.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_vc4.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_vc4.h |  4 +++
 2 files changed, 100 insertions(+)

diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
index 16dfe67a..0b5124f3 100644
--- a/lib/igt_vc4.c
+++ b/lib/igt_vc4.c
@@ -34,6 +34,7 @@
 #include "drmtest.h"
 #include "igt_aux.h"
 #include "igt_core.h"
+#include "igt_fb.h"
 #include "igt_vc4.h"
 #include "ioctl_wrappers.h"
 #include "intel_reg.h"
@@ -176,3 +177,98 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable)
 
 	return arg.retained;
 }
+
+unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src)
+{
+	unsigned int fb_id;
+	unsigned int i, j;
+	uint32_t *src_buf;
+	uint32_t *dst_buf;
+	size_t offset;
+
+	igt_assert(src->drm_format == DRM_FORMAT_XRGB8888);
+
+	fb_id = igt_create_fb(src->fd, src->width, src->height, src->drm_format,
+			      DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED, dst);
+	igt_assert(fb_id > 0);
+
+	src_buf = igt_fb_map_buffer(src->fd, src);
+	igt_assert(src_buf);
+
+	dst_buf = igt_fb_map_buffer(dst->fd, dst);
+	igt_assert(dst_buf);
+
+	for (i = 0; i < src->height; i++) {
+		for (j = 0; j < src->width; j++) {
+			offset = igt_vc4_t_tiled_offset(src->width, src->height,
+							src->drm_format, j, i) / 4;
+			*(dst_buf + offset) = *(src_buf +
+						src->strides[0] / 4 * i + j);
+		}
+	}
+
+	igt_fb_unmap_buffer(src, src_buf);
+	igt_fb_unmap_buffer(dst, dst_buf);
+
+	return fb_id;
+}
+
+size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
+			      size_t x, size_t y)
+{
+	size_t t1k_map_even[4] = { 0, 3, 1, 2 };
+	size_t t1k_map_odd[4] = { 2, 1, 3, 0 };
+	size_t offset = 0;
+	size_t t4k_w, t4k_x, t4k_y;
+	size_t t1k_x, t1k_y;
+	size_t t64b_x, t64b_y;
+	size_t pix_x, pix_y;
+	unsigned int index;
+
+	igt_assert(format == DRM_FORMAT_XRGB8888);
+
+	/* Width in number of 4K tiles. */
+	t4k_w = ALIGN(width, 32) / 32;
+
+	/* X and y coordinates in number of 4K tiles (32x32). */
+	t4k_x = x / 32;
+	t4k_y = y / 32;
+
+	/* Increase offset to the beginning of the corresponding 4K tile. */
+	offset += t4k_y * t4k_w * 32 * 32;
+
+	/* X and Y coordinates in number of 1K tiles (16x16) within the 4K tile. */
+	t1k_x = (x % 32) / 16;
+	t1k_y = (y % 32) / 16;
+
+	index = 2 * t1k_y + t1k_x;
+
+	if (t4k_y % 2) {
+		/* Odd rows start from the right. */
+		offset += (t4k_w - t4k_x - 1) * 32 * 32;
+		offset += t1k_map_odd[index] * 16 * 16;
+	} else {
+		/* Even rows start from the left. */
+		offset += t4k_x * 32 * 32;
+		offset += t1k_map_even[index] * 16 * 16;
+	}
+
+	/* X and Y coordinates in number of 64 byte tiles (4x4) within the 1K tile. */
+	t64b_x = (x % 16) / 4;
+	t64b_y = (y % 16) / 4;
+
+	/* Increase offset to the beginning of the corresponding 64 byte tile. */
+	offset += t64b_y * 4 * 4 * 4 + t64b_x * 4 * 4;
+
+	/* X and Y coordinates in number of pixels within the 64 byte tile. */
+	pix_x = x % 4;
+	pix_y = y % 4;
+
+	/* Increase offset to the correct pixel. */
+	offset += pix_y * 4 + pix_x;
+
+	/* Multiply by the number of bytes per pixel for the format. */
+	offset *= 4;
+
+	return offset;
+}
diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
index ebc8a388..ab44f771 100644
--- a/lib/igt_vc4.h
+++ b/lib/igt_vc4.h
@@ -33,4 +33,8 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable);
 void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
 uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
 
+unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src);
+size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
+			      size_t x, size_t y);
+
 #endif /* IGT_VC4_H */
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 6/9] lib/igt_vc4: Add helpers for converting linear to T-tiled XRGB buffers

[Lyude Paul]

One small nitpick

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> In order to integrate testing of T-tiled buffers, the easiest path is to
> generate patterns (with the already-existing functions) in linear
> buffers and convert them to T-tiled subsequently.
> 
> Add helpers to do that conversion, with a first helper that returns the
> memory offset for a given position in a T-tiled buffer and a second
> helper that uses it for converting between framebuffers.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_vc4.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_vc4.h |  4 +++
>  2 files changed, 100 insertions(+)
> 
> diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
> index 16dfe67a..0b5124f3 100644
> --- a/lib/igt_vc4.c
> +++ b/lib/igt_vc4.c
> @@ -34,6 +34,7 @@
>  #include "drmtest.h"
>  #include "igt_aux.h"
>  #include "igt_core.h"
> +#include "igt_fb.h"
>  #include "igt_vc4.h"
>  #include "ioctl_wrappers.h"
>  #include "intel_reg.h"
> @@ -176,3 +177,98 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool
> purgeable)
>  
>  	return arg.retained;
>  }
> +
> +unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb
> *src)
> +{
> +	unsigned int fb_id;
> +	unsigned int i, j;
> +	uint32_t *src_buf;
> +	uint32_t *dst_buf;
> +	size_t offset;
> +
> +	igt_assert(src->drm_format == DRM_FORMAT_XRGB8888);
> +
> +	fb_id = igt_create_fb(src->fd, src->width, src->height, src-
> >drm_format,
> +			      DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED, dst);
> +	igt_assert(fb_id > 0);
> +
> +	src_buf = igt_fb_map_buffer(src->fd, src);
> +	igt_assert(src_buf);
> +
> +	dst_buf = igt_fb_map_buffer(dst->fd, dst);
> +	igt_assert(dst_buf);
> +
> +	for (i = 0; i < src->height; i++) {
> +		for (j = 0; j < src->width; j++) {
> +			offset = igt_vc4_t_tiled_offset(src->width, src-
> >height,
> +							src->drm_format, j, i)
> / 4;
> +			*(dst_buf + offset) = *(src_buf +
> +						src->strides[0] / 4 * i + j);
> +		}
> +	}
> +
> +	igt_fb_unmap_buffer(src, src_buf);
> +	igt_fb_unmap_buffer(dst, dst_buf);
> +
> +	return fb_id;
> +}
> +
> +size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
> +			      size_t x, size_t y)
> +{
> +	size_t t1k_map_even[4] = { 0, 3, 1, 2 };
> +	size_t t1k_map_odd[4] = { 2, 1, 3, 0 };
No need to specify the array size here, t1k_map_even[] and t1k_map_odd[]
should be fine. Also, these look like they probably should be const.

> +	size_t offset = 0;
> +	size_t t4k_w, t4k_x, t4k_y;
> +	size_t t1k_x, t1k_y;
> +	size_t t64b_x, t64b_y;
> +	size_t pix_x, pix_y;
> +	unsigned int index;
> +
> +	igt_assert(format == DRM_FORMAT_XRGB8888);
> +
> +	/* Width in number of 4K tiles. */
> +	t4k_w = ALIGN(width, 32) / 32;
> +
> +	/* X and y coordinates in number of 4K tiles (32x32). */
> +	t4k_x = x / 32;
> +	t4k_y = y / 32;
> +
> +	/* Increase offset to the beginning of the corresponding 4K tile. */
> +	offset += t4k_y * t4k_w * 32 * 32;
> +
> +	/* X and Y coordinates in number of 1K tiles (16x16) within the 4K
> tile. */
> +	t1k_x = (x % 32) / 16;
> +	t1k_y = (y % 32) / 16;
> +
> +	index = 2 * t1k_y + t1k_x;
> +
> +	if (t4k_y % 2) {
> +		/* Odd rows start from the right. */
> +		offset += (t4k_w - t4k_x - 1) * 32 * 32;
> +		offset += t1k_map_odd[index] * 16 * 16;
> +	} else {
> +		/* Even rows start from the left. */
> +		offset += t4k_x * 32 * 32;
> +		offset += t1k_map_even[index] * 16 * 16;
> +	}
> +
> +	/* X and Y coordinates in number of 64 byte tiles (4x4) within the 1K
> tile. */
> +	t64b_x = (x % 16) / 4;
> +	t64b_y = (y % 16) / 4;
> +
> +	/* Increase offset to the beginning of the corresponding 64 byte tile.
> */
> +	offset += t64b_y * 4 * 4 * 4 + t64b_x * 4 * 4;
> +
> +	/* X and Y coordinates in number of pixels within the 64 byte tile. */
> +	pix_x = x % 4;
> +	pix_y = y % 4;
> +
> +	/* Increase offset to the correct pixel. */
> +	offset += pix_y * 4 + pix_x;
> +
> +	/* Multiply by the number of bytes per pixel for the format. */
> +	offset *= 4;
> +
> +	return offset;
> +}
> diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> index ebc8a388..ab44f771 100644
> --- a/lib/igt_vc4.h
> +++ b/lib/igt_vc4.h
> @@ -33,4 +33,8 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool
> purgeable);
>  void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
>  uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
>  
> +unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb
> *src);
> +size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
> +			      size_t x, size_t y);
> +
>  #endif /* IGT_VC4_H */
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 6/9] lib/igt_vc4: Add helpers for converting linear to T-tiled XRGB buffers

[Paul Kocialkowski]

Hi,

On Thu, 2018-12-06 at 16:51 -0500, Lyude Paul wrote:
> One small nitpick
> 
> On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> > In order to integrate testing of T-tiled buffers, the easiest path is to
> > generate patterns (with the already-existing functions) in linear
> > buffers and convert them to T-tiled subsequently.
> > 
> > Add helpers to do that conversion, with a first helper that returns the
> > memory offset for a given position in a T-tiled buffer and a second
> > helper that uses it for converting between framebuffers.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  lib/igt_vc4.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_vc4.h |  4 +++
> >  2 files changed, 100 insertions(+)
> > 
> > diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
> > index 16dfe67a..0b5124f3 100644
> > --- a/lib/igt_vc4.c
> > +++ b/lib/igt_vc4.c
> > @@ -34,6 +34,7 @@
> >  #include "drmtest.h"
> >  #include "igt_aux.h"
> >  #include "igt_core.h"
> > +#include "igt_fb.h"
> >  #include "igt_vc4.h"
> >  #include "ioctl_wrappers.h"
> >  #include "intel_reg.h"
> > @@ -176,3 +177,98 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool
> > purgeable)
> >  
> >  	return arg.retained;
> >  }
> > +
> > +unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb
> > *src)
> > +{
> > +	unsigned int fb_id;
> > +	unsigned int i, j;
> > +	uint32_t *src_buf;
> > +	uint32_t *dst_buf;
> > +	size_t offset;
> > +
> > +	igt_assert(src->drm_format == DRM_FORMAT_XRGB8888);
> > +
> > +	fb_id = igt_create_fb(src->fd, src->width, src->height, src-
> > > drm_format,
> > +			      DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED, dst);
> > +	igt_assert(fb_id > 0);
> > +
> > +	src_buf = igt_fb_map_buffer(src->fd, src);
> > +	igt_assert(src_buf);
> > +
> > +	dst_buf = igt_fb_map_buffer(dst->fd, dst);
> > +	igt_assert(dst_buf);
> > +
> > +	for (i = 0; i < src->height; i++) {
> > +		for (j = 0; j < src->width; j++) {
> > +			offset = igt_vc4_t_tiled_offset(src->width, src-
> > > height,
> > +							src->drm_format, j, i)
> > / 4;
> > +			*(dst_buf + offset) = *(src_buf +
> > +						src->strides[0] / 4 * i + j);
> > +		}
> > +	}
> > +
> > +	igt_fb_unmap_buffer(src, src_buf);
> > +	igt_fb_unmap_buffer(dst, dst_buf);
> > +
> > +	return fb_id;
> > +}
> > +
> > +size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
> > +			      size_t x, size_t y)
> > +{
> > +	size_t t1k_map_even[4] = { 0, 3, 1, 2 };
> > +	size_t t1k_map_odd[4] = { 2, 1, 3, 0 };
> No need to specify the array size here, t1k_map_even[] and t1k_map_odd[]
> should be fine. Also, these look like they probably should be const.

Definitely, I will make these changes in v2!

Cheers,

Paul

> > +	size_t offset = 0;
> > +	size_t t4k_w, t4k_x, t4k_y;
> > +	size_t t1k_x, t1k_y;
> > +	size_t t64b_x, t64b_y;
> > +	size_t pix_x, pix_y;
> > +	unsigned int index;
> > +
> > +	igt_assert(format == DRM_FORMAT_XRGB8888);
> > +
> > +	/* Width in number of 4K tiles. */
> > +	t4k_w = ALIGN(width, 32) / 32;
> > +
> > +	/* X and y coordinates in number of 4K tiles (32x32). */
> > +	t4k_x = x / 32;
> > +	t4k_y = y / 32;
> > +
> > +	/* Increase offset to the beginning of the corresponding 4K tile. */
> > +	offset += t4k_y * t4k_w * 32 * 32;
> > +
> > +	/* X and Y coordinates in number of 1K tiles (16x16) within the 4K
> > tile. */
> > +	t1k_x = (x % 32) / 16;
> > +	t1k_y = (y % 32) / 16;
> > +
> > +	index = 2 * t1k_y + t1k_x;
> > +
> > +	if (t4k_y % 2) {
> > +		/* Odd rows start from the right. */
> > +		offset += (t4k_w - t4k_x - 1) * 32 * 32;
> > +		offset += t1k_map_odd[index] * 16 * 16;
> > +	} else {
> > +		/* Even rows start from the left. */
> > +		offset += t4k_x * 32 * 32;
> > +		offset += t1k_map_even[index] * 16 * 16;
> > +	}
> > +
> > +	/* X and Y coordinates in number of 64 byte tiles (4x4) within the 1K
> > tile. */
> > +	t64b_x = (x % 16) / 4;
> > +	t64b_y = (y % 16) / 4;
> > +
> > +	/* Increase offset to the beginning of the corresponding 64 byte tile.
> > */
> > +	offset += t64b_y * 4 * 4 * 4 + t64b_x * 4 * 4;
> > +
> > +	/* X and Y coordinates in number of pixels within the 64 byte tile. */
> > +	pix_x = x % 4;
> > +	pix_y = y % 4;
> > +
> > +	/* Increase offset to the correct pixel. */
> > +	offset += pix_y * 4 + pix_x;
> > +
> > +	/* Multiply by the number of bytes per pixel for the format. */
> > +	offset *= 4;
> > +
> > +	return offset;
> > +}
> > diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> > index ebc8a388..ab44f771 100644
> > --- a/lib/igt_vc4.h
> > +++ b/lib/igt_vc4.h
> > @@ -33,4 +33,8 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool
> > purgeable);
> >  void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
> >  uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
> >  
> > +unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb
> > *src);
> > +size_t igt_vc4_t_tiled_offset(size_t width, size_t height, size_t format,
> > +			      size_t x, size_t y);
> > +
> >  #endif /* IGT_VC4_H */
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 7/9] lib/igt_fb: Add a stride-provisioned fashion of igt_fb_convert

[Paul Kocialkowski]

The current implementation of igt_fb_convert does not allow passing
the destination stride, which is something we want to change for tests.

Add a new fashion of this function that allocates the desintation buffer
with a given stride. Since the current function does the same thing with
an unspecified stride (set to zero, which will be filled later), make it
call our new fashion with the stride set to zero to avoid duplication.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_fb.c | 38 ++++++++++++++++++++++++++++++++------
 lib/igt_fb.h |  3 +++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 61250b0e..1548bf4e 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2253,14 +2253,15 @@ void igt_remove_fb(int fd, struct igt_fb *fb)
 }
 
 /**
- * igt_fb_convert:
+ * igt_fb_convert_with_stride:
  * @dst: pointer to the #igt_fb structure that will store the conversion result
  * @src: pointer to the #igt_fb structure that stores the frame we convert
  * @dst_fourcc: DRM format specifier to convert to
+ * @dst_stride: DRM format specifier to convert to
  *
  * This will convert a given @src content to the @dst_fourcc format,
  * storing the result in the @dst fb, allocating the @dst fb
- * underlying buffer.
+ * underlying buffer with a stride of @dst_stride stride.
  *
  * Once done with @dst, the caller will have to call igt_remove_fb()
  * on it to free the associated resources.
@@ -2268,15 +2269,18 @@ void igt_remove_fb(int fd, struct igt_fb *fb)
  * Returns:
  * The kms id of the created framebuffer.
  */
-unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
-			    uint32_t dst_fourcc)
+unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
+					uint32_t dst_fourcc,
+					unsigned int dst_stride)
 {
 	struct fb_convert cvt = { };
 	void *dst_ptr, *src_ptr;
 	int fb_id;
 
-	fb_id = igt_create_fb(src->fd, src->width, src->height,
-			      dst_fourcc, LOCAL_DRM_FORMAT_MOD_NONE, dst);
+	fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
+					   dst_fourcc,
+					   LOCAL_DRM_FORMAT_MOD_NONE,
+					   dst, 0, dst_stride);
 	igt_assert(fb_id > 0);
 
 	src_ptr = igt_fb_map_buffer(src->fd, src);
@@ -2297,6 +2301,28 @@ unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
 	return fb_id;
 }
 
+/**
+ * igt_fb_convert:
+ * @dst: pointer to the #igt_fb structure that will store the conversion result
+ * @src: pointer to the #igt_fb structure that stores the frame we convert
+ * @dst_fourcc: DRM format specifier to convert to
+ *
+ * This will convert a given @src content to the @dst_fourcc format,
+ * storing the result in the @dst fb, allocating the @dst fb
+ * underlying buffer.
+ *
+ * Once done with @dst, the caller will have to call igt_remove_fb()
+ * on it to free the associated resources.
+ *
+ * Returns:
+ * The kms id of the created framebuffer.
+ */
+unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
+			    uint32_t dst_fourcc)
+{
+	return igt_fb_convert_with_stride(dst, src, dst_fourcc, 0);
+}
+
 /**
  * igt_bpp_depth_to_drm_format:
  * @bpp: desired bits per pixel
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 9f027deb..cb21fdbc 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -130,6 +130,9 @@ unsigned int igt_create_image_fb(int drm_fd,  int width, int height,
 				 struct igt_fb *fb /* out */);
 unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
 				  uint32_t format, uint64_t tiling);
+unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
+					uint32_t dst_fourcc,
+					unsigned int stride);
 unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
 			    uint32_t dst_fourcc);
 void igt_remove_fb(int fd, struct igt_fb *fb);
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 7/9] lib/igt_fb: Add a stride-provisioned fashion of igt_fb_convert

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1444 bytes --]

65;5402;1c
On Thu, Dec 06, 2018 at 03:11:30PM +0100, Paul Kocialkowski wrote:
> The current implementation of igt_fb_convert does not allow passing
> the destination stride, which is something we want to change for tests.
> 
> Add a new fashion of this function that allocates the desintation buffer
> with a given stride. Since the current function does the same thing with
> an unspecified stride (set to zero, which will be filled later), make it
> call our new fashion with the stride set to zero to avoid duplication.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_fb.c | 38 ++++++++++++++++++++++++++++++++------
>  lib/igt_fb.h |  3 +++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 61250b0e..1548bf4e 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2253,14 +2253,15 @@ void igt_remove_fb(int fd, struct igt_fb *fb)
>  }
>  
>  /**
> - * igt_fb_convert:
> + * igt_fb_convert_with_stride:
>   * @dst: pointer to the #igt_fb structure that will store the conversion result
>   * @src: pointer to the #igt_fb structure that stores the frame we convert
>   * @dst_fourcc: DRM format specifier to convert to
> + * @dst_stride: DRM format specifier to convert to

I'm not sure that's what you meant :)

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 7/9] lib/igt_fb: Add a stride-provisioned fashion of igt_fb_convert

[Lyude Paul]

With the changes that Maxime mentioned:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> The current implementation of igt_fb_convert does not allow passing
> the destination stride, which is something we want to change for tests.
> 
> Add a new fashion of this function that allocates the desintation buffer
> with a given stride. Since the current function does the same thing with
> an unspecified stride (set to zero, which will be filled later), make it
> call our new fashion with the stride set to zero to avoid duplication.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_fb.c | 38 ++++++++++++++++++++++++++++++++------
>  lib/igt_fb.h |  3 +++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 61250b0e..1548bf4e 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2253,14 +2253,15 @@ void igt_remove_fb(int fd, struct igt_fb *fb)
>  }
>  
>  /**
> - * igt_fb_convert:
> + * igt_fb_convert_with_stride:
>   * @dst: pointer to the #igt_fb structure that will store the conversion
> result
>   * @src: pointer to the #igt_fb structure that stores the frame we convert
>   * @dst_fourcc: DRM format specifier to convert to
> + * @dst_stride: DRM format specifier to convert to
>   *
>   * This will convert a given @src content to the @dst_fourcc format,
>   * storing the result in the @dst fb, allocating the @dst fb
> - * underlying buffer.
> + * underlying buffer with a stride of @dst_stride stride.
>   *
>   * Once done with @dst, the caller will have to call igt_remove_fb()
>   * on it to free the associated resources.
> @@ -2268,15 +2269,18 @@ void igt_remove_fb(int fd, struct igt_fb *fb)
>   * Returns:
>   * The kms id of the created framebuffer.
>   */
> -unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
> -			    uint32_t dst_fourcc)
> +unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb
> *src,
> +					uint32_t dst_fourcc,
> +					unsigned int dst_stride)
>  {
>  	struct fb_convert cvt = { };
>  	void *dst_ptr, *src_ptr;
>  	int fb_id;
>  
> -	fb_id = igt_create_fb(src->fd, src->width, src->height,
> -			      dst_fourcc, LOCAL_DRM_FORMAT_MOD_NONE, dst);
> +	fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> +					   dst_fourcc,
> +					   LOCAL_DRM_FORMAT_MOD_NONE,
> +					   dst, 0, dst_stride);
>  	igt_assert(fb_id > 0);
>  
>  	src_ptr = igt_fb_map_buffer(src->fd, src);
> @@ -2297,6 +2301,28 @@ unsigned int igt_fb_convert(struct igt_fb *dst,
> struct igt_fb *src,
>  	return fb_id;
>  }
>  
> +/**
> + * igt_fb_convert:
> + * @dst: pointer to the #igt_fb structure that will store the conversion
> result
> + * @src: pointer to the #igt_fb structure that stores the frame we convert
> + * @dst_fourcc: DRM format specifier to convert to
> + *
> + * This will convert a given @src content to the @dst_fourcc format,
> + * storing the result in the @dst fb, allocating the @dst fb
> + * underlying buffer.
> + *
> + * Once done with @dst, the caller will have to call igt_remove_fb()
> + * on it to free the associated resources.
> + *
> + * Returns:
> + * The kms id of the created framebuffer.
> + */
> +unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
> +			    uint32_t dst_fourcc)
> +{
> +	return igt_fb_convert_with_stride(dst, src, dst_fourcc, 0);
> +}
> +
>  /**
>   * igt_bpp_depth_to_drm_format:
>   * @bpp: desired bits per pixel
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 9f027deb..cb21fdbc 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -130,6 +130,9 @@ unsigned int igt_create_image_fb(int drm_fd,  int width,
> int height,
>  				 struct igt_fb *fb /* out */);
>  unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
>  				  uint32_t format, uint64_t tiling);
> +unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb
> *src,
> +					uint32_t dst_fourcc,
> +					unsigned int stride);
>  unsigned int igt_fb_convert(struct igt_fb *dst, struct igt_fb *src,
>  			    uint32_t dst_fourcc);
>  void igt_remove_fb(int fd, struct igt_fb *fb);
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 8/9] lib/igt_fb: Add a helper to retreive the plane bpp for a given format

[Paul Kocialkowski]

The format bpp for a given plane is stored in the static format_desc
structure and is not accessible to tests, which is inconvenient to
get the minimum stride for a format when testing various strides.

Add a simple helper to expose the information.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_fb.c | 16 ++++++++++++++++
 lib/igt_fb.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 1548bf4e..b02f7f22 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2416,3 +2416,19 @@ bool igt_format_is_yuv(uint32_t drm_format)
 		return false;
 	}
 }
+
+/**
+ * igt_format_plane_bpp:
+ * @drm_format: drm fourcc
+ * @plane: format plane index
+ *
+ * This functions returns the number of bits per pixel for the given @plane
+ * index of the @drm_format.
+ */
+int igt_format_plane_bpp(uint32_t drm_format, int plane)
+{
+	const struct format_desc_struct *format =
+		lookup_drm_format(drm_format);
+
+	return format->plane_bpp[plane];
+}
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index cb21fdbc..a39c4d2f 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -175,6 +175,7 @@ uint32_t igt_drm_format_to_bpp(uint32_t drm_format);
 const char *igt_format_str(uint32_t drm_format);
 bool igt_fb_supported_format(uint32_t drm_format);
 bool igt_format_is_yuv(uint32_t drm_format);
+int igt_format_plane_bpp(uint32_t drm_format, int plane);
 
 #endif /* __IGT_FB_H__ */
 
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 8/9] lib/igt_fb: Add a helper to retreive the plane bpp for a given format

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> The format bpp for a given plane is stored in the static format_desc
> structure and is not accessible to tests, which is inconvenient to
> get the minimum stride for a format when testing various strides.
> 
> Add a simple helper to expose the information.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_fb.c | 16 ++++++++++++++++
>  lib/igt_fb.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 1548bf4e..b02f7f22 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2416,3 +2416,19 @@ bool igt_format_is_yuv(uint32_t drm_format)
>  		return false;
>  	}
>  }
> +
> +/**
> + * igt_format_plane_bpp:
> + * @drm_format: drm fourcc
> + * @plane: format plane index
> + *
> + * This functions returns the number of bits per pixel for the given @plane
> + * index of the @drm_format.
> + */
> +int igt_format_plane_bpp(uint32_t drm_format, int plane)
> +{
> +	const struct format_desc_struct *format =
> +		lookup_drm_format(drm_format);
> +
> +	return format->plane_bpp[plane];
> +}
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index cb21fdbc..a39c4d2f 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -175,6 +175,7 @@ uint32_t igt_drm_format_to_bpp(uint32_t drm_format);
>  const char *igt_format_str(uint32_t drm_format);
>  bool igt_fb_supported_format(uint32_t drm_format);
>  bool igt_format_is_yuv(uint32_t drm_format);
> +int igt_format_plane_bpp(uint32_t drm_format, int plane);
>  
>  #endif /* __IGT_FB_H__ */
>  
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 9/9] chamelium: Add a CRC-based display test for randomized planes

[Paul Kocialkowski]

This introduces a new test for the Chamelium, that sets up planes
with randomized properties such as the format, dimensions, position,
in-framebuffer offsets and stride. The Chamelium capture is checked
against the reference generated by cairo with a CRC.

This test also includes testing for some VC4-specific features, such as
T-tiled mode (in XR24 format), bandwidth limitation and underrun
(that require kernel-side patches that are currently under review).

Since this test does not share much with previous CRC-based display
tests (especially regarding KMS configuration), most of the code is
not shared with other tests.

This test can be derived with reproducible properties for regression
testing in the future. For now, it serves as a kind of fuzzing test.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 331 insertions(+)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 7d95a8bc..a4977309 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -26,6 +26,8 @@
 
 #include "config.h"
 #include "igt.h"
+#include "igt_sysfs.h"
+#include "igt_vc4.h"
 
 #include <fcntl.h>
 #include <string.h>
@@ -703,6 +705,330 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
 	drmModeFreeConnector(connector);
 }
 
+static uint32_t random_plane_formats[] = {
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_ARGB1555,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_XRGB8888,
+};
+
+static void test_display_planes_random(data_t *data,
+				       struct chamelium_port *port,
+				       enum chamelium_check check)
+{
+	igt_output_t *output;
+	igt_pipe_t *pipe;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary_plane;
+	struct igt_fb primary_fb;
+	struct igt_fb result_fb;
+	struct igt_fb *overlay_fbs;
+	igt_crc_t *crc;
+	igt_crc_t *expected_crc;
+	struct chamelium_fb_crc_async_data *fb_crc;
+	unsigned int overlay_planes_max = 0;
+	unsigned int overlay_planes_count;
+	unsigned int overlay_planes_index;
+	bool bandwidth_exceeded = false;
+	bool underrun_detected = false;
+	cairo_surface_t *result_surface;
+	cairo_surface_t *primary_surface;
+	cairo_surface_t *overlay_surface;
+	cairo_t *cr;
+	int captured_frame_count;
+	unsigned int i;
+	char *underrun;
+	int fb_id;
+	int debugfs;
+	int ret;
+
+	igt_assert(check == CHAMELIUM_CHECK_CRC);
+
+	srand(time(NULL));
+
+	reset_state(data, port);
+
+	/* Find the connector and pipe. */
+	output = prepare_output(data, port);
+	igt_assert(output);
+
+	if (is_vc4_device(data->drm_fd)) {
+		debugfs = igt_debugfs_dir(data->drm_fd);
+		igt_assert(debugfs >= 0);
+	}
+
+	pipe = &data->display.pipes[output->pending_pipe];
+
+	mode = igt_output_get_mode(output);
+	igt_assert(mode);
+
+	/* Get a framebuffer for the primary plane. */
+	primary_plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+	igt_assert(primary_plane);
+
+	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
+					 DRM_FORMAT_XRGB8888, 64, &primary_fb);
+	igt_assert(fb_id > 0);
+
+	igt_plane_set_fb(primary_plane, &primary_fb);
+
+	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
+
+	/* Get a framebuffer for the cairo composition result. */
+	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay,
+			      mode->vdisplay, DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE, &result_fb);
+	igt_assert(fb_id > 0);
+
+	/* Initialize cairo with the result surface as target. */
+	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
+
+	cr = cairo_create(result_surface);
+
+	/* Blend the primary surface. */
+	cairo_set_source_surface(cr, primary_surface, 0, 0);
+	cairo_surface_destroy(primary_surface);
+	cairo_paint(cr);
+
+	cairo_destroy(cr);
+
+	/* Count available overlay planes. */
+	for (i = 0; i < pipe->n_planes; i++) {
+		igt_plane_t *plane = &pipe->planes[i];
+
+		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
+			overlay_planes_max++;
+	}
+
+	/* Limit the number of planes to a reasonable scene. */
+	if (overlay_planes_max > 4)
+		overlay_planes_max = 4;
+
+	overlay_planes_count = (rand() % overlay_planes_max) + 1;
+	igt_debug("Using %d overlay planes\n", overlay_planes_count);
+
+	overlay_fbs = calloc(sizeof(struct igt_fb), overlay_planes_count);
+
+	for (i = 0, overlay_planes_index = 0;
+	     i < pipe->n_planes && overlay_planes_index < overlay_planes_count;
+	     i++) {
+		struct igt_fb *overlay_fb = &overlay_fbs[overlay_planes_index];
+		igt_plane_t *plane = &pipe->planes[i];
+		struct igt_fb pattern_fb;
+		uint32_t overlay_fb_w, overlay_fb_h;
+		int32_t overlay_crtc_x, overlay_crtc_y;
+		uint32_t overlay_crtc_w, overlay_crtc_h;
+		uint32_t overlay_src_x, overlay_src_y;
+		uint32_t overlay_src_w, overlay_src_h;
+		int overlay_surface_x, overlay_surface_y;
+		unsigned int index;
+		unsigned int stride_min;
+		unsigned int stride;
+		bool vc4_t_tiled;
+		uint32_t format;
+
+		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
+			continue;
+
+		/* Randomize width and height in the mode dimensions range. */
+		overlay_fb_w = (rand() % mode->hdisplay) + 1;
+		overlay_fb_h = (rand() % mode->vdisplay) + 1;
+
+		/*
+		 * Randomize source offset, but keep at least half of the
+		 * original size.
+		 */
+		overlay_src_x = rand() % (overlay_fb_w / 2);
+		overlay_src_y = rand() % (overlay_fb_h / 2);
+
+		/*
+		 * The on-crtc size does not include the source offset, so it
+		 * needs to be subtracted to avoid scaling.
+		 */
+		overlay_crtc_w = overlay_fb_w - overlay_src_x;
+		overlay_crtc_h = overlay_fb_h - overlay_src_y;
+
+		/* Same goes for the framebuffer source size. */
+		overlay_src_w = overlay_crtc_w;
+		overlay_src_h = overlay_crtc_h;
+
+		/*
+		 * Randomize the on-crtc position and allow the plane to go
+		 * off-display by up to half of its width and height.
+		 */
+		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w / 2;
+		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h / 2;
+
+		igt_debug("Plane %d: on-crtc size %dx%d\n",
+			  overlay_planes_index, overlay_crtc_w, overlay_crtc_h);
+		igt_debug("Plane %d: on-crtc position %dx%d\n",
+			  overlay_planes_index, overlay_crtc_x, overlay_crtc_y);
+		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
+			  overlay_planes_index, overlay_src_x, overlay_src_y);
+
+		/* Get a pattern framebuffer for the overlay plane. */
+		fb_id = chamelium_get_pattern_fb(data, overlay_fb_w,
+						 overlay_fb_h,
+						 DRM_FORMAT_XRGB8888,
+						 32, &pattern_fb);
+		igt_assert(fb_id > 0);
+
+		/* Randomize the use of tiled mode with a 1/4 probability. */
+		index = rand() % 4;
+
+		if (is_vc4_device(data->drm_fd) && index == 0) {
+			format = DRM_FORMAT_XRGB8888;
+			vc4_t_tiled = true;
+
+			igt_debug("Plane %d: VC4 T-tiled %s format\n",
+				  overlay_planes_index, igt_format_str(format));
+		} else {
+			/* Randomize the format to test. */
+			index = rand() % ARRAY_SIZE(random_plane_formats);
+			format = random_plane_formats[index];
+			vc4_t_tiled = false;
+
+			igt_debug("Plane %d: %s format\n", overlay_planes_index,
+				  igt_format_str(format));
+		}
+
+		/* Convert the pattern to the test format if needed. */
+		if (vc4_t_tiled) {
+			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
+							   &pattern_fb);
+			igt_assert(fb_id > 0);
+		} else {
+			stride_min = overlay_fb_w *
+				     igt_format_plane_bpp(format, 0) / 8;
+
+			/* Randomize the stride with at most twice the minimum. */
+			stride = (rand() % stride_min) + stride_min;
+
+			/* Pixman requires the stride to be aligned to 32-byte words. */
+			stride = ALIGN(stride, sizeof(uint32_t));
+
+			igt_debug("Plane %d: using stride %d\n", overlay_planes_index, stride);
+
+			fb_id = igt_fb_convert_with_stride(overlay_fb,
+							   &pattern_fb,
+							   format, stride);
+			igt_assert(fb_id);
+		}
+
+		igt_plane_set_fb(plane, overlay_fb);
+
+		overlay_surface = igt_get_cairo_surface(data->drm_fd,
+							&pattern_fb);
+
+		/* Blend the overlay surface. */
+		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
+		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
+
+		cr = cairo_create(result_surface);
+
+		cairo_set_source_surface(cr, overlay_surface, overlay_surface_x,
+					 overlay_surface_y);
+		cairo_surface_destroy(overlay_surface);
+
+		/* Clip the surface to a rectangle. */
+		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
+				overlay_crtc_w, overlay_crtc_h);
+		cairo_clip(cr);
+
+		cairo_paint(cr);
+
+		cairo_destroy(cr);
+
+		/* Configure the plane with framebuffer source coordinates. */
+		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
+		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
+
+		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
+				    overlay_src_y);
+		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
+				overlay_src_h);
+
+		igt_remove_fb(data->drm_fd, &pattern_fb);
+
+		overlay_planes_index++;
+	}
+
+	cairo_surface_destroy(result_surface);
+
+	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
+							&result_fb);
+
+	if (is_vc4_device(data->drm_fd)) {
+		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
+		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
+
+		igt_debug("Bandwidth limitation exeeded: %s\n",
+			  bandwidth_exceeded ? "Yes" : "No");
+
+		igt_assert(!bandwidth_exceeded);
+	}
+
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1);
+	crc = chamelium_read_captured_crcs(data->chamelium,
+					   &captured_frame_count);
+
+	igt_assert(captured_frame_count == 1);
+
+	if (is_vc4_device(data->drm_fd)) {
+		underrun = igt_sysfs_get(debugfs, "underrun");
+		igt_assert(underrun);
+
+		underrun_detected = (underrun[0] == 'Y');
+		free(underrun);
+
+		igt_debug("Underrun detected: %s\n",
+			  underrun_detected ? "Yes" : "No");
+
+		igt_assert(!underrun_detected);
+
+		close(debugfs);
+	}
+
+	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
+
+	chamelium_assert_crc_eq_or_dump(data->chamelium,
+					expected_crc, crc,
+					&result_fb, i);
+
+	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
+	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
+
+	free(expected_crc);
+	free(crc);
+
+	for (i = 0, overlay_planes_index = 0;
+	     i < pipe->n_planes && overlay_planes_index < overlay_planes_count;
+	     i++) {
+		igt_plane_t *plane = &pipe->planes[i];
+		struct igt_fb *overlay_fb = &overlay_fbs[overlay_planes_index];
+
+		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
+			continue;
+
+		igt_remove_fb(data->drm_fd, overlay_fb);
+
+		overlay_planes_index++;
+	}
+
+	free(overlay_fbs);
+
+	igt_remove_fb(data->drm_fd, &primary_fb);
+	igt_remove_fb(data->drm_fd, &result_fb);
+}
+
 static void
 test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
 {
@@ -981,6 +1307,11 @@ igt_main
 			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
 					      CHAMELIUM_CHECK_ANALOG, 1);
 
+		connector_subtest("hdmi-crc-planes-random", HDMIA)
+			test_display_planes_random(&data, port,
+						   CHAMELIUM_CHECK_CRC);
+
+
 		connector_subtest("hdmi-frame-dump", HDMIA)
 			test_display_frame_dump(&data, port);
 	}
-- 
2.19.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 9/9] chamelium: Add a CRC-based display test for randomized planes

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1426 bytes --]

Hi,

On Thu, Dec 06, 2018 at 03:11:32PM +0100, Paul Kocialkowski wrote:
> This introduces a new test for the Chamelium, that sets up planes
> with randomized properties such as the format, dimensions, position,
> in-framebuffer offsets and stride. The Chamelium capture is checked
> against the reference generated by cairo with a CRC.
> 
> This test also includes testing for some VC4-specific features, such as
> T-tiled mode (in XR24 format), bandwidth limitation and underrun
> (that require kernel-side patches that are currently under review).

it's not really clear to me why both are part of the same test?  You
don't really need to test the HVS bandwidth limitation with the
patterns, and you don't really need a pattern and a CRC to test the
HVS bandwith?

> Since this test does not share much with previous CRC-based display
> tests (especially regarding KMS configuration), most of the code is
> not shared with other tests.
> 
> This test can be derived with reproducible properties for regression
> testing in the future. For now, it serves as a kind of fuzzing test.

That function is also pretty long. While sticking to function that are
under 50-80 lines is not always easy, this one is over 300 lines long
and would definitely benefit being split into many, smaller,
functions.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 9/9] chamelium: Add a CRC-based display test for randomized planes

[Lyude Paul]

I agree with Maxime, this definitely should be split up into some more
functions. Some other comments below below

On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> This introduces a new test for the Chamelium, that sets up planes
> with randomized properties such as the format, dimensions, position,
> in-framebuffer offsets and stride. The Chamelium capture is checked
> against the reference generated by cairo with a CRC.
>
> This test also includes testing for some VC4-specific features, such as
> T-tiled mode (in XR24 format), bandwidth limitation and underrun
> (that require kernel-side patches that are currently under review).
>
> Since this test does not share much with previous CRC-based display
> tests (especially regarding KMS configuration), most of the code is
> not shared with other tests.
>
> This test can be derived with reproducible properties for regression
> testing in the future. For now, it serves as a kind of fuzzing test.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 331 insertions(+)
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 7d95a8bc..a4977309 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -26,6 +26,8 @@
>
>  #include "config.h"
>  #include "igt.h"
> +#include "igt_sysfs.h"
> +#include "igt_vc4.h"
>
>  #include <fcntl.h>
>  #include <string.h>
> @@ -703,6 +705,330 @@ test_display_frame_dump(data_t *data, struct
> chamelium_port *port)
>  	drmModeFreeConnector(connector);
>  }
>
> +static uint32_t random_plane_formats[] = {
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static void test_display_planes_random(data_t *data,
> +				       struct chamelium_port *port,
> +				       enum chamelium_check check)
> +{
> +	igt_output_t *output;
> +	igt_pipe_t *pipe;
> +	drmModeModeInfo *mode;
> +	igt_plane_t *primary_plane;
> +	struct igt_fb primary_fb;
> +	struct igt_fb result_fb;
> +	struct igt_fb *overlay_fbs;
> +	igt_crc_t *crc;
> +	igt_crc_t *expected_crc;
> +	struct chamelium_fb_crc_async_data *fb_crc;
> +	unsigned int overlay_planes_max = 0;
> +	unsigned int overlay_planes_count;
> +	unsigned int overlay_planes_index;
> +	bool bandwidth_exceeded = false;
> +	bool underrun_detected = false;
> +	cairo_surface_t *result_surface;
> +	cairo_surface_t *primary_surface;
> +	cairo_surface_t *overlay_surface;
> +	cairo_t *cr;
> +	int captured_frame_count;
> +	unsigned int i;
> +	char *underrun;
> +	int fb_id;
> +	int debugfs;
> +	int ret;
> +
> +	igt_assert(check == CHAMELIUM_CHECK_CRC);
> +
> +	srand(time(NULL));
> +
> +	reset_state(data, port);
> +
> +	/* Find the connector and pipe. */
> +	output = prepare_output(data, port);
> +	igt_assert(output);
Do we actually need this? We're not running igt_assert on the prepare_output()
function for any of the other tests.
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		debugfs = igt_debugfs_dir(data->drm_fd);
> +		igt_assert(debugfs >= 0);
> +	}
> +
> +	pipe = &data->display.pipes[output->pending_pipe];
> +
> +	mode = igt_output_get_mode(output);
> +	igt_assert(mode);
This assert doesn't look like it's needed either

> +
> +	/* Get a framebuffer for the primary plane. */
> +	primary_plane = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> +	igt_assert(primary_plane);
> +
> +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> +					 DRM_FORMAT_XRGB8888, 64,
> &primary_fb);
> +	igt_assert(fb_id > 0);
> +
> +	igt_plane_set_fb(primary_plane, &primary_fb);
> +
> +	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
> +
> +	/* Get a framebuffer for the cairo composition result. */
> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay,
> +			      mode->vdisplay, DRM_FORMAT_XRGB8888,
> +			      LOCAL_DRM_FORMAT_MOD_NONE, &result_fb);
> +	igt_assert(fb_id > 0);
> +
> +	/* Initialize cairo with the result surface as target. */
> +	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
> +
> +	cr = cairo_create(result_surface);
> +
> +	/* Blend the primary surface. */
> +	cairo_set_source_surface(cr, primary_surface, 0, 0);
> +	cairo_surface_destroy(primary_surface);
> +	cairo_paint(cr);
> +
> +	cairo_destroy(cr);
> +
> +	/* Count available overlay planes. */
> +	for (i = 0; i < pipe->n_planes; i++) {
> +		igt_plane_t *plane = &pipe->planes[i];
> +
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			overlay_planes_max++;
> +	}
Why not add a helper for this in a seperate patch? This seems quite
useful

> +
> +	/* Limit the number of planes to a reasonable scene. */
> +	if (overlay_planes_max > 4)
> +		overlay_planes_max = 4;
> +
> +	overlay_planes_count = (rand() % overlay_planes_max) + 1;
> +	igt_debug("Using %d overlay planes\n", overlay_planes_count);
> +
> +	overlay_fbs = calloc(sizeof(struct igt_fb), overlay_planes_count);
igt_assert(overlay_fbs)
> +
> +	for (i = 0, overlay_planes_index = 0;
> +	     i < pipe->n_planes && overlay_planes_index <
> overlay_planes_count;
> +	     i++) {
> +		struct igt_fb *overlay_fb =
> &overlay_fbs[overlay_planes_index];
> +		igt_plane_t *plane = &pipe->planes[i];
> +		struct igt_fb pattern_fb;
> +		uint32_t overlay_fb_w, overlay_fb_h;
> +		int32_t overlay_crtc_x, overlay_crtc_y;
> +		uint32_t overlay_crtc_w, overlay_crtc_h;
> +		uint32_t overlay_src_x, overlay_src_y;
> +		uint32_t overlay_src_w, overlay_src_h;
> +		int overlay_surface_x, overlay_surface_y;
> +		unsigned int index;
> +		unsigned int stride_min;
> +		unsigned int stride;
> +		bool vc4_t_tiled;
> +		uint32_t format;
> +
> +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> +			continue;
We could also add a helper for looping through planes of specific types,
since we seem to do that multiple times here and will probably do it in
future tests at some poit

> +
> +		/* Randomize width and height in the mode dimensions range. */
> +		overlay_fb_w = (rand() % mode->hdisplay) + 1;
> +		overlay_fb_h = (rand() % mode->vdisplay) + 1;
> +
> +		/*
> +		 * Randomize source offset, but keep at least half of the
> +		 * original size.
> +		 */
> +		overlay_src_x = rand() % (overlay_fb_w / 2);
> +		overlay_src_y = rand() % (overlay_fb_h / 2);
> +
> +		/*
> +		 * The on-crtc size does not include the source offset, so it
> +		 * needs to be subtracted to avoid scaling.
> +		 */
> +		overlay_crtc_w = overlay_fb_w - overlay_src_x;
> +		overlay_crtc_h = overlay_fb_h - overlay_src_y;
> +
> +		/* Same goes for the framebuffer source size. */
> +		overlay_src_w = overlay_crtc_w;
> +		overlay_src_h = overlay_crtc_h;
> +
> +		/*
> +		 * Randomize the on-crtc position and allow the plane to go
> +		 * off-display by up to half of its width and height.
> +		 */
> +		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w /
> 2;
> +		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h /
> 2;
> +
> +		igt_debug("Plane %d: on-crtc size %dx%d\n",
> +			  overlay_planes_index, overlay_crtc_w,
> overlay_crtc_h);
> +		igt_debug("Plane %d: on-crtc position %dx%d\n",
> +			  overlay_planes_index, overlay_crtc_x,
> overlay_crtc_y);
> +		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
> +			  overlay_planes_index, overlay_src_x, overlay_src_y);
> +
> +		/* Get a pattern framebuffer for the overlay plane. */
> +		fb_id = chamelium_get_pattern_fb(data, overlay_fb_w,
> +						 overlay_fb_h,
> +						 DRM_FORMAT_XRGB8888,
> +						 32, &pattern_fb);
> +		igt_assert(fb_id > 0);
> +
> +		/* Randomize the use of tiled mode with a 1/4 probability. */
> +		index = rand() % 4;
> +
> +		if (is_vc4_device(data->drm_fd) && index == 0) {
> +			format = DRM_FORMAT_XRGB8888;
> +			vc4_t_tiled = true;
> +
> +			igt_debug("Plane %d: VC4 T-tiled %s format\n",
> +				  overlay_planes_index,
> igt_format_str(format));
> +		} else {
> +			/* Randomize the format to test. */
> +			index = rand() % ARRAY_SIZE(random_plane_formats);
> +			format = random_plane_formats[index];
> +			vc4_t_tiled = false;
> +
> +			igt_debug("Plane %d: %s format\n",
> overlay_planes_index,
> +				  igt_format_str(format));
> +		}
> +
> +		/* Convert the pattern to the test format if needed. */
> +		if (vc4_t_tiled) {
> +			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
> +							   &pattern_fb);
> +			igt_assert(fb_id > 0);
> +		} else {
> +			stride_min = overlay_fb_w *
> +				     igt_format_plane_bpp(format, 0) / 8;
> +
> +			/* Randomize the stride with at most twice the
> minimum. */
> +			stride = (rand() % stride_min) + stride_min;
> +
> +			/* Pixman requires the stride to be aligned to 32-byte
> words. */
> +			stride = ALIGN(stride, sizeof(uint32_t));
> +
> +			igt_debug("Plane %d: using stride %d\n",
> overlay_planes_index, stride);
> +
> +			fb_id = igt_fb_convert_with_stride(overlay_fb,
> +							   &pattern_fb,
> +							   format, stride);
> +			igt_assert(fb_id);
> +		}
> +
> +		igt_plane_set_fb(plane, overlay_fb);
> +
> +		overlay_surface = igt_get_cairo_surface(data->drm_fd,
> +							&pattern_fb);
> +
> +		/* Blend the overlay surface. */
> +		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
> +		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
> +
> +		cr = cairo_create(result_surface);
> +
> +		cairo_set_source_surface(cr, overlay_surface,
> overlay_surface_x,
> +					 overlay_surface_y);
> +		cairo_surface_destroy(overlay_surface);
> +
> +		/* Clip the surface to a rectangle. */
> +		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
> +				overlay_crtc_w, overlay_crtc_h);
> +		cairo_clip(cr);
> +
> +		cairo_paint(cr);
> +
> +		cairo_destroy(cr);
> +
> +		/* Configure the plane with framebuffer source coordinates. */
> +		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
> +		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
> +
> +		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
> +				    overlay_src_y);
> +		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
> +				overlay_src_h);
> +
> +		igt_remove_fb(data->drm_fd, &pattern_fb);
> +
> +		overlay_planes_index++;
> +	}
> +
> +	cairo_surface_destroy(result_surface);
> +
> +	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> +							&result_fb);
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> +		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
> +
> +		igt_debug("Bandwidth limitation exeeded: %s\n",
> +			  bandwidth_exceeded ? "Yes" : "No");
> +
> +		igt_assert(!bandwidth_exceeded);
> +	}
Why does this need to be handled separately for VC4 devices? We already
print the return code from a failed atomic commit into the debugging
output, and I'd assume anyone looking at the output from the failure of
one of these tests probably knows what that means. Additionally, if we
wanted to print debugging messages for bandwidth limitations being
exceeded for atomic commits on vc4, seems like it would probably be
better to just add that code to igt_display_try_commit2()

> +
> +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +
> +	chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1);
> +	crc = chamelium_read_captured_crcs(data->chamelium,
> +					   &captured_frame_count);
> +
> +	igt_assert(captured_frame_count == 1);
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		underrun = igt_sysfs_get(debugfs, "underrun");
> +		igt_assert(underrun);
> +
> +		underrun_detected = (underrun[0] == 'Y');
> +		free(underrun);
> +
> +		igt_debug("Underrun detected: %s\n",
> +			  underrun_detected ? "Yes" : "No");
> +
> +		igt_assert(!underrun_detected);
> +
> +		close(debugfs);
> +	}
This will definitely be used again in the future, this should also go
into a helper for vc4.

> +
> +	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
> +
> +	chamelium_assert_crc_eq_or_dump(data->chamelium,
> +					expected_crc, crc,
> +					&result_fb, i);
> +
> +	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
> +	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
Why not just add this debugging output to
chamelium_assert_crc_eq_or_dump()?

> +
> +	free(expected_crc);
> +	free(crc);
> +
> +	for (i = 0, overlay_planes_index = 0;
> +	     i < pipe->n_planes && overlay_planes_index <
> overlay_planes_count;
> +	     i++) {
> +		igt_plane_t *plane = &pipe->planes[i];
> +		struct igt_fb *overlay_fb =
> &overlay_fbs[overlay_planes_index];
> +
> +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> +			continue;
> +
> +		igt_remove_fb(data->drm_fd, overlay_fb);
> +
> +		overlay_planes_index++;
> +	}
> +
> +	free(overlay_fbs);
> +
> +	igt_remove_fb(data->drm_fd, &primary_fb);
> +	igt_remove_fb(data->drm_fd, &result_fb);
> +}
> +
>  static void
>  test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
>  {
> @@ -981,6 +1307,11 @@ igt_main
>  			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
>  					      CHAMELIUM_CHECK_ANALOG, 1);
>
> +		connector_subtest("hdmi-crc-planes-random", HDMIA)
> +			test_display_planes_random(&data, port,
> +						   CHAMELIUM_CHECK_CRC);
> +
> +
>  		connector_subtest("hdmi-frame-dump", HDMIA)
>  			test_display_frame_dump(&data, port);
>  	}
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 9/9] chamelium: Add a CRC-based display test for randomized planes

[Paul Kocialkowski]

Hi,

Thanks for the review!

On Thu, 2018-12-06 at 17:52 -0500, Lyude Paul wrote:
> I agree with Maxime, this definitely should be split up into some more
> functions. Some other comments below below
> 
> On Thu, 2018-12-06 at 15:11 +0100, Paul Kocialkowski wrote:
> > This introduces a new test for the Chamelium, that sets up planes
> > with randomized properties such as the format, dimensions, position,
> > in-framebuffer offsets and stride. The Chamelium capture is checked
> > against the reference generated by cairo with a CRC.
> > 
> > This test also includes testing for some VC4-specific features, such as
> > T-tiled mode (in XR24 format), bandwidth limitation and underrun
> > (that require kernel-side patches that are currently under review).
> > 
> > Since this test does not share much with previous CRC-based display
> > tests (especially regarding KMS configuration), most of the code is
> > not shared with other tests.
> > 
> > This test can be derived with reproducible properties for regression
> > testing in the future. For now, it serves as a kind of fuzzing test.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 331 insertions(+)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 7d95a8bc..a4977309 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -26,6 +26,8 @@
> > 
> >  #include "config.h"
> >  #include "igt.h"
> > +#include "igt_sysfs.h"
> > +#include "igt_vc4.h"
> > 
> >  #include <fcntl.h>
> >  #include <string.h>
> > @@ -703,6 +705,330 @@ test_display_frame_dump(data_t *data, struct
> > chamelium_port *port)
> >  	drmModeFreeConnector(connector);
> >  }
> > 
> > +static uint32_t random_plane_formats[] = {
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_ARGB1555,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_BGR565,
> > +	DRM_FORMAT_RGB888,
> > +	DRM_FORMAT_BGR888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_XRGB8888,
> > +};
> > +
> > +static void test_display_planes_random(data_t *data,
> > +				       struct chamelium_port *port,
> > +				       enum chamelium_check check)
> > +{
> > +	igt_output_t *output;
> > +	igt_pipe_t *pipe;
> > +	drmModeModeInfo *mode;
> > +	igt_plane_t *primary_plane;
> > +	struct igt_fb primary_fb;
> > +	struct igt_fb result_fb;
> > +	struct igt_fb *overlay_fbs;
> > +	igt_crc_t *crc;
> > +	igt_crc_t *expected_crc;
> > +	struct chamelium_fb_crc_async_data *fb_crc;
> > +	unsigned int overlay_planes_max = 0;
> > +	unsigned int overlay_planes_count;
> > +	unsigned int overlay_planes_index;
> > +	bool bandwidth_exceeded = false;
> > +	bool underrun_detected = false;
> > +	cairo_surface_t *result_surface;
> > +	cairo_surface_t *primary_surface;
> > +	cairo_surface_t *overlay_surface;
> > +	cairo_t *cr;
> > +	int captured_frame_count;
> > +	unsigned int i;
> > +	char *underrun;
> > +	int fb_id;
> > +	int debugfs;
> > +	int ret;
> > +
> > +	igt_assert(check == CHAMELIUM_CHECK_CRC);
> > +
> > +	srand(time(NULL));
> > +
> > +	reset_state(data, port);
> > +
> > +	/* Find the connector and pipe. */
> > +	output = prepare_output(data, port);
> > +	igt_assert(output);
> Do we actually need this? We're not running igt_assert on the prepare_output()
> function for any of the other tests.

This can indeed be dropped. Looking at prepare_output, a few things
would fail before the returns NULL anyway.

> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		debugfs = igt_debugfs_dir(data->drm_fd);
> > +		igt_assert(debugfs >= 0);
> > +	}
> > +
> > +	pipe = &data->display.pipes[output->pending_pipe];
> > +
> > +	mode = igt_output_get_mode(output);
> > +	igt_assert(mode);
> This assert doesn't look like it's needed either

That function can never return NULL anyway, so you're totally right :)

> > +
> > +	/* Get a framebuffer for the primary plane. */
> > +	primary_plane = igt_output_get_plane_type(output,
> > DRM_PLANE_TYPE_PRIMARY);
> > +	igt_assert(primary_plane);
> > +
> > +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> > +					 DRM_FORMAT_XRGB8888, 64,
> > &primary_fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	igt_plane_set_fb(primary_plane, &primary_fb);
> > +
> > +	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
> > +
> > +	/* Get a framebuffer for the cairo composition result. */
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay,
> > +			      mode->vdisplay, DRM_FORMAT_XRGB8888,
> > +			      LOCAL_DRM_FORMAT_MOD_NONE, &result_fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	/* Initialize cairo with the result surface as target. */
> > +	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
> > +
> > +	cr = cairo_create(result_surface);
> > +
> > +	/* Blend the primary surface. */
> > +	cairo_set_source_surface(cr, primary_surface, 0, 0);
> > +	cairo_surface_destroy(primary_surface);
> > +	cairo_paint(cr);
> > +
> > +	cairo_destroy(cr);
> > +
> > +	/* Count available overlay planes. */
> > +	for (i = 0; i < pipe->n_planes; i++) {
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> > +			overlay_planes_max++;
> > +	}
> Why not add a helper for this in a seperate patch? This seems quite
> useful

Yes, defintiely!

> > +
> > +	/* Limit the number of planes to a reasonable scene. */
> > +	if (overlay_planes_max > 4)
> > +		overlay_planes_max = 4;
> > +
> > +	overlay_planes_count = (rand() % overlay_planes_max) + 1;
> > +	igt_debug("Using %d overlay planes\n", overlay_planes_count);
> > +
> > +	overlay_fbs = calloc(sizeof(struct igt_fb), overlay_planes_count);
> igt_assert(overlay_fbs)
> > +
> > +	for (i = 0, overlay_planes_index = 0;
> > +	     i < pipe->n_planes && overlay_planes_index <
> > overlay_planes_count;
> > +	     i++) {
> > +		struct igt_fb *overlay_fb =
> > &overlay_fbs[overlay_planes_index];
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +		struct igt_fb pattern_fb;
> > +		uint32_t overlay_fb_w, overlay_fb_h;
> > +		int32_t overlay_crtc_x, overlay_crtc_y;
> > +		uint32_t overlay_crtc_w, overlay_crtc_h;
> > +		uint32_t overlay_src_x, overlay_src_y;
> > +		uint32_t overlay_src_w, overlay_src_h;
> > +		int overlay_surface_x, overlay_surface_y;
> > +		unsigned int index;
> > +		unsigned int stride_min;
> > +		unsigned int stride;
> > +		bool vc4_t_tiled;
> > +		uint32_t format;
> > +
> > +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> > +			continue;
> We could also add a helper for looping through planes of specific types,
> since we seem to do that multiple times here and will probably do it in
> future tests at some poit

That would definitely make sense, I'll craft something in that
direction in the next revision.

> > +
> > +		/* Randomize width and height in the mode dimensions range. */
> > +		overlay_fb_w = (rand() % mode->hdisplay) + 1;
> > +		overlay_fb_h = (rand() % mode->vdisplay) + 1;
> > +
> > +		/*
> > +		 * Randomize source offset, but keep at least half of the
> > +		 * original size.
> > +		 */
> > +		overlay_src_x = rand() % (overlay_fb_w / 2);
> > +		overlay_src_y = rand() % (overlay_fb_h / 2);
> > +
> > +		/*
> > +		 * The on-crtc size does not include the source offset, so it
> > +		 * needs to be subtracted to avoid scaling.
> > +		 */
> > +		overlay_crtc_w = overlay_fb_w - overlay_src_x;
> > +		overlay_crtc_h = overlay_fb_h - overlay_src_y;
> > +
> > +		/* Same goes for the framebuffer source size. */
> > +		overlay_src_w = overlay_crtc_w;
> > +		overlay_src_h = overlay_crtc_h;
> > +
> > +		/*
> > +		 * Randomize the on-crtc position and allow the plane to go
> > +		 * off-display by up to half of its width and height.
> > +		 */
> > +		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w /
> > 2;
> > +		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h /
> > 2;
> > +
> > +		igt_debug("Plane %d: on-crtc size %dx%d\n",
> > +			  overlay_planes_index, overlay_crtc_w,
> > overlay_crtc_h);
> > +		igt_debug("Plane %d: on-crtc position %dx%d\n",
> > +			  overlay_planes_index, overlay_crtc_x,
> > overlay_crtc_y);
> > +		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
> > +			  overlay_planes_index, overlay_src_x, overlay_src_y);
> > +
> > +		/* Get a pattern framebuffer for the overlay plane. */
> > +		fb_id = chamelium_get_pattern_fb(data, overlay_fb_w,
> > +						 overlay_fb_h,
> > +						 DRM_FORMAT_XRGB8888,
> > +						 32, &pattern_fb);
> > +		igt_assert(fb_id > 0);
> > +
> > +		/* Randomize the use of tiled mode with a 1/4 probability. */
> > +		index = rand() % 4;
> > +
> > +		if (is_vc4_device(data->drm_fd) && index == 0) {
> > +			format = DRM_FORMAT_XRGB8888;
> > +			vc4_t_tiled = true;
> > +
> > +			igt_debug("Plane %d: VC4 T-tiled %s format\n",
> > +				  overlay_planes_index,
> > igt_format_str(format));
> > +		} else {
> > +			/* Randomize the format to test. */
> > +			index = rand() % ARRAY_SIZE(random_plane_formats);
> > +			format = random_plane_formats[index];
> > +			vc4_t_tiled = false;
> > +
> > +			igt_debug("Plane %d: %s format\n",
> > overlay_planes_index,
> > +				  igt_format_str(format));
> > +		}
> > +
> > +		/* Convert the pattern to the test format if needed. */
> > +		if (vc4_t_tiled) {
> > +			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
> > +							   &pattern_fb);
> > +			igt_assert(fb_id > 0);
> > +		} else {
> > +			stride_min = overlay_fb_w *
> > +				     igt_format_plane_bpp(format, 0) / 8;
> > +
> > +			/* Randomize the stride with at most twice the
> > minimum. */
> > +			stride = (rand() % stride_min) + stride_min;
> > +
> > +			/* Pixman requires the stride to be aligned to 32-byte
> > words. */
> > +			stride = ALIGN(stride, sizeof(uint32_t));
> > +
> > +			igt_debug("Plane %d: using stride %d\n",
> > overlay_planes_index, stride);
> > +
> > +			fb_id = igt_fb_convert_with_stride(overlay_fb,
> > +							   &pattern_fb,
> > +							   format, stride);
> > +			igt_assert(fb_id);
> > +		}
> > +
> > +		igt_plane_set_fb(plane, overlay_fb);
> > +
> > +		overlay_surface = igt_get_cairo_surface(data->drm_fd,
> > +							&pattern_fb);
> > +
> > +		/* Blend the overlay surface. */
> > +		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
> > +		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
> > +
> > +		cr = cairo_create(result_surface);
> > +
> > +		cairo_set_source_surface(cr, overlay_surface,
> > overlay_surface_x,
> > +					 overlay_surface_y);
> > +		cairo_surface_destroy(overlay_surface);
> > +
> > +		/* Clip the surface to a rectangle. */
> > +		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
> > +				overlay_crtc_w, overlay_crtc_h);
> > +		cairo_clip(cr);
> > +
> > +		cairo_paint(cr);
> > +
> > +		cairo_destroy(cr);
> > +
> > +		/* Configure the plane with framebuffer source coordinates. */
> > +		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
> > +		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
> > +
> > +		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
> > +				    overlay_src_y);
> > +		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
> > +				overlay_src_h);
> > +
> > +		igt_remove_fb(data->drm_fd, &pattern_fb);
> > +
> > +		overlay_planes_index++;
> > +	}
> > +
> > +	cairo_surface_destroy(result_surface);
> > +
> > +	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> > +							&result_fb);
> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> > +		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
> > +
> > +		igt_debug("Bandwidth limitation exeeded: %s\n",
> > +			  bandwidth_exceeded ? "Yes" : "No");
> > +
> > +		igt_assert(!bandwidth_exceeded);
> > +	}
> Why does this need to be handled separately for VC4 devices? We already
> print the return code from a failed atomic commit into the debugging
> output, and I'd assume anyone looking at the output from the failure of
> one of these tests probably knows what that means. Additionally, if we
> wanted to print debugging messages for bandwidth limitations being
> exceeded for atomic commits on vc4, seems like it would probably be
> better to just add that code to igt_display_try_commit2()

You're right, after all the following call will produce more or less
the same result and there is nothing really specific to the VC4 there.
I'll drop it in the future.

> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1);
> > +	crc = chamelium_read_captured_crcs(data->chamelium,
> > +					   &captured_frame_count);
> > +
> > +	igt_assert(captured_frame_count == 1);
> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		underrun = igt_sysfs_get(debugfs, "underrun");
> > +		igt_assert(underrun);
> > +
> > +		underrun_detected = (underrun[0] == 'Y');
> > +		free(underrun);
> > +
> > +		igt_debug("Underrun detected: %s\n",
> > +			  underrun_detected ? "Yes" : "No");
> > +
> > +		igt_assert(!underrun_detected);
> > +
> > +		close(debugfs);
> > +	}
> This will definitely be used again in the future, this should also go
> into a helper for vc4.

Since we're having problems with underrun testing and the Chamelium, I
think I'll drop that part for now and maybe re-introduce it as a vc4
helper (it's specific to the vc4 driver in the latest iteration) later.

> > +
> > +	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
> > +
> > +	chamelium_assert_crc_eq_or_dump(data->chamelium,
> > +					expected_crc, crc,
> > +					&result_fb, i);
> > +
> > +	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
> > +	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
> Why not just add this debugging output to
> chamelium_assert_crc_eq_or_dump()?

Yes, let's do that.

> > +
> > +	free(expected_crc);
> > +	free(crc);
> > +
> > +	for (i = 0, overlay_planes_index = 0;
> > +	     i < pipe->n_planes && overlay_planes_index <
> > overlay_planes_count;
> > +	     i++) {
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +		struct igt_fb *overlay_fb =
> > &overlay_fbs[overlay_planes_index];
> > +
> > +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> > +			continue;
> > +
> > +		igt_remove_fb(data->drm_fd, overlay_fb);
> > +
> > +		overlay_planes_index++;
> > +	}
> > +
> > +	free(overlay_fbs);
> > +
> > +	igt_remove_fb(data->drm_fd, &primary_fb);
> > +	igt_remove_fb(data->drm_fd, &result_fb);
> > +}
> > +
> >  static void
> >  test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
> >  {
> > @@ -981,6 +1307,11 @@ igt_main
> >  			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
> >  					      CHAMELIUM_CHECK_ANALOG, 1);
> > 
> > +		connector_subtest("hdmi-crc-planes-random", HDMIA)
> > +			test_display_planes_random(&data, port,
> > +						   CHAMELIUM_CHECK_CRC);
> > +
> > +
> >  		connector_subtest("hdmi-frame-dump", HDMIA)
> >  			test_display_frame_dump(&data, port);
> >  	}

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for Chamelium VC4 plane testing, with T-tiled mode

[Patchwork]

== Series Details ==

Series: Chamelium VC4 plane testing, with T-tiled mode
URL   : https://patchwork.freedesktop.org/series/53665/
State : failure

== Summary ==

Applying: chamelium: Pass dimensions instead of mode to pattern generation helper
Using index info to reconstruct a base tree...
M	tests/kms_chamelium.c
Falling back to patching base and 3-way merge...
Auto-merging tests/kms_chamelium.c
CONFLICT (content): Merge conflict in tests/kms_chamelium.c
Patch failed at 0001 chamelium: Pass dimensions instead of mode to pattern generation helper
Use 'git am --show-current-patch' to see the failed patch
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".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev