Hi Marc-Andre,

<snip>
+    create = g_malloc0(sizeof(*create) +
+                       res->iov_cnt * sizeof (struct udmabuf_create_item));
+    if (!create)
+        return NULL;

Pointless allocation check (g_malloc will abort if it failed to allocate)
[Kasireddy, Vivek] Ok, will remove it in v2.

+
+    create->count = res->iov_cnt;
+    create->flags = UDMABUF_FLAGS_CLOEXEC;
+    for (i = 0; i < res->iov_cnt; i++) {
+        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
+        if (!rb || rb->fd < 0) {
+                g_free(create);
+                return NULL;
+        }
+
+        create->list[i].memfd = rb->fd;
+        create->list[i].__pad = 0;
+        create->list[i].offset = offset;
+        create->list[i].size = res->iov[i].iov_len;
+    }
+
+    dmabuf_fd = ioctl(udmabuf_fd, UDMABUF_CREATE_LIST, create);
+    if (dmabuf_fd < 0) {

It would be useful to print the error with errno.
[Kasireddy, Vivek] Sure.

+        g_free(create);

(we now like new code to use g_auto)

+        return NULL;
+    }
+
+    /* FIXME: We should get the modifier and format info with blob resources */
+    modifier_hi = fourcc_mod_code(INTEL, I915_FORMAT_MOD_X_TILED) >> 32;
+    modifier_lo = fourcc_mod_code(INTEL,I915_FORMAT_MOD_X_TILED) & 0xFFFFFFFF;

I have no idea if this format is going to work with various drivers and matches the underlying memory representation.
[Kasireddy, Vivek] No, it won’t work with other drivers. The modifier information should
be obtained from the Guest that allocates the buffers. I just added it as a temporary hack
for testing with a Intel GPU.

+    modifier = ((uint64_t)modifier_hi << 32) | modifier_lo;
+
+    dmabuf = g_new0(VGPUDMABuf, 1);
+    dmabuf->dmabuf_id = ids++;
+    dmabuf->buf.width = res->width;
+    dmabuf->buf.height = res->height;
+    dmabuf->buf.stride = pixman_image_get_stride(res->image);
+    dmabuf->buf.fourcc = DRM_FORMAT_XRGB8888;
+    dmabuf->buf.modifier = modifier;
+    dmabuf->buf.fd = dmabuf_fd;
+
+    QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
+    g_free(create);
+
+    return dmabuf;
+}


+    udmabuf_fd = open("/dev/udmabuf", O_RDWR);

If udmabuf_fd is already opened, this will leak fds.
[Kasireddy, Vivek] Yup, will take care of it in v2.

+    if (udmabuf_fd < 0) {
+        error_setg_errno(errp, errno,
+                         "udmabuf: failed to open vhost device");
+        return;

The fallback path should keep working
[Kasireddy, Vivek] Makes sense; will fix it in v2.


It would be worth doing a similar change in vhost-user-gpu.
[Kasireddy, Vivek] Sure, will look into it after understanding the deltas between
vhost-user and the in-qemu implementations.

Thanks,
Vivek

--
Marc-André Lureau