[PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good

[PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

This is a "review by igt test" for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:

commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo <oscar.mateo@intel.com>
Date:   Fri May 16 11:23:12 2014 +0100

    drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display

    Otherwise, we do a NULL pointer dereference.

    I've seen this happen while handling an error in
    i915_gem_object_pin_to_display_plane():

    If i915_gem_object_set_cache_level() fails, we call is_pin_display()
    to handle the error. At this point, the object is still not pinned
    to GGTT and maybe not even bound, so we have to check before we
    dereference its GGTT vma.

    v2: Chris Wilson says restoring the old value is easier, but that
    is_pin_display is useful as a theory of operation. Take the solomonic
    decision: at least this way is_pin_display is a little more robust
    (until Chris can kill it off).

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 tests/kms_flip.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 5 deletions(-)

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..7296122 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
 #define TEST_RPM		(1 << 25)
 #define TEST_SUSPEND		(1 << 26)
 #define TEST_TS_CONT		(1 << 27)
+#define TEST_BO_TOOBIG		(1 << 28)
 
 #define EVENT_FLIP		(1 << 0)
 #define EVENT_VBLANK		(1 << 1)
@@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
 	}
 }
 
+static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
+				   bool tiled, uint32_t *gem_handle_ret,
+				   unsigned *size_ret, unsigned *stride_ret)
+{
+	uint32_t gem_handle;
+	int size, ret = 0;
+	unsigned stride;
+
+	if (tiled) {
+		int v;
+
+		v = width * bpp / 8;
+		for (stride = 512; stride < v; stride *= 2)
+			;
+
+		v = stride * height;
+		for (size = 1024*1024; size < v; size *= 2)
+			;
+	} else {
+		/* Scan-out has a 64 byte alignment restriction */
+		stride = (width * (bpp / 8) + 63) & ~63;
+		size = stride * height;
+	}
+
+	/* 256 MB is usually the maximum mappable aperture,
+	 * (make it 4x times that to ensure failure) */
+	gem_handle = gem_create(fd, 4*256*1024*1024);
+
+	if (tiled)
+		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+
+	*stride_ret = stride;
+	*size_ret = size;
+	*gem_handle_ret = gem_handle;
+
+	return ret;
+}
+
+static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
+					     uint32_t format, bool tiled,
+					     struct igt_fb *fb)
+{
+	uint32_t handles[4];
+	uint32_t pitches[4];
+	uint32_t offsets[4];
+	uint32_t fb_id;
+	int bpp;
+	int ret;
+
+	memset(fb, 0, sizeof(*fb));
+
+	bpp = igt_drm_format_to_bpp(format);
+	ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
+			&fb->gem_handle, &fb->size, &fb->stride);
+	if (ret < 0)
+		return ret;
+
+	memset(handles, 0, sizeof(handles));
+	handles[0] = fb->gem_handle;
+	memset(pitches, 0, sizeof(pitches));
+	pitches[0] = fb->stride;
+	memset(offsets, 0, sizeof(offsets));
+	if (drmModeAddFB2(fd, width, height, format, handles, pitches,
+			  offsets, &fb_id, 0) < 0) {
+		gem_close(fd, fb->gem_handle);
+
+		return 0;
+	}
+
+	fb->width = width;
+	fb->height = height;
+	fb->tiling = tiled;
+	fb->drm_format = format;
+	fb->fb_id = fb_id;
+
+	return fb_id;
+}
+
 static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 				 int crtc_count, int duration_ms)
 {
@@ -1252,9 +1331,15 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	o->fb_ids[0] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
 					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
 					 tiled, &o->fb_info[0]);
-	o->fb_ids[1] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
-					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
-					 tiled, &o->fb_info[1]);
+	if (o->flags & TEST_BO_TOOBIG) {
+		o->fb_ids[1] = igt_create_fb_with_bigbo(drm_fd, o->fb_width, o->fb_height,
+						 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
+						 tiled, &o->fb_info[1]);
+	} else {
+		o->fb_ids[1] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
+						 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
+						 tiled, &o->fb_info[1]);
+	}
 	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),
 					 true, &o->fb_info[2]);
@@ -1264,7 +1349,8 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 		igt_require(o->fb_ids[2]);
 
 	paint_flip_mode(&o->fb_info[0], false);
-	paint_flip_mode(&o->fb_info[1], true);
+	if (!(o->flags & TEST_BO_TOOBIG))
+		paint_flip_mode(&o->fb_info[1], true);
 	if (o->fb_ids[2])
 		paint_flip_mode(&o->fb_info[2], true);
 
@@ -1288,10 +1374,15 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	if (o->flags & TEST_CHECK_TS)
 		sleep(1);
 
-	igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
+	if (o->flags & TEST_BO_TOOBIG) {
+		igt_assert(do_page_flip(o, o->fb_ids[1], true) == -E2BIG);
+		goto out;
+	} else
+		igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
 	wait_for_events(o);
 
 	o->current_fb_id = 1;
+
 	if (o->flags & TEST_FLIP)
 		o->flip_state.seq_step = 1;
 	else
@@ -1543,6 +1634,7 @@ int main(int argc, char **argv)
 		{ 10, TEST_VBLANK | TEST_DPMS | TEST_SUSPEND | TEST_TS_CONT, "dpms-vs-suspend" },
 		{ 0, TEST_VBLANK | TEST_MODESET | TEST_RPM | TEST_TS_CONT, "modeset-vs-rpm" },
 		{ 0, TEST_VBLANK | TEST_MODESET | TEST_SUSPEND | TEST_TS_CONT, "modeset-vs-suspend" },
+		{ 0, TEST_BO_TOOBIG | TEST_NO_2X_OUTPUT, "bo-too-big" },
 	};
 	int i;
 
-- 
1.9.0

Re: [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good

[Ville Syrjälä]

On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is a "review by igt test" for a bug located in
> i915_gem_object_pin_to_display_plane and fixed by:
> 
> commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
> Author: Oscar Mateo <oscar.mateo@intel.com>
> Date:   Fri May 16 11:23:12 2014 +0100
> 
>     drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
> 
>     Otherwise, we do a NULL pointer dereference.
> 
>     I've seen this happen while handling an error in
>     i915_gem_object_pin_to_display_plane():
> 
>     If i915_gem_object_set_cache_level() fails, we call is_pin_display()
>     to handle the error. At this point, the object is still not pinned
>     to GGTT and maybe not even bound, so we have to check before we
>     dereference its GGTT vma.
> 
>     v2: Chris Wilson says restoring the old value is easier, but that
>     is_pin_display is useful as a theory of operation. Take the solomonic
>     decision: at least this way is_pin_display is a little more robust
>     (until Chris can kill it off).
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  tests/kms_flip.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 97 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index bb4f71d..7296122 100644
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -74,6 +74,7 @@
>  #define TEST_RPM		(1 << 25)
>  #define TEST_SUSPEND		(1 << 26)
>  #define TEST_TS_CONT		(1 << 27)
> +#define TEST_BO_TOOBIG		(1 << 28)
>  
>  #define EVENT_FLIP		(1 << 0)
>  #define EVENT_VBLANK		(1 << 1)
> @@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
>  	}
>  }
>  
> +static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
> +				   bool tiled, uint32_t *gem_handle_ret,
> +				   unsigned *size_ret, unsigned *stride_ret)
> +{
> +	uint32_t gem_handle;
> +	int size, ret = 0;
> +	unsigned stride;
> +
> +	if (tiled) {
> +		int v;
> +
> +		v = width * bpp / 8;
> +		for (stride = 512; stride < v; stride *= 2)
> +			;
> +
> +		v = stride * height;
> +		for (size = 1024*1024; size < v; size *= 2)
> +			;
> +	} else {
> +		/* Scan-out has a 64 byte alignment restriction */
> +		stride = (width * (bpp / 8) + 63) & ~63;
> +		size = stride * height;
> +	}
> +
> +	/* 256 MB is usually the maximum mappable aperture,
> +	 * (make it 4x times that to ensure failure) */
> +	gem_handle = gem_create(fd, 4*256*1024*1024);
> +
> +	if (tiled)
> +		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
> +
> +	*stride_ret = stride;
> +	*size_ret = size;
> +	*gem_handle_ret = gem_handle;
> +
> +	return ret;
> +}
> +
> +static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
> +					     uint32_t format, bool tiled,
> +					     struct igt_fb *fb)
> +{
> +	uint32_t handles[4];
> +	uint32_t pitches[4];
> +	uint32_t offsets[4];
> +	uint32_t fb_id;
> +	int bpp;
> +	int ret;
> +
> +	memset(fb, 0, sizeof(*fb));
> +
> +	bpp = igt_drm_format_to_bpp(format);
> +	ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
> +			&fb->gem_handle, &fb->size, &fb->stride);
> +	if (ret < 0)
> +		return ret;
> +
> +	memset(handles, 0, sizeof(handles));
> +	handles[0] = fb->gem_handle;
> +	memset(pitches, 0, sizeof(pitches));
> +	pitches[0] = fb->stride;
> +	memset(offsets, 0, sizeof(offsets));
> +	if (drmModeAddFB2(fd, width, height, format, handles, pitches,
> +			  offsets, &fb_id, 0) < 0) {
> +		gem_close(fd, fb->gem_handle);
> +
> +		return 0;
> +	}
> +
> +	fb->width = width;
> +	fb->height = height;
> +	fb->tiling = tiled;
> +	fb->drm_format = format;
> +	fb->fb_id = fb_id;
> +
> +	return fb_id;
> +}

