[v11,1/1] vfio: ABI for mdev display dma-buf operation

Commit Message

Zhang, Tina July 12, 2017, 6:10 a.m. UTC

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 needs to create new resource according to
the new fd or just use the existed resource related to the old fd.

Signed-off-by: Tina Zhang <[e-mail]>

Comments

Zhang, Tina July 12, 2017, 6:56 a.m. UTC | #1

> -----Original Message-----
> From: Zhang, Tina
> Sent: Wednesday, July 12, 2017 2:11 PM
> To: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Lv, Zhiyuan <[e-mail]>; Wang, Zhi A
> <[e-mail]>; Tian, Kevin <[e-mail]>; [e-mail];
> [e-mail]
> Cc: Zhang, Tina <[e-mail]>; [e-mail]; intel-
> [e-mail]
> Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> 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 needs to create new resource according to the new fd or just use the
> existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <[e-mail]>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> ae46105..c176cc8 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vfio_device_gfx_plane_info {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> +	/* out */
> +	__u32 drm_format;	/* drm format of plane */
> +	__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, upper left corner
> in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> lines*/
> +	__s32 fd;	/* dma-buf fd */
> +};
> +
I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I don't know how it could be used by region usage.
So, if someone can explain its usage, I can add it back. Thanks.

Another open is, so far, our design is focused on dmabuf based gfx plane only. Can we go a step further to consider a general
Buffer sharing interface? If we reference V4L2, we can see dmabuf can be considered as one kind of buffers, we can have  more
kinds of buffers, like mmap buffer and so on. So, if we think about that, we may need another ioctl to ask the mdev device which
kind of buffer it supports, and then use the VFIO_DEVICE_QUERY_* ioctl to get it back for user mode accessing. Different kinds
of mdev devices can have different query ioctl name and structure. Is it reasonable?
Thanks.

Tina

> +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> +
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
> 
>  /**
> --
> 2.7.4

Daniel Vetter July 12, 2017, 7:54 a.m. UTC | #2

On Wed, Jul 12, 2017 at 02:10:51PM +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 needs to create new resource according to
> the new fd or just use the existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <[e-mail]>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..c176cc8 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vfio_device_gfx_plane_info {
> +	__u32 argsz;

What's argsz for?

> +	__u32 flags;
> +	/* in */
> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */

Do we plan to support multiple outputs on a vfio? In that case just the
plane-type doesn't identify the right plane. Also, if you want to support
overlay planes, then usually hw has a lot of those.
-Daniel

> +	/* out */
> +	__u32 drm_format;	/* drm format of plane */
> +	__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, upper left corner in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/

Why is x/y pos only for cursors? I'm not clear at all what this would be
for ... Is this an offset within the plane where the cursor starts?
-Daniel

> +	__s32 fd;	/* dma-buf fd */
> +};
> +
> +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> +
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**
> -- 
> 2.7.4
>

Daniel Vetter July 12, 2017, 7:56 a.m. UTC | #3

On Wed, Jul 12, 2017 at 06:56:53AM +0000, Zhang, Tina wrote:
> 
> 
> > -----Original Message-----
> > From: Zhang, Tina
> > Sent: Wednesday, July 12, 2017 2:11 PM
> > To: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Lv, Zhiyuan <[e-mail]>; Wang, Zhi A
> > <[e-mail]>; Tian, Kevin <[e-mail]>; [e-mail];
> > [e-mail]
> > Cc: Zhang, Tina <[e-mail]>; [e-mail]; intel-
> > [e-mail]
> > Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> > 
> > 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 needs to create new resource according to the new fd or just use the
> > existed resource related to the old fd.
> > 
> > Signed-off-by: Tina Zhang <[e-mail]>
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> > ae46105..c176cc8 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> > + *
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	/* in */
> > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > +	/* out */
> > +	__u32 drm_format;	/* drm format of plane */
> > +	__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, upper left corner
> > in pixels */
> > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > lines*/
> > +	__s32 fd;	/* dma-buf fd */
> > +};
> > +
> I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I don't know how it could be used by region usage.
> So, if someone can explain its usage, I can add it back. Thanks.
> 
> Another open is, so far, our design is focused on dmabuf based gfx plane only. Can we go a step further to consider a general
> Buffer sharing interface? If we reference V4L2, we can see dmabuf can be considered as one kind of buffers, we can have  more
> kinds of buffers, like mmap buffer and so on. So, if we think about that, we may need another ioctl to ask the mdev device which
> kind of buffer it supports, and then use the VFIO_DEVICE_QUERY_* ioctl to get it back for user mode accessing. Different kinds
> of mdev devices can have different query ioctl name and structure. Is it reasonable?

This is what dma-buf are for. v4l2 also supports dma-buf, and dma-buf
support mmap. I think dma-buf will give you everything you want.

Aside: You're threading is broken somehow, the patch isn't linked to the
cover letter. Not sure what's wrong, I thought latest git gets this right
by default ...
-Daniel

Zhenyu Wang July 12, 2017, 9:48 a.m. UTC | #4

On 2017.07.12 09:56:11 +0200, Daniel Vetter wrote:
> On Wed, Jul 12, 2017 at 06:56:53AM +0000, Zhang, Tina wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Zhang, Tina
> > > Sent: Wednesday, July 12, 2017 2:11 PM
> > > To: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Lv, Zhiyuan <[e-mail]>; Wang, Zhi A
> > > <[e-mail]>; Tian, Kevin <[e-mail]>; [e-mail];
> > > [e-mail]
> > > Cc: Zhang, Tina <[e-mail]>; [e-mail]; intel-
> > > [e-mail]
> > > Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> > > 
> > > 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 needs to create new resource according to the new fd or just use the
> > > existed resource related to the old fd.
> > > 
> > > Signed-off-by: Tina Zhang <[e-mail]>
> > > 
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> > > ae46105..c176cc8 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> > > + *
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +struct vfio_device_gfx_plane_info {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +	/* in */
> > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > +	/* out */
> > > +	__u32 drm_format;	/* drm format of plane */
> > > +	__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, upper left corner
> > > in pixels */
> > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > > lines*/
> > > +	__s32 fd;	/* dma-buf fd */
> > > +};
> > > +
> > I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I don't know how it could be used by region usage.
> > So, if someone can explain its usage, I can add it back. Thanks.
> > 
> > Another open is, so far, our design is focused on dmabuf based gfx plane only. Can we go a step further to consider a general
> > Buffer sharing interface? If we reference V4L2, we can see dmabuf can be considered as one kind of buffers, we can have  more
> > kinds of buffers, like mmap buffer and so on. So, if we think about that, we may need another ioctl to ask the mdev device which
> > kind of buffer it supports, and then use the VFIO_DEVICE_QUERY_* ioctl to get it back for user mode accessing. Different kinds
> > of mdev devices can have different query ioctl name and structure. Is it reasonable?
> 
> This is what dma-buf are for. v4l2 also supports dma-buf, and dma-buf
> support mmap. I think dma-buf will give you everything you want.
>

yep, I think Tina's point is to how to provide that dmabuf interface
properly, either in this case for vgpu display specifically or any
benefit to provide a generic buffer expose/share interface for
vfio/mdev. But even for vgpu display interface, e.g gvt driver would
go with dmabuf but nv driver will go with region based, then I don't
think we could easily have a generic enough design for every mdev type
or driver. So I believe we should stick with and hopefully get aligned
for this interface now.

Zhenyu Wang July 12, 2017, 9:55 a.m. UTC | #5

On 2017.07.12 09:54:13 +0200, Daniel Vetter wrote:
> On Wed, Jul 12, 2017 at 02:10:51PM +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 needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> > 
> > Signed-off-by: Tina Zhang <[e-mail]>
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..c176cc8 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> > + *
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +	__u32 argsz;
> 
> What's argsz for?

It's vfio ioctl idiom for parameter sanity check.

> 
> > +	__u32 flags;
> > +	/* in */
> > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> 
> Do we plan to support multiple outputs on a vfio? In that case just the
> plane-type doesn't identify the right plane. Also, if you want to support
> overlay planes, then usually hw has a lot of those.

Currently no plan to support multiple outputs on one mdev. And no overlay
plane support in gvt now, even we'd support that maybe also one plane only.

> 
> > +	/* out */
> > +	__u32 drm_format;	/* drm format of plane */
> > +	__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, upper left corner in pixels */
> > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
> 
> Why is x/y pos only for cursors? I'm not clear at all what this would be
> for ... Is this an offset within the plane where the cursor starts?

yes, it's cursor start in primary plane.

> 
> > +	__s32 fd;	/* dma-buf fd */
> > +};
> > +
> > +#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

Daniel Vetter July 12, 2017, 10:07 a.m. UTC | #6

On Wed, Jul 12, 2017 at 05:48:31PM +0800, Zhenyu Wang wrote:
> On 2017.07.12 09:56:11 +0200, Daniel Vetter wrote:
> > On Wed, Jul 12, 2017 at 06:56:53AM +0000, Zhang, Tina wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Zhang, Tina
> > > > Sent: Wednesday, July 12, 2017 2:11 PM
> > > > To: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Lv, Zhiyuan <[e-mail]>; Wang, Zhi A
> > > > <[e-mail]>; Tian, Kevin <[e-mail]>; [e-mail];
> > > > [e-mail]
> > > > Cc: Zhang, Tina <[e-mail]>; [e-mail]; intel-
> > > > [e-mail]
> > > > Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> > > > 
> > > > 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 needs to create new resource according to the new fd or just use the
> > > > existed resource related to the old fd.
> > > > 
> > > > Signed-off-by: Tina Zhang <[e-mail]>
> > > > 
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> > > > ae46105..c176cc8 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vfio_device_gfx_plane_info {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	/* in */
> > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > > +	/* out */
> > > > +	__u32 drm_format;	/* drm format of plane */
> > > > +	__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, upper left corner
> > > > in pixels */
> > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > > > lines*/
> > > > +	__s32 fd;	/* dma-buf fd */
> > > > +};
> > > > +
> > > I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I don't know how it could be used by region usage.
> > > So, if someone can explain its usage, I can add it back. Thanks.
> > > 
> > > Another open is, so far, our design is focused on dmabuf based gfx plane only. Can we go a step further to consider a general
> > > Buffer sharing interface? If we reference V4L2, we can see dmabuf can be considered as one kind of buffers, we can have  more
> > > kinds of buffers, like mmap buffer and so on. So, if we think about that, we may need another ioctl to ask the mdev device which
> > > kind of buffer it supports, and then use the VFIO_DEVICE_QUERY_* ioctl to get it back for user mode accessing. Different kinds
> > > of mdev devices can have different query ioctl name and structure. Is it reasonable?
> > 
> > This is what dma-buf are for. v4l2 also supports dma-buf, and dma-buf
> > support mmap. I think dma-buf will give you everything you want.
> >
> 
> yep, I think Tina's point is to how to provide that dmabuf interface
> properly, either in this case for vgpu display specifically or any
> benefit to provide a generic buffer expose/share interface for
> vfio/mdev. But even for vgpu display interface, e.g gvt driver would
> go with dmabuf but nv driver will go with region based, then I don't
> think we could easily have a generic enough design for every mdev type
> or driver. So I believe we should stick with and hopefully get aligned
> for this interface now.

If you expose a dma-buf, you _can_ mmap that thing. Not sure what you mean
with "region", but at least within drm anything that exposes physical
addresses to userspace is not allowed. Only thing you're allowed to do is
have per-process gpu address spaces, because otherwise we can't move stuff
around. Virtualization might be a bit different, but then I'm not clear at
all how exactly this all interacts with nouveau.ko.
-Daniel

Zhang, Tina July 14, 2017, 2:12 a.m. UTC | #7

> -----Original Message-----
> From: intel-gvt-dev [mailto:[e-mail]] On
> Behalf Of Zhenyu Wang
> Sent: Wednesday, July 12, 2017 5:49 PM
> To: Daniel Vetter <[e-mail]>
> Cc: Tian, Kevin <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; chris@chris-
> wilson.co.uk; [e-mail]; [e-mail]; Zhang, Tina
> <[e-mail]>; [e-mail]; Wang, Zhi A
> <[e-mail]>; Lv, Zhiyuan <[e-mail]>
> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> On 2017.07.12 09:56:11 +0200, Daniel Vetter wrote:
> > On Wed, Jul 12, 2017 at 06:56:53AM +0000, Zhang, Tina wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Tina
> > > > Sent: Wednesday, July 12, 2017 2:11 PM
> > > > To: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; Lv, Zhiyuan
> > > > <[e-mail]>; Wang, Zhi A <[e-mail]>; Tian,
> > > > Kevin <[e-mail]>; [e-mail];
> > > > [e-mail]
> > > > Cc: Zhang, Tina <[e-mail]>;
> > > > [e-mail]; intel-
> > > > [e-mail]
> > > > Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > 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 needs to create new resource
> > > > according to the new fd or just use the existed resource related to the old
> fd.
> > > >
> > > > Signed-off-by: Tina Zhang <[e-mail]>
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index
> > > > ae46105..c176cc8 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,32 @@ 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 retrieve information about the gfx plane.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vfio_device_gfx_plane_info {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	/* in */
> > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > > +	/* out */
> > > > +	__u32 drm_format;	/* drm format of plane */
> > > > +	__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, upper left corner
> > > > in pixels */
> > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > > > lines*/
> > > > +	__s32 fd;	/* dma-buf fd */
> > > > +};
> > > > +
> > > I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I
> don't know how it could be used by region usage.
> > > So, if someone can explain its usage, I can add it back. Thanks.
> > >
> > > Another open is, so far, our design is focused on dmabuf based gfx
> > > plane only. Can we go a step further to consider a general Buffer
> > > sharing interface? If we reference V4L2, we can see dmabuf can be
> > > considered as one kind of buffers, we can have  more kinds of
> > > buffers, like mmap buffer and so on. So, if we think about that, we may
> need another ioctl to ask the mdev device which kind of buffer it supports, and
> then use the VFIO_DEVICE_QUERY_* ioctl to get it back for user mode accessing.
> Different kinds of mdev devices can have different query ioctl name and
> structure. Is it reasonable?
> >
> > This is what dma-buf are for. v4l2 also supports dma-buf, and dma-buf
> > support mmap. I think dma-buf will give you everything you want.
> >
> 
> yep, I think Tina's point is to how to provide that dmabuf interface properly,
> either in this case for vgpu display specifically or any benefit to provide a generic
> buffer expose/share interface for vfio/mdev. But even for vgpu display interface,
> e.g gvt driver would go with dmabuf but nv driver will go with region based,
> then I don't think we could easily have a generic enough design for every mdev
> type or driver. So I believe we should stick with and hopefully get aligned for this
> interface now.
Thanks, Zhenyu. I'm just wondering to know if it is reasonable to support a general buffer expose/share interface for vfio/mdev device. After all, what we do here is to provide a way to expose/share the mdev buffer to host Apps, (not sure whether different mdev devices would also be interested in this), e.g. Qemu. Is it possible that we define a genera way to support different kinds of buffers? For example, could region be a kind of buffer existing with dma-buf type of buffer. Is there any other flavor of buffer we want to support, if we take some reference for V4L2 to buffer support
Enum v4l2_memory {
	V4L2_MEMORY_MMAP
	V4L2_MEMORY_USERPTR
	V4L2_MEMORY_OVERLAY
	V4L2_MEMORY_DMABUF
}
Thanks.

Tina
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

Gerd Hoffmann July 14, 2017, 10:05 a.m. UTC | #8

Hi,

> Another open is, so far, our design is focused on dmabuf based gfx
> plane only. Can we go a step further to consider a general
> Buffer sharing interface? If we reference V4L2, we can see dmabuf can
> be considered as one kind of buffers, we can have  more
> kinds of buffers, like mmap buffer and so on.

regions would effectively be mmap buffers, no?

cheers,
  Gerd

Gerd Hoffmann July 14, 2017, 10:11 a.m. UTC | #9

Hi,

