RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

From: Zhang, Tina
Date: Wed Jun 21 2017 - 05:21:03 EST

Thanks for all the comments. I'm planning to cook the next version of this patch set which I'd like to include all the comments and ideas. Here is the summary:

1. How to figure the device capabilities
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.
Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF (1 << 5) /* vfio-dmabuf device */
#define VFIO_DEVICE_FLAGS_GFX_REGION (1 << 6) /* vfio-gfx-region 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;
};
> 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?

4. Two ioctl commands
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

5. > Then we should kill off the manager fd unless there are arguments that still give it value.
Agree.

If there is anything I miss, please tell us.

Thanks.
Tina


> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: Wednesday, June 21, 2017 7:22 AM
> To: Zhang, Tina <tina.zhang@xxxxxxxxx>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Kirti Wankhede <kwankhede@xxxxxxxxxx>;
> Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>; intel-gvt-
> dev@xxxxxxxxxxxxxxxxxxxxx; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Wang, Zhi A
> <zhi.a.wang@xxxxxxxxx>; Wang, Zhenyu Z <zhenyu.z.wang@xxxxxxxxx>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Tue, 20 Jun 2017 23:01:53 +0000
> "Zhang, Tina" <tina.zhang@xxxxxxxxx> wrote:
>
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> > > Sent: Tuesday, June 20, 2017 11:00 PM
> > > To: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > Cc: Zhang, Tina <tina.zhang@xxxxxxxxx>;
> > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux- kernel@xxxxxxxxxxxxxxx;
> > > Kirti Wankhede <kwankhede@xxxxxxxxxx>; Chen, Xiaoguang
> > > <xiaoguang.chen@xxxxxxxxx>; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx;
> > > Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Wang, Zhi A
> > > <zhi.a.wang@xxxxxxxxx>; Wang, Zhenyu Z <zhenyu.z.wang@xxxxxxxxx>
> > > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based
> > > dma-buf operations
> > >
> > > On Tue, 20 Jun 2017 12:57:36 +0200
> > > Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
> > >
> > > > On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> > > > > 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;
> > > > > };
> > > >
> > > > Looks good to me.
> > > >
> > > > > 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; };
> > >
> > > 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.
> > >
> > > > 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.
> > >
> > > > 2. Remove dmabuf mgr fd and add these two ioctl commands to the
> > > > vfio device fd.
> > > > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > > > vfio_device_gfx_plane_info.
> > > >
> > > > Yes.
> > > >
> > > > > 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.
> > 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.
>
> 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
Thanks Alex. I guess we all prefer the first one.

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