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@xxxxxxxxxxxxxxxxxxxxx; Davies, Devon Cc: Gore, Tim; Vetter, Daniel Subject: Re: [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@xxxxxxxxx escreveu: > From: Devon Davies <devon.davies@xxxxxxxxx> > > 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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