Could we avoid a bit of code duplication with something like this?

create_bo_for_fb(..., bo_size)
{
	...
	if (bo_size == 0)
		bo_size = size;
	gem_handle = gem_create(fd, bo_size);
	...
}
igt_create_fb_with_bo_size(xxx, bo_size)
{
	...
	create_bo_for_fb(..., bo_size);
	...
}
igt_create_fb(xxx)
{
	return igt_create_fb_with_bo_size(xxx, 0);
}

Otherwise looks pretty good.

> +
>  static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  				 int crtc_count, int duration_ms)
>  {
> @@ -1252,9 +1331,15 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  	o->fb_ids[0] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
>  					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
>  					 tiled, &o->fb_info[0]);
> -	o->fb_ids[1] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
> -					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
> -					 tiled, &o->fb_info[1]);
> +	if (o->flags & TEST_BO_TOOBIG) {
> +		o->fb_ids[1] = igt_create_fb_with_bigbo(drm_fd, o->fb_width, o->fb_height,
> +						 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
> +						 tiled, &o->fb_info[1]);
> +	} else {
> +		o->fb_ids[1] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
> +						 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
> +						 tiled, &o->fb_info[1]);
> +	}
>  	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),
>  					 true, &o->fb_info[2]);
> @@ -1264,7 +1349,8 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  		igt_require(o->fb_ids[2]);
>  
>  	paint_flip_mode(&o->fb_info[0], false);
> -	paint_flip_mode(&o->fb_info[1], true);
> +	if (!(o->flags & TEST_BO_TOOBIG))
> +		paint_flip_mode(&o->fb_info[1], true);
>  	if (o->fb_ids[2])
>  		paint_flip_mode(&o->fb_info[2], true);
>  
> @@ -1288,10 +1374,15 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  	if (o->flags & TEST_CHECK_TS)
>  		sleep(1);
>  
> -	igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
> +	if (o->flags & TEST_BO_TOOBIG) {
> +		igt_assert(do_page_flip(o, o->fb_ids[1], true) == -E2BIG);
> +		goto out;
> +	} else
> +		igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
>  	wait_for_events(o);
>  
>  	o->current_fb_id = 1;
> +
>  	if (o->flags & TEST_FLIP)
>  		o->flip_state.seq_step = 1;
>  	else
> @@ -1543,6 +1634,7 @@ int main(int argc, char **argv)
>  		{ 10, TEST_VBLANK | TEST_DPMS | TEST_SUSPEND | TEST_TS_CONT, "dpms-vs-suspend" },
>  		{ 0, TEST_VBLANK | TEST_MODESET | TEST_RPM | TEST_TS_CONT, "modeset-vs-rpm" },
>  		{ 0, TEST_VBLANK | TEST_MODESET | TEST_SUSPEND | TEST_TS_CONT, "modeset-vs-suspend" },
> +		{ 0, TEST_BO_TOOBIG | TEST_NO_2X_OUTPUT, "bo-too-big" },
>  	};
>  	int i;
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good

[Ville Syrjälä]

On Fri, May 16, 2014 at 03:04:21PM +0300, Ville Syrjälä wrote:
> On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.mateo@intel.com wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> > 
> > This is a "review by igt test" for a bug located in
> > i915_gem_object_pin_to_display_plane and fixed by:
> > 
> > commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
> > Author: Oscar Mateo <oscar.mateo@intel.com>
> > Date:   Fri May 16 11:23:12 2014 +0100
> > 
> >     drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
> > 
> >     Otherwise, we do a NULL pointer dereference.
> > 
> >     I've seen this happen while handling an error in
> >     i915_gem_object_pin_to_display_plane():
> > 
> >     If i915_gem_object_set_cache_level() fails, we call is_pin_display()
> >     to handle the error. At this point, the object is still not pinned
> >     to GGTT and maybe not even bound, so we have to check before we
> >     dereference its GGTT vma.
> > 
> >     v2: Chris Wilson says restoring the old value is easier, but that
> >     is_pin_display is useful as a theory of operation. Take the solomonic
> >     decision: at least this way is_pin_display is a little more robust
> >     (until Chris can kill it off).
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > ---
> >  tests/kms_flip.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 97 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> > index bb4f71d..7296122 100644
> > --- a/tests/kms_flip.c
> > +++ b/tests/kms_flip.c
> > @@ -74,6 +74,7 @@
> >  #define TEST_RPM		(1 << 25)
> >  #define TEST_SUSPEND		(1 << 26)
> >  #define TEST_TS_CONT		(1 << 27)
> > +#define TEST_BO_TOOBIG		(1 << 28)
> >  
> >  #define EVENT_FLIP		(1 << 0)
> >  #define EVENT_VBLANK		(1 << 1)
> > @@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
> >  	}
> >  }
> >  
> > +static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
> > +				   bool tiled, uint32_t *gem_handle_ret,
> > +				   unsigned *size_ret, unsigned *stride_ret)
> > +{
> > +	uint32_t gem_handle;
> > +	int size, ret = 0;
> > +	unsigned stride;
> > +
> > +	if (tiled) {
> > +		int v;
> > +
> > +		v = width * bpp / 8;
> > +		for (stride = 512; stride < v; stride *= 2)
> > +			;
> > +
> > +		v = stride * height;
> > +		for (size = 1024*1024; size < v; size *= 2)
> > +			;
> > +	} else {
> > +		/* Scan-out has a 64 byte alignment restriction */
> > +		stride = (width * (bpp / 8) + 63) & ~63;
> > +		size = stride * height;
> > +	}
> > +
> > +	/* 256 MB is usually the maximum mappable aperture,
> > +	 * (make it 4x times that to ensure failure) */
> > +	gem_handle = gem_create(fd, 4*256*1024*1024);
> > +
> > +	if (tiled)
> > +		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
> > +
> > +	*stride_ret = stride;
> > +	*size_ret = size;
> > +	*gem_handle_ret = gem_handle;
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
> > +					     uint32_t format, bool tiled,
> > +					     struct igt_fb *fb)
> > +{
> > +	uint32_t handles[4];
> > +	uint32_t pitches[4];
> > +	uint32_t offsets[4];
> > +	uint32_t fb_id;
> > +	int bpp;
> > +	int ret;
> > +
> > +	memset(fb, 0, sizeof(*fb));
> > +
> > +	bpp = igt_drm_format_to_bpp(format);
> > +	ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
> > +			&fb->gem_handle, &fb->size, &fb->stride);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	memset(handles, 0, sizeof(handles));
> > +	handles[0] = fb->gem_handle;
> > +	memset(pitches, 0, sizeof(pitches));
> > +	pitches[0] = fb->stride;
> > +	memset(offsets, 0, sizeof(offsets));
> > +	if (drmModeAddFB2(fd, width, height, format, handles, pitches,
> > +			  offsets, &fb_id, 0) < 0) {
> > +		gem_close(fd, fb->gem_handle);
> > +
> > +		return 0;
> > +	}
> > +
> > +	fb->width = width;
> > +	fb->height = height;
> > +	fb->tiling = tiled;
> > +	fb->drm_format = format;
> > +	fb->fb_id = fb_id;
> > +
> > +	return fb_id;
> > +}
> 
> Could we avoid a bit of code duplication with something like this?
> 
> create_bo_for_fb(..., bo_size)
> {
> 	...
> 	if (bo_size == 0)
> 		bo_size = size;
> 	gem_handle = gem_create(fd, bo_size);
> 	...
> }
> igt_create_fb_with_bo_size(xxx, bo_size)
> {
> 	...
> 	create_bo_for_fb(..., bo_size);
> 	...
> }
> igt_create_fb(xxx)
> {
> 	return igt_create_fb_with_bo_size(xxx, 0);
> }

Oh and that gives me an idea for another test: try to create fb with
bo_size < size and check that we get back some kind of error. Assuming
we don't have such a test already.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, May 16, 2014 1:14 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too big
> for its own good
> 
> On Fri, May 16, 2014 at 03:04:21PM +0300, Ville Syrjälä wrote:
> > On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.mateo@intel.com wrote:
> > > From: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > This is a "review by igt test" for a bug located in
> > > i915_gem_object_pin_to_display_plane and fixed by:
> > >
> > > commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
> > > Author: Oscar Mateo <oscar.mateo@intel.com>
> > > Date:   Fri May 16 11:23:12 2014 +0100
> > >
> > >     drm/i915: Gracefully handle obj not bound to GGTT in
> > > is_pin_display
> > >
> > >     Otherwise, we do a NULL pointer dereference.
> > >
> > >     I've seen this happen while handling an error in
> > >     i915_gem_object_pin_to_display_plane():
> > >
> > >     If i915_gem_object_set_cache_level() fails, we call is_pin_display()
> > >     to handle the error. At this point, the object is still not pinned
> > >     to GGTT and maybe not even bound, so we have to check before we
> > >     dereference its GGTT vma.
> > >
> > >     v2: Chris Wilson says restoring the old value is easier, but that
> > >     is_pin_display is useful as a theory of operation. Take the solomonic
> > >     decision: at least this way is_pin_display is a little more robust
> > >     (until Chris can kill it off).
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > ---
> > >  tests/kms_flip.c | 102
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 97 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c index
> > > bb4f71d..7296122 100644
> > > --- a/tests/kms_flip.c
> > > +++ b/tests/kms_flip.c
> > > @@ -74,6 +74,7 @@
> > >  #define TEST_RPM		(1 << 25)
> > >  #define TEST_SUSPEND		(1 << 26)
> > >  #define TEST_TS_CONT		(1 << 27)
> > > +#define TEST_BO_TOOBIG		(1 << 28)
> > >
> > >  #define EVENT_FLIP		(1 << 0)
> > >  #define EVENT_VBLANK		(1 << 1)
> > > @@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output
> *o)
> > >  	}
> > >  }
> > >
> > > +static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
> > > +				   bool tiled, uint32_t *gem_handle_ret,
> > > +				   unsigned *size_ret, unsigned *stride_ret) {
> > > +	uint32_t gem_handle;
> > > +	int size, ret = 0;
> > > +	unsigned stride;
> > > +
> > > +	if (tiled) {
> > > +		int v;
> > > +
> > > +		v = width * bpp / 8;
> > > +		for (stride = 512; stride < v; stride *= 2)
> > > +			;
> > > +
> > > +		v = stride * height;
> > > +		for (size = 1024*1024; size < v; size *= 2)
> > > +			;
> > > +	} else {
> > > +		/* Scan-out has a 64 byte alignment restriction */
> > > +		stride = (width * (bpp / 8) + 63) & ~63;
> > > +		size = stride * height;
> > > +	}
> > > +
> > > +	/* 256 MB is usually the maximum mappable aperture,
> > > +	 * (make it 4x times that to ensure failure) */
> > > +	gem_handle = gem_create(fd, 4*256*1024*1024);
> > > +
> > > +	if (tiled)
> > > +		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
> > > +
> > > +	*stride_ret = stride;
> > > +	*size_ret = size;
> > > +	*gem_handle_ret = gem_handle;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
> > > +					     uint32_t format, bool tiled,
> > > +					     struct igt_fb *fb)
> > > +{
> > > +	uint32_t handles[4];
> > > +	uint32_t pitches[4];
> > > +	uint32_t offsets[4];
> > > +	uint32_t fb_id;
> > > +	int bpp;
> > > +	int ret;
> > > +
> > > +	memset(fb, 0, sizeof(*fb));
> > > +
> > > +	bpp = igt_drm_format_to_bpp(format);
> > > +	ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
> > > +			&fb->gem_handle, &fb->size, &fb->stride);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	memset(handles, 0, sizeof(handles));
> > > +	handles[0] = fb->gem_handle;
> > > +	memset(pitches, 0, sizeof(pitches));
> > > +	pitches[0] = fb->stride;
> > > +	memset(offsets, 0, sizeof(offsets));
> > > +	if (drmModeAddFB2(fd, width, height, format, handles, pitches,
> > > +			  offsets, &fb_id, 0) < 0) {
> > > +		gem_close(fd, fb->gem_handle);
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	fb->width = width;
> > > +	fb->height = height;
> > > +	fb->tiling = tiled;
> > > +	fb->drm_format = format;
> > > +	fb->fb_id = fb_id;
> > > +
> > > +	return fb_id;
> > > +}
> >
> > Could we avoid a bit of code duplication with something like this?
> >
> > create_bo_for_fb(..., bo_size)
> > {
> > 	...
> > 	if (bo_size == 0)
> > 		bo_size = size;
> > 	gem_handle = gem_create(fd, bo_size);
> > 	...
> > }
> > igt_create_fb_with_bo_size(xxx, bo_size) {
> > 	...
> > 	create_bo_for_fb(..., bo_size);
> > 	...
> > }
> > igt_create_fb(xxx)
> > {
> > 	return igt_create_fb_with_bo_size(xxx, 0); }

Sure, I´ll send a new version.

> Oh and that gives me an idea for another test: try to create fb with bo_size <
> size and check that we get back some kind of error. Assuming we don't have
> such a test already.

I think we do: bo-too-small in kms_addfb.

[PATCH 1/2] lib/igt_fb: igt_create_fb_with_bo_size

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Useful for testing bigger/smaller fb-wrapped buffer objects.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 lib/igt_fb.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 lib/igt_fb.h |  3 +++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 2149fcd..f43af93 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -76,7 +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,
 			    bool tiled, uint32_t *gem_handle_ret,
-			    unsigned *size_ret, unsigned *stride_ret)
+			    unsigned *size_ret, unsigned *stride_ret,
+			    unsigned bo_size)
 {
 	uint32_t gem_handle;
 	int size, ret = 0;
@@ -106,7 +107,9 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
 		size = stride * height;
 	}
 
-	gem_handle = gem_create(fd, size);
+	if (bo_size == 0)
+		bo_size = size;
+	gem_handle = gem_create(fd, bo_size);
 
 	if (tiled)
 		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
@@ -376,17 +379,18 @@ void igt_paint_image(cairo_t *cr, const char *filename,
 }
 
 /**
- * igt_create_fb:
+ * igt_create_fb_with_bo_size:
  * @fd: open i915 drm file descriptor
  * @width: width of the framebuffer in pixel
  * @height: height of the framebuffer in pixel
  * @format: drm fourcc pixel format code
  * @tiled: X-tiled or linear framebuffer
  * @fb: pointer to an #igt_fb structure
+ * @bo_size: size of the backing bo (0 for minimum needed 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
- * object. All metadata is stored in @fb.
+ * object of the requested size. All metadata is stored in @fb.
  *
  * The backing storage of the framebuffer is filled with all zeros, i.e. black
  * for rgb pixel formats.
@@ -395,8 +399,9 @@ void igt_paint_image(cairo_t *cr, const char *filename,
  * The kms id of the created framebuffer on success or a negative error code on
  * failure.
  */
-unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
-			   bool tiled, struct igt_fb *fb)
+unsigned int igt_create_fb_with_bo_size(int fd, int width, int height,
+					uint32_t format, bool tiled,
+					struct igt_fb *fb, unsigned bo_size)
 {
 	uint32_t handles[4];
 	uint32_t pitches[4];
@@ -409,7 +414,7 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 
 	bpp = igt_drm_format_to_bpp(format);
 	ret = create_bo_for_fb(fd, width, height, bpp, tiled, &fb->gem_handle,
-			      &fb->size, &fb->stride);
+			      &fb->size, &fb->stride, bo_size);
 	if (ret < 0)
 		return ret;
 
@@ -435,6 +440,32 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 }
 
 /**
+ * igt_create_fb:
+ * @fd: open i915 drm file descriptor
+ * @width: width of the framebuffer in pixel
+ * @height: height of the framebuffer in pixel
+ * @format: drm fourcc pixel format code
+ * @tiled: X-tiled or linear framebuffer
+ * @fb: pointer to an #igt_fb structure
+ *
+ * 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
+ * object. All metadata is stored in @fb.
+ *
+ * The backing storage of the framebuffer is filled with all zeros, i.e. black
+ * for rgb pixel formats.
+ *
+ * Returns:
+ * The kms id of the created framebuffer on success or a negative error code on
+ * failure.
+ */
+unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
+			   bool tiled, struct igt_fb *fb)
+{
+	return igt_create_fb_with_bo_size(fd, width, height, format, tiled, fb, 0);
+}
+
+/**
  * igt_create_color_fb:
  * @fd: open i915 drm file descriptor
  * @width: width of the framebuffer in pixel
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index e8bb2a8..c6558bf 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -62,6 +62,9 @@ enum igt_text_align {
 	align_hcenter	= 0x08,
 };
 
+unsigned int igt_create_fb_with_bo_size(int fd, int width, int height,
+					uint32_t format, bool tiled,
+					struct igt_fb *fb, unsigned bo_size);
 unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 			       bool tiled, struct igt_fb *fb);
 unsigned int igt_create_color_fb(int fd, int width, int height,
-- 
1.9.0

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

[PATCH v2 2/2] tests/kms_flip: test a fb backed by a bo too big/small for its own good

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

This is a "review by igt test" for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:

commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo <oscar.mateo@intel.com>
Date:   Fri May 16 11:23:12 2014 +0100

    drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display

    Otherwise, we do a NULL pointer dereference.

    I've seen this happen while handling an error in
    i915_gem_object_pin_to_display_plane():

    If i915_gem_object_set_cache_level() fails, we call is_pin_display()
    to handle the error. At this point, the object is still not pinned
    to GGTT and maybe not even bound, so we have to check before we
    dereference its GGTT vma.

    v2: Chris Wilson says restoring the old value is easier, but that
    is_pin_display is useful as a theory of operation. Take the solomonic
    decision: at least this way is_pin_display is a little more robust
    (until Chris can kill it off).

v2: Avoid code duplication by using igt_create_fb_with_bo_size() as
requested by Ville Syrjälä (original author of the "too big" test idea).

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 tests/kms_flip.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..f2ec9ef 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
 #define TEST_RPM		(1 << 25)
 #define TEST_SUSPEND		(1 << 26)
 #define TEST_TS_CONT		(1 << 27)
+#define TEST_BO_TOOBIG		(1 << 28)
 
 #define EVENT_FLIP		(1 << 0)
 #define EVENT_VBLANK		(1 << 1)
@@ -1213,6 +1214,7 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 {
 	char test_name[128];
 	unsigned elapsed;
+	unsigned bo_size = 0;
 	bool tiled;
 	int i;
 
@@ -1249,12 +1251,17 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	if (o->flags & TEST_FENCE_STRESS)
 		tiled = true;
 
+	/* 256 MB is usually the maximum mappable aperture,
+	 * (make it 4x times that to ensure failure) */
+	if (o->flags & TEST_BO_TOOBIG)
+		bo_size = 4*256*1024*1024;
+
 	o->fb_ids[0] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
 					 igt_bpp_depth_to_drm_format(o->bpp, o->depth),
 					 tiled, &o->fb_info[0]);
