diff mbox

[i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw

Message ID 1452872548-4959-1-git-send-email-devon.davies@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

devon.davies@intel.com Jan. 15, 2016, 3:42 p.m. UTC
From: Devon Davies <devon.davies@intel.com>

tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE
    Each call to the function drmModeSetPlane now has an addtional NULL in the
    arguments if DRM_PRIMARY_DISABLE is set.

tests/Android.mk: Allow the above tests to be built without Cairo
    Simply removed them from the tests the be skipped.

libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE
    I had to define ffs as __builtin_fss due to compiler issues.
    Each call to the function drmModeSetPlane now has an addtional NULL in the
    arguments if DRM_PRIMARY_DISABLE is set.

libs/igt_fb.c: Will now build some functions without Cairo
    Functions which aren't used by the framebuffer compression tests are
    now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&
    ANDROID_HAS_CAIRO

libs/Android.mk
    igt_fb and igt_kms are no longer ignored if we don't have Cairo.

The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary
dependance on the Cairo graphics engine.
Also, drmModeSetPlane may have an additional argument if DRM_PRIMARY_DISABLE
is set (as it was for me), I have fixed that issue.

Signed-off-by: Devon Davies <devon.davies@intel.com>
---
 lib/Android.mk                   |  4 --
 lib/igt_fb.c                     | 26 ++++++++++++-
 lib/igt_kms.c                    | 15 ++++++--
 tests/Android.mk                 |  5 +++
 tests/kms_fbc_crc.c              | 20 ++++++----
 tests/kms_frontbuffer_tracking.c | 79 +++++++++++++++++++++++++++++++++-------
 6 files changed, 119 insertions(+), 30 deletions(-)

Comments

Zanoni, Paulo R Jan. 20, 2016, 7:34 p.m. UTC | #1
Hi

In our IGT/Kernel culture, patch commit titles (here, presented as the
email subject) are usually small (under 70-80 chars) providing a quick
summary of what the change does.

Also, we try to make each patch contain a single logical and complete
change. So, for example, one approach you could have taken here would
be:
 - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
 - patch 2: solve the ffs() problem
 - patch 3: solve the drmModeSetPlane() problem
 - patch 4: change kms_fbc_crc to not need cairo
 - patch 5: change the build system so it compiles tests that now work
on Android

Following is another text explaining this pattern:
http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
hes#n201


Now, regarding the Android vs Cairo macros: I know this has been
discussed in the mailing list a few times but I didn't follow it
closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like
Daniel or others to comment on this. Do we want the ifdef dance in
igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo
parts, and igt_cairo for the cairo parts? The igt_fb library is
definitely useful without cairo, so this would make sense to me. But
perhaps we want to just keep everything as-is since it's possible to
have cairo on Android?

There are some other comments inline. Please see below.


Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> From: Devon Davies <devon.davies@intel.com>

> 

> tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE

>     Each call to the function drmModeSetPlane now has an addtional

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> tests/Android.mk: Allow the above tests to be built without Cairo

>     Simply removed them from the tests the be skipped.

> 

> libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE

>     I had to define ffs as __builtin_fss due to compiler issues.

>     Each call to the function drmModeSetPlane now has an addtional

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> libs/igt_fb.c: Will now build some functions without Cairo

>     Functions which aren't used by the framebuffer compression tests

> are

>     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&

>     ANDROID_HAS_CAIRO

> 

> libs/Android.mk

>     igt_fb and igt_kms are no longer ignored if we don't have Cairo.

> 

> The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary

> dependance on the Cairo graphics engine.

> Also, drmModeSetPlane may have an additional argument if

> DRM_PRIMARY_DISABLE

> is set (as it was for me), I have fixed that issue.

> 

> Signed-off-by: Devon Davies <devon.davies@intel.com>

> ---

>  lib/Android.mk                   |  4 --

>  lib/igt_fb.c                     | 26 ++++++++++++-

>  lib/igt_kms.c                    | 15 ++++++--

>  tests/Android.mk                 |  5 +++

>  tests/kms_fbc_crc.c              | 20 ++++++----

>  tests/kms_frontbuffer_tracking.c | 79

> +++++++++++++++++++++++++++++++++-------

>  6 files changed, 119 insertions(+), 30 deletions(-)

> 

> diff --git a/lib/Android.mk b/lib/Android.mk

> index badec1e..bbdb051 100644

> --- a/lib/Android.mk

> +++ b/lib/Android.mk

> @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")

>      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-

> 1.12.16/src

>      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"

> -DIGT_SRCDIR=\".\"

>  else

> -skip_lib_list := \

> -    igt_kms.c \

> -    igt_kms.h \

> -    igt_fb.c

>      -DANDROID_HAS_CAIRO=0

>  endif

>  

> diff --git a/lib/igt_fb.c b/lib/igt_fb.c

> index c985824..5acdaa7 100644

> --- a/lib/igt_fb.c

> +++ b/lib/igt_fb.c

> @@ -33,6 +33,7 @@

>  #include "igt_fb.h"

>  #include "ioctl_wrappers.h"

>  

> +


Please don't add random lines to files.


>  /**

>   * SECTION:igt_fb

>   * @short_description: Framebuffer handling and drawing library

> @@ -52,11 +53,23 @@

>   */

>  

>  /* drm fourcc/cairo format maps */

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  #define DF(did, cid, _bpp, _depth)	\

>  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth

> }

> +

> +#else

> +

> +#define DF(did, cid, _bpp, _depth)	\

> +	{ DRM_FORMAT_##did, # did, _bpp, _depth }

> +

> +#endif

> +

>  static struct format_desc_struct {

>  	uint32_t drm_id;

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_format_t cairo_id;

> +#endif

>  	const char *name;

>  	int bpp;

>  	int depth;

> @@ -72,7 +85,6 @@ static struct format_desc_struct {

>  #define for_each_format(f)	\

>  	for (f = format_desc; f - format_desc <

> ARRAY_SIZE(format_desc); f++)

>  

> -


Please don't remove random lines from files.


>  /* helpers to create nice-looking framebuffers */

>  static int create_bo_for_fb(int fd, int width, int height, int bpp,

>  			    uint64_t tiling, unsigned bo_size,

> @@ -125,6 +137,8 @@ static int create_bo_for_fb(int fd, int width,

> int height, int bpp,

>  	return ret;

>  }

>  

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  /**

>   * igt_paint_color:

>   * @cr: cairo drawing context

> @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char

> *filename,

>  

>  	fclose(f);

>  }

> +#endif

>  

>  /**

>   * igt_create_fb_with_bo_size:

> @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int

> height, uint32_t format,

>  	return igt_create_fb_with_bo_size(fd, width, height, format,

> tiling, fb,

>  					  0, 0);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_create_color_fb:

> @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb

> *fb, const char *filename)

>  

>  	igt_assert(status == CAIRO_STATUS_SUCCESS);

>  }

> +#endif

>  

>  /**

>   * igt_remove_fb:

> @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb

> *fb, const char *filename)

>   */

>  void igt_remove_fb(int fd, struct igt_fb *fb)

>  {

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_surface_destroy(fb->cairo_surface);

> +#endif

>  	do_or_die(drmModeRmFB(fd, fb->fb_id));

>  	gem_close(fd, fb->gem_handle);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_bpp_depth_to_drm_format:

> @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp,

> int depth)

>  		     depth);

>  }

>  

> +#endif

> +

>  /**

>   * igt_drm_format_to_bpp:

>   * @drm_format: drm fourcc pixel format code

> @@ -1062,6 +1084,7 @@ const char *igt_format_str(uint32_t drm_format)

>  

>  	return "invalid";

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_get_all_formats:

> @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t

> **formats, int *format_count)

>  	*formats = drm_formats;

>  	*format_count = ARRAY_SIZE(format_desc);

>  }

> +#endif

> diff --git a/lib/igt_kms.c b/lib/igt_kms.c

> index 497118a..7b682cb 100644

> --- a/lib/igt_kms.c

> +++ b/lib/igt_kms.c

> @@ -49,6 +49,8 @@

>  #include "intel_chipset.h"

>  #include "igt_debugfs.h"

>  

> +#define ffs __builtin_ffs


A define like this can be dangerous. Shouldn't we just fix all the
callers, so code readers know exactly which function is being run?

On the other hand, it seems that this change will restrict us to gcc-
only. So maybe we could just define ffs as __builtin_ffs if ANDROID is
defined?


> +

>  /* list of connectors that need resetting on exit */

>  #define MAX_CONNECTORS 32

>  static char *forced_connectors[MAX_CONNECTORS + 1];

> @@ -1354,8 +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t

> *plane,

>  				      IGT_FIXED(0,0), /* src_x */

>  				      IGT_FIXED(0,0), /* src_y */

>  				      IGT_FIXED(0,0), /* src_w */

> -				      IGT_FIXED(0,0) /* src_h */);

> -

> +				      IGT_FIXED(0,0) /* src_h */

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif


If we're going this way, maybe the best approach would be to add a
wrapper (igt_kms_set_plane?) and make the current callers use it.

On a side note: why is Android different here!?


Thanks,
Paulo

> +					  );

>  		CHECK_RETURN(ret, fail_on_error);

>  	} else if (plane->fb_changed || plane->position_changed ||

>  		plane->size_changed) {

> @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t

> *plane,

>  				      crtc_x, crtc_y,

>  				      crtc_w, crtc_h,

>  				      src_x, src_y,

> -				      src_w, src_h);

> +				      src_w, src_h

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif

> +					  );

>  

>  		CHECK_RETURN(ret, fail_on_error);

>  	}

> diff --git a/tests/Android.mk b/tests/Android.mk

> index 8457125..eb287a6 100644

> --- a/tests/Android.mk

> +++ b/tests/Android.mk

> @@ -65,6 +65,11 @@ else

>  

>      tmp_list := $(foreach test_name, $(TESTS_progs_M),\

>          $(if $(findstring kms_,$(test_name)),$(test_name)))

> +

> +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo

> +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))

> +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))

> +

>      skip_tests_list += $(tmp_list)

>  

>      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0

> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c

> index 02e95e5..717e891 100644

> --- a/tests/kms_fbc_crc.c

> +++ b/tests/kms_fbc_crc.c

> @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool

> tiled, struct igt_fb *fbs)

>  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :

>  				  LOCAL_DRM_FORMAT_MOD_NONE;

>  

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.0, 0.0, 0.0, &fbs[0]);

> -	igt_assert(rc);

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.1, 0.1, 0.1, &fbs[1]);

> -	igt_assert(rc);

> +	unsigned int fb_id;

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[0]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[1]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);

> +

>  }

>  

>  /* Since we want to be really safe that the CRCs are actually what

> we really

> diff --git a/tests/kms_frontbuffer_tracking.c

> b/tests/kms_frontbuffer_tracking.c

> index e7acc7c..f8b9eca 100644

> --- a/tests/kms_frontbuffer_tracking.c

> +++ b/tests/kms_frontbuffer_tracking.c

> @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)

>  

>  	for (i = 0; i < drm.plane_res->count_planes; i++) {

>  		rc = drmModeSetPlane(drm.fd, drm.plane_res-

> >planes[i], 0, 0, 0,

> -				     0, 0, 0, 0, 0, 0, 0, 0);

> +				     0, 0, 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +				     , NULL

> +#endif

> +			     );

>  		igt_assert_eq(rc, 0);

>  	}

>  }

> @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct

> test_mode *t,

>  			     params->sprite.fb->fb_id, 0, 0, 0,

>  			     params->sprite.w, params->sprite.h,

>  			     0, 0, params->sprite.w << 16,

> -			     params->sprite.h << 16);

> +			     params->sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	do_assertions(ASSERT_NO_ACTION_CHANGE);

> @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct

> modeset_params *params)

>  			     params->mode->hdisplay,

>  			     params->mode->vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  }

>  

> @@ -2406,7 +2418,11 @@ static void move_subtest(const struct

> test_mode *t)

>  					     params->sprite.fb-

> >fb_id, 0,

>  					     rect.x, rect.y, rect.w,

>  					     rect.h, 0, 0, rect.w <<

> 16,

> -					     rect.h << 16);

> +					     rect.h << 16

> +#if DRM_PRIMARY_DISABLE

> +					     , NULL

> +#endif

> +			     );

>  			igt_assert_eq(rc, 0);

>  			break;

>  		default:

> @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct

> test_mode *t)

>  				break;

>  			case PLANE_SPR:

>  				rc = drmModeSetPlane(drm.fd, params-

> >sprite_id,

> -						     0, 0, 0, 0, 0,

> 0, 0, 0, 0,

> -						     0, 0);

> +						     0, 0, 0, 0, 0,

> 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct

> test_mode *t)

>  						     params-

> >sprite.h, 0,

>  						     0,

>  						     params-

> >sprite.w << 16,

> -						     params-

> >sprite.h << 16);

> +						     params-

> >sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const

> struct test_mode *t)

>  			     fullscreen_fb.fb_id, 0, 0, 0,

> fullscreen_fb.width,

>  			     fullscreen_fb.height, 0, 0,

>  			     fullscreen_fb.width << 16,

> -			     fullscreen_fb.height << 16);

> +			     fullscreen_fb.height << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  	update_wanted_crc(t, &pattern->crcs[t->format][0]);

>  

> @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const

> struct test_mode *t)

>  	do_assertions(assertions);

>  

>  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0,

> 0, 0, 0, 0,

> -			     0, 0, 0);

> +			     0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	if (t->screen == SCREEN_PRIM)

> @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct

> test_mode *t)

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct

> test_mode *t)

>  			     params->mode->vdisplay / 2,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct

> test_mode *t)

>  			     (params->fb.x + params->fb.w / 2) <<

> 16,

>  			     (params->fb.y + params->fb.h / 2) <<

> 16,

>  			     (params->fb.w / 4) << 16,

> -			     (params->fb.h / 4) << 16);

> +			     (params->fb.h / 4) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(0);

>
devon.davies@intel.com Jan. 22, 2016, 3:18 p.m. UTC | #2
Hi,

Thanks for the feedback.

I'll update the commit title. I will also separate this patch into 5 patches.

For now I want to keep the "ifdef dance" in igt_fbc.c. If we want to split the file up, that should be a new commit/patch.
(I think we should get the fbc tests building on Android first before we start the splitting up igt_fbc.c, since we want to start testing asap.)

I looked to see if I had removed/added any extra lines... some must have missed me, I will revert them.

The ffs define: I can see that being dangerous.
Should I do
#if defined(__GNUC__)
#define ffs __builtin_ffs
#endif
?
In my opinion that is cleaner than having a ifdef around the actual ffs function call. It's also easier to add to (in case someone in the future uses a different compiler).

igt_drm_plane_commit:
I don't know why Android is different, according to git blame, it's been this way since 13th June 2014.
I'll ask why separately to this thread.
As for making a wrapper. I think that should be part of another commit.
(Again to be done after this, since we want to begin testing asap.)

Thanks,
Devon.

-----Original Message-----
From: Zanoni, Paulo R 

Sent: Wednesday, January 20, 2016 7:35 PM
To: intel-gfx@lists.freedesktop.org; Davies, Devon
Cc: Gore, Tim; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw

Hi

In our IGT/Kernel culture, patch commit titles (here, presented as the email subject) are usually small (under 70-80 chars) providing a quick summary of what the change does.

Also, we try to make each patch contain a single logical and complete change. So, for example, one approach you could have taken here would
be:
 - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
 - patch 2: solve the ffs() problem
 - patch 3: solve the drmModeSetPlane() problem
 - patch 4: change kms_fbc_crc to not need cairo
 - patch 5: change the build system so it compiles tests that now work on Android

Following is another text explaining this pattern:
http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
hes#n201


Now, regarding the Android vs Cairo macros: I know this has been discussed in the mailing list a few times but I didn't follow it closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like Daniel or others to comment on this. Do we want the ifdef dance in igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo parts, and igt_cairo for the cairo parts? The igt_fb library is definitely useful without cairo, so this would make sense to me. But perhaps we want to just keep everything as-is since it's possible to have cairo on Android?

There are some other comments inline. Please see below.


Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> From: Devon Davies <devon.davies@intel.com>

> 

> tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE

>     Each call to the function drmModeSetPlane now has an addtional 

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> tests/Android.mk: Allow the above tests to be built without Cairo

>     Simply removed them from the tests the be skipped.

> 

> libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE

>     I had to define ffs as __builtin_fss due to compiler issues.

>     Each call to the function drmModeSetPlane now has an addtional 

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> libs/igt_fb.c: Will now build some functions without Cairo

>     Functions which aren't used by the framebuffer compression tests 

> are

>     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&

>     ANDROID_HAS_CAIRO

> 

> libs/Android.mk

>     igt_fb and igt_kms are no longer ignored if we don't have Cairo.

> 

> The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary 

> dependance on the Cairo graphics engine.

> Also, drmModeSetPlane may have an additional argument if 

> DRM_PRIMARY_DISABLE is set (as it was for me), I have fixed that 

> issue.

> 

> Signed-off-by: Devon Davies <devon.davies@intel.com>

> ---

>  lib/Android.mk                   |  4 --

>  lib/igt_fb.c                     | 26 ++++++++++++-

>  lib/igt_kms.c                    | 15 ++++++--

>  tests/Android.mk                 |  5 +++

>  tests/kms_fbc_crc.c              | 20 ++++++----

>  tests/kms_frontbuffer_tracking.c | 79

> +++++++++++++++++++++++++++++++++-------

>  6 files changed, 119 insertions(+), 30 deletions(-)

> 

> diff --git a/lib/Android.mk b/lib/Android.mk index badec1e..bbdb051 

> 100644

> --- a/lib/Android.mk

> +++ b/lib/Android.mk

> @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")

>      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-

> 1.12.16/src

>      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"

> -DIGT_SRCDIR=\".\"

>  else

> -skip_lib_list := \

> -    igt_kms.c \

> -    igt_kms.h \

> -    igt_fb.c

>      -DANDROID_HAS_CAIRO=0

>  endif

>  

> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index c985824..5acdaa7 100644

> --- a/lib/igt_fb.c

> +++ b/lib/igt_fb.c

> @@ -33,6 +33,7 @@

>  #include "igt_fb.h"

>  #include "ioctl_wrappers.h"

>  

> +


Please don't add random lines to files.


>  /**

>   * SECTION:igt_fb

>   * @short_description: Framebuffer handling and drawing library @@ 

> -52,11 +53,23 @@

>   */

>  

>  /* drm fourcc/cairo format maps */

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  #define DF(did, cid, _bpp, _depth)	\

>  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }

> +

> +#else

> +

> +#define DF(did, cid, _bpp, _depth)	\

> +	{ DRM_FORMAT_##did, # did, _bpp, _depth }

> +

> +#endif

> +

>  static struct format_desc_struct {

>  	uint32_t drm_id;

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_format_t cairo_id;

> +#endif

>  	const char *name;

>  	int bpp;

>  	int depth;

> @@ -72,7 +85,6 @@ static struct format_desc_struct {

>  #define for_each_format(f)	\

>  	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); 

> f++)

>  

> -


Please don't remove random lines from files.


>  /* helpers to create nice-looking framebuffers */

>  static int create_bo_for_fb(int fd, int width, int height, int bpp,

>  			    uint64_t tiling, unsigned bo_size, @@ -125,6 +137,8 @@ static 

> int create_bo_for_fb(int fd, int width, int height, int bpp,

>  	return ret;

>  }

>  

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  /**

>   * igt_paint_color:

>   * @cr: cairo drawing context

> @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char 

> *filename,

>  

>  	fclose(f);

>  }

> +#endif

>  

>  /**

>   * igt_create_fb_with_bo_size:

> @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int 

> height, uint32_t format,

>  	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, 

> fb,

>  					  0, 0);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_create_color_fb:

> @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb 

> *fb, const char *filename)

>  

>  	igt_assert(status == CAIRO_STATUS_SUCCESS);

>  }

> +#endif

>  

>  /**

>   * igt_remove_fb:

> @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb 

> *fb, const char *filename)

>   */

>  void igt_remove_fb(int fd, struct igt_fb *fb)

>  {

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_surface_destroy(fb->cairo_surface);

> +#endif

>  	do_or_die(drmModeRmFB(fd, fb->fb_id));

>  	gem_close(fd, fb->gem_handle);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_bpp_depth_to_drm_format:

> @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp, 

> int depth)

>  		     depth);

>  }

>  

> +#endif

> +

>  /**

>   * igt_drm_format_to_bpp:

>   * @drm_format: drm fourcc pixel format code @@ -1062,6 +1084,7 @@ 

> const char *igt_format_str(uint32_t drm_format)

>  

>  	return "invalid";

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_get_all_formats:

> @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t 

> **formats, int *format_count)

>  	*formats = drm_formats;

>  	*format_count = ARRAY_SIZE(format_desc);

>  }

> +#endif

> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..7b682cb 

> 100644

> --- a/lib/igt_kms.c

> +++ b/lib/igt_kms.c

> @@ -49,6 +49,8 @@

>  #include "intel_chipset.h"

>  #include "igt_debugfs.h"

>  

> +#define ffs __builtin_ffs


A define like this can be dangerous. Shouldn't we just fix all the callers, so code readers know exactly which function is being run?

On the other hand, it seems that this change will restrict us to gcc- only. So maybe we could just define ffs as __builtin_ffs if ANDROID is defined?


> +

>  /* list of connectors that need resetting on exit */

>  #define MAX_CONNECTORS 32

>  static char *forced_connectors[MAX_CONNECTORS + 1]; @@ -1354,8 

> +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,

>  				      IGT_FIXED(0,0), /* src_x */

>  				      IGT_FIXED(0,0), /* src_y */

>  				      IGT_FIXED(0,0), /* src_w */

> -				      IGT_FIXED(0,0) /* src_h */);

> -

> +				      IGT_FIXED(0,0) /* src_h */

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif


If we're going this way, maybe the best approach would be to add a wrapper (igt_kms_set_plane?) and make the current callers use it.

On a side note: why is Android different here!?


Thanks,
Paulo

> +					  );

>  		CHECK_RETURN(ret, fail_on_error);

>  	} else if (plane->fb_changed || plane->position_changed ||

>  		plane->size_changed) {

> @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t 

> *plane,

>  				      crtc_x, crtc_y,

>  				      crtc_w, crtc_h,

>  				      src_x, src_y,

> -				      src_w, src_h);

> +				      src_w, src_h

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif

> +					  );

>  

>  		CHECK_RETURN(ret, fail_on_error);

>  	}

> diff --git a/tests/Android.mk b/tests/Android.mk index 

> 8457125..eb287a6 100644

> --- a/tests/Android.mk

> +++ b/tests/Android.mk

> @@ -65,6 +65,11 @@ else

>  

>      tmp_list := $(foreach test_name, $(TESTS_progs_M),\

>          $(if $(findstring kms_,$(test_name)),$(test_name)))

> +

> +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo

> +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))

> +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))

> +

>      skip_tests_list += $(tmp_list)

>  

>      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0 diff --git 

> a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 02e95e5..717e891 

> 100644

> --- a/tests/kms_fbc_crc.c

> +++ b/tests/kms_fbc_crc.c

> @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool tiled, 

> struct igt_fb *fbs)

>  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :

>  				  LOCAL_DRM_FORMAT_MOD_NONE;

>  

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.0, 0.0, 0.0, &fbs[0]);

> -	igt_assert(rc);

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.1, 0.1, 0.1, &fbs[1]);

> -	igt_assert(rc);

> +	unsigned int fb_id;

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[0]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[1]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);

> +

>  }

>  

