[v13,5/7] vfio: ABI for mdev display dma-buf operation

Commit Message

Zhang, Tina July 25, 2017, 9:28 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]>
---
 include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Gerd Hoffmann July 28, 2017, 8:27 a.m. UTC | #1

Hi,

> +/**
> + * 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.

I think this should be more verbose, especially documenting that the
"guest driver didn't initialize the display yet" case isn't and error
and fields should be set to zero then (as discussed on the list).

> + */
> +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 */
> +	__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,
> upper left corner in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane,
> upper left corner in lines*/
> +	__u32 region_index;
> +	__s32 fd;	/* dma-buf fd */
> +};

Looks good to me.

Unfortunately I havn't been able to test the whole series yet due to
being busy with other stuff, and I'm about to leave for my summer
vacation.  Will be back online on Aug 21st.

cheers,
  Gerd

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

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

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

> Behalf Of Gerd Hoffmann

> Sent: Friday, July 28, 2017 4:27 PM

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

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

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

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

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

> [e-mail]; Tian, Kevin <[e-mail]>

> Subject: Re: [PATCH v13 5/7] 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 retrieve information about the gfx

> > plane.

> + *

> > + * Return: 0 on success, -errno on failure.

> 

> I think this should be more verbose, especially documenting that the "guest

> driver didn't initialize the display yet" case isn't and error and fields should be set

> to zero then (as discussed on the list).

I can add this in the next version. 
Thanks.

Tina
> 

> > + */

> > +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 */

> > +	__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,

> > upper left corner in pixels */

> > +	__u32 y_pos;	/* vertical position of cursor plane,

> > upper left corner in lines*/

> > +	__u32 region_index;

> > +	__s32 fd;	/* dma-buf fd */

> > +};

> 

> Looks good to me.

> 

> Unfortunately I havn't been able to test the whole series yet due to being busy

> with other stuff, and I'm about to leave for my summer vacation.  Will be back

> online on Aug 21st.

Fine to me. I will also update our qemu sample code and some wiki according to the current interface in the next version, which
may give you some help for your test.
Thanks.

Tina

> 

> cheers,

>   Gerd

> 

> 

> _______________________________________________

> intel-gvt-dev mailing list

> [e-mail]

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

Alex Williamson Aug. 1, 2017, 9:18 p.m. UTC | #3

On Tue, 25 Jul 2017 17:28:18 +0800
Tina Zhang <[e-mail]> 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]>
> ---
>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..827a230 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,34 @@ 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 */
> +	__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, upper left corner in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
> +	__u32 region_index;
> +	__s32 fd;	/* dma-buf fd */

How do I know which of these is valid, region_index or fd?  Can I ask
for one vs the other?  What are the errno values to differentiate
unsupported vs not initialized?  Is there a "probe" flag that I can use
to determine what the device supports without allocating those
resources yet?

Kirti, does this otherwise meet your needs?

Thanks,
Alex

Kirti Wankhede Aug. 2, 2017, 3:56 p.m. UTC | #4

On 8/2/2017 2:48 AM, Alex Williamson wrote:
> On Tue, 25 Jul 2017 17:28:18 +0800
> Tina Zhang <[e-mail]> 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]>
>> ---
>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index ae46105..827a230 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -502,6 +502,34 @@ 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 */
>> +	__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, upper left corner in pixels */
>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
>> +	__u32 region_index;
>> +	__s32 fd;	/* dma-buf fd */
> 
> How do I know which of these is valid, region_index or fd?  Can I ask
> for one vs the other?  What are the errno values to differentiate
> unsupported vs not initialized?  Is there a "probe" flag that I can use
> to determine what the device supports without allocating those
> resources yet?
> 
> Kirti, does this otherwise meet your needs?
> 

I wanted to test my change with this interface, but I couldn't, was busy
with other stuff.
This structure looks good to me for region type support.

> What are the errno values to differentiate
> unsupported vs not initialized?

In previous version discussion, we decided that vendor driver should
set all members to 0 in such cases. Mainly, if size is 0 then there is
nothing to copy.

Thanks,
Kirti

> Thanks,
> Alex
>

Zhang, Tina Aug. 3, 2017, 3:17 a.m. UTC | #5

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

> From: dri-devel [mailto:[e-mail]] On Behalf Of

> Alex Williamson

> Sent: Wednesday, August 2, 2017 5:18 AM

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

> Cc: Tian, Kevin <[e-mail]>; [e-mail]; intel-

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

> [e-mail]; [e-mail]; intel-gvt-

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

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

> 

> On Tue, 25 Jul 2017 17:28:18 +0800

> Tina Zhang <[e-mail]> 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]>

> > ---

> >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++

> >  1 file changed, 28 insertions(+)

> >

> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h

> > index ae46105..827a230 100644

> > --- a/include/uapi/linux/vfio.h

> > +++ b/include/uapi/linux/vfio.h

> > @@ -502,6 +502,34 @@ 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 */

> > +	__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, upper left corner

> in pixels */

> > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in

> lines*/

> > +	__u32 region_index;

> > +	__s32 fd;	/* dma-buf fd */

> 

> How do I know which of these is valid, region_index or fd?  Can I ask for one vs

> the other?  What are the errno values to differentiate unsupported vs not

> initialized?  Is there a "probe" flag that I can use to determine what the device

> supports without allocating those resources yet?

Dma-buf won't use region_index, which means region_index is zero all the time for dma-buf usage. 
As we discussed, there won't be errno if not initialized, just keep all fields zero.
I will add the comments about these in the next version. Thanks

Tina

> 

> Kirti, does this otherwise meet your needs?

> 

> Thanks,

> Alex

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alex Williamson Aug. 3, 2017, 3:37 a.m. UTC | #6

On Thu, 3 Aug 2017 03:17:09 +0000
"Zhang, Tina" <[e-mail]> wrote:

> > -----Original Message-----
> > From: dri-devel [mailto:[e-mail]] On Behalf Of
> > Alex Williamson
> > Sent: Wednesday, August 2, 2017 5:18 AM
> > To: Zhang, Tina <[e-mail]>
> > Cc: Tian, Kevin <[e-mail]>; [e-mail]; intel-
> > [e-mail]; Wang, Zhi A <[e-mail]>;
> > [e-mail]; [e-mail]; intel-gvt-
> > [e-mail]; Lv, Zhiyuan <[e-mail]>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Tue, 25 Jul 2017 17:28:18 +0800
> > Tina Zhang <[e-mail]> 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]>
> > > ---
> > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index ae46105..827a230 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -502,6 +502,34 @@ 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 */
> > > +	__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, upper left corner  
> > in pixels */  
> > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in  
> > lines*/  
> > > +	__u32 region_index;
> > > +	__s32 fd;	/* dma-buf fd */  
> > 
> > How do I know which of these is valid, region_index or fd?  Can I ask for one vs
> > the other?  What are the errno values to differentiate unsupported vs not
> > initialized?  Is there a "probe" flag that I can use to determine what the device
> > supports without allocating those resources yet?  
> Dma-buf won't use region_index, which means region_index is zero all the time for dma-buf usage. 
> As we discussed, there won't be errno if not initialized, just keep all fields zero.
> I will add the comments about these in the next version. Thanks

Zero is a valid region index.

Zhang, Tina Aug. 3, 2017, 7:08 a.m. UTC | #7

> -----Original Message-----
> From: Alex Williamson [mailto:[e-mail]]
> Sent: Thursday, August 3, 2017 11:38 AM
> To: Zhang, Tina <[e-mail]>
> Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Wang, Zhi A <[e-mail]>; Lv,
> Zhiyuan <[e-mail]>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, 3 Aug 2017 03:17:09 +0000
> "Zhang, Tina" <[e-mail]> wrote:
> 
> > > -----Original Message-----
> > > From: dri-devel [mailto:[e-mail]] On
> > > Behalf Of Alex Williamson
> > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > To: Zhang, Tina <[e-mail]>
> > > Cc: Tian, Kevin <[e-mail]>; [e-mail]; intel-
> > > [e-mail]; Wang, Zhi A <[e-mail]>;
> > > [e-mail]; [e-mail]; intel-gvt-
> > > [e-mail]; Lv, Zhiyuan <[e-mail]>
> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > operation
> > >
> > > On Tue, 25 Jul 2017 17:28:18 +0800
> > > Tina Zhang <[e-mail]> 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]>
> > > > ---
> > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index ae46105..827a230 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,34 @@ 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 */
> > > > +	__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, upper left corner
> > > in pixels */
> > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > > lines*/
> > > > +	__u32 region_index;
> > > > +	__s32 fd;	/* dma-buf fd */
> > >
> > > How do I know which of these is valid, region_index or fd?  Can I
> > > ask for one vs the other?  What are the errno values to
> > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > flag that I can use to determine what the device supports without allocating
> those resources yet?
> > Dma-buf won't use region_index, which means region_index is zero all the
> time for dma-buf usage.
> > As we discussed, there won't be errno if not initialized, just keep all fields zero.
> > I will add the comments about these in the next version. Thanks
> 
> Zero is a valid region index.
If zero is valid, can we find out other invalid value? How about 0xffffffff?

Tina

Alex Williamson Aug. 3, 2017, 2:42 p.m. UTC | #8

On Thu, 3 Aug 2017 07:08:15 +0000
"Zhang, Tina" <[e-mail]> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:[e-mail]]
> > Sent: Thursday, August 3, 2017 11:38 AM
> > To: Zhang, Tina <[e-mail]>
> > Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Wang, Zhi A <[e-mail]>; Lv,
> > Zhiyuan <[e-mail]>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Thu, 3 Aug 2017 03:17:09 +0000
> > "Zhang, Tina" <[e-mail]> wrote:
> >   
> > > > -----Original Message-----
> > > > From: dri-devel [mailto:[e-mail]] On
> > > > Behalf Of Alex Williamson
> > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > To: Zhang, Tina <[e-mail]>
> > > > Cc: Tian, Kevin <[e-mail]>; [e-mail]; intel-
> > > > [e-mail]; Wang, Zhi A <[e-mail]>;
> > > > [e-mail]; [e-mail]; intel-gvt-
> > > > [e-mail]; Lv, Zhiyuan <[e-mail]>
> > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > On Tue, 25 Jul 2017 17:28:18 +0800
> > > > Tina Zhang <[e-mail]> 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]>
> > > > > ---
> > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > > index ae46105..827a230 100644
> > > > > --- a/include/uapi/linux/vfio.h
> > > > > +++ b/include/uapi/linux/vfio.h
> > > > > @@ -502,6 +502,34 @@ 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 */
> > > > > +	__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, upper left corner  
> > > > in pixels */  
> > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in  
> > > > lines*/  
> > > > > +	__u32 region_index;
> > > > > +	__s32 fd;	/* dma-buf fd */  
> > > >
> > > > How do I know which of these is valid, region_index or fd?  Can I
> > > > ask for one vs the other?  What are the errno values to
> > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > flag that I can use to determine what the device supports without allocating  
> > those resources yet?  
> > > Dma-buf won't use region_index, which means region_index is zero all the  
> > time for dma-buf usage.  
> > > As we discussed, there won't be errno if not initialized, just keep all fields zero.
> > > I will add the comments about these in the next version. Thanks  
> > 
> > Zero is a valid region index.  
> If zero is valid, can we find out other invalid value? How about 0xffffffff?

Unlikely, but also valid.  Isn't this why we have a flags field in the
structure, so we don't need to rely on implicit values as invalid.
Also, all of the previously discussed usage models needs to be
documented, either here in the header or in a Documentation/ file.
Thanks,

Alex

Zhang, Tina Aug. 7, 2017, 3:22 a.m. UTC | #9

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

> From: dri-devel [mailto:[e-mail]] On Behalf Of

> Alex Williamson

> Sent: Thursday, August 3, 2017 10:43 PM

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

> Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-

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

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

> Zhiyuan <[e-mail]>

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

> 

> On Thu, 3 Aug 2017 07:08:15 +0000

> "Zhang, Tina" <[e-mail]> wrote:

> 

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

> > > From: Alex Williamson [mailto:[e-mail]]

> > > Sent: Thursday, August 3, 2017 11:38 AM

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

> > > Cc: Tian, Kevin <[e-mail]>;

> > > [e-mail]; dri- [e-mail];

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

> > > [e-mail]; Wang, Zhi A

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

> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf

> > > operation

> > >

> > > On Thu, 3 Aug 2017 03:17:09 +0000

> > > "Zhang, Tina" <[e-mail]> wrote:

> > >

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

> > > > > From: dri-devel [mailto:[e-mail]]

> > > > > On Behalf Of Alex Williamson

> > > > > Sent: Wednesday, August 2, 2017 5:18 AM

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

> > > > > Cc: Tian, Kevin <[e-mail]>; [e-mail];

> > > > > intel- [e-mail]; Wang, Zhi A

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

> > > > > [e-mail]; intel-gvt-

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

> > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf

> > > > > operation

> > > > >

> > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang

> > > > > <[e-mail]> 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]>

> > > > > > ---

> > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++

> > > > > >  1 file changed, 28 insertions(+)

> > > > > >

> > > > > > diff --git a/include/uapi/linux/vfio.h

> > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644

> > > > > > --- a/include/uapi/linux/vfio.h

> > > > > > +++ b/include/uapi/linux/vfio.h

> > > > > > @@ -502,6 +502,34 @@ 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 */

> > > > > > +	__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, upper

> left corner

> > > > > in pixels */

> > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left

> corner in

> > > > > lines*/

> > > > > > +	__u32 region_index;

> > > > > > +	__s32 fd;	/* dma-buf fd */

> > > > >

> > > > > How do I know which of these is valid, region_index or fd?  Can

> > > > > I ask for one vs the other?  What are the errno values to

> > > > > differentiate unsupported vs not initialized?  Is there a "probe"

> > > > > flag that I can use to determine what the device supports

> > > > > without allocating

> > > those resources yet?

> > > > Dma-buf won't use region_index, which means region_index is zero

> > > > all the

> > > time for dma-buf usage.

> > > > As we discussed, there won't be errno if not initialized, just keep all fields

> zero.

> > > > I will add the comments about these in the next version. Thanks

> > >

> > > Zero is a valid region index.

> > If zero is valid, can we find out other invalid value? How about 0xffffffff?

> 

> Unlikely, but also valid.  Isn't this why we have a flags field in the structure, so

