| Message ID | 1423068134-12884-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 04, 2015 at 04:42:14PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Just a few basic tests to make sure fb modifiers can be used and > behave sanely when mixed with the old set_tiling API. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > lib/ioctl_wrappers.c | 45 ++++++++++++++++++++++++++++++++++++++ > lib/ioctl_wrappers.h | 36 ++++++++++++++++++++++++++++++ > tests/kms_addfb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 143 insertions(+) > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > index 19a457a..bca6d2a 100644 > --- a/lib/ioctl_wrappers.c > +++ b/lib/ioctl_wrappers.c > @@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param) > > return gem_context_get_param(fd, &p) == 0; > } > + gtkdoc is missing here. Easiest way to avoid it is to just move these two wrappers into the kms_fb testcase. Whomever needs to reuse these gets to write the docs. > +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, > + uint32_t pixel_format, uint32_t bo_handles[4], > + uint32_t pitches[4], uint32_t offsets[4], > + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags) > +{ > + struct local_drm_mode_fb_cmd2 f; > + int ret; > + > + f.width = width; > + f.height = height; > + f.pixel_format = pixel_format; > + f.flags = flags; > + > + memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0])); > + memcpy(f.pitches, pitches, 4 * sizeof(pitches[0])); > + memcpy(f.offsets, offsets, 4 * sizeof(offsets[0])); > + memcpy(f.modifier, modifier, 4 * sizeof(modifier[0])); > + > + if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f))) > + return ret < 0 ? -errno : ret; > + > + *buf_id = f.fb_id; > + return 0; > +} > + > +unsigned int has_drm_fb_modifiers(int fd) > +{ > + static unsigned int has_modifiers, cap_modifiers_tested; > + uint64_t cap_modifiers; > + int ret; > + > + if (cap_modifiers_tested) > + return has_modifiers; > + > + ret = drmGetCap(fd, LOCAL_DRM_CAP_ADDFB2_MODIFIERS, &cap_modifiers); > + igt_assert(ret == 0 || errno == EINVAL); > + has_modifiers = ret == 0 && cap_modifiers == 1; > + cap_modifiers_tested = 1; > + > + if (has_modifiers) > + igt_debug("DRM_CAP_ADDFB2_MODIFIERS\n"); > + > + return has_modifiers; > +} > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h > index 30ab836..c277012 100644 > --- a/lib/ioctl_wrappers.h > +++ b/lib/ioctl_wrappers.h > @@ -117,4 +117,40 @@ int gem_context_has_param(int fd, uint64_t param); > int gem_context_get_param(int fd, struct local_i915_gem_context_param *p); > int gem_context_set_param(int fd, struct local_i915_gem_context_param *p); > > +struct local_drm_mode_fb_cmd2 { > + uint32_t fb_id; > + uint32_t width, height; > + uint32_t pixel_format; > + uint32_t flags; > + uint32_t handles[4]; > + uint32_t pitches[4]; > + uint32_t offsets[4]; > + uint64_t modifier[4]; > +}; > + > +#define LOCAL_DRM_MODE_FB_MODIFIERS (1<<1) > + > +#define LOCAL_DRM_FORMAT_MOD_VENDOR_INTEL 0x01 > + > +#define local_fourcc_mod_code(vendor, val) \ > + ((((uint64_t)LOCAL_DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | \ > + (val & 0x00ffffffffffffffL)) > + > +#define LOCAL_I915_FORMAT_MOD_NONE local_fourcc_mod_code(INTEL, \ > + 0x00000000000000L) > +#define LOCAL_I915_FORMAT_MOD_X_TILED local_fourcc_mod_code(INTEL, \ > + 0x00000000000001L) > + > +#define LOCAL_DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, \ > + struct local_drm_mode_fb_cmd2) > + > +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, > + uint32_t pixel_format, uint32_t bo_handles[4], > + uint32_t pitches[4], uint32_t offsets[4], > + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags); > + > +#define LOCAL_DRM_CAP_ADDFB2_MODIFIERS 0x10 > + > +unsigned int has_drm_fb_modifiers(int fd); > + > #endif /* IOCTL_WRAPPERS_H */ > diff --git a/tests/kms_addfb.c b/tests/kms_addfb.c > index 756589e..9b0f77c 100644 > --- a/tests/kms_addfb.c > +++ b/tests/kms_addfb.c > @@ -213,6 +213,66 @@ static void size_tests(int fd) > } > } > > +static void addfb25_tests(int fd) > +{ > + struct local_drm_mode_fb_cmd2 f = {}; > + > + > + igt_require(has_drm_fb_modifiers(fd)); Imo better to move the igt_require int igt_main so that ppl aren't suprised that they're tests are skipping when they add more below the call to addfb25_tests. > + > + memset(&f, 0, sizeof(f)); > + > + f.width = 1024; > + f.height = 1024; > + f.pixel_format = DRM_FORMAT_XRGB8888; > + f.pitches[0] = 1024*4; > + f.flags = LOCAL_DRM_MODE_FB_MODIFIERS; > + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; c99 initializes nicely combine the memset with the explicit struct init. But just a bikeshed. > + > + igt_fixture { > + gem_bo = gem_create(fd, 1024*1024*4); > + igt_assert(gem_bo); > + } > + > + f.handles[0] = gem_bo; > + > + igt_subtest("addfb25-X-tiled") { > + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; > + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > + f.fb_id = 0; > + } > + > + igt_subtest("addfb25-framebuffer-vs-set-tiling") { > + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY); > + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4) == -EBUSY); > + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > + f.fb_id = 0; > + } > + > + igt_fixture > + gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4); > + f.pitches[0] = 1024*4; > + > + igt_subtest("addfb25-X-tiled-both") { > + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; > + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > + f.fb_id = 0; > + } > + > + igt_subtest("addfb25-X-tiled-mismatch") { > + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; > + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL); > + f.fb_id = 0; > + } Looks good, but two abuse cases are imo missing: - f.flags = 0 but f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED - f.flags = LOCAL_DRM_MODE_FB_MODIFIERS but f.modifier[0] = 0 Just to make sure we have that part of the addfb extension covered, too. Cheers, Daniel > + > + igt_fixture { > + gem_close(fd, gem_bo); > + } > +} > + > int fd; > > igt_main > @@ -224,6 +284,8 @@ igt_main > > size_tests(fd); > > + addfb25_tests(fd); > + > igt_fixture > close(fd); > } > -- > 2.2.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 02/05/2015 02:21 PM, Daniel Vetter wrote: > On Wed, Feb 04, 2015 at 04:42:14PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Just a few basic tests to make sure fb modifiers can be used and >> behave sanely when mixed with the old set_tiling API. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> lib/ioctl_wrappers.c | 45 ++++++++++++++++++++++++++++++++++++++ >> lib/ioctl_wrappers.h | 36 ++++++++++++++++++++++++++++++ >> tests/kms_addfb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 143 insertions(+) >> >> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >> index 19a457a..bca6d2a 100644 >> --- a/lib/ioctl_wrappers.c >> +++ b/lib/ioctl_wrappers.c >> @@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param) >> >> return gem_context_get_param(fd, &p) == 0; >> } >> + > > gtkdoc is missing here. Easiest way to avoid it is to just move these two > wrappers into the kms_fb testcase. Whomever needs to reuse these gets to > write the docs. I'll need it so will fix myself. >> +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, >> + uint32_t pixel_format, uint32_t bo_handles[4], >> + uint32_t pitches[4], uint32_t offsets[4], >> + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags) >> +{ >> + struct local_drm_mode_fb_cmd2 f; >> + int ret; >> + >> + f.width = width; >> + f.height = height; >> + f.pixel_format = pixel_format; >> + f.flags = flags; >> + >> + memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0])); >> + memcpy(f.pitches, pitches, 4 * sizeof(pitches[0])); >> + memcpy(f.offsets, offsets, 4 * sizeof(offsets[0])); >> + memcpy(f.modifier, modifier, 4 * sizeof(modifier[0])); >> + >> + if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f))) >> + return ret < 0 ? -errno : ret; >> + >> + *buf_id = f.fb_id; >> + return 0; >> +} >> + >> +unsigned int has_drm_fb_modifiers(int fd) >> +{ >> + static unsigned int has_modifiers, cap_modifiers_tested; >> + uint64_t cap_modifiers; >> + int ret; >> + >> + if (cap_modifiers_tested) >> + return has_modifiers; >> + >> + ret = drmGetCap(fd, LOCAL_DRM_CAP_ADDFB2_MODIFIERS, &cap_modifiers); >> + igt_assert(ret == 0 || errno == EINVAL); >> + has_modifiers = ret == 0 && cap_modifiers == 1; >> + cap_modifiers_tested = 1; >> + >> + if (has_modifiers) >> + igt_debug("DRM_CAP_ADDFB2_MODIFIERS\n"); >> + >> + return has_modifiers; >> +} >> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h >> index 30ab836..c277012 100644 >> --- a/lib/ioctl_wrappers.h >> +++ b/lib/ioctl_wrappers.h >> @@ -117,4 +117,40 @@ int gem_context_has_param(int fd, uint64_t param); >> int gem_context_get_param(int fd, struct local_i915_gem_context_param *p); >> int gem_context_set_param(int fd, struct local_i915_gem_context_param *p); >> >> +struct local_drm_mode_fb_cmd2 { >> + uint32_t fb_id; >> + uint32_t width, height; >> + uint32_t pixel_format; >> + uint32_t flags; >> + uint32_t handles[4]; >> + uint32_t pitches[4]; >> + uint32_t offsets[4]; >> + uint64_t modifier[4]; >> +}; >> + >> +#define LOCAL_DRM_MODE_FB_MODIFIERS (1<<1) >> + >> +#define LOCAL_DRM_FORMAT_MOD_VENDOR_INTEL 0x01 >> + >> +#define local_fourcc_mod_code(vendor, val) \ >> + ((((uint64_t)LOCAL_DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | \ >> + (val & 0x00ffffffffffffffL)) >> + >> +#define LOCAL_I915_FORMAT_MOD_NONE local_fourcc_mod_code(INTEL, \ >> + 0x00000000000000L) >> +#define LOCAL_I915_FORMAT_MOD_X_TILED local_fourcc_mod_code(INTEL, \ >> + 0x00000000000001L) >> + >> +#define LOCAL_DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, \ >> + struct local_drm_mode_fb_cmd2) >> + >> +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, >> + uint32_t pixel_format, uint32_t bo_handles[4], >> + uint32_t pitches[4], uint32_t offsets[4], >> + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags); >> + >> +#define LOCAL_DRM_CAP_ADDFB2_MODIFIERS 0x10 >> + >> +unsigned int has_drm_fb_modifiers(int fd); >> + >> #endif /* IOCTL_WRAPPERS_H */ >> diff --git a/tests/kms_addfb.c b/tests/kms_addfb.c >> index 756589e..9b0f77c 100644 >> --- a/tests/kms_addfb.c >> +++ b/tests/kms_addfb.c >> @@ -213,6 +213,66 @@ static void size_tests(int fd) >> } >> } >> >> +static void addfb25_tests(int fd) >> +{ >> + struct local_drm_mode_fb_cmd2 f = {}; >> + >> + >> + igt_require(has_drm_fb_modifiers(fd)); > > Imo better to move the igt_require int igt_main so that ppl aren't > suprised that they're tests are skipping when they add more below the call > to addfb25_tests. Skipping individual test cases was my intention, but I overlooked the fact this function is not a subtest... I suppose igt_subtest in igt_subtest is not possible. :) >> + >> + memset(&f, 0, sizeof(f)); >> + >> + f.width = 1024; >> + f.height = 1024; >> + f.pixel_format = DRM_FORMAT_XRGB8888; >> + f.pitches[0] = 1024*4; >> + f.flags = LOCAL_DRM_MODE_FB_MODIFIERS; >> + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; > > c99 initializes nicely combine the memset with the explicit struct init. > But just a bikeshed. I just copy pasted from libdrm, presumably at some point similar code will appear there and then after some years this can be removed. :) >> + >> + igt_fixture { >> + gem_bo = gem_create(fd, 1024*1024*4); >> + igt_assert(gem_bo); >> + } >> + >> + f.handles[0] = gem_bo; >> + >> + igt_subtest("addfb25-X-tiled") { >> + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; >> + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); >> + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); >> + f.fb_id = 0; >> + } >> + >> + igt_subtest("addfb25-framebuffer-vs-set-tiling") { >> + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); >> + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY); >> + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4) == -EBUSY); >> + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); >> + f.fb_id = 0; >> + } >> + >> + igt_fixture >> + gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4); >> + f.pitches[0] = 1024*4; >> + >> + igt_subtest("addfb25-X-tiled-both") { >> + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; >> + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); >> + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); >> + f.fb_id = 0; >> + } >> + >> + igt_subtest("addfb25-X-tiled-mismatch") { >> + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; >> + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL); >> + f.fb_id = 0; >> + } > > Looks good, but two abuse cases are imo missing: > > - f.flags = 0 but f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED > - f.flags = LOCAL_DRM_MODE_FB_MODIFIERS but f.modifier[0] = 0 > > Just to make sure we have that part of the addfb extension covered, too. Test cases did look suspiciously few to me yesterday but obviously testing mindset wasn't fully on - you are right for those two. Thanks, Tvrtko
On Thu, Feb 05, 2015 at 02:50:09PM +0000, Tvrtko Ursulin wrote: > > On 02/05/2015 02:21 PM, Daniel Vetter wrote: > >On Wed, Feb 04, 2015 at 04:42:14PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Just a few basic tests to make sure fb modifiers can be used and > >>behave sanely when mixed with the old set_tiling API. > >> > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>--- > >> lib/ioctl_wrappers.c | 45 ++++++++++++++++++++++++++++++++++++++ > >> lib/ioctl_wrappers.h | 36 ++++++++++++++++++++++++++++++ > >> tests/kms_addfb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 143 insertions(+) > >> > >>diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > >>index 19a457a..bca6d2a 100644 > >>--- a/lib/ioctl_wrappers.c > >>+++ b/lib/ioctl_wrappers.c > >>@@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param) > >> > >> return gem_context_get_param(fd, &p) == 0; > >> } > >>+ > > > >gtkdoc is missing here. Easiest way to avoid it is to just move these two > >wrappers into the kms_fb testcase. Whomever needs to reuse these gets to > >write the docs. > > I'll need it so will fix myself. A then we'll need to do some proper abi polish I think. > > >>+int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, > >>+ uint32_t pixel_format, uint32_t bo_handles[4], > >>+ uint32_t pitches[4], uint32_t offsets[4], > >>+ uint64_t modifier[0], uint32_t *buf_id, uint32_t flags) No camelcase for library functions. Also I think we want a kms_ prefix here for a bit of namespacing. And since your test only creates framebuffers with 1 plane I think simplifying the function interface would be good to. Finally we use a __ prefix for raw functions (i.e. those returning errno) in the ioctl wrapper library. And I'd put the bo handle right after the fd to be more consistent with other functions. And usually we put out-parameters last. So int __kms_addfb(int fd, unint32_t handle, width, height, stride, pixel_format, modifier, flags, uint32_t *buf_id); > >>+{ > >>+ struct local_drm_mode_fb_cmd2 f; > >>+ int ret; > >>+ > >>+ f.width = width; > >>+ f.height = height; > >>+ f.pixel_format = pixel_format; > >>+ f.flags = flags; > >>+ > >>+ memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0])); > >>+ memcpy(f.pitches, pitches, 4 * sizeof(pitches[0])); > >>+ memcpy(f.offsets, offsets, 4 * sizeof(offsets[0])); > >>+ memcpy(f.modifier, modifier, 4 * sizeof(modifier[0])); > >>+ > >>+ if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f))) > >>+ return ret < 0 ? -errno : ret; > >>+ > >>+ *buf_id = f.fb_id; > >>+ return 0; > >>+} > >>+ > >>+unsigned int has_drm_fb_modifiers(int fd) Common patterin is to pusht he require into this function and use a void return value. Also drm prefix is used for core drm and libdrm stuff, better to pick kms_. So void drm_require_fb_modifiers(int fd); Also please make sure the new kms_ funcs are grouped into a separate section, so that we can easily split them out. I've just noticed that the new context stuff didn't follow that, so will fix that up asap. -Daniel > >>+{ > >>+ static unsigned int has_modifiers, cap_modifiers_tested; > >>+ uint64_t cap_modifiers; > >>+ int ret; > >>+ > >>+ if (cap_modifiers_tested) > >>+ return has_modifiers; > >>+ > >>+ ret = drmGetCap(fd, LOCAL_DRM_CAP_ADDFB2_MODIFIERS, &cap_modifiers); > >>+ igt_assert(ret == 0 || errno == EINVAL); > >>+ has_modifiers = ret == 0 && cap_modifiers == 1; > >>+ cap_modifiers_tested = 1; > >>+ > >>+ if (has_modifiers) > >>+ igt_debug("DRM_CAP_ADDFB2_MODIFIERS\n"); > >>+ > >>+ return has_modifiers; > >>+} > >>diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h > >>index 30ab836..c277012 100644 > >>--- a/lib/ioctl_wrappers.h > >>+++ b/lib/ioctl_wrappers.h > >>@@ -117,4 +117,40 @@ int gem_context_has_param(int fd, uint64_t param); > >> int gem_context_get_param(int fd, struct local_i915_gem_context_param *p); > >> int gem_context_set_param(int fd, struct local_i915_gem_context_param *p); > >> > >>+struct local_drm_mode_fb_cmd2 { > >>+ uint32_t fb_id; > >>+ uint32_t width, height; > >>+ uint32_t pixel_format; > >>+ uint32_t flags; > >>+ uint32_t handles[4]; > >>+ uint32_t pitches[4]; > >>+ uint32_t offsets[4]; > >>+ uint64_t modifier[4]; > >>+}; > >>+ > >>+#define LOCAL_DRM_MODE_FB_MODIFIERS (1<<1) > >>+ > >>+#define LOCAL_DRM_FORMAT_MOD_VENDOR_INTEL 0x01 > >>+ > >>+#define local_fourcc_mod_code(vendor, val) \ > >>+ ((((uint64_t)LOCAL_DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | \ > >>+ (val & 0x00ffffffffffffffL)) > >>+ > >>+#define LOCAL_I915_FORMAT_MOD_NONE local_fourcc_mod_code(INTEL, \ > >>+ 0x00000000000000L) > >>+#define LOCAL_I915_FORMAT_MOD_X_TILED local_fourcc_mod_code(INTEL, \ > >>+ 0x00000000000001L) > >>+ > >>+#define LOCAL_DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, \ > >>+ struct local_drm_mode_fb_cmd2) > >>+ > >>+int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, > >>+ uint32_t pixel_format, uint32_t bo_handles[4], > >>+ uint32_t pitches[4], uint32_t offsets[4], > >>+ uint64_t modifier[0], uint32_t *buf_id, uint32_t flags); > >>+ > >>+#define LOCAL_DRM_CAP_ADDFB2_MODIFIERS 0x10 > >>+ > >>+unsigned int has_drm_fb_modifiers(int fd); > >>+ > >> #endif /* IOCTL_WRAPPERS_H */ > >>diff --git a/tests/kms_addfb.c b/tests/kms_addfb.c > >>index 756589e..9b0f77c 100644 > >>--- a/tests/kms_addfb.c > >>+++ b/tests/kms_addfb.c > >>@@ -213,6 +213,66 @@ static void size_tests(int fd) > >> } > >> } > >> > >>+static void addfb25_tests(int fd) > >>+{ > >>+ struct local_drm_mode_fb_cmd2 f = {}; > >>+ > >>+ > >>+ igt_require(has_drm_fb_modifiers(fd)); > > > >Imo better to move the igt_require int igt_main so that ppl aren't > >suprised that they're tests are skipping when they add more below the call > >to addfb25_tests. > > Skipping individual test cases was my intention, but I overlooked the fact > this function is not a subtest... I suppose igt_subtest in igt_subtest is > not possible. :) > > >>+ > >>+ memset(&f, 0, sizeof(f)); > >>+ > >>+ f.width = 1024; > >>+ f.height = 1024; > >>+ f.pixel_format = DRM_FORMAT_XRGB8888; > >>+ f.pitches[0] = 1024*4; > >>+ f.flags = LOCAL_DRM_MODE_FB_MODIFIERS; > >>+ f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; > > > >c99 initializes nicely combine the memset with the explicit struct init. > >But just a bikeshed. > > I just copy pasted from libdrm, presumably at some point similar code will > appear there and then after some years this can be removed. :) > > >>+ > >>+ igt_fixture { > >>+ gem_bo = gem_create(fd, 1024*1024*4); > >>+ igt_assert(gem_bo); > >>+ } > >>+ > >>+ f.handles[0] = gem_bo; > >>+ > >>+ igt_subtest("addfb25-X-tiled") { > >>+ f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; > >>+ igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > >>+ igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > >>+ f.fb_id = 0; > >>+ } > >>+ > >>+ igt_subtest("addfb25-framebuffer-vs-set-tiling") { > >>+ igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > >>+ igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY); > >>+ igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4) == -EBUSY); > >>+ igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > >>+ f.fb_id = 0; > >>+ } > >>+ > >>+ igt_fixture > >>+ gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4); > >>+ f.pitches[0] = 1024*4; > >>+ > >>+ igt_subtest("addfb25-X-tiled-both") { > >>+ f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; > >>+ igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); > >>+ igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); > >>+ f.fb_id = 0; > >>+ } > >>+ > >>+ igt_subtest("addfb25-X-tiled-mismatch") { > >>+ f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; > >>+ igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL); > >>+ f.fb_id = 0; > >>+ } > > > >Looks good, but two abuse cases are imo missing: > > > >- f.flags = 0 but f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED > >- f.flags = LOCAL_DRM_MODE_FB_MODIFIERS but f.modifier[0] = 0 > > > >Just to make sure we have that part of the addfb extension covered, too. > > Test cases did look suspiciously few to me yesterday but obviously testing > mindset wasn't fully on - you are right for those two. > > Thanks, > > Tvrtko
On 02/06/2015 09:33 AM, Daniel Vetter wrote: > On Thu, Feb 05, 2015 at 02:50:09PM +0000, Tvrtko Ursulin wrote: >> >> On 02/05/2015 02:21 PM, Daniel Vetter wrote: >>> On Wed, Feb 04, 2015 at 04:42:14PM +0000, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> Just a few basic tests to make sure fb modifiers can be used and >>>> behave sanely when mixed with the old set_tiling API. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> --- >>>> lib/ioctl_wrappers.c | 45 ++++++++++++++++++++++++++++++++++++++ >>>> lib/ioctl_wrappers.h | 36 ++++++++++++++++++++++++++++++ >>>> tests/kms_addfb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 143 insertions(+) >>>> >>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >>>> index 19a457a..bca6d2a 100644 >>>> --- a/lib/ioctl_wrappers.c >>>> +++ b/lib/ioctl_wrappers.c >>>> @@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param) >>>> >>>> return gem_context_get_param(fd, &p) == 0; >>>> } >>>> + >>> >>> gtkdoc is missing here. Easiest way to avoid it is to just move these two >>> wrappers into the kms_fb testcase. Whomever needs to reuse these gets to >>> write the docs. >> >> I'll need it so will fix myself. > > A then we'll need to do some proper abi polish I think. >> >>>> +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, >>>> + uint32_t pixel_format, uint32_t bo_handles[4], >>>> + uint32_t pitches[4], uint32_t offsets[4], >>>> + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags) > > No camelcase for library functions. Also I think we want a kms_ prefix > here for a bit of namespacing. And since your test only creates > framebuffers with 1 plane I think simplifying the function interface would > be good to. Finally we use a __ prefix for raw functions (i.e. those > returning errno) in the ioctl wrapper library. And I'd put the bo handle > right after the fd to be more consistent with other functions. And usually > we put out-parameters last. So > > int __kms_addfb(int fd, unint32_t handle, > width, height, stride, pixel_format, modifier, flags, > uint32_t *buf_id); I wanted to stay close to the drmModeAddFB2 as in libdrm, expecting new flavour for fb modifiers there will stay close to that API. Benefit would be trivial edits to move over to the official API and ability to drop the internal copy&paste when that happens. And looked more convenient for users like igt_create_fb_with_bo_size who are using drmModeAddFB2 today, and are expected to grow features in the future. Seems trivial to me, but if you feel strongly about this sure I'll change it. >>>> +{ >>>> + struct local_drm_mode_fb_cmd2 f; >>>> + int ret; >>>> + >>>> + f.width = width; >>>> + f.height = height; >>>> + f.pixel_format = pixel_format; >>>> + f.flags = flags; >>>> + >>>> + memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0])); >>>> + memcpy(f.pitches, pitches, 4 * sizeof(pitches[0])); >>>> + memcpy(f.offsets, offsets, 4 * sizeof(offsets[0])); >>>> + memcpy(f.modifier, modifier, 4 * sizeof(modifier[0])); >>>> + >>>> + if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f))) >>>> + return ret < 0 ? -errno : ret; >>>> + >>>> + *buf_id = f.fb_id; >>>> + return 0; >>>> +} >>>> + >>>> +unsigned int has_drm_fb_modifiers(int fd) > > Common patterin is to pusht he require into this function and use a void > return value. Also drm prefix is used for core drm and libdrm stuff, > better to pick kms_. So Yes if I wanted it to be called igt_require_fb_modifiers, but it wasn't exactly the case. Idea was slightly different usage in future work, outside the kms_addfb test. Regards, Tvrtko
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 19a457a..bca6d2a 100644 --- a/lib/ioctl_wrappers.c +++ b/lib/ioctl_wrappers.c @@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param) return gem_context_get_param(fd, &p) == 0; } + +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, + uint32_t pixel_format, uint32_t bo_handles[4], + uint32_t pitches[4], uint32_t offsets[4], + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags) +{ + struct local_drm_mode_fb_cmd2 f; + int ret; + + f.width = width; + f.height = height; + f.pixel_format = pixel_format; + f.flags = flags; + + memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0])); + memcpy(f.pitches, pitches, 4 * sizeof(pitches[0])); + memcpy(f.offsets, offsets, 4 * sizeof(offsets[0])); + memcpy(f.modifier, modifier, 4 * sizeof(modifier[0])); + + if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f))) + return ret < 0 ? -errno : ret; + + *buf_id = f.fb_id; + return 0; +} + +unsigned int has_drm_fb_modifiers(int fd) +{ + static unsigned int has_modifiers, cap_modifiers_tested; + uint64_t cap_modifiers; + int ret; + + if (cap_modifiers_tested) + return has_modifiers; + + ret = drmGetCap(fd, LOCAL_DRM_CAP_ADDFB2_MODIFIERS, &cap_modifiers); + igt_assert(ret == 0 || errno == EINVAL); + has_modifiers = ret == 0 && cap_modifiers == 1; + cap_modifiers_tested = 1; + + if (has_modifiers) + igt_debug("DRM_CAP_ADDFB2_MODIFIERS\n"); + + return has_modifiers; +} diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index 30ab836..c277012 100644 --- a/lib/ioctl_wrappers.h +++ b/lib/ioctl_wrappers.h @@ -117,4 +117,40 @@ int gem_context_has_param(int fd, uint64_t param); int gem_context_get_param(int fd, struct local_i915_gem_context_param *p); int gem_context_set_param(int fd, struct local_i915_gem_context_param *p); +struct local_drm_mode_fb_cmd2 { + uint32_t fb_id; + uint32_t width, height; + uint32_t pixel_format; + uint32_t flags; + uint32_t handles[4]; + uint32_t pitches[4]; + uint32_t offsets[4]; + uint64_t modifier[4]; +}; + +#define LOCAL_DRM_MODE_FB_MODIFIERS (1<<1) + +#define LOCAL_DRM_FORMAT_MOD_VENDOR_INTEL 0x01 + +#define local_fourcc_mod_code(vendor, val) \ + ((((uint64_t)LOCAL_DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | \ + (val & 0x00ffffffffffffffL)) + +#define LOCAL_I915_FORMAT_MOD_NONE local_fourcc_mod_code(INTEL, \ + 0x00000000000000L) +#define LOCAL_I915_FORMAT_MOD_X_TILED local_fourcc_mod_code(INTEL, \ + 0x00000000000001L) + +#define LOCAL_DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, \ + struct local_drm_mode_fb_cmd2) + +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height, + uint32_t pixel_format, uint32_t bo_handles[4], + uint32_t pitches[4], uint32_t offsets[4], + uint64_t modifier[0], uint32_t *buf_id, uint32_t flags); + +#define LOCAL_DRM_CAP_ADDFB2_MODIFIERS 0x10 + +unsigned int has_drm_fb_modifiers(int fd); + #endif /* IOCTL_WRAPPERS_H */ diff --git a/tests/kms_addfb.c b/tests/kms_addfb.c index 756589e..9b0f77c 100644 --- a/tests/kms_addfb.c +++ b/tests/kms_addfb.c @@ -213,6 +213,66 @@ static void size_tests(int fd) } } +static void addfb25_tests(int fd) +{ + struct local_drm_mode_fb_cmd2 f = {}; + + + igt_require(has_drm_fb_modifiers(fd)); + + memset(&f, 0, sizeof(f)); + + f.width = 1024; + f.height = 1024; + f.pixel_format = DRM_FORMAT_XRGB8888; + f.pitches[0] = 1024*4; + f.flags = LOCAL_DRM_MODE_FB_MODIFIERS; + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; + + igt_fixture { + gem_bo = gem_create(fd, 1024*1024*4); + igt_assert(gem_bo); + } + + f.handles[0] = gem_bo; + + igt_subtest("addfb25-X-tiled") { + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); + f.fb_id = 0; + } + + igt_subtest("addfb25-framebuffer-vs-set-tiling") { + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY); + igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4) == -EBUSY); + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); + f.fb_id = 0; + } + + igt_fixture + gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4); + f.pitches[0] = 1024*4; + + igt_subtest("addfb25-X-tiled-both") { + f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == 0); + igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0); + f.fb_id = 0; + } + + igt_subtest("addfb25-X-tiled-mismatch") { + f.modifier[0] = LOCAL_I915_FORMAT_MOD_NONE; + igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL); + f.fb_id = 0; + } + + igt_fixture { + gem_close(fd, gem_bo); + } +} + int fd; igt_main @@ -224,6 +284,8 @@ igt_main size_tests(fd); + addfb25_tests(fd); + igt_fixture close(fd); }