>  /* Since we want to be really safe that the CRCs are actually what we 

> really diff --git a/tests/kms_frontbuffer_tracking.c

> b/tests/kms_frontbuffer_tracking.c

> index e7acc7c..f8b9eca 100644

> --- a/tests/kms_frontbuffer_tracking.c

> +++ b/tests/kms_frontbuffer_tracking.c

> @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)

>  

>  	for (i = 0; i < drm.plane_res->count_planes; i++) {

>  		rc = drmModeSetPlane(drm.fd, drm.plane_res-

> >planes[i], 0, 0, 0,

> -				     0, 0, 0, 0, 0, 0, 0, 0);

> +				     0, 0, 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +				     , NULL

> +#endif

> +			     );

>  		igt_assert_eq(rc, 0);

>  	}

>  }

> @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct 

> test_mode *t,

>  			     params->sprite.fb->fb_id, 0, 0, 0,

>  			     params->sprite.w, params->sprite.h,

>  			     0, 0, params->sprite.w << 16,

> -			     params->sprite.h << 16);

> +			     params->sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	do_assertions(ASSERT_NO_ACTION_CHANGE);

> @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct 

> modeset_params *params)

>  			     params->mode->hdisplay,

>  			     params->mode->vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  }

>  

> @@ -2406,7 +2418,11 @@ static void move_subtest(const struct test_mode 

> *t)

>  					     params->sprite.fb-

> >fb_id, 0,

>  					     rect.x, rect.y, rect.w,

>  					     rect.h, 0, 0, rect.w <<

> 16,

> -					     rect.h << 16);

> +					     rect.h << 16

> +#if DRM_PRIMARY_DISABLE

> +					     , NULL

> +#endif

> +			     );

>  			igt_assert_eq(rc, 0);

>  			break;

>  		default:

> @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct 

> test_mode *t)

>  				break;

>  			case PLANE_SPR:

>  				rc = drmModeSetPlane(drm.fd, params-

> >sprite_id,

> -						     0, 0, 0, 0, 0,

> 0, 0, 0, 0,

> -						     0, 0);

> +						     0, 0, 0, 0, 0,

> 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct 

> test_mode *t)

>  						     params-

> >sprite.h, 0,

>  						     0,

>  						     params-

> >sprite.w << 16,

> -						     params-

> >sprite.h << 16);

> +						     params-

> >sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const 

> struct test_mode *t)

>  			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,

>  			     fullscreen_fb.height, 0, 0,

>  			     fullscreen_fb.width << 16,

> -			     fullscreen_fb.height << 16);

> +			     fullscreen_fb.height << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  	update_wanted_crc(t, &pattern->crcs[t->format][0]);

>  

> @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const 

> struct test_mode *t)

>  	do_assertions(assertions);

>  

>  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 

> 0,

> -			     0, 0, 0);

> +			     0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	if (t->screen == SCREEN_PRIM)

> @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     params->mode->vdisplay / 2,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     (params->fb.x + params->fb.w / 2) << 16,

>  			     (params->fb.y + params->fb.h / 2) << 16,

>  			     (params->fb.w / 4) << 16,

> -			     (params->fb.h / 4) << 16);

> +			     (params->fb.h / 4) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(0);

>
devon.davies@intel.com Jan. 22, 2016, 5:02 p.m. UTC | #3
Hi,

With regards to the additional NULL argument to drmModeSetPlane on Android.

Apparently, there was a previous attempt to make a wrapper for it.
Would it be possible to upstream that?

Devon


-----Original Message-----
From: Davies, Devon 

Sent: Friday, January 22, 2016 3:18 PM
To: Zanoni, Paulo R; intel-gfx@lists.freedesktop.org
Cc: Gore, Tim; Vetter, Daniel
Subject: RE: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw

Hi,

Thanks for the feedback.

I'll update the commit title. I will also separate this patch into 5 patches.

For now I want to keep the "ifdef dance" in igt_fbc.c. If we want to split the file up, that should be a new commit/patch.
(I think we should get the fbc tests building on Android first before we start the splitting up igt_fbc.c, since we want to start testing asap.)

I looked to see if I had removed/added any extra lines... some must have missed me, I will revert them.

The ffs define: I can see that being dangerous.
Should I do
#if defined(__GNUC__)
#define ffs __builtin_ffs
#endif
?
In my opinion that is cleaner than having a ifdef around the actual ffs function call. It's also easier to add to (in case someone in the future uses a different compiler).

igt_drm_plane_commit:
I don't know why Android is different, according to git blame, it's been this way since 13th June 2014.
I'll ask why separately to this thread.
As for making a wrapper. I think that should be part of another commit.
(Again to be done after this, since we want to begin testing asap.)

Thanks,
Devon.

-----Original Message-----
From: Zanoni, Paulo R

Sent: Wednesday, January 20, 2016 7:35 PM
To: intel-gfx@lists.freedesktop.org; Davies, Devon
Cc: Gore, Tim; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw

Hi

In our IGT/Kernel culture, patch commit titles (here, presented as the email subject) are usually small (under 70-80 chars) providing a quick summary of what the change does.

Also, we try to make each patch contain a single logical and complete change. So, for example, one approach you could have taken here would
be:
 - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
 - patch 2: solve the ffs() problem
 - patch 3: solve the drmModeSetPlane() problem
 - patch 4: change kms_fbc_crc to not need cairo
 - patch 5: change the build system so it compiles tests that now work on Android

Following is another text explaining this pattern:
http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
hes#n201


Now, regarding the Android vs Cairo macros: I know this has been discussed in the mailing list a few times but I didn't follow it closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like Daniel or others to comment on this. Do we want the ifdef dance in igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo parts, and igt_cairo for the cairo parts? The igt_fb library is definitely useful without cairo, so this would make sense to me. But perhaps we want to just keep everything as-is since it's possible to have cairo on Android?

There are some other comments inline. Please see below.


Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> From: Devon Davies <devon.davies@intel.com>

> 

> tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE

>     Each call to the function drmModeSetPlane now has an addtional 

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> tests/Android.mk: Allow the above tests to be built without Cairo

>     Simply removed them from the tests the be skipped.

> 

> libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE

>     I had to define ffs as __builtin_fss due to compiler issues.

>     Each call to the function drmModeSetPlane now has an addtional 

> NULL in the

>     arguments if DRM_PRIMARY_DISABLE is set.

> 

> libs/igt_fb.c: Will now build some functions without Cairo

>     Functions which aren't used by the framebuffer compression tests 

> are

>     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&

>     ANDROID_HAS_CAIRO

> 

> libs/Android.mk

>     igt_fb and igt_kms are no longer ignored if we don't have Cairo.

> 

> The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary 

> dependance on the Cairo graphics engine.

> Also, drmModeSetPlane may have an additional argument if 

> DRM_PRIMARY_DISABLE is set (as it was for me), I have fixed that 

> issue.

> 

> Signed-off-by: Devon Davies <devon.davies@intel.com>

> ---

>  lib/Android.mk                   |  4 --

>  lib/igt_fb.c                     | 26 ++++++++++++-

>  lib/igt_kms.c                    | 15 ++++++--

>  tests/Android.mk                 |  5 +++

>  tests/kms_fbc_crc.c              | 20 ++++++----

>  tests/kms_frontbuffer_tracking.c | 79

> +++++++++++++++++++++++++++++++++-------

>  6 files changed, 119 insertions(+), 30 deletions(-)

> 

> diff --git a/lib/Android.mk b/lib/Android.mk index badec1e..bbdb051

> 100644

> --- a/lib/Android.mk

> +++ b/lib/Android.mk

> @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")

>      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-

> 1.12.16/src

>      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"

> -DIGT_SRCDIR=\".\"

>  else

> -skip_lib_list := \

> -    igt_kms.c \

> -    igt_kms.h \

> -    igt_fb.c

>      -DANDROID_HAS_CAIRO=0

>  endif

>  

> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index c985824..5acdaa7 100644

> --- a/lib/igt_fb.c

> +++ b/lib/igt_fb.c

> @@ -33,6 +33,7 @@

>  #include "igt_fb.h"

>  #include "ioctl_wrappers.h"

>  

> +


Please don't add random lines to files.


>  /**

>   * SECTION:igt_fb

>   * @short_description: Framebuffer handling and drawing library @@

> -52,11 +53,23 @@

>   */

>  

>  /* drm fourcc/cairo format maps */

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  #define DF(did, cid, _bpp, _depth)	\

>  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }

> +

> +#else

> +

> +#define DF(did, cid, _bpp, _depth)	\

> +	{ DRM_FORMAT_##did, # did, _bpp, _depth }

> +

> +#endif

> +

>  static struct format_desc_struct {

>  	uint32_t drm_id;

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_format_t cairo_id;

> +#endif

>  	const char *name;

>  	int bpp;

>  	int depth;

> @@ -72,7 +85,6 @@ static struct format_desc_struct {

>  #define for_each_format(f)	\

>  	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc);

> f++)

>  

> -


Please don't remove random lines from files.


>  /* helpers to create nice-looking framebuffers */

>  static int create_bo_for_fb(int fd, int width, int height, int bpp,

>  			    uint64_t tiling, unsigned bo_size, @@ -125,6 +137,8 @@ static 

> int create_bo_for_fb(int fd, int width, int height, int bpp,

>  	return ret;

>  }

>  

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

> +

>  /**

>   * igt_paint_color:

>   * @cr: cairo drawing context

> @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char 

> *filename,

>  

>  	fclose(f);

>  }

> +#endif

>  

>  /**

>   * igt_create_fb_with_bo_size:

> @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int 

> height, uint32_t format,

>  	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, 

> fb,

>  					  0, 0);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_create_color_fb:

> @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb 

> *fb, const char *filename)

>  

>  	igt_assert(status == CAIRO_STATUS_SUCCESS);

>  }

> +#endif

>  

>  /**

>   * igt_remove_fb:

> @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb 

> *fb, const char *filename)

>   */

>  void igt_remove_fb(int fd, struct igt_fb *fb)

>  {

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  	cairo_surface_destroy(fb->cairo_surface);

> +#endif

>  	do_or_die(drmModeRmFB(fd, fb->fb_id));

>  	gem_close(fd, fb->gem_handle);

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_bpp_depth_to_drm_format:

> @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp, 

> int depth)

>  		     depth);

>  }

>  

> +#endif

> +

>  /**

>   * igt_drm_format_to_bpp:

>   * @drm_format: drm fourcc pixel format code @@ -1062,6 +1084,7 @@ 

> const char *igt_format_str(uint32_t drm_format)

>  

>  	return "invalid";

>  }

> +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)

>  

>  /**

>   * igt_get_all_formats:

> @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t 

> **formats, int *format_count)

>  	*formats = drm_formats;

>  	*format_count = ARRAY_SIZE(format_desc);

>  }

> +#endif

> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..7b682cb

> 100644

> --- a/lib/igt_kms.c

> +++ b/lib/igt_kms.c

> @@ -49,6 +49,8 @@

>  #include "intel_chipset.h"

>  #include "igt_debugfs.h"

>  

> +#define ffs __builtin_ffs


A define like this can be dangerous. Shouldn't we just fix all the callers, so code readers know exactly which function is being run?

On the other hand, it seems that this change will restrict us to gcc- only. So maybe we could just define ffs as __builtin_ffs if ANDROID is defined?


> +

>  /* list of connectors that need resetting on exit */

>  #define MAX_CONNECTORS 32

>  static char *forced_connectors[MAX_CONNECTORS + 1]; @@ -1354,8

> +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,

>  				      IGT_FIXED(0,0), /* src_x */

>  				      IGT_FIXED(0,0), /* src_y */

>  				      IGT_FIXED(0,0), /* src_w */

> -				      IGT_FIXED(0,0) /* src_h */);

> -

> +				      IGT_FIXED(0,0) /* src_h */

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif


If we're going this way, maybe the best approach would be to add a wrapper (igt_kms_set_plane?) and make the current callers use it.

On a side note: why is Android different here!?


Thanks,
Paulo

> +					  );

>  		CHECK_RETURN(ret, fail_on_error);

>  	} else if (plane->fb_changed || plane->position_changed ||

>  		plane->size_changed) {

> @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t 

> *plane,

>  				      crtc_x, crtc_y,

>  				      crtc_w, crtc_h,

>  				      src_x, src_y,

> -				      src_w, src_h);

> +				      src_w, src_h

> +#if DRM_PRIMARY_DISABLE

> +					  , NULL

> +#endif

> +					  );

>  

>  		CHECK_RETURN(ret, fail_on_error);

>  	}

> diff --git a/tests/Android.mk b/tests/Android.mk index

> 8457125..eb287a6 100644

> --- a/tests/Android.mk

> +++ b/tests/Android.mk

> @@ -65,6 +65,11 @@ else

>  

>      tmp_list := $(foreach test_name, $(TESTS_progs_M),\

>          $(if $(findstring kms_,$(test_name)),$(test_name)))

> +

> +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo

> +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))

> +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))

> +

>      skip_tests_list += $(tmp_list)

>  

>      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0 diff --git 

> a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 02e95e5..717e891

> 100644

> --- a/tests/kms_fbc_crc.c

> +++ b/tests/kms_fbc_crc.c

> @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool tiled, 

> struct igt_fb *fbs)

>  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :

>  				  LOCAL_DRM_FORMAT_MOD_NONE;

>  

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.0, 0.0, 0.0, &fbs[0]);

> -	igt_assert(rc);

> -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> -				 DRM_FORMAT_XRGB8888, tiling,

> -				 0.1, 0.1, 0.1, &fbs[1]);

> -	igt_assert(rc);

> +	unsigned int fb_id;

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[0]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);

> +

> +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-

> >vdisplay,

> +				DRM_FORMAT_XRGB8888, tiling,

> &fbs[1]);

> +	igt_assert(fb_id);

> +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);

> +

>  }

>  

>  /* Since we want to be really safe that the CRCs are actually what we 

> really diff --git a/tests/kms_frontbuffer_tracking.c

> b/tests/kms_frontbuffer_tracking.c

> index e7acc7c..f8b9eca 100644

> --- a/tests/kms_frontbuffer_tracking.c

> +++ b/tests/kms_frontbuffer_tracking.c

> @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)

>  

>  	for (i = 0; i < drm.plane_res->count_planes; i++) {

>  		rc = drmModeSetPlane(drm.fd, drm.plane_res-

> >planes[i], 0, 0, 0,

> -				     0, 0, 0, 0, 0, 0, 0, 0);

> +				     0, 0, 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +				     , NULL

> +#endif

> +			     );

>  		igt_assert_eq(rc, 0);

>  	}

>  }

> @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct 

> test_mode *t,

>  			     params->sprite.fb->fb_id, 0, 0, 0,

>  			     params->sprite.w, params->sprite.h,

>  			     0, 0, params->sprite.w << 16,

> -			     params->sprite.h << 16);

> +			     params->sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	do_assertions(ASSERT_NO_ACTION_CHANGE);

> @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct 

> modeset_params *params)

>  			     params->mode->hdisplay,

>  			     params->mode->vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  }

>  

> @@ -2406,7 +2418,11 @@ static void move_subtest(const struct test_mode

> *t)

>  					     params->sprite.fb-

> >fb_id, 0,

>  					     rect.x, rect.y, rect.w,

>  					     rect.h, 0, 0, rect.w <<

> 16,

> -					     rect.h << 16);

> +					     rect.h << 16

> +#if DRM_PRIMARY_DISABLE

> +					     , NULL

> +#endif

> +			     );

>  			igt_assert_eq(rc, 0);

>  			break;

>  		default:

> @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct 

> test_mode *t)

>  				break;

>  			case PLANE_SPR:

>  				rc = drmModeSetPlane(drm.fd, params-

> >sprite_id,

> -						     0, 0, 0, 0, 0,

> 0, 0, 0, 0,

> -						     0, 0);

> +						     0, 0, 0, 0, 0,

> 0, 0, 0, 0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct 

> test_mode *t)

>  						     params-

> >sprite.h, 0,

>  						     0,

>  						     params-

> >sprite.w << 16,

> -						     params-

> >sprite.h << 16);

> +						     params-

> >sprite.h << 16

> +#if DRM_PRIMARY_DISABLE

> +						     , NULL

> +#endif

> +			     );

>  				igt_assert_eq(rc, 0);

>  				break;

>  			default:

> @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const 

> struct test_mode *t)

>  			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,

>  			     fullscreen_fb.height, 0, 0,

>  			     fullscreen_fb.width << 16,

> -			     fullscreen_fb.height << 16);

> +			     fullscreen_fb.height << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  	update_wanted_crc(t, &pattern->crcs[t->format][0]);

>  

> @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const 

> struct test_mode *t)

>  	do_assertions(assertions);

>  

>  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 

> 0,

> -			     0, 0, 0);

> +			     0, 0, 0

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert_eq(rc, 0);

>  

>  	if (t->screen == SCREEN_PRIM)

> @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     params->mode->vdisplay / 2,

>  			     params->fb.x << 16, params->fb.y << 16,

>  			     (params->fb.w / 2) << 16,

> -			     (params->fb.h / 2) << 16);

> +			     (params->fb.h / 2) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     (params->fb.x + params->fb.w / 2) << 16,

>  			     (params->fb.y + params->fb.h / 2) << 16,

>  			     (params->fb.w / 4) << 16,

> -			     (params->fb.h / 4) << 16);

> +			     (params->fb.h / 4) << 16

> +#if DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(DONT_ASSERT_CRC);

>  

> @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct 

> test_mode *t)

>  			     0, 0,

>  			     params->mode->hdisplay, params->mode-

> >vdisplay,

>  			     params->fb.x << 16, params->fb.y << 16,

> -			     params->fb.w << 16, params->fb.h <<

> 16);

> +			     params->fb.w << 16, params->fb.h << 16 #if 

> +DRM_PRIMARY_DISABLE

> +			     , NULL

> +#endif

> +			     );

>  	igt_assert(rc == 0);

>  	do_assertions(0);

>
Daniel Vetter Jan. 22, 2016, 5:12 p.m. UTC | #4
On Fri, Jan 22, 2016 at 05:02:07PM +0000, Davies, Devon wrote:
> Hi,
> 
> With regards to the additional NULL argument to drmModeSetPlane on Android.
> 
> Apparently, there was a previous attempt to make a wrapper for it.
> Would it be possible to upstream that?

No. Whoever has done that private patch in Android has no idea about abi
compatibility and better carry the fallout from that themselves. Iirc this
fun is a few years old already, at least it looks very familiar.
-Daniel

> 
> Devon
> 
> 
> -----Original Message-----
> From: Davies, Devon 
> Sent: Friday, January 22, 2016 3:18 PM
> To: Zanoni, Paulo R; intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; Vetter, Daniel
> Subject: RE: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw
> 
> Hi,
> 
> Thanks for the feedback.
> 
> I'll update the commit title. I will also separate this patch into 5 patches.
> 
> For now I want to keep the "ifdef dance" in igt_fbc.c. If we want to split the file up, that should be a new commit/patch.
> (I think we should get the fbc tests building on Android first before we start the splitting up igt_fbc.c, since we want to start testing asap.)
> 
> I looked to see if I had removed/added any extra lines... some must have missed me, I will revert them.
> 
> The ffs define: I can see that being dangerous.
> Should I do
> #if defined(__GNUC__)
> #define ffs __builtin_ffs
> #endif
> ?
> In my opinion that is cleaner than having a ifdef around the actual ffs function call. It's also easier to add to (in case someone in the future uses a different compiler).
> 
> igt_drm_plane_commit:
> I don't know why Android is different, according to git blame, it's been this way since 13th June 2014.
> I'll ask why separately to this thread.
> As for making a wrapper. I think that should be part of another commit.
> (Again to be done after this, since we want to begin testing asap.)
> 
> Thanks,
> Devon.
> 
> -----Original Message-----
> From: Zanoni, Paulo R
> Sent: Wednesday, January 20, 2016 7:35 PM
> To: intel-gfx@lists.freedesktop.org; Davies, Devon
> Cc: Gore, Tim; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw
> 
> Hi
> 
> In our IGT/Kernel culture, patch commit titles (here, presented as the email subject) are usually small (under 70-80 chars) providing a quick summary of what the change does.
> 
> Also, we try to make each patch contain a single logical and complete change. So, for example, one approach you could have taken here would
> be:
>  - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
>  - patch 2: solve the ffs() problem
>  - patch 3: solve the drmModeSetPlane() problem
>  - patch 4: change kms_fbc_crc to not need cairo
>  - patch 5: change the build system so it compiles tests that now work on Android
> 
> Following is another text explaining this pattern:
> http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
> hes#n201
> 
> 
> Now, regarding the Android vs Cairo macros: I know this has been discussed in the mailing list a few times but I didn't follow it closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like Daniel or others to comment on this. Do we want the ifdef dance in igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo parts, and igt_cairo for the cairo parts? The igt_fb library is definitely useful without cairo, so this would make sense to me. But perhaps we want to just keep everything as-is since it's possible to have cairo on Android?
> 
> There are some other comments inline. Please see below.
> 
> 
> Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> > From: Devon Davies <devon.davies@intel.com>
> > 
> > tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > tests/Android.mk: Allow the above tests to be built without Cairo
> >     Simply removed them from the tests the be skipped.
> > 
> > libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE
> >     I had to define ffs as __builtin_fss due to compiler issues.
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > libs/igt_fb.c: Will now build some functions without Cairo
> >     Functions which aren't used by the framebuffer compression tests 
> > are
> >     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&
> >     ANDROID_HAS_CAIRO
> > 
> > libs/Android.mk
> >     igt_fb and igt_kms are no longer ignored if we don't have Cairo.
> > 
> > The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary 
> > dependance on the Cairo graphics engine.
> > Also, drmModeSetPlane may have an additional argument if 
> > DRM_PRIMARY_DISABLE is set (as it was for me), I have fixed that 
> > issue.
> > 
> > Signed-off-by: Devon Davies <devon.davies@intel.com>
> > ---
> >  lib/Android.mk                   |  4 --
> >  lib/igt_fb.c                     | 26 ++++++++++++-
> >  lib/igt_kms.c                    | 15 ++++++--
> >  tests/Android.mk                 |  5 +++
> >  tests/kms_fbc_crc.c              | 20 ++++++----
> >  tests/kms_frontbuffer_tracking.c | 79
> > +++++++++++++++++++++++++++++++++-------
> >  6 files changed, 119 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/Android.mk b/lib/Android.mk index badec1e..bbdb051
> > 100644
> > --- a/lib/Android.mk
> > +++ b/lib/Android.mk
> > @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")
> >      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-
> > 1.12.16/src
> >      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"
> > -DIGT_SRCDIR=\".\"
> >  else
> > -skip_lib_list := \
> > -    igt_kms.c \
> > -    igt_kms.h \
> > -    igt_fb.c
> >      -DANDROID_HAS_CAIRO=0
> >  endif
> >  
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c index c985824..5acdaa7 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -33,6 +33,7 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +
> 
> Please don't add random lines to files.
> 
> 
> >  /**
> >   * SECTION:igt_fb
> >   * @short_description: Framebuffer handling and drawing library @@
> > -52,11 +53,23 @@
> >   */
> >  
> >  /* drm fourcc/cairo format maps */
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  #define DF(did, cid, _bpp, _depth)	\
> >  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }
> > +
> > +#else
> > +
> > +#define DF(did, cid, _bpp, _depth)	\
> > +	{ DRM_FORMAT_##did, # did, _bpp, _depth }
> > +
> > +#endif
> > +
> >  static struct format_desc_struct {
> >  	uint32_t drm_id;
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_format_t cairo_id;
> > +#endif
> >  	const char *name;
> >  	int bpp;
> >  	int depth;
> > @@ -72,7 +85,6 @@ static struct format_desc_struct {
> >  #define for_each_format(f)	\
> >  	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc);
> > f++)
> >  
> > -
> 
> Please don't remove random lines from files.
> 
> 
> >  /* helpers to create nice-looking framebuffers */
> >  static int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  			    uint64_t tiling, unsigned bo_size, @@ -125,6 +137,8 @@ static 
> > int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  	return ret;
> >  }
> >  
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  /**
> >   * igt_paint_color:
> >   * @cr: cairo drawing context
> > @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char 
> > *filename,
> >  
> >  	fclose(f);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_create_fb_with_bo_size:
> > @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int 
> > height, uint32_t format,
> >  	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, 
> > fb,
> >  					  0, 0);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_create_color_fb:
> > @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >  
> >  	igt_assert(status == CAIRO_STATUS_SUCCESS);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_remove_fb:
> > @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >   */
> >  void igt_remove_fb(int fd, struct igt_fb *fb)
> >  {
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_surface_destroy(fb->cairo_surface);
> > +#endif
> >  	do_or_die(drmModeRmFB(fd, fb->fb_id));
> >  	gem_close(fd, fb->gem_handle);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_bpp_depth_to_drm_format:
> > @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp, 
> > int depth)
> >  		     depth);
> >  }
> >  
> > +#endif
> > +
> >  /**
> >   * igt_drm_format_to_bpp:
> >   * @drm_format: drm fourcc pixel format code @@ -1062,6 +1084,7 @@ 
> > const char *igt_format_str(uint32_t drm_format)
> >  
> >  	return "invalid";
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_get_all_formats:
> > @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t 
> > **formats, int *format_count)
> >  	*formats = drm_formats;
> >  	*format_count = ARRAY_SIZE(format_desc);
> >  }
> > +#endif
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..7b682cb
> > 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -49,6 +49,8 @@
> >  #include "intel_chipset.h"
> >  #include "igt_debugfs.h"
> >  
> > +#define ffs __builtin_ffs
> 
> A define like this can be dangerous. Shouldn't we just fix all the callers, so code readers know exactly which function is being run?
> 
> On the other hand, it seems that this change will restrict us to gcc- only. So maybe we could just define ffs as __builtin_ffs if ANDROID is defined?
> 
> 
> > +
> >  /* list of connectors that need resetting on exit */
> >  #define MAX_CONNECTORS 32
> >  static char *forced_connectors[MAX_CONNECTORS + 1]; @@ -1354,8
> > +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> >  				      IGT_FIXED(0,0), /* src_x */
> >  				      IGT_FIXED(0,0), /* src_y */
> >  				      IGT_FIXED(0,0), /* src_w */
> > -				      IGT_FIXED(0,0) /* src_h */);
> > -
> > +				      IGT_FIXED(0,0) /* src_h */
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> 
> If we're going this way, maybe the best approach would be to add a wrapper (igt_kms_set_plane?) and make the current callers use it.
> 
> On a side note: why is Android different here!?
> 
> 
> Thanks,
> Paulo
> 
> > +					  );
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	} else if (plane->fb_changed || plane->position_changed ||
> >  		plane->size_changed) {
> > @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t 
> > *plane,
> >  				      crtc_x, crtc_y,
> >  				      crtc_w, crtc_h,
> >  				      src_x, src_y,
> > -				      src_w, src_h);
> > +				      src_w, src_h
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> > +					  );
> >  
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	}
> > diff --git a/tests/Android.mk b/tests/Android.mk index
> > 8457125..eb287a6 100644
> > --- a/tests/Android.mk
> > +++ b/tests/Android.mk
> > @@ -65,6 +65,11 @@ else
> >  
> >      tmp_list := $(foreach test_name, $(TESTS_progs_M),\
> >          $(if $(findstring kms_,$(test_name)),$(test_name)))
> > +
> > +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo
> > +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))
> > +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))
> > +
> >      skip_tests_list += $(tmp_list)
> >  
> >      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0 diff --git 
> > a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 02e95e5..717e891
> > 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool tiled, 
> > struct igt_fb *fbs)
> >  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :
> >  				  LOCAL_DRM_FORMAT_MOD_NONE;
> >  
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.0, 0.0, 0.0, &fbs[0]);
> > -	igt_assert(rc);
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.1, 0.1, 0.1, &fbs[1]);
> > -	igt_assert(rc);
> > +	unsigned int fb_id;
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[0]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[1]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);
> > +
> >  }
> >  
> >  /* Since we want to be really safe that the CRCs are actually what we 
> > really diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e7acc7c..f8b9eca 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)
> >  
> >  	for (i = 0; i < drm.plane_res->count_planes; i++) {
> >  		rc = drmModeSetPlane(drm.fd, drm.plane_res-
> > >planes[i], 0, 0, 0,
> > -				     0, 0, 0, 0, 0, 0, 0, 0);
> > +				     0, 0, 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +				     , NULL
> > +#endif
> > +			     );
> >  		igt_assert_eq(rc, 0);
> >  	}
> >  }
> > @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct 
> > test_mode *t,
> >  			     params->sprite.fb->fb_id, 0, 0, 0,
> >  			     params->sprite.w, params->sprite.h,
> >  			     0, 0, params->sprite.w << 16,
> > -			     params->sprite.h << 16);
> > +			     params->sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	do_assertions(ASSERT_NO_ACTION_CHANGE);
> > @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct 
> > modeset_params *params)
> >  			     params->mode->hdisplay,
> >  			     params->mode->vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  }
> >  
> > @@ -2406,7 +2418,11 @@ static void move_subtest(const struct test_mode
> > *t)
> >  					     params->sprite.fb-
> > >fb_id, 0,
> >  					     rect.x, rect.y, rect.w,
> >  					     rect.h, 0, 0, rect.w <<
> > 16,
> > -					     rect.h << 16);
> > +					     rect.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +					     , NULL
> > +#endif
> > +			     );
> >  			igt_assert_eq(rc, 0);
> >  			break;
> >  		default:
> > @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  				break;
> >  			case PLANE_SPR:
> >  				rc = drmModeSetPlane(drm.fd, params-
> > >sprite_id,
> > -						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0,
> > -						     0, 0);
> > +						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  						     params-
> > >sprite.h, 0,
> >  						     0,
> >  						     params-
> > >sprite.w << 16,
> > -						     params-
> > >sprite.h << 16);
> > +						     params-
> > >sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,
> >  			     fullscreen_fb.height, 0, 0,
> >  			     fullscreen_fb.width << 16,
> > -			     fullscreen_fb.height << 16);
> > +			     fullscreen_fb.height << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  	update_wanted_crc(t, &pattern->crcs[t->format][0]);
> >  
> > @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  	do_assertions(assertions);
> >  
> >  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 
> > 0,
> > -			     0, 0, 0);
> > +			     0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	if (t->screen == SCREEN_PRIM)
> > @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->vdisplay / 2,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     (params->fb.x + params->fb.w / 2) << 16,
> >  			     (params->fb.y + params->fb.h / 2) << 16,
> >  			     (params->fb.w / 4) << 16,
> > -			     (params->fb.h / 4) << 16);
> > +			     (params->fb.h / 4) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(0);
> >  
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Jan. 22, 2016, 5:14 p.m. UTC | #5
On Fri, Jan 22, 2016 at 03:18:15PM +0000, Davies, Devon wrote:
> Hi,
> 
> Thanks for the feedback.
> 
> I'll update the commit title. I will also separate this patch into 5 patches.
> 
> For now I want to keep the "ifdef dance" in igt_fbc.c. If we want to split the file up, that should be a new commit/patch.
> (I think we should get the fbc tests building on Android first before we start the splitting up igt_fbc.c, since we want to start testing asap.)

Nope, no #ifdef madness in library code. If we need that in headers, or
for truly exceptional cases in intel_os.c then ok. But in core library
code it just makes reading things much, much harder. So either we rewrite
igt_fb.c to not need cairo at all, or android just grows itself some
cairo.

Thanks, Daniel

> 
> I looked to see if I had removed/added any extra lines... some must have missed me, I will revert them.
> 
> The ffs define: I can see that being dangerous.
> Should I do
> #if defined(__GNUC__)
> #define ffs __builtin_ffs
> #endif
> ?
> In my opinion that is cleaner than having a ifdef around the actual ffs function call. It's also easier to add to (in case someone in the future uses a different compiler).
> 
> igt_drm_plane_commit:
> I don't know why Android is different, according to git blame, it's been this way since 13th June 2014.
> I'll ask why separately to this thread.
> As for making a wrapper. I think that should be part of another commit.
> (Again to be done after this, since we want to begin testing asap.)
> 
> Thanks,
> Devon.
> 
> -----Original Message-----
> From: Zanoni, Paulo R 
> Sent: Wednesday, January 20, 2016 7:35 PM
> To: intel-gfx@lists.freedesktop.org; Davies, Devon
> Cc: Gore, Tim; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw
> 
> Hi
> 
> In our IGT/Kernel culture, patch commit titles (here, presented as the email subject) are usually small (under 70-80 chars) providing a quick summary of what the change does.
> 
> Also, we try to make each patch contain a single logical and complete change. So, for example, one approach you could have taken here would
> be:
>  - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
>  - patch 2: solve the ffs() problem
>  - patch 3: solve the drmModeSetPlane() problem
>  - patch 4: change kms_fbc_crc to not need cairo
>  - patch 5: change the build system so it compiles tests that now work on Android
> 
> Following is another text explaining this pattern:
> http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
> hes#n201
> 
> 
> Now, regarding the Android vs Cairo macros: I know this has been discussed in the mailing list a few times but I didn't follow it closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like Daniel or others to comment on this. Do we want the ifdef dance in igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo parts, and igt_cairo for the cairo parts? The igt_fb library is definitely useful without cairo, so this would make sense to me. But perhaps we want to just keep everything as-is since it's possible to have cairo on Android?
> 
> There are some other comments inline. Please see below.
> 
> 
> Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> > From: Devon Davies <devon.davies@intel.com>
> > 
> > tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > tests/Android.mk: Allow the above tests to be built without Cairo
> >     Simply removed them from the tests the be skipped.
> > 
> > libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE
> >     I had to define ffs as __builtin_fss due to compiler issues.
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > libs/igt_fb.c: Will now build some functions without Cairo
> >     Functions which aren't used by the framebuffer compression tests 
> > are
> >     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&
> >     ANDROID_HAS_CAIRO
> > 
> > libs/Android.mk
> >     igt_fb and igt_kms are no longer ignored if we don't have Cairo.
> > 
> > The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary 
> > dependance on the Cairo graphics engine.
> > Also, drmModeSetPlane may have an additional argument if 
> > DRM_PRIMARY_DISABLE is set (as it was for me), I have fixed that 
> > issue.
> > 
> > Signed-off-by: Devon Davies <devon.davies@intel.com>
> > ---
> >  lib/Android.mk                   |  4 --
> >  lib/igt_fb.c                     | 26 ++++++++++++-
> >  lib/igt_kms.c                    | 15 ++++++--
> >  tests/Android.mk                 |  5 +++
> >  tests/kms_fbc_crc.c              | 20 ++++++----
> >  tests/kms_frontbuffer_tracking.c | 79
> > +++++++++++++++++++++++++++++++++-------
> >  6 files changed, 119 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/Android.mk b/lib/Android.mk index badec1e..bbdb051 
> > 100644
> > --- a/lib/Android.mk
> > +++ b/lib/Android.mk
> > @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")
> >      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-
> > 1.12.16/src
> >      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"
> > -DIGT_SRCDIR=\".\"
> >  else
> > -skip_lib_list := \
> > -    igt_kms.c \
> > -    igt_kms.h \
> > -    igt_fb.c
> >      -DANDROID_HAS_CAIRO=0
> >  endif
> >  
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c index c985824..5acdaa7 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -33,6 +33,7 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +
> 
> Please don't add random lines to files.
> 
> 
> >  /**
> >   * SECTION:igt_fb
> >   * @short_description: Framebuffer handling and drawing library @@ 
> > -52,11 +53,23 @@
> >   */
> >  
> >  /* drm fourcc/cairo format maps */
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  #define DF(did, cid, _bpp, _depth)	\
> >  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }
> > +
> > +#else
> > +
> > +#define DF(did, cid, _bpp, _depth)	\
> > +	{ DRM_FORMAT_##did, # did, _bpp, _depth }
> > +
> > +#endif
> > +
> >  static struct format_desc_struct {
> >  	uint32_t drm_id;
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_format_t cairo_id;
> > +#endif
> >  	const char *name;
> >  	int bpp;
> >  	int depth;
> > @@ -72,7 +85,6 @@ static struct format_desc_struct {
> >  #define for_each_format(f)	\
> >  	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); 
> > f++)
> >  
> > -
> 
> Please don't remove random lines from files.
> 
> 
> >  /* helpers to create nice-looking framebuffers */
> >  static int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  			    uint64_t tiling, unsigned bo_size, @@ -125,6 +137,8 @@ static 
> > int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  	return ret;
> >  }
> >  
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  /**
> >   * igt_paint_color:
> >   * @cr: cairo drawing context
> > @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char 
> > *filename,
> >  
> >  	fclose(f);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_create_fb_with_bo_size:
> > @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int 
> > height, uint32_t format,
> >  	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, 
> > fb,
> >  					  0, 0);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_create_color_fb:
> > @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >  
> >  	igt_assert(status == CAIRO_STATUS_SUCCESS);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_remove_fb:
> > @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >   */
> >  void igt_remove_fb(int fd, struct igt_fb *fb)
> >  {
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_surface_destroy(fb->cairo_surface);
> > +#endif
> >  	do_or_die(drmModeRmFB(fd, fb->fb_id));
> >  	gem_close(fd, fb->gem_handle);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_bpp_depth_to_drm_format:
> > @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp, 
> > int depth)
> >  		     depth);
> >  }
> >  
> > +#endif
> > +
> >  /**
> >   * igt_drm_format_to_bpp:
> >   * @drm_format: drm fourcc pixel format code @@ -1062,6 +1084,7 @@ 
> > const char *igt_format_str(uint32_t drm_format)
> >  
> >  	return "invalid";
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_get_all_formats:
> > @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t 
> > **formats, int *format_count)
> >  	*formats = drm_formats;
> >  	*format_count = ARRAY_SIZE(format_desc);
> >  }
> > +#endif
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..7b682cb 
> > 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -49,6 +49,8 @@
> >  #include "intel_chipset.h"
> >  #include "igt_debugfs.h"
> >  
> > +#define ffs __builtin_ffs
> 
> A define like this can be dangerous. Shouldn't we just fix all the callers, so code readers know exactly which function is being run?
> 
> On the other hand, it seems that this change will restrict us to gcc- only. So maybe we could just define ffs as __builtin_ffs if ANDROID is defined?
> 
> 
> > +
> >  /* list of connectors that need resetting on exit */
> >  #define MAX_CONNECTORS 32
> >  static char *forced_connectors[MAX_CONNECTORS + 1]; @@ -1354,8 
> > +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> >  				      IGT_FIXED(0,0), /* src_x */
> >  				      IGT_FIXED(0,0), /* src_y */
> >  				      IGT_FIXED(0,0), /* src_w */
> > -				      IGT_FIXED(0,0) /* src_h */);
> > -
> > +				      IGT_FIXED(0,0) /* src_h */
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> 
> If we're going this way, maybe the best approach would be to add a wrapper (igt_kms_set_plane?) and make the current callers use it.
> 
> On a side note: why is Android different here!?
> 
> 
> Thanks,
> Paulo
> 
> > +					  );
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	} else if (plane->fb_changed || plane->position_changed ||
> >  		plane->size_changed) {
> > @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t 
> > *plane,
> >  				      crtc_x, crtc_y,
> >  				      crtc_w, crtc_h,
> >  				      src_x, src_y,
> > -				      src_w, src_h);
> > +				      src_w, src_h
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> > +					  );
> >  
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	}
> > diff --git a/tests/Android.mk b/tests/Android.mk index 
> > 8457125..eb287a6 100644
> > --- a/tests/Android.mk
> > +++ b/tests/Android.mk
> > @@ -65,6 +65,11 @@ else
> >  
> >      tmp_list := $(foreach test_name, $(TESTS_progs_M),\
> >          $(if $(findstring kms_,$(test_name)),$(test_name)))
> > +
> > +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo
> > +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))
> > +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))
> > +
> >      skip_tests_list += $(tmp_list)
> >  
> >      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0 diff --git 
> > a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 02e95e5..717e891 
> > 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool tiled, 
> > struct igt_fb *fbs)
> >  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :
> >  				  LOCAL_DRM_FORMAT_MOD_NONE;
> >  
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.0, 0.0, 0.0, &fbs[0]);
> > -	igt_assert(rc);
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.1, 0.1, 0.1, &fbs[1]);
> > -	igt_assert(rc);
> > +	unsigned int fb_id;
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[0]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[1]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);
> > +
> >  }
> >  
> >  /* Since we want to be really safe that the CRCs are actually what we 
> > really diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e7acc7c..f8b9eca 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)
> >  
> >  	for (i = 0; i < drm.plane_res->count_planes; i++) {
> >  		rc = drmModeSetPlane(drm.fd, drm.plane_res-
> > >planes[i], 0, 0, 0,
> > -				     0, 0, 0, 0, 0, 0, 0, 0);
> > +				     0, 0, 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +				     , NULL
> > +#endif
> > +			     );
> >  		igt_assert_eq(rc, 0);
> >  	}
> >  }
> > @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct 
> > test_mode *t,
> >  			     params->sprite.fb->fb_id, 0, 0, 0,
> >  			     params->sprite.w, params->sprite.h,
> >  			     0, 0, params->sprite.w << 16,
> > -			     params->sprite.h << 16);
> > +			     params->sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	do_assertions(ASSERT_NO_ACTION_CHANGE);
> > @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct 
> > modeset_params *params)
> >  			     params->mode->hdisplay,
> >  			     params->mode->vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  }
> >  
> > @@ -2406,7 +2418,11 @@ static void move_subtest(const struct test_mode 
> > *t)
> >  					     params->sprite.fb-
> > >fb_id, 0,
> >  					     rect.x, rect.y, rect.w,
> >  					     rect.h, 0, 0, rect.w <<
> > 16,
> > -					     rect.h << 16);
> > +					     rect.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +					     , NULL
> > +#endif
> > +			     );
> >  			igt_assert_eq(rc, 0);
> >  			break;
> >  		default:
> > @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  				break;
> >  			case PLANE_SPR:
> >  				rc = drmModeSetPlane(drm.fd, params-
> > >sprite_id,
> > -						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0,
> > -						     0, 0);
> > +						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  						     params-
> > >sprite.h, 0,
> >  						     0,
> >  						     params-
> > >sprite.w << 16,
> > -						     params-
> > >sprite.h << 16);
> > +						     params-
> > >sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,
> >  			     fullscreen_fb.height, 0, 0,
> >  			     fullscreen_fb.width << 16,
> > -			     fullscreen_fb.height << 16);
> > +			     fullscreen_fb.height << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  	update_wanted_crc(t, &pattern->crcs[t->format][0]);
> >  
> > @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  	do_assertions(assertions);
> >  
> >  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 
> > 0,
> > -			     0, 0, 0);
> > +			     0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	if (t->screen == SCREEN_PRIM)
> > @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->vdisplay / 2,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     (params->fb.x + params->fb.w / 2) << 16,
> >  			     (params->fb.y + params->fb.h / 2) << 16,
> >  			     (params->fb.w / 4) << 16,
> > -			     (params->fb.h / 4) << 16);
> > +			     (params->fb.h / 4) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(0);
> >  
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala Jan. 22, 2016, 5:17 p.m. UTC | #6
On Fri, Jan 22, 2016 at 05:02:07PM +0000, Davies, Devon wrote:
> Hi,
> 
> With regards to the additional NULL argument to drmModeSetPlane on Android.
> 
> Apparently, there was a previous attempt to make a wrapper for it.
> Would it be possible to upstream that?

How about fix it in Android with SetPlaneAndroidHack() or somesuch?

> 
> Devon
> 
> 
> -----Original Message-----
> From: Davies, Devon 
> Sent: Friday, January 22, 2016 3:18 PM
> To: Zanoni, Paulo R; intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; Vetter, Daniel
> Subject: RE: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw
> 
> Hi,
> 
> Thanks for the feedback.
> 
> I'll update the commit title. I will also separate this patch into 5 patches.
> 
> For now I want to keep the "ifdef dance" in igt_fbc.c. If we want to split the file up, that should be a new commit/patch.
> (I think we should get the fbc tests building on Android first before we start the splitting up igt_fbc.c, since we want to start testing asap.)
> 
> I looked to see if I had removed/added any extra lines... some must have missed me, I will revert them.
> 
> The ffs define: I can see that being dangerous.
> Should I do
> #if defined(__GNUC__)
> #define ffs __builtin_ffs
> #endif
> ?
> In my opinion that is cleaner than having a ifdef around the actual ffs function call. It's also easier to add to (in case someone in the future uses a different compiler).
> 
> igt_drm_plane_commit:
> I don't know why Android is different, according to git blame, it's been this way since 13th June 2014.
> I'll ask why separately to this thread.
> As for making a wrapper. I think that should be part of another commit.
> (Again to be done after this, since we want to begin testing asap.)
> 
> Thanks,
> Devon.
> 
> -----Original Message-----
> From: Zanoni, Paulo R
> Sent: Wednesday, January 20, 2016 7:35 PM
> To: intel-gfx@lists.freedesktop.org; Davies, Devon
> Cc: Gore, Tim; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_fbc_crc.c: No longer dependant on Cairo A setup function that used to use Cairo to draw 2 rectangles covering the whole screen has been changed to use igt_draw
> 
> Hi
> 
> In our IGT/Kernel culture, patch commit titles (here, presented as the email subject) are usually small (under 70-80 chars) providing a quick summary of what the change does.
> 
> Also, we try to make each patch contain a single logical and complete change. So, for example, one approach you could have taken here would
> be:
>  - patch 1: update lib/igt_fb.c so it compiles on Android without cairo
>  - patch 2: solve the ffs() problem
>  - patch 3: solve the drmModeSetPlane() problem
>  - patch 4: change kms_fbc_crc to not need cairo
>  - patch 5: change the build system so it compiles tests that now work on Android
> 
> Following is another text explaining this pattern:
> http://cgit.freedesktop.org/drm-intel/tree/Documentation/SubmittingPatc
> hes#n201
> 
> 
> Now, regarding the Android vs Cairo macros: I know this has been discussed in the mailing list a few times but I didn't follow it closely, and IGT even has the ANDROID_HAS_CAIRO support, so I'd like Daniel or others to comment on this. Do we want the ifdef dance in igt_fb? Perhaps we could split the libs into igt_fb for the non-cairo parts, and igt_cairo for the cairo parts? The igt_fb library is definitely useful without cairo, so this would make sense to me. But perhaps we want to just keep everything as-is since it's possible to have cairo on Android?
> 
> There are some other comments inline. Please see below.
> 
> 
> Em Sex, 2016-01-15 às 15:42 +0000, devon.davies@intel.com escreveu:
> > From: Devon Davies <devon.davies@intel.com>
> > 
> > tests/kms_frontbuffer_tracking.c: Now builds with DRM_PRIMARY_DISABLE
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > tests/Android.mk: Allow the above tests to be built without Cairo
> >     Simply removed them from the tests the be skipped.
> > 
> > libs/igt_kms.c: Now builds with DRM_PRIMARY_DISABLE
> >     I had to define ffs as __builtin_fss due to compiler issues.
> >     Each call to the function drmModeSetPlane now has an addtional 
> > NULL in the
> >     arguments if DRM_PRIMARY_DISABLE is set.
> > 
> > libs/igt_fb.c: Will now build some functions without Cairo
> >     Functions which aren't used by the framebuffer compression tests 
> > are
> >     now behind an #if (!defined(ANDROID)) || (defined(ANDROID) &&
> >     ANDROID_HAS_CAIRO
> > 
> > libs/Android.mk
> >     igt_fb and igt_kms are no longer ignored if we don't have Cairo.
> > 
> > The tests kms_fbc_crc and kms_frontbuffer_tracking had an unnecessary 
> > dependance on the Cairo graphics engine.
> > Also, drmModeSetPlane may have an additional argument if 
> > DRM_PRIMARY_DISABLE is set (as it was for me), I have fixed that 
> > issue.
> > 
> > Signed-off-by: Devon Davies <devon.davies@intel.com>
> > ---
> >  lib/Android.mk                   |  4 --
> >  lib/igt_fb.c                     | 26 ++++++++++++-
> >  lib/igt_kms.c                    | 15 ++++++--
> >  tests/Android.mk                 |  5 +++
> >  tests/kms_fbc_crc.c              | 20 ++++++----
> >  tests/kms_frontbuffer_tracking.c | 79
> > +++++++++++++++++++++++++++++++++-------
> >  6 files changed, 119 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/Android.mk b/lib/Android.mk index badec1e..bbdb051
> > 100644
> > --- a/lib/Android.mk
> > +++ b/lib/Android.mk
> > @@ -37,10 +37,6 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")
> >      LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-
> > 1.12.16/src
> >      LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\"
> > -DIGT_SRCDIR=\".\"
> >  else
> > -skip_lib_list := \
> > -    igt_kms.c \
> > -    igt_kms.h \
> > -    igt_fb.c
> >      -DANDROID_HAS_CAIRO=0
> >  endif
> >  
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c index c985824..5acdaa7 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -33,6 +33,7 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +
> 
> Please don't add random lines to files.
> 
> 
> >  /**
> >   * SECTION:igt_fb
> >   * @short_description: Framebuffer handling and drawing library @@
> > -52,11 +53,23 @@
> >   */
> >  
> >  /* drm fourcc/cairo format maps */
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  #define DF(did, cid, _bpp, _depth)	\
> >  	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }
> > +
> > +#else
> > +
> > +#define DF(did, cid, _bpp, _depth)	\
> > +	{ DRM_FORMAT_##did, # did, _bpp, _depth }
> > +
> > +#endif
> > +
> >  static struct format_desc_struct {
> >  	uint32_t drm_id;
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_format_t cairo_id;
> > +#endif
> >  	const char *name;
> >  	int bpp;
> >  	int depth;
> > @@ -72,7 +85,6 @@ static struct format_desc_struct {
> >  #define for_each_format(f)	\
> >  	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc);
> > f++)
> >  
> > -
> 
> Please don't remove random lines from files.
> 
> 
> >  /* helpers to create nice-looking framebuffers */
> >  static int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  			    uint64_t tiling, unsigned bo_size, @@ -125,6 +137,8 @@ static 
> > int create_bo_for_fb(int fd, int width, int height, int bpp,
> >  	return ret;
> >  }
> >  
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> > +
> >  /**
> >   * igt_paint_color:
> >   * @cr: cairo drawing context
> > @@ -394,6 +408,7 @@ void igt_paint_image(cairo_t *cr, const char 
> > *filename,
> >  
> >  	fclose(f);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_create_fb_with_bo_size:
> > @@ -494,6 +509,7 @@ unsigned int igt_create_fb(int fd, int width, int 
> > height, uint32_t format,
> >  	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, 
> > fb,
> >  					  0, 0);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_create_color_fb:
> > @@ -985,6 +1001,7 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >  
> >  	igt_assert(status == CAIRO_STATUS_SUCCESS);
> >  }
> > +#endif
> >  
> >  /**
> >   * igt_remove_fb:
> > @@ -997,10 +1014,13 @@ void igt_write_fb_to_png(int fd, struct igt_fb 
> > *fb, const char *filename)
> >   */
> >  void igt_remove_fb(int fd, struct igt_fb *fb)
> >  {
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  	cairo_surface_destroy(fb->cairo_surface);
> > +#endif
> >  	do_or_die(drmModeRmFB(fd, fb->fb_id));
> >  	gem_close(fd, fb->gem_handle);
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_bpp_depth_to_drm_format:
> > @@ -1024,6 +1044,8 @@ uint32_t igt_bpp_depth_to_drm_format(int bpp, 
> > int depth)
> >  		     depth);
> >  }
> >  
> > +#endif
> > +
> >  /**
> >   * igt_drm_format_to_bpp:
> >   * @drm_format: drm fourcc pixel format code @@ -1062,6 +1084,7 @@ 
> > const char *igt_format_str(uint32_t drm_format)
> >  
> >  	return "invalid";
> >  }
> > +#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
> >  
> >  /**
> >   * igt_get_all_formats:
> > @@ -1089,3 +1112,4 @@ void igt_get_all_formats(const uint32_t 
> > **formats, int *format_count)
> >  	*formats = drm_formats;
> >  	*format_count = ARRAY_SIZE(format_desc);
> >  }
> > +#endif
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..7b682cb
> > 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -49,6 +49,8 @@
> >  #include "intel_chipset.h"
> >  #include "igt_debugfs.h"
> >  
> > +#define ffs __builtin_ffs
> 
> A define like this can be dangerous. Shouldn't we just fix all the callers, so code readers know exactly which function is being run?
> 
> On the other hand, it seems that this change will restrict us to gcc- only. So maybe we could just define ffs as __builtin_ffs if ANDROID is defined?
> 
> 
> > +
> >  /* list of connectors that need resetting on exit */
> >  #define MAX_CONNECTORS 32
> >  static char *forced_connectors[MAX_CONNECTORS + 1]; @@ -1354,8
> > +1356,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> >  				      IGT_FIXED(0,0), /* src_x */
> >  				      IGT_FIXED(0,0), /* src_y */
> >  				      IGT_FIXED(0,0), /* src_w */
> > -				      IGT_FIXED(0,0) /* src_h */);
> > -
> > +				      IGT_FIXED(0,0) /* src_h */
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> 
> If we're going this way, maybe the best approach would be to add a wrapper (igt_kms_set_plane?) and make the current callers use it.
> 
> On a side note: why is Android different here!?
> 
> 
> Thanks,
> Paulo
> 
> > +					  );
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	} else if (plane->fb_changed || plane->position_changed ||
> >  		plane->size_changed) {
> > @@ -1386,7 +1391,11 @@ static int igt_drm_plane_commit(igt_plane_t 
> > *plane,
> >  				      crtc_x, crtc_y,
> >  				      crtc_w, crtc_h,
> >  				      src_x, src_y,
> > -				      src_w, src_h);
> > +				      src_w, src_h
> > +#if DRM_PRIMARY_DISABLE
> > +					  , NULL
> > +#endif
> > +					  );
> >  
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	}
> > diff --git a/tests/Android.mk b/tests/Android.mk index
> > 8457125..eb287a6 100644
> > --- a/tests/Android.mk
> > +++ b/tests/Android.mk
> > @@ -65,6 +65,11 @@ else
> >  
> >      tmp_list := $(foreach test_name, $(TESTS_progs_M),\
> >          $(if $(findstring kms_,$(test_name)),$(test_name)))
> > +
> > +# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo
> > +    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))
> > +    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))
> > +
> >      skip_tests_list += $(tmp_list)
> >  
> >      IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0 diff --git 
> > a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 02e95e5..717e891
> > 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -336,14 +336,18 @@ static void create_fbs(data_t *data, bool tiled, 
> > struct igt_fb *fbs)
> >  	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :
> >  				  LOCAL_DRM_FORMAT_MOD_NONE;
> >  
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.0, 0.0, 0.0, &fbs[0]);
> > -	igt_assert(rc);
> > -	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > -				 DRM_FORMAT_XRGB8888, tiling,
> > -				 0.1, 0.1, 0.1, &fbs[1]);
> > -	igt_assert(rc);
> > +	unsigned int fb_id;
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[0]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);
> > +
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode-
> > >vdisplay,
> > +				DRM_FORMAT_XRGB8888, tiling,
> > &fbs[1]);
> > +	igt_assert(fb_id);
> > +	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);
> > +
> >  }
> >  
> >  /* Since we want to be really safe that the CRCs are actually what we 
> > really diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e7acc7c..f8b9eca 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1079,7 +1079,11 @@ static void unset_all_crtcs(void)
> >  
> >  	for (i = 0; i < drm.plane_res->count_planes; i++) {
> >  		rc = drmModeSetPlane(drm.fd, drm.plane_res-
> > >planes[i], 0, 0, 0,
> > -				     0, 0, 0, 0, 0, 0, 0, 0);
> > +				     0, 0, 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +				     , NULL
> > +#endif
> > +			     );
> >  		igt_assert_eq(rc, 0);
> >  	}
> >  }
> > @@ -1715,7 +1719,11 @@ static void set_sprite_for_test(const struct 
> > test_mode *t,
> >  			     params->sprite.fb->fb_id, 0, 0, 0,
> >  			     params->sprite.w, params->sprite.h,
> >  			     0, 0, params->sprite.w << 16,
> > -			     params->sprite.h << 16);
> > +			     params->sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	do_assertions(ASSERT_NO_ACTION_CHANGE);
> > @@ -2220,7 +2228,11 @@ static void set_prim_plane_for_params(struct 
> > modeset_params *params)
> >  			     params->mode->hdisplay,
> >  			     params->mode->vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  }
> >  
> > @@ -2406,7 +2418,11 @@ static void move_subtest(const struct test_mode
> > *t)
> >  					     params->sprite.fb-
> > >fb_id, 0,
> >  					     rect.x, rect.y, rect.w,
> >  					     rect.h, 0, 0, rect.w <<
> > 16,
> > -					     rect.h << 16);
> > +					     rect.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +					     , NULL
> > +#endif
> > +			     );
> >  			igt_assert_eq(rc, 0);
> >  			break;
> >  		default:
> > @@ -2463,8 +2479,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  				break;
> >  			case PLANE_SPR:
> >  				rc = drmModeSetPlane(drm.fd, params-
> > >sprite_id,
> > -						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0,
> > -						     0, 0);
> > +						     0, 0, 0, 0, 0,
> > 0, 0, 0, 0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2489,7 +2508,11 @@ static void onoff_subtest(const struct 
> > test_mode *t)
> >  						     params-
> > >sprite.h, 0,
> >  						     0,
> >  						     params-
> > >sprite.w << 16,
> > -						     params-
> > >sprite.h << 16);
> > +						     params-
> > >sprite.h << 16
> > +#if DRM_PRIMARY_DISABLE
> > +						     , NULL
> > +#endif
> > +			     );
> >  				igt_assert_eq(rc, 0);
> >  				break;
> >  			default:
> > @@ -2561,7 +2584,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,
> >  			     fullscreen_fb.height, 0, 0,
> >  			     fullscreen_fb.width << 16,
> > -			     fullscreen_fb.height << 16);
> > +			     fullscreen_fb.height << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  	update_wanted_crc(t, &pattern->crcs[t->format][0]);
> >  
> > @@ -2581,7 +2608,11 @@ static void fullscreen_plane_subtest(const 
> > struct test_mode *t)
> >  	do_assertions(assertions);
> >  
> >  	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 
> > 0,
> > -			     0, 0, 0);
> > +			     0, 0, 0
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert_eq(rc, 0);
> >  
> >  	if (t->screen == SCREEN_PRIM)
> > @@ -2657,7 +2688,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2668,7 +2703,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2681,7 +2720,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     params->mode->vdisplay / 2,
> >  			     params->fb.x << 16, params->fb.y << 16,
> >  			     (params->fb.w / 2) << 16,
> > -			     (params->fb.h / 2) << 16);
> > +			     (params->fb.h / 2) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2695,7 +2738,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     (params->fb.x + params->fb.w / 2) << 16,
> >  			     (params->fb.y + params->fb.h / 2) << 16,
> >  			     (params->fb.w / 4) << 16,
> > -			     (params->fb.h / 4) << 16);
> > +			     (params->fb.h / 4) << 16
> > +#if DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(DONT_ASSERT_CRC);
> >  
> > @@ -2705,7 +2752,11 @@ static void scaledprimary_subtest(const struct 
> > test_mode *t)
> >  			     0, 0,
> >  			     params->mode->hdisplay, params->mode-
> > >vdisplay,
> >  			     params->fb.x << 16, params->fb.y << 16,
> > -			     params->fb.w << 16, params->fb.h <<
> > 16);
> > +			     params->fb.w << 16, params->fb.h << 16 #if 
> > +DRM_PRIMARY_DISABLE
> > +			     , NULL
> > +#endif
> > +			     );
> >  	igt_assert(rc == 0);
> >  	do_assertions(0);
> >  
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/lib/Android.mk b/lib/Android.mk
index badec1e..bbdb051 100644
--- a/lib/Android.mk
+++ b/lib/Android.mk
@@ -37,10 +37,6 @@  ifeq ("${ANDROID_HAS_CAIRO}", "1")
     LOCAL_C_INCLUDES += $(ANDROID_BUILD_TOP)/external/cairo-1.12.16/src
     LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=1 -DIGT_DATADIR=\".\" -DIGT_SRCDIR=\".\"
 else
-skip_lib_list := \
-    igt_kms.c \
-    igt_kms.h \
-    igt_fb.c
     -DANDROID_HAS_CAIRO=0
 endif
 
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index c985824..5acdaa7 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -33,6 +33,7 @@ 
 #include "igt_fb.h"
 #include "ioctl_wrappers.h"
 
+
 /**
  * SECTION:igt_fb
  * @short_description: Framebuffer handling and drawing library
@@ -52,11 +53,23 @@ 
  */
 
 /* drm fourcc/cairo format maps */
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
+
 #define DF(did, cid, _bpp, _depth)	\
 	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp, _depth }
+
+#else
+
+#define DF(did, cid, _bpp, _depth)	\
+	{ DRM_FORMAT_##did, # did, _bpp, _depth }
+
+#endif
+
 static struct format_desc_struct {
 	uint32_t drm_id;
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
 	cairo_format_t cairo_id;
+#endif
 	const char *name;
 	int bpp;
 	int depth;
@@ -72,7 +85,6 @@  static struct format_desc_struct {
 #define for_each_format(f)	\
 	for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); f++)
 
-
 /* helpers to create nice-looking framebuffers */
 static int create_bo_for_fb(int fd, int width, int height, int bpp,
 			    uint64_t tiling, unsigned bo_size,
@@ -125,6 +137,8 @@  static int create_bo_for_fb(int fd, int width, int height, int bpp,
 	return ret;
 }
 
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
+
 /**
  * igt_paint_color:
  * @cr: cairo drawing context
@@ -394,6 +408,7 @@  void igt_paint_image(cairo_t *cr, const char *filename,
 
 	fclose(f);
 }
+#endif
 
 /**
  * igt_create_fb_with_bo_size:
@@ -494,6 +509,7 @@  unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 	return igt_create_fb_with_bo_size(fd, width, height, format, tiling, fb,
 					  0, 0);
 }
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
 
 /**
  * igt_create_color_fb:
@@ -985,6 +1001,7 @@  void igt_write_fb_to_png(int fd, struct igt_fb *fb, const char *filename)
 
 	igt_assert(status == CAIRO_STATUS_SUCCESS);
 }
+#endif
 
 /**
  * igt_remove_fb:
@@ -997,10 +1014,13 @@  void igt_write_fb_to_png(int fd, struct igt_fb *fb, const char *filename)
  */
 void igt_remove_fb(int fd, struct igt_fb *fb)
 {
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
 	cairo_surface_destroy(fb->cairo_surface);
+#endif
 	do_or_die(drmModeRmFB(fd, fb->fb_id));
 	gem_close(fd, fb->gem_handle);
 }
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
 
 /**
  * igt_bpp_depth_to_drm_format:
@@ -1024,6 +1044,8 @@  uint32_t igt_bpp_depth_to_drm_format(int bpp, int depth)
 		     depth);
 }
 
+#endif
+
 /**
  * igt_drm_format_to_bpp:
  * @drm_format: drm fourcc pixel format code
@@ -1062,6 +1084,7 @@  const char *igt_format_str(uint32_t drm_format)
 
 	return "invalid";
 }
+#if (!defined(ANDROID)) || (defined(ANDROID) && ANDROID_HAS_CAIRO)
 
 /**
  * igt_get_all_formats:
@@ -1089,3 +1112,4 @@  void igt_get_all_formats(const uint32_t **formats, int *format_count)
 	*formats = drm_formats;
 	*format_count = ARRAY_SIZE(format_desc);
 }
+#endif
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 497118a..7b682cb 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -49,6 +49,8 @@ 
 #include "intel_chipset.h"
 #include "igt_debugfs.h"
 
+#define ffs __builtin_ffs
+
 /* list of connectors that need resetting on exit */
 #define MAX_CONNECTORS 32
 static char *forced_connectors[MAX_CONNECTORS + 1];
@@ -1354,8 +1356,11 @@  static int igt_drm_plane_commit(igt_plane_t *plane,
 				      IGT_FIXED(0,0), /* src_x */
 				      IGT_FIXED(0,0), /* src_y */
 				      IGT_FIXED(0,0), /* src_w */
-				      IGT_FIXED(0,0) /* src_h */);
-
+				      IGT_FIXED(0,0) /* src_h */
+#if DRM_PRIMARY_DISABLE
+					  , NULL
+#endif
+					  );
 		CHECK_RETURN(ret, fail_on_error);
 	} else if (plane->fb_changed || plane->position_changed ||
 		plane->size_changed) {
@@ -1386,7 +1391,11 @@  static int igt_drm_plane_commit(igt_plane_t *plane,
 				      crtc_x, crtc_y,
 				      crtc_w, crtc_h,
 				      src_x, src_y,
-				      src_w, src_h);
+				      src_w, src_h
+#if DRM_PRIMARY_DISABLE
+					  , NULL
+#endif
+					  );
 
 		CHECK_RETURN(ret, fail_on_error);
 	}
