Re: [PATCH igt 2/2] kms_frontbuffer_tracking: standardize the used FB sizes

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Sex, 2015-12-04 às 16:28 +0100, Daniel Vetter escreveu:
> On Wed, Dec 02, 2015 at 10:47:01AM -0200, Paulo Zanoni wrote:
> > We want to make sure that both tiled and untiled 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 we won't execute the
> > error
> > path that checks for buffer sizes.
> > 
> > v2: Use the new igt_calc_fb_size() instead of implementing our own
> > size calculation (Daniel).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++
> > --------------
> >  1 file changed, 34 insertions(+), 17 deletions(-)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index 81703ec..3db95d2 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void)
> >  	return true;
> >  }
> >  
> > +static int format_get_bpp(uint32_t format)
> 
> Ah, missed one: igt_drm_format_to_bpp please, and if it doesn't cover
> them all we
> need to fix that asap.

I'm not sure if this is a good idea. Not every DRM format has a
matching cairo format, and it seems the whole igt_fb code is built
based on this assumption. On a quick look, it really seems that both
kms_render.c and kms_atomic.c will break if I add ARGB2101010 with a
matching CAIRO_INVALID (since they call igt_get_all_formats() and use
cairo).

I know, you can question the use of ARGB2101010 by
kms_frontbuffer_tracking (we don't use it anymore, the format is there
as an artifact of an older attempt when I initially added support for
multiple formats), but that doesn't solve the bigger problem that we
can't easily expand igt_drm_format_to_bpp().

If you still insist, the big plan should be to make sure that both
igt_fb and the libs can properly handle the cases where a DRM format
doesn't have a matching cairo format, but I don't want to block my
current tasks on this. So I'd vote to merge my patches as-is for now.

What do you think?

> -Daniel
> 
> > +{
> > +	switch (format) {
> > +	case DRM_FORMAT_RGB565:
> > +		return 16;
> > +	case DRM_FORMAT_XRGB8888:
> > +	case DRM_FORMAT_ARGB8888:
> > +	case DRM_FORMAT_ARGB2101010:
> > +	case DRM_FORMAT_XRGB2101010:
> > +		return 32;
> > +	default:
> > +		igt_assert(false);
> > +	}
> > +}
> > +
> >  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 size, stride;
> > +	int bpp;
> > +	uint64_t tiling_for_size;
> >  
> >  	switch (pformat) {
> >  	case FORMAT_RGB888:
> > @@ -512,7 +530,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/height/format to have
> > +	 * the same size regardless of tiling since we want to
> > properly exercise
> > +	 * the Kernel's specific tiling-checking code paths
> > without accidentally
> > +	 * hitting size-checking ones first. */
> > +	bpp = format_get_bpp(format);
> > +	if (plane == PLANE_CUR)
> > +		tiling_for_size = LOCAL_DRM_FORMAT_MOD_NONE;
> > +	else
> > +		tiling_for_size = LOCAL_I915_FORMAT_MOD_X_TILED;
> > +
> > +	igt_calc_fb_size(drm.fd, width, height, bpp,
> > tiling_for_size, &size,
> > +			 &stride);
> > +
> > +	igt_create_fb_with_bo_size(drm.fd, width, height, format,
> > tiling, fb,
> > +				   size, stride);
> >  }
> >  
> >  static uint32_t pick_color(struct igt_fb *fb, enum color ecolor)
> > @@ -1094,21 +1126,6 @@ static void *busy_thread_func(void *data)
> >  	pthread_exit(0);
> >  }
> >  
> > -static int fb_get_bpp(struct igt_fb *fb)
> > -{
> > -	switch (fb->drm_format) {
> > -	case DRM_FORMAT_RGB565:
> > -		return 16;
> > -	case DRM_FORMAT_XRGB8888:
> > -	case DRM_FORMAT_ARGB8888:
> > -	case DRM_FORMAT_ARGB2101010:
> > -	case DRM_FORMAT_XRGB2101010:
> > -		return 32;
> > -	default:
> > -		igt_assert(false);
> > -	}
> > -}
> > -
> >  static void start_busy_thread(struct igt_fb *fb)
> >  {
> >  	int rc;
> > @@ -1121,7 +1138,7 @@ static void start_busy_thread(struct igt_fb
> > *fb)
> >  	busy_thread.width = fb->width;
> >  	busy_thread.height = fb->height;
> >  	busy_thread.color = pick_color(fb, COLOR_PRIM_BG);
> > -	busy_thread.bpp = fb_get_bpp(fb);
> > +	busy_thread.bpp = format_get_bpp(fb->drm_format);
> >  
> >  	rc = pthread_create(&busy_thread.thread, NULL,
> > busy_thread_func, NULL);
> >  	igt_assert_eq(rc, 0);
> > -- 
> > 2.6.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx