Re: [igt-dev] [PATCH i-g-t] igt/kms_addfb_basic: Exercise legacy interface to addfb2

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 05, 2018 at 11:05:22AM +0100, Chris Wilson wrote:
> The legacy interface passes in a single handle, and instead of providing
> the pixel_format fourcc, passes in a bpp/depth combination that the
> kernel translates into a fourcc. If that is an illegal combination, the
> kernel should be reporting EINVAL rather than pass an unknown
> framebuffer to the drivers.
> 
> As the number of possible permutations of bpp/depth (both are strictly
> u32 parameters) is huge, we simply fuzz the interface for 1s.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tests/kms_addfb_basic.c | 103 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index 7d8852f02..b1b143f12 100644
> --- a/tests/kms_addfb_basic.c
> +++ b/tests/kms_addfb_basic.c
> @@ -38,9 +38,35 @@
>  #include "drm.h"
>  #include "drm_fourcc.h"
>  
> +#include "igt_rand.h"
> +
>  uint32_t gem_bo;
>  uint32_t gem_bo_small;
>  
> +static int legacy_addfb(int fd, struct drm_mode_fb_cmd *arg)
> +{
> +	int err;
> +
> +	err = 0;
> +	if (igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB, arg))
> +		err = -errno;
> +
> +	errno = 0;
> +	return err;
> +}
> +
> +static int rmfb(int fd, uint32_t id)
> +{
> +	int err;
> +
> +	err = 0;
> +	if (igt_ioctl(fd, DRM_IOCTL_MODE_RMFB, &id))
> +		err = -errno;
> +
> +	errno = 0;
> +	return err;
> +}
> +
>  static void invalid_tests(int fd)
>  {
>  	struct local_drm_mode_fb_cmd2 f = {};
> @@ -113,6 +139,83 @@ static void invalid_tests(int fd)
>  		igt_assert(f.modifier[0] == 0);
>  	}
>  
> +	igt_subtest("legacy-format") {
> +		struct {
> +			/* drm_mode_legacy_fb_format() */
> +			int bpp, depth;
> +			int expect;
> +		} known_formats[] = {
> +			{ 8, 0 },   /* palette */

Apparently 8/8
> +			{ 16, 15 }, /* x1r5g5b5 */
> +			{ 16, 16 }, /* r5g6b5 */
> +			{ 24, 24 }, /* r8g8b8 */
> +			{ 32, 24 }, /* x8r8g8b8 */
> +			{ 32, 30 }, /* x2r10g10b10 */
> +			{ 32, 32 }, /* a2r10g10b10 */
> +		};
> +		struct drm_mode_fb_cmd arg = {
> +			.width = f.width,
> +			.height = f.height,
> +			.pitch = f.pitches[0],
> +			.handle = f.handles[0],
> +		};
> +		uint32_t prng = 0x12345678;
> +
> +		/*
> +		 * First confirm the kernel recognises our known_formats;
> +		 * some may be invalid for different devices.
> +		 */
> +		for (int i = 0; i < ARRAY_SIZE(known_formats); i++) {
> +			arg.bpp = known_formats[i].bpp;
> +			arg.depth = known_formats[i].depth;
> +			known_formats[i].expect = legacy_addfb(fd, &arg);
> +			igt_debug("(bpp:%d, depth:%d) -> expect:%d\n",
> +				  arg.bpp, arg.depth, known_formats[i].expect);
> +			if (arg.fb_id) {
> +				igt_assert_eq(rmfb(fd, arg.fb_id), 0);
> +				arg.fb_id = 0;
> +			}
> +		}
> +
> +		igt_until_timeout(1) {
> +			int expect = -EINVAL;
> +			int err;
> +
> +			arg.bpp = hars_petruska_f54_1_random(&prng);
> +			arg.depth = hars_petruska_f54_1_random(&prng);
> +			for (int start = 0, end = ARRAY_SIZE(known_formats);
> +			     start < end; ) {

The b-search is maybe a bit overkill for an array with 7 entries :-) I'd
simplify to a brute-force match.

> +				int mid = start + (end - start) / 2;
> +				typeof(*known_formats) *tbl = &known_formats[mid];
> +
> +				if (arg.bpp < tbl->bpp) {
> +					end = mid;
> +				} else if (arg.bpp > tbl->bpp) {
> +					start = mid + 1;
> +				} else {
> +					if (arg.depth < tbl->depth) {
> +						end = mid;
> +					} else if (arg.depth > tbl->depth) {
> +						start = mid + 1;
> +					} else {
> +						expect = tbl->expect;
> +						break;
> +					}
> +				}
> +			}
> +
> +
> +			err = legacy_addfb(fd, &arg);
> +			igt_assert_f(err == expect,
> +				     "Expected %d with (bpp:%d, depth:%d), got %d instead\n",
> +				     expect, arg.bpp, arg.depth, err);
> +			if (arg.fb_id) {
> +				igt_assert_eq(rmfb(fd, arg.fb_id), 0);
> +				arg.fb_id = 0;
> +			}
> +		}
> +	}

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	igt_fixture {
>  		gem_close(fd, gem_bo);
>  		gem_close(fd, gem_bo_small);
> -- 
> 2.19.0.rc1
> 

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