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

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

[Tina Zhang]

Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
get the plan and its related information.

The dma-buf's life cycle is handled by user mode and tracked by kernel.
The returned fd in struct vfio_device_query_gfx_plane can be a new
fd or an old fd of a re-exported dma-buf. Host User mode can check the
value of fd and to see if it need to creat new resource according to the
new fd or just use the existed resource related to the old fd.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..c92bc69 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
+ *                           struct vfio_device_query_gfx_plane)
+ * Return: 0 on success, -errno on failure.
+ */
+
+struct vfio_device_gfx_plane_info {
+	__u64 start;
+	__u64 drm_format_mod;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 size;
+	__u32 x_pos;
+	__u32 y_pos;
+};
+
+struct vfio_device_query_gfx_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_device_gfx_plane_info plane_info;
+	__u32 plane_type;
+	__s32 fd; /* dma-buf fd */
+	__u32 plane_id;
+};
+
+#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
+
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.7.4

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

[Kirti Wankhede]

On 7/6/2017 3:59 AM, Tina Zhang wrote:
> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
> get the plan and its related information.
> 
> The dma-buf's life cycle is handled by user mode and tracked by kernel.
> The returned fd in struct vfio_device_query_gfx_plane can be a new
> fd or an old fd of a re-exported dma-buf. Host User mode can check the
> value of fd and to see if it need to creat new resource according to the
> new fd or just use the existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..c92bc69 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *                           struct vfio_device_query_gfx_plane)
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +struct vfio_device_gfx_plane_info {
> +	__u64 start;
> +	__u64 drm_format_mod;
> +	__u32 drm_format;
> +	__u32 width;
> +	__u32 height;
> +	__u32 stride;
> +	__u32 size;
> +	__u32 x_pos;
> +	__u32 y_pos;
> +};
> +

Above structure looks good to me.

> +struct vfio_device_query_gfx_plane {
> +	__u32 argsz;
> +	__u32 flags;
> +	struct vfio_device_gfx_plane_info plane_info;
> +	__u32 plane_type;
> +	__s32 fd; /* dma-buf fd */
> +	__u32 plane_id;
> +};
> +

It would be better to have comment here about what are expected values
for plane_type and plane_id.

Thanks,
Kirti

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

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

[Gerd Hoffmann]

> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *                           struct vfio_device_query_gfx_plane)
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +struct vfio_device_gfx_plane_info {
> +	__u64 start;
> +	__u64 drm_format_mod;
> +	__u32 drm_format;
> +	__u32 width;
> +	__u32 height;
> +	__u32 stride;
> +	__u32 size;
> +	__u32 x_pos;
> +	__u32 y_pos;
> +};

Do we want keep that as separate struct?  Given we now have only a
single struct using that as sub-struct it looks pointless, at least
from a API point of view.  Does the driver use the struct internally?

> +
> +struct vfio_device_query_gfx_plane {
> +	__u32 argsz;
> +	__u32 flags;
> +	struct vfio_device_gfx_plane_info plane_info;
> +	__u32 plane_type;
> +	__s32 fd; /* dma-buf fd */
> +	__u32 plane_id;

What is plane_id?

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > +struct vfio_device_query_gfx_plane {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	struct vfio_device_gfx_plane_info plane_info;
> > +	__u32 plane_type;
> > +	__s32 fd; /* dma-buf fd */
> > +	__u32 plane_id;
> > +};
> > +
> 
> It would be better to have comment here about what are expected
> values
> for plane_type and plane_id.

plane_type is DRM_PLANE_TYPE_*.

yes, a comment saying so would be good, same for drm_format which is
DRM_FORMAT_*.  While looking at these two: renaming plane_type to
drm_plane_type (for consistency) is probably a good idea too.

plane_id needs a specification.

cheers,
  Gerd

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

[Daniel Vetter]

On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +struct vfio_device_query_gfx_plane {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +	struct vfio_device_gfx_plane_info plane_info;
> > > +	__u32 plane_type;
> > > +	__s32 fd; /* dma-buf fd */
> > > +	__u32 plane_id;
> > > +};
> > > +
> > 
> > It would be better to have comment here about what are expected
> > values
> > for plane_type and plane_id.
> 
> plane_type is DRM_PLANE_TYPE_*.
> 
> yes, a comment saying so would be good, same for drm_format which is
> DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> drm_plane_type (for consistency) is probably a good idea too.
> 
> plane_id needs a specification.

Why do you need plane_type? With universal planes the plane_id along is
sufficient to identify a plane on a given drm device instance. I'd just
remove it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Jul 06, 2017 at 06:29:55AM +0800, Tina Zhang wrote:
> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
> get the plan and its related information.
> 
> The dma-buf's life cycle is handled by user mode and tracked by kernel.
> The returned fd in struct vfio_device_query_gfx_plane can be a new
> fd or an old fd of a re-exported dma-buf. Host User mode can check the
> value of fd and to see if it need to creat new resource according to the
> new fd or just use the existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..c92bc69 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *                           struct vfio_device_query_gfx_plane)
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +struct vfio_device_gfx_plane_info {
> +	__u64 start;
> +	__u64 drm_format_mod;
> +	__u32 drm_format;
> +	__u32 width;
> +	__u32 height;
> +	__u32 stride;
> +	__u32 size;
> +	__u32 x_pos;
> +	__u32 y_pos;
> +};

