[Intel-gfx] [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 at linux.intel.com]
> Sent: Friday, May 16, 2014 1:14 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at 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 at intel.com wrote:
> > > From: Oscar Mateo <oscar.mateo at 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 at 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 at 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.