-	o->fb_ids[1] = igt_create_fb(drm_fd, o->fb_width, o->fb_height,
+	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),
-					 tiled, &o->fb_info[1]);
+					 tiled, &o->fb_info[1], bo_size);
 	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),
 					 true, &o->fb_info[2]);
@@ -1264,7 +1271,8 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 		igt_require(o->fb_ids[2]);
 
 	paint_flip_mode(&o->fb_info[0], false);
-	paint_flip_mode(&o->fb_info[1], true);
+	if (!(o->flags & TEST_BO_TOOBIG))
+		paint_flip_mode(&o->fb_info[1], true);
 	if (o->fb_ids[2])
 		paint_flip_mode(&o->fb_info[2], true);
 
@@ -1288,10 +1296,15 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	if (o->flags & TEST_CHECK_TS)
 		sleep(1);
 
-	igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
+	if (o->flags & TEST_BO_TOOBIG) {
+		igt_assert(do_page_flip(o, o->fb_ids[1], true) == -E2BIG);
+		goto out;
+	} else
+		igt_assert(do_page_flip(o, o->fb_ids[1], true) == 0);
 	wait_for_events(o);
 
 	o->current_fb_id = 1;
+
 	if (o->flags & TEST_FLIP)
 		o->flip_state.seq_step = 1;
 	else
@@ -1543,6 +1556,7 @@ int main(int argc, char **argv)
 		{ 10, TEST_VBLANK | TEST_DPMS | TEST_SUSPEND | TEST_TS_CONT, "dpms-vs-suspend" },
 		{ 0, TEST_VBLANK | TEST_MODESET | TEST_RPM | TEST_TS_CONT, "modeset-vs-rpm" },
 		{ 0, TEST_VBLANK | TEST_MODESET | TEST_SUSPEND | TEST_TS_CONT, "modeset-vs-suspend" },
+		{ 0, TEST_BO_TOOBIG | TEST_NO_2X_OUTPUT, "bo-too-big" },
 	};
 	int i;
 
-- 
1.9.0

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

Re: [PATCH v2 2/2] tests/kms_flip: test a fb backed by a bo too big/small for its own good

[Ville Syrjälä]

On Fri, May 16, 2014 at 02:07:12PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is a "review by igt test" for a bug located in
> i915_gem_object_pin_to_display_plane and fixed by:

Thanks. Both patches pushed.

-- 
Ville Syrjälä
Intel OTC