RE: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation

["Zhang, Tina" <tina.zhang@intel.com>]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Monday, November 6, 2017 5:01 PM
> To: Zhang, Tina <tina.zhang@intel.com>; alex.williamson@redhat.com;
> chris@chris-wilson.co.uk; joonas.lahtinen@linux.intel.com;
> zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> <zhi.a.wang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; daniel@ffwll.ch;
> kwankhede@nvidia.com
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-dev@lists.freedesktop.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > +/**
> > + * 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.
> > + * - Others are invalid and return -EINVAL.
> > + *
> > + * Return: 0 on success, -ENODEV with all out fields zero on mdev
> > + * device initialization, -errno on other failure.
> 
> Should also not here that it is not an error if the guest has not defined a plane
> yet.  The ioctl should return success in that case and zero-initialize plane info
> (drm_format + size + width + height fields).
Indeed. I just need to update the comments, as this version is implemented with this in mind. Thanks.

> 
> > + */
> > +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_* */
> 
> Add a head field here?  People asked @ kvm forum about multihead support.
> Even if the initial driver version doesn't support it we could add a field so it
> becomes easier to add it at some point in the future.
> 
> Probing for available heads could be done with the PROBE flag, i.e.
> flags = PROBE | DMABUF, plane_type = PRIMARY, head = 0, 1, ...
Does the multihead mean multiple display monitor? So each head can have its own one primary plane, one cursor plane and maybe some sprite planes if supported.
And the flow could be like this:
1) flags = PROBE | DMABUF, then return the number of heads in vfio_device_gfx_plane_info.head.
2) flags = DMABUF with plane_type = PRIMARY, head = 0, 1, ..., then return the id of the exposed framebuffer of that head.
Am I correct?

Another question is if the sprite plane is supported, how we're going to identify them, as there could be several sprite planes for one display monitor.
Add another field "num_plane" for it? Then, I guess the flow could be like this:
1) flags = PROBE | DMABUF, then return the number of heads in vfio_device_gfx_plane_info.head.
2) flags = PROBE | DMABUF and head = 0, 1, ..., and plane_type = PRIMARY/CURSOR/SPRITE, then return the num_plane.
3) flags = DMABUF with plane_type = PRIMARY, head = 0, 1, ..., num_plane =0, 1, ..., then return the id of the exposed framebuffer of that head.
Does it make sense?
Thanks.

> 
> > +	__u32 x_hot;    /* horizontal position of cursor hotspot */
> > +	__u32 y_hot;    /* vertical position of cursor hotspot */
> 
> Needs documentation how the driver signals "no hotspot information available"
> (using 0xffffffff for example).
This version has implemented this. I will update the comments.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev