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.dav...@intel.com escreveu: > > From: Devon Davies <devon.dav...@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.dav...@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 Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