Would be good to have a detailed spec of all this stuff, plus what's it
meant to be used for. I assume that e.g. start would be the opaque cookie
thing we've talked about, for dma-buf reuse? Otherwise I'm not sure what
it's good for, since the same gpu vram address can be reused for different
memory objects ...

> +
> +struct vfio_device_query_gfx_plane {
> +	__u32 argsz;
> +	__u32 flags;
> +	struct vfio_device_gfx_plane_info plane_info;
> +	__u32 plane_type;
> +	__s32 fd; /* dma-buf fd */
> +	__u32 plane_id;
> +};

As mentioned in the other reply already, I'm not sure what plane_type is
for. Otherwise this looks ok-ish, but hard to tell without more detailed
spec.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Zhang, Tina]

> -----Original Message-----
> From: Kirti Wankhede [mailto:kwankhede@nvidia.com]
> Sent: Thursday, July 6, 2017 10:02 PM
> To: Zhang, Tina <tina.zhang@intel.com>; alex.williamson@redhat.com;
> kraxel@redhat.com; chris@chris-wilson.co.uk; zhenyuw@linux.intel.com; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> Kevin <kevin.tian@intel.com>; daniel@ffwll.ch
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> On 7/6/2017 3:59 AM, Tina Zhang wrote:
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it need to creat new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..c92bc69 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *                           struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +struct vfio_device_gfx_plane_info {
> > +	__u64 start;
> > +	__u64 drm_format_mod;
> > +	__u32 drm_format;
> > +	__u32 width;
> > +	__u32 height;
> > +	__u32 stride;
> > +	__u32 size;
> > +	__u32 x_pos;
> > +	__u32 y_pos;
> > +};
> > +
> 
> Above structure looks good to me.
> 
> > +struct vfio_device_query_gfx_plane {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	struct vfio_device_gfx_plane_info plane_info;
> > +	__u32 plane_type;
> > +	__s32 fd; /* dma-buf fd */
> > +	__u32 plane_id;
> > +};
> > +
> 
> It would be better to have comment here about what are expected values for
> plane_type and plane_id.
I will add the comments for these fields. Thanks.

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

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

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Tuesday, July 11, 2017 5:13 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; linux-kernel@vger.kernel.org; intel-
> gfx@lists.freedesktop.org; alex.williamson@redhat.com;
> zhenyuw@linux.intel.com; chris@chris-wilson.co.uk; Kirti Wankhede
> <kwankhede@nvidia.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
> daniel@ffwll.ch; Zhang, Tina <tina.zhang@intel.com>; intel-gvt-
> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > +struct vfio_device_query_gfx_plane {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	struct vfio_device_gfx_plane_info plane_info;
> > > > +	__u32 plane_type;
> > > > +	__s32 fd; /* dma-buf fd */
> > > > +	__u32 plane_id;
> > > > +};
> > > > +
> > >
> > > It would be better to have comment here about what are expected
> > > values for plane_type and plane_id.
> >
> > plane_type is DRM_PLANE_TYPE_*.
> >
> > yes, a comment saying so would be good, same for drm_format which is
> > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > drm_plane_type (for consistency) is probably a good idea too.
> >
> > plane_id needs a specification.
> 
> Why do you need plane_type? With universal planes the plane_id along is
> sufficient to identify a plane on a given drm device instance. I'd just remove it.
> -Daniel
The plane_type here, is to ask the mdev vendor driver to return the information according to the value in field plane_type. So, it's a input field.
The values in plane_type field is the same of drm_plane_type. And yes, it's better to use drm_plane_type instead of plane_id. 
Thanks.

Tina
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

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

[Zhang, Tina]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Tuesday, July 11, 2017 2:08 PM
> To: Zhang, Tina <tina.zhang@intel.com>; alex.williamson@redhat.com;
> chris@chris-wilson.co.uk; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; daniel@ffwll.ch; kwankhede@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *                           struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +struct vfio_device_gfx_plane_info {
> > +	__u64 start;
> > +	__u64 drm_format_mod;
> > +	__u32 drm_format;
> > +	__u32 width;
> > +	__u32 height;
> > +	__u32 stride;
> > +	__u32 size;
> > +	__u32 x_pos;
> > +	__u32 y_pos;
> > +};
> 
> Do we want keep that as separate struct?  Given we now have only a single
> struct using that as sub-struct it looks pointless, at least from a API point of view.
> Does the driver use the struct internally?
Driver has another struct which is super-set of these fields. Yes, we can move all these fields into struct vfio_device_query_gfx_plane. Also, we can remove some of these fields which may seem useless for user mode.
Thanks.

Tina
> 
> > +
> > +struct vfio_device_query_gfx_plane {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	struct vfio_device_gfx_plane_info plane_info;
> > +	__u32 plane_type;
> > +	__s32 fd; /* dma-buf fd */
> > +	__u32 plane_id;
> 
> What is plane_id?
I cannot figure out the mean of plane_id either. If I remember correctly, it was asked by region usage. Of course, if no one needs it, I'd like to remove it.
Thanks.

Tina

> 
> cheers,
>   Gerd

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

[Zhang, Tina]

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

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

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

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

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

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On 2017.07.11 11:12:36 +0200, Daniel Vetter wrote:
> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > +struct vfio_device_query_gfx_plane {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	struct vfio_device_gfx_plane_info plane_info;
> > > > +	__u32 plane_type;
> > > > +	__s32 fd; /* dma-buf fd */
> > > > +	__u32 plane_id;
> > > > +};
> > > > +
> > > 
> > > It would be better to have comment here about what are expected
> > > values
> > > for plane_type and plane_id.
> > 
> > plane_type is DRM_PLANE_TYPE_*.
> > 
> > yes, a comment saying so would be good, same for drm_format which is
> > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > drm_plane_type (for consistency) is probably a good idea too.

For drm universal plane, this is not in drm uapi, but uabi. I think we
can align with drm plane definition for sure, but not need to pull in
drm header for that enum type.

> > 
> > plane_id needs a specification.
> 
> Why do you need plane_type? With universal planes the plane_id along is
> sufficient to identify a plane on a given drm device instance. I'd just
> remove it.

This interface is to get vGPU display plane info, there's no normal
drm kms client involved, but vGPU device model trys to expose guest
planes for display. We need to ask for what type of plane required on
target vGPU. I think plane_id here doesn't mean like in drm kms,
but I'm not sure about plane_id here without details, what's the
purpose, etc.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Daniel Vetter]

On Wed, Jul 12, 2017 at 02:31:40AM +0000, Zhang, Tina wrote:
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of Daniel Vetter
> > Sent: Tuesday, July 11, 2017 5:13 PM
> > To: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; linux-kernel@vger.kernel.org; intel-
> > gfx@lists.freedesktop.org; alex.williamson@redhat.com;
> > zhenyuw@linux.intel.com; chris@chris-wilson.co.uk; Kirti Wankhede
> > <kwankhede@nvidia.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
> > daniel@ffwll.ch; Zhang, Tina <tina.zhang@intel.com>; intel-gvt-
> > dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> > Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> > 
> > On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > > +struct vfio_device_query_gfx_plane {
> > > > > +	__u32 argsz;
> > > > > +	__u32 flags;
> > > > > +	struct vfio_device_gfx_plane_info plane_info;
> > > > > +	__u32 plane_type;
> > > > > +	__s32 fd; /* dma-buf fd */
> > > > > +	__u32 plane_id;
> > > > > +};
> > > > > +
> > > >
> > > > It would be better to have comment here about what are expected
> > > > values for plane_type and plane_id.
> > >
> > > plane_type is DRM_PLANE_TYPE_*.
> > >
> > > yes, a comment saying so would be good, same for drm_format which is
> > > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > > drm_plane_type (for consistency) is probably a good idea too.
> > >
> > > plane_id needs a specification.
> > 
> > Why do you need plane_type? With universal planes the plane_id along is
> > sufficient to identify a plane on a given drm device instance. I'd just remove it.
> > -Daniel
> The plane_type here, is to ask the mdev vendor driver to return the information according to the value in field plane_type. So, it's a input field.
> The values in plane_type field is the same of drm_plane_type. And yes, it's better to use drm_plane_type instead of plane_id. 

I have no idea what you mean here, I guess that just shows that discussing
an ioctl struct without solid definitions of what field does what and why
is not all that useful. What exactly it plane_id for then?

This just confused me more ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Kirti Wankhede]

On 7/12/2017 1:10 PM, Daniel Vetter wrote:
> On Wed, Jul 12, 2017 at 02:31:40AM +0000, Zhang, Tina wrote:
>>
>>
>>> -----Original Message-----
>>> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>>> Behalf Of Daniel Vetter
>>> Sent: Tuesday, July 11, 2017 5:13 PM
>>> To: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Tian, Kevin <kevin.tian@intel.com>; linux-kernel@vger.kernel.org; intel-
>>> gfx@lists.freedesktop.org; alex.williamson@redhat.com;
>>> zhenyuw@linux.intel.com; chris@chris-wilson.co.uk; Kirti Wankhede
>>> <kwankhede@nvidia.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
>>> daniel@ffwll.ch; Zhang, Tina <tina.zhang@intel.com>; intel-gvt-
>>> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
>>>
>>> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
>>>>   Hi,
>>>>
>>>>>> +struct vfio_device_query_gfx_plane {
>>>>>> +	__u32 argsz;
>>>>>> +	__u32 flags;
>>>>>> +	struct vfio_device_gfx_plane_info plane_info;
>>>>>> +	__u32 plane_type;
>>>>>> +	__s32 fd; /* dma-buf fd */
>>>>>> +	__u32 plane_id;
>>>>>> +};
>>>>>> +
>>>>>
>>>>> It would be better to have comment here about what are expected
>>>>> values for plane_type and plane_id.
>>>>
>>>> plane_type is DRM_PLANE_TYPE_*.
>>>>
>>>> yes, a comment saying so would be good, same for drm_format which is
>>>> DRM_FORMAT_*.  While looking at these two: renaming plane_type to
>>>> drm_plane_type (for consistency) is probably a good idea too.
>>>>
>>>> plane_id needs a specification.
>>>
>>> Why do you need plane_type? With universal planes the plane_id along is
>>> sufficient to identify a plane on a given drm device instance. I'd just remove it.
>>> -Daniel
>> The plane_type here, is to ask the mdev vendor driver to return the information according to the value in field plane_type. So, it's a input field.
>> The values in plane_type field is the same of drm_plane_type. And yes, it's better to use drm_plane_type instead of plane_id. 
> 
> I have no idea what you mean here, I guess that just shows that discussing
> an ioctl struct without solid definitions of what field does what and why
> is not all that useful. What exactly it plane_id for then?
> 

plane type could be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

In case when VFIO region is used to provide surface to QEMU, plane_id
would be region index, for example region 10 could be used for primary
surface and region 11 could be used for cursor surface. So in that case,
mdev vendor driver should return plane_type and its corresponding plane_id.

Thanks,
Kirti

> This just confused me more ...
> -Daniel
> 

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

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Kirti Wankhede
> Sent: Wednesday, July 12, 2017 8:45 PM
> To: Zhang, Tina <tina.zhang@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>; linux-
> kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org;
> alex.williamson@redhat.com; zhenyuw@linux.intel.com; chris@chris-
> wilson.co.uk; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> On 7/12/2017 1:10 PM, Daniel Vetter wrote:
> > On Wed, Jul 12, 2017 at 02:31:40AM +0000, Zhang, Tina wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: intel-gvt-dev
> >>> [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
> >>> Daniel Vetter
> >>> Sent: Tuesday, July 11, 2017 5:13 PM
> >>> To: Gerd Hoffmann <kraxel@redhat.com>
> >>> Cc: Tian, Kevin <kevin.tian@intel.com>;
> >>> linux-kernel@vger.kernel.org; intel- gfx@lists.freedesktop.org;
> >>> alex.williamson@redhat.com; zhenyuw@linux.intel.com;
> >>> chris@chris-wilson.co.uk; Kirti Wankhede <kwankhede@nvidia.com>; Lv,
> >>> Zhiyuan <zhiyuan.lv@intel.com>; daniel@ffwll.ch; Zhang, Tina
> >>> <tina.zhang@intel.com>; intel-gvt- dev@lists.freedesktop.org; Wang,
> >>> Zhi A <zhi.a.wang@intel.com>
> >>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf
> >>> operation
> >>>
> >>> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >>>>   Hi,
> >>>>
> >>>>>> +struct vfio_device_query_gfx_plane {
> >>>>>> +	__u32 argsz;
> >>>>>> +	__u32 flags;
> >>>>>> +	struct vfio_device_gfx_plane_info plane_info;
> >>>>>> +	__u32 plane_type;
> >>>>>> +	__s32 fd; /* dma-buf fd */
> >>>>>> +	__u32 plane_id;
> >>>>>> +};
> >>>>>> +
> >>>>>
> >>>>> It would be better to have comment here about what are expected
> >>>>> values for plane_type and plane_id.
> >>>>
> >>>> plane_type is DRM_PLANE_TYPE_*.
> >>>>
> >>>> yes, a comment saying so would be good, same for drm_format which
> >>>> is DRM_FORMAT_*.  While looking at these two: renaming plane_type
> >>>> to drm_plane_type (for consistency) is probably a good idea too.
> >>>>
> >>>> plane_id needs a specification.
> >>>
> >>> Why do you need plane_type? With universal planes the plane_id along
> >>> is sufficient to identify a plane on a given drm device instance. I'd just
> remove it.
> >>> -Daniel
> >> The plane_type here, is to ask the mdev vendor driver to return the
> information according to the value in field plane_type. So, it's a input field.
> >> The values in plane_type field is the same of drm_plane_type. And yes, it's
> better to use drm_plane_type instead of plane_id.
> >
> > I have no idea what you mean here, I guess that just shows that
> > discussing an ioctl struct without solid definitions of what field
> > does what and why is not all that useful. What exactly it plane_id for then?
> >
> 
> plane type could be DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR.
> 
> In case when VFIO region is used to provide surface to QEMU, plane_id would
> be region index, for example region 10 could be used for primary surface and
> region 11 could be used for cursor surface. So in that case, mdev vendor driver
> should return plane_type and its corresponding plane_id.
Thanks, Kirti, do you mean there will be multiple DRM_PLANE_TYPE_PRIMARY and multiple DRM_PLANE_TYPE_CURSOR planes existing in the same time and region usage needs to use plane_id to distinguish among them? Is it for the multiple output or that's the typical way of region usage? Thanks.

Tina

> 
> Thanks,
> Kirti
> 
> > This just confused me more ...
> > -Daniel
> >
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

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

[Gerd Hoffmann]

  Hi,

> In case when VFIO region is used to provide surface to QEMU, plane_id
> would be region index,

Then we should name it "region_index" not "plane_id".

> for example region 10 could be used for primary
> surface and region 11 could be used for cursor surface. So in that
> case,
> mdev vendor driver should return plane_type and its corresponding
> plane_id.

Hmm?  plane_type is the input (userspace -> kernel) parameter.

cheers,
  Gerd

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

[Kirti Wankhede]

On 7/14/2017 7:00 AM, Zhang, Tina wrote:
> 
> 
>> -----Original Message-----
>> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>> Behalf Of Kirti Wankhede
>> Sent: Wednesday, July 12, 2017 8:45 PM
>> To: Zhang, Tina <tina.zhang@intel.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>; linux-
>> kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org;
>> alex.williamson@redhat.com; zhenyuw@linux.intel.com; chris@chris-
>> wilson.co.uk; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
>> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
>>
>>
>>
>> On 7/12/2017 1:10 PM, Daniel Vetter wrote:
>>> On Wed, Jul 12, 2017 at 02:31:40AM +0000, Zhang, Tina wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: intel-gvt-dev
>>>>> [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
>>>>> Daniel Vetter
>>>>> Sent: Tuesday, July 11, 2017 5:13 PM
>>>>> To: Gerd Hoffmann <kraxel@redhat.com>
>>>>> Cc: Tian, Kevin <kevin.tian@intel.com>;
>>>>> linux-kernel@vger.kernel.org; intel- gfx@lists.freedesktop.org;
>>>>> alex.williamson@redhat.com; zhenyuw@linux.intel.com;
>>>>> chris@chris-wilson.co.uk; Kirti Wankhede <kwankhede@nvidia.com>; Lv,
>>>>> Zhiyuan <zhiyuan.lv@intel.com>; daniel@ffwll.ch; Zhang, Tina
>>>>> <tina.zhang@intel.com>; intel-gvt- dev@lists.freedesktop.org; Wang,
>>>>> Zhi A <zhi.a.wang@intel.com>
>>>>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf
>>>>> operation
>>>>>
>>>>> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
>>>>>>   Hi,
>>>>>>
>>>>>>>> +struct vfio_device_query_gfx_plane {
>>>>>>>> +	__u32 argsz;
>>>>>>>> +	__u32 flags;
>>>>>>>> +	struct vfio_device_gfx_plane_info plane_info;
>>>>>>>> +	__u32 plane_type;
>>>>>>>> +	__s32 fd; /* dma-buf fd */
>>>>>>>> +	__u32 plane_id;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> It would be better to have comment here about what are expected
>>>>>>> values for plane_type and plane_id.
>>>>>>
>>>>>> plane_type is DRM_PLANE_TYPE_*.
>>>>>>
>>>>>> yes, a comment saying so would be good, same for drm_format which
>>>>>> is DRM_FORMAT_*.  While looking at these two: renaming plane_type
>>>>>> to drm_plane_type (for consistency) is probably a good idea too.
>>>>>>
>>>>>> plane_id needs a specification.
>>>>>
>>>>> Why do you need plane_type? With universal planes the plane_id along
>>>>> is sufficient to identify a plane on a given drm device instance. I'd just
>> remove it.
>>>>> -Daniel
>>>> The plane_type here, is to ask the mdev vendor driver to return the
>> information according to the value in field plane_type. So, it's a input field.
>>>> The values in plane_type field is the same of drm_plane_type. And yes, it's
>> better to use drm_plane_type instead of plane_id.
>>>
>>> I have no idea what you mean here, I guess that just shows that
>>> discussing an ioctl struct without solid definitions of what field
>>> does what and why is not all that useful. What exactly it plane_id for then?
>>>
>>
>> plane type could be DRM_PLANE_TYPE_PRIMARY or
>> DRM_PLANE_TYPE_CURSOR.
>>
>> In case when VFIO region is used to provide surface to QEMU, plane_id would
>> be region index, for example region 10 could be used for primary surface and
>> region 11 could be used for cursor surface. So in that case, mdev vendor driver
>> should return plane_type and its corresponding plane_id.

> Thanks, Kirti, do you mean there will be multiple DRM_PLANE_TYPE_PRIMARY and multiple DRM_PLANE_TYPE_CURSOR planes existing in the same time and region usage needs to use plane_id to distinguish among them? Is it for the multiple output or that's the typical way of region usage? Thanks.

There could be only two planes, one DRM_PLANE_TYPE_PRIMARY and one
DRM_PLANE_TYPE_CURSOR.
Steps from gfx_update for region case would be:
- VFIO_DEVICE_QUERY_GFX_PLANE with plane_type = DRM_PLANE_TYPE_PRIMARY
- if vfio_device_gfx_plane_info.size > 0, read region for primary
surface and update console surface
- VFIO_DEVICE_QUERY_GFX_PLANE with plane_type = DRM_PLANE_TYPE_CURSOR
- if vfio_device_gfx_plane_info.size > 0, read region for cursor surface
update cursor on surface.

Thanks,
Kirti


> 
> Tina
> 
>>
>> Thanks,
>> Kirti
>>
>>> This just confused me more ...
>>> -Daniel
>>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

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

[Kirti Wankhede]

On 7/14/2017 3:31 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> In case when VFIO region is used to provide surface to QEMU, plane_id
>> would be region index,
> 
> Then we should name it "region_index" not "plane_id".
> 
>> for example region 10 could be used for primary
>> surface and region 11 could be used for cursor surface. So in that
>> case,
>> mdev vendor driver should return plane_type and its corresponding
>> plane_id.
> 
> Hmm?  plane_type is the input (userspace -> kernel) parameter.
> 

Yes, that's right. Sorry for confusion. And mdev vendor driver would
return which region to read.

Thanks,
Kirti

> cheers,
>   Gerd
> 

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

[Gerd Hoffmann]

On Fri, 2017-07-14 at 15:45 +0530, Kirti Wankhede wrote:
> 
> On 7/14/2017 3:31 PM, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > In case when VFIO region is used to provide surface to QEMU,
> > > plane_id
> > > would be region index,
> > 
> > Then we should name it "region_index" not "plane_id".
> > 
> > > for example region 10 could be used for primary
> > > surface and region 11 could be used for cursor surface. So in
> > > that
> > > case,
> > > mdev vendor driver should return plane_type and its corresponding
> > > plane_id.
> > 
> > Hmm?  plane_type is the input (userspace -> kernel) parameter.
> > 
> 
> Yes, that's right. Sorry for confusion. And mdev vendor driver would
> return which region to read.

Ok, so the plan for supporting multiple planes is to have a separate
vfio region for each?  Ok, we should add region_index to the struct
then.

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> There could be only two planes, one DRM_PLANE_TYPE_PRIMARY and one
> DRM_PLANE_TYPE_CURSOR.
> Steps from gfx_update for region case would be:
> - VFIO_DEVICE_QUERY_GFX_PLANE with plane_type =
> DRM_PLANE_TYPE_PRIMARY

> - if vfio_device_gfx_plane_info.size > 0, read region for primary
> surface and update console surface

Why?  I suspect you want notify the caller whenever the surface has
been updated or not?  If so we should add an explicit flag or field for
that.

cheers,
  Gerd

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

[Kirti Wankhede]

On 7/14/2017 5:35 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> There could be only two planes, one DRM_PLANE_TYPE_PRIMARY and one
>> DRM_PLANE_TYPE_CURSOR.
>> Steps from gfx_update for region case would be:
>> - VFIO_DEVICE_QUERY_GFX_PLANE with plane_type =
>> DRM_PLANE_TYPE_PRIMARY
> 
>> - if vfio_device_gfx_plane_info.size > 0, read region for primary
>> surface and update console surface
> 
> Why?  I suspect you want notify the caller whenever the surface has
> been updated or not?  If so we should add an explicit flag or field for
> that.
> 

No need of flag here. If vGPU driver is not loaded in the guest, there
is no surface being managed by vGPU, in that case this size will be zero.

Thanks,
Kirti

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

[Gerd Hoffmann]

  Hi,

> No need of flag here. If vGPU driver is not loaded in the guest,
> there
> is no surface being managed by vGPU, in that case this size will be
> zero.

Ok, we certainly have the same situation with intel.  When the guest
driver is not loaded (yet) there is no valid surface.

We should cleanly define what the ioctl should do in that case, so all
drivers behave the same way.

I'd suggest that all fields defining the surface (drm_format, width,
height, stride, size) should be set to zero in that case.

cheers,
  Gerd

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

[Zhang, Tina]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, July 17, 2017 7:03 PM
> To: Kirti Wankhede <kwankhede@nvidia.com>; Zhang, Tina
> <tina.zhang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; linux-
> kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org;
> alex.williamson@redhat.com; zhenyuw@linux.intel.com; chris@chris-
> wilson.co.uk; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > No need of flag here. If vGPU driver is not loaded in the guest, there
> > is no surface being managed by vGPU, in that case this size will be
> > zero.
> 
> Ok, we certainly have the same situation with intel.  When the guest driver is not
> loaded (yet) there is no valid surface.
> 
> We should cleanly define what the ioctl should do in that case, so all drivers
> behave the same way.
> 
> I'd suggest that all fields defining the surface (drm_format, width, height, stride,
> size) should be set to zero in that case.
Yeah, it's reasonable. How about the return value? Currently, the ioctl also returns "-ENODEV" in that situation.
 
thanks,
Tina
> 
> cheers,
>   Gerd

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

[Gerd Hoffmann]

On Wed, 2017-07-19 at 00:16 +0000, Zhang, Tina wrote:
> > -----Original Message-----
> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> > Sent: Monday, July 17, 2017 7:03 PM
> > To: Kirti Wankhede <kwankhede@nvidia.com>; Zhang, Tina
> > <tina.zhang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; linux-
> > kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org;
> > alex.williamson@redhat.com; zhenyuw@linux.intel.com; chris@chris-
> > wilson.co.uk; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
> > dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> > Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf
> > operation
> > 
> >   Hi,
> > 
> > > No need of flag here. If vGPU driver is not loaded in the guest,
> > > there
> > > is no surface being managed by vGPU, in that case this size will
> > > be
> > > zero.
> > 
> > Ok, we certainly have the same situation with intel.  When the
> > guest driver is not
> > loaded (yet) there is no valid surface.
> > 
> > We should cleanly define what the ioctl should do in that case, so
> > all drivers
> > behave the same way.
> > 
> > I'd suggest that all fields defining the surface (drm_format,
> > width, height, stride,
> > size) should be set to zero in that case.
> 
> Yeah, it's reasonable. How about the return value? Currently, the
> ioctl also returns "-ENODEV" in that situation.

I think it should not return an error.  Querying the plane parameters
worked fine.

cheers,
  Gerd

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

[Kirti Wankhede]

On 7/19/2017 11:55 AM, Gerd Hoffmann wrote:
> On Wed, 2017-07-19 at 00:16 +0000, Zhang, Tina wrote:
>>> -----Original Message-----
>>> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>>> Sent: Monday, July 17, 2017 7:03 PM
>>> To: Kirti Wankhede <kwankhede@nvidia.com>; Zhang, Tina
>>> <tina.zhang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; linux-
>>> kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org;
>>> alex.williamson@redhat.com; zhenyuw@linux.intel.com; chris@chris-
>>> wilson.co.uk; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
>>> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf
>>> operation
>>>
>>>   Hi,
>>>
>>>> No need of flag here. If vGPU driver is not loaded in the guest,
>>>> there
>>>> is no surface being managed by vGPU, in that case this size will
>>>> be
>>>> zero.
>>>
>>> Ok, we certainly have the same situation with intel.  When the
>>> guest driver is not
>>> loaded (yet) there is no valid surface.
>>>
>>> We should cleanly define what the ioctl should do in that case, so
>>> all drivers
>>> behave the same way.
>>>
>>> I'd suggest that all fields defining the surface (drm_format,
>>> width, height, stride,
>>> size) should be set to zero in that case.
>>
>> Yeah, it's reasonable. How about the return value? Currently, the
>> ioctl also returns "-ENODEV" in that situation.
> 
> I think it should not return an error.  Querying the plane parameters
> worked fine.
> 

Sounds good to me too.

Thanks,
Kirti