> we don't need to rely on implicit values as invalid.

> Also, all of the previously discussed usage models needs to be documented,

> either here in the header or in a Documentation/ file.

According to the previously discussion, we also have the following propose:
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;
};
So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
User mode can query this before querying the plan info, and to see which usage
(dma-buf or region) is supported.
Does it still make sense?
Thanks.

Tina


> Thanks,

> 

> Alex

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Zhang, Tina Aug. 7, 2017, 8:11 a.m. UTC | #10

After going through the previous discussions, here are some summaries may be related to the current discussion:
1. How does user mode figure the device capabilities between region and dma-buf?
VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
Otherwise, the mdev supports dma-buf.

2. For dma-buf, how to differentiate unsupported vs not initialized?
For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.

3. The id field in structure vfio_device_gfx_plane_info
So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
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;
 };
Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.

Thanks.

BR,
Tina

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

> From: Zhang, Tina

> Sent: Monday, August 7, 2017 11:23 AM

> To: 'Alex Williamson' <[e-mail]>

> Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-

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

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

> Zhiyuan <[e-mail]>

> Subject: RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

> 

> 

> 

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

> > From: dri-devel [mailto:[e-mail]] On

> > Behalf Of Alex Williamson

> > Sent: Thursday, August 3, 2017 10:43 PM

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

> > Cc: Tian, Kevin <[e-mail]>;

> > [e-mail]; dri- [e-mail];

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

> > [e-mail]; Wang, Zhi A

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

> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf

> > operation

> >

> > On Thu, 3 Aug 2017 07:08:15 +0000

> > "Zhang, Tina" <[e-mail]> wrote:

> >

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

> > > > From: Alex Williamson [mailto:[e-mail]]

> > > > Sent: Thursday, August 3, 2017 11:38 AM

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

> > > > Cc: Tian, Kevin <[e-mail]>;

> > > > [e-mail]; dri- [e-mail];

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

> > > > [e-mail]; Wang, Zhi A

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

> > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf

> > > > operation

> > > >

> > > > On Thu, 3 Aug 2017 03:17:09 +0000

> > > > "Zhang, Tina" <[e-mail]> wrote:

> > > >

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

> > > > > > From: dri-devel

> > > > > > [mailto:[e-mail]]

> > > > > > On Behalf Of Alex Williamson

> > > > > > Sent: Wednesday, August 2, 2017 5:18 AM

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

> > > > > > Cc: Tian, Kevin <[e-mail]>; [e-mail];

> > > > > > intel- [e-mail]; Wang, Zhi A

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

> > > > > > [e-mail]; intel-gvt-

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

> > > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display

> > > > > > dma-buf operation

> > > > > >

> > > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang

> > > > > > <[e-mail]> 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]>

> > > > > > > ---

> > > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++

> > > > > > >  1 file changed, 28 insertions(+)

> > > > > > >

> > > > > > > diff --git a/include/uapi/linux/vfio.h

> > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644

> > > > > > > --- a/include/uapi/linux/vfio.h

> > > > > > > +++ b/include/uapi/linux/vfio.h

> > > > > > > @@ -502,6 +502,34 @@ 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 */

> > > > > > > +	__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, upper

> > left corner

> > > > > > in pixels */

> > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left

> > corner in

> > > > > > lines*/

> > > > > > > +	__u32 region_index;

> > > > > > > +	__s32 fd;	/* dma-buf fd */

> > > > > >

> > > > > > How do I know which of these is valid, region_index or fd?

> > > > > > Can I ask for one vs the other?  What are the errno values to

> > > > > > differentiate unsupported vs not initialized?  Is there a "probe"

> > > > > > flag that I can use to determine what the device supports

> > > > > > without allocating

> > > > those resources yet?

> > > > > Dma-buf won't use region_index, which means region_index is zero

> > > > > all the

> > > > time for dma-buf usage.

> > > > > As we discussed, there won't be errno if not initialized, just

> > > > > keep all fields

> > zero.

> > > > > I will add the comments about these in the next version. Thanks

> > > >

> > > > Zero is a valid region index.

> > > If zero is valid, can we find out other invalid value? How about 0xffffffff?

> >

> > Unlikely, but also valid.  Isn't this why we have a flags field in the

> > structure, so we don't need to rely on implicit values as invalid.

> > Also, all of the previously discussed usage models needs to be

> > documented, either here in the header or in a Documentation/ file.

> According to the previously discussion, we also have the following propose:

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

> };

> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.

> User mode can query this before querying the plan info, and to see which usage

> (dma-buf or region) is supported.

> Does it still make sense?

> Thanks.

So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
Thanks

Tina
> 

> Tina

> 

> 

> > Thanks,

> >

> > Alex

> > _______________________________________________

> > dri-devel mailing list

> > [e-mail]

> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alex Williamson Aug. 7, 2017, 5:43 p.m. UTC | #11

On Mon, 7 Aug 2017 08:11:43 +0000
"Zhang, Tina" <[e-mail]> wrote:

> After going through the previous discussions, here are some summaries may be related to the current discussion:
> 1. How does user mode figure the device capabilities between region and dma-buf?
> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
> Otherwise, the mdev supports dma-buf.

Why do we need to make this assumption?  What happens when dma-buf is
superseded?  What happens if a device supports both dma-buf and
regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
it make sense to use it to identify which field, between region_index
and fd, is valid?  We could even put region_index and fd into a union
with the flag bits indicating how to interpret the union, but I'm not
sure everyone was onboard with this idea.  Seems like a waste of 4
bytes not to do that though.

Thinking further, is the user ever in a situation where they query the
graphics plane info and can handle either a dma-buf or a region?  It
seems more likely that the user needs to know early on which is
supported and would then require that they continue to see compatible
plane information...  Should the user then be able to specify whether
they want a dma-buf or a region?  Perhaps these flag bits are actually
input and the return should be -errno if the driver cannot produce
something compatible.

Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
this initial implementation, DMABUF or REGION would always be set by
the user to request that type of interface.  Additionally, the QUERY
bit could be set to probe compatibility, thus if PROBE and REGION are
set, the vendor driver would return success only if it supports the
region based interface.  If PROBE and DMABUF are set, the vendor driver
returns success only if the dma-buf based interface is supported.  The
value of the remainder of the structure is undefined for PROBE.
Additionally setting both DMABUF and REGION is invalid.  Undefined
flags bits must be validated as zero by the drivers for future use
(thus if we later define DMABUFv2, an older driver should
automatically return -errno when probed or requested).

It seems like this handles all the cases, the user can ask what's
supported and specifies the interface they want on every call.  The
user therefore can also choose between region_index and fd and we can
make that a union.

> 2. For dma-buf, how to differentiate unsupported vs not initialized?
> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.

So we're relying on special values again :-\  For which fields is zero
not a valid value?  I prefer the probe interface above unless there are
better ideas.
 
> 3. The id field in structure vfio_device_gfx_plane_info
> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
> 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;
>  };
> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.

The usefulness of this ioctl seems really limited and once again we get
into a situation where having two ioctls leaves doubt whether this is
describing the current plane state.  Thanks,

Alex

> > > > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > @@ -502,6 +502,34 @@ 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 */
> > > > > > > > +	__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, upper  
> > > left corner  
> > > > > > > in pixels */  
> > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
> > > corner in  
> > > > > > > lines*/  
> > > > > > > > +	__u32 region_index;
> > > > > > > > +	__s32 fd;	/* dma-buf fd */  
> > > > > > >
> > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > Can I ask for one vs the other?  What are the errno values to
> > > > > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > > flag that I can use to determine what the device supports
> > > > > > > without allocating  
> > > > > those resources yet?  
> > > > > > Dma-buf won't use region_index, which means region_index is zero
> > > > > > all the  
> > > > > time for dma-buf usage.  
> > > > > > As we discussed, there won't be errno if not initialized, just
> > > > > > keep all fields  
> > > zero.  
> > > > > > I will add the comments about these in the next version. Thanks  
> > > > >
> > > > > Zero is a valid region index.  
> > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?  
> > >
> > > Unlikely, but also valid.  Isn't this why we have a flags field in the
> > > structure, so we don't need to rely on implicit values as invalid.
> > > Also, all of the previously discussed usage models needs to be
> > > documented, either here in the header or in a Documentation/ file.  
> > According to the previously discussion, we also have the following propose:
> > 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;
> > };
> > So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
> > User mode can query this before querying the plan info, and to see which usage
> > (dma-buf or region) is supported.
> > Does it still make sense?
> > Thanks.  
> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
> Thanks
> 
> Tina
> > 
> > Tina
> > 
> >   
> > > Thanks,
> > >
> > > Alex
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Kirti Wankhede Aug. 8, 2017, 8:48 a.m. UTC | #12

On 8/7/2017 11:13 PM, Alex Williamson wrote:
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <[e-mail]> wrote:
> 
>> After going through the previous discussions, here are some summaries may be related to the current discussion:
>> 1. How does user mode figure the device capabilities between region and dma-buf?
>> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
>> Otherwise, the mdev supports dma-buf.
> 
> Why do we need to make this assumption?

Right, we should not make such assumption. Vendor driver might not
support both or disable console vnc ( for example, for performance
testing console VNC need to be disabled)

>  What happens when dma-buf is
> superseded?  What happens if a device supports both dma-buf and
> regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
> it make sense to use it to identify which field, between region_index
> and fd, is valid?  We could even put region_index and fd into a union
> with the flag bits indicating how to interpret the union, but I'm not
> sure everyone was onboard with this idea.  Seems like a waste of 4
> bytes not to do that though.
> 

Agree.

> Thinking further, is the user ever in a situation where they query the
> graphics plane info and can handle either a dma-buf or a region?  It
> seems more likely that the user needs to know early on which is
> supported and would then require that they continue to see compatible
> plane information...  Should the user then be able to specify whether
> they want a dma-buf or a region?  Perhaps these flag bits are actually
> input and the return should be -errno if the driver cannot produce
> something compatible.
> 
> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
> this initial implementation, DMABUF or REGION would always be set by
> the user to request that type of interface.  Additionally, the QUERY
> bit could be set to probe compatibility, thus if PROBE and REGION are
> set, the vendor driver would return success only if it supports the
> region based interface.  If PROBE and DMABUF are set, the vendor driver
> returns success only if the dma-buf based interface is supported.  The
> value of the remainder of the structure is undefined for PROBE.
> Additionally setting both DMABUF and REGION is invalid.  Undefined
> flags bits must be validated as zero by the drivers for future use
> (thus if we later define DMABUFv2, an older driver should
> automatically return -errno when probed or requested).
> 

Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?

Let me summarize what we need, taking QEMU as reference:
1. From vfio_initfn(), for REGION case, get region info:
vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
&vdev->console_opregion)

If above return success, setup console REGION and mmap.
I don't know what is required for DMABUF at this moment.

If console VNC is supported either DMABUF or REGION, initialize console
and register its callback operations:

static const GraphicHwOps vfio_console_ops = {
    .gfx_update  = vfio_console_update_display,
};

vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
vdev);

2. On above console registration, vfio_console_update_display() gets
called for each refresh cycle of console. In that:
- call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
- if (queried size > 0), update QEMU console surface (for REGION case
read mmaped region, for DMABUF read surface using fd)


Alex, in your proposal above, my understanding is
ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
step #1.
In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
flag, but either DMABUF or REGION flag based on what is returned as
supported by vendor driver in step #1. Is that correct?


> It seems like this handles all the cases, the user can ask what's
> supported and specifies the interface they want on every call.  The
> user therefore can also choose between region_index and fd and we can
> make that a union.
>
>> 2. For dma-buf, how to differentiate unsupported vs not initialized?
>> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
>> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.
> 
> So we're relying on special values again :-\  For which fields is zero
> not a valid value?  I prefer the probe interface above unless there are
> better ideas.
> 

PROBE will be called during vdev initialization and after that
vfio_console_update_display() gets called at every console refresh
cycle. But until driver in guest is loaded, mmaped buffer/ DMABUF will
not be populated with correct surface data. In that case for QUERY,
vendor driver should return (atleast) size=0 which means there is
nothing to copy. Once guest driver is loaded and surface is created by
guest driver, QUERY interface should return valid size.

Thanks,
Kirti

>> 3. The id field in structure vfio_device_gfx_plane_info
>> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
>> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
>> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
>> 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;
>>  };
>> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.
> 
> The usefulness of this ioctl seems really limited and once again we get
> into a situation where having two ioctls leaves doubt whether this is
> describing the current plane state.  Thanks,
> 
> Alex
> 
>>>>>>>>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>>>>>>>>>  1 file changed, 28 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/uapi/linux/vfio.h
>>>>>>>>> b/include/uapi/linux/vfio.h index ae46105..827a230 100644
>>>>>>>>> --- a/include/uapi/linux/vfio.h
>>>>>>>>> +++ b/include/uapi/linux/vfio.h
>>>>>>>>> @@ -502,6 +502,34 @@ 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 */
>>>>>>>>> +	__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, upper  
>>>> left corner  
>>>>>>>> in pixels */  
>>>>>>>>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
>>>> corner in  
>>>>>>>> lines*/  
>>>>>>>>> +	__u32 region_index;
>>>>>>>>> +	__s32 fd;	/* dma-buf fd */  
>>>>>>>>
>>>>>>>> How do I know which of these is valid, region_index or fd?
>>>>>>>> Can I ask for one vs the other?  What are the errno values to
>>>>>>>> differentiate unsupported vs not initialized?  Is there a "probe"
>>>>>>>> flag that I can use to determine what the device supports
>>>>>>>> without allocating  
>>>>>> those resources yet?  
>>>>>>> Dma-buf won't use region_index, which means region_index is zero
>>>>>>> all the  
>>>>>> time for dma-buf usage.  
>>>>>>> As we discussed, there won't be errno if not initialized, just
>>>>>>> keep all fields  
>>>> zero.  
>>>>>>> I will add the comments about these in the next version. Thanks  
>>>>>>
>>>>>> Zero is a valid region index.  
>>>>> If zero is valid, can we find out other invalid value? How about 0xffffffff?  
>>>>
>>>> Unlikely, but also valid.  Isn't this why we have a flags field in the
>>>> structure, so we don't need to rely on implicit values as invalid.
>>>> Also, all of the previously discussed usage models needs to be
>>>> documented, either here in the header or in a Documentation/ file.  
>>> According to the previously discussion, we also have the following propose:
>>> 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;
>>> };
>>> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
>>> User mode can query this before querying the plan info, and to see which usage
>>> (dma-buf or region) is supported.
>>> Does it still make sense?
>>> Thanks.  
>> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
>> Thanks
>>
>> Tina
>>>
>>> Tina
>>>
>>>   
>>>> Thanks,
>>>>
>>>> Alex
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> [e-mail]
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel  
>

Alex Williamson Aug. 8, 2017, 6:07 p.m. UTC | #13

On Tue, 8 Aug 2017 14:18:07 +0530
Kirti Wankhede <[e-mail]> wrote:

> On 8/7/2017 11:13 PM, Alex Williamson wrote:
> > On Mon, 7 Aug 2017 08:11:43 +0000
> > "Zhang, Tina" <[e-mail]> wrote:
> >   
> >> After going through the previous discussions, here are some summaries may be related to the current discussion:
> >> 1. How does user mode figure the device capabilities between region and dma-buf?
> >> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
> >> Otherwise, the mdev supports dma-buf.  
> > 
> > Why do we need to make this assumption?  
> 
> Right, we should not make such assumption. Vendor driver might not
> support both or disable console vnc ( for example, for performance
> testing console VNC need to be disabled)
> 
> >  What happens when dma-buf is
> > superseded?  What happens if a device supports both dma-buf and
> > regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
> > it make sense to use it to identify which field, between region_index
> > and fd, is valid?  We could even put region_index and fd into a union
> > with the flag bits indicating how to interpret the union, but I'm not
> > sure everyone was onboard with this idea.  Seems like a waste of 4
> > bytes not to do that though.
> >   
> 
> Agree.
> 
> > Thinking further, is the user ever in a situation where they query the
> > graphics plane info and can handle either a dma-buf or a region?  It
> > seems more likely that the user needs to know early on which is
> > supported and would then require that they continue to see compatible
> > plane information...  Should the user then be able to specify whether
> > they want a dma-buf or a region?  Perhaps these flag bits are actually
> > input and the return should be -errno if the driver cannot produce
> > something compatible.
> > 
> > Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
> > this initial implementation, DMABUF or REGION would always be set by
> > the user to request that type of interface.  Additionally, the QUERY
> > bit could be set to probe compatibility, thus if PROBE and REGION are
> > set, the vendor driver would return success only if it supports the
> > region based interface.  If PROBE and DMABUF are set, the vendor driver
> > returns success only if the dma-buf based interface is supported.  The
> > value of the remainder of the structure is undefined for PROBE.
> > Additionally setting both DMABUF and REGION is invalid.  Undefined
> > flags bits must be validated as zero by the drivers for future use
> > (thus if we later define DMABUFv2, an older driver should
> > automatically return -errno when probed or requested).
> >   
> 
> Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?
> 
> Let me summarize what we need, taking QEMU as reference:
> 1. From vfio_initfn(), for REGION case, get region info:
> vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
> &vdev->console_opregion)
> 
> If above return success, setup console REGION and mmap.
> I don't know what is required for DMABUF at this moment.
> 
> If console VNC is supported either DMABUF or REGION, initialize console
> and register its callback operations:
> 
> static const GraphicHwOps vfio_console_ops = {
>     .gfx_update  = vfio_console_update_display,
> };
> 
> vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
> vdev);

I might structure it that vfio_initfn() would call
ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with the PROBE bit set with either
DMABUF or REGION set as the interface type in the flags field.  If
REGION is the probed interface and success is returned, then QEMU might
go look for regions of the appropriate type, however the
vfio_device_gfx_plane_info structure is canonical source for the region
index, so QEMU would probably be wise to use that and only use the
region type for consistency testing.

> 2. On above console registration, vfio_console_update_display() gets
> called for each refresh cycle of console. In that:
> - call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
> - if (queried size > 0), update QEMU console surface (for REGION case
> read mmaped region, for DMABUF read surface using fd)

The ioctl would be called with REGION or DMABUF based on the initial
probe call, ex. we probed that REGION is supported and now we continue
to ask for region based updates.  QEMU would need to verify the region
index matches that already mapped, remapping a different region if
necessary, and interpret the graphics parameters to provide the screen
update.
 
> Alex, in your proposal above, my understanding is
> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
> step #1.
> In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
> flag, but either DMABUF or REGION flag based on what is returned as
> supported by vendor driver in step #1. Is that correct?

Yes, that's the idea.  Does it make sense/provide value?

> > It seems like this handles all the cases, the user can ask what's
> > supported and specifies the interface they want on every call.  The
> > user therefore can also choose between region_index and fd and we can
> > make that a union.
> >  
> >> 2. For dma-buf, how to differentiate unsupported vs not initialized?
> >> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
> >> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.  
> > 
> > So we're relying on special values again :-\  For which fields is zero
> > not a valid value?  I prefer the probe interface above unless there are
> > better ideas.
> >   
> 
> PROBE will be called during vdev initialization and after that
> vfio_console_update_display() gets called at every console refresh
> cycle. But until driver in guest is loaded, mmaped buffer/ DMABUF will
> not be populated with correct surface data. In that case for QUERY,
> vendor driver should return (atleast) size=0 which means there is
> nothing to copy. Once guest driver is loaded and surface is created by
> guest driver, QUERY interface should return valid size.

That seems reasonable and well specified.  Thanks,

Alex

> >> 3. The id field in structure vfio_device_gfx_plane_info
> >> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
> >> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
> >> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
> >> 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;
> >>  };
> >> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.  
> > 
> > The usefulness of this ioctl seems really limited and once again we get
> > into a situation where having two ioctls leaves doubt whether this is
> > describing the current plane state.  Thanks,
> > 
> > Alex
> >   
> >>>>>>>>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> >>>>>>>>>  1 file changed, 28 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/include/uapi/linux/vfio.h
> >>>>>>>>> b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> >>>>>>>>> --- a/include/uapi/linux/vfio.h
> >>>>>>>>> +++ b/include/uapi/linux/vfio.h
> >>>>>>>>> @@ -502,6 +502,34 @@ 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 */
> >>>>>>>>> +	__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, upper    
> >>>> left corner    
> >>>>>>>> in pixels */    
> >>>>>>>>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left    
> >>>> corner in    
> >>>>>>>> lines*/    
> >>>>>>>>> +	__u32 region_index;
> >>>>>>>>> +	__s32 fd;	/* dma-buf fd */    
> >>>>>>>>
> >>>>>>>> How do I know which of these is valid, region_index or fd?
> >>>>>>>> Can I ask for one vs the other?  What are the errno values to
> >>>>>>>> differentiate unsupported vs not initialized?  Is there a "probe"
> >>>>>>>> flag that I can use to determine what the device supports
> >>>>>>>> without allocating    
> >>>>>> those resources yet?    
> >>>>>>> Dma-buf won't use region_index, which means region_index is zero
> >>>>>>> all the    
> >>>>>> time for dma-buf usage.    
> >>>>>>> As we discussed, there won't be errno if not initialized, just
> >>>>>>> keep all fields    
> >>>> zero.    
> >>>>>>> I will add the comments about these in the next version. Thanks    
> >>>>>>
> >>>>>> Zero is a valid region index.    
> >>>>> If zero is valid, can we find out other invalid value? How about 0xffffffff?    
> >>>>
> >>>> Unlikely, but also valid.  Isn't this why we have a flags field in the
> >>>> structure, so we don't need to rely on implicit values as invalid.
> >>>> Also, all of the previously discussed usage models needs to be
> >>>> documented, either here in the header or in a Documentation/ file.    
> >>> According to the previously discussion, we also have the following propose:
> >>> 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;
> >>> };
> >>> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
> >>> User mode can query this before querying the plan info, and to see which usage
> >>> (dma-buf or region) is supported.
> >>> Does it still make sense?
> >>> Thanks.    
> >> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
> >> Thanks
> >>
> >> Tina  
> >>>
> >>> Tina
> >>>
> >>>     
> >>>> Thanks,
> >>>>
> >>>> Alex
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> [e-mail]
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel    
> >

Zhang, Tina Aug. 9, 2017, 8:31 a.m. UTC | #14

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

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

> Behalf Of Alex Williamson

> Sent: Tuesday, August 8, 2017 1:43 AM

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

> Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-

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

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

> Zhiyuan <[e-mail]>

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

> 

> On Mon, 7 Aug 2017 08:11:43 +0000

> "Zhang, Tina" <[e-mail]> wrote:

> 

> > After going through the previous discussions, here are some summaries may

> be related to the current discussion:

> > 1. How does user mode figure the device capabilities between region and

> dma-buf?

> > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.

> > Otherwise, the mdev supports dma-buf.

> 

> Why do we need to make this assumption?  What happens when dma-buf is

> superseded?  What happens if a device supports both dma-buf and regions?

> We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to use

> it to identify which field, between region_index and fd, is valid?  We could even

> put region_index and fd into a union with the flag bits indicating how to

> interpret the union, but I'm not sure everyone was onboard with this idea.

> Seems like a waste of 4 bytes not to do that though.

It seems we discussed this idea before:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html
Thanks.

Tina
> 

> Thinking further, is the user ever in a situation where they query the graphics

> plane info and can handle either a dma-buf or a region?  It seems more likely

> that the user needs to know early on which is supported and would then require

> that they continue to see compatible plane information...  Should the user then

> be able to specify whether they want a dma-buf or a region?  Perhaps these flag

> bits are actually input and the return should be -errno if the driver cannot

> produce something compatible.

From the previously discussion, it seems user space workflow will look quite different
for these two cases. So once user space finds out which case is supported, it just uses
that case, and won't change it.

Meanwhile, I'm not sure whether there will be a mdev would like to support both region
and dma-buf cases. In my opinion, either region or dma-buf is supported by one mdev. (Yeah,
agree, there may be other cases in future)
It's like we want to propose a general interface used to share guest's buffer with host. And the
general interface, so far, has two choice: region and dma-buf. So each mdev likes this interface
can implement one kind of it and gets the benefit from the general interface.
So, if we think about this, the difference in user mode should be as little as possible.
Thanks.

Tina
> 

> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In this

> initial implementation, DMABUF or REGION would always be set by the user to

> request that type of interface.  Additionally, the QUERY bit could be set to probe

> compatibility, thus if PROBE and REGION are set, the vendor driver would return

> success only if it supports the region based interface.  If PROBE and DMABUF are

> set, the vendor driver returns success only if the dma-buf based interface is

> supported.  The value of the remainder of the structure is undefined for PROBE.

> Additionally setting both DMABUF and REGION is invalid.  Undefined flags bits

> must be validated as zero by the drivers for future use (thus if we later define

> DMABUFv2, an older driver should automatically return -errno when probed or

> requested).

> 

> It seems like this handles all the cases, the user can ask what's supported and

> specifies the interface they want on every call.  The user therefore can also

> choose between region_index and fd and we can make that a union.

Agree, that's a good proposal, which can handle all the cases.
I'm just not sure about the usage case of "on every call". In previous discussion, it seems we think static is enough.
Thanks.

Tina

> 

> > 2. For dma-buf, how to differentiate unsupported vs not initialized?

> > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will

> be returned. And -errno will return when meeting other failures, like -ENOMEM.

> > If the mdev is not initialized, there won't be any returned err. Just zero all the

> fields in structure vfio_device_gfx_plane_info.

> 

> So we're relying on special values again :-\  For which fields is zero not a valid

> value?  I prefer the probe interface above unless there are better ideas.

> 

> > 3. The id field in structure vfio_device_gfx_plane_info So far we

> > haven't figured out the usage of this field for dma-buf usage. So, this field is

> changed to "region_index" and only used for region usage.

> > In previous discussions, we thought this "id" field might be used for both dma-

> buf and region usages.

> > So, we proposed some ways, like adding flags field to the structure. Another

> way to do it was to add this:

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

> >  };

> > Obviously, we don't need to consider this again, unless we find the

> "region_index" means something for dma-buf usage.

> 

> The usefulness of this ioctl seems really limited and once again we get into a

> situation where having two ioctls leaves doubt whether this is describing the

> current plane state.  Thanks,

> 

> Alex

> 

> > > > > > > > >  include/uapi/linux/vfio.h | 28

> > > > > > > > > ++++++++++++++++++++++++++++

> > > > > > > > >  1 file changed, 28 insertions(+)

> > > > > > > > >

> > > > > > > > > diff --git a/include/uapi/linux/vfio.h

> > > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230

> > > > > > > > > 100644

> > > > > > > > > --- a/include/uapi/linux/vfio.h

> > > > > > > > > +++ b/include/uapi/linux/vfio.h

> > > > > > > > > @@ -502,6 +502,34 @@ 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 */

> > > > > > > > > +	__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, upper

> > > > left corner

> > > > > > > > in pixels */

> > > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left

> > > > corner in

> > > > > > > > lines*/

> > > > > > > > > +	__u32 region_index;

> > > > > > > > > +	__s32 fd;	/* dma-buf fd */

> > > > > > > >

> > > > > > > > How do I know which of these is valid, region_index or fd?

> > > > > > > > Can I ask for one vs the other?  What are the errno values

> > > > > > > > to differentiate unsupported vs not initialized?  Is there a "probe"

> > > > > > > > flag that I can use to determine what the device supports

> > > > > > > > without allocating

> > > > > > those resources yet?

> > > > > > > Dma-buf won't use region_index, which means region_index is

> > > > > > > zero all the

> > > > > > time for dma-buf usage.

> > > > > > > As we discussed, there won't be errno if not initialized,

> > > > > > > just keep all fields

> > > > zero.

> > > > > > > I will add the comments about these in the next version.

> > > > > > > Thanks

> > > > > >

> > > > > > Zero is a valid region index.

> > > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?

> > > >

> > > > Unlikely, but also valid.  Isn't this why we have a flags field in

> > > > the structure, so we don't need to rely on implicit values as invalid.

> > > > Also, all of the previously discussed usage models needs to be

> > > > documented, either here in the header or in a Documentation/ file.

> > > According to the previously discussion, we also have the following propose:

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

> > > };

> > > So, we may need to add one more ioctl, e.g.

> VFIO_DEVICE_QUERY_GFX_CAPS.

> > > User mode can query this before querying the plan info, and to see

> > > which usage (dma-buf or region) is supported.

> > > Does it still make sense?

> > > Thanks.

> > So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode

> whether the region case is using, instead of introducing a new ioctl.

> > Thanks

> >

> > Tina

> > >

> > > Tina

> > >

> > >

> > > > Thanks,

> > > >

> > > > Alex

> > > > _______________________________________________

> > > > dri-devel mailing list

> > > > [e-mail]

> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel

> 

> _______________________________________________

> intel-gvt-dev mailing list

> [e-mail]

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

Kirti Wankhede Aug. 9, 2017, 1:57 p.m. UTC | #15

On 8/8/2017 11:37 PM, Alex Williamson wrote:
> On Tue, 8 Aug 2017 14:18:07 +0530
> Kirti Wankhede <[e-mail]> wrote:
> 
>> On 8/7/2017 11:13 PM, Alex Williamson wrote:
>>> On Mon, 7 Aug 2017 08:11:43 +0000
>>> "Zhang, Tina" <[e-mail]> wrote:
>>>   
>>>> After going through the previous discussions, here are some summaries may be related to the current discussion:
>>>> 1. How does user mode figure the device capabilities between region and dma-buf?
>>>> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
>>>> Otherwise, the mdev supports dma-buf.  
>>>
>>> Why do we need to make this assumption?  
>>
>> Right, we should not make such assumption. Vendor driver might not
>> support both or disable console vnc ( for example, for performance
>> testing console VNC need to be disabled)
>>
>>>  What happens when dma-buf is
>>> superseded?  What happens if a device supports both dma-buf and
>>> regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
>>> it make sense to use it to identify which field, between region_index
>>> and fd, is valid?  We could even put region_index and fd into a union
>>> with the flag bits indicating how to interpret the union, but I'm not
>>> sure everyone was onboard with this idea.  Seems like a waste of 4
>>> bytes not to do that though.
>>>   
>>
>> Agree.
>>
>>> Thinking further, is the user ever in a situation where they query the
>>> graphics plane info and can handle either a dma-buf or a region?  It
>>> seems more likely that the user needs to know early on which is
>>> supported and would then require that they continue to see compatible
>>> plane information...  Should the user then be able to specify whether
>>> they want a dma-buf or a region?  Perhaps these flag bits are actually
>>> input and the return should be -errno if the driver cannot produce
>>> something compatible.
>>>
>>> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
>>> this initial implementation, DMABUF or REGION would always be set by
>>> the user to request that type of interface.  Additionally, the QUERY
>>> bit could be set to probe compatibility, thus if PROBE and REGION are
>>> set, the vendor driver would return success only if it supports the
>>> region based interface.  If PROBE and DMABUF are set, the vendor driver
>>> returns success only if the dma-buf based interface is supported.  The
>>> value of the remainder of the structure is undefined for PROBE.
>>> Additionally setting both DMABUF and REGION is invalid.  Undefined
>>> flags bits must be validated as zero by the drivers for future use
>>> (thus if we later define DMABUFv2, an older driver should
>>> automatically return -errno when probed or requested).
>>>   
>>
>> Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?
>>
>> Let me summarize what we need, taking QEMU as reference:
>> 1. From vfio_initfn(), for REGION case, get region info:
>> vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
>> &vdev->console_opregion)
>>
>> If above return success, setup console REGION and mmap.
>> I don't know what is required for DMABUF at this moment.
>>
>> If console VNC is supported either DMABUF or REGION, initialize console
>> and register its callback operations:
>>
>> static const GraphicHwOps vfio_console_ops = {
>>     .gfx_update  = vfio_console_update_display,
>> };
>>
>> vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
>> vdev);
> 
> I might structure it that vfio_initfn() would call
> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with the PROBE bit set with either
> DMABUF or REGION set as the interface type in the flags field.  If
> REGION is the probed interface and success is returned, then QEMU might
> go look for regions of the appropriate type, however the
> vfio_device_gfx_plane_info structure is canonical source for the region
> index, so QEMU would probably be wise to use that and only use the
> region type for consistency testing.
> 
>> 2. On above console registration, vfio_console_update_display() gets
>> called for each refresh cycle of console. In that:
>> - call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
>> - if (queried size > 0), update QEMU console surface (for REGION case
>> read mmaped region, for DMABUF read surface using fd)
> 
> The ioctl would be called with REGION or DMABUF based on the initial
> probe call, ex. we probed that REGION is supported and now we continue
> to ask for region based updates.  QEMU would need to verify the region
> index matches that already mapped, remapping a different region if
> necessary, and interpret the graphics parameters to provide the screen
> update.
>  
>> Alex, in your proposal above, my understanding is
>> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
>> step #1.
>> In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
>> flag, but either DMABUF or REGION flag based on what is returned as
>> supported by vendor driver in step #1. Is that correct?
> 
> Yes, that's the idea.  Does it make sense/provide value?
> 

Yes, sounds good to me.

Thanks,
Kirti

Alex Williamson Aug. 9, 2017, 4:57 p.m. UTC | #16

On Wed, 9 Aug 2017 08:31:00 +0000
"Zhang, Tina" <[e-mail]> wrote:

> > -----Original Message-----
> > From: intel-gvt-dev [mailto:[e-mail]] On
> > Behalf Of Alex Williamson
> > Sent: Tuesday, August 8, 2017 1:43 AM
> > To: Zhang, Tina <[e-mail]>
> > Cc: Tian, Kevin <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Wang, Zhi A <[e-mail]>; Lv,
> > Zhiyuan <[e-mail]>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Mon, 7 Aug 2017 08:11:43 +0000
> > "Zhang, Tina" <[e-mail]> wrote:
> >   
> > > After going through the previous discussions, here are some summaries may  
> > be related to the current discussion:  
> > > 1. How does user mode figure the device capabilities between region and  
> > dma-buf?  
> > > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.
> > > Otherwise, the mdev supports dma-buf.  
> > 
> > Why do we need to make this assumption?  What happens when dma-buf is
> > superseded?  What happens if a device supports both dma-buf and regions?
> > We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to use
> > it to identify which field, between region_index and fd, is valid?  We could even
> > put region_index and fd into a union with the flag bits indicating how to
> > interpret the union, but I'm not sure everyone was onboard with this idea.
> > Seems like a waste of 4 bytes not to do that though.  
> It seems we discussed this idea before:
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html

These are both from Gerd.  Gerd, do you have any objection to using a
union to provide either the dmabuf fd or region index?  It's
inefficient to provide separate fields when we can only provide one or
the other and I don't like the idea of using implicit values to
determine which is active.

> > Thinking further, is the user ever in a situation where they query the graphics
> > plane info and can handle either a dma-buf or a region?  It seems more likely
> > that the user needs to know early on which is supported and would then require
> > that they continue to see compatible plane information...  Should the user then
> > be able to specify whether they want a dma-buf or a region?  Perhaps these flag
> > bits are actually input and the return should be -errno if the driver cannot
> > produce something compatible.  
> From the previously discussion, it seems user space workflow will look quite different
> for these two cases. So once user space finds out which case is supported, it just uses
> that case, and won't change it.

And that's supported, the user tests whether a given interface type is
supported and continues to request updates for that interface type.
 
> Meanwhile, I'm not sure whether there will be a mdev would like to support both region
> and dma-buf cases. In my opinion, either region or dma-buf is supported by one mdev. (Yeah,
> agree, there may be other cases in future)

There's no requirement to support both.

> It's like we want to propose a general interface used to share guest's buffer with host. And the
> general interface, so far, has two choice: region and dma-buf. So each mdev likes this interface
> can implement one kind of it and gets the benefit from the general interface.
> So, if we think about this, the difference in user mode should be as little as possible.

The difference seems pretty minimal here, the user probes supported
interface types, and explicitly picks one by requesting updates using
that interface type.  The difference is only in the interpretation of
one dword field.  Furthermore, we're not limiting ourselves to these
two interface types, this same API could support dmabuf-v2 if we define
a flag bit for it and define the structure of the interface union.

> > Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In this
> > initial implementation, DMABUF or REGION would always be set by the user to
> > request that type of interface.  Additionally, the QUERY bit could be set to probe
> > compatibility, thus if PROBE and REGION are set, the vendor driver would return
> > success only if it supports the region based interface.  If PROBE and DMABUF are
> > set, the vendor driver returns success only if the dma-buf based interface is
> > supported.  The value of the remainder of the structure is undefined for PROBE.
> > Additionally setting both DMABUF and REGION is invalid.  Undefined flags bits
> > must be validated as zero by the drivers for future use (thus if we later define
> > DMABUFv2, an older driver should automatically return -errno when probed or
> > requested).
> > 
> > It seems like this handles all the cases, the user can ask what's supported and
> > specifies the interface they want on every call.  The user therefore can also
> > choose between region_index and fd and we can make that a union.  
> Agree, that's a good proposal, which can handle all the cases.
> I'm just not sure about the usage case of "on every call". In previous discussion, it seems we think static is enough.

Is it somehow a problem for the user to set the type bit in the flag on
every call?  This makes it explicit that the user is asking for an
update for a specific interface type, if the vendor driver doesn't
support it, return -errno.  This makes it explicit whether the return
is a dmabuf fd or a region index and the user knows how to interpret
that union because they have asked for a specific type.  As you've
likely gathered from my previous replies, I don't like implicit
interfaces.  I don't want the interpretation of a field to depend on
something the user has done in the past and I see no reason to impose a
limitation on the vendor driver supporting only one of two pre-defined
interfaces types.  Thanks,

Alex

> > > 2. For dma-buf, how to differentiate unsupported vs not initialized?
> > > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will  
> > be returned. And -errno will return when meeting other failures, like -ENOMEM.  
> > > If the mdev is not initialized, there won't be any returned err. Just zero all the  
> > fields in structure vfio_device_gfx_plane_info.
> > 
> > So we're relying on special values again :-\  For which fields is zero not a valid
> > value?  I prefer the probe interface above unless there are better ideas.
> >   
> > > 3. The id field in structure vfio_device_gfx_plane_info So far we
> > > haven't figured out the usage of this field for dma-buf usage. So, this field is  
> > changed to "region_index" and only used for region usage.  
> > > In previous discussions, we thought this "id" field might be used for both dma-  
> > buf and region usages.  
> > > So, we proposed some ways, like adding flags field to the structure. Another  
> > way to do it was to add this:  
> > > 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;
> > >  };
> > > Obviously, we don't need to consider this again, unless we find the  
> > "region_index" means something for dma-buf usage.
> > 
> > The usefulness of this ioctl seems really limited and once again we get into a
> > situation where having two ioctls leaves doubt whether this is describing the
> > current plane state.  Thanks,
> > 
> > Alex
> >   
> > > > > > > > > >  include/uapi/linux/vfio.h | 28
> > > > > > > > > > ++++++++++++++++++++++++++++
> > > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230
> > > > > > > > > > 100644
> > > > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > > > @@ -502,6 +502,34 @@ 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 */
> > > > > > > > > > +	__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, upper  
> > > > > left corner  
> > > > > > > > > in pixels */  
> > > > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
> > > > > corner in  
> > > > > > > > > lines*/  
> > > > > > > > > > +	__u32 region_index;
> > > > > > > > > > +	__s32 fd;	/* dma-buf fd */  
> > > > > > > > >
> > > > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > > > Can I ask for one vs the other?  What are the errno values
> > > > > > > > > to differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > > > > flag that I can use to determine what the device supports
> > > > > > > > > without allocating  
> > > > > > > those resources yet?  
> > > > > > > > Dma-buf won't use region_index, which means region_index is
> > > > > > > > zero all the  
> > > > > > > time for dma-buf usage.  
> > > > > > > > As we discussed, there won't be errno if not initialized,
> > > > > > > > just keep all fields  
> > > > > zero.  
> > > > > > > > I will add the comments about these in the next version.
> > > > > > > > Thanks  
> > > > > > >
> > > > > > > Zero is a valid region index.  
> > > > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?  
> > > > >
> > > > > Unlikely, but also valid.  Isn't this why we have a flags field in
> > > > > the structure, so we don't need to rely on implicit values as invalid.
> > > > > Also, all of the previously discussed usage models needs to be
> > > > > documented, either here in the header or in a Documentation/ file.  
> > > > According to the previously discussion, we also have the following propose:
> > > > 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;
> > > > };
> > > > So, we may need to add one more ioctl, e.g.  
> > VFIO_DEVICE_QUERY_GFX_CAPS.  
> > > > User mode can query this before querying the plan info, and to see
> > > > which usage (dma-buf or region) is supported.
> > > > Does it still make sense?
> > > > Thanks.  
> > > So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode  
> > whether the region case is using, instead of introducing a new ioctl.  
> > > Thanks
> > >
> > > Tina  
> > > >
> > > > Tina
> > > >
> > > >  
> > > > > Thanks,
> > > > >
> > > > > Alex
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Gerd Hoffmann Aug. 22, 2017, 8:24 a.m. UTC | #17

Hi,

> These are both from Gerd.  Gerd, do you have any objection to using a
> union to provide either the dmabuf fd or region index?

No.

> > It's like we want to propose a general interface used to share
> > guest's buffer with host. And the
> > general interface, so far, has two choice: region and dma-buf. So
> > each mdev likes this interface
> > can implement one kind of it and gets the benefit from the general
> > interface.
> > So, if we think about this, the difference in user mode should be
> > as little as possible.
> 
> The difference seems pretty minimal here, the user probes supported
> interface types, and explicitly picks one by requesting updates using
> that interface type.  The difference is only in the interpretation of
> one dword field.  Furthermore, we're not limiting ourselves to these
> two interface types, this same API could support dmabuf-v2 if we
> define
> a flag bit for it and define the structure of the interface union.

Yep, using the flags is more future-proof and continues to work in case
another interface type shows up (unlike looking for a gfx region being
present).

> > Agree, that's a good proposal, which can handle all the cases.
> > I'm just not sure about the usage case of "on every call". In
> > previous discussion, it seems we think static is enough.
> 
> Is it somehow a problem for the user to set the type bit in the flag
> on
> every call?

Not at all.

cheers,
  Gerd

Patch

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..827a230 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,34 @@  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 */
+	__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, upper left corner in pixels */
+	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
+	__u32 region_index;
+	__s32 fd;	/* dma-buf fd */
+};
+
+#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
+
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**