[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
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Wed Jan 20 11:34:35 PST 2016
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 at intel.com escreveu:
> From: Devon Davies <devon.davies at 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 at 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);
>
More information about the Intel-gfx
mailing list