[Intel-gfx] [PATCH] tests/kms_addfb: Add support for fb modifiers
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Feb 5 06:50:09 PST 2015
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.
>> +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
More information about the Intel-gfx
mailing list