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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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