[PATCH v9 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[PATCH v9 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[Xiaoguang Chen]

v8->v9:
1) refine the dma-buf ioctl definition
2) add a lock to protect the dmabuf list
3) move drm format change to a separate patch
4) codes cleanup

v7->v8:
1) refine framebuffer decoder code
2) fix a bug in decoding primary plane

v6->v7:
1) release dma-buf related allocations in dma-buf's associated release
function.
2) refine ioctl interface for querying plane info or create dma-buf
3) refine framebuffer decoder code
4) the patch series is based on 4.12.0-rc1

v5->v6:
1) align the dma-buf life cycle with the vfio device.
2) add the dma-buf releated operations in a separate patch.
3) i915 releated changes.

v4->v5:
1) fix bug while checking whether the gem obj is gvt's dma-buf when user
change caching mode or domains. Add a helper function to do it.
2) add definition for the query plane and create dma-buf.

v3->v4:
1) fix bug while checking whether the gem obj is gvt's dma-buf when set
caching mode or doamins.

v2->v3:
1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save
the decoded plane information to avoid look up while need the plane info.
2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in drm_i915_gem_object
to represent the gem obj for gvt's dma-buf. The tiling mode, caching mode
and domains can not be changed for this kind of gem object.
3) change dma-buf related information to be more generic. So other vendor
can use the same interface.

v1->v2:
1) create a management fd for dma-buf operations.
2) alloc gem object's backing storage in gem obj's get_pages() callback.

This patch set adds the dma-buf support for intel GVT-g.
dma-buf is a uniform mechanism to share DMA buffers across different
devices and sub-systems.
dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer
to other users or sub-systems so they can use the dma-buf to show the
desktop of a vm which uses intel vgpu.

The main idea is we create a gem object and set vgpu's framebuffer as
the backing storage of this gem object. And associate this gem obj
to a dma-buf object then export this dma-buf at the meantime
generate a file descriptor for this dma-buf. Finally deliver this file
descriptor to user space. And user can use this dma-buf fd to do render
or other operations.
User need to create a fd(for intel GVT-g dma-buf support it is a:dma-buf
management fd) then user can use this fd to query the plane information
or create a dma-buf. The life cycle of this fd is managed by GVT-g user
do not need to care about that.

We have an example program on how to use the dma-buf. You can download
the program to have a try. Good luck :)
git repo: https://github.com/01org/igvtg-qemu branch:kvmgt_dmabuf_example

Xiaoguang Chen (7):
  drm/i915/gvt: Extend the GVT-g architecture
  drm/i915/gvt: OpRegion support for GVT-g
  drm: Extend the drm format
  drm/i915/gvt: Frame buffer decoder support for GVT-g
  vfio: Define vfio based dma-buf operations
  drm/i915/gvt: Dmabuf support for GVT-g
  drm/i915/gvt: Adding user interface for dma-buf

 drivers/gpu/drm/i915/gvt/Makefile      |   3 +-
 drivers/gpu/drm/i915/gvt/display.c     |   2 +-
 drivers/gpu/drm/i915/gvt/display.h     |   2 +
 drivers/gpu/drm/i915/gvt/dmabuf.c      | 307 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h      |  42 ++++
 drivers/gpu/drm/i915/gvt/fb_decoder.c  | 425 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h  | 171 +++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.c         |   3 +
 drivers/gpu/drm/i915/gvt/gvt.h         |   8 +
 drivers/gpu/drm/i915/gvt/hypercall.h   |   4 +
 drivers/gpu/drm/i915/gvt/kvmgt.c       | 245 ++++++++++++++++++-
 drivers/gpu/drm/i915/gvt/mpt.h         |  45 ++++
 drivers/gpu/drm/i915/gvt/opregion.c    |  26 +-
 drivers/gpu/drm/i915/gvt/vgpu.c        |   6 +
 drivers/gpu/drm/i915/i915_gem.c        |  26 +-
 drivers/gpu/drm/i915/i915_gem_object.h |   9 +
 drivers/gpu/drm/i915/i915_gem_tiling.c |   5 +
 include/uapi/drm/drm_fourcc.h          |   4 +
 include/uapi/linux/vfio.h              |  57 +++++
 19 files changed, 1378 insertions(+), 12 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

-- 
2.7.4

[PATCH v9 1/7] drm/i915/gvt: Extend the GVT-g architecture

[Xiaoguang Chen]

This patch extends the GVT-g architecture to support vfio device region.
Later we will add a vfio device region for the vgpu to support OpRegion.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 1ae0b40..3c6a02b 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -53,11 +53,21 @@ static const struct intel_gvt_ops *intel_gvt_ops;
 #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
 #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
 
+struct vfio_region;
+struct intel_vgpu_regops {
+	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
+			size_t count, loff_t *ppos, bool iswrite);
+	void (*release)(struct intel_vgpu *vgpu,
+			struct vfio_region *region);
+};
+
 struct vfio_region {
 	u32				type;
 	u32				subtype;
 	size_t				size;
 	u32				flags;
+	const struct intel_vgpu_regops	*ops;
+	void				*data;
 };
 
 struct kvmgt_pgfn {
@@ -642,7 +652,7 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	int ret = -EINVAL;
 
 
-	if (index >= VFIO_PCI_NUM_REGIONS) {
+	if (index >= VFIO_PCI_NUM_REGIONS + vgpu->vdev.num_regions) {
 		gvt_vgpu_err("invalid index: %u\n", index);
 		return -EINVAL;
 	}
@@ -676,8 +686,11 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	case VFIO_PCI_BAR5_REGION_INDEX:
 	case VFIO_PCI_VGA_REGION_INDEX:
 	case VFIO_PCI_ROM_REGION_INDEX:
+		break;
 	default:
-		gvt_vgpu_err("unsupported region: %u\n", index);
+		index -= VFIO_PCI_NUM_REGIONS;
+		return vgpu->vdev.region[index].ops->rw(vgpu, buf, count,
+				ppos, is_write);
 	}
 
 	return ret == 0 ? count : ret;
@@ -940,7 +953,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 
 		info.flags = VFIO_DEVICE_FLAGS_PCI;
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
-		info.num_regions = VFIO_PCI_NUM_REGIONS;
+		info.num_regions = VFIO_PCI_NUM_REGIONS +
+				vgpu->vdev.num_regions;
 		info.num_irqs = VFIO_PCI_NUM_IRQS;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
@@ -1061,6 +1075,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		}
 
 		if (caps.size) {
+			info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
 			if (info.argsz < sizeof(info) + caps.size) {
 				info.argsz = sizeof(info) + caps.size;
 				info.cap_offset = 0;
-- 
2.7.4

[PATCH v9 2/7] drm/i915/gvt: OpRegion support for GVT-g

[Xiaoguang Chen]

OpRegion is needed to support display related operation for
intel vgpu.

A vfio device region is added to intel vgpu to deliver the
host OpRegion information to user space so user space can
construct the OpRegion for vgpu.

Signed-off-by: Bing Niu <bing.niu@intel.com>
Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 88 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/mpt.h       | 15 ++++++
 drivers/gpu/drm/i915/gvt/opregion.c  | 26 ++++++++---
 drivers/gpu/drm/i915/gvt/vgpu.c      |  4 ++
 5 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index df7f33a..32c345c 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -55,6 +55,7 @@ struct intel_gvt_mpt {
 			      unsigned long mfn, unsigned int nr, bool map);
 	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
 			     bool map);
+	int (*set_opregion)(void *vgpu);
 };
 
 extern struct intel_gvt_mpt xengt_mpt;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 3c6a02b..6b4652a 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -53,6 +53,8 @@ static const struct intel_gvt_ops *intel_gvt_ops;
 #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
 #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
 
+#define OPREGION_SIGNATURE "IntelGraphicsMem"
+
 struct vfio_region;
 struct intel_vgpu_regops {
 	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
@@ -436,6 +438,91 @@ static void kvmgt_protect_table_del(struct kvmgt_guest_info *info,
 	}
 }
 
+static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
+		size_t count, loff_t *ppos, bool iswrite)
+{
+	unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
+			VFIO_PCI_NUM_REGIONS;
+	void *base = vgpu->vdev.region[i].data;
+	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+
+	if (pos >= vgpu->vdev.region[i].size || iswrite) {
+		gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
+		return -EINVAL;
+	}
+	count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
+	memcpy(buf, base + pos, count);
+
+	return count;
+}
+
+static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu,
+		struct vfio_region *region)
+{
+	memunmap(region->data);
+}
+
+static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
+	.rw = intel_vgpu_reg_rw_opregion,
+	.release = intel_vgpu_reg_release_opregion,
+};
+
+static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype,
+		const struct intel_vgpu_regops *ops,
+		size_t size, u32 flags, void *data)
+{
+	struct vfio_region *region;
+
+	region = krealloc(vgpu->vdev.region,
+			(vgpu->vdev.num_regions + 1) * sizeof(*region),
+			GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	vgpu->vdev.region = region;
+	vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
+	vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
+	vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
+	vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
+	vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
+	vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
+	vgpu->vdev.num_regions++;
+
+	return 0;
+}
+
+static int kvmgt_set_opregion(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+	unsigned int addr;
+	void *base;
+	int ret;
+
+	addr = vgpu->gvt->opregion.opregion_pa;
+	if (!addr || !(~addr))
+		return -ENODEV;
+
+	base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
+	if (!base)
+		return -ENOMEM;
+
+	if (memcmp(base, OPREGION_SIGNATURE, 16)) {
+		memunmap(base);
+		return -EINVAL;
+	}
+
+	ret = intel_vgpu_register_reg(vgpu,
+			PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+			VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
+			&intel_vgpu_regops_opregion, OPREGION_SIZE,
+			VFIO_REGION_INFO_FLAG_READ, base);
+	if (ret)
+		memunmap(base);
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -1524,6 +1611,7 @@ struct intel_gvt_mpt kvmgt_mpt = {
 	.read_gpa = kvmgt_read_gpa,
 	.write_gpa = kvmgt_write_gpa,
 	.gfn_to_mfn = kvmgt_gfn_to_pfn,
+	.set_opregion = kvmgt_set_opregion,
 };
 EXPORT_SYMBOL_GPL(kvmgt_mpt);
 
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 4193536..ab71300 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -293,4 +293,19 @@ static inline int intel_gvt_hypervisor_set_trap_area(
 	return intel_gvt_host.mpt->set_trap_area(vgpu->handle, start, end, map);
 }
 
+/**
+ * intel_gvt_hypervisor_set_opregion - Set opregion for guest
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->set_opregion)
+		return 0;
+
+	return intel_gvt_host.mpt->set_opregion(vgpu);
+}
+
 #endif /* _GVT_MPT_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 3117991..04c0452 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -113,23 +113,37 @@ void intel_vgpu_clean_opregion(struct intel_vgpu *vgpu)
  */
 int intel_vgpu_init_opregion(struct intel_vgpu *vgpu, u32 gpa)
 {
-	int ret;
+	int ret = 0;
+	unsigned long pfn;
 
 	gvt_dbg_core("vgpu%d: init vgpu opregion\n", vgpu->id);
 
-	if (intel_gvt_host.hypervisor_type == INTEL_GVT_HYPERVISOR_XEN) {
+	switch (intel_gvt_host.hypervisor_type) {
+	case INTEL_GVT_HYPERVISOR_KVM:
+		pfn = intel_gvt_hypervisor_gfn_to_mfn(vgpu, gpa >> PAGE_SHIFT);
+		vgpu_opregion(vgpu)->va = memremap(pfn << PAGE_SHIFT,
+						INTEL_GVT_OPREGION_SIZE,
+						MEMREMAP_WB);
+		if (!vgpu_opregion(vgpu)->va) {
+			gvt_vgpu_err("failed to map guest opregion\n");
+			ret = -EFAULT;
+		}
+		break;
+	case INTEL_GVT_HYPERVISOR_XEN:
 		gvt_dbg_core("emulate opregion from kernel\n");
 
 		ret = init_vgpu_opregion(vgpu, gpa);
 		if (ret)
-			return ret;
+			break;
 
 		ret = map_vgpu_opregion(vgpu, true);
-		if (ret)
-			return ret;
+		break;
+	default:
+		ret = -EINVAL;
+		gvt_vgpu_err("not supported hypervisor\n");
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 6e3cbd8..8e1d504 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -383,6 +383,10 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
 	if (ret)
 		goto out_clean_shadow_ctx;
 
+	ret = intel_gvt_hypervisor_set_opregion(vgpu);
+	if (ret)
+		goto out_clean_shadow_ctx;
+
 	mutex_unlock(&gvt->lock);
 
 	return vgpu;
-- 
2.7.4

[PATCH v9 3/7] drm: Extend the drm format

[Xiaoguang Chen]

Add new drm format which will be used by GVT-g.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 include/uapi/drm/drm_fourcc.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 55e3010..2681862 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -113,6 +113,10 @@ extern "C" {
 
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
 
+/* 64 bpp RGB */
+#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
+#define DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */
+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has
-- 
2.7.4

Re: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format

[Ville Syrjälä]

On Thu, Jun 15, 2017 at 04:00:07PM +0800, Xiaoguang Chen wrote:
> Add new drm format which will be used by GVT-g.
> 
> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 55e3010..2681862 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -113,6 +113,10 @@ extern "C" {
>  
>  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
>  
> +/* 64 bpp RGB */
> +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
> +#define DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */

Are these supposed to be the half float formats? If so the docs are
lacking. Also this sort of stuff must be sent to dri-devel for everyone
to see.

That said, I don't really like having them in any gvt code until
they're handled by the driver proper.

> +
>  /*
>   * 2 plane RGB + A
>   * index 0 = RGB plane, same format as the corresponding non _A8 format has
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

RE: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format

[Zhang, Tina]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: Thursday, June 15, 2017 6:22 PM
> To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> kraxel@redhat.com; intel-gvt-dev@lists.freedesktop.org; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format
> 
> On Thu, Jun 15, 2017 at 04:00:07PM +0800, Xiaoguang Chen wrote:
> > Add new drm format which will be used by GVT-g.
> >
> > Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> > b/include/uapi/drm/drm_fourcc.h index 55e3010..2681862 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -113,6 +113,10 @@ extern "C" {
> >
> >  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0]
> A:Y:Cb:Cr 8:8:8:8 little endian */
> >
> > +/* 64 bpp RGB */
> > +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
> > +[63:0] x:R:G:B 16:16:16:16 little endian */ #define
> > +DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
> > +x:B:G:R 16:16:16:16 little endian */
> 
> Are these supposed to be the half float formats? If so the docs are lacking. Also
> this sort of stuff must be sent to dri-devel for everyone to see.
> 
> That said, I don't really like having them in any gvt code until they're handled by
> the driver proper.
This is needed by some Apps running on Windows VM. These can be separated to another patch set where more can be included, e.g. docs.
Thanks.

> 
> > +
> >  /*
> >   * 2 plane RGB + A
> >   * index 0 = RGB plane, same format as the corresponding non _A8
> > format has
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v9 4/7] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Xiaoguang Chen]

decode frambuffer attributes of primary, cursor and sprite plane

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile     |   3 +-
 drivers/gpu/drm/i915/gvt/display.c    |   2 +-
 drivers/gpu/drm/i915/gvt/display.h    |   2 +
 drivers/gpu/drm/i915/gvt/fb_decoder.c | 425 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h | 171 ++++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h        |   1 +
 6 files changed, 602 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index b123c20..192ca26 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -1,7 +1,8 @@
 GVT_DIR := gvt
 GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
-	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
+	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
+	fb_decoder.o
 
 ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index e0261fc..f5f63c5 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
 	return 1;
 }
 
-static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
 
diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
index d73de22..b46b868 100644
--- a/drivers/gpu/drm/i915/gvt/display.h
+++ b/drivers/gpu/drm/i915/gvt/display.h
@@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution);
 void intel_vgpu_reset_display(struct intel_vgpu *vgpu);
 void intel_vgpu_clean_display(struct intel_vgpu *vgpu);
 
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
+
 #endif
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c
new file mode 100644
index 0000000..a4614b5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
@@ -0,0 +1,425 @@
+/*
+ * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Authors:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *
+ */
+
+#include <uapi/drm/drm_fourcc.h>
+#include "i915_drv.h"
+#include "gvt.h"
+
+#define PRIMARY_FORMAT_NUM	16
+struct pixel_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	int	bpp;		/* Bits per pixel, 0 indicates invalid */
+	char *desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = {
+	[0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	[0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	[0x6] = {DRM_FORMAT_XRGB8888, 32,
+		"32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	[0x8] = {DRM_FORMAT_XBGR2101010, 32,
+		"32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	[0xa] = {DRM_FORMAT_XRGB2101010, 32,
+		"32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+	[0xc] = {DRM_FORMAT_XRGB161616, 64,
+		"64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"},
+	[0xe] = {DRM_FORMAT_XBGR8888, 32,
+		"32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format skl_pixel_formats[] = {
+	{DRM_FORMAT_YUYV, 16, "16-bit packed YUYV (8:8:8:8 MSB-V:Y2:U:Y1)"},
+	{DRM_FORMAT_UYVY, 16, "16-bit packed UYVY (8:8:8:8 MSB-Y2:V:Y1:U)"},
+	{DRM_FORMAT_YVYU, 16, "16-bit packed YVYU (8:8:8:8 MSB-U:Y2:V:Y1)"},
+	{DRM_FORMAT_VYUY, 16, "16-bit packed VYUY (8:8:8:8 MSB-Y2:U:Y1:V)"},
+
+	{DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	{DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	{DRM_FORMAT_ABGR8888, 32, "32-bit RGBA (8:8:8:8 MSB-A:B:G:R)"},
+	{DRM_FORMAT_XBGR8888, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+
+	{DRM_FORMAT_ARGB8888, 32, "32-bit BGRA (8:8:8:8 MSB-A:R:G:B)"},
+	{DRM_FORMAT_XRGB8888, 32, "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	{DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+
+	{DRM_FORMAT_XRGB161616, 64,
+		"64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR161616, 64,
+		"64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"},
+
+	/* non-supported format has bpp default to 0 */
+	{0, 0, NULL},
+};
+
+static int skl_format_to_drm(int format, bool rgb_order, bool alpha,
+	int yuv_order)
+{
+	int skl_pixel_formats_index = 14;
+
+	switch (format) {
+	case PLANE_CTL_FORMAT_INDEXED:
+		skl_pixel_formats_index = 4;
+		break;
+	case PLANE_CTL_FORMAT_RGB_565:
+		skl_pixel_formats_index = 5;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_8888:
+		if (rgb_order)
+			skl_pixel_formats_index = alpha ? 6 : 7;
+		else
+			skl_pixel_formats_index = alpha ? 8 : 9;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_2101010:
+		skl_pixel_formats_index = rgb_order ? 10 : 11;
+		break;
+
+	case PLANE_CTL_FORMAT_XRGB_16161616F:
+		skl_pixel_formats_index = rgb_order ? 12 : 13;
+		break;
+
+	case PLANE_CTL_FORMAT_YUV422:
+		skl_pixel_formats_index = yuv_order >> 16;
+		if (skl_pixel_formats_index > 3)
+			return -EINVAL;
+		break;
+
+	default:
+		break;
+	}
+
+	return skl_pixel_formats_index;
+}
+
+static u32 intel_vgpu_get_stride(struct intel_vgpu *vgpu, int pipe,
+	u32 tiled, int stride_mask, int bpp)
+{
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+
+	u32 stride_reg = vgpu_vreg(vgpu, DSPSTRIDE(pipe)) & stride_mask;
+	u32 stride = stride_reg;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		switch (tiled) {
+		case PLANE_CTL_TILED_LINEAR:
+			stride = stride_reg * 64;
+			break;
+		case PLANE_CTL_TILED_X:
+			stride = stride_reg * 512;
+			break;
+		case PLANE_CTL_TILED_Y:
+			stride = stride_reg * 128;
+			break;
+		case PLANE_CTL_TILED_YF:
+			if (bpp == 8)
+				stride = stride_reg * 64;
+			else if (bpp == 16 || bpp == 32 || bpp == 64)
+				stride = stride_reg * 128;
+			else
+				gvt_dbg_core("skl: unsupported bpp:%d\n", bpp);
+			break;
+		default:
+			gvt_dbg_core("skl: unsupported tile format:%x\n",
+				tiled);
+		}
+	}
+
+	return stride;
+}
+
+static int get_active_pipe(struct intel_vgpu *vgpu)
+{
+	int i;
+
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		if (pipe_is_enabled(vgpu, i))
+			break;
+
+	return i;
+}
+
+/**
+ * intel_vgpu_decode_primary_plane - Decode primary plane
+ * @vgpu: input vgpu
+ * @plane: primary plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_primary_plane_format *plane)
+{
+	u32 val, fmt;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, DSPCNTR(pipe));
+	plane->enabled = !!(val & DISPLAY_PLANE_ENABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		plane->tiled = (val & PLANE_CTL_TILED_MASK) >>
+		_PLANE_CTL_TILED_SHIFT;
+		fmt = skl_format_to_drm(
+			val & PLANE_CTL_FORMAT_MASK,
+			val & PLANE_CTL_ORDER_RGBX,
+			val & PLANE_CTL_ALPHA_MASK,
+			val & PLANE_CTL_YUV422_ORDER_MASK);
+		plane->bpp = skl_pixel_formats[fmt].bpp;
+		plane->drm_format = skl_pixel_formats[fmt].drm_format;
+	} else {
+		plane->tiled = !!(val & DISPPLANE_TILED);
+		fmt = (val & DISPPLANE_PIXFORMAT_MASK) >> _PRI_PLANE_FMT_SHIFT;
+		plane->bpp = bdw_pixel_formats[fmt].bpp;
+		plane->drm_format = bdw_pixel_formats[fmt].drm_format;
+	}
+
+	if (!plane->bpp) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+
+	plane->hw_format = fmt;
+
+	plane->base = vgpu_vreg(vgpu, DSPSURF(pipe)) & GTT_PAGE_MASK;
+
+	plane->stride = intel_vgpu_get_stride(vgpu, pipe, (plane->tiled << 10),
+		(IS_SKYLAKE(dev_priv)) ? (_PRI_PLANE_STRIDE_MASK >> 6) :
+		_PRI_PLANE_STRIDE_MASK, plane->bpp);
+
+	plane->width = (vgpu_vreg(vgpu, PIPESRC(pipe)) & _PIPE_H_SRCSZ_MASK) >>
+		_PIPE_H_SRCSZ_SHIFT;
+	plane->width += 1;
+	plane->height = (vgpu_vreg(vgpu, PIPESRC(pipe)) &
+			_PIPE_V_SRCSZ_MASK) >> _PIPE_V_SRCSZ_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+
+	val = vgpu_vreg(vgpu, DSPTILEOFF(pipe));
+	plane->x_offset = (val & _PRI_PLANE_X_OFF_MASK) >>
+		_PRI_PLANE_X_OFF_SHIFT;
+	plane->y_offset = (val & _PRI_PLANE_Y_OFF_MASK) >>
+		_PRI_PLANE_Y_OFF_SHIFT;
+
+	return 0;
+}
+
+#define CURSOR_FORMAT_NUM	(1 << 6)
+struct cursor_mode_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	u8	bpp;		/* Bits per pixel; 0 indicates invalid */
+	u32	width;		/* In pixel */
+	u32	height;		/* In lines */
+	char	*desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct cursor_mode_format cursor_pixel_formats[CURSOR_FORMAT_NUM] = {
+	[0x22]  = {DRM_FORMAT_ARGB8888, 32, 128, 128, "128x128 32bpp ARGB"},
+	[0x23]  = {DRM_FORMAT_ARGB8888, 32, 256, 256, "256x256 32bpp ARGB"},
+	[0x27]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+	[0x7]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+};
+
+/**
+ * intel_vgpu_decode_cursor_plane - Decode sprite plane
+ * @vgpu: input vgpu
+ * @plane: cursor plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_cursor_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_cursor_plane_format *plane)
+{
+	u32 val, mode;
+	u32 alpha_plane, alpha_force;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, CURCNTR(pipe));
+	mode = val & CURSOR_MODE;
+	plane->enabled = (mode != CURSOR_MODE_DISABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	if (!cursor_pixel_formats[mode].bpp) {
+		gvt_vgpu_err("Non-supported cursor mode (0x%x)\n", mode);
+		return -EINVAL;
+	}
+	plane->mode = mode;
+	plane->bpp = cursor_pixel_formats[mode].bpp;
+	plane->drm_format = cursor_pixel_formats[mode].drm_format;
+	plane->width = cursor_pixel_formats[mode].width;
+	plane->height = cursor_pixel_formats[mode].height;
+
+	alpha_plane = (val & _CURSOR_ALPHA_PLANE_MASK) >>
+				_CURSOR_ALPHA_PLANE_SHIFT;
+	alpha_force = (val & _CURSOR_ALPHA_FORCE_MASK) >>
+				_CURSOR_ALPHA_FORCE_SHIFT;
+	if (alpha_plane || alpha_force)
+		gvt_dbg_core("alpha_plane=0x%x, alpha_force=0x%x\n",
+			alpha_plane, alpha_force);
+
+	plane->base = vgpu_vreg(vgpu, CURBASE(pipe)) & GTT_PAGE_MASK;
+
+	val = vgpu_vreg(vgpu, CURPOS(pipe));
+	plane->x_pos = (val & _CURSOR_POS_X_MASK) >> _CURSOR_POS_X_SHIFT;
+	plane->x_sign = (val & _CURSOR_SIGN_X_MASK) >> _CURSOR_SIGN_X_SHIFT;
+	plane->y_pos = (val & _CURSOR_POS_Y_MASK) >> _CURSOR_POS_Y_SHIFT;
+	plane->y_sign = (val & _CURSOR_SIGN_Y_MASK) >> _CURSOR_SIGN_Y_SHIFT;
+
+	return 0;
+}
+
+#define SPRITE_FORMAT_NUM	(1 << 3)
+
+static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = {
+	[0x0]  = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"},
+	[0x1]  = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"},
+	[0x2]  = {DRM_FORMAT_XRGB8888, 32, "RGB 32-bit 8:8:8:8"},
+	[0x3]  = {DRM_FORMAT_XRGB161616, 64,
+		    "RGB 64-bit 16:16:16:16 Floating Point"},
+	[0x4] = {DRM_FORMAT_AYUV, 32,
+		"YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"},
+};
+
+/**
+ * intel_vgpu_decode_sprite_plane - Decode sprite plane
+ * @vgpu: input vgpu
+ * @plane: sprite plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_sprite_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_sprite_plane_format *plane)
+{
+	u32 val, fmt;
+	u32 color_order, yuv_order;
+	int drm_format;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, SPRCTL(pipe));
+	plane->enabled = !!(val & SPRITE_ENABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	plane->tiled = !!(val & SPRITE_TILED);
+	color_order = !!(val & SPRITE_RGB_ORDER_RGBX);
+	yuv_order = (val & SPRITE_YUV_BYTE_ORDER_MASK) >>
+				_SPRITE_YUV_ORDER_SHIFT;
+
+	fmt = (val & SPRITE_PIXFORMAT_MASK) >> _SPRITE_FMT_SHIFT;
+	if (!sprite_pixel_formats[fmt].bpp) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+	plane->hw_format = fmt;
+	plane->bpp = sprite_pixel_formats[fmt].bpp;
+	drm_format = sprite_pixel_formats[fmt].drm_format;
+
+	/* Order of RGB values in an RGBxxx buffer may be ordered RGB or
+	 * BGR depending on the state of the color_order field
+	 */
+	if (!color_order) {
+		if (drm_format == DRM_FORMAT_XRGB2101010)
+			drm_format = DRM_FORMAT_XBGR2101010;
+		else if (drm_format == DRM_FORMAT_XRGB8888)
+			drm_format = DRM_FORMAT_XBGR8888;
+	}
+
+	if (drm_format == DRM_FORMAT_YUV422) {
+		switch (yuv_order) {
+		case 0:
+			drm_format = DRM_FORMAT_YUYV;
+			break;
+		case 1:
+			drm_format = DRM_FORMAT_UYVY;
+			break;
+		case 2:
+			drm_format = DRM_FORMAT_YVYU;
+			break;
+		case 3:
+			drm_format = DRM_FORMAT_VYUY;
+			break;
+		default:
+			/* yuv_order has only 2 bits */
+			break;
+		}
+	}
+
+	plane->drm_format = drm_format;
+
+	plane->base = vgpu_vreg(vgpu, SPRSURF(pipe)) & GTT_PAGE_MASK;
+	plane->stride = vgpu_vreg(vgpu, SPRSTRIDE(pipe)) &
+				_SPRITE_STRIDE_MASK;
+
+	val = vgpu_vreg(vgpu, SPRSIZE(pipe));
+	plane->height = (val & _SPRITE_SIZE_HEIGHT_MASK) >>
+		_SPRITE_SIZE_HEIGHT_SHIFT;
+	plane->width = (val & _SPRITE_SIZE_WIDTH_MASK) >>
+		_SPRITE_SIZE_WIDTH_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+	plane->width += 1;	/* raw width is one minus the real value */
+
+	val = vgpu_vreg(vgpu, SPRPOS(pipe));
+	plane->x_pos = (val & _SPRITE_POS_X_MASK) >> _SPRITE_POS_X_SHIFT;
+	plane->y_pos = (val & _SPRITE_POS_Y_MASK) >> _SPRITE_POS_Y_SHIFT;
+
+	val = vgpu_vreg(vgpu, SPROFFSET(pipe));
+	plane->x_offset = (val & _SPRITE_OFFSET_START_X_MASK) >>
+			   _SPRITE_OFFSET_START_X_SHIFT;
+	plane->y_offset = (val & _SPRITE_OFFSET_START_Y_MASK) >>
+			   _SPRITE_OFFSET_START_Y_SHIFT;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.h b/drivers/gpu/drm/i915/gvt/fb_decoder.h
new file mode 100644
index 0000000..08d4d8a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.h
@@ -0,0 +1,171 @@
+/*
+ * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Authors:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *
+ */
+
+#ifndef _GVT_FB_DECODER_H_
+#define _GVT_FB_DECODER_H_
+
+#define _PLANE_CTL_FORMAT_SHIFT		24
+#define _PLANE_CTL_TILED_SHIFT		10
+#define _PIPE_V_SRCSZ_SHIFT		0
+#define _PIPE_V_SRCSZ_MASK		(0xfff << _PIPE_V_SRCSZ_SHIFT)
+#define _PIPE_H_SRCSZ_SHIFT		16
+#define _PIPE_H_SRCSZ_MASK		(0x1fff << _PIPE_H_SRCSZ_SHIFT)
+
+#define _PRI_PLANE_FMT_SHIFT		26
+#define _PRI_PLANE_STRIDE_MASK		(0x3ff << 6)
+#define _PRI_PLANE_X_OFF_SHIFT		0
+#define _PRI_PLANE_X_OFF_MASK		(0x1fff << _PRI_PLANE_X_OFF_SHIFT)
+#define _PRI_PLANE_Y_OFF_SHIFT		16
+#define _PRI_PLANE_Y_OFF_MASK		(0xfff << _PRI_PLANE_Y_OFF_SHIFT)
+
+#define _CURSOR_MODE			0x3f
+#define _CURSOR_ALPHA_FORCE_SHIFT	8
+#define _CURSOR_ALPHA_FORCE_MASK	(0x3 << _CURSOR_ALPHA_FORCE_SHIFT)
+#define _CURSOR_ALPHA_PLANE_SHIFT	10
+#define _CURSOR_ALPHA_PLANE_MASK	(0x3 << _CURSOR_ALPHA_PLANE_SHIFT)
+#define _CURSOR_POS_X_SHIFT		0
+#define _CURSOR_POS_X_MASK		(0x1fff << _CURSOR_POS_X_SHIFT)
+#define _CURSOR_SIGN_X_SHIFT		15
+#define _CURSOR_SIGN_X_MASK		(1 << _CURSOR_SIGN_X_SHIFT)
+#define _CURSOR_POS_Y_SHIFT		16
+#define _CURSOR_POS_Y_MASK		(0xfff << _CURSOR_POS_Y_SHIFT)
+#define _CURSOR_SIGN_Y_SHIFT		31
+#define _CURSOR_SIGN_Y_MASK		(1 << _CURSOR_SIGN_Y_SHIFT)
+
+#define _SPRITE_FMT_SHIFT		25
+#define _SPRITE_COLOR_ORDER_SHIFT	20
+#define _SPRITE_YUV_ORDER_SHIFT		16
+#define _SPRITE_STRIDE_SHIFT		6
+#define _SPRITE_STRIDE_MASK		(0x1ff << _SPRITE_STRIDE_SHIFT)
+#define _SPRITE_SIZE_WIDTH_SHIFT	0
+#define _SPRITE_SIZE_HEIGHT_SHIFT	16
+#define _SPRITE_SIZE_WIDTH_MASK		(0x1fff << _SPRITE_SIZE_WIDTH_SHIFT)
+#define _SPRITE_SIZE_HEIGHT_MASK	(0xfff << _SPRITE_SIZE_HEIGHT_SHIFT)
+#define _SPRITE_POS_X_SHIFT		0
+#define _SPRITE_POS_Y_SHIFT		16
+#define _SPRITE_POS_X_MASK		(0x1fff << _SPRITE_POS_X_SHIFT)
+#define _SPRITE_POS_Y_MASK		(0xfff << _SPRITE_POS_Y_SHIFT)
+#define _SPRITE_OFFSET_START_X_SHIFT	0
+#define _SPRITE_OFFSET_START_Y_SHIFT	16
+#define _SPRITE_OFFSET_START_X_MASK	(0x1fff << _SPRITE_OFFSET_START_X_SHIFT)
+#define _SPRITE_OFFSET_START_Y_MASK	(0xfff << _SPRITE_OFFSET_START_Y_SHIFT)
+
+enum GVT_FB_EVENT {
+	FB_MODE_SET_START = 1,
+	FB_MODE_SET_END,
+	FB_DISPLAY_FLIP,
+};
+
+enum DDI_PORT {
+	DDI_PORT_NONE	= 0,
+	DDI_PORT_B	= 1,
+	DDI_PORT_C	= 2,
+	DDI_PORT_D	= 3,
+	DDI_PORT_E	= 4
+};
+
+struct intel_gvt;
+
+struct gvt_fb_notify_msg {
+	unsigned int vm_id;
+	unsigned int pipe_id; /* id starting from 0 */
+	unsigned int plane_id; /* primary, cursor, or sprite */
+};
+
+/* color space conversion and gamma correction are not included */
+struct intel_vgpu_primary_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the PRI_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* framebuffer base in graphics memory */
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	stride;		/* in bytes */
+};
+
+struct intel_vgpu_sprite_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the SPR_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* sprite base in graphics memory */
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	stride;		/* in bytes */
+};
+
+struct intel_vgpu_cursor_plane_format {
+	u8	enabled;
+	u8	mode;		/* cursor mode select */
+	u8	bpp;		/* bits per pixel */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* cursor base in graphics memory */
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u8	x_sign;		/* X Position Sign */
+	u8	y_sign;		/* Y Position Sign */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	x_hot;		/* in pixels */
+	u32	y_hot;		/* in pixels */
+};
+
+struct intel_vgpu_pipe_format {
+	struct intel_vgpu_primary_plane_format	primary;
+	struct intel_vgpu_sprite_plane_format	sprite;
+	struct intel_vgpu_cursor_plane_format	cursor;
+	enum DDI_PORT ddi_port;  /* the DDI port that pipe is connected to */
+};
+
+struct intel_vgpu_fb_format {
+	struct intel_vgpu_pipe_format	pipes[I915_MAX_PIPES];
+};
+
+int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_primary_plane_format *plane);
+int intel_vgpu_decode_cursor_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_cursor_plane_format *plane);
+int intel_vgpu_decode_sprite_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_sprite_plane_format *plane);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 930732e..c42266c 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -46,6 +46,7 @@
 #include "sched_policy.h"
 #include "render.h"
 #include "cmd_parser.h"
+#include "fb_decoder.h"
 
 #define GVT_MAX_VGPU 8
 
-- 
2.7.4

[PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Xiaoguang Chen]

Here we defined a new ioctl to create a fd for a vfio device based on
the input type. Now only one type is supported that is a dma-buf
management fd.
Two ioctls are defined for the dma-buf management fd: query the vfio
vgpu's plane information and create a dma-buf for a plane.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 include/uapi/linux/vfio.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..7d86101 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,63 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
+ *
+ * Create a fd for a vfio device based on the input type
+ * Vendor driver should handle this ioctl to create a fd and manage the
+ * life cycle of this fd.
+ *
+ * Return: a fd if vendor support that type, -errno if not supported
+ */
+
+#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
+
+#define VFIO_DEVICE_DMABUF_MGR_FD	0 /* Supported fd types */
+
+struct vfio_dmabuf_mgr_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;
+	__u32 padding;
+};
+
+/*
+ * VFIO_DMABUF_MGR_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
+ *					struct vfio_dmabuf_mgr_query_plane)
+ * Query plane information
+ */
+struct vfio_dmabuf_mgr_query_plane {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+};
+
+#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+
+/*
+ * VFIO_DMABUF_MGR_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
+ *					struct vfio_dmabuf_mgr_create_dmabuf)
+ *
+ * Create a dma-buf for a plane
+ */
+struct vfio_dmabuf_mgr_create_dmabuf {
+	__u32 argsz;
+	__u32 flags;
+	struct vfio_dmabuf_mgr_plane_info plane_info;
+	__u32 plane_id;
+	__s32 fd;
+};
+
+#define VFIO_DMABUF_MGR_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.7.4

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/15/2017 1:30 PM, Xiaoguang Chen wrote:
> Here we defined a new ioctl to create a fd for a vfio device based on
> the input type. Now only one type is supported that is a dma-buf
> management fd.
> Two ioctls are defined for the dma-buf management fd: query the vfio
> vgpu's plane information and create a dma-buf for a plane.
> 

I had suggested how we can use common structures for both type of ways
to query surface on v6 version of your patch,
https://lkml.org/lkml/2017/6/1/890


> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> ---
>  include/uapi/linux/vfio.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..7d86101 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,63 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
> + *
> + * Create a fd for a vfio device based on the input type
> + * Vendor driver should handle this ioctl to create a fd and manage the
> + * life cycle of this fd.
> + *
> + * Return: a fd if vendor support that type, -errno if not supported
> + */
> +
> +#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
> +
> +#define VFIO_DEVICE_DMABUF_MGR_FD	0 /* Supported fd types */
> +
> +struct vfio_dmabuf_mgr_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;
> +	__u32 padding;
> +};
> +

This structure is generic, can remove dmabuf from its name,
vfio_plane_info or vfio_vgpu_surface_info since this will only be used
by vgpu.

> +/*
> + * VFIO_DMABUF_MGR_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
> + *					struct vfio_dmabuf_mgr_query_plane)
> + * Query plane information
> + */
> +struct vfio_dmabuf_mgr_query_plane {
> +	__u32 argsz;
> +	__u32 flags;
> +	struct vfio_dmabuf_mgr_plane_info plane_info;
> +	__u32 plane_id;
> +};
> +
> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> +

This same interface can be used to query surface/plane information for
both, dmabuf and region, case. Here also 'DMABUF' can be removed and
define flags if you want to differentiate query for 'dmabuf' and 'region'.

Thanks,
Kirti

> +/*
> + * VFIO_DMABUF_MGR_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
> + *					struct vfio_dmabuf_mgr_create_dmabuf)
> + *
> + * Create a dma-buf for a plane
> + */
> +struct vfio_dmabuf_mgr_create_dmabuf {
> +	__u32 argsz;
> +	__u32 flags;
> +	struct vfio_dmabuf_mgr_plane_info plane_info;
> +	__u32 plane_id;
> +	__s32 fd;
> +};
> +
> +#define VFIO_DMABUF_MGR_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  /**
> 

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > +struct vfio_dmabuf_mgr_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;
> > +	__u32 padding;
> > +};
> > +
> 
> This structure is generic, can remove dmabuf from its name,
> vfio_plane_info or vfio_vgpu_surface_info since this will only be
> used
> by vgpu.

Agree.

> > +struct vfio_dmabuf_mgr_query_plane {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	struct vfio_dmabuf_mgr_plane_info plane_info;
> > +	__u32 plane_id;
> > +};
> > +
> > +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> > +
> 
> This same interface can be used to query surface/plane information
> for
> both, dmabuf and region, case. Here also 'DMABUF' can be removed and
> define flags if you want to differentiate query for 'dmabuf' and
> 'region'.

Hmm, any specific reason why you want use a ioctl for that?  I would
simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
final name will be) at the start of the region.

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Thu, 15 Jun 2017 18:00:38 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > +struct vfio_dmabuf_mgr_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;
> > > +	__u32 padding;
> > > +};
> > > +  
> > 
> > This structure is generic, can remove dmabuf from its name,
> > vfio_plane_info or vfio_vgpu_surface_info since this will only be
> > used
> > by vgpu.  
> 
> Agree.

I'm not sure I agree regarding the vgpu statement, maybe this is not
dmabuf specific, but what makes it vgpu specific?  We need to separate
our current usage plans from what it's actually describing and I don't
see that it describes anything vgpu specific.
 
> > > +struct vfio_dmabuf_mgr_query_plane {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +	struct vfio_dmabuf_mgr_plane_info plane_info;
> > > +	__u32 plane_id;
> > > +};
> > > +
> > > +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> > > +  
> > 
> > This same interface can be used to query surface/plane information
> > for
> > both, dmabuf and region, case. Here also 'DMABUF' can be removed and
> > define flags if you want to differentiate query for 'dmabuf' and
> > 'region'.  
> 
> Hmm, any specific reason why you want use a ioctl for that?  I would
> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
> final name will be) at the start of the region.

Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd,
if you're exposing a region with the info I wouldn't think you'd want
the hassle of managing this separate fd when you could do something
like Gerd suggests with defining the first page of the regions as
containing the structure.  Maybe you could even allow mmap of that page
to reduce the overhead of getting the current state.  For the sake of
userspace, I'd hope we'd still use the same structure for either the
ioctl or region mapping.  I'm not really in favor of declaring that
this particular ioctl might exist on the device fd when such-and-such
region is present otherwise it might exist on a dmabuf manager fd.
Thanks,

Alex

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> I'm not sure I agree regarding the vgpu statement, maybe this is not
> dmabuf specific, but what makes it vgpu specific?  We need to
> separate
> our current usage plans from what it's actually describing and I
> don't
> see that it describes anything vgpu specific.

Well, it describes a framebuffer, what non-graphic device would need
that?

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Fri, 16 Jun 2017 12:24:42 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > I'm not sure I agree regarding the vgpu statement, maybe this is not
> > dmabuf specific, but what makes it vgpu specific?  We need to
> > separate
> > our current usage plans from what it's actually describing and I
> > don't
> > see that it describes anything vgpu specific.  
> 
> Well, it describes a framebuffer, what non-graphic device would need
> that?

Graphics is not necessarily vgpu though, which is my point.  It should
not be named after our intended use case (vgpu), it should be named
after what it's describing (a framebuffer, or graphics plane).  Thanks,

Alex

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/16/2017 2:08 AM, Alex Williamson wrote:
> On Thu, 15 Jun 2017 18:00:38 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>>   Hi,
>>
>>>> +struct vfio_dmabuf_mgr_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;
>>>> +	__u32 padding;
>>>> +};
>>>> +  
>>>
>>> This structure is generic, can remove dmabuf from its name,
>>> vfio_plane_info or vfio_vgpu_surface_info since this will only be
>>> used
>>> by vgpu.  
>>
>> Agree.
> 
> I'm not sure I agree regarding the vgpu statement, maybe this is not
> dmabuf specific, but what makes it vgpu specific?  We need to separate
> our current usage plans from what it's actually describing and I don't
> see that it describes anything vgpu specific.
>  
>>>> +struct vfio_dmabuf_mgr_query_plane {
>>>> +	__u32 argsz;
>>>> +	__u32 flags;
>>>> +	struct vfio_dmabuf_mgr_plane_info plane_info;
>>>> +	__u32 plane_id;
>>>> +};
>>>> +
>>>> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>>>> +  
>>>
>>> This same interface can be used to query surface/plane information
>>> for
>>> both, dmabuf and region, case. Here also 'DMABUF' can be removed and
>>> define flags if you want to differentiate query for 'dmabuf' and
>>> 'region'.  
>>
>> Hmm, any specific reason why you want use a ioctl for that?  I would
>> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
>> final name will be) at the start of the region.
> 
> Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd,
> if you're exposing a region with the info I wouldn't think you'd want
> the hassle of managing this separate fd when you could do something
> like Gerd suggests with defining the first page of the regions as
> containing the structure.

My suggestion was to use vfio device fd for this ioctl and have dmabuf
mgr fd as member in above query_plane structure, for region type it
would be set to 0.
Yes there is other way to query surface information as Gerd suggested,
but my point is: if a ioctl is being add, it could be used for both
types, dmabuf and region.

>  Maybe you could even allow mmap of that page
> to reduce the overhead of getting the current state.  

Can't mmap that page to get surface information. There is no way to
synchronize between QEMU reading this mmapped region and vendor driver
writing it. There could be race condition in these two operations. Read
on this page should be trapped and blocking, so that surface in that
region is only updated when its asked for.

> For the sake of
> userspace, I'd hope we'd still use the same structure for either the
> ioctl or region mapping.  I'm not really in favor of declaring that
> this particular ioctl might exist on the device fd when such-and-such
> region is present otherwise it might exist on a dmabuf manager fd.

Userspace will always use vfio device fd for this ioctl, it only have to
set proper arguments to its structure based on type.

Thanks,
Kirti

> Thanks,
> 
> Alex
> 

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Fri, 16 Jun 2017 19:02:30 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/16/2017 2:08 AM, Alex Williamson wrote:
> > On Thu, 15 Jun 2017 18:00:38 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> >>   Hi,
> >>  
> >>>> +struct vfio_dmabuf_mgr_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;
> >>>> +	__u32 padding;
> >>>> +};
> >>>> +    
> >>>
> >>> This structure is generic, can remove dmabuf from its name,
> >>> vfio_plane_info or vfio_vgpu_surface_info since this will only be
> >>> used
> >>> by vgpu.    
> >>
> >> Agree.  
> > 
> > I'm not sure I agree regarding the vgpu statement, maybe this is not
> > dmabuf specific, but what makes it vgpu specific?  We need to separate
> > our current usage plans from what it's actually describing and I don't
> > see that it describes anything vgpu specific.
> >    
> >>>> +struct vfio_dmabuf_mgr_query_plane {
> >>>> +	__u32 argsz;
> >>>> +	__u32 flags;
> >>>> +	struct vfio_dmabuf_mgr_plane_info plane_info;
> >>>> +	__u32 plane_id;
> >>>> +};
> >>>> +
> >>>> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> >>>> +    
> >>>
> >>> This same interface can be used to query surface/plane information
> >>> for
> >>> both, dmabuf and region, case. Here also 'DMABUF' can be removed and
> >>> define flags if you want to differentiate query for 'dmabuf' and
> >>> 'region'.    
> >>
> >> Hmm, any specific reason why you want use a ioctl for that?  I would
> >> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
> >> final name will be) at the start of the region.  
> > 
> > Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd,
> > if you're exposing a region with the info I wouldn't think you'd want
> > the hassle of managing this separate fd when you could do something
> > like Gerd suggests with defining the first page of the regions as
> > containing the structure.  
> 
> My suggestion was to use vfio device fd for this ioctl and have dmabuf
> mgr fd as member in above query_plane structure, for region type it
> would be set to 0.
> Yes there is other way to query surface information as Gerd suggested,
> but my point is: if a ioctl is being add, it could be used for both
> types, dmabuf and region.

I think this suggests abandoning the dmabuf manager fd entirely.  That's
not necessarily a bad thing, but I don't think the idea of the dmabuf
manager fd stands if we push one of its primary reasons for existing
back to the device fd.  Reading though previous posts, I think we
embraced the dmabuf manager as a separate fd primarily for
consolidation and the potential to use it as a notification point, the
latter being only theoretically useful.

So perhaps this becomes:

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;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};

The flag defines the data in the id field as either referring to a
region (perhaps there could be multiple regions with only one active)
or a plane ID, which is acquired separately, such as via a dmabuf fd.
This would be retrieved via an optional VFIO_DEVICE_QUERY_GFX_PLANE
ioctl on the vfio device, implemented in the vendor driver.

Would the above, along with the already defined mechanism for defining
device specific regions, account for NVIDIA's needs?

For dmabuf users, we'd still need a new ioctl to get the dmabuf fd.  We
could either create a specific ioctl for this (ex.
VFIO_DEVICE_GET_DMABUF_FD) or we could create a shared, generic GET_FD
interface on the device.

> >  Maybe you could even allow mmap of that page
> > to reduce the overhead of getting the current state.    
> 
> Can't mmap that page to get surface information. There is no way to
> synchronize between QEMU reading this mmapped region and vendor driver
> writing it. There could be race condition in these two operations. Read
> on this page should be trapped and blocking, so that surface in that
> region is only updated when its asked for.
> 
> > For the sake of
> > userspace, I'd hope we'd still use the same structure for either the
> > ioctl or region mapping.  I'm not really in favor of declaring that
> > this particular ioctl might exist on the device fd when such-and-such
> > region is present otherwise it might exist on a dmabuf manager fd.  
> 
> Userspace will always use vfio device fd for this ioctl, it only have to
> set proper arguments to its structure based on type.

Then we should kill off the manager fd unless there are arguments that
still give it value.  Thanks,

Alex

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/16/2017 10:09 PM, Alex Williamson wrote:
> On Fri, 16 Jun 2017 19:02:30 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/16/2017 2:08 AM, Alex Williamson wrote:
>>> On Thu, 15 Jun 2017 18:00:38 +0200
>>> Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>   
>>>>   Hi,
>>>>  
>>>>>> +struct vfio_dmabuf_mgr_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;
>>>>>> +	__u32 padding;
>>>>>> +};
>>>>>> +    
>>>>>
>>>>> This structure is generic, can remove dmabuf from its name,
>>>>> vfio_plane_info or vfio_vgpu_surface_info since this will only be
>>>>> used
>>>>> by vgpu.    
>>>>
>>>> Agree.  
>>>
>>> I'm not sure I agree regarding the vgpu statement, maybe this is not
>>> dmabuf specific, but what makes it vgpu specific?  We need to separate
>>> our current usage plans from what it's actually describing and I don't
>>> see that it describes anything vgpu specific.
>>>    
>>>>>> +struct vfio_dmabuf_mgr_query_plane {
>>>>>> +	__u32 argsz;
>>>>>> +	__u32 flags;
>>>>>> +	struct vfio_dmabuf_mgr_plane_info plane_info;
>>>>>> +	__u32 plane_id;
>>>>>> +};
>>>>>> +
>>>>>> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>>>>>> +    
>>>>>
>>>>> This same interface can be used to query surface/plane information
>>>>> for
>>>>> both, dmabuf and region, case. Here also 'DMABUF' can be removed and
>>>>> define flags if you want to differentiate query for 'dmabuf' and
>>>>> 'region'.    
>>>>
>>>> Hmm, any specific reason why you want use a ioctl for that?  I would
>>>> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
>>>> final name will be) at the start of the region.  
>>>
>>> Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd,
>>> if you're exposing a region with the info I wouldn't think you'd want
>>> the hassle of managing this separate fd when you could do something
>>> like Gerd suggests with defining the first page of the regions as
>>> containing the structure.  
>>
>> My suggestion was to use vfio device fd for this ioctl and have dmabuf
>> mgr fd as member in above query_plane structure, for region type it
>> would be set to 0.
>> Yes there is other way to query surface information as Gerd suggested,
>> but my point is: if a ioctl is being add, it could be used for both
>> types, dmabuf and region.
> 
> I think this suggests abandoning the dmabuf manager fd entirely.  That's
> not necessarily a bad thing, but I don't think the idea of the dmabuf
> manager fd stands if we push one of its primary reasons for existing
> back to the device fd.  Reading though previous posts, I think we
> embraced the dmabuf manager as a separate fd primarily for
> consolidation and the potential to use it as a notification point, the
> latter being only theoretically useful.
> 
> So perhaps this becomes:
> 
> 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;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> 	struct vfio_device_gfx_plane_info plane_info;
> 	__u32 id; 
> };
> 
> The flag defines the data in the id field as either referring to a
> region (perhaps there could be multiple regions with only one active)
> or a plane ID, which is acquired separately, such as via a dmabuf fd.
> This would be retrieved via an optional VFIO_DEVICE_QUERY_GFX_PLANE
> ioctl on the vfio device, implemented in the vendor driver.
> 
> Would the above, along with the already defined mechanism for defining
> device specific regions, account for NVIDIA's needs?
> 

Yes, works for region type solution that we would go with.

Thanks,
Kirti

> For dmabuf users, we'd still need a new ioctl to get the dmabuf fd.  We
> could either create a specific ioctl for this (ex.
> VFIO_DEVICE_GET_DMABUF_FD) or we could create a shared, generic GET_FD
> interface on the device.
> 
>>>  Maybe you could even allow mmap of that page
>>> to reduce the overhead of getting the current state.    
>>
>> Can't mmap that page to get surface information. There is no way to
>> synchronize between QEMU reading this mmapped region and vendor driver
>> writing it. There could be race condition in these two operations. Read
>> on this page should be trapped and blocking, so that surface in that
>> region is only updated when its asked for.
>>
>>> For the sake of
>>> userspace, I'd hope we'd still use the same structure for either the
>>> ioctl or region mapping.  I'm not really in favor of declaring that
>>> this particular ioctl might exist on the device fd when such-and-such
>>> region is present otherwise it might exist on a dmabuf manager fd.  
>>
>> Userspace will always use vfio device fd for this ioctl, it only have to
>> set proper arguments to its structure based on type.
> 
> Then we should kill off the manager fd unless there are arguments that
> still give it value.  Thanks,
> 
> Alex
> 

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> So perhaps this becomes:
> 
> 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;
> };

Looks good.

> struct vfio_device_query_gfx_plane {
> 	__u32 argsz;
> 	__u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> 	struct vfio_device_gfx_plane_info plane_info;
> 	__u32 id; 
> };

Hmm, plane isn't really an ID, it is a type, with type being either
DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think the
flage above make sense.

What are the nvidia plane for cursor support btw?

> The flag defines the data in the id field as either referring to a
> region (perhaps there could be multiple regions with only one active)

Well, we have a "start" field in vfio_device_gfx_plane_info (maybe we
should rename that to "offset"), which can be used to place multiple
planes into a single, fixed region.

Also I think it would be useful to have some way to figure the device
capabilities as the userspace workflow will look quite different for
the two cases.

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Mon, 19 Jun 2017 08:34:13 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > So perhaps this becomes:
> > 
> > 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;
> > };  
> 
> Looks good.
> 
> > struct vfio_device_query_gfx_plane {
> > 	__u32 argsz;
> > 	__u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > 	struct vfio_device_gfx_plane_info plane_info;
> > 	__u32 id; 
> > };  
> 
> Hmm, plane isn't really an ID, it is a type, with type being either
> DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think the
> flage above make sense.

The intention was that ..._REGION_ID and ...PLANE_ID are describing
what the vfio_device_query_gfx_plane.id field represents, either a
region index or a plane identifier.  The type of plane would be
represented within the vfio_device_gfx_plane_info struct.

> What are the nvidia plane for cursor support btw?
> 
> > The flag defines the data in the id field as either referring to a
> > region (perhaps there could be multiple regions with only one active)  
> 
> Well, we have a "start" field in vfio_device_gfx_plane_info (maybe we
> should rename that to "offset"), which can be used to place multiple
> planes into a single, fixed region.

That seems reasonable.
 
> Also I think it would be useful to have some way to figure the device
> capabilities as the userspace workflow will look quite different for
> the two cases.

In the region case, VFIO_DEVICE_GET_REGION_INFO would include a device
specific region with a hopefully common identifier to identify it as a
graphics framebuffer.  VFIO_DEVICE_QUERY_GFX_PLANE would indicate the
plane as a region index.

In the dmabuf case, VFIO_DEVICE_QUERY_GFX_PLANE would indicate the
plane as a "plane ID" and some sort of
VFIO_DEVICE_GET_GFX_PLANE(VFIO_GFX_TYPE_DMABUF) ioctl would be
necessary to get a file descriptor to that plane.

What else are you thinking we need?  Thanks,

Alex

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > Hmm, plane isn't really an ID, it is a type, with type being either
> > DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think
> > the
> > flage above make sense.
> 
> The intention was that ..._REGION_ID and ...PLANE_ID are describing
> what the vfio_device_query_gfx_plane.id field represents, either a
> region index or a plane identifier.  The type of plane would be
> represented within the vfio_device_gfx_plane_info struct.

The planes don't really have an id, we should rename that to
plane_type, or maybe drm_plane_type (simliar to the drm_format_*
fields), to avoid that confusion.

plane_type is set by userspace to specify what kind of plane it asks
for.

> > Also I think it would be useful to have some way to figure the
> > device
> > capabilities as the userspace workflow will look quite different
> > for
> > the two cases.
> 
> In the region case, VFIO_DEVICE_GET_REGION_INFO would include a
> device
> specific region with a hopefully common identifier to identify it as
> a
> graphics framebuffer.

Ok, that should work to figure whenever the mdev supports a plane
region or not.

> In the dmabuf case,VFIO_DEVICE_QUERY_GFX_PLANE would indicate the
> plane as a "plane ID" and some sort of
> VFIO_DEVICE_GET_GFX_PLANE(VFIO_GFX_TYPE_DMABUF) ioctl would be
> necessary to get a file descriptor to that plane.
> 
> What else are you thinking we need?  Thanks,

I need to know whenever the mdev supports dmabufs or not, at device
initialization time (because dmabufs require opengl support), when
VFIO_DEVICE_QUERY_GFX_PLANE doesn't work due to the guest not having
the device initialized yet.

Maybe we should have a error field in the ioctl struct, or we need to
clearly define error codes so the kernel doesn't just throw EINVAL in
all cases.

Or just a VFIO_DEVICE_GFX_CAPS ioctl which returns NONE, REGION or
DMABUF.

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/20/2017 2:05 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> Hmm, plane isn't really an ID, it is a type, with type being either
>>> DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR, so I don't think
>>> the
>>> flage above make sense.
>>
>> The intention was that ..._REGION_ID and ...PLANE_ID are describing
>> what the vfio_device_query_gfx_plane.id field represents, either a
>> region index or a plane identifier.  The type of plane would be
>> represented within the vfio_device_gfx_plane_info struct.
> 
> The planes don't really have an id, we should rename that to
> plane_type, or maybe drm_plane_type (simliar to the drm_format_*
> fields), to avoid that confusion.
> 
> plane_type is set by userspace to specify what kind of plane it asks
> for.
> 

Ok. so there should be two fields:
- plane type : DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR
- id : fd for dmabuf or region index for region type

Adding reply to Gerd's question from earlier mail:
> What are the nvidia plane for cursor support btw?

We don't support cursor for console vnc. Ideally console vnc should be
used by admin for configuration or during maintenance, which refresh
primary surface at low refresh rate, 10 fps. We recommend to use
remoting solution for actual use.

>>> Also I think it would be useful to have some way to figure the
>>> device
>>> capabilities as the userspace workflow will look quite different
>>> for
>>> the two cases.
>>
>> In the region case, VFIO_DEVICE_GET_REGION_INFO would include a
>> device
>> specific region with a hopefully common identifier to identify it as
>> a
>> graphics framebuffer.
> 
> Ok, that should work to figure whenever the mdev supports a plane
> region or not.
> 
>> In the dmabuf case,VFIO_DEVICE_QUERY_GFX_PLANE would indicate the
>> plane as a "plane ID" and some sort of
>> VFIO_DEVICE_GET_GFX_PLANE(VFIO_GFX_TYPE_DMABUF) ioctl would be
>> necessary to get a file descriptor to that plane.
>>
>> What else are you thinking we need?  Thanks,
> 
> I need to know whenever the mdev supports dmabufs or not, at device
> initialization time (because dmabufs require opengl support), when
> VFIO_DEVICE_QUERY_GFX_PLANE doesn't work due to the guest not having
> the device initialized yet.
> 
> Maybe we should have a error field in the ioctl struct, or we need to
> clearly define error codes so the kernel doesn't just throw EINVAL in
> all cases.
> 
> Or just a VFIO_DEVICE_GFX_CAPS ioctl which returns NONE, REGION or
> DMABUF.
>

Right we need to know this at device initialization time for both cases
to initialize VGACommonState structure for that device and also need
NONE to decide whether to init console vnc or not. We have a mechanism
to disable console vnc path and we recommend to disable it for better
performance.

Thanks,
Kirti

> cheers,
>   Gerd
> 

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> We don't support cursor for console vnc. Ideally console vnc should
> be
> used by admin for configuration or during maintenance, which refresh
> primary surface at low refresh rate, 10 fps.

But you surely want a mouse pointer for the admin?
You render it directly to the primary surface then I guess?

> Right we need to know this at device initialization time for both
> cases
> to initialize VGACommonState structure for that device

Why do you need a VGACommonState?

> and also need
> NONE to decide whether to init console vnc or not. We have a
> mechanism
> to disable console vnc path and we recommend to disable it for better
> performance.

Hmm, maybe we should have a ioctl to configure the refresh rate, or a
ioctl to allow qemu ask for a refresh when needed?

