Re: [PATCH] tests/kms_addfb: Add support for fb modifiers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>

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@xxxxxxxxx>
---
  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux