Re: [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Wed, 11 Oct 2017 01:46:37 +0000
"Zhang, Tina" <tina.zhang@xxxxxxxxx> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> > Sent: Wednesday, October 11, 2017 3:17 AM
> > To: Zhang, Tina <tina.zhang@xxxxxxxxx>
> > Cc: kraxel@xxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx;
> > Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>;
> > Tian, Kevin <kevin.tian@xxxxxxxxx>; daniel@xxxxxxxx; kwankhede@xxxxxxxxxx;
> > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Subject: Re: [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Tue, 10 Oct 2017 17:50:05 +0800
> > Tina Zhang <tina.zhang@xxxxxxxxx> wrote:
> >   
> > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > > and get the plan and its related information. This ioctl can be invoked with:  
> > 
> > s/plan/plane/  
> Sorry about this typo :).
> 
> >   
> > > 1) either flag DMABUF or REGION is set. Vendor driver returns success
> > > and the plane_info only when the specific kind of buffer is supported.
> > > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver
> > > returns success only when the specific kind of buffer is supported.
> > >
> > > 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 needs to create new resource according to
> > > the new fd or just use the existed resource related to the old fd.
> > >
> > > v15:
> > > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd)
> > >
> > > v14:
> > > - add PROBE, DMABUF and REGION flags. (Alex)
> > >
> > > v12:
> > > - add drm_format_mod back. (Gerd and Zhenyu)
> > > - add region_index. (Gerd)
> > >
> > > v11:
> > > - rename plane_type to drm_plane_type. (Gerd)
> > > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info.
> > >   (Gerd)
> > > - remove drm_format_mod, start fields. (Daniel)
> > > - remove plane_id.
> > >
> > > v10:
> > > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd)
> > >
> > > v3:
> > > - add a field gvt_plane_info in the drm_i915_gem_obj structure to save
> > >   the decoded plane information to avoid look up while need the plane
> > >   info. (Gerd)
> > >
> > > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> > > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
> > >  include/uapi/linux/vfio.h | 62
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index ae46105..fdf9a9c 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -502,6 +502,68 @@ 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)
> > > + *
> > > + * Set the drm_plane_type and flags, then retrieve the gfx plane info.
> > > + *
> > > + * flags supported:
> > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF  
> > are set  
> > > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no
> > > + *   support for dma-buf.
> > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION  
> > are set  
> > > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > > + *   support for region.
> > > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION  
> > is set  
> > > + *   with each call to query the plane info.  
> > 
> > So dmabuf_id is effectively just a token that can be fed into GET_GFX_DMABUF
> > to get the fd.  The implementation of the token is vendor specific, but can be
> > thought of as some sort of sequence ID or generation ID (but not necessarily
> > monotonically increasing), so GET_GFX_DMABUF may fail if the previously
> > provided dmabuf_id is no longer valid.  Do I have this correct?  
> Exactly. GET_GFX_DMABUF may fail if the dmabuf_id is no longer valid. -EINVAL will be
> returned in that case. 
> And dmabuf_id is invalid when:
> 1) user space has closed all dma-buf fds which are exposed with the dmabuf_id,
> 2) or the dmabuf_id isn't returned from QUERY_GFX_PLANE.
> I will add the comments. Thanks.
> 
> >   
> > > + * - Others are invalid and return -EINVAL.  
> > 
> > And I see that in patch 7/7 that i915 is checking explicitly for only these flag
> > combinations, great!
> >   
> > > + *
> > > + * Return: 0 on success, -ENODEV with all out fields zero on mdev
> > > + * device initialization, -errno on other failure.
> > > + */
> > > +struct vfio_device_gfx_plane_info {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) #define
> > > +VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) #define
> > > +VFIO_GFX_PLANE_TYPE_REGION (1 << 2)
> > > +	/* in */
> > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > +	/* out */
> > > +	__u32 drm_format;	/* drm format of plane */
> > > +	__u64 drm_format_mod;   /* tiled mode */
> > > +	__u32 width;	/* width of plane */
> > > +	__u32 height;	/* height of plane */
> > > +	__u32 stride;	/* stride of plane */
> > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > +	__u32 x_pos;	/* horizontal position of cursor plane */
> > > +	__u32 y_pos;	/* vertical position of cursor plane*/
> > > +	union {
> > > +		__u32 region_index;	/* region index */
> > > +		__s32 dmabuf_id;	/* dma-buf fd */  
> > 
> > "dma-buf fd", but it's not an fd.  Why is this signed since it's no longer an fd?  
> Again, sorry about the typo :)
> 
> >   
> > > +	};
> > > +};
> > > +
> > > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> > > +
> > > +/**
> > > + * VFIO_DEVICE_GET_GFX_DMABUF - _IOW(VFIO_TYPE, VFIO_BASE + 15,
> > > + *				    struct vfio_device_gfx_dmabuf_fd)
> > > + *
> > > + * Return: 0 on success, -errno on failure.
> > > + */  
> > 
> > So given a dmabuf_id, return a dmabuf_fd, which may be the same as a fd
> > previously returned to the user.  In the latter case, can we assume that there's
> > no new reference added to the existing fd?  Is the user permitted to cache the
> > dmabuf_id to dmabuf_fd mapping?  Similarly, if a user already has a dmabuf_fd,
> > can calling GET_GFX_DMABUF ever fail or return a different result for the same
> > dmabuf_id which provided that fd?  Maybe we could define some of these
> > behaviors in the header file to keep vendor implementations consistent.  
> 
> Actually, a new fd is returned with each time GET_GFX_DMABUF invoked, even
> with the same dmabuf_id. The benefit is to leave the dma-buf's life cycle management
> to userspace. The dma-buf will be released when all its fds in userspace are closed.

Ok, so userspace could actually have multiple dmabuf_fds open at the
same time for the same dmabuf_id if they were sloppy with management of
those fds.

> Then, I think I need to add some comments like this:
> "dmabuf_fd will return a new fd with each time GET_GFX_DMABUF being invoked"

Yes, that would be great.

> >   
> > > +struct vfio_device_gfx_dmabuf_fd {
> > > +	__u32 argsz;
> > > +	__u32 flags;  
> > 
> > Patch 7/7 however fails to validate this flags field.  It needs to return -EINVAL if
> > any unsupported flags are set, which at this point is all of them.  
> Indeed.
>  
> >   
> > > +	/* in */
> > > +	__u32 dmabuf_id;  
> > 
> > This unsigned dmabuf_id is probably correct, but it doesn't match the signed
> > one provided above.  
> Will make them both unsigned :)
> 
> >   
> > > +	/* out */
> > > +	__s32 dmabuf_fd;
> > > +};  
> > 
> > VFIO_GROUP_GET_DEVICE_FD returns the fd through the return code, is there
> > a reason to do it differently here?  GET_DEVICE_FD also only takes a char
> > parameter without the extra future proofing, I wonder if this ioctl is simple
> > enough to do the same.  
> So that means we don't need the "flages" field and change dmabuf_id to a character string.
> Maybe it's fine for the current usage. But, I'm not sure if it's suitable for future.
> Gerd, Zhenyu and Zhi, how do you think about it?

No, the parameter wouldn't be a char, you'd use an __u32 for the
dmabuf_id.  I'm just referencing the structure of the GET_DEVICE_FD as
an ioctl which returns an fd through the return value and takes a
single parameter.  I don't mean to imply matching the type of that
parameter.  Gerd, what are your thoughts on that, it'd make it slightly
easier to call GET_GFX_DMABUF, but it limits us to file descriptor for
the dmabuf whereas with a flag and expandable structure we could use
some future mechanism to return the dmabuf to userspace.  Thanks,

Alex