[PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Paulo Zanoni]

Hello

I've been carrying some local IGT patches that reduced the size of buffers
created by igt_create_fb() so they would fit the stolen memory, but when I
decided to test the tree without them, I concluded the lack of sane sizes was
even causing test failures. So here's my attempt to fix this. This series alone
should help reducing the number of kms_frontbuffer_tracking failures seen by QA.

The last few patches make the FBC tests a little harder. They are all based on
the feedback I got from the last patches I sent.

Thanks,
Paulo

Paulo Zanoni (10):
  lib/igt_fb: fix fb->size when provided by the user
  lib/igt_fb: fix igt_create_fb_with_bo_size() documentation
  lib/igt_fb: fix open-coded ALIGN()
  lib/igt_fb: also pass the stride to igt_create_fb_with_bo_size()
  kms_frontbuffer_tracking: set our own size for the FBs we create
  kms_frontbuffer_tracking: do page flips using the planes API
  kms_frontbuffer_tracking: move flip_type to struct test_mode
  kms_frontbuffer_tracking: expand badstride and stridechange
  kms_frontbuffer_tracking: assert the stride changes at stridechange()
  kms_frontbuffer_tracking: add tilingchange subtest

 lib/igt_fb.c                     |  31 +++---
 lib/igt_fb.h                     |   3 +-
 tests/kms_flip.c                 |   2 +-
 tests/kms_frontbuffer_tracking.c | 220 ++++++++++++++++++++++++++++++++-------
 4 files changed, 204 insertions(+), 52 deletions(-)

-- 
2.6.2

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

[PATCH igt 01/10] lib/igt_fb: fix fb->size when provided by the user

[Paulo Zanoni]

I want to have a little more control over the size of the buffers in
kms_frontbuffer_tracking, so I decided to start calling
igt_create_fb_with_bo_size() instead of igt_create_fb(). The problem
is that create_bo_for_fb() returns its own calculated size as size_ret
instead of the actual used size.

So we fix this by returning the actual size, the one used in
gem_create instead of the calculated size that's not used anywhere.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 lib/igt_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 13a6a34..c70fb2c 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -116,7 +116,7 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
 		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
 
 	*stride_ret = stride;
-	*size_ret = size;
+	*size_ret = bo_size;
 	*gem_handle_ret = gem_handle;
 
 	return ret;
-- 
2.6.2

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

[PATCH igt 02/10] lib/igt_fb: fix igt_create_fb_with_bo_size() documentation

[Paulo Zanoni]

If we pass zero as the bo_size we won't get the minimum needed size,
we'll just get a size that works. The size is decided by
create_bo_for_fb(). The selected size is really not minimal for tiled
objects.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 lib/igt_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index c70fb2c..088bc0d 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -400,7 +400,7 @@ void igt_paint_image(cairo_t *cr, const char *filename,
  * @format: drm fourcc pixel format code
  * @tiling: tiling layout of the framebuffer (as framebuffer modifier)
  * @fb: pointer to an #igt_fb structure
- * @bo_size: size of the backing bo (0 for minimum needed size)
+ * @bo_size: size of the backing bo (0 for automatic size)
  *
  * This function allocates a gem buffer object suitable to back a framebuffer
  * with the requested properties and then wraps it up in a drm framebuffer
-- 
2.6.2

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

[PATCH igt 03/10] lib/igt_fb: fix open-coded ALIGN()

[Paulo Zanoni]

Maybe this will help someone's life in the future.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 lib/igt_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 088bc0d..2818c9f 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -104,7 +104,7 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
 			;
 	} else {
 		/* Scan-out has a 64 byte alignment restriction */
-		stride = (width * (bpp / 8) + 63) & ~63;
+		stride = ALIGN(width * (bpp / 8), 64);
 		size = stride * height;
 	}
 
-- 
2.6.2

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

[PATCH igt 04/10] lib/igt_fb: also pass the stride to igt_create_fb_with_bo_size()

[Paulo Zanoni]

If the caller is going to specify a custom size, it's likely that he
will also specify a custom stride. The automatic stride picked by
create_bo_for_fb() is too huge for tiled buffers, so if the caller
wants smaller buffers, then he'll need a smaller stride too, otherwise
the Kernel will reject the addfb IOCTL due to stride * height being
bigger than the size.

I want to make tests/kms_frontbuffer_tracking use
igt_create_fb_with_bo_size() so I can provide smaller buffers that
will fit into the CFB. I'm also planning to make all frontbuffers with
the same width/height/format have the same stride and size regardless
of tiling method so I can exercise specific code paths.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 lib/igt_fb.c     | 25 ++++++++++++++++---------
 lib/igt_fb.h     |  3 ++-
 tests/kms_flip.c |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 2818c9f..3ea9915 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -76,9 +76,8 @@ static struct format_desc_struct {
 /* helpers to create nice-looking framebuffers */
 static int create_bo_for_fb(int fd, int width, int height, int bpp,
 			    uint64_t tiling, unsigned bo_size,
-			    uint32_t *gem_handle_ret,
-			    unsigned *size_ret,
-			    unsigned *stride_ret)
+			    unsigned bo_stride, uint32_t *gem_handle_ret,
+			    unsigned *size_ret, unsigned *stride_ret)
 {
 	uint32_t gem_handle;
 	int size, ret = 0;
@@ -110,12 +109,16 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
 
 	if (bo_size == 0)
 		bo_size = size;
+	if (bo_stride == 0)
+		bo_stride = stride;
+
 	gem_handle = gem_create(fd, bo_size);
 
 	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
-		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
+				       bo_stride);
 
-	*stride_ret = stride;
+	*stride_ret = bo_stride;
 	*size_ret = bo_size;
 	*gem_handle_ret = gem_handle;
 
@@ -401,6 +404,7 @@ void igt_paint_image(cairo_t *cr, const char *filename,
  * @tiling: tiling layout of the framebuffer (as framebuffer modifier)
  * @fb: pointer to an #igt_fb structure
  * @bo_size: size of the backing bo (0 for automatic size)
+ * @bo_stride: stride of the backing bo (0 for automatic stride)
  *
  * This function allocates a gem buffer object suitable to back a framebuffer
  * with the requested properties and then wraps it up in a drm framebuffer
@@ -415,7 +419,8 @@ void igt_paint_image(cairo_t *cr, const char *filename,
 unsigned int
 igt_create_fb_with_bo_size(int fd, int width, int height,
 			   uint32_t format, uint64_t tiling,
-			   struct igt_fb *fb, unsigned bo_size)
+			   struct igt_fb *fb, unsigned bo_size,
+			   unsigned bo_stride)
 {
 	uint32_t fb_id;
 	int bpp;
@@ -427,7 +432,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
 	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
 		  __func__, width, height, format, bpp, tiling, bo_size);
 	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
-				   &fb->gem_handle, &fb->size, &fb->stride));
+				   bo_stride, &fb->gem_handle, &fb->size,
+				   &fb->stride));
 
 	igt_debug("%s(handle=%d, pitch=%d)\n",
 		  __func__, fb->gem_handle, fb->stride);
@@ -485,7 +491,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
 unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 			   uint64_t tiling, struct igt_fb *fb)
 {
-	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, fb, 0);
+	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, fb,
+					  0, 0);
 }
 
 /**
@@ -714,7 +721,7 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
 	 */
 	bpp = igt_drm_format_to_bpp(fb->drm_format);
 	ret = create_bo_for_fb(fd, fb->width, fb->height, bpp,
-				LOCAL_DRM_FORMAT_MOD_NONE, 0,
+				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
 				&blit->linear.handle,
 				&blit->linear.size,
 				&blit->linear.stride);
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index a07acd2..37892b5 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -72,7 +72,8 @@ enum igt_text_align {
 unsigned int
 igt_create_fb_with_bo_size(int fd, int width, int height,
 			   uint32_t format, uint64_t tiling,
-			   struct igt_fb *fb, unsigned bo_size);
+			   struct igt_fb *fb, unsigned bo_size,
+			   unsigned bo_stride);
 unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 			   uint64_t tiling, struct igt_fb *fb);
 unsigned int igt_create_color_fb(int fd, int width, int height,
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index e1b5856..e65fef2 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -1390,7 +1390,7 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 					 tiling, &o->fb_info[0]);
 	o->fb_ids[1] = igt_create_fb_with_bo_size(drm_fd, o->fb_width, o->fb_height,
 					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
-					 tiling, &o->fb_info[1], bo_size);
+					 tiling, &o->fb_info[1], bo_size, 0);
 	o->fb_ids[2] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
 					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
 					 LOCAL_I915_FORMAT_MOD_X_TILED, &o->fb_info[2]);
-- 
2.6.2

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

[PATCH igt 05/10] kms_frontbuffer_tracking: set our own size for the FBs we create

[Paulo Zanoni]

If we just call igt_create_fb(), the automatic size calculated by
create_bo_for_fb() may be way too big for the FBC CFB, and this will
result in SKIPs due to not enough stolen memroy. So in order to avoid
that, let's compute our own sizes.

Besides this, I want to make sure that both the untiled and X tiled -
and, in the future, Y tiled - buffers have the same size for the same
width/height/format. This will allow better control over the failure
paths exercised by our tests: when we try to flip from tiled to
untiled, we'll be sure that the change in buffer size does not cause
the wrong error path to be executed.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index ee72532..74acc73 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -479,6 +479,7 @@ static void create_fb(enum pixel_format pformat, int width, int height,
 		      uint64_t tiling, int plane, struct igt_fb *fb)
 {
 	uint32_t format;
+	unsigned int buf_size, pixel_size, stride;
 
 	switch (pformat) {
 	case FORMAT_RGB888:
@@ -508,7 +509,21 @@ static void create_fb(enum pixel_format pformat, int width, int height,
 		igt_assert(false);
 	}
 
-	igt_create_fb(drm.fd, width, height, format, tiling, fb);
+	/* We want all frontbuffers with the same width/weight/format to have
+	 * the same size regardless of tiling. Besides, the automatic size from
+	 * intel_create_fb() is too big and may lead to many SKIPs due to not
+	 * enough stolen memory. */
+	pixel_size = igt_drm_format_to_bpp(format) / 8;
+	if (plane == PLANE_CUR) {
+		stride = ALIGN(width * pixel_size, 64);
+		buf_size = stride * height;
+	} else {
+		stride = ALIGN(width * pixel_size, 512);
+		buf_size = stride * ALIGN(height, 32);
+	}
+
+	igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling, fb,
+				   buf_size, stride);
 }
 
 static uint32_t pick_color(struct igt_fb *fb, enum color ecolor)
-- 
2.6.2

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

[PATCH igt 06/10] kms_frontbuffer_tracking: do page flips using the planes API

[Paulo Zanoni]

Add a new FLIP_PLANES enum so we can do "page flips" using it too. The
goal is to exercise the fast modeset paths on the Kernel.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 74acc73..994bb4a 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -122,6 +122,7 @@ enum flip_type {
 	FLIP_PAGEFLIP,
 	FLIP_PAGEFLIP_EVENT,
 	FLIP_MODESET,
+	FLIP_PLANES,
 };
 
 enum color {
@@ -2194,6 +2195,31 @@ static void wait_flip_event(void)
 	}
 }
 
+static void set_prim_plane_for_params(struct modeset_params *params)
+{
+	int rc, i, crtc_index = -1;
+	uint32_t plane_id = 0;
+
+	for (i = 0; i < drm.res->count_crtcs; i++)
+		if (drm.res->crtcs[i] == params->crtc_id)
+			crtc_index = i;
+	igt_assert(crtc_index >= 0);
+
+	for (i = 0; i < drm.plane_res->count_planes; i++)
+		if ((drm.planes[i]->possible_crtcs & (1 << crtc_index)) &&
+		    drm.plane_types[i] == DRM_PLANE_TYPE_PRIMARY)
+			plane_id = drm.planes[i]->plane_id;
+	igt_assert(plane_id);
+
+	rc = drmModeSetPlane(drm.fd, plane_id, params->crtc_id,
+			     params->fb.fb->fb_id, 0, 0, 0,
+			     params->mode->hdisplay,
+			     params->mode->vdisplay,
+			     params->fb.x << 16, params->fb.y << 16,
+			     params->fb.w << 16, params->fb.h << 16);
+	igt_assert(rc == 0);
+}
+
 static void page_flip_for_params(struct modeset_params *params,
 				 enum flip_type type)
 {
@@ -2215,6 +2241,9 @@ static void page_flip_for_params(struct modeset_params *params,
 	case FLIP_MODESET:
 		set_mode_for_params(params);
 		break;
+	case FLIP_PLANES:
+		set_prim_plane_for_params(params);
+		break;
 	default:
 		igt_assert(false);
 	}
@@ -3184,6 +3213,13 @@ int main(int argc, char *argv[])
 			      igt_draw_get_method_name(t.method))
 			flip_subtest(&t, FLIP_MODESET);
 
+		igt_subtest_f("%s-%s-%s-%s-plflip-%s",
+			      feature_str(t.feature),
+			      pipes_str(t.pipes),
+			      screen_str(t.screen),
+			      fbs_str(t.fbs),
+			      igt_draw_get_method_name(t.method))
+			flip_subtest(&t, FLIP_PLANES);
 	TEST_MODE_ITER_END
 
 	TEST_MODE_ITER_BEGIN(t)
-- 
2.6.2

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

[PATCH igt 07/10] kms_frontbuffer_tracking: move flip_type to struct test_mode

[Paulo Zanoni]

Handle it just like we handle t->format. IMHO, it's better.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 81 ++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 41 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 994bb4a..25e6afd 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -115,14 +115,17 @@ struct test_mode {
 		FORMAT_DEFAULT = FORMAT_RGB888,
 	} format;
 
-	enum igt_draw_method method;
-};
+	/* There are multiple APIs where we can do the equivalent of a page flip
+	 * and they exercise slightly different codepaths inside the Kernel. */
+	enum flip_type {
+		FLIP_PAGEFLIP,
+		FLIP_PAGEFLIP_EVENT,
+		FLIP_MODESET,
+		FLIP_PLANES,
+		FLIP_COUNT,
+	} flip;
 
-enum flip_type {
-	FLIP_PAGEFLIP,
-	FLIP_PAGEFLIP_EVENT,
-	FLIP_MODESET,
-	FLIP_PLANES,
+	enum igt_draw_method method;
 };
 
 enum color {
@@ -2264,7 +2267,7 @@ static void page_flip_for_params(struct modeset_params *params,
  *   On a failure here you need to go directly to the Kernel's flip code and see
  *   how it interacts with the feature being tested.
  */
-static void flip_subtest(const struct test_mode *t, enum flip_type type)
+static void flip_subtest(const struct test_mode *t)
 {
 	int r;
 	int assertions = 0;
@@ -2301,7 +2304,7 @@ static void flip_subtest(const struct test_mode *t, enum flip_type type)
 		draw_rect(pattern, &params->fb, t->method, r);
 		update_wanted_crc(t, &pattern->crcs[t->format][r]);
 
-		page_flip_for_params(params, type);
+		page_flip_for_params(params, t->flip);
 
 		do_assertions(assertions);
 	}
@@ -3105,8 +3108,25 @@ static const char *format_str(enum pixel_format format)
 	}
 }
 
+static const char *flip_str(enum flip_type flip)
+{
+	switch (flip) {
+	case FLIP_PAGEFLIP:
+		return "pg";
+	case FLIP_PAGEFLIP_EVENT:
+		return "ev";
+	case FLIP_MODESET:
+		return "ms";
+	case FLIP_PLANES:
+		return "pl";
+	default:
+		igt_assert(false);
+	}
+}
+
 #define TEST_MODE_ITER_BEGIN(t) \
 	t.format = FORMAT_DEFAULT;					   \
+	t.flip = FLIP_PAGEFLIP;						   \
 	for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) {	   \
 	for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) {		   \
 	for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) {	   \
@@ -3162,7 +3182,8 @@ int main(int argc, char *argv[])
 			t.plane = PLANE_PRI;
 			t.fbs = FBS_INDIVIDUAL;
 			t.format = FORMAT_DEFAULT;
-			/* Make sure nothing is using this value. */
+			/* Make sure nothing is using these values. */
+			t.flip = -1;
 			t.method = -1;
 
 			igt_subtest_f("%s-%s-rte",
@@ -3189,37 +3210,15 @@ int main(int argc, char *argv[])
 		    (!opt.show_hidden && t.method != IGT_DRAW_BLT))
 			continue;
 
-		igt_subtest_f("%s-%s-%s-%s-flip-%s",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      fbs_str(t.fbs),
-			      igt_draw_get_method_name(t.method))
-			flip_subtest(&t, FLIP_PAGEFLIP);
-
-		igt_subtest_f("%s-%s-%s-%s-evflip-%s",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      fbs_str(t.fbs),
-			      igt_draw_get_method_name(t.method))
-			flip_subtest(&t, FLIP_PAGEFLIP_EVENT);
-
-		igt_subtest_f("%s-%s-%s-%s-msflip-%s",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      fbs_str(t.fbs),
-			      igt_draw_get_method_name(t.method))
-			flip_subtest(&t, FLIP_MODESET);
-
-		igt_subtest_f("%s-%s-%s-%s-plflip-%s",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      fbs_str(t.fbs),
-			      igt_draw_get_method_name(t.method))
-			flip_subtest(&t, FLIP_PLANES);
+		for (t.flip = 0; t.flip < FLIP_COUNT; t.flip++)
+			igt_subtest_f("%s-%s-%s-%s-%sflip-%s",
+				      feature_str(t.feature),
+				      pipes_str(t.pipes),
+				      screen_str(t.screen),
+				      fbs_str(t.fbs),
+				      flip_str(t.flip),
+				      igt_draw_get_method_name(t.method))
+				flip_subtest(&t);
 	TEST_MODE_ITER_END
 
 	TEST_MODE_ITER_BEGIN(t)
-- 
2.6.2

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

[PATCH igt 08/10] kms_frontbuffer_tracking: expand badstride and stridechange

[Paulo Zanoni]

Make those subtests try to change the stride using multiple APIs so we
can catch errors that affect full modesets, fast modesets and page
flips.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 52 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 25e6afd..436f1ec 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -2898,24 +2898,47 @@ static void try_invalid_strides(void)
  */
 static void badstride_subtest(const struct test_mode *t)
 {
-	struct igt_fb wide_fb;
+	struct igt_fb wide_fb, *old_fb;
 	struct modeset_params *params = pick_params(t);
+	int rc;
 
 	try_invalid_strides();
 
 	prepare_subtest(t, NULL);
 
+	old_fb = params->fb.fb;
+
 	create_fb(t->format, params->fb.fb->width + 4096, params->fb.fb->height,
 		  LOCAL_I915_FORMAT_MOD_X_TILED, t->plane, &wide_fb);
 	igt_assert(wide_fb.stride > 16384);
 
 	fill_fb(&wide_fb, COLOR_PRIM_BG);
 
+	/* Try a simple modeset with the new fb. */
 	params->fb.fb = &wide_fb;
 	set_mode_for_params(params);
+	do_assertions(ASSERT_FBC_DISABLED);
+
+	/* Go back to the old fb so FBC works again. */
+	params->fb.fb = old_fb;
+	set_mode_for_params(params);
+	do_assertions(0);
 
+	/* We're doing the equivalent of a modeset, but with the planes API. */
+	params->fb.fb = &wide_fb;
+	set_prim_plane_for_params(params);
 	do_assertions(ASSERT_FBC_DISABLED);
 
+	params->fb.fb = old_fb;
+	set_mode_for_params(params);
+	do_assertions(0);
+
+	/* We can't use the page flip IOCTL to flip to a buffer with a different
+	 * stride. */
+	rc = drmModePageFlip(drm.fd, params->crtc_id, wide_fb.fb_id, 0, NULL);
+	igt_assert(rc == -EINVAL);
+	do_assertions(0);
+
 	igt_remove_fb(drm.fd, &wide_fb);
 }
 
@@ -2941,22 +2964,43 @@ static void badstride_subtest(const struct test_mode *t)
  */
 static void stridechange_subtest(const struct test_mode *t)
 {
-	struct igt_fb new_fb;
+	struct igt_fb new_fb, *old_fb;
 	struct modeset_params *params = pick_params(t);
+	int rc;
 
 	prepare_subtest(t, NULL);
 
+	old_fb = params->fb.fb;
+
 	create_fb(t->format, params->fb.fb->width + 512, params->fb.fb->height,
 		  LOCAL_I915_FORMAT_MOD_X_TILED, t->plane, &new_fb);
 	fill_fb(&new_fb, COLOR_PRIM_BG);
 
+	/* We can't assert that FBC will be enabled since there may not be
+	 * enough space for the CFB, but we can check the CRC. */
 	params->fb.fb = &new_fb;
 	set_mode_for_params(params);
+	do_assertions(DONT_ASSERT_FEATURE_STATUS);
 
-	/* We can't assert that FBC will be enabled since there may not be
-	 * enough space for the CFB, but we can check the CRC. */
+	/* Go back to the fb that can have FBC. */
+	params->fb.fb = old_fb;
+	set_mode_for_params(params);
+	do_assertions(0);
+
+	/* This operation is the same as above, but with the planes API. */
+	params->fb.fb = &new_fb;
+	set_prim_plane_for_params(params);
 	do_assertions(DONT_ASSERT_FEATURE_STATUS);
 
+	params->fb.fb = old_fb;
+	set_prim_plane_for_params(params);
+	do_assertions(0);
+
+	/* We just can't page flip with a new stride. */
+	rc = drmModePageFlip(drm.fd, params->crtc_id, new_fb.fb_id, 0, NULL);
+	igt_assert(rc == -EINVAL);
+	do_assertions(0);
+
 	igt_remove_fb(drm.fd, &new_fb);
 }
 
-- 
2.6.2

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

[PATCH igt 09/10] kms_frontbuffer_tracking: assert the stride changes at stridechange()

[Paulo Zanoni]

We were previously using igt_create_fb(), which decided the stride by
itself: there was no guarantee that making a buffer 512 pixels bigger
would make its stride change. We're setting our own stride now so we
have some guarantees, but let's put an assertion just to make sure
we're doing the right thing.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 436f1ec..5f6f3f1 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -2976,6 +2976,8 @@ static void stridechange_subtest(const struct test_mode *t)
 		  LOCAL_I915_FORMAT_MOD_X_TILED, t->plane, &new_fb);
 	fill_fb(&new_fb, COLOR_PRIM_BG);
 
+	igt_assert(old_fb->stride != new_fb.stride);
+
 	/* We can't assert that FBC will be enabled since there may not be
 	 * enough space for the CFB, but we can check the CRC. */
 	params->fb.fb = &new_fb;
-- 
2.6.2

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

[PATCH igt 10/10] kms_frontbuffer_tracking: add tilingchange subtest

[Paulo Zanoni]

During the review of a recent FBC patch, Ville pointed a problem that
happens when we use the page flip IOCTL to switch between buffers that
have different tiling formats. This test should catch the problem
introduced by that patch - which was not merged, by the way, so the
test should be passing.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 48 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 5f6f3f1..63a2a0d 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -3006,6 +3006,51 @@ static void stridechange_subtest(const struct test_mode *t)
 	igt_remove_fb(drm.fd, &new_fb);
 }
 
+/**
+ * tilingchange - alternate between tiled and untiled in multiple ways
+ *
+ * METHOD
+ *   This test alternates between tiled and untiled frontbuffers of the same
+ *   size and format through multiple different APIs: the page flip IOCTL,
+ *   normal modesets and the plane APIs.
+ *
+ * EXPECTED RESULTS
+ *   FBC gets properly disabled for the untiled FB and reenabled for the
+ *   tiled FB.
+ *
+ * FAILURES
+ *   Bad Kernels may somehow leave FBC enabled, which can cause FIFO underruns
+ *   that lead to CRC assertion failures.
+ */
+static void tilingchange_subtest(const struct test_mode *t)
+{
+	struct igt_fb new_fb, *old_fb;
+	struct modeset_params *params = pick_params(t);
+	enum flip_type flip_type;
+
+	prepare_subtest(t, NULL);
+
+	old_fb = params->fb.fb;
+
+	create_fb(t->format, params->fb.fb->width, params->fb.fb->height,
+		  LOCAL_DRM_FORMAT_MOD_NONE, t->plane, &new_fb);
+	fill_fb(&new_fb, COLOR_PRIM_BG);
+
+	for (flip_type = 0; flip_type < FLIP_COUNT; flip_type++) {
+		igt_debug("Flip type: %d\n", flip_type);
+
+		/* Set a buffer with no tiling. */
+		params->fb.fb = &new_fb;
+		page_flip_for_params(params, flip_type);
+		do_assertions(ASSERT_FBC_DISABLED);
+
+		/* Put FBC back in a working state. */
+		params->fb.fb = old_fb;
+		page_flip_for_params(params, flip_type);
+		do_assertions(0);
+	}
+}
+
 static int opt_handler(int option, int option_index, void *data)
 {
 	switch (option) {
@@ -3395,6 +3440,9 @@ int main(int argc, char *argv[])
 
 			igt_subtest_f("%s-stridechange", feature_str(t.feature))
 				stridechange_subtest(&t);
+
+			igt_subtest_f("%s-tilingchange", feature_str(t.feature))
+				tilingchange_subtest(&t);
 		}
 
 		if (t.feature & FEATURE_PSR)
-- 
2.6.2

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

Re: [PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Daniel Vetter]

On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
> Hello
> 
> I've been carrying some local IGT patches that reduced the size of buffers
> created by igt_create_fb() so they would fit the stolen memory, but when I
> decided to test the tree without them, I concluded the lack of sane sizes was
> even causing test failures. So here's my attempt to fix this. This series alone
> should help reducing the number of kms_frontbuffer_tracking failures seen by QA.
> 
> The last few patches make the FBC tests a little harder. They are all based on
> the feedback I got from the last patches I sent.

The point of a helper library is that it helps, not that every caller has
to work around it's choice of size and stride.

The only thing we need to do here is fix up the selection of stride and
size to make it not pick the super-conservative value that work even on
gen2&3. Something like:

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 13a6a34982e0..9eb97952ed95 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
 	if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
 		int v;
 
-		/* Round the tiling up to the next power-of-two and the
-		 * region up to the next pot fence size so that this works
-		 * on all generations.
-		 *
-		 * This can still fail if the framebuffer is too large to
-		 * be tiled. But then that failure is expected.
-		 */
-
-		v = width * bpp / 8;
-		for (stride = 512; stride < v; stride *= 2)
-			;
-
-		v = stride * height;
-		for (size = 1024*1024; size < v; size *= 2)
-			;
+		if (gen < 4) {
+			/* Round the tiling up to the next power-of-two and the
+			 * region up to the next pot fence size so that this works
+			 * on all generations.
+			 *
+			 * This can still fail if the framebuffer is too large to
+			 * be tiled. But then that failure is expected.
+			 */
+
+			v = width * bpp / 8;
+			for (stride = 512; stride < v; stride *= 2)
+				;
+
+			v = stride * height;
+			for (size = 1024*1024; size < v; size *= 2)
+				;
+		} else {
+			stride = ALIGN(stride, 512);
+			size = ALIGN(size, stride * 32);
+		}
 	} else {
 		/* Scan-out has a 64 byte alignment restriction */
 		stride = (width * (bpp / 8) + 63) & ~63;


Or whatever is the right thing to pick that works on gen4+.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Paulo Zanoni]

2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
>> Hello
>>
>> I've been carrying some local IGT patches that reduced the size of buffers
>> created by igt_create_fb() so they would fit the stolen memory, but when I
>> decided to test the tree without them, I concluded the lack of sane sizes was
>> even causing test failures. So here's my attempt to fix this. This series alone
>> should help reducing the number of kms_frontbuffer_tracking failures seen by QA.
>>
>> The last few patches make the FBC tests a little harder. They are all based on
>> the feedback I got from the last patches I sent.
>
> The point of a helper library is that it helps, not that every caller has
> to work around it's choice of size and stride.

Judging by the amount of users, it is helping even without my changes :)

>
> The only thing we need to do here is fix up the selection of stride and
> size to make it not pick the super-conservative value that work even on
> gen2&3. Something like:
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 13a6a34982e0..9eb97952ed95 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
>         if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
>                 int v;
>
> -               /* Round the tiling up to the next power-of-two and the
> -                * region up to the next pot fence size so that this works
> -                * on all generations.
> -                *
> -                * This can still fail if the framebuffer is too large to
> -                * be tiled. But then that failure is expected.
> -                */
> -
> -               v = width * bpp / 8;
> -               for (stride = 512; stride < v; stride *= 2)
> -                       ;
> -
> -               v = stride * height;
> -               for (size = 1024*1024; size < v; size *= 2)
> -                       ;
> +               if (gen < 4) {
> +                       /* Round the tiling up to the next power-of-two and the
> +                        * region up to the next pot fence size so that this works
> +                        * on all generations.
> +                        *
> +                        * This can still fail if the framebuffer is too large to
> +                        * be tiled. But then that failure is expected.
> +                        */
> +
> +                       v = width * bpp / 8;
> +                       for (stride = 512; stride < v; stride *= 2)
> +                               ;
> +
> +                       v = stride * height;
> +                       for (size = 1024*1024; size < v; size *= 2)
> +                               ;
> +               } else {
> +                       stride = ALIGN(stride, 512);
> +                       size = ALIGN(size, stride * 32);

Shouldn't it be size = stride * ALIGN(height, 32)?
(it still wouldn't be the minimal size, but would be close to it)

> +               }
>         } else {
>                 /* Scan-out has a 64 byte alignment restriction */
>                 stride = (width * (bpp / 8) + 63) & ~63;
>
>
> Or whatever is the right thing to pick that works on gen4+.

While that sounds like an improvement, it won't solve the
kms_frontbuffer_tracking problem where we want to specify size+stride
since we want all buffers using the same size+stride independently of
tiling/no-tiling.


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Daniel Vetter]

On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
>>> Hello
>>>
>>> I've been carrying some local IGT patches that reduced the size of buffers
>>> created by igt_create_fb() so they would fit the stolen memory, but when I
>>> decided to test the tree without them, I concluded the lack of sane sizes was
>>> even causing test failures. So here's my attempt to fix this. This series alone
>>> should help reducing the number of kms_frontbuffer_tracking failures seen by QA.
>>>
>>> The last few patches make the FBC tests a little harder. They are all based on
>>> the feedback I got from the last patches I sent.
>>
>> The point of a helper library is that it helps, not that every caller has
>> to work around it's choice of size and stride.
>
> Judging by the amount of users, it is helping even without my changes :)
>
>>
>> The only thing we need to do here is fix up the selection of stride and
>> size to make it not pick the super-conservative value that work even on
>> gen2&3. Something like:
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index 13a6a34982e0..9eb97952ed95 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
>>         if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
>>                 int v;
>>
>> -               /* Round the tiling up to the next power-of-two and the
>> -                * region up to the next pot fence size so that this works
>> -                * on all generations.
>> -                *
>> -                * This can still fail if the framebuffer is too large to
>> -                * be tiled. But then that failure is expected.
>> -                */
>> -
>> -               v = width * bpp / 8;
>> -               for (stride = 512; stride < v; stride *= 2)
>> -                       ;
>> -
>> -               v = stride * height;
>> -               for (size = 1024*1024; size < v; size *= 2)
>> -                       ;
>> +               if (gen < 4) {
>> +                       /* Round the tiling up to the next power-of-two and the
>> +                        * region up to the next pot fence size so that this works
>> +                        * on all generations.
>> +                        *
>> +                        * This can still fail if the framebuffer is too large to
>> +                        * be tiled. But then that failure is expected.
>> +                        */
>> +
>> +                       v = width * bpp / 8;
>> +                       for (stride = 512; stride < v; stride *= 2)
>> +                               ;
>> +
>> +                       v = stride * height;
>> +                       for (size = 1024*1024; size < v; size *= 2)
>> +                               ;
>> +               } else {
>> +                       stride = ALIGN(stride, 512);
>> +                       size = ALIGN(size, stride * 32);
>
> Shouldn't it be size = stride * ALIGN(height, 32)?
> (it still wouldn't be the minimal size, but would be close to it)

Yeah that's probably what we want.

>> +               }
>>         } else {
>>                 /* Scan-out has a 64 byte alignment restriction */
>>                 stride = (width * (bpp / 8) + 63) & ~63;
>>
>>
>> Or whatever is the right thing to pick that works on gen4+.
>
> While that sounds like an improvement, it won't solve the
> kms_frontbuffer_tracking problem where we want to specify size+stride
> since we want all buffers using the same size+stride independently of
> tiling/no-tiling.

Matching stride is a good reason for your changes (and then
kms_frontbuffer_tracking should allocate the tiled fb first and then
ask for an untiled fb with matching stride to avoid reimplementing the
stride rounding). But your cover letter talked about allocating less
in general, and that problem really should be fixed in the library
itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Ville Syrjälä]

On Wed, Nov 18, 2015 at 05:38:47PM +0100, Daniel Vetter wrote:
> On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> >> On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
> >>> Hello
> >>>
> >>> I've been carrying some local IGT patches that reduced the size of buffers
> >>> created by igt_create_fb() so they would fit the stolen memory, but when I
> >>> decided to test the tree without them, I concluded the lack of sane sizes was
> >>> even causing test failures. So here's my attempt to fix this. This series alone
> >>> should help reducing the number of kms_frontbuffer_tracking failures seen by QA.
> >>>
> >>> The last few patches make the FBC tests a little harder. They are all based on
> >>> the feedback I got from the last patches I sent.
> >>
> >> The point of a helper library is that it helps, not that every caller has
> >> to work around it's choice of size and stride.
> >
> > Judging by the amount of users, it is helping even without my changes :)
> >
> >>
> >> The only thing we need to do here is fix up the selection of stride and
> >> size to make it not pick the super-conservative value that work even on
> >> gen2&3. Something like:
> >>
> >> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> >> index 13a6a34982e0..9eb97952ed95 100644
> >> --- a/lib/igt_fb.c
> >> +++ b/lib/igt_fb.c
> >> @@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
> >>         if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
> >>                 int v;
> >>
> >> -               /* Round the tiling up to the next power-of-two and the
> >> -                * region up to the next pot fence size so that this works
> >> -                * on all generations.
> >> -                *
> >> -                * This can still fail if the framebuffer is too large to
> >> -                * be tiled. But then that failure is expected.
> >> -                */
> >> -
> >> -               v = width * bpp / 8;
> >> -               for (stride = 512; stride < v; stride *= 2)
> >> -                       ;
> >> -
> >> -               v = stride * height;
> >> -               for (size = 1024*1024; size < v; size *= 2)
> >> -                       ;
> >> +               if (gen < 4) {
> >> +                       /* Round the tiling up to the next power-of-two and the
> >> +                        * region up to the next pot fence size so that this works
> >> +                        * on all generations.
> >> +                        *
> >> +                        * This can still fail if the framebuffer is too large to
> >> +                        * be tiled. But then that failure is expected.
> >> +                        */
> >> +
> >> +                       v = width * bpp / 8;
> >> +                       for (stride = 512; stride < v; stride *= 2)
> >> +                               ;
> >> +
> >> +                       v = stride * height;
> >> +                       for (size = 1024*1024; size < v; size *= 2)
> >> +                               ;
> >> +               } else {
> >> +                       stride = ALIGN(stride, 512);
> >> +                       size = ALIGN(size, stride * 32);
> >
> > Shouldn't it be size = stride * ALIGN(height, 32)?
> > (it still wouldn't be the minimal size, but would be close to it)
> 
> Yeah that's probably what we want.

Or just have a helper to get us the actual tile size and use that.

> 
> >> +               }
> >>         } else {
> >>                 /* Scan-out has a 64 byte alignment restriction */
> >>                 stride = (width * (bpp / 8) + 63) & ~63;
> >>
> >>
> >> Or whatever is the right thing to pick that works on gen4+.
> >
> > While that sounds like an improvement, it won't solve the
> > kms_frontbuffer_tracking problem where we want to specify size+stride
> > since we want all buffers using the same size+stride independently of
> > tiling/no-tiling.
> 
> Matching stride is a good reason for your changes (and then
> kms_frontbuffer_tracking should allocate the tiled fb first and then
> ask for an untiled fb with matching stride to avoid reimplementing the
> stride rounding). But your cover letter talked about allocating less
> in general, and that problem really should be fixed in the library
> itself.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 00/10] igt_fb buffer sizes + kms_frontbuffer_tracking

[Zanoni, Paulo R]

Em Qua, 2015-11-18 às 17:38 +0100, Daniel Vetter escreveu:
> On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com>
> wrote:
> > 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > > On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
> > > > Hello
> > > > 
> > > > I've been carrying some local IGT patches that reduced the size
> > > > of buffers
> > > > created by igt_create_fb() so they would fit the stolen memory,
> > > > but when I
> > > > decided to test the tree without them, I concluded the lack of
> > > > sane sizes was
> > > > even causing test failures. So here's my attempt to fix this.
> > > > This series alone
> > > > should help reducing the number of kms_frontbuffer_tracking
> > > > failures seen by QA.
> > > > 
> > > > The last few patches make the FBC tests a little harder. They
> > > > are all based on
> > > > the feedback I got from the last patches I sent.
> > > 
> > > The point of a helper library is that it helps, not that every
> > > caller has
> > > to work around it's choice of size and stride.
> > 
> > Judging by the amount of users, it is helping even without my
> > changes :)
> > 
> > > 
> > > The only thing we need to do here is fix up the selection of
> > > stride and
> > > size to make it not pick the super-conservative value that work
> > > even on
> > > gen2&3. Something like:
> > > 
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index 13a6a34982e0..9eb97952ed95 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int
> > > width, int height, int bpp,
> > >         if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
> > >                 int v;
> > > 
> > > -               /* Round the tiling up to the next power-of-two
> > > and the
> > > -                * region up to the next pot fence size so that
> > > this works
> > > -                * on all generations.
> > > -                *
> > > -                * This can still fail if the framebuffer is too
> > > large to
> > > -                * be tiled. But then that failure is expected.
> > > -                */
> > > -
> > > -               v = width * bpp / 8;
> > > -               for (stride = 512; stride < v; stride *= 2)
> > > -                       ;
> > > -
> > > -               v = stride * height;
> > > -               for (size = 1024*1024; size < v; size *= 2)
> > > -                       ;
> > > +               if (gen < 4) {
> > > +                       /* Round the tiling up to the next power-
> > > of-two and the
> > > +                        * region up to the next pot fence size
> > > so that this works
> > > +                        * on all generations.
> > > +                        *
> > > +                        * This can still fail if the framebuffer
> > > is too large to
> > > +                        * be tiled. But then that failure is
> > > expected.
> > > +                        */
> > > +
> > > +                       v = width * bpp / 8;
> > > +                       for (stride = 512; stride < v; stride *=
> > > 2)
> > > +                               ;
> > > +
> > > +                       v = stride * height;
> > > +                       for (size = 1024*1024; size < v; size *=
> > > 2)
> > > +                               ;
> > > +               } else {
> > > +                       stride = ALIGN(stride, 512);
> > > +                       size = ALIGN(size, stride * 32);
> > 
> > Shouldn't it be size = stride * ALIGN(height, 32)?
> > (it still wouldn't be the minimal size, but would be close to it)
> 
> Yeah that's probably what we want.
> 
> > > +               }
> > >         } else {
> > >                 /* Scan-out has a 64 byte alignment restriction
> > > */
> > >                 stride = (width * (bpp / 8) + 63) & ~63;
> > > 
> > > 
> > > Or whatever is the right thing to pick that works on gen4+.
> > 
> > While that sounds like an improvement, it won't solve the
> > kms_frontbuffer_tracking problem where we want to specify
> > size+stride
> > since we want all buffers using the same size+stride independently
> > of
> > tiling/no-tiling.
> 
> Matching stride is a good reason for your changes (and then
> kms_frontbuffer_tracking should allocate the tiled fb first and then
> ask for an untiled fb with matching stride to avoid reimplementing
> the
> stride rounding).

That sounds a good idea, but it will require yet another rework to the
code.

>  But your cover letter talked about allocating less
> in general, and that problem really should be fixed in the library
> itself.

It is both problems actually. That's why I think we need both this
series so it's still possible to specify a custom stride+size, and also
your patch so the sizes are saner.

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