RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Daniel Vetter
> Sent: Tuesday, July 11, 2017 5:16 PM
> To: Zhang, Tina <tina.zhang@xxxxxxxxx>
> Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; kwankhede@xxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx;
> chris@xxxxxxxxxxxxxxxxxx; alex.williamson@xxxxxxxxxx; Lv, Zhiyuan
> <zhiyuan.lv@xxxxxxxxx>; daniel@xxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx;
> Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; kraxel@xxxxxxxxxx
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, Jul 06, 2017 at 06:29:55AM +0800, Tina Zhang wrote:
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it need to creat new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..c92bc69 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *                           struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +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;
> > +};
> 
> Would be good to have a detailed spec of all this stuff, plus what's it meant to be
> used for. I assume that e.g. start would be the opaque cookie thing we've talked
> about, for dma-buf reuse? Otherwise I'm not sure what it's good for, since the
> same gpu vram address can be reused for different memory objects ...

Yes, I will add more comments in the next version. Here, some historical reason might be helpful to understand. Previously, we reported all the information to user mode and let user mode to decide whether it was an exported dmabuf or not. If it was an exported dmabuf, user mode can directly use its cached resources, without needing to create again. This design turned out to have some limitations:
1). User mode has to keep so many information to do the comparing.
2). The design needs at least two ioctls, with one for query info and the other one for exporting dmabuf. So, there will be a time window between these two ioctls, during which the guest framebuffer might be changed.

In this patch, we leave the comparing logic to kernel, and return the dmabuf fd everytime when user mode calling VFIO_DEVICE_QUERY_GFX_PLANE ioctl. User mode can compare the value of fd to see whether it can reuse the resource of the old fd, or need to create new according to the new fd.
If we take the idea in this patch, we don't need so many fields in the struct vfio_device_gfx_plane_info which seem meaningless to user mode. I'm going to remove some in the next version, including start.
Thanks.

Tina
> > +
> > +struct vfio_device_query_gfx_plane {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	struct vfio_device_gfx_plane_info plane_info;
> > +	__u32 plane_type;
> > +	__s32 fd; /* dma-buf fd */
> > +	__u32 plane_id;
> > +};
> 
> As mentioned in the other reply already, I'm not sure what plane_type is for.
> Otherwise this looks ok-ish, but hard to tell without more detailed spec.
> -Daniel
> 
> > +
> > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> > +
> > +
> >  /* -------- API for Type1 VFIO IOMMU -------- */
> >
> >  /**
> > --
> > 2.7.4
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev