[Intel-gfx] [PATCH] tests/kms_addfb: Add support for fb modifiers
Daniel Vetter
daniel at ffwll.ch
Fri Feb 6 01:33:42 PST 2015
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 at 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 at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list