diff --git a/tests/Android.mk b/tests/Android.mk
index 8457125..eb287a6 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -65,6 +65,11 @@  else
 
     tmp_list := $(foreach test_name, $(TESTS_progs_M),\
         $(if $(findstring kms_,$(test_name)),$(test_name)))
+
+# kms_fbc_crc and kms_frontbuffer_tracking no longer depend on Cairo
+    tmp_list := $(filter-out kms_fbc_crc, $(tmp_list))
+    tmp_list := $(filter-out kms_frontbuffer_tracking, $(tmp_list))
+
     skip_tests_list += $(tmp_list)
 
     IGT_LOCAL_CFLAGS += -DANDROID_HAS_CAIRO=0
diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
index 02e95e5..717e891 100644
--- a/tests/kms_fbc_crc.c
+++ b/tests/kms_fbc_crc.c
@@ -336,14 +336,18 @@  static void create_fbs(data_t *data, bool tiled, struct igt_fb *fbs)
 	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED :
 				  LOCAL_DRM_FORMAT_MOD_NONE;
 
-	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-				 DRM_FORMAT_XRGB8888, tiling,
-				 0.0, 0.0, 0.0, &fbs[0]);
-	igt_assert(rc);
-	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-				 DRM_FORMAT_XRGB8888, tiling,
-				 0.1, 0.1, 0.1, &fbs[1]);
-	igt_assert(rc);
+	unsigned int fb_id;
+
+	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+				DRM_FORMAT_XRGB8888, tiling, &fbs[0]);
+	igt_assert(fb_id);
+	igt_draw_fill_fb(data->drm_fd, &fbs[0], 0);
+
+	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+				DRM_FORMAT_XRGB8888, tiling, &fbs[1]);
+	igt_assert(fb_id);
+	igt_draw_fill_fb(data->drm_fd, &fbs[1], 0x77);
+
 }
 
 /* Since we want to be really safe that the CRCs are actually what we really
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index e7acc7c..f8b9eca 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1079,7 +1079,11 @@  static void unset_all_crtcs(void)
 
 	for (i = 0; i < drm.plane_res->count_planes; i++) {
 		rc = drmModeSetPlane(drm.fd, drm.plane_res->planes[i], 0, 0, 0,
-				     0, 0, 0, 0, 0, 0, 0, 0);
+				     0, 0, 0, 0, 0, 0, 0, 0
+#if DRM_PRIMARY_DISABLE
+				     , NULL
+#endif
+			     );
 		igt_assert_eq(rc, 0);
 	}
 }
@@ -1715,7 +1719,11 @@  static void set_sprite_for_test(const struct test_mode *t,
 			     params->sprite.fb->fb_id, 0, 0, 0,
 			     params->sprite.w, params->sprite.h,
 			     0, 0, params->sprite.w << 16,
-			     params->sprite.h << 16);
+			     params->sprite.h << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert_eq(rc, 0);
 
 	do_assertions(ASSERT_NO_ACTION_CHANGE);
@@ -2220,7 +2228,11 @@  static void set_prim_plane_for_params(struct modeset_params *params)
 			     params->mode->hdisplay,
 			     params->mode->vdisplay,
 			     params->fb.x << 16, params->fb.y << 16,
-			     params->fb.w << 16, params->fb.h << 16);
+			     params->fb.w << 16, params->fb.h << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 }
 
@@ -2406,7 +2418,11 @@  static void move_subtest(const struct test_mode *t)
 					     params->sprite.fb->fb_id, 0,
 					     rect.x, rect.y, rect.w,
 					     rect.h, 0, 0, rect.w << 16,
-					     rect.h << 16);
+					     rect.h << 16
+#if DRM_PRIMARY_DISABLE
+					     , NULL
+#endif
+			     );
 			igt_assert_eq(rc, 0);
 			break;
 		default:
@@ -2463,8 +2479,11 @@  static void onoff_subtest(const struct test_mode *t)
 				break;
 			case PLANE_SPR:
 				rc = drmModeSetPlane(drm.fd, params->sprite_id,
-						     0, 0, 0, 0, 0, 0, 0, 0, 0,
-						     0, 0);
+						     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+#if DRM_PRIMARY_DISABLE
+						     , NULL
+#endif
+			     );
 				igt_assert_eq(rc, 0);
 				break;
 			default:
@@ -2489,7 +2508,11 @@  static void onoff_subtest(const struct test_mode *t)
 						     params->sprite.h, 0,
 						     0,
 						     params->sprite.w << 16,
-						     params->sprite.h << 16);
+						     params->sprite.h << 16
+#if DRM_PRIMARY_DISABLE
+						     , NULL
+#endif
+			     );
 				igt_assert_eq(rc, 0);
 				break;
 			default:
@@ -2561,7 +2584,11 @@  static void fullscreen_plane_subtest(const struct test_mode *t)
 			     fullscreen_fb.fb_id, 0, 0, 0, fullscreen_fb.width,
 			     fullscreen_fb.height, 0, 0,
 			     fullscreen_fb.width << 16,
-			     fullscreen_fb.height << 16);
+			     fullscreen_fb.height << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert_eq(rc, 0);
 	update_wanted_crc(t, &pattern->crcs[t->format][0]);
 
@@ -2581,7 +2608,11 @@  static void fullscreen_plane_subtest(const struct test_mode *t)
 	do_assertions(assertions);
 
 	rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 0,
-			     0, 0, 0);
+			     0, 0, 0
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert_eq(rc, 0);
 
 	if (t->screen == SCREEN_PRIM)
@@ -2657,7 +2688,11 @@  static void scaledprimary_subtest(const struct test_mode *t)
 			     0, 0,
 			     params->mode->hdisplay, params->mode->vdisplay,
 			     params->fb.x << 16, params->fb.y << 16,
-			     params->fb.w << 16, params->fb.h << 16);
+			     params->fb.w << 16, params->fb.h << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 	do_assertions(DONT_ASSERT_CRC);
 
@@ -2668,7 +2703,11 @@  static void scaledprimary_subtest(const struct test_mode *t)
 			     params->mode->hdisplay, params->mode->vdisplay,
 			     params->fb.x << 16, params->fb.y << 16,
 			     (params->fb.w / 2) << 16,
-			     (params->fb.h / 2) << 16);
+			     (params->fb.h / 2) << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 	do_assertions(DONT_ASSERT_CRC);
 
@@ -2681,7 +2720,11 @@  static void scaledprimary_subtest(const struct test_mode *t)
 			     params->mode->vdisplay / 2,
 			     params->fb.x << 16, params->fb.y << 16,
 			     (params->fb.w / 2) << 16,
-			     (params->fb.h / 2) << 16);
+			     (params->fb.h / 2) << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 	do_assertions(DONT_ASSERT_CRC);
 
@@ -2695,7 +2738,11 @@  static void scaledprimary_subtest(const struct test_mode *t)
 			     (params->fb.x + params->fb.w / 2) << 16,
 			     (params->fb.y + params->fb.h / 2) << 16,
 			     (params->fb.w / 4) << 16,
-			     (params->fb.h / 4) << 16);
+			     (params->fb.h / 4) << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 	do_assertions(DONT_ASSERT_CRC);
 
@@ -2705,7 +2752,11 @@  static void scaledprimary_subtest(const struct test_mode *t)
 			     0, 0,
 			     params->mode->hdisplay, params->mode->vdisplay,
 			     params->fb.x << 16, params->fb.y << 16,
-			     params->fb.w << 16, params->fb.h << 16);
+			     params->fb.w << 16, params->fb.h << 16
+#if DRM_PRIMARY_DISABLE
+			     , NULL
+#endif
+			     );
 	igt_assert(rc == 0);
 	do_assertions(0);