> +struct vfio_device_gfx_plane_info {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 drm_plane_type;	/* type of plane:
> DRM_PLANE_TYPE_* */
> +	/* out */
> +	__u32 drm_format;	/* drm format of plane */

DRM_FORMAT_*

drm_format_mod is gone?  Not needed?
How tiled buffers are handled then?

cheers,
  Gerd

Zhang, Tina July 17, 2017, 1:10 a.m. UTC | #10

> -----Original Message-----

> From: intel-gvt-dev [mailto:[e-mail]] On

> Behalf Of Gerd Hoffmann

> Sent: Friday, July 14, 2017 6:11 PM

> To: Zhang, Tina <[e-mail]>; [e-mail];

> [e-mail]; [e-mail]; Lv, Zhiyuan

> <[e-mail]>; Wang, Zhi A <[e-mail]>; Tian, Kevin

> <[e-mail]>; [e-mail]; [e-mail]

> Cc: [e-mail]; [e-mail]

> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation

> 

>   Hi,

> 

> > +struct vfio_device_gfx_plane_info {

> > +	__u32 argsz;

> > +	__u32 flags;

> > +	/* in */

> > +	__u32 drm_plane_type;	/* type of plane:

> > DRM_PLANE_TYPE_* */

> > +	/* out */

> > +	__u32 drm_format;	/* drm format of plane */

> 

> DRM_FORMAT_*

> 

> drm_format_mod is gone?  Not needed?

> How tiled buffers are handled then?

Drm_format_mod is used as one of the plane's characteristics when judging the dma-buf of the plane is new to expose or an old exposed one. As from V10 we leave this comparing logic to kernel, user mode might not need this field any more. If user mode wants, this can be also got though drm APIs. For example, eglCreateImageKHR() uses drm APIs to get the tiling format, without asking the invoker to explicitly use it as a parameter. 
Do you think this field is still needed?

Tina
> 

> cheers,

>   Gerd

> 

> _______________________________________________

> intel-gvt-dev mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Zhenyu Wang July 17, 2017, 2:26 a.m. UTC | #11

On 2017.07.17 01:10:03 +0000, Zhang, Tina wrote:
> > > +struct vfio_device_gfx_plane_info {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +	/* in */
> > > +	__u32 drm_plane_type;	/* type of plane:
> > > DRM_PLANE_TYPE_* */
> > > +	/* out */
> > > +	__u32 drm_format;	/* drm format of plane */
> > 
> > DRM_FORMAT_*
> > 
> > drm_format_mod is gone?  Not needed?
> > How tiled buffers are handled then?
> Drm_format_mod is used as one of the plane's characteristics when judging the dma-buf of the plane is new to expose or an old exposed one. As from V10 we leave this comparing logic to kernel, user mode might not need this field any more. If user mode wants, this can be also got though drm APIs. For example, eglCreateImageKHR() uses drm APIs to get the tiling format, without asking the invoker to explicitly use it as a parameter. 
> Do you think this field is still needed?
> 

Of course we need that modifier for complete format info. Don't just
think for i915 usage, there's possible modifier for other vendor
driver, and it's required for e.g ADDFB2 in drm kms. Pls add it back
in next version.

Zhang, Tina July 19, 2017, 12:55 a.m. UTC | #12

> -----Original Message-----
> From: intel-gvt-dev [mailto:[e-mail]] On
> Behalf Of Zhenyu Wang
> Sent: Monday, July 17, 2017 10:27 AM
> To: Zhang, Tina <[e-mail]>
> Cc: Tian, Kevin <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Lv, Zhiyuan <[e-mail]>;
> [e-mail]; [e-mail]; Wang, Zhi A
> <[e-mail]>; Gerd Hoffmann <[e-mail]>
> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> On 2017.07.17 01:10:03 +0000, Zhang, Tina wrote:
> > > > +struct vfio_device_gfx_plane_info {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	/* in */
> > > > +	__u32 drm_plane_type;	/* type of plane:
> > > > DRM_PLANE_TYPE_* */
> > > > +	/* out */
> > > > +	__u32 drm_format;	/* drm format of plane */
> > >
> > > DRM_FORMAT_*
> > >
> > > drm_format_mod is gone?  Not needed?
> > > How tiled buffers are handled then?
> > Drm_format_mod is used as one of the plane's characteristics when judging
> the dma-buf of the plane is new to expose or an old exposed one. As from V10
> we leave this comparing logic to kernel, user mode might not need this field any
> more. If user mode wants, this can be also got though drm APIs. For example,
> eglCreateImageKHR() uses drm APIs to get the tiling format, without asking the
> invoker to explicitly use it as a parameter.
> > Do you think this field is still needed?
> >
> 
> Of course we need that modifier for complete format info. Don't just think for
> i915 usage, there's possible modifier for other vendor driver, and it's required
> for e.g ADDFB2 in drm kms. Pls add it back in next version.
We definitely can add it back if it is thought to be useful. Just some curious, as we don't produce the content in the GEM buffer (a.k.a we just expose it here), why we need to know the tile mode. In addition, DRM API can support the functionality to get the tile mode ot GEM buffer. 

Thanks,
Tina
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

Zhenyu Wang July 19, 2017, 2:09 a.m. UTC | #13

On 2017.07.19 00:55:19 +0000, Zhang, Tina wrote:
> > 
> > Of course we need that modifier for complete format info. Don't just think for
> > i915 usage, there's possible modifier for other vendor driver, and it's required
> > for e.g ADDFB2 in drm kms. Pls add it back in next version.
> We definitely can add it back if it is thought to be useful. Just some curious, as we don't produce the content in the GEM buffer (a.k.a we just expose it here), why we need to know the tile mode. In addition, DRM API can support the functionality to get the tile mode ot GEM buffer. 
> 

That's vendor driver specific ioctl API instead of DRM API.
And besides tiling there could be other vendor's modifiers
which need to be kept for complete info.

Patch

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..c176cc8 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,32 @@  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 retrieve information about the gfx plane.
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+struct vfio_device_gfx_plane_info {
+	__u32 argsz;
+	__u32 flags;
+	/* in */
+	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
+	/* out */
+	__u32 drm_format;	/* drm format of plane */
+	__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, upper left corner in pixels */
+	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
+	__s32 fd;	/* dma-buf fd */
+};
+
+#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
+
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**