qemu can throttle the display update rate, which for example happens in
case no vnc client is connected.  qemu updates the display only once
every few seconds then.

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

Hey Gerd,

Sorry, I missed this mail earlier.

On 6/21/2017 12:52 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> We don't support cursor for console vnc. Ideally console vnc should
>> be
>> used by admin for configuration or during maintenance, which refresh
>> primary surface at low refresh rate, 10 fps.
> 
> But you surely want a mouse pointer for the admin?
> You render it directly to the primary surface then I guess?
> 

If cursor surface is not provided, a dot for cursor is seen on the
primary surface, which is pretty much usable.

>> Right we need to know this at device initialization time for both
>> cases
>> to initialize VGACommonState structure for that device
> 
> Why do you need a VGACommonState?
> 

We need to create a GRAPHIC_CONSOLE for vGPU device and specify
GraphicHwOps so that from its .gfx_update callback, surface can be
queried and updated.

>> and also need
>> NONE to decide whether to init console vnc or not. We have a
>> mechanism
>> to disable console vnc path and we recommend to disable it for better
>> performance.
> 
> Hmm, maybe we should have a ioctl to configure the refresh rate, or a
> ioctl to allow qemu ask for a refresh when needed?
> 

What is default refresh rate of QEMU if vnc is connected?

Thanks,
Kirti

> qemu can throttle the display update rate, which for example happens in
> case no vnc client is connected.  qemu updates the display only once
> every few seconds then.
> 
> cheers,
>   Gerd
> 

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > > Right we need to know this at device initialization time for both
> > > cases
> > > to initialize VGACommonState structure for that device
> > 
> > Why do you need a VGACommonState?
> > 
> 
> We need to create a GRAPHIC_CONSOLE for vGPU device and specify
> GraphicHwOps so that from its .gfx_update callback, surface can be
> queried and updated.

Yes, you need GraphicHwOps, but there is no need to have a
VGACommonState for that.

> > > and also need
> > > NONE to decide whether to init console vnc or not. We have a
> > > mechanism
> > > to disable console vnc path and we recommend to disable it for
> > > better
> > > performance.
> > 
> > Hmm, maybe we should have a ioctl to configure the refresh rate, or
> > a
> > ioctl to allow qemu ask for a refresh when needed?
> > 
> 
> What is default refresh rate of QEMU if vnc is connected?

/* in ms */
#define GUI_REFRESH_INTERVAL_DEFAULT    30
#define GUI_REFRESH_INTERVAL_IDLE     3000

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> My suggestion was to use vfio device fd for this ioctl and have
> dmabuf
> mgr fd as member in above query_plane structure, for region type it
> would be set to 0.

Region type should be DRM_PLANE_TYPE_PRIMARY

> Can't mmap that page to get surface information. There is no way to
> synchronize between QEMU reading this mmapped region and vendor
> driver
> writing it. There could be race condition in these two operations.
> Read
> on this page should be trapped and blocking, so that surface in that
> region is only updated when its asked for.

Does it make sense to have a "generation" field in the plane_info
struct (which gets increased each time the struct changes) ?

cheers,
  Gerd

Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Mon, 19 Jun 2017 08:38:32 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > My suggestion was to use vfio device fd for this ioctl and have
> > dmabuf
> > mgr fd as member in above query_plane structure, for region type it
> > would be set to 0.  
> 
> Region type should be DRM_PLANE_TYPE_PRIMARY
> 
> > Can't mmap that page to get surface information. There is no way to
> > synchronize between QEMU reading this mmapped region and vendor
> > driver
> > writing it. There could be race condition in these two operations.
> > Read
> > on this page should be trapped and blocking, so that surface in that
> > region is only updated when its asked for.  
> 
> Does it make sense to have a "generation" field in the plane_info
> struct (which gets increased each time the struct changes) ?

It seems less cumbersome than checking each field to see if it has
changed.  Thanks,

Alex

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.
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;
	__u32 generation;
};
struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id; 
};
2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.
VFIO_DEVICE_QUERY_GFX_PLANE : used to query vfio_device_gfx_plane_info.
VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Am I correct? Thanks.

Tina


> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Monday, June 19, 2017 10:56 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; Kirti
> Wankhede <kwankhede@nvidia.com>; Chen, Xiaoguang
> <xiaoguang.chen@intel.com>; intel-gvt-dev@lists.freedesktop.org; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Mon, 19 Jun 2017 08:38:32 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> >
> > > My suggestion was to use vfio device fd for this ioctl and have
> > > dmabuf mgr fd as member in above query_plane structure, for region
> > > type it would be set to 0.
> >
> > Region type should be DRM_PLANE_TYPE_PRIMARY
> >
> > > Can't mmap that page to get surface information. There is no way to
> > > synchronize between QEMU reading this mmapped region and vendor
> > > driver writing it. There could be race condition in these two
> > > operations.
> > > Read
> > > on this page should be trapped and blocking, so that surface in that
> > > region is only updated when its asked for.
> >
> > Does it make sense to have a "generation" field in the plane_info
> > struct (which gets increased each time the struct changes) ?
> 
> It seems less cumbersome than checking each field to see if it has changed.
> Thanks,
> 
> Alex
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> Hi,
> 
> Thanks for all the comments. Here are the summaries:
> 
> 1. Modify the structures to make it more general.
> 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;
> 	__u32 generation;
> };

Looks good to me.

> struct vfio_device_query_gfx_plane {
> 	__u32 argsz;
> 	__u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> 	struct vfio_device_gfx_plane_info plane_info;
> 	__u32 id; 
> };

I'm not convinced the flags are a great idea.  Whenever dmabufs or a
region is used is a static property of the device, not of each
individual plane.


I think we should have this for userspace to figure:

enum vfio_device_gfx_type {
        VFIO_DEVICE_GFX_NONE,
        VFIO_DEVICE_GFX_DMABUF,
        VFIO_DEVICE_GFX_REGION,
};

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};


Then this to query the plane:

struct vfio_device_gfx_query_plane {
        __u32 argsz;
        __u32 flags;
        struct vfio_device_gfx_plane_info plane_info;  /* out */
        __u32 plane_type;                              /* in  */
};


2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
device fd.
> VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> vfio_device_gfx_plane_info.

Yes.

> VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Yes.  The plane might have changed between query-plane and get-dmabuf
ioctl calls though, we must make sure we handle that somehow.  Current
patches return plane_info on get-dmabuf ioctl too, so userspace can see
what it actually got.

With the generation we can also do something different:  Pass in
plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
an error in case the generation doesn't match.  In that case it doesn't
make much sense any more to have a separate plane_info struct, which
was added so we don't have to duplicate things in query-plane and get-
dmabuf ioctl structs.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Tue, 20 Jun 2017 12:57:36 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> > Hi,
> > 
> > Thanks for all the comments. Here are the summaries:
> > 
> > 1. Modify the structures to make it more general.
> > 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;
> > 	__u32 generation;
> > };  
> 
> Looks good to me.
> 
> > struct vfio_device_query_gfx_plane {
> > 	__u32 argsz;
> > 	__u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > 	struct vfio_device_gfx_plane_info plane_info;
> > 	__u32 id; 
> > };  
> 
> I'm not convinced the flags are a great idea.  Whenever dmabufs or a
> region is used is a static property of the device, not of each
> individual plane.
> 
> 
> I think we should have this for userspace to figure:
> 
> enum vfio_device_gfx_type {
>         VFIO_DEVICE_GFX_NONE,
>         VFIO_DEVICE_GFX_DMABUF,
>         VFIO_DEVICE_GFX_REGION,
> };
> 
> struct vfio_device_gfx_query_caps {
>         __u32 argsz;
>         __u32 flags;
>         enum vfio_device_gfx_type;
> };

We already have VFIO_DEVICE_GET_INFO which returns:

struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */
};

We could use two flag bits to indicate dmabuf or graphics region
support.  vfio_device_gfx_query_caps seems to imply a new ioctl, which
would be unnecessary.
 
> Then this to query the plane:
> 
> struct vfio_device_gfx_query_plane {
>         __u32 argsz;
>         __u32 flags;
>         struct vfio_device_gfx_plane_info plane_info;  /* out */
>         __u32 plane_type;                              /* in  */
> };

I'm not sure why we're using an enum for something that can currently
be defined with 2 bits, seems like this would be another good use of
flags.  We could even embed an enum into the flags if we want to
leave some expansion room, 4 bits maybe?  Also, I was imagining that a
device could support multiple graphics regions, that's where specifying
the "id" as a region index seemed useful.  We lose that ability here
unless we go back to defining a flag bit to specify how to interpret
this last field.

> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> device fd.
> > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > vfio_device_gfx_plane_info.  
> 
> Yes.
> 
> > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.  

I'm not convinced this adds value, but I'll list it as an option:

VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)

The benefit is that it might help to avoid a proliferation of ioctls on
the device the pain is that we need to either define a field or section
of flags which identify what is being queried or what type of device fd
is being requested.

> Yes.  The plane might have changed between query-plane and get-dmabuf
> ioctl calls though, we must make sure we handle that somehow.  Current
> patches return plane_info on get-dmabuf ioctl too, so userspace can see
> what it actually got.
> 
> With the generation we can also do something different:  Pass in
> plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
> an error in case the generation doesn't match.  In that case it doesn't
> make much sense any more to have a separate plane_info struct, which
> was added so we don't have to duplicate things in query-plane and get-
> dmabuf ioctl structs.

I'm not sure I understand how this works for a region, the region is
always the current generation, how can the user ever be sure the
plane_info matches what is exposed in the region?  Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/20/2017 8:30 PM, Alex Williamson wrote:
> On Tue, 20 Jun 2017 12:57:36 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>> On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
>>> Hi,
>>>
>>> Thanks for all the comments. Here are the summaries:
>>>
>>> 1. Modify the structures to make it more general.
>>> 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;
>>> 	__u32 generation;
>>> };  
>>
>> Looks good to me.
>>
>>> struct vfio_device_query_gfx_plane {
>>> 	__u32 argsz;
>>> 	__u32 flags;
>>> #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
>>> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
>>> 	struct vfio_device_gfx_plane_info plane_info;
>>> 	__u32 id; 
>>> };  
>>
>> I'm not convinced the flags are a great idea.  Whenever dmabufs or a
>> region is used is a static property of the device, not of each
>> individual plane.
>>
>>
>> I think we should have this for userspace to figure:
>>
>> enum vfio_device_gfx_type {
>>         VFIO_DEVICE_GFX_NONE,
>>         VFIO_DEVICE_GFX_DMABUF,
>>         VFIO_DEVICE_GFX_REGION,
>> };
>>
>> struct vfio_device_gfx_query_caps {
>>         __u32 argsz;
>>         __u32 flags;
>>         enum vfio_device_gfx_type;
>> };
> 
> We already have VFIO_DEVICE_GET_INFO which returns:
> 
> struct vfio_device_info {
>         __u32   argsz;
>         __u32   flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
>         __u32   num_regions;    /* Max region index + 1 */
>         __u32   num_irqs;       /* Max IRQ index + 1 */
> };
> 
> We could use two flag bits to indicate dmabuf or graphics region
> support.  vfio_device_gfx_query_caps seems to imply a new ioctl, which
> would be unnecessary.
>

Sounds good to me.

>> Then this to query the plane:
>>
>> struct vfio_device_gfx_query_plane {
>>         __u32 argsz;
>>         __u32 flags;
>>         struct vfio_device_gfx_plane_info plane_info;  /* out */
>>         __u32 plane_type;                              /* in  */
>> };
> 
> I'm not sure why we're using an enum for something that can currently
> be defined with 2 bits, seems like this would be another good use of
> flags.  We could even embed an enum into the flags if we want to
> leave some expansion room, 4 bits maybe?  Also, I was imagining that a
> device could support multiple graphics regions, that's where specifying
> the "id" as a region index seemed useful.  We lose that ability here
> unless we go back to defining a flag bit to specify how to interpret
> this last field.
> 

Right, as I mentioned in earlier reply, we need 2 seperate fields
- plane type : DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR
- id : fd for dmabuf or region index for region type


>> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
>> device fd.
>>> VFIO_DEVICE_QUERY_GFX_PLANE : used to query
>>> vfio_device_gfx_plane_info.  
>>
>> Yes.
>>
>>> VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.  
> 
> I'm not convinced this adds value, but I'll list it as an option:
> 
> VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
> 
> The benefit is that it might help to avoid a proliferation of ioctls on
> the device the pain is that we need to either define a field or section
> of flags which identify what is being queried or what type of device fd
> is being requested.
> 
>> Yes.  The plane might have changed between query-plane and get-dmabuf
>> ioctl calls though, we must make sure we handle that somehow.  Current
>> patches return plane_info on get-dmabuf ioctl too, so userspace can see
>> what it actually got.
>>
>> With the generation we can also do something different:  Pass in
>> plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
>> an error in case the generation doesn't match.  In that case it doesn't
>> make much sense any more to have a separate plane_info struct, which
>> was added so we don't have to duplicate things in query-plane and get-
>> dmabuf ioctl structs.
> 
> I'm not sure I understand how this works for a region, the region is
> always the current generation, how can the user ever be sure the
> plane_info matches what is exposed in the region?  Thanks,
>

Userspace have to follow the sequence to query plane info
(VFIO_DEVICE_QUERY_GFX_PLANE) and then read primary surface from the region.
On kernel space side, from VFIO_DEVICE_QUERY_GFX_PLANE ioctl, driver
should update surface which is being exposed by the GFX region, fill
vfio_device_gfx_plane_info structure then return. GFX region surface
would only get updated from this ioctl.

Thanks,
Kirti

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, June 20, 2017 11:00 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; Kirti Wankhede <kwankhede@nvidia.com>; Chen,
> Xiaoguang <xiaoguang.chen@intel.com>; intel-gvt-dev@lists.freedesktop.org;
> Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>;
> Wang, Zhenyu Z <zhenyu.z.wang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 20 Jun 2017 12:57:36 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> > > Hi,
> > >
> > > Thanks for all the comments. Here are the summaries:
> > >
> > > 1. Modify the structures to make it more general.
> > > 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;
> > > 	__u32 generation;
> > > };
> >
> > Looks good to me.
> >
> > > struct vfio_device_query_gfx_plane {
> > > 	__u32 argsz;
> > > 	__u32 flags;
> > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > > 	struct vfio_device_gfx_plane_info plane_info;
> > > 	__u32 id;
> > > };
> >
> > I'm not convinced the flags are a great idea.  Whenever dmabufs or a
> > region is used is a static property of the device, not of each
> > individual plane.
> >
> >
> > I think we should have this for userspace to figure:
> >
> > enum vfio_device_gfx_type {
> >         VFIO_DEVICE_GFX_NONE,
> >         VFIO_DEVICE_GFX_DMABUF,
> >         VFIO_DEVICE_GFX_REGION,
> > };
> >
> > struct vfio_device_gfx_query_caps {
> >         __u32 argsz;
> >         __u32 flags;
> >         enum vfio_device_gfx_type;
> > };
> 
> We already have VFIO_DEVICE_GET_INFO which returns:
> 
> struct vfio_device_info {
>         __u32   argsz;
>         __u32   flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
>         __u32   num_regions;    /* Max region index + 1 */
>         __u32   num_irqs;       /* Max IRQ index + 1 */
> };
> 
> We could use two flag bits to indicate dmabuf or graphics region support.
> vfio_device_gfx_query_caps seems to imply a new ioctl, which would be
> unnecessary.
> 
> > Then this to query the plane:
> >
> > struct vfio_device_gfx_query_plane {
> >         __u32 argsz;
> >         __u32 flags;
> >         struct vfio_device_gfx_plane_info plane_info;  /* out */
> >         __u32 plane_type;                              /* in  */
> > };
> 
> I'm not sure why we're using an enum for something that can currently be
> defined with 2 bits, seems like this would be another good use of flags.  We
> could even embed an enum into the flags if we want to leave some expansion
> room, 4 bits maybe?  Also, I was imagining that a device could support multiple
> graphics regions, that's where specifying the "id" as a region index seemed
> useful.  We lose that ability here unless we go back to defining a flag bit to
> specify how to interpret this last field.
> 
> > 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> > device fd.
> > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > vfio_device_gfx_plane_info.
> >
> > Yes.
> >
> > > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
> 
> I'm not convinced this adds value, but I'll list it as an option:
> 
> VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
> 
> The benefit is that it might help to avoid a proliferation of ioctls on the device the
> pain is that we need to either define a field or section of flags which identify
> what is being queried or what type of device fd is being requested.
I didn't understand here. The patch introduces three ioctl commands: VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE, VFIO_DMABUF_MGR_CREATE_DMABUF.
What I mean was we could remove the first one, a.k.a  VFIO_DEVICE_GET_FD, which is used to get the fd of dmabuf mgr, as we want to remove the logic of dmabuf mgr. For the other two ioctls, I think we can give them new names which looks like more general. 
So, do you mean there is another way instead of ioctls? Thanks.

BR,
Tina

> > Yes.  The plane might have changed between query-plane and get-dmabuf
> > ioctl calls though, we must make sure we handle that somehow.  Current
> > patches return plane_info on get-dmabuf ioctl too, so userspace can
> > see what it actually got.
> >
> > With the generation we can also do something different:  Pass in
> > plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
> > an error in case the generation doesn't match.  In that case it
> > doesn't make much sense any more to have a separate plane_info struct,
> > which was added so we don't have to duplicate things in query-plane
> > and get- dmabuf ioctl structs.
> 
> I'm not sure I understand how this works for a region, the region is always the
> current generation, how can the user ever be sure the plane_info matches what
> is exposed in the region?  Thanks,
> 
> Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Tue, 20 Jun 2017 23:01:53 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, June 20, 2017 11:00 PM
> > To: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org; Kirti Wankhede <kwankhede@nvidia.com>; Chen,
> > Xiaoguang <xiaoguang.chen@intel.com>; intel-gvt-dev@lists.freedesktop.org;
> > Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>;
> > Wang, Zhenyu Z <zhenyu.z.wang@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> > operations
> > 
> > On Tue, 20 Jun 2017 12:57:36 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:  
> > > > Hi,
> > > >
> > > > Thanks for all the comments. Here are the summaries:
> > > >
> > > > 1. Modify the structures to make it more general.
> > > > 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;
> > > > 	__u32 generation;
> > > > };  
> > >
> > > Looks good to me.
> > >  
> > > > struct vfio_device_query_gfx_plane {
> > > > 	__u32 argsz;
> > > > 	__u32 flags;
> > > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > > > 	struct vfio_device_gfx_plane_info plane_info;
> > > > 	__u32 id;
> > > > };  
> > >
> > > I'm not convinced the flags are a great idea.  Whenever dmabufs or a
> > > region is used is a static property of the device, not of each
> > > individual plane.
> > >
> > >
> > > I think we should have this for userspace to figure:
> > >
> > > enum vfio_device_gfx_type {
> > >         VFIO_DEVICE_GFX_NONE,
> > >         VFIO_DEVICE_GFX_DMABUF,
> > >         VFIO_DEVICE_GFX_REGION,
> > > };
> > >
> > > struct vfio_device_gfx_query_caps {
> > >         __u32 argsz;
> > >         __u32 flags;
> > >         enum vfio_device_gfx_type;
> > > };  
> > 
> > We already have VFIO_DEVICE_GET_INFO which returns:
> > 
> > struct vfio_device_info {
> >         __u32   argsz;
> >         __u32   flags;
> > #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
> > #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
> > #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
> > #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
> >         __u32   num_regions;    /* Max region index + 1 */
> >         __u32   num_irqs;       /* Max IRQ index + 1 */
> > };
> > 
> > We could use two flag bits to indicate dmabuf or graphics region support.
> > vfio_device_gfx_query_caps seems to imply a new ioctl, which would be
> > unnecessary.
> >   
> > > Then this to query the plane:
> > >
> > > struct vfio_device_gfx_query_plane {
> > >         __u32 argsz;
> > >         __u32 flags;
> > >         struct vfio_device_gfx_plane_info plane_info;  /* out */
> > >         __u32 plane_type;                              /* in  */
> > > };  
> > 
> > I'm not sure why we're using an enum for something that can currently be
> > defined with 2 bits, seems like this would be another good use of flags.  We
> > could even embed an enum into the flags if we want to leave some expansion
> > room, 4 bits maybe?  Also, I was imagining that a device could support multiple
> > graphics regions, that's where specifying the "id" as a region index seemed
> > useful.  We lose that ability here unless we go back to defining a flag bit to
> > specify how to interpret this last field.
> >   
> > > 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> > > device fd.  
> > > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > > vfio_device_gfx_plane_info.  
> > >
> > > Yes.
> > >  
> > > > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.  
> > 
> > I'm not convinced this adds value, but I'll list it as an option:
> > 
> > VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> > VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
> > 
> > The benefit is that it might help to avoid a proliferation of ioctls on the device the
> > pain is that we need to either define a field or section of flags which identify
> > what is being queried or what type of device fd is being requested.  
> I didn't understand here. The patch introduces three ioctl commands: VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE, VFIO_DMABUF_MGR_CREATE_DMABUF.
> What I mean was we could remove the first one, a.k.a  VFIO_DEVICE_GET_FD, which is used to get the fd of dmabuf mgr, as we want to remove the logic of dmabuf mgr. For the other two ioctls, I think we can give them new names which looks like more general. 
> So, do you mean there is another way instead of ioctls? Thanks.

In this v9 series, we have a VFIO_DEVICE_GET_FD where we could pass
VFIO_DEVICE_DMABUF_MGR_FD to get the manager fd, from that fd we could
query plane info or get a dmabuf fd.  Now we're getting rid of the
manager fd and I'm questioning whether generic ioctls or specific
ioctls are the right path.

For instance we could still use a VFIO_DEVICE_GET_FD ioctl to get the
dmabuf fd rather than creating a VFIO_DEVICE_GET_DMABUF_FD ioctl.  It's
just a matter of defining a common header on the data structure so that
we know how to interpret the remainder of the structure.

Likewise Gerd proposes above a VFIO_DEVICE_QUERY_GFX_PLANE ioctl and
I'm asking whether a generic VFIO_DEVICE_QUERY ioctl where we define a
common header in the structure to know that the query is for graphics
plane information has value.

IOW, should we spend a little bit of time now crafting ioctls that we
can use for purposes beyond what we're looking at today, or do we burn
off a couple for singular uses here?  Thanks,

Alex


> > > Yes.  The plane might have changed between query-plane and get-dmabuf
> > > ioctl calls though, we must make sure we handle that somehow.  Current
> > > patches return plane_info on get-dmabuf ioctl too, so userspace can
> > > see what it actually got.
> > >
> > > With the generation we can also do something different:  Pass in
> > > plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return
> > > an error in case the generation doesn't match.  In that case it
> > > doesn't make much sense any more to have a separate plane_info struct,
> > > which was added so we don't have to duplicate things in query-plane
> > > and get- dmabuf ioctl structs.  
> > 
> > I'm not sure I understand how this works for a region, the region is always the
> > current generation, how can the user ever be sure the plane_info matches what
> > is exposed in the region?  Thanks,
> > 
> > Alex  

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

Thanks for all the comments. I'm planning to cook the next version of this patch set which I'd like to include all the comments and ideas. Here is the summary:

1.  How to figure the device capabilities
struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */
};
> We could use two flag bits to indicate dmabuf or graphics region support. 
Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf device */
#define VFIO_DEVICE_FLAGS_GFX_REGION   (1 << 6)        /* vfio-gfx-region device */

2. vfio_device_gfx_plane_info
struct vfio_device_gfx_plane_info {
	__u64 start;-> offset
	__u64 drm_format_mod;
	__u32 drm_format;
	__u32 width;
	__u32 height;
	__u32 stride;
	__u32 size;
	__u32 x_pos;
	__u32 y_pos;
};
> Does it make sense to have a "generation" field in the plane_info struct (which gets increased each time the struct changes) ?
Well,  Gerd, can you share more details about how to use this field in user mode, so that we can figure out a way to support it? Thanks.

3. vfio_device_query_gfx_plane
struct vfio_device_query_gfx_plane {
	__u32 argsz;
	__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
	struct vfio_device_gfx_plane_info plane_info;
	__u32 id;
	__u32 plane_type;
};
So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR. If the newly added plane_type is used for this, the id field may be useless in dmabuf usage. Do you have any idea about the usage of this id field in dmabuf usage?

4. Two ioctl commands
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

5. > Then we should kill off the manager fd unless there are arguments that still give it value.
Agree.

If there is anything I miss, please tell us.

Thanks.
Tina


> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, June 21, 2017 7:22 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; intel-gfx@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; Kirti Wankhede <kwankhede@nvidia.com>;
> Chen, Xiaoguang <xiaoguang.chen@intel.com>; intel-gvt-
> dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> <zhi.a.wang@intel.com>; Wang, Zhenyu Z <zhenyu.z.wang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 20 Jun 2017 23:01:53 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Tuesday, June 20, 2017 11:00 PM
> > > To: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Zhang, Tina <tina.zhang@intel.com>;
> > > intel-gfx@lists.freedesktop.org; linux- kernel@vger.kernel.org;
> > > Kirti Wankhede <kwankhede@nvidia.com>; Chen, Xiaoguang
> > > <xiaoguang.chen@intel.com>; intel-gvt-dev@lists.freedesktop.org;
> > > Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> > > <zhi.a.wang@intel.com>; Wang, Zhenyu Z <zhenyu.z.wang@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based
> > > dma-buf operations
> > >
> > > On Tue, 20 Jun 2017 12:57:36 +0200
> > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > > On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> > > > > Hi,
> > > > >
> > > > > Thanks for all the comments. Here are the summaries:
> > > > >
> > > > > 1. Modify the structures to make it more general.
> > > > > 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;
> > > > > 	__u32 generation;
> > > > > };
> > > >
> > > > Looks good to me.
> > > >
> > > > > struct vfio_device_query_gfx_plane {
> > > > > 	__u32 argsz;
> > > > > 	__u32 flags;
> > > > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > > > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > > > > 	struct vfio_device_gfx_plane_info plane_info;
> > > > > 	__u32 id;
> > > > > };
> > > >
> > > > I'm not convinced the flags are a great idea.  Whenever dmabufs or
> > > > a region is used is a static property of the device, not of each
> > > > individual plane.
> > > >
> > > >
> > > > I think we should have this for userspace to figure:
> > > >
> > > > enum vfio_device_gfx_type {
> > > >         VFIO_DEVICE_GFX_NONE,
> > > >         VFIO_DEVICE_GFX_DMABUF,
> > > >         VFIO_DEVICE_GFX_REGION,
> > > > };
> > > >
> > > > struct vfio_device_gfx_query_caps {
> > > >         __u32 argsz;
> > > >         __u32 flags;
> > > >         enum vfio_device_gfx_type; };
> > >
> > > We already have VFIO_DEVICE_GET_INFO which returns:
> > >
> > > struct vfio_device_info {
> > >         __u32   argsz;
> > >         __u32   flags;
> > > #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
> > > #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> > > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device
> */
> > > #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
> > > #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
> > >         __u32   num_regions;    /* Max region index + 1 */
> > >         __u32   num_irqs;       /* Max IRQ index + 1 */
> > > };
> > >
> > > We could use two flag bits to indicate dmabuf or graphics region support.
> > > vfio_device_gfx_query_caps seems to imply a new ioctl, which would
> > > be unnecessary.
> > >
> > > > Then this to query the plane:
> > > >
> > > > struct vfio_device_gfx_query_plane {
> > > >         __u32 argsz;
> > > >         __u32 flags;
> > > >         struct vfio_device_gfx_plane_info plane_info;  /* out */
> > > >         __u32 plane_type;                              /* in  */
> > > > };
> > >
> > > I'm not sure why we're using an enum for something that can
> > > currently be defined with 2 bits, seems like this would be another
> > > good use of flags.  We could even embed an enum into the flags if we
> > > want to leave some expansion room, 4 bits maybe?  Also, I was
> > > imagining that a device could support multiple graphics regions,
> > > that's where specifying the "id" as a region index seemed useful.
> > > We lose that ability here unless we go back to defining a flag bit to specify
> how to interpret this last field.
> > >
> > > > 2. Remove dmabuf mgr fd and add these two ioctl commands to the
> > > > vfio device fd.
> > > > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > > > vfio_device_gfx_plane_info.
> > > >
> > > > Yes.
> > > >
> > > > > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf
> fd.
> > >
> > > I'm not convinced this adds value, but I'll list it as an option:
> > >
> > > VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> > > VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
> > >
> > > The benefit is that it might help to avoid a proliferation of ioctls
> > > on the device the pain is that we need to either define a field or
> > > section of flags which identify what is being queried or what type of device
> fd is being requested.
> > I didn't understand here. The patch introduces three ioctl commands:
> VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE,
> VFIO_DMABUF_MGR_CREATE_DMABUF.
> > What I mean was we could remove the first one, a.k.a  VFIO_DEVICE_GET_FD,
> which is used to get the fd of dmabuf mgr, as we want to remove the logic of
> dmabuf mgr. For the other two ioctls, I think we can give them new names
> which looks like more general.
> > So, do you mean there is another way instead of ioctls? Thanks.
> 
> In this v9 series, we have a VFIO_DEVICE_GET_FD where we could pass
> VFIO_DEVICE_DMABUF_MGR_FD to get the manager fd, from that fd we could
> query plane info or get a dmabuf fd.  Now we're getting rid of the manager fd
> and I'm questioning whether generic ioctls or specific ioctls are the right path.
> 
> For instance we could still use a VFIO_DEVICE_GET_FD ioctl to get the dmabuf
> fd rather than creating a VFIO_DEVICE_GET_DMABUF_FD ioctl.  It's just a
> matter of defining a common header on the data structure so that we know
> how to interpret the remainder of the structure.
> 
> Likewise Gerd proposes above a VFIO_DEVICE_QUERY_GFX_PLANE ioctl and
> I'm asking whether a generic VFIO_DEVICE_QUERY ioctl where we define a
> common header in the structure to know that the query is for graphics plane
> information has value.
> 
> IOW, should we spend a little bit of time now crafting ioctls that we can use for
> purposes beyond what we're looking at today, or do we burn off a couple for
> singular uses here?  Thanks,
> 
> Alex
Thanks Alex. I guess we all prefer the first one.

> 
> > > > Yes.  The plane might have changed between query-plane and
> > > > get-dmabuf ioctl calls though, we must make sure we handle that
> > > > somehow.  Current patches return plane_info on get-dmabuf ioctl
> > > > too, so userspace can see what it actually got.
> > > >
> > > > With the generation we can also do something different:  Pass in
> > > > plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD
> > > > return an error in case the generation doesn't match.  In that
> > > > case it doesn't make much sense any more to have a separate
> > > > plane_info struct, which was added so we don't have to duplicate
> > > > things in query-plane and get- dmabuf ioctl structs.
> > >
> > > I'm not sure I understand how this works for a region, the region is
> > > always the current generation, how can the user ever be sure the
> > > plane_info matches what is exposed in the region?  Thanks,
> > >
> > > Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

On Wed, 2017-06-21 at 09:20 +0000, Zhang, Tina wrote:
> Thanks for all the comments. I'm planning to cook the next version of
> this patch set

How about posting only this patch instead of the whole series until
we've settled the interfaces?

> Could the following two works?
> #define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf
> device */

VFIO_DEVICE_FLAGS_GFX_DMABUF?

> 2. vfio_device_gfx_plane_info
> struct vfio_device_gfx_plane_info {
> 	__u64 start;-> offset
> 	__u64 drm_format_mod;
> 	__u32 drm_format;
> 	__u32 width;
> 	__u32 height;
> 	__u32 stride;
> 	__u32 size;
> 	__u32 x_pos;
> 	__u32 y_pos;
> };
> > Does it make sense to have a "generation" field in the plane_info
> > struct (which gets increased each time the struct changes) ?

> Well,  Gerd, can you share more details about how to use this field
> in user mode, so that we can figure out a way to support it? Thanks.

generation would be increased each time one of the fields in
vfio_device_gfx_plane_info changes, typically on mode switches
(width/height changes) and pageflips (offset changes).  So userspace
can simply compare generation instead of comparing every field to
figure whenever something changed compared to the previous poll.

> 
> 3. vfio_device_query_gfx_plane
> struct vfio_device_query_gfx_plane {
> 	__u32 argsz;
> 	__u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> 	struct vfio_device_gfx_plane_info plane_info;
> 	__u32 id;
> 	__u32 plane_type;
> };
> So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR.


>  If the newly added plane_type is used for this, the id field may be
> useless in dmabuf usage. Do you have any idea about the usage of this
> id field in dmabuf usage?

plane_type should be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR
for dmabuf.

Given that nvidia doesn't support a separate cursor plane in their
region they would support DRM_PLANE_TYPE_PRIMARY only.

I can't see yet what id would be useful for.

Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Wed, 21 Jun 2017 13:03:31 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Wed, 2017-06-21 at 09:20 +0000, Zhang, Tina wrote:
> > Thanks for all the comments. I'm planning to cook the next version of
> > this patch set  
> 
> How about posting only this patch instead of the whole series until
> we've settled the interfaces?
> 
> > Could the following two works?
> > #define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf
> > device */  
> 
> VFIO_DEVICE_FLAGS_GFX_DMABUF?

After proposing these, I'm kind of questioning their purpose.  In the
case of a GFX region, the user is going to learn that this is supported
as they parse the region information and find the device specific
region identifying itself as a GFX area.  That needs to happen early
when the user is evaluating the device, so it seems rather redundant
to the flag.

If we have dmabuf support, isn't that indicated by trying to query the
graphics plane_info and getting back a result indicating a dmabuf fd?
Is there any point in time where a device supporting dmabuf fds would
not report this here?  Userspace could really do the same process for a
graphics region, ie. querying the plane_info, if it exists pursuing
either the region or dmabuf path to get it.

FWIW, I think the RESET flag in the device_info struct was a mistake,
we could have simply let the user probe for it, perhaps with a no-op
flag in the ioctl explicitly for that purpose.  So I'm not in favor of
adding device_info flags that only indicate the presence of a given
ioctl, the user can try it and find out so long as it doesn't cause a
state change or we have a probe option.

> > 2. vfio_device_gfx_plane_info
> > struct vfio_device_gfx_plane_info {
> > 	__u64 start;-> offset
> > 	__u64 drm_format_mod;
> > 	__u32 drm_format;
> > 	__u32 width;
> > 	__u32 height;
> > 	__u32 stride;
> > 	__u32 size;
> > 	__u32 x_pos;
> > 	__u32 y_pos;
> > };  
> > > Does it make sense to have a "generation" field in the plane_info
> > > struct (which gets increased each time the struct changes) ?  
> 
> > Well,  Gerd, can you share more details about how to use this field
> > in user mode, so that we can figure out a way to support it? Thanks.  
> 
> generation would be increased each time one of the fields in
> vfio_device_gfx_plane_info changes, typically on mode switches
> (width/height changes) and pageflips (offset changes).  So userspace
> can simply compare generation instead of comparing every field to
> figure whenever something changed compared to the previous poll.

So we have two scenarios, dmabuf and region.  When the user retrieves a
dmabuf they get plane_info which includes the generation, so they know
the dmabuf is for that generation.  If they query the plane_info and
get a different generation they should close the previous dmabuf and
get another.  Does this promote the caching idea that a user might
maintain multiple open dmabuf fds and select the appropriate one for
the current device state?  Is it entirely the user's responsibility to
remember the plane info for an open dmabuf fd?  What happens to
existing dmabuf fds when the generation updates, do they stop
refreshing?  Does it blank the framebuffer?  Can the dmabuf fd
transparently update to the new plane_info?

The other case is a region, the user queries the plane_info records the
parameters and region info, and configures access to the region using
that information.  Meanwhile, something changed, plane_info including
generation is updated, but the user is still assuming the previous
plane_info.  How can we avoid such a race?  What is the responsibility
of the user and how are they notified to refresh the plane_info?  It
seems there's no way for the user to avoid occasionally being out of
sync with the current plane_info for a region.

> > 
> > 3. vfio_device_query_gfx_plane
> > struct vfio_device_query_gfx_plane {
> > 	__u32 argsz;
> > 	__u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > 	struct vfio_device_gfx_plane_info plane_info;
> > 	__u32 id;
> > 	__u32 plane_type;
> > };
> > So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR.  
> 
> 
> >  If the newly added plane_type is used for this, the id field may be
> > useless in dmabuf usage. Do you have any idea about the usage of this
> > id field in dmabuf usage?  
> 
> plane_type should be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR
> for dmabuf.
> 
> Given that nvidia doesn't support a separate cursor plane in their
> region they would support DRM_PLANE_TYPE_PRIMARY only.
> 
> I can't see yet what id would be useful for.
> 
> Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.

I think we're trying to identify where this plane_info exists.  Does
the user ask for a dmabuf fd for it or use a region.  If it's a dmabuf
fd, how might they match this to one they already know about (assuming
a generation ID is compatible with such a notion)?  If it's a region,
which region?

Cut off here, but from Tina's previous reply:

> 4. Two ioctl commands
> VFIO_DEVICE_QUERY_GFX_PLANE
> VFIO_DEVICE_GET_FD

Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any
opinions?  Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > VFIO_DEVICE_FLAGS_GFX_DMABUF?
> 
> After proposing these, I'm kind of questioning their purpose.  In the
> case of a GFX region, the user is going to learn that this is
> supported
> as they parse the region information and find the device specific
> region identifying itself as a GFX area.  That needs to happen early
> when the user is evaluating the device, so it seems rather redundant
> to the flag.

Indeed.

> If we have dmabuf support, isn't that indicated by trying to query
> the
> graphics plane_info and getting back a result indicating a dmabuf fd?
> Is there any point in time where a device supporting dmabuf fds would
> not report this here?  Userspace could really do the same process for
> a
> graphics region, ie. querying the plane_info, if it exists pursuing
> either the region or dmabuf path to get it.

Well, you can get a dma-buf only after the guest loaded the driver and
initialized the device, so a plane actually exists ...

Right now the experimental intel patches throw errors in case no plane
exists (yet).  Maybe we should have a "bool is_enabled" field in the
plane_info struct, so drivers can use that to signal whenever the guest
has programmed a valid video mode or not (likewise for the cursor,
which doesn't exist with fbcon, only when running xorg).  With that in
place using the QUERY_PLANE ioctl also for probing looks reasonable.

> > generation would be increased each time one of the fields in
> > vfio_device_gfx_plane_info changes, typically on mode switches
> > (width/height changes) and pageflips (offset changes).  So
> > userspace
> > can simply compare generation instead of comparing every field to
> > figure whenever something changed compared to the previous poll.
> 
> So we have two scenarios, dmabuf and region.  When the user retrieves
> a
> dmabuf they get plane_info which includes the generation, so they
> know
> the dmabuf is for that generation.  If they query the plane_info and
> get a different generation they should close the previous dmabuf and
> get another.

Keeping it cached is a valid choice too.

> Does this promote the caching idea that a user might
> maintain multiple open dmabuf fds and select the appropriate one for
> the current device state?  Is it entirely the user's responsibility
> to
> remember the plane info for an open dmabuf fd?

Yes, I'd leave that to userspace.  So, when the generation changes
userspace knows the guest changed the plane.  It could be a
configuration the guest has used before (and where userspace could have
a cached dma-buf handle for), or it could be something new.

> What happens to
> existing dmabuf fds when the generation updates, do they stop
> refreshing?

Depends on what the guest is doing ;)

The dma-buf is just a host-side handle for the piece of video memory
where the guest stored the framebuffer.

> Does it blank the framebuffer?

No.

> Can the dmabuf fd
> transparently update to the new plane_info?

No.

> The other case is a region, the user queries the plane_info records
> the
> parameters and region info, and configures access to the region using
> that information.  Meanwhile, something changed, plane_info including
> generation is updated, but the user is still assuming the previous
> plane_info.  How can we avoid such a race?

Qemu doesn't.  You might get rendering glitches in that case, due to
accessing the plane with the wrong configuration.  It's fundamentally
the same with stdvga btw.

> What is the responsibility
> of the user and how are they notified to refresh the plane_info?

qemu polls in regular intervals, simliar to how it checks the dirty
bitmap for video memory in regular intervals with stdvga.

qemu display update timer runs on 30fps usually, in case nobody is
looking (no spice/vnc client connected) it reduces the update frequency
though.

> > plane_type should be DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR
> > for dmabuf.
> > 
> > Given that nvidia doesn't support a separate cursor plane in their
> > region they would support DRM_PLANE_TYPE_PRIMARY only.
> > 
> > I can't see yet what id would be useful for.
> > 
> > Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good
> > for.
> 
> I think we're trying to identify where this plane_info exists.  Does
> the user ask for a dmabuf fd for it or use a region.

But whenever a region or a dmabuf is used is a fixed property of the
device, why associate that with a plane?  It will be the same for all
planes of a device anyway ...

> If it's a region,
> which region?

Ok, if we want support multiple regions.  Do we?  Using the offset we
can place multiple planes in a single region.  And I'm not sure nvidia
plans to use multiple planes in the first place ...

> 4. Two ioctl commands
> > VFIO_DEVICE_QUERY_GFX_PLANE
> > VFIO_DEVICE_GET_FD
> 
> Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any
> opinions?  Thanks,

I don't think a generic device query is that helpful.  Depending on the
kind of query you'll get back different stuff, and we need to handle
that somehow, like this:

vfio_device_query {
    u32 argsz;
    u32 flags;
    enum query_type;  /* or use flags for that */
    union {
        plane_info plane;
        /* whatever else comes up */
    } query_params;
};

I fail to see how this is fundamentally different from having multiple
vfio_device_query_* structs (and ioctls).  It only pushes the problem
one level down ...

Or do you have something else in mind?

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Thu, 22 Jun 2017 10:30:15 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > VFIO_DEVICE_FLAGS_GFX_DMABUF?  
> > 
> > After proposing these, I'm kind of questioning their purpose.  In the
> > case of a GFX region, the user is going to learn that this is
> > supported
> > as they parse the region information and find the device specific
> > region identifying itself as a GFX area.  That needs to happen early
> > when the user is evaluating the device, so it seems rather redundant
> > to the flag.  
> 
> Indeed.
> 
> > If we have dmabuf support, isn't that indicated by trying to query
> > the
> > graphics plane_info and getting back a result indicating a dmabuf fd?
> > Is there any point in time where a device supporting dmabuf fds would
> > not report this here?  Userspace could really do the same process for
> > a
> > graphics region, ie. querying the plane_info, if it exists pursuing
> > either the region or dmabuf path to get it.  
> 
> Well, you can get a dma-buf only after the guest loaded the driver and
> initialized the device, so a plane actually exists ...

Is this only going to support accelerated driver output, not basic VGA
modes for BIOS interaction?
 
> Right now the experimental intel patches throw errors in case no plane
> exists (yet).  Maybe we should have a "bool is_enabled" field in the
> plane_info struct, so drivers can use that to signal whenever the guest
> has programmed a valid video mode or not (likewise for the cursor,
> which doesn't exist with fbcon, only when running xorg).  With that in
> place using the QUERY_PLANE ioctl also for probing looks reasonable.

Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no available
plane, but then that might not help the user know how a plane would be
available if it were available.

> > > generation would be increased each time one of the fields in
> > > vfio_device_gfx_plane_info changes, typically on mode switches
> > > (width/height changes) and pageflips (offset changes).  So
> > > userspace
> > > can simply compare generation instead of comparing every field to
> > > figure whenever something changed compared to the previous poll.  
> > 
> > So we have two scenarios, dmabuf and region.  When the user retrieves
> > a
> > dmabuf they get plane_info which includes the generation, so they
> > know
> > the dmabuf is for that generation.  If they query the plane_info and
> > get a different generation they should close the previous dmabuf and
> > get another.  
> 
> Keeping it cached is a valid choice too.

So generation is just intended to be a unique handle, like a uuid but
cheaper.  Generally I think of a generation field only to track what's
current.  Userspace might assume a "generation" never goes backwards
(until it wraps).

> > Does this promote the caching idea that a user might
> > maintain multiple open dmabuf fds and select the appropriate one for
> > the current device state?  Is it entirely the user's responsibility
> > to
> > remember the plane info for an open dmabuf fd?  
> 
> Yes, I'd leave that to userspace.  So, when the generation changes
> userspace knows the guest changed the plane.  It could be a
> configuration the guest has used before (and where userspace could have
> a cached dma-buf handle for), or it could be something new.

But userspace also doesn't know that a dmabuf generation will ever be
visited again, so they're bound to have some stale descriptors.  Are
we thinking userspace would have some LRU list of dmabufs so that they
don't collect too many?  Each uses some resources,  do we just rely on
open file handles to set an upper limit?
 
> > What happens to
> > existing dmabuf fds when the generation updates, do they stop
> > refreshing?  
> 
> Depends on what the guest is doing ;)
> 
> The dma-buf is just a host-side handle for the piece of video memory
> where the guest stored the framebuffer.

So the resources the user is holding if they don't release their dmabuf
are potentially non-trivial.  The user could also have this video
memory mmap'd, which makes it harder to recover from the user.  This
seems like a problem.
 
> > Does it blank the framebuffer?  
> 
> No.
> 
> > Can the dmabuf fd
> > transparently update to the new plane_info?  
> 
> No.

So the user hold a reference to video memory with no idea whether it
will be reused, we have no way to tell them to release that reference
or mechanism to force them to do so... something is wrong here.

> > The other case is a region, the user queries the plane_info records
> > the
> > parameters and region info, and configures access to the region using
> > that information.  Meanwhile, something changed, plane_info including
> > generation is updated, but the user is still assuming the previous
> > plane_info.  How can we avoid such a race?  
> 
> Qemu doesn't.  You might get rendering glitches in that case, due to
> accessing the plane with the wrong configuration.  It's fundamentally
> the same with stdvga btw.
> 
> > What is the responsibility
> > of the user and how are they notified to refresh the plane_info?  
> 
> qemu polls in regular intervals, simliar to how it checks the dirty
> bitmap for video memory in regular intervals with stdvga.
> 
> qemu display update timer runs on 30fps usually, in case nobody is
> looking (no spice/vnc client connected) it reduces the update frequency
> though.
> 
> > > plane_type should be DRM_PLANE_TYPE_PRIMARY or
> > > DRM_PLANE_TYPE_CURSOR
> > > for dmabuf.
> > > 
> > > Given that nvidia doesn't support a separate cursor plane in their
> > > region they would support DRM_PLANE_TYPE_PRIMARY only.
> > > 
> > > I can't see yet what id would be useful for.
> > > 
> > > Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good
> > > for.  
> > 
> > I think we're trying to identify where this plane_info exists.  Does
> > the user ask for a dmabuf fd for it or use a region.  
> 
> But whenever a region or a dmabuf is used is a fixed property of the
> device, why associate that with a plane?  It will be the same for all
> planes of a device anyway ...
> 
> > If it's a region,
> > which region?  
> 
> Ok, if we want support multiple regions.  Do we?  Using the offset we
> can place multiple planes in a single region.  And I'm not sure nvidia
> plans to use multiple planes in the first place ...

I don't want to take a driver ioctl interface as a throw away, one time
use exercise.  If we can think of such questions now, let's define how
they work.  A device could have multiple graphics regions with multiple
planes within each region.  Do we also want to exclude that device
needs to be strictly region or dmabuf?  Maybe it does both.  Or maybe
it supports dmabuf-ng (ie. whatever comes next).

> > 4. Two ioctl commands  
> > > VFIO_DEVICE_QUERY_GFX_PLANE
> > > VFIO_DEVICE_GET_FD  
> > 
> > Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any
> > opinions?  Thanks,  
> 
> I don't think a generic device query is that helpful.  Depending on the
> kind of query you'll get back different stuff, and we need to handle
> that somehow, like this:
> 
> vfio_device_query {
>     u32 argsz;
>     u32 flags;
>     enum query_type;  /* or use flags for that */
>     union {
>         plane_info plane;
>         /* whatever else comes up */
>     } query_params;
> };
> 
> I fail to see how this is fundamentally different from having multiple
> vfio_device_query_* structs (and ioctls).  It only pushes the problem
> one level down ...
> 
> Or do you have something else in mind?

We don't have an infinite number of ioctls and each ioctl defines a
user interface that needs to be supported and maintain an ABI.  So if
we define something like above where we can screw up some of the query
types without throwing away the entire ioctl, I think that's a good
thing.  Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> Is this only going to support accelerated driver output, not basic
> VGA
> modes for BIOS interaction?

Right now there is no vgabios or uefi support for the vgpu.

But even with that in place there still is the problem that the display
device initialization happens before the guest runs and therefore there
isn't an plane yet ...

> > Right now the experimental intel patches throw errors in case no
> > plane
> > exists (yet).  Maybe we should have a "bool is_enabled" field in
> > the
> > plane_info struct, so drivers can use that to signal whenever the
> > guest
> > has programmed a valid video mode or not (likewise for the cursor,
> > which doesn't exist with fbcon, only when running xorg).  With that
> > in
> > place using the QUERY_PLANE ioctl also for probing looks
> > reasonable.
> 
> Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no
> available
> plane, but then that might not help the user know how a plane would
> be
> available if it were available.

So maybe a "enum plane_state" (instead of "bool is_enabled")?  So we
can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?

> > Yes, I'd leave that to userspace.  So, when the generation changes
> > userspace knows the guest changed the plane.  It could be a
> > configuration the guest has used before (and where userspace could
> > have
> > a cached dma-buf handle for), or it could be something new.
> 
> But userspace also doesn't know that a dmabuf generation will ever be
> visited again,

kernel wouldn't know either, only the guest knows ...

> so they're bound to have some stale descriptors.  Are
> we thinking userspace would have some LRU list of dmabufs so that
> they
> don't collect too many?  Each uses some resources,  do we just rely
> on
> open file handles to set an upper limit?

Yep, this is exactly what my qemu patches are doing, keep a LRU list.
 
> > > What happens to
> > > existing dmabuf fds when the generation updates, do they stop
> > > refreshing?  
> > 
> > Depends on what the guest is doing ;)
> > 
> > The dma-buf is just a host-side handle for the piece of video
> > memory
> > where the guest stored the framebuffer.
> 
> So the resources the user is holding if they don't release their
> dmabuf
> are potentially non-trivial.

Not really.  Host IGD has a certain amount of memory, some of it is
assigned to the guest, guest stores the framebuffer there, the dma-buf
is a host handle (drm object, usable for rendering ops) for the guest
framebuffer.  So it doesn't use much ressources.  Some memory is needed
for management structs, but not for the actual data as this in the
video memory dedicated to the guest.

> > Ok, if we want support multiple regions.  Do we?  Using the offset
> > we
> > can place multiple planes in a single region.  And I'm not sure
> > nvidia
> > plans to use multiple planes in the first place ...
> 
> I don't want to take a driver ioctl interface as a throw away, one
> time
> use exercise.  If we can think of such questions now, let's define
> how
> they work.  A device could have multiple graphics regions with
> multiple
> planes within each region.

I'd suggest to settle for one of these two.  Either one region and
multiple planes inside (using offset) or one region per plane.  I'd
prefer the former.  When going for the latter then yes we have to
specify the region.  I'd name the field region_id then to make clear
what it is.

What would be the use case for multiple planes?

cursor support?  We already have plane_type for that.

multihead support?  We'll need (at minimum) a head_id field for that
(for both dma-buf and region)

pageflipping support?  Nothing needed, query_plane will simply return
the currently visible plane.  Region only needs to be big enough to fit
the framebuffer twice.  Then the driver can flip between two buffers,
point to the one qemu should display using "offset".

> Do we also want to exclude that device
> needs to be strictly region or dmabuf?  Maybe it does both.

Very unlikely IMHO.

> Or maybe
> it supports dmabuf-ng (ie. whatever comes next).

Possibly happens some day, but who knows what interfaces we'll need to
support that ...

> > vfio_device_query {
> >     u32 argsz;
> >     u32 flags;
> >     enum query_type;  /* or use flags for that */

> We don't have an infinite number of ioctls

The limited ioctl number space is a good reason indeed.
Ok, lets take this route then.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhi Wang]

Hi:
     Thanks for the discussions! If the userspace application has 
already maintained a LRU list, it looks like we don't need generation 
anymore, as userspace application will lookup the guest framebuffer from 
the LRU list by "offset". No matter how, it would know if this is a new 
guest framebuffer or not. If it's a new guest framebuffer, a new dmabuf 
fd should be generated. If it's an old framebuffer, it can just show 
that framebuffer.

Thanks,
Zhi.

On 06/23/17 15:26, Gerd Hoffmann wrote:
>    Hi,
>
>> Is this only going to support accelerated driver output, not basic
>> VGA
>> modes for BIOS interaction?
> Right now there is no vgabios or uefi support for the vgpu.
>
> But even with that in place there still is the problem that the display
> device initialization happens before the guest runs and therefore there
> isn't an plane yet ...
>
>>> Right now the experimental intel patches throw errors in case no
>>> plane
>>> exists (yet).  Maybe we should have a "bool is_enabled" field in
>>> the
>>> plane_info struct, so drivers can use that to signal whenever the
>>> guest
>>> has programmed a valid video mode or not (likewise for the cursor,
>>> which doesn't exist with fbcon, only when running xorg).  With that
>>> in
>>> place using the QUERY_PLANE ioctl also for probing looks
>>> reasonable.
>> Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no
>> available
>> plane, but then that might not help the user know how a plane would
>> be
>> available if it were available.
> So maybe a "enum plane_state" (instead of "bool is_enabled")?  So we
> can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?
>
>>> Yes, I'd leave that to userspace.  So, when the generation changes
>>> userspace knows the guest changed the plane.  It could be a
>>> configuration the guest has used before (and where userspace could
>>> have
>>> a cached dma-buf handle for), or it could be something new.
>> But userspace also doesn't know that a dmabuf generation will ever be
>> visited again,
> kernel wouldn't know either, only the guest knows ...
>
>> so they're bound to have some stale descriptors.  Are
>> we thinking userspace would have some LRU list of dmabufs so that
>> they
>> don't collect too many?  Each uses some resources,  do we just rely
>> on
>> open file handles to set an upper limit?
> Yep, this is exactly what my qemu patches are doing, keep a LRU list.
>   
>>>> What happens to
>>>> existing dmabuf fds when the generation updates, do they stop
>>>> refreshing?
>>> Depends on what the guest is doing ;)
>>>
>>> The dma-buf is just a host-side handle for the piece of video
>>> memory
>>> where the guest stored the framebuffer.
>> So the resources the user is holding if they don't release their
>> dmabuf
>> are potentially non-trivial.
> Not really.  Host IGD has a certain amount of memory, some of it is
> assigned to the guest, guest stores the framebuffer there, the dma-buf
> is a host handle (drm object, usable for rendering ops) for the guest
> framebuffer.  So it doesn't use much ressources.  Some memory is needed
> for management structs, but not for the actual data as this in the
> video memory dedicated to the guest.
>
>>> Ok, if we want support multiple regions.  Do we?  Using the offset
>>> we
>>> can place multiple planes in a single region.  And I'm not sure
>>> nvidia
>>> plans to use multiple planes in the first place ...
>> I don't want to take a driver ioctl interface as a throw away, one
>> time
>> use exercise.  If we can think of such questions now, let's define
>> how
>> they work.  A device could have multiple graphics regions with
>> multiple
>> planes within each region.
> I'd suggest to settle for one of these two.  Either one region and
> multiple planes inside (using offset) or one region per plane.  I'd
> prefer the former.  When going for the latter then yes we have to
> specify the region.  I'd name the field region_id then to make clear
> what it is.
>
> What would be the use case for multiple planes?
>
> cursor support?  We already have plane_type for that.
>
> multihead support?  We'll need (at minimum) a head_id field for that
> (for both dma-buf and region)
>
> pageflipping support?  Nothing needed, query_plane will simply return
> the currently visible plane.  Region only needs to be big enough to fit
> the framebuffer twice.  Then the driver can flip between two buffers,
> point to the one qemu should display using "offset".
>
>> Do we also want to exclude that device
>> needs to be strictly region or dmabuf?  Maybe it does both.
> Very unlikely IMHO.
>
>> Or maybe
>> it supports dmabuf-ng (ie. whatever comes next).
> Possibly happens some day, but who knows what interfaces we'll need to
> support that ...
>
>>> vfio_device_query {
>>>      u32 argsz;
>>>      u32 flags;
>>>      enum query_type;  /* or use flags for that */
>> We don't have an infinite number of ioctls
> The limited ioctl number space is a good reason indeed.
> Ok, lets take this route then.
>
> cheers,
>    Gerd
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

On Fri, 2017-06-23 at 15:49 +0800, Zhi Wang wrote:
> Hi:
>      Thanks for the discussions! If the userspace application has 
> already maintained a LRU list, it looks like we don't need
> generation 
> anymore,

generation isn't required, things are working just fine without that. 
It is just a small optimization, userspace can skip the LRU lookup
altogether if the generation didn't change.

But of couse that only pays off if the kernel doesn't has to put much
effort into maintaining the generation id.  Something simple like
increasing it each time the guest writes a register which affects
plane_info.

If you think it doesn't make sense from the driver point of view we can
drop the generation.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Fri, 23 Jun 2017 10:31:28 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Fri, 2017-06-23 at 15:49 +0800, Zhi Wang wrote:
> > Hi:
> >      Thanks for the discussions! If the userspace application has 
> > already maintained a LRU list, it looks like we don't need
> > generation 
> > anymore,  
> 
> generation isn't required, things are working just fine without that. 
> It is just a small optimization, userspace can skip the LRU lookup
> altogether if the generation didn't change.
> 
> But of couse that only pays off if the kernel doesn't has to put much
> effort into maintaining the generation id.  Something simple like
> increasing it each time the guest writes a register which affects
> plane_info.

But it seems like that simple management algorithm pretty much
guarantees that the kernel will never revisit a generation and
therefore caching dmabuf fds is pointless.  AIUI the optimization is to
allow userspace to 'at a glance' test one plane_info vs another.  The
user could also do this with a memcmp of the plane_info structs if
that's its only purpose.  A randomly incremented field within that
struct could actually be a hindrance to the user for such a
comparison.  Are there cases where the plane_info struct is otherwise
identical where the user would need to get a new dmabuf fd anyway?
Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Fri, 23 Jun 2017 09:26:59 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > Is this only going to support accelerated driver output, not basic
> > VGA
> > modes for BIOS interaction?  
> 
> Right now there is no vgabios or uefi support for the vgpu.
> 
> But even with that in place there still is the problem that the display
> device initialization happens before the guest runs and therefore there
> isn't an plane yet ...
> 
> > > Right now the experimental intel patches throw errors in case no
> > > plane
> > > exists (yet).  Maybe we should have a "bool is_enabled" field in
> > > the
> > > plane_info struct, so drivers can use that to signal whenever the
> > > guest
> > > has programmed a valid video mode or not (likewise for the cursor,
> > > which doesn't exist with fbcon, only when running xorg).  With that
> > > in
> > > place using the QUERY_PLANE ioctl also for probing looks
> > > reasonable.  
> > 
> > Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no
> > available
> > plane, but then that might not help the user know how a plane would
> > be
> > available if it were available.  
> 
> So maybe a "enum plane_state" (instead of "bool is_enabled")?  So we
> can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?

What's the difference between NOT_SUPPORTED and -ENOTTY on the ioctl?
Perhaps a bit in a flags field could specify EN/DIS-ABLED and leave
room for things we're forgetting.  Keep in mind that we need to use
explicit width fields for a uapi structure, so __u32 vs enum.  Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > So maybe a "enum plane_state" (instead of "bool is_enabled")?  So
> > we
> > can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?
> 
> What's the difference between NOT_SUPPORTED and -ENOTTY on the ioctl?
> Perhaps a bit in a flags field could specify EN/DIS-ABLED and leave
> room for things we're forgetting.

So throw error in the NOT_SUPPORTED case, otherwise set/clear a
PLANE_ENABLED bit in flags?

Yep, that will work too.

cheers,
  Gerd

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Wednesday, June 21, 2017 7:04 PM
> To: Zhang, Tina <tina.zhang@intel.com>; Alex Williamson
> <alex.williamson@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; intel-
> gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.chen@intel.com>; Kirti Wankhede <kwankhede@nvidia.com>; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Wed, 2017-06-21 at 09:20 +0000, Zhang, Tina wrote:
> > Thanks for all the comments. I'm planning to cook the next version of
> > this patch set
> 
> How about posting only this patch instead of the whole series until we've settled
> the interfaces?
OK.

> 
> > Could the following two works?
> > #define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)        /* vfio-dmabuf
> > device */
> 
> VFIO_DEVICE_FLAGS_GFX_DMABUF?
> 
> > 2. vfio_device_gfx_plane_info
> > struct vfio_device_gfx_plane_info {
> > 	__u64 start;-> offset
> > 	__u64 drm_format_mod;
> > 	__u32 drm_format;
> > 	__u32 width;
> > 	__u32 height;
> > 	__u32 stride;
> > 	__u32 size;
> > 	__u32 x_pos;
> > 	__u32 y_pos;
> > };
> > > Does it make sense to have a "generation" field in the plane_info
> > > struct (which gets increased each time the struct changes) ?
> 
> > Well,  Gerd, can you share more details about how to use this field in
> > user mode, so that we can figure out a way to support it? Thanks.
> 
> generation would be increased each time one of the fields in
> vfio_device_gfx_plane_info changes, typically on mode switches (width/height
> changes) and pageflips (offset changes).  So userspace can simply compare
> generation instead of comparing every field to figure whenever something
> changed compared to the previous poll.
Make sense for dma-buf. Thanks.

> 
> >
> > 3. vfio_device_query_gfx_plane
> > struct vfio_device_query_gfx_plane {
> > 	__u32 argsz;
> > 	__u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID	(1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > 	struct vfio_device_gfx_plane_info plane_info;
> > 	__u32 id;
> > 	__u32 plane_type;
> > };
> > So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR.
> 
> 
> >  If the newly added plane_type is used for this, the id field may be
> > useless in dmabuf usage. Do you have any idea about the usage of this
> > id field in dmabuf usage?
> 
> plane_type should be DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR for dmabuf.
> 
> Given that nvidia doesn't support a separate cursor plane in their region they
> would support DRM_PLANE_TYPE_PRIMARY only.
> 
> I can't see yet what id would be useful for.
> 
> Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> We already have VFIO_DEVICE_GET_INFO which returns:
> 
> struct vfio_device_info {
>         __u32   argsz;
>         __u32   flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports
> reset */
> #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform
> device */
> #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device
> */
> #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
>         __u32   num_regions;    /* Max region index + 1 */
>         __u32   num_irqs;       /* Max IRQ index + 1 */
> };
> 
> We could use two flag bits to indicate dmabuf or graphics region
> support.

That works too.

> > Then this to query the plane:
> > 
> > struct vfio_device_gfx_query_plane {
> >         __u32 argsz;
> >         __u32 flags;
> >         struct vfio_device_gfx_plane_info plane_info;  /* out */
> >         __u32 plane_type;                              /* in  */
> > };
> 
> I'm not sure why we're using an enum for something that can currently
> be defined with 2 bits,

We can reuse the DRM_PLANE_TYPE_* then.

>  seems like this would be another good use of
> flags.  We could even embed an enum into the flags if we want to
> leave some expansion room, 4 bits maybe?  Also, I was imagining that
> a
> device could support multiple graphics regions, that's where
> specifying
> the "id" as a region index seemed useful.

Hmm, yes, possibly for multihead configurations.  But I guess for
proper multihead support we would need more than just an region id.

Or do you have something else in mind?

> > With the generation we can also do something different:  Pass in
> > plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD
> > return
> > an error in case the generation doesn't match.  In that case it
> > doesn't
> > make much sense any more to have a separate plane_info struct,
> > which
> > was added so we don't have to duplicate things in query-plane and
> > get-
> > dmabuf ioctl structs.
> 
> I'm not sure I understand how this works for a region, the region is
> always the current generation, how can the user ever be sure the
> plane_info matches what is exposed in the region?

generation will change each time the plane configuration (not content)
changes.  Typically that will be on video mode switches.  In the dmabuf
case also on pageflips.

cheers,
  Gerd

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Tuesday, June 20, 2017 6:58 PM
> To: Zhang, Tina <tina.zhang@intel.com>; Alex Williamson
> <alex.williamson@redhat.com>
> Cc: intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; Kirti
> Wankhede <kwankhede@nvidia.com>; Chen, Xiaoguang
> <xiaoguang.chen@intel.com>; intel-gvt-dev@lists.freedesktop.org; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Wang, Zhenyu
> Z <zhenyu.z.wang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote:
> > Hi,
> >
> > Thanks for all the comments. Here are the summaries:
> >
> > 1. Modify the structures to make it more general.
> > 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;
> > 	__u32 generation;
> > };
> 
> Looks good to me.
> 
> > struct vfio_device_query_gfx_plane {
> > 	__u32 argsz;
> > 	__u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> > 	struct vfio_device_gfx_plane_info plane_info;
> > 	__u32 id;
> > };
> 
> I'm not convinced the flags are a great idea.  Whenever dmabufs or a region is
> used is a static property of the device, not of each individual plane.
> 
> 
> I think we should have this for userspace to figure:
> 
> enum vfio_device_gfx_type {
>         VFIO_DEVICE_GFX_NONE,
>         VFIO_DEVICE_GFX_DMABUF,
>         VFIO_DEVICE_GFX_REGION,
> };
> 
> struct vfio_device_gfx_query_caps {
>         __u32 argsz;
>         __u32 flags;
>         enum vfio_device_gfx_type;
> };
> 
> 
> Then this to query the plane:
> 
> struct vfio_device_gfx_query_plane {
>         __u32 argsz;
>         __u32 flags;
>         struct vfio_device_gfx_plane_info plane_info;  /* out */
>         __u32 plane_type;                              /* in  */
> };
> 
> 
> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device
> fd.
> > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > vfio_device_gfx_plane_info.
> 
> Yes.
> 
> > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
> 
> Yes.  The plane might have changed between query-plane and get-dmabuf ioctl
> calls though, we must make sure we handle that somehow.  Current patches
> return plane_info on get-dmabuf ioctl too, so userspace can see what it actually
> got.
> 
> With the generation we can also do something different:  Pass in plane_type and
> generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case
> the generation doesn't match.  In that case it doesn't make much sense any
> more to have a separate plane_info struct, which was added so we don't have
> to duplicate things in query-plane and get- dmabuf ioctl structs.
Comparing with the current patch, this would make user space a little bit harder to
get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it efficient for
user mode usage?

Thanks
Tina
> 
> cheers,
>   Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > With the generation we can also do something different:  Pass in
> > plane_type and
> > generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in
> > case
> > the generation doesn't match.  In that case it doesn't make much
> > sense any
> > more to have a separate plane_info struct, which was added so we
> > don't have
> > to duplicate things in query-plane and get- dmabuf ioctl structs.
> 
> Comparing with the current patch, this would make user space a little
> bit harder to
> get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it
> efficient for
> user mode usage?

user space has to call QUERY-PLANE first, then looks if it has a dma-
buf for that, if not call GET-DMABUF.

Problem is the guest could have changed the plane between the QUERY-
PLANE and GET-DMABUF ioctls.

Current patches (v8 series) just returns plane-info on GET-DMABUF too,
so userspace can at least detect something changed.

It would be easier for userspace if GET-DMABUF throws an error in case
the plane changed since the last QUERY-PLANE ioctl.  The generation id
would be one way to handle it, but possibly it is easier if the kernel
driver just keeps track internally.  So GET-DMABUF would be defined to
return a dmabuf for the plane returned by the previous QUERY-PLANE
ioctl (on the same file handle), or return an error in case the plane
has changed meanwhile.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Mon, 26 Jun 2017 08:39:17 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > With the generation we can also do something different:  Pass in
> > > plane_type and
> > > generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in
> > > case
> > > the generation doesn't match.  In that case it doesn't make much
> > > sense any
> > > more to have a separate plane_info struct, which was added so we
> > > don't have
> > > to duplicate things in query-plane and get- dmabuf ioctl structs.  
> > 
> > Comparing with the current patch, this would make user space a little
> > bit harder to
> > get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it
> > efficient for
> > user mode usage?  
> 
> user space has to call QUERY-PLANE first, then looks if it has a dma-
> buf for that, if not call GET-DMABUF.
> 
> Problem is the guest could have changed the plane between the QUERY-
> PLANE and GET-DMABUF ioctls.
> 
> Current patches (v8 series) just returns plane-info on GET-DMABUF too,
> so userspace can at least detect something changed.
> 
> It would be easier for userspace if GET-DMABUF throws an error in case
> the plane changed since the last QUERY-PLANE ioctl.  The generation id
> would be one way to handle it, but possibly it is easier if the kernel
> driver just keeps track internally.  So GET-DMABUF would be defined to
> return a dmabuf for the plane returned by the previous QUERY-PLANE
> ioctl (on the same file handle), or return an error in case the plane
> has changed meanwhile.

Hmm, I don't like that interface.  Can you cite examples of other
ioctls that behave this way?  It doesn't feel like an elegant user
interface; the user can get the dmabuf, but only after they query the
dmabuf, even though the get-dmabuf ioctl returns the same data as the
query-plane ioctl, but they can't get the dmabuf if the plane has
changed in the interim, which is not something the user can know.  Are
we causing our own problems with this model of cycling through dmabuf
fds?  We talked previously about an enum of plane types, primary and
cursor.  What if the user was simply able to get a dmabuf fd for each of
those and they queried the current plane information via those fds?
IOW, the fd is persistent and specific to a given plane type, but the
format within it is dynamic.  For instance, I don't have a separate
monitor on my desktop for each resolution I want to run, the monitor
adapts to the signal it gets.  I don't grasp the technical reasons why
the user can't stop using the dmabuf fd with the previous format
parameters and start using it with the new parameters.  Maybe the user
even has multiple dmabuf fds open, but they switch to only actively
using then one(s) that match the current format.  I don't know if
that's viable, but there seems to be a fundamental synchronization
issue if a given dmabuf fd only represents a transient state.  Thanks,

Alex

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> Hmm, I don't like that interface.  Can you cite examples of other
> ioctls that behave this way?  It doesn't feel like an elegant user
> interface; the user can get the dmabuf, but only after they query the
> dmabuf, even though the get-dmabuf ioctl returns the same data as the
> query-plane ioctl, but they can't get the dmabuf if the plane has
> changed in the interim, which is not something the user can
> know.  Are
> we causing our own problems with this model of cycling through dmabuf
> fds?  We talked previously about an enum of plane types, primary and
> cursor.  What if the user was simply able to get a dmabuf fd for each
> of
> those and they queried the current plane information via those fds?
> IOW, the fd is persistent and specific to a given plane type, but the
> format within it is dynamic.

Will not work due to how dma-bufs are designed.

But, yes, the QUERY then GET split is ugly for a number of reasons.

Does gvt track the live cycle of all dma-bufs it has handed out?
If so, then maybe we can let the kernel check whenever a dma-buf for
the current plane exists?  And if that isn't the case hand out a dma-
buf right away, without expecting userspace explicitly asking for it?

That will simplify the interface and remove the race condition at the
expense of some additional bookkeeping in the kernel.

cheers,
  Gerd

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Tuesday, June 27, 2017 2:13 PM
> To: Alex Williamson <alex.williamson@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; intel-
> gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.chen@intel.com>; Zhang, Tina <tina.zhang@intel.com>; Kirti
> Wankhede <kwankhede@nvidia.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
> intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
>   Hi,
> 
> > Hmm, I don't like that interface.  Can you cite examples of other
> > ioctls that behave this way?  It doesn't feel like an elegant user
> > interface; the user can get the dmabuf, but only after they query the
> > dmabuf, even though the get-dmabuf ioctl returns the same data as the
> > query-plane ioctl, but they can't get the dmabuf if the plane has
> > changed in the interim, which is not something the user can know.  Are
> > we causing our own problems with this model of cycling through dmabuf
> > fds?  We talked previously about an enum of plane types, primary and
> > cursor.  What if the user was simply able to get a dmabuf fd for each
> > of those and they queried the current plane information via those fds?
> > IOW, the fd is persistent and specific to a given plane type, but the
> > format within it is dynamic.
> 
> Will not work due to how dma-bufs are designed.
> 
> But, yes, the QUERY then GET split is ugly for a number of reasons.
> 
> Does gvt track the live cycle of all dma-bufs it has handed out?
The V9 implementation does track the dma-bufs' live cycle. The original idea was that leaving the dma-bufs' live cycle management to user mode.

> If so, then maybe we can let the kernel check whenever a dma-buf for the
> current plane exists?  And if that isn't the case hand out a dma- buf right away,
> without expecting userspace explicitly asking for it?
I think this is a good advice. We are going to try this idea and add some tracking logic to kernel mode.

> 
> That will simplify the interface and remove the race condition at the expense of
> some additional bookkeeping in the kernel.
In this case, maybe one ioctl like QUERY_PLAN is enough. We can block it this ioctl and return it when the fd and info are ready.

Thanks.
Tina


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

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Gerd Hoffmann]

  Hi,

> > Does gvt track the live cycle of all dma-bufs it has handed out?
> 
> The V9 implementation does track the dma-bufs' live cycle. The
> original idea was that leaving the dma-bufs' live cycle management to
> user mode.

That is still the case, user space decides which dma-bufs it'll go keep
cached.  But kernel space can see what user space is doing, so there is
no need to explicitly tell the kernel whenever a cached dma-buf exists
or not.

cheers,
  Gerd

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Daniel Vetter]

On Thu, Jun 29, 2017 at 08:41:53AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Does gvt track the live cycle of all dma-bufs it has handed out?
> > 
> > The V9 implementation does track the dma-bufs' live cycle. The
> > original idea was that leaving the dma-bufs' live cycle management to
> > user mode.
> 
> That is still the case, user space decides which dma-bufs it'll go keep
> cached.  But kernel space can see what user space is doing, so there is
> no need to explicitly tell the kernel whenever a cached dma-buf exists
> or not.

We do the same trick in drm_prime.c, keeping a cache of exported dma-buf
around for re-exporting. Since for prime sharing the use-case is almost
always re-importing as a drm gem buffer again we can then on re-import
also tell userspace whether it already has that buffer in it's userspace
buffer manager, but that's an additional optimization. With plain dma-buf
we could achieve the same by wiring up a real stat() implementation with
unique inode numbers (atm they all share the anon_inode singleton). But
thus far no one asked for that.

btw I'm lost a bit in the discussion (was on vacation), but I think all
the concerns I've noticed with the initial rfc have been raised already,
so things look good. I'll check the next rfc once that shows up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Thursday, June 29, 2017 4:39 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; intel-
> gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.chen@intel.com>; Zhang, Tina <tina.zhang@intel.com>; Alex
> Williamson <alex.williamson@redhat.com>; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Kirti Wankhede <kwankhede@nvidia.com>; intel-gvt-
> dev@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Thu, Jun 29, 2017 at 08:41:53AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > Does gvt track the live cycle of all dma-bufs it has handed out?
> > >
> > > The V9 implementation does track the dma-bufs' live cycle. The
> > > original idea was that leaving the dma-bufs' live cycle management
> > > to user mode.
> >
> > That is still the case, user space decides which dma-bufs it'll go
> > keep cached.  But kernel space can see what user space is doing, so
> > there is no need to explicitly tell the kernel whenever a cached
> > dma-buf exists or not.
> 
> We do the same trick in drm_prime.c, keeping a cache of exported dma-buf
> around for re-exporting. Since for prime sharing the use-case is almost always
> re-importing as a drm gem buffer again we can then on re-import also tell
> userspace whether it already has that buffer in it's userspace buffer manager,
> but that's an additional optimization. With plain dma-buf we could achieve the
> same by wiring up a real stat() implementation with unique inode numbers (atm
> they all share the anon_inode singleton). But thus far no one asked for that.

Thanks. I'm going to submit the v10 version of ABI interface.

> 
> btw I'm lost a bit in the discussion (was on vacation), but I think all the concerns
> I've noticed with the initial rfc have been raised already, so things look good. I'll
> check the next rfc once that shows up.
> -Daniel
> --
> 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 v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede]

On 6/19/2017 8:25 PM, Alex Williamson wrote:
> On Mon, 19 Jun 2017 08:38:32 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>>   Hi,
>>
>>> My suggestion was to use vfio device fd for this ioctl and have
>>> dmabuf
>>> mgr fd as member in above query_plane structure, for region type it
>>> would be set to 0.  
>>
>> Region type should be DRM_PLANE_TYPE_PRIMARY
>>
>>> Can't mmap that page to get surface information. There is no way to
>>> synchronize between QEMU reading this mmapped region and vendor
>>> driver
>>> writing it. There could be race condition in these two operations.
>>> Read
>>> on this page should be trapped and blocking, so that surface in that
>>> region is only updated when its asked for.  
>>
>> Does it make sense to have a "generation" field in the plane_info
>> struct (which gets increased each time the struct changes) ?
> 
> It seems less cumbersome than checking each field to see if it has
> changed.  Thanks,
> 

Looks good. And vendor driver should take care of rounding up the value
when it reaches its max limit.

Thanks,
Kirti


> Alex
> 

[PATCH v9 6/7] drm/i915/gvt: Dmabuf support for GVT-g

[Xiaoguang Chen]

dmabuf for GVT-g can be exported to users who can use the dmabuf to show
the desktop of vm which use intel vgpu.

Currently we provide query and create new dmabuf operations.

Users of dmabuf can cache some created dmabufs and related information
such as the framebuffer's address, size, tiling mode, width, height etc.
When refresh the screen first query the currnet vgpu's frambuffer and
compare with the cached ones(address, size, tiling, width, height etc)
if found one then reuse the found dmabuf to gain performance improvment.

If there is no dmabuf created yet or not found in the cached dmabufs then
need to create a new dmabuf. To create a dmabuf first a gem object will
be created and the backing storage of this gem object is the vgpu's
framebuffer(primary/cursor).
Set caching mode, change tiling mode and set domains of this gem object
is not supported.
Then associate this gem object to a dmabuf and export this dmabuf.
A file descriptor will be generated for this dmabuf and this file
descriptor can be sent to user space to do display.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Tested-by: Kechen Lu <kechen.lu@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile      |   2 +-
 drivers/gpu/drm/i915/gvt/dmabuf.c      | 264 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h      |  37 +++++
 drivers/gpu/drm/i915/gvt/gvt.h         |   1 +
 drivers/gpu/drm/i915/i915_gem.c        |  26 +++-
 drivers/gpu/drm/i915/i915_gem_object.h |   9 ++
 drivers/gpu/drm/i915/i915_gem_tiling.c |   5 +
 7 files changed, 342 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index 192ca26..e480f7d 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -2,7 +2,7 @@ GVT_DIR := gvt
 GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
 	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
-	fb_decoder.o
+	fb_decoder.o dmabuf.o
 
 ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
new file mode 100644
index 0000000..6ef4f60
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -0,0 +1,264 @@
+/*
+ * Copyright 2017 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Zhiyuan Lv <zhiyuan.lv@intel.com>
+ *
+ * Contributors:
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ */
+
+#include <linux/dma-buf.h>
+#include <drm/drmP.h>
+#include <linux/vfio.h>
+
+#include "i915_drv.h"
+#include "gvt.h"
+
+#define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
+
+static struct sg_table *intel_vgpu_gem_get_pages(
+		struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+	struct sg_table *st;
+	struct scatterlist *sg;
+	int i, ret;
+	gen8_pte_t __iomem *gtt_entries;
+	struct intel_vgpu_fb_info *fb_info;
+
+	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
+	if (WARN_ON(!fb_info))
+		return ERR_PTR(-ENODEV);
+
+	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return ERR_PTR(-ENOMEM);
+
+	ret = sg_alloc_table(st, fb_info->fb_size, GFP_KERNEL);
+	if (ret) {
+		kfree(st);
+		return ERR_PTR(ret);
+	}
+	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
+		(fb_info->fb_addr >> PAGE_SHIFT);
+	for_each_sg(st->sgl, sg, fb_info->fb_size, i) {
+		sg->offset = 0;
+		sg->length = PAGE_SIZE;
+		sg_dma_address(sg) =
+			GEN8_DECODE_PTE(readq(&gtt_entries[i]));
+		sg_dma_len(sg) = PAGE_SIZE;
+	}
+
+	return st;
+}
+
+static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
+		struct sg_table *pages)
+{
+	sg_free_table(pages);
+	kfree(pages);
+}
+
+static void intel_vgpu_gem_release(struct drm_i915_gem_object *obj)
+{
+	kfree(obj->gvt_info);
+}
+
+static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
+	.flags = I915_GEM_OBJECT_IS_PROXY,
+	.get_pages = intel_vgpu_gem_get_pages,
+	.put_pages = intel_vgpu_gem_put_pages,
+	.release = intel_vgpu_gem_release,
+};
+
+static struct drm_i915_gem_object *intel_vgpu_create_gem(struct drm_device *dev,
+		struct vfio_dmabuf_mgr_plane_info *info)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_gem_object *obj;
+
+	obj = i915_gem_object_alloc(dev_priv);
+	if (obj == NULL)
+		return NULL;
+
+	drm_gem_private_object_init(dev, &obj->base,
+		info->size << PAGE_SHIFT);
+	i915_gem_object_init(obj, &intel_vgpu_gem_ops);
+
+	obj->base.read_domains = I915_GEM_DOMAIN_GTT;
+	obj->base.write_domain = 0;
+	if (IS_SKYLAKE(dev_priv)) {
+		unsigned int tiling_mode = 0;
+		unsigned int stride = 0;
+
+		switch (info->drm_format_mod << 10) {
+		case PLANE_CTL_TILED_LINEAR:
+			tiling_mode = I915_TILING_NONE;
+			break;
+		case PLANE_CTL_TILED_X:
+			tiling_mode = I915_TILING_X;
+			stride = info->stride;
+			break;
+		case PLANE_CTL_TILED_Y:
+			tiling_mode = I915_TILING_Y;
+			stride = info->stride;
+			break;
+		default:
+			gvt_dbg_core("not supported tiling mode\n");
+		}
+		obj->tiling_and_stride = tiling_mode | stride;
+	} else {
+		obj->tiling_and_stride = info->drm_format_mod ?
+					I915_TILING_X : 0;
+	}
+
+	return obj;
+}
+
+static int intel_vgpu_get_plane_info(struct drm_device *dev,
+		struct intel_vgpu *vgpu,
+		struct vfio_dmabuf_mgr_plane_info *info,
+		int plane_id)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_vgpu_primary_plane_format p;
+	struct intel_vgpu_cursor_plane_format c;
+	int ret;
+
+	if (plane_id == DRM_PLANE_TYPE_PRIMARY) {
+		ret = intel_vgpu_decode_primary_plane(vgpu, &p);
+		if (ret)
+			return ret;
+		info->start = p.base;
+		info->width = p.width;
+		info->height = p.height;
+		info->stride = p.stride;
+		info->drm_format = p.drm_format;
+		info->drm_format_mod = p.tiled;
+		info->size = (((p.stride * p.height * p.bpp) / 8) +
+				(PAGE_SIZE - 1)) >> PAGE_SHIFT;
+	} else if (plane_id == DRM_PLANE_TYPE_CURSOR) {
+		ret = intel_vgpu_decode_cursor_plane(vgpu, &c);
+		if (ret)
+			return ret;
+		info->start = c.base;
+		info->width = c.width;
+		info->height = c.height;
+		info->stride = c.width * (c.bpp / 8);
+		info->drm_format = c.drm_format;
+		info->drm_format_mod = 0;
+		info->x_pos = c.x_pos;
+		info->y_pos = c.y_pos;
+		info->size = (((info->stride * c.height * c.bpp) / 8)
+				+ (PAGE_SIZE - 1)) >> PAGE_SHIFT;
+	} else {
+		gvt_vgpu_err("invalid plane id:%d\n", plane_id);
+		return -EINVAL;
+	}
+
+	if (info->size == 0) {
+		gvt_vgpu_err("fb size is zero\n");
+		return -EINVAL;
+	}
+
+	if (info->start & (PAGE_SIZE - 1)) {
+		gvt_vgpu_err("Not aligned fb address:0x%llx\n", info->start);
+		return -EFAULT;
+	}
+	if (((info->start >> PAGE_SHIFT) + info->size) >
+		ggtt_total_entries(&dev_priv->ggtt)) {
+		gvt_vgpu_err("Invalid GTT offset or size\n");
+		return -EFAULT;
+	}
+
+	if (!intel_gvt_ggtt_validate_range(vgpu, info->start, info->size)) {
+		gvt_vgpu_err("invalid gma addr\n");
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)
+{
+	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
+	struct vfio_dmabuf_mgr_query_plane *plane = args;
+
+	return intel_vgpu_get_plane_info(dev, vgpu, &plane->plane_info,
+					plane->plane_id);
+}
+
+int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
+{
+	struct dma_buf *dmabuf;
+	struct drm_i915_gem_object *obj;
+	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
+	struct vfio_dmabuf_mgr_create_dmabuf *gvt_dmabuf = args;
+	struct intel_vgpu_fb_info *fb_info;
+	int ret;
+
+	ret = intel_vgpu_get_plane_info(dev, vgpu, &gvt_dmabuf->plane_info,
+					gvt_dmabuf->plane_id);
+	if (ret != 0)
+		return ret;
+
+	obj = intel_vgpu_create_gem(dev, &gvt_dmabuf->plane_info);
+	if (obj == NULL) {
+		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
+		return -ENOMEM;
+	}
+	fb_info = kmalloc(sizeof(*fb_info), GFP_KERNEL);
+	if (!fb_info) {
+		gvt_vgpu_err("allocate intel vgpu fb info failed\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+	fb_info->fb_addr = gvt_dmabuf->plane_info.start;
+	fb_info->fb_size = gvt_dmabuf->plane_info.size;
+	fb_info->vgpu = vgpu;
+	obj->gvt_info = fb_info;
+
+	dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
+
+	if (IS_ERR(dmabuf)) {
+		gvt_vgpu_err("export dma-buf failed\n");
+		ret = PTR_ERR(dmabuf);
+		goto out_free;
+	}
+
+	ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
+	if (ret < 0) {
+		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
+		goto out_free;
+	}
+
+	gvt_dmabuf->fd = ret;
+
+	return 0;
+out_free:
+	kfree(fb_info);
+out:
+	i915_gem_object_put(obj);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h b/drivers/gpu/drm/i915/gvt/dmabuf.h
new file mode 100644
index 0000000..8be9979
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#ifndef _GVT_DMABUF_H_
+#define _GVT_DMABUF_H_
+
+struct intel_vgpu_fb_info {
+	struct intel_vgpu *vgpu;
+	uint32_t fb_addr;
+	uint32_t fb_size;
+};
+
+int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);
+int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index c42266c..763a8c5 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -47,6 +47,7 @@
 #include "render.h"
 #include "cmd_parser.h"
 #include "fb_decoder.h"
+#include "dmabuf.h"
 
 #define GVT_MAX_VGPU 8
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0c1cbe9..f91342b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1609,6 +1609,12 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* proxy gem object does not support setting domain */
+	if (i915_gem_object_is_proxy(obj)) {
+		err = -EPERM;
+		goto out;
+	}
+
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1677,6 +1683,12 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* proxy gem obj does not support this operation */
+	if (i915_gem_object_is_proxy(obj)) {
+		i915_gem_object_put(obj);
+		return -EPERM;
+	}
+
 	/* Pinned buffers may be scanout, so flush the cache */
 	i915_gem_object_flush_if_display(obj);
 	i915_gem_object_put(obj);
@@ -1727,7 +1739,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	 */
 	if (!obj->base.filp) {
 		i915_gem_object_put(obj);
-		return -EINVAL;
+		return -EPERM;
 	}
 
 	addr = vm_mmap(obj->base.filp, 0, args->size,
@@ -3717,6 +3729,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* proxy gem obj does not support setting caching mode */
+	if (i915_gem_object_is_proxy(obj)) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	if (obj->cache_level == level)
 		goto out;
 
@@ -4168,6 +4186,12 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* proxy gem obj does not support changing backding storage */
+	if (i915_gem_object_is_proxy(obj)) {
+		err = -EPERM;
+		goto out;
+	}
+
 	err = mutex_lock_interruptible(&obj->mm.lock);
 	if (err)
 		goto out;
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 174cf92..be18a43 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
 	unsigned int flags;
 #define I915_GEM_OBJECT_HAS_STRUCT_PAGE 0x1
 #define I915_GEM_OBJECT_IS_SHRINKABLE   0x2
+#define I915_GEM_OBJECT_IS_PROXY	0x4
 
 	/* Interface between the GEM object and its backing storage.
 	 * get_pages() is called once prior to the use of the associated set
@@ -184,6 +185,8 @@ struct drm_i915_gem_object {
 		} userptr;
 
 		unsigned long scratch;
+
+		void *gvt_info;
 	};
 
 	/** for phys allocated objects */
@@ -286,6 +289,12 @@ i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
 }
 
 static inline bool
+i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj)
+{
+	return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY;
+}
+
+static inline bool
 i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
 {
 	return obj->active_count;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index a0d6d43..2ecf5d7 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -345,6 +345,11 @@ i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	if (i915_gem_object_is_proxy(obj)) {
+		err = -EPERM;
+		goto err;
+	}
+
 	if (!i915_tiling_ok(obj, args->tiling_mode, args->stride)) {
 		err = -EINVAL;
 		goto err;
-- 
2.7.4

[PATCH v9 7/7] drm/i915/gvt: Adding user interface for dma-buf

[Xiaoguang Chen]

User space should create the management fd for the dma-buf operation first.
Then user can query the plane information and create dma-buf if necessary
using the management fd.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Tested-by: Kechen Lu <kechen.lu@intel.com>
---
 drivers/gpu/drm/i915/gvt/dmabuf.c    |  45 +++++++++++-
 drivers/gpu/drm/i915/gvt/dmabuf.h    |   5 ++
 drivers/gpu/drm/i915/gvt/gvt.c       |   3 +
 drivers/gpu/drm/i915/gvt/gvt.h       |   6 ++
 drivers/gpu/drm/i915/gvt/hypercall.h |   3 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 136 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/mpt.h       |  30 ++++++++
 drivers/gpu/drm/i915/gvt/vgpu.c      |   2 +
 8 files changed, 229 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 6ef4f60..a6a6f6d 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -81,6 +81,31 @@ static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
 
 static void intel_vgpu_gem_release(struct drm_i915_gem_object *obj)
 {
+	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
+	struct intel_vgpu_fb_info *fb_info;
+	struct intel_vgpu *vgpu;
+	struct list_head *pos;
+
+	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
+	if (WARN_ON(!fb_info || !fb_info->vgpu)) {
+		gvt_vgpu_err("gvt info is invalid\n");
+		goto out;
+	}
+
+	vgpu = fb_info->vgpu;
+	mutex_lock(&vgpu->dmabuf_list_lock);
+	list_for_each(pos, &vgpu->dmabuf_obj_list_head) {
+		dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
+						list);
+		if ((dmabuf_obj != NULL) && (dmabuf_obj->obj == obj)) {
+			kfree(dmabuf_obj);
+			list_del(pos);
+			break;
+		}
+	}
+	mutex_unlock(&vgpu->dmabuf_list_lock);
+	intel_gvt_hypervisor_put_vfio_device(vgpu);
+out:
 	kfree(obj->gvt_info);
 }
 
@@ -216,6 +241,7 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
 	struct vfio_dmabuf_mgr_create_dmabuf *gvt_dmabuf = args;
 	struct intel_vgpu_fb_info *fb_info;
 	int ret;
+	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
 
 	ret = intel_vgpu_get_plane_info(dev, vgpu, &gvt_dmabuf->plane_info,
 					gvt_dmabuf->plane_id);
@@ -238,6 +264,18 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
 	fb_info->vgpu = vgpu;
 	obj->gvt_info = fb_info;
 
+	dmabuf_obj = kmalloc(sizeof(*dmabuf_obj), GFP_KERNEL);
+	if (!dmabuf_obj) {
+		gvt_vgpu_err("alloc dmabuf_obj failed\n");
+		ret = -ENOMEM;
+		goto out_free_info;
+	}
+	dmabuf_obj->obj = obj;
+	INIT_LIST_HEAD(&dmabuf_obj->list);
+	mutex_lock(&vgpu->dmabuf_list_lock);
+	list_add_tail(&dmabuf_obj->list, &vgpu->dmabuf_obj_list_head);
+	mutex_unlock(&vgpu->dmabuf_list_lock);
+
 	dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
 
 	if (IS_ERR(dmabuf)) {
@@ -251,11 +289,16 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
 		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
 		goto out_free;
 	}
-
+	if (intel_gvt_hypervisor_get_vfio_device(vgpu)) {
+		gvt_vgpu_err("get vfio device failed\n");
+		goto out_free;
+	}
 	gvt_dmabuf->fd = ret;
 
 	return 0;
 out_free:
+	kfree(dmabuf_obj);
+out_free_info:
 	kfree(fb_info);
 out:
 	i915_gem_object_put(obj);
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h b/drivers/gpu/drm/i915/gvt/dmabuf.h
index 8be9979..cafa781 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.h
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
@@ -31,6 +31,11 @@ struct intel_vgpu_fb_info {
 	uint32_t fb_size;
 };
 
+struct intel_vgpu_dmabuf_obj {
+	struct drm_i915_gem_object *obj;
+	struct list_head list;
+};
+
 int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);
 int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args);
 
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index 2032917..d589830 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -54,6 +54,9 @@ static const struct intel_gvt_ops intel_gvt_ops = {
 	.vgpu_reset = intel_gvt_reset_vgpu,
 	.vgpu_activate = intel_gvt_activate_vgpu,
 	.vgpu_deactivate = intel_gvt_deactivate_vgpu,
+	.vgpu_query_plane = intel_vgpu_query_plane,
+	.vgpu_create_dmabuf = intel_vgpu_create_dmabuf,
+
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 763a8c5..df7e216 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -185,8 +185,12 @@ struct intel_vgpu {
 		struct kvm *kvm;
 		struct work_struct release_work;
 		atomic_t released;
+		struct vfio_device *vfio_device;
 	} vdev;
 #endif
+	atomic_t mgr_fd_opened;
+	struct list_head dmabuf_obj_list_head;
+	struct mutex dmabuf_list_lock;
 };
 
 struct intel_gvt_gm {
@@ -467,6 +471,8 @@ struct intel_gvt_ops {
 	void (*vgpu_reset)(struct intel_vgpu *);
 	void (*vgpu_activate)(struct intel_vgpu *);
 	void (*vgpu_deactivate)(struct intel_vgpu *);
+	int (*vgpu_query_plane)(struct intel_vgpu *vgpu, void *);
+	int (*vgpu_create_dmabuf)(struct intel_vgpu *vgpu, void *);
 };
 
 
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 32c345c..4f2161c 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -56,6 +56,9 @@ struct intel_gvt_mpt {
 	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
 			     bool map);
 	int (*set_opregion)(void *vgpu);
+	int (*get_vfio_device)(void *vgpu);
+	void (*put_vfio_device)(void *vgpu);
+
 };
 
 extern struct intel_gvt_mpt xengt_mpt;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 6b4652a..9f71325 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -41,6 +41,7 @@
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/anon_inodes.h>
 
 #include "i915_drv.h"
 #include "gvt.h"
@@ -523,6 +524,98 @@ static int kvmgt_set_opregion(void *p_vgpu)
 	return ret;
 }
 
+static int kvmgt_get_vfio_device(void *vgpu)
+{
+	if (!vfio_device_get_from_dev(
+			mdev_dev(((struct intel_vgpu *)vgpu)->vdev.mdev)))
+		return -ENODEV;
+
+	return 0;
+}
+
+static void kvmgt_put_vfio_device(void *vgpu)
+{
+	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
+		return;
+
+	vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
+}
+
+static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file,
+		struct vm_area_struct *vma)
+{
+	return -EPERM;
+}
+
+static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
+		struct file *filp)
+{
+	struct intel_vgpu *vgpu = filp->private_data;
+	struct intel_vgpu_dmabuf_obj *obj;
+	struct list_head *pos;
+
+	list_for_each(pos, &vgpu->dmabuf_obj_list_head) {
+		obj = container_of(pos, struct intel_vgpu_dmabuf_obj, list);
+		if (WARN_ON(!obj))
+			return -ENODEV;
+		i915_gem_object_put(obj->obj);
+	}
+	kvmgt_put_vfio_device(vgpu);
+	atomic_set(&vgpu->mgr_fd_opened, 0);
+
+	return 0;
+}
+
+static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
+		unsigned int ioctl, unsigned long arg)
+{
+	struct intel_vgpu *vgpu = filp->private_data;
+	int minsz;
+	int ret = 0;
+
+	if (ioctl == VFIO_DMABUF_MGR_QUERY_PLANE) {
+		struct vfio_dmabuf_mgr_query_plane plane_info;
+
+		minsz = offsetofend(struct vfio_dmabuf_mgr_query_plane,
+					plane_id);
+		if (copy_from_user(&plane_info, (void __user *)arg, minsz))
+			return -EFAULT;
+		if (plane_info.argsz < minsz || plane_info.flags != 0)
+			return -EINVAL;
+		ret = intel_gvt_ops->vgpu_query_plane(vgpu, &plane_info);
+		if (ret != 0) {
+			gvt_vgpu_err("query plane failed:%d\n", ret);
+			return -EINVAL;
+		}
+		return copy_to_user((void __user *)arg, &plane_info, minsz) ?
+								-EFAULT : 0;
+	} else if (ioctl == VFIO_DMABUF_MGR_CREATE_DMABUF) {
+		struct vfio_dmabuf_mgr_create_dmabuf dmabuf;
+
+		minsz = offsetofend(struct vfio_dmabuf_mgr_create_dmabuf, fd);
+		if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+			return -EFAULT;
+		if (dmabuf.argsz < minsz || dmabuf.flags != 0)
+			return -EINVAL;
+
+		ret = intel_gvt_ops->vgpu_create_dmabuf(vgpu, &dmabuf);
+		if (ret != 0)
+			return ret;
+
+		return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
+								-EFAULT : 0;
+	} else
+		gvt_vgpu_err("unsupported dmabuf mgr fd operation\n");
+
+	return -EINVAL;
+}
+
+static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
+	.release        = intel_vgpu_dmabuf_mgr_fd_release,
+	.unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
+	.mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
+	.llseek         = noop_llseek,
+};
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -530,6 +623,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	struct device *pdev;
 	void *gvt;
 	int ret;
+	struct vfio_device *device;
 
 	pdev = mdev_parent_dev(mdev);
 	gvt = kdev_to_i915(pdev)->gvt;
@@ -554,6 +648,14 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	vgpu->vdev.mdev = mdev;
 	mdev_set_drvdata(mdev, vgpu);
 
+	device = vfio_device_get_from_dev(mdev_dev(mdev));
+	if (device == NULL) {
+		gvt_vgpu_err("get vfio device failed\n");
+		ret = -ENODEV;
+		goto out;
+	}
+	vgpu->vdev.vfio_device = device;
+
 	gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
@@ -1249,6 +1351,36 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_RESET) {
 		intel_gvt_ops->vgpu_reset(vgpu);
 		return 0;
+	} else if (cmd == VFIO_DEVICE_GET_FD) {
+		int fd;
+		u32 type;
+		int ret;
+
+		if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+			return -EINVAL;
+		if (type != VFIO_DEVICE_DMABUF_MGR_FD)
+			return -EINVAL;
+
+		if (atomic_read(&vgpu->mgr_fd_opened)) {
+			gvt_vgpu_err("mgr fd already opened\n");
+			return -EPERM;
+		}
+
+		ret = kvmgt_get_vfio_device(vgpu);
+		if (ret != 0)
+			return ret;
+
+		fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd",
+			&intel_vgpu_dmabuf_mgr_fd_ops,
+			vgpu, O_RDWR | O_CLOEXEC);
+		if (fd < 0) {
+			kvmgt_put_vfio_device(vgpu);
+			gvt_vgpu_err("create dmabuf mgr fd failed\n");
+			return -EINVAL;
+		}
+		atomic_cmpxchg(&vgpu->mgr_fd_opened, 0, 1);
+
+		return fd;
 	}
 
 	return 0;
@@ -1470,6 +1602,7 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
 
 	kvmgt_protect_table_init(info);
 	gvt_cache_init(vgpu);
+	mutex_init(&vgpu->dmabuf_list_lock);
 
 	info->track_node.track_write = kvmgt_page_track_write;
 	info->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
@@ -1612,6 +1745,9 @@ struct intel_gvt_mpt kvmgt_mpt = {
 	.write_gpa = kvmgt_write_gpa,
 	.gfn_to_mfn = kvmgt_gfn_to_pfn,
 	.set_opregion = kvmgt_set_opregion,
+	.get_vfio_device = kvmgt_get_vfio_device,
+	.put_vfio_device = kvmgt_put_vfio_device,
+
 };
 EXPORT_SYMBOL_GPL(kvmgt_mpt);
 
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index ab71300..12dabfc 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -308,4 +308,34 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
 	return intel_gvt_host.mpt->set_opregion(vgpu);
 }
 
+/**
+ * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_get_vfio_device(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->get_vfio_device)
+		return 0;
+
+	return intel_gvt_host.mpt->get_vfio_device(vgpu);
+}
+
+/**
+ * intel_gvt_hypervisor_put_vfio_device - decrease vfio device ref count
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline void intel_gvt_hypervisor_put_vfio_device(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->put_vfio_device)
+		return;
+
+	intel_gvt_host.mpt->put_vfio_device(vgpu);
+}
+
 #endif /* _GVT_MPT_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 8e1d504..8747613 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -346,6 +346,8 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
 	vgpu->gvt = gvt;
 	vgpu->sched_ctl.weight = param->weight;
 	bitmap_zero(vgpu->tlb_handle_pending, I915_NUM_ENGINES);
+	INIT_LIST_HEAD(&vgpu->dmabuf_obj_list_head);
+	atomic_set(&vgpu->mgr_fd_opened, 0);
 
 	intel_vgpu_init_cfg_space(vgpu, param->primary);
 
-- 
2.7.4