[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
Daniel Vetter
daniel at ffwll.ch
Fri Jan 22 09:14:31 PST 2016
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 at 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 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);
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list