[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

✓ Fi.CI.BAT: success for vfio: ABI for mdev display dma-buf operation (rev2)

[Patchwork]

== Series Details ==

Series: vfio: ABI for mdev display dma-buf operation (rev2)
URL   : https://patchwork.freedesktop.org/series/26786/
State : success

== Summary ==

Series 26786v2 vfio: ABI for mdev display dma-buf operation
https://patchwork.freedesktop.org/api/1.0/series/26786/revisions/2/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:432s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:356s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:529s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:515s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:483s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:592s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:416s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:422s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:472s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:575s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:577s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:559s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-6700hq    total:279  pass:262  dwarn:0   dfail:0   fail:0   skip:17  time:591s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:465s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:479s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:439s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:547s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:406s

cefc82aab8bb8ec201e922cf23d227c47845094c drm-tip: 2017y-07m-05d-20h-21m-17s UTC integration manifest
b641b66 vfio: ABI for mdev display dma-buf operation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5119/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.

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

[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 -------- */
>  
>  /**
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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: 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 -------- */
> >
> >  /**
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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: 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx