drm/i915/gvt: Dma-buf support for GVT-g

Add new drm format which will be used by GVT-g.

Signed-off-by Xiaoguang Chen <[email protected]>

---
 include/uapi/drm/drm_fourcc.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/drm/drm_fourcc.h +0 -0

index 55e3010..2681862 100644

include/uapi/drm/drm_fourcc.h +1 -0

+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has

Signature · 6 lines

2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Ville Syrjälä
Sent: Thursday, June 15, 2017 6:22 PM
To: Chen, Xiaoguang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format

Quoted 12 lines · Xiaoguang Chen

Add new drm format which will be used by GVT-g.

Signed-off-by Xiaoguang Chen <[email protected]>

---
 include/uapi/drm/drm_fourcc.h | 4 ++++
 1 file changed, 4 insertions(+)

(quoted diff) +0 -0

b/include/uapi/drm/drm_fourcc.h index 55e3010..2681862 100644

include/uapi/drm/drm_fourcc.h extern "C" { · line 114

Quoted 2 lines

include/uapi/drm/drm_fourcc.h +0 -0

@@ -113,6 +113,10 @@ extern "C" {
 
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0]

A:Y:Cb:Cr 8:8:8:8 little endian */

include/uapi/drm/drm_fourcc.h extern "C" { · line 119

Quoted 6 lines

include/uapi/drm/drm_fourcc.h +5 -0

+/* 64 bpp RGB */
+#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
+[63:0] x:R:G:B 16:16:16:16 little endian */ #define
+DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
+x:B:G:R 16:16:16:16 little endian */

Are these supposed to be the half float formats? If so the docs are lacking. Also this sort of stuff must be sent to dri-devel for everyone to see.

That said, I don't really like having them in any gvt code until they're handled by the driver proper.

Quoted 9 lines

include/uapi/drm/drm_fourcc.h +1 -0

+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8

format has
--
2.7.4

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Here we defined a new ioctl to create a fd for a vfio device based on the input type. Now only one type is supported that is a dma-buf management fd.
Two ioctls are defined for the dma-buf management fd: query the vfio vgpu's plane information and create a dma-buf for a plane.

Signed-off-by Xiaoguang Chen <[email protected]>

---
 include/uapi/linux/vfio.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

include/uapi/linux/vfio.h +17 -0

index ae46105..7d86101 100644
@@ -502,6 +502,63 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
+ *
+ * Create a fd for a vfio device based on the input type
+ * Vendor driver should handle this ioctl to create a fd and manage the
+ * life cycle of this fd.
+ *
+ * Return: a fd if vendor support that type, -errno if not supported
+ */
+
+#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
+
+#define VFIO_DEVICE_DMABUF_MGR_FD	0 /* Supported fd types */
+
+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;

include/uapi/linux/vfio.h +4 -0

+/*
+ * VFIO_DMABUF_MGR_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
+ *					struct vfio_dmabuf_mgr_query_plane)
+ * Query plane information

include/uapi/linux/vfio.h +16 -0

+/*
+ * VFIO_DMABUF_MGR_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
+ *					struct vfio_dmabuf_mgr_create_dmabuf)
+ *
+ * Create a dma-buf for a plane
+ */
+struct vfio_dmabuf_mgr_create_dmabuf {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+	__s32 fd;
+};
+
+#define VFIO_DMABUF_MGR_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 /**
 

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 531

Quoted 3 lines

include/uapi/linux/vfio.h +3 -0

+	__u32 padding;
+};
+

This structure is generic, can remove dmabuf from its name, vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

Quoted 7 lines

include/uapi/linux/vfio.h +7 -0

+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and define flags if you want to differentiate query for 'dmabuf' and 'region'.

Hi,

Quoted 10 lines

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;

Quoted 8 lines

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 531

Quoted 3 lines

include/uapi/linux/vfio.h +3 -0

+	__u32 padding;
+};
+  

This structure is generic, can remove dmabuf from its name, vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

Agree.

Quoted 7 lines

Quoted 7 lines

include/uapi/linux/vfio.h +7 -0

+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+  

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and define flags if you want to differentiate query for 'dmabuf' and 'region'.

Hmm, any specific reason why you want use a ioctl for that? I would simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the final name will be) at the start of the region.

I'm not sure I agree regarding the vgpu statement, maybe this is not dmabuf specific, but what makes it vgpu specific?  We need to separate
our current usage plans from what it's actually describing and I don't
see that it describes anything vgpu specific.

Hi,

Quoted 6 lines

I'm not sure I agree regarding the vgpu statement, maybe this is not dmabuf specific, but what makes it vgpu specific?  We need to separate
our current usage plans from what it's actually describing and I don't
see that it describes anything vgpu specific.

Well, it describes a framebuffer, what non-graphic device would need that?

Quoted 21 lines · On Thu, 15 Jun 2017 18:00:38 +0200 Gerd Hoffmann <[email protected]> wrote:

Hi,

Quoted 10 lines

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;

Quoted 8 lines

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 531

Quoted 3 lines

include/uapi/linux/vfio.h +3 -0

+	__u32 padding;
+};
+  

This structure is generic, can remove dmabuf from its name, vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

Quoted 1 line

Agree.

I'm not sure I agree regarding the vgpu statement, maybe this is not dmabuf specific, but what makes it vgpu specific? We need to separate our current usage plans from what it's actually describing and I don't see that it describes anything vgpu specific.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

include/uapi/linux/vfio.h +7 -0

+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+  

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and define flags if you want to differentiate query for 'dmabuf' and 'region'.

Quoted 3 lines

Hmm, any specific reason why you want use a ioctl for that? I would simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the final name will be) at the start of the region.

Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd, if you're exposing a region with the info I wouldn't think you'd want the hassle of managing this separate fd when you could do something like Gerd suggests with defining the first page of the regions as containing the structure.

Maybe you could even allow mmap of that page
to reduce the overhead of getting the current state.

For the sake of
userspace, I'd hope we'd still use the same structure for either the ioctl or region mapping. I'm not really in favor of declaring that this particular ioctl might exist on the device fd when such-and-such region is present otherwise it might exist on a dmabuf manager fd.

Thanks,

Alex

Quoted 51 lines · Alex Williamson

Quoted 21 lines · On Thu, 15 Jun 2017 18:00:38 +0200 Gerd Hoffmann <[email protected]> wrote:

Hi,

Quoted 10 lines

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;

Quoted 8 lines

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 531

Quoted 3 lines

include/uapi/linux/vfio.h +3 -0

+	__u32 padding;
+};
+    

This structure is generic, can remove dmabuf from its name, vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

Quoted 1 line

Agree.

I'm not sure I agree regarding the vgpu statement, maybe this is not dmabuf specific, but what makes it vgpu specific? We need to separate our current usage plans from what it's actually describing and I don't see that it describes anything vgpu specific.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

include/uapi/linux/vfio.h +7 -0

+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+    

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and define flags if you want to differentiate query for 'dmabuf' and 'region'.

Quoted 3 lines

Hmm, any specific reason why you want use a ioctl for that? I would simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the final name will be) at the start of the region.

Quoted 5 lines

Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd, if you're exposing a region with the info I wouldn't think you'd want the hassle of managing this separate fd when you could do something like Gerd suggests with defining the first page of the regions as containing the structure.

My suggestion was to use vfio device fd for this ioctl and have dmabuf mgr fd as member in above query_plane structure, for region type it would be set to 0.
Yes there is other way to query surface information as Gerd suggested, but my point is: if a ioctl is being add, it could be used for both types, dmabuf and region.

Quoted 2 lines

Maybe you could even allow mmap of that page
to reduce the overhead of getting the current state.

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver writing it. There could be race condition in these two operations. Read on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Quoted 5 lines

For the sake of
userspace, I'd hope we'd still use the same structure for either the ioctl or region mapping. I'm not really in favor of declaring that this particular ioctl might exist on the device fd when such-and-such region is present otherwise it might exist on a dmabuf manager fd.

Userspace will always use vfio device fd for this ioctl, it only have to set proper arguments to its structure based on type.

Quoted 58 lines · On Fri, 16 Jun 2017 19:02:30 +0530 Kirti Wankhede <[email protected]> wrote:

Quoted 51 lines · Alex Williamson

Quoted 21 lines · On Thu, 15 Jun 2017 18:00:38 +0200 Gerd Hoffmann <[email protected]> wrote:

Hi,

Quoted 10 lines

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+struct vfio_dmabuf_mgr_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;

Quoted 8 lines

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 531

Quoted 3 lines

include/uapi/linux/vfio.h +3 -0

+	__u32 padding;
+};
+    

This structure is generic, can remove dmabuf from its name, vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

Quoted 1 line

Agree.

I'm not sure I agree regarding the vgpu statement, maybe this is not dmabuf specific, but what makes it vgpu specific? We need to separate our current usage plans from what it's actually describing and I don't see that it describes anything vgpu specific.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

include/uapi/linux/vfio.h +7 -0

+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+    

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and define flags if you want to differentiate query for 'dmabuf' and 'region'.

Quoted 3 lines

Hmm, any specific reason why you want use a ioctl for that? I would simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the final name will be) at the start of the region.

Quoted 5 lines

Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd, if you're exposing a region with the info I wouldn't think you'd want the hassle of managing this separate fd when you could do something like Gerd suggests with defining the first page of the regions as containing the structure.

Quoted 6 lines

My suggestion was to use vfio device fd for this ioctl and have dmabuf mgr fd as member in above query_plane structure, for region type it would be set to 0.
Yes there is other way to query surface information as Gerd suggested, but my point is: if a ioctl is being add, it could be used for both types, dmabuf and region.

I think this suggests abandoning the dmabuf manager fd entirely. That's not necessarily a bad thing, but I don't think the idea of the dmabuf manager fd stands if we push one of its primary reasons for existing back to the device fd. Reading though previous posts, I think we embraced the dmabuf manager as a separate fd primarily for consolidation and the potential to use it as a notification point, the latter being only theoretically useful.

So perhaps this becomes:

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};

The flag defines the data in the id field as either referring to a region (perhaps there could be multiple regions with only one active) or a plane ID, which is acquired separately, such as via a dmabuf fd. This would be retrieved via an optional VFIO_DEVICE_QUERY_GFX_PLANE ioctl on the vfio device, implemented in the vendor driver.

Would the above, along with the already defined mechanism for defining device specific regions, account for NVIDIA's needs?

For dmabuf users, we'd still need a new ioctl to get the dmabuf fd. We could either create a specific ioctl for this (ex. VFIO_DEVICE_GET_DMABUF_FD) or we could create a shared, generic GET_FD interface on the device.

Quoted 15 lines

Quoted 2 lines

Maybe you could even allow mmap of that page
to reduce the overhead of getting the current state.

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver writing it. There could be race condition in these two operations. Read on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Quoted 5 lines

For the sake of
userspace, I'd hope we'd still use the same structure for either the ioctl or region mapping. I'm not really in favor of declaring that this particular ioctl might exist on the device fd when such-and-such region is present otherwise it might exist on a dmabuf manager fd.

Quoted 2 lines

Userspace will always use vfio device fd for this ioctl, it only have to set proper arguments to its structure based on type.

Then we should kill off the manager fd unless there are arguments that still give it value. Thanks,

Alex

So perhaps this becomes:

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};

The flag defines the data in the id field as either referring to a region (perhaps there could be multiple regions with only one active)

Hi,

Quoted 13 lines

So perhaps this becomes:

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};  

Looks good.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};  

Hmm, plane isn't really an ID, it is a type, with type being either DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think the flage above make sense.

What are the nvidia plane for cursor support btw?

Quoted 2 lines

The flag defines the data in the id field as either referring to a region (perhaps there could be multiple regions with only one active)

Well, we have a "start" field in vfio_device_gfx_plane_info (maybe we should rename that to "offset"), which can be used to place multiple planes into a single, fixed region.

Also I think it would be useful to have some way to figure the device capabilities as the userspace workflow will look quite different for the two cases.

Quoted 4 lines

Hmm, plane isn't really an ID, it is a type, with type being either DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think the
flage above make sense.

The intention was that ..._REGION_ID and ...PLANE_ID are describing what the vfio_device_query_gfx_plane.id field represents, either a region index or a plane identifier.  The type of plane would be represented within the vfio_device_gfx_plane_info struct.

Quoted 5 lines

Also I think it would be useful to have some way to figure the device
capabilities as the userspace workflow will look quite different for
the two cases.

In the region case, VFIO_DEVICE_GET_REGION_INFO would include a device
specific region with a hopefully common identifier to identify it as a
graphics framebuffer.

In the dmabuf case,VFIO_DEVICE_QUERY_GFX_PLANE would indicate the plane as a "plane ID" and some sort of VFIO_DEVICE_GET_GFX_PLANE(VFIO_GFX_TYPE_DMABUF) ioctl would be necessary to get a file descriptor to that plane.

What else are you thinking we need?  Thanks,

Hi,

Quoted 9 lines

Quoted 4 lines

Hmm, plane isn't really an ID, it is a type, with type being either DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think the
flage above make sense.

The intention was that ..._REGION_ID and ...PLANE_ID are describing what the vfio_device_query_gfx_plane.id field represents, either a region index or a plane identifier. The type of plane would be represented within the vfio_device_gfx_plane_info struct.

The planes don't really have an id, we should rename that to plane_type, or maybe drm_plane_type (simliar to the drm_format_* fields), to avoid that confusion.

plane_type is set by userspace to specify what kind of plane it asks for.

What are the nvidia plane for cursor support btw?

Quoted 11 lines

Quoted 5 lines

Also I think it would be useful to have some way to figure the device
capabilities as the userspace workflow will look quite different for
the two cases.

In the region case, VFIO_DEVICE_GET_REGION_INFO would include a device
specific region with a hopefully common identifier to identify it as a
graphics framebuffer.

Ok, that should work to figure whenever the mdev supports a plane region or not.

Quoted 6 lines

In the dmabuf case,VFIO_DEVICE_QUERY_GFX_PLANE would indicate the plane as a "plane ID" and some sort of VFIO_DEVICE_GET_GFX_PLANE(VFIO_GFX_TYPE_DMABUF) ioctl would be necessary to get a file descriptor to that plane.

What else are you thinking we need? Thanks,

I need to know whenever the mdev supports dmabufs or not, at device initialization time (because dmabufs require opengl support), when VFIO_DEVICE_QUERY_GFX_PLANE doesn't work due to the guest not having the device initialized yet.

Maybe we should have a error field in the ioctl struct, or we need to clearly define error codes so the kernel doesn't just throw EINVAL in all cases.

Or just a VFIO_DEVICE_GFX_CAPS ioctl which returns NONE, REGION or DMABUF.

cheers,
Gerd

We don't support cursor for console vnc. Ideally console vnc should be
used by admin for configuration or during maintenance, which refresh primary surface at low refresh rate, 10 fps.

Right we need to know this at device initialization time for both cases
to initialize VGACommonState structure for that device

and also need
NONE to decide whether to init console vnc or not. We have a mechanism
to disable console vnc path and we recommend to disable it for better performance.

Hi,

Quoted 4 lines

We don't support cursor for console vnc. Ideally console vnc should be
used by admin for configuration or during maintenance, which refresh primary surface at low refresh rate, 10 fps.

But you surely want a mouse pointer for the admin? You render it directly to the primary surface then I guess?

Quoted 3 lines

Right we need to know this at device initialization time for both cases
to initialize VGACommonState structure for that device

Why do you need a VGACommonState?

Quoted 5 lines

and also need
NONE to decide whether to init console vnc or not. We have a mechanism
to disable console vnc path and we recommend to disable it for better performance.

Hmm, maybe we should have a ioctl to configure the refresh rate, or a ioctl to allow qemu ask for a refresh when needed?

qemu can throttle the display update rate, which for example happens in case no vnc client is connected. qemu updates the display only once every few seconds then.

cheers,
Gerd

Quoted 6 lines

Quoted 3 lines

Right we need to know this at device initialization time for both cases
to initialize VGACommonState structure for that device

Why do you need a VGACommonState?

We need to create a GRAPHIC_CONSOLE for vGPU device and specify GraphicHwOps so that from its .gfx_update callback, surface can be queried and updated.

Quoted 7 lines

Quoted 6 lines

and also need
NONE to decide whether to init console vnc or not. We have a mechanism
to disable console vnc path and we recommend to disable it for better
performance.

Quoted 4 lines

Hmm, maybe we should have a ioctl to configure the refresh rate, or a
ioctl to allow qemu ask for a refresh when needed?

What is default refresh rate of QEMU if vnc is connected?

My suggestion was to use vfio device fd for this ioctl and have dmabuf
mgr fd as member in above query_plane structure, for region type it would be set to 0.

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver
writing it. There could be race condition in these two operations. Read
on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Hi,

Quoted 4 lines

My suggestion was to use vfio device fd for this ioctl and have dmabuf
mgr fd as member in above query_plane structure, for region type it would be set to 0.

Region type should be DRM_PLANE_TYPE_PRIMARY

Quoted 7 lines

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver
writing it. There could be race condition in these two operations. Read
on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Alex Williamson
Sent: Monday, June 19, 2017 10:56 PM
To: Gerd Hoffmann <[email protected]>
Cc: [email protected]; [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; [email protected]; Lv, Zhiyuan <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 16 lines · On Mon, 19 Jun 2017 08:38:32 +0200 Gerd Hoffmann <[email protected]> wrote:

Hi,

Quoted 3 lines

My suggestion was to use vfio device fd for this ioctl and have dmabuf mgr fd as member in above query_plane structure, for region type it would be set to 0.

Region type should be DRM_PLANE_TYPE_PRIMARY

Quoted 7 lines

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver writing it. There could be race condition in these two operations.
Read
on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Quoted 2 lines

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

It seems less cumbersome than checking each field to see if it has changed. Thanks,

Alex
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};  

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};  

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

Quoted 43 lines · On Tue, 20 Jun 2017 12:57:36 +0200 Gerd Hoffmann <[email protected]> wrote:

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};  

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};  

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

Quoted 5 lines

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */

};

We could use two flag bits to indicate dmabuf or graphics region support. vfio_device_gfx_query_caps seems to imply a new ioctl, which would be unnecessary.

Quoted 8 lines

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

I'm not sure why we're using an enum for something that can currently be defined with 2 bits, seems like this would be another good use of flags. We could even embed an enum into the flags if we want to leave some expansion room, 4 bits maybe? Also, I was imagining that a device could support multiple graphics regions, that's where specifying the "id" as a region index seemed useful. We lose that ability here unless we go back to defining a flag bit to specify how to interpret this last field.

Quoted 8 lines

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

I'm not convinced this adds value, but I'll list it as an option:

VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)

The benefit is that it might help to avoid a proliferation of ioctls on the device the pain is that we need to either define a field or section of flags which identify what is being queried or what type of device fd is being requested.

Quoted 11 lines

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

I'm not sure I understand how this works for a region, the region is always the current generation, how can the user ever be sure the plane_info matches what is exposed in the region? Thanks,

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Tuesday, June 20, 2017 11:00 PM
To: Gerd Hoffmann <[email protected]>
Cc: Zhang, Tina <[email protected]>; [email protected]; linux- [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; [email protected]; Lv, Zhiyuan <[email protected]>; Wang, Zhi A <[email protected]>; Wang, Zhenyu Z <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 43 lines · On Tue, 20 Jun 2017 12:57:36 +0200 Gerd Hoffmann <[email protected]> wrote:

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
};

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

Quoted 5 lines

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */

};

We could use two flag bits to indicate dmabuf or graphics region support. vfio_device_gfx_query_caps seems to imply a new ioctl, which would be unnecessary.

Quoted 8 lines

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

I'm not sure why we're using an enum for something that can currently be defined with 2 bits, seems like this would be another good use of flags. We could even embed an enum into the flags if we want to leave some expansion room, 4 bits maybe? Also, I was imagining that a device could support multiple graphics regions, that's where specifying the "id" as a region index seemed useful. We lose that ability here unless we go back to defining a flag bit to specify how to interpret this last field.

Quoted 8 lines

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

I'm not convinced this adds value, but I'll list it as an option:

VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)

The benefit is that it might help to avoid a proliferation of ioctls on the device the pain is that we need to either define a field or section of flags which identify what is being queried or what type of device fd is being requested.

Quoted 11 lines

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

I'm not sure I understand how this works for a region, the region is always the current generation, how can the user ever be sure the plane_info matches what is exposed in the region? Thanks,

Alex

Quoted 113 lines

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Tuesday, June 20, 2017 11:00 PM
To: Gerd Hoffmann <[email protected]>
Cc: Zhang, Tina <[email protected]>; [email protected]; linux- [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; [email protected]; Lv, Zhiyuan <[email protected]>; Wang, Zhi A <[email protected]>; Wang, Zhenyu Z <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 43 lines · On Tue, 20 Jun 2017 12:57:36 +0200 Gerd Hoffmann <[email protected]> wrote:

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};  

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
};  

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

Quoted 5 lines

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};  

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */

};

We could use two flag bits to indicate dmabuf or graphics region support. vfio_device_gfx_query_caps seems to imply a new ioctl, which would be unnecessary.

Quoted 8 lines

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

I'm not sure why we're using an enum for something that can currently be defined with 2 bits, seems like this would be another good use of flags. We could even embed an enum into the flags if we want to leave some expansion room, 4 bits maybe? Also, I was imagining that a device could support multiple graphics regions, that's where specifying the "id" as a region index seemed useful. We lose that ability here unless we go back to defining a flag bit to specify how to interpret this last field.

Quoted 8 lines

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

I'm not convinced this adds value, but I'll list it as an option:

VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)

Quoted 3 lines

The benefit is that it might help to avoid a proliferation of ioctls on the device the pain is that we need to either define a field or section of flags which identify what is being queried or what type of device fd is being requested.

I didn't understand here. The patch introduces three ioctl commands: VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE, VFIO_DMABUF_MGR_CREATE_DMABUF.
What I mean was we could remove the first one, a.k.a VFIO_DEVICE_GET_FD, which is used to get the fd of dmabuf mgr, as we want to remove the logic of dmabuf mgr. For the other two ioctls, I think we can give them new names which looks like more general. So, do you mean there is another way instead of ioctls? Thanks.

Quoted 12 lines

Quoted 11 lines

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

Quoted 5 lines

I'm not sure I understand how this works for a region, the region is always the current generation, how can the user ever be sure the plane_info matches what is exposed in the region? Thanks,

Alex

We could use two flag bits to indicate dmabuf or graphics region support.

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Wednesday, June 21, 2017 7:22 AM
To: Zhang, Tina <[email protected]>
Cc: Gerd Hoffmann <[email protected]>; [email protected]; [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; intel-gvt- [email protected]; Lv, Zhiyuan <[email protected]>; Wang, Zhi A <[email protected]>; Wang, Zhenyu Z <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 64 lines · Tina

Quoted 64 lines

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Tuesday, June 20, 2017 11:00 PM
To: Gerd Hoffmann <[email protected]>
Cc: Zhang, Tina <[email protected]>; [email protected]; linux- [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; [email protected]; Lv, Zhiyuan <[email protected]>; Wang, Zhi A <[email protected]>; Wang, Zhenyu Z <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 47 lines · On Tue, 20 Jun 2017 12:57:36 +0200 Gerd Hoffmann <[email protected]> wrote:

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
};

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type; };

Quoted 8 lines

Quoted 8 lines

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device

*/

Quoted 19 lines

Quoted 19 lines

#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */

};

We could use two flag bits to indicate dmabuf or graphics region support. vfio_device_gfx_query_caps seems to imply a new ioctl, which would be unnecessary.

Quoted 8 lines

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

Quoted 7 lines

Quoted 7 lines

I'm not sure why we're using an enum for something that can currently be defined with 2 bits, seems like this would be another good use of flags. We could even embed an enum into the flags if we want to leave some expansion room, 4 bits maybe? Also, I was imagining that a device could support multiple graphics regions, that's where specifying the "id" as a region index seemed useful. We lose that ability here unless we go back to defining a flag bit to specify

how to interpret this last field.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf

fd.

Quoted 9 lines

Quoted 9 lines

I'm not convinced this adds value, but I'll list it as an option:

VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)

The benefit is that it might help to avoid a proliferation of ioctls on the device the pain is that we need to either define a field or section of flags which identify what is being queried or what type of device

fd is being requested.

Quoted 1 line

I didn't understand here. The patch introduces three ioctl commands:

VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE,
VFIO_DMABUF_MGR_CREATE_DMABUF.

Quoted 1 line

What I mean was we could remove the first one, a.k.a VFIO_DEVICE_GET_FD,

which is used to get the fd of dmabuf mgr, as we want to remove the logic of dmabuf mgr. For the other two ioctls, I think we can give them new names which looks like more general.

Quoted 1 line

So, do you mean there is another way instead of ioctls? Thanks.

In this v9 series, we have a VFIO_DEVICE_GET_FD where we could pass VFIO_DEVICE_DMABUF_MGR_FD to get the manager fd, from that fd we could query plane info or get a dmabuf fd. Now we're getting rid of the manager fd and I'm questioning whether generic ioctls or specific ioctls are the right path.

For instance we could still use a VFIO_DEVICE_GET_FD ioctl to get the dmabuf fd rather than creating a VFIO_DEVICE_GET_DMABUF_FD ioctl. It's just a matter of defining a common header on the data structure so that we know how to interpret the remainder of the structure.

Likewise Gerd proposes above a VFIO_DEVICE_QUERY_GFX_PLANE ioctl and I'm asking whether a generic VFIO_DEVICE_QUERY ioctl where we define a common header in the structure to know that the query is for graphics plane information has value.

IOW, should we spend a little bit of time now crafting ioctls that we can use for purposes beyond what we're looking at today, or do we burn off a couple for singular uses here? Thanks,

Alex

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

Quoted 5 lines

Quoted 5 lines

I'm not sure I understand how this works for a region, the region is always the current generation, how can the user ever be sure the plane_info matches what is exposed in the region? Thanks,

Alex

Thanks for all the comments. I'm planning to cook the next version of this patch set

Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf device */

2. vfio_device_gfx_plane_info

struct vfio_device_gfx_plane_info {
	__u64 start;-> offset
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};

Quoted 2 lines

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

Well,  Gerd, can you share more details about how to use this field in user mode, so that we can figure out a way to support it? Thanks.

3. vfio_device_query_gfx_plane

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
	__u32 plane_type;
};

So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

If the newly added plane_type is used for this, the id field may be
useless in dmabuf usage. Do you have any idea about the usage of this id field in dmabuf usage?

Quoted 2 lines · Tina

Thanks for all the comments. I'm planning to cook the next version of this patch set

How about posting only this patch instead of the whole series until we've settled the interfaces?

Quoted 3 lines

Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf device */

VFIO_DEVICE_FLAGS_GFX_DMABUF?

Quoted 14 lines

2. vfio_device_gfx_plane_info

struct vfio_device_gfx_plane_info {
	__u64 start;-> offset
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};  

Quoted 2 lines

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

Quoted 2 lines

Well,  Gerd, can you share more details about how to use this field in user mode, so that we can figure out a way to support it? Thanks.

generation would be increased each time one of the fields in vfio_device_gfx_plane_info changes, typically on mode switches (width/height changes) and pageflips (offset changes). So userspace can simply compare generation instead of comparing every field to figure whenever something changed compared to the previous poll.

Quoted 11 lines

3. vfio_device_query_gfx_plane

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
	__u32 plane_type;
};

Quoted 7 lines

So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

If the newly added plane_type is used for this, the id field may be
useless in dmabuf usage. Do you have any idea about the usage of this id field in dmabuf usage?

plane_type should be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR for dmabuf.

Given that nvidia doesn't support a separate cursor plane in their region they would support DRM_PLANE_TYPE_PRIMARY only.

I can't see yet what id would be useful for.

Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

4. Two ioctl commands
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

Quoted 1 line

VFIO_DEVICE_FLAGS_GFX_DMABUF?

After proposing these, I'm kind of questioning their purpose.  In the case of a GFX region, the user is going to learn that this is supported
as they parse the region information and find the device specific region identifying itself as a GFX area.  That needs to happen early when the user is evaluating the device, so it seems rather redundant to the flag.

If we have dmabuf support, isn't that indicated by trying to query the
graphics plane_info and getting back a result indicating a dmabuf fd? Is there any point in time where a device supporting dmabuf fds would not report this here?  Userspace could really do the same process for a
graphics region, ie. querying the plane_info, if it exists pursuing either the region or dmabuf path to get it.

Quoted 6 lines

generation would be increased each time one of the fields in vfio_device_gfx_plane_info changes, typically on mode switches (width/height changes) and pageflips (offset changes).  So userspace
can simply compare generation instead of comparing every field to figure whenever something changed compared to the previous poll.

So we have two scenarios, dmabuf and region.  When the user retrieves a
dmabuf they get plane_info which includes the generation, so they know
the dmabuf is for that generation.  If they query the plane_info and get a different generation they should close the previous dmabuf and get another.

Does this promote the caching idea that a user might maintain multiple open dmabuf fds and select the appropriate one for the current device state?  Is it entirely the user's responsibility to
remember the plane info for an open dmabuf fd?

What happens to
existing dmabuf fds when the generation updates, do they stop refreshing?

Does it blank the framebuffer?

Can the dmabuf fd
transparently update to the new plane_info?

The other case is a region, the user queries the plane_info records the
parameters and region info, and configures access to the region using that information.  Meanwhile, something changed, plane_info including generation is updated, but the user is still assuming the previous plane_info.  How can we avoid such a race?

What is the responsibility
of the user and how are they notified to refresh the plane_info?

Quoted 7 lines

plane_type should be DRM_PLANE_TYPE_PRIMARY or
DRM_PLANE_TYPE_CURSOR
for dmabuf.

Given that nvidia doesn't support a separate cursor plane in their region they would support DRM_PLANE_TYPE_PRIMARY only.

Quoted 4 lines

I can't see yet what id would be useful for.

Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

I think we're trying to identify where this plane_info exists.  Does the user ask for a dmabuf fd for it or use a region.

If it's a region,
which region?

4. Two ioctl commands

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any opinions?  Thanks,

Hi,

Quoted 9 lines

Quoted 1 line

VFIO_DEVICE_FLAGS_GFX_DMABUF?

After proposing these, I'm kind of questioning their purpose.  In the case of a GFX region, the user is going to learn that this is supported
as they parse the region information and find the device specific region identifying itself as a GFX area.  That needs to happen early when the user is evaluating the device, so it seems rather redundant to the flag.

Indeed.

Quoted 8 lines

If we have dmabuf support, isn't that indicated by trying to query the
graphics plane_info and getting back a result indicating a dmabuf fd? Is there any point in time where a device supporting dmabuf fds would not report this here?  Userspace could really do the same process for a
graphics region, ie. querying the plane_info, if it exists pursuing either the region or dmabuf path to get it.

Well, you can get a dma-buf only after the guest loaded the driver and initialized the device, so a plane actually exists ...

Right now the experimental intel patches throw errors in case no plane exists (yet). Maybe we should have a "bool is_enabled" field in the plane_info struct, so drivers can use that to signal whenever the guest has programmed a valid video mode or not (likewise for the cursor, which doesn't exist with fbcon, only when running xorg). With that in place using the QUERY_PLANE ioctl also for probing looks reasonable.

Quoted 7 lines

Quoted 6 lines

generation would be increased each time one of the fields in vfio_device_gfx_plane_info changes, typically on mode switches (width/height changes) and pageflips (offset changes).  So userspace
can simply compare generation instead of comparing every field to figure whenever something changed compared to the previous poll.

Quoted 7 lines

So we have two scenarios, dmabuf and region.  When the user retrieves a
dmabuf they get plane_info which includes the generation, so they know
the dmabuf is for that generation.  If they query the plane_info and get a different generation they should close the previous dmabuf and get another.

Keeping it cached is a valid choice too.

Quoted 5 lines

Does this promote the caching idea that a user might maintain multiple open dmabuf fds and select the appropriate one for the current device state?  Is it entirely the user's responsibility to
remember the plane info for an open dmabuf fd?

Yes, I'd leave that to userspace. So, when the generation changes userspace knows the guest changed the plane. It could be a configuration the guest has used before (and where userspace could have a cached dma-buf handle for), or it could be something new.

Quoted 3 lines

What happens to
existing dmabuf fds when the generation updates, do they stop refreshing?

Depends on what the guest is doing ;)

The dma-buf is just a host-side handle for the piece of video memory where the guest stored the framebuffer.

Quoted 1 line

Does it blank the framebuffer?

No.

Quoted 2 lines

Can the dmabuf fd
transparently update to the new plane_info?

No.

Quoted 6 lines

The other case is a region, the user queries the plane_info records the
parameters and region info, and configures access to the region using that information.  Meanwhile, something changed, plane_info including generation is updated, but the user is still assuming the previous plane_info.  How can we avoid such a race?

Qemu doesn't. You might get rendering glitches in that case, due to accessing the plane with the wrong configuration. It's fundamentally the same with stdvga btw.

Quoted 2 lines

What is the responsibility
of the user and how are they notified to refresh the plane_info?

qemu polls in regular intervals, simliar to how it checks the dirty bitmap for video memory in regular intervals with stdvga.

qemu display update timer runs on 30fps usually, in case nobody is looking (no spice/vnc client connected) it reduces the update frequency though.

Quoted 7 lines

Quoted 7 lines

plane_type should be DRM_PLANE_TYPE_PRIMARY or
DRM_PLANE_TYPE_CURSOR
for dmabuf.

Given that nvidia doesn't support a separate cursor plane in their region they would support DRM_PLANE_TYPE_PRIMARY only.

Quoted 7 lines

Quoted 4 lines

I can't see yet what id would be useful for.

Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

I think we're trying to identify where this plane_info exists.  Does the user ask for a dmabuf fd for it or use a region.

But whenever a region or a dmabuf is used is a fixed property of the device, why associate that with a plane? It will be the same for all planes of a device anyway ...

Quoted 2 lines

If it's a region,
which region?

Ok, if we want support multiple regions. Do we? Using the offset we can place multiple planes in a single region. And I'm not sure nvidia plans to use multiple planes in the first place ...

Quoted 6 lines

4. Two ioctl commands

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any opinions?  Thanks,

I don't think a generic device query is that helpful. Depending on the kind of query you'll get back different stuff, and we need to handle that somehow, like this:

vfio_device_query {
    u32 argsz;
    u32 flags;
    enum query_type;  /* or use flags for that */
    union {
        plane_info plane;
        /* whatever else comes up */
    } query_params;
};

I fail to see how this is fundamentally different from having multiple vfio_device_query_* structs (and ioctls). It only pushes the problem one level down ...

Or do you have something else in mind?

Is this only going to support accelerated driver output, not basic VGA
modes for BIOS interaction?

Quoted 11 lines

Right now the experimental intel patches throw errors in case no plane
exists (yet).  Maybe we should have a "bool is_enabled" field in the
plane_info struct, so drivers can use that to signal whenever the guest
has programmed a valid video mode or not (likewise for the cursor, which doesn't exist with fbcon, only when running xorg).  With that in
place using the QUERY_PLANE ioctl also for probing looks reasonable.

Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no available
plane, but then that might not help the user know how a plane would be
available if it were available.

Quoted 5 lines

Yes, I'd leave that to userspace.  So, when the generation changes userspace knows the guest changed the plane.  It could be a configuration the guest has used before (and where userspace could have
a cached dma-buf handle for), or it could be something new.

But userspace also doesn't know that a dmabuf generation will ever be visited again,

so they're bound to have some stale descriptors.  Are
we thinking userspace would have some LRU list of dmabufs so that they
don't collect too many?  Each uses some resources,  do we just rely on
open file handles to set an upper limit?

Quoted 6 lines

Quoted 3 lines

What happens to
existing dmabuf fds when the generation updates, do they stop refreshing?

Depends on what the guest is doing ;)

Quoted 3 lines

The dma-buf is just a host-side handle for the piece of video memory
where the guest stored the framebuffer.

So the resources the user is holding if they don't release their dmabuf
are potentially non-trivial.

Quoted 5 lines

Ok, if we want support multiple regions.  Do we?  Using the offset we
can place multiple planes in a single region.  And I'm not sure nvidia
plans to use multiple planes in the first place ...

I don't want to take a driver ioctl interface as a throw away, one time
use exercise.  If we can think of such questions now, let's define how
they work.  A device could have multiple graphics regions with multiple
planes within each region.

Do we also want to exclude that device
needs to be strictly region or dmabuf?  Maybe it does both.

Or maybe
it supports dmabuf-ng (ie. whatever comes next).

Quoted 4 lines

vfio_device_query {
    u32 argsz;
    u32 flags;
    enum query_type;  /* or use flags for that */

We don't have an infinite number of ioctls

   Hi,

Quoted 3 lines

Is this only going to support accelerated driver output, not basic VGA
modes for BIOS interaction?

Right now there is no vgabios or uefi support for the vgpu.

But even with that in place there still is the problem that the display device initialization happens before the guest runs and therefore there isn't an plane yet ...

Quoted 11 lines

Quoted 11 lines

Right now the experimental intel patches throw errors in case no plane
exists (yet). Maybe we should have a "bool is_enabled" field in the
plane_info struct, so drivers can use that to signal whenever the guest
has programmed a valid video mode or not (likewise for the cursor, which doesn't exist with fbcon, only when running xorg). With that in
place using the QUERY_PLANE ioctl also for probing looks reasonable.

Quoted 5 lines

Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no available
plane, but then that might not help the user know how a plane would be
available if it were available.

So maybe a "enum plane_state" (instead of "bool is_enabled")? So we can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?

Quoted 7 lines

Quoted 5 lines

Yes, I'd leave that to userspace. So, when the generation changes userspace knows the guest changed the plane. It could be a configuration the guest has used before (and where userspace could have
a cached dma-buf handle for), or it could be something new.

But userspace also doesn't know that a dmabuf generation will ever be visited again,

kernel wouldn't know either, only the guest knows ...

Quoted 6 lines

so they're bound to have some stale descriptors. Are
we thinking userspace would have some LRU list of dmabufs so that they
don't collect too many? Each uses some resources, do we just rely on
open file handles to set an upper limit?

Yep, this is exactly what my qemu patches are doing, keep a LRU list.

Quoted 11 lines

Quoted 8 lines

Quoted 3 lines

What happens to
existing dmabuf fds when the generation updates, do they stop refreshing?

Depends on what the guest is doing ;)

The dma-buf is just a host-side handle for the piece of video memory
where the guest stored the framebuffer.

So the resources the user is holding if they don't release their dmabuf
are potentially non-trivial.

Not really. Host IGD has a certain amount of memory, some of it is assigned to the guest, guest stores the framebuffer there, the dma-buf is a host handle (drm object, usable for rendering ops) for the guest framebuffer. So it doesn't use much ressources. Some memory is needed for management structs, but not for the actual data as this in the video memory dedicated to the guest.

Quoted 5 lines

Quoted 5 lines

Ok, if we want support multiple regions. Do we? Using the offset we
can place multiple planes in a single region. And I'm not sure nvidia
plans to use multiple planes in the first place ...

Quoted 7 lines

I don't want to take a driver ioctl interface as a throw away, one time
use exercise. If we can think of such questions now, let's define how
they work. A device could have multiple graphics regions with multiple
planes within each region.

I'd suggest to settle for one of these two. Either one region and multiple planes inside (using offset) or one region per plane. I'd prefer the former. When going for the latter then yes we have to specify the region. I'd name the field region_id then to make clear what it is.

What would be the use case for multiple planes?

cursor support? We already have plane_type for that.

multihead support? We'll need (at minimum) a head_id field for that (for both dma-buf and region)

pageflipping support? Nothing needed, query_plane will simply return the currently visible plane. Region only needs to be big enough to fit the framebuffer twice. Then the driver can flip between two buffers, point to the one qemu should display using "offset".

Quoted 2 lines

Do we also want to exclude that device
needs to be strictly region or dmabuf? Maybe it does both.

Very unlikely IMHO.

Quoted 2 lines

Or maybe
it supports dmabuf-ng (ie. whatever comes next).

Possibly happens some day, but who knows what interfaces we'll need to support that ...

Quoted 5 lines

Quoted 4 lines

vfio_device_query {
     u32 argsz;
     u32 flags;
     enum query_type;  /* or use flags for that */

We don't have an infinite number of ioctls

The limited ioctl number space is a good reason indeed. Ok, lets take this route then.

cheers,
Gerd

_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Hi:
     Thanks for the discussions! If the userspace application has already maintained a LRU list, it looks like we don't need generation
anymore,

Quoted 5 lines · Zhi Wang

Hi:
     Thanks for the discussions! If the userspace application has already maintained a LRU list, it looks like we don't need generation
anymore,

generation isn't required, things are working just fine without that. It is just a small optimization, userspace can skip the LRU lookup altogether if the generation didn't change.

But of couse that only pays off if the kernel doesn't has to put much effort into maintaining the generation id. Something simple like increasing it each time the guest writes a register which affects plane_info.

Hi,

Quoted 3 lines

Is this only going to support accelerated driver output, not basic VGA
modes for BIOS interaction?

Right now there is no vgabios or uefi support for the vgpu.

But even with that in place there still is the problem that the display device initialization happens before the guest runs and therefore there isn't an plane yet ...

Quoted 12 lines

Quoted 11 lines

Right now the experimental intel patches throw errors in case no plane
exists (yet).  Maybe we should have a "bool is_enabled" field in the
plane_info struct, so drivers can use that to signal whenever the guest
has programmed a valid video mode or not (likewise for the cursor, which doesn't exist with fbcon, only when running xorg).  With that in
place using the QUERY_PLANE ioctl also for probing looks reasonable.

Quoted 5 lines

Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no available
plane, but then that might not help the user know how a plane would be
available if it were available.

So maybe a "enum plane_state" (instead of "bool is_enabled")? So we can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?

Quoted 3 lines

So maybe a "enum plane_state" (instead of "bool is_enabled")?  So we
can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?

What's the difference between NOT_SUPPORTED and -ENOTTY on the ioctl? Perhaps a bit in a flags field could specify EN/DIS-ABLED and leave room for things we're forgetting.

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Gerd Hoffmann
Sent: Wednesday, June 21, 2017 7:04 PM
To: Zhang, Tina <[email protected]>; Alex Williamson <[email protected]>
Cc: Wang, Zhenyu Z <[email protected]>; intel- [email protected]; [email protected]; Chen, Xiaoguang <[email protected]>; Kirti Wankhede <[email protected]>; Lv, Zhiyuan <[email protected]>; [email protected]; Wang, Zhi A <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 2 lines · Tina

Thanks for all the comments. I'm planning to cook the next version of this patch set

How about posting only this patch instead of the whole series until we've settled the interfaces?

Quoted 4 lines

Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf device */

VFIO_DEVICE_FLAGS_GFX_DMABUF?

Quoted 14 lines

2. vfio_device_gfx_plane_info

struct vfio_device_gfx_plane_info {
	__u64 start;-> offset
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};

Quoted 2 lines

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

Quoted 2 lines

Well,  Gerd, can you share more details about how to use this field in user mode, so that we can figure out a way to support it? Thanks.

generation would be increased each time one of the fields in vfio_device_gfx_plane_info changes, typically on mode switches (width/height changes) and pageflips (offset changes). So userspace can simply compare generation instead of comparing every field to figure whenever something changed compared to the previous poll.

Quoted 12 lines

3. vfio_device_query_gfx_plane

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
	__u32 plane_type;
};

Quoted 7 lines

So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

If the newly added plane_type is used for this, the id field may be
useless in dmabuf usage. Do you have any idea about the usage of this id field in dmabuf usage?

plane_type should be DRM_PLANE_TYPE_PRIMARY or
DRM_PLANE_TYPE_CURSOR for dmabuf.

Given that nvidia doesn't support a separate cursor plane in their region they would support DRM_PLANE_TYPE_PRIMARY only.

I can't see yet what id would be useful for.

Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

cheers,
Gerd

_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */ #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */ __u32   num_regions;    /* Max region index + 1 */ __u32   num_irqs;       /* Max IRQ index + 1 */
};

We could use two flag bits to indicate dmabuf or graphics region support.

Quoted 8 lines

Then this to query the plane:

struct vfio_device_gfx_query_plane {
        __u32 argsz;
        __u32 flags;
        struct vfio_device_gfx_plane_info plane_info;  /* out */ __u32 plane_type;                              /* in  */ };

I'm not sure why we're using an enum for something that can currently be defined with 2 bits,

seems like this would be another good use of
flags.  We could even embed an enum into the flags if we want to leave some expansion room, 4 bits maybe?  Also, I was imagining that a
device could support multiple graphics regions, that's where specifying
the "id" as a region index seemed useful.

Quoted 10 lines

With the generation we can also do something different:  Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
an error in case the generation doesn't match.  In that case it doesn't
make much sense any more to have a separate plane_info struct, which
was added so we don't have to duplicate things in query-plane and get-
dmabuf ioctl structs.

I'm not sure I understand how this works for a region, the region is always the current generation, how can the user ever be sure the plane_info matches what is exposed in the region?

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Tuesday, June 20, 2017 6:58 PM
To: Zhang, Tina <[email protected]>; Alex Williamson <[email protected]>
Cc: [email protected]; [email protected]; Kirti Wankhede <[email protected]>; Chen, Xiaoguang <[email protected]>; [email protected]; Lv, Zhiyuan <[email protected]>; Wang, Zhi A <[email protected]>; Wang, Zhenyu Z <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 5 lines · Tina

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.

Quoted 12 lines

struct vfio_device_gfx_plane_info {
	__u64 start;
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
	__u32 generation;
};

Looks good to me.

Quoted 8 lines

struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
};

I'm not convinced the flags are a great idea. Whenever dmabufs or a region is used is a static property of the device, not of each individual plane.

I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};

Then this to query the plane:

struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};

2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.

Quoted 2 lines

VFIO_DEVICE_QUERY_GFX_PLANE : used to query
vfio_device_gfx_plane_info.

Yes.

Quoted 1 line

VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Yes. The plane might have changed between query-plane and get-dmabuf ioctl calls though, we must make sure we handle that somehow. Current patches return plane_info on get-dmabuf ioctl too, so userspace can see what it actually got.

With the generation we can also do something different: Pass in plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case the generation doesn't match. In that case it doesn't make much sense any more to have a separate plane_info struct, which was added so we don't have to duplicate things in query-plane and get- dmabuf ioctl structs.

cheers,
Gerd

Quoted 9 lines

With the generation we can also do something different:  Pass in plane_type and
generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case
the generation doesn't match.  In that case it doesn't make much sense any
more to have a separate plane_info struct, which was added so we don't have
to duplicate things in query-plane and get- dmabuf ioctl structs.

Comparing with the current patch, this would make user space a little bit harder to
get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it efficient for
user mode usage?

Hi,

Quoted 10 lines

Quoted 9 lines

With the generation we can also do something different:  Pass in plane_type and
generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case
the generation doesn't match.  In that case it doesn't make much sense any
more to have a separate plane_info struct, which was added so we don't have
to duplicate things in query-plane and get- dmabuf ioctl structs.

Quoted 5 lines

Comparing with the current patch, this would make user space a little bit harder to
get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it efficient for
user mode usage?

user space has to call QUERY-PLANE first, then looks if it has a dma- buf for that, if not call GET-DMABUF.

Problem is the guest could have changed the plane between the QUERY- PLANE and GET-DMABUF ioctls.

Current patches (v8 series) just returns plane-info on GET-DMABUF too, so userspace can at least detect something changed.

It would be easier for userspace if GET-DMABUF throws an error in case the plane changed since the last QUERY-PLANE ioctl. The generation id would be one way to handle it, but possibly it is easier if the kernel driver just keeps track internally. So GET-DMABUF would be defined to return a dmabuf for the plane returned by the previous QUERY-PLANE ioctl (on the same file handle), or return an error in case the plane has changed meanwhile.

Hmm, I don't like that interface.  Can you cite examples of other ioctls that behave this way?  It doesn't feel like an elegant user interface; the user can get the dmabuf, but only after they query the dmabuf, even though the get-dmabuf ioctl returns the same data as the query-plane ioctl, but they can't get the dmabuf if the plane has changed in the interim, which is not something the user can

know.  Are
we causing our own problems with this model of cycling through dmabuf fds?  We talked previously about an enum of plane types, primary and cursor.  What if the user was simply able to get a dmabuf fd for each of
those and they queried the current plane information via those fds? IOW, the fd is persistent and specific to a given plane type, but the format within it is dynamic.

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Gerd Hoffmann
Sent: Tuesday, June 27, 2017 2:13 PM
To: Alex Williamson <[email protected]>
Cc: Wang, Zhenyu Z <[email protected]>; intel- [email protected]; [email protected]; Chen, Xiaoguang <[email protected]>; Zhang, Tina <[email protected]>; Kirti Wankhede <[email protected]>; Lv, Zhiyuan <[email protected]>; [email protected]; Wang, Zhi A <[email protected]>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Hi,

Quoted 4 lines

Hmm, I don't like that interface.  Can you cite examples of other ioctls that behave this way?  It doesn't feel like an elegant user interface; the user can get the dmabuf, but only after they query the dmabuf, even though the get-dmabuf ioctl returns the same data as the

Quoted 8 lines

query-plane ioctl, but they can't get the dmabuf if the plane has changed in the interim, which is not something the user can know.  Are we causing our own problems with this model of cycling through dmabuf fds?  We talked previously about an enum of plane types, primary and cursor.  What if the user was simply able to get a dmabuf fd for each of those and they queried the current plane information via those fds? IOW, the fd is persistent and specific to a given plane type, but the format within it is dynamic.

Will not work due to how dma-bufs are designed.

But, yes, the QUERY then GET split is ugly for a number of reasons.

Does gvt track the live cycle of all dma-bufs it has handed out?

If so, then maybe we can let the kernel check whenever a dma-buf for the current plane exists? And if that isn't the case hand out a dma- buf right away, without expecting userspace explicitly asking for it?

That will simplify the interface and remove the race condition at the expense of some additional bookkeeping in the kernel.

cheers,
Gerd

_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Quoted 1 line

Does gvt track the live cycle of all dma-bufs it has handed out?

The V9 implementation does track the dma-bufs' live cycle. The original idea was that leaving the dma-bufs' live cycle management to user mode.

Hi,

Quoted 2 lines

Quoted 1 line

Does gvt track the live cycle of all dma-bufs it has handed out?

Quoted 3 lines

The V9 implementation does track the dma-bufs' live cycle. The original idea was that leaving the dma-bufs' live cycle management to user mode.

That is still the case, user space decides which dma-bufs it'll go keep cached. But kernel space can see what user space is doing, so there is no need to explicitly tell the kernel whenever a cached dma-buf exists or not.

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, June 29, 2017 4:39 PM
To: Gerd Hoffmann <[email protected]>
Cc: Wang, Zhenyu Z <[email protected]>; intel- [email protected]; [email protected]; Chen, Xiaoguang <[email protected]>; Zhang, Tina <[email protected]>; Alex Williamson <[email protected]>; Lv, Zhiyuan <[email protected]>; Kirti Wankhede <[email protected]>; intel-gvt- [email protected]
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

Quoted 4 lines · Gerd Hoffmann

Hi,

Quoted 2 lines

Quoted 1 line

Does gvt track the live cycle of all dma-bufs it has handed out?

Quoted 8 lines

Quoted 3 lines

The V9 implementation does track the dma-bufs' live cycle. The original idea was that leaving the dma-bufs' live cycle management to user mode.

That is still the case, user space decides which dma-bufs it'll go keep cached. But kernel space can see what user space is doing, so there is no need to explicitly tell the kernel whenever a cached dma-buf exists or not.

We do the same trick in drm_prime.c, keeping a cache of exported dma-buf around for re-exporting. Since for prime sharing the use-case is almost always re-importing as a drm gem buffer again we can then on re-import also tell userspace whether it already has that buffer in it's userspace buffer manager, but that's an additional optimization. With plain dma-buf we could achieve the same by wiring up a real stat() implementation with unique inode numbers (atm they all share the anon_inode singleton). But thus far no one asked for that.

btw I'm lost a bit in the discussion (was on vacation), but I think all the concerns I've noticed with the initial rfc have been raised already, so things look good. I'll check the next rfc once that shows up.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Quoted 17 lines · On Mon, 19 Jun 2017 08:38:32 +0200 Gerd Hoffmann <[email protected]> wrote:

Hi,

Quoted 4 lines

My suggestion was to use vfio device fd for this ioctl and have dmabuf
mgr fd as member in above query_plane structure, for region type it would be set to 0.

Region type should be DRM_PLANE_TYPE_PRIMARY

Quoted 7 lines

Can't mmap that page to get surface information. There is no way to synchronize between QEMU reading this mmapped region and vendor driver
writing it. There could be race condition in these two operations. Read
on this page should be trapped and blocking, so that surface in that region is only updated when its asked for.

Quoted 2 lines

Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?

It seems less cumbersome than checking each field to see if it has changed. Thanks,

Alex