2017-06-09 06:52:09

by Xiaoguang Chen

Subject: [PATCH v8 0/6] drm/i915/gvt: Dma-buf support for GVT-g

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 (6):
drm/i915/gvt: Extend the GVT-g architecture to support vfio device
region
drm/i915/gvt: OpRegion support for GVT-g
drm/i915/gvt: Frame buffer decoder support for GVT-g
vfio: Define vfio based vgpu's 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 | 298 +++++++++++++++++++++++
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 | 7 +
drivers/gpu/drm/i915/gvt/hypercall.h | 4 +
drivers/gpu/drm/i915/gvt/kvmgt.c | 254 +++++++++++++++++++-
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 | 6 +
include/uapi/linux/vfio.h | 58 +++++
19 files changed, 1380 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

2017-06-09 06:52:13

by Xiaoguang Chen

Subject: [PATCH v8 1/6] drm/i915/gvt: Extend the GVT-g architecture to support vfio device region

Signed-off-by: Xiaoguang Chen <[email protected]>
---
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

2017-06-09 06:52:20

by Xiaoguang Chen

Subject: [PATCH v8 2/6] drm/i915/gvt: OpRegion support for GVT-g

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 <[email protected]>
Signed-off-by: Xiaoguang Chen <[email protected]>
---
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

2017-06-09 06:52:29

by Xiaoguang Chen

Subject: [PATCH v8 5/6] drm/i915/gvt: Dmabuf support for GVT-g

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 <[email protected]>
Tested-by: Kechen Lu <[email protected]>
---
drivers/gpu/drm/i915/gvt/Makefile | 2 +-
drivers/gpu/drm/i915/gvt/dmabuf.c | 263 +++++++++++++++++++++++++++++++++
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, 341 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..c38ed8e9fc
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -0,0 +1,263 @@
+/*
+ * 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 <[email protected]>
+ *
+ * Contributors:
+ * Xiaoguang Chen <[email protected]>
+ */
+
+#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_vgpu_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_vgpu_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_vgpu_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_vgpu_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

2017-06-09 06:52:31

by Xiaoguang Chen

Subject: [PATCH v8 6/6] drm/i915/gvt: Adding user interface for dma-buf

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 <[email protected]>
Tested-by: Kechen Lu <[email protected]>
---
drivers/gpu/drm/i915/gvt/dmabuf.c | 37 ++++++++-
drivers/gpu/drm/i915/gvt/dmabuf.h | 5 ++
drivers/gpu/drm/i915/gvt/gvt.c | 3 +
drivers/gpu/drm/i915/gvt/gvt.h | 5 ++
drivers/gpu/drm/i915/gvt/hypercall.h | 3 +
drivers/gpu/drm/i915/gvt/kvmgt.c | 145 +++++++++++++++++++++++++++++++++++
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 c38ed8e9fc..1811abd 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -81,6 +81,28 @@ 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 = NULL;
+ 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;
+ 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);
+ break;
+ }
+ }
+ intel_gvt_hypervisor_put_vfio_device(vgpu);
+out:
kfree(obj->gvt_info);
}

@@ -215,6 +237,7 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
struct vfio_vgpu_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);
@@ -237,6 +260,16 @@ 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);
+ list_add_tail(&dmabuf_obj->list, &vgpu->dmabuf_obj_list_head);
+
dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);

if (IS_ERR(dmabuf)) {
@@ -250,11 +283,13 @@ 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;
}
-
+ intel_gvt_hypervisor_get_vfio_device(vgpu);
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..8f08c42 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -185,8 +185,11 @@ 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 intel_gvt_gm {
@@ -467,6 +470,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..8f29c23 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)(unsigned long handle);
+ void (*put_vfio_device)(unsigned long handle);
+
};

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..e1d3492 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,117 @@ static int kvmgt_set_opregion(void *p_vgpu)
return ret;
}

+static int kvmgt_get_vfio_device(unsigned long handle)
+{
+ struct kvmgt_guest_info *info;
+ struct intel_vgpu *vgpu;
+ struct vfio_device *device;
+
+ if (!handle_valid(handle))
+ return -ESRCH;
+
+ info = (struct kvmgt_guest_info *)handle;
+ vgpu = info->vgpu;
+
+ device = vfio_device_get_from_dev(mdev_dev(vgpu->vdev.mdev));
+ if (device == NULL)
+ return -ENODEV;
+ vgpu->vdev.vfio_device = device;
+
+ return 0;
+}
+
+static void kvmgt_put_vfio_device(unsigned long handle)
+{
+ struct kvmgt_guest_info *info;
+ struct intel_vgpu *vgpu;
+
+ if (!handle_valid(handle))
+ return;
+
+ info = (struct kvmgt_guest_info *)handle;
+ vgpu = info->vgpu;
+
+ vfio_device_put(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;
+
+ if (WARN_ON(!vgpu->vdev.vfio_device))
+ return -ENODEV;
+
+ 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->handle);
+ 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_DEVICE_QUERY_PLANE) {
+ struct vfio_vgpu_query_plane plane_info;
+
+ minsz = offsetofend(struct vfio_vgpu_query_plane, padding);
+ 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_DEVICE_CREATE_DMABUF) {
+ struct vfio_vgpu_create_dmabuf dmabuf;
+
+ minsz = offsetofend(struct vfio_vgpu_create_dmabuf, plane_id);
+ 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 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;
@@ -1249,6 +1361,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 (atomic_read(&vgpu->mgr_fd_opened)) {
+ gvt_vgpu_err("mgr fd already opened\n");
+ return -EPERM;
+ }
+
+ if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+ return -EINVAL;
+ if (type != VFIO_DEVICE_DMABUF_MGR_FD)
+ return -EINVAL;
+
+ ret = kvmgt_get_vfio_device(vgpu->handle);
+ 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->handle);
+ gvt_vgpu_err("create dmabuf mgr fd failed\n");
+ return -EINVAL;
+ }
+ atomic_inc(&vgpu->mgr_fd_opened);
+
+ return fd;
}

return 0;
@@ -1612,6 +1754,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..1be961e 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->handle);
+}
+
+/**
+ * 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->handle);
+}
+
#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

2017-06-09 06:52:57

by Xiaoguang Chen

Subject: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

decode frambuffer attributes of primary, cursor and sprite plane

Signed-off-by: Xiaoguang Chen <[email protected]>
---
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 +
include/uapi/drm/drm_fourcc.h | 6 +
7 files changed, 608 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..beac70b
--- /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 <[email protected]>
+ *
+ * Contributors:
+ * Bing Niu <[email protected]>
+ * Xu Han <[email protected]>
+ * Ping Gao <[email protected]>
+ * Xiaoguang Chen <[email protected]>
+ * Yang Liu <[email protected]>
+ *
+ */
+
+#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_GVT, 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_GVT, 64,
+ "64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"},
+ {DRM_FORMAT_XBGR161616_GVT, 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 (!skl_pixel_formats[fmt].bpp && !bdw_pixel_formats[fmt].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_GVT, 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 <[email protected]>
+ *
+ * Contributors:
+ * Bing Niu <[email protected]>
+ * Xu Han <[email protected]>
+ * Ping Gao <[email protected]>
+ * Xiaoguang Chen <[email protected]>
+ * Yang Liu <[email protected]>
+ *
+ */
+
+#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

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 55e3010..400759f 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -161,6 +161,12 @@ extern "C" {
#define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */
#define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */

+/*
+ * Intel GVT-g plane format definition
+ */
+#define DRM_FORMAT_XRGB161616_GVT fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
+#define DRM_FORMAT_XBGR161616_GVT fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */
+

/*
* Format Modifiers:
--
2.7.4

2017-06-09 06:53:09

by Xiaoguang Chen

Subject: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

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 <[email protected]>
---
include/uapi/linux/vfio.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..24427b7 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,64 @@ 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)
+
+struct vfio_vgpu_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;
+};
+
+#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */
+
+/*
+ * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
+ * struct vfio_vgpu_query_plane)
+ * Query plane information
+ */
+struct vfio_vgpu_query_plane {
+ __u32 argsz;
+ __u32 flags;
+ struct vfio_vgpu_plane_info plane_info;
+ __u32 plane_id;
+ __u32 padding;
+};
+
+#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
+
+/*
+ * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
+ * struct vfio_vgpu_create_dmabuf)
+ *
+ * Create a dma-buf for a plane
+ */
+struct vfio_vgpu_create_dmabuf {
+ __u32 argsz;
+ __u32 flags;
+ struct vfio_vgpu_plane_info plane_info;
+ __s32 fd;
+ __u32 plane_id;
+};
+
+#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
+
/* -------- API for Type1 VFIO IOMMU -------- */

/**
--
2.7.4

2017-06-13 21:25:05

by Alex Williamson

Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

On Fri, 9 Jun 2017 14:50:40 +0800
Xiaoguang Chen <[email protected]> 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.
>
> Signed-off-by: Xiaoguang Chen <[email protected]>
> ---
> include/uapi/linux/vfio.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..24427b7 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,64 @@ 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)
> +
> +struct vfio_vgpu_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;
> +};
> +
> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */

Move this #define up above vfio_vgpu_plane_info to associate it with the
VFIO_DEVICE_GET_FD ioctl.

> +
> +/*
> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
> + * struct vfio_vgpu_query_plane)
> + * Query plane information
> + */
> +struct vfio_vgpu_query_plane {
> + __u32 argsz;
> + __u32 flags;
> + struct vfio_vgpu_plane_info plane_info;
> + __u32 plane_id;
> + __u32 padding;

This padding doesn't make sense.

> +};
> +
> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> +
> +/*
> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
> + * struct vfio_vgpu_create_dmabuf)
> + *
> + * Create a dma-buf for a plane
> + */
> +struct vfio_vgpu_create_dmabuf {
> + __u32 argsz;
> + __u32 flags;
> + struct vfio_vgpu_plane_info plane_info;
> + __s32 fd;
> + __u32 plane_id;
> +};

Both of these have a plane_id, should plane_id simply replace the
padding in plane_info? If not, let's at least put them in the same
order so that plane_id is after plane_info for both structs.

> +
> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)

I don't think these should be named just VFIO_DEVICE_foo, that implies
they're ioctls on the vfio device fd, they're not. They need to be
associated both in name and more complete descriptions as ioctls to
the fd returned from a request for a VFIO_DEVICE_DMABUF_MGR_FD. Perhaps
VFIO_DMABUF_MGR_QUERY_PLANE and VFIO_DMABUF_MGR_CREATE_DMABUF. I'm
also not sure why we're using "vgpu" in the structure names here either,
the ioctls aren't named after vgpus. Aren't these rather generic to
graphics dmabufs, not specifically vgpus? Thanks,

Alex

> +
> /* -------- API for Type1 VFIO IOMMU -------- */
>
> /**

2017-06-13 22:29:27

by Alex Williamson

Subject: Re: [PATCH v8 6/6] drm/i915/gvt: Adding user interface for dma-buf

On Fri, 9 Jun 2017 14:50:42 +0800
Xiaoguang Chen <[email protected]> wrote:

> 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 <[email protected]>
> Tested-by: Kechen Lu <[email protected]>
> ---
> drivers/gpu/drm/i915/gvt/dmabuf.c | 37 ++++++++-
> drivers/gpu/drm/i915/gvt/dmabuf.h | 5 ++
> drivers/gpu/drm/i915/gvt/gvt.c | 3 +
> drivers/gpu/drm/i915/gvt/gvt.h | 5 ++
> drivers/gpu/drm/i915/gvt/hypercall.h | 3 +
> drivers/gpu/drm/i915/gvt/kvmgt.c | 145 +++++++++++++++++++++++++++++++++++
> 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 c38ed8e9fc..1811abd 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -81,6 +81,28 @@ 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 = NULL;

unnecessary initialization

> + 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;
> + 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);
> + break;

Did we intend to remove it from the list?

> + }
> + }
> + intel_gvt_hypervisor_put_vfio_device(vgpu);
> +out:
> kfree(obj->gvt_info);
> }
>
> @@ -215,6 +237,7 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
> struct vfio_vgpu_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);
> @@ -237,6 +260,16 @@ 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);
> + list_add_tail(&dmabuf_obj->list, &vgpu->dmabuf_obj_list_head);

Does this list need protection?

> +
> dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
>
> if (IS_ERR(dmabuf)) {
> @@ -250,11 +283,13 @@ 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;
> }
> -
> + intel_gvt_hypervisor_get_vfio_device(vgpu);
> 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..8f08c42 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -185,8 +185,11 @@ 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 intel_gvt_gm {
> @@ -467,6 +470,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..8f29c23 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)(unsigned long handle);
> + void (*put_vfio_device)(unsigned long handle);
> +
> };
>
> 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..e1d3492 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,117 @@ static int kvmgt_set_opregion(void *p_vgpu)
> return ret;
> }
>
> +static int kvmgt_get_vfio_device(unsigned long handle)
> +{
> + struct kvmgt_guest_info *info;
> + struct intel_vgpu *vgpu;
> + struct vfio_device *device;
> +
> + if (!handle_valid(handle))
> + return -ESRCH;
> +
> + info = (struct kvmgt_guest_info *)handle;
> + vgpu = info->vgpu;
> +
> + device = vfio_device_get_from_dev(mdev_dev(vgpu->vdev.mdev));
> + if (device == NULL)
> + return -ENODEV;
> + vgpu->vdev.vfio_device = device;
> +
> + return 0;
> +}
> +
> +static void kvmgt_put_vfio_device(unsigned long handle)
> +{
> + struct kvmgt_guest_info *info;
> + struct intel_vgpu *vgpu;
> +
> + if (!handle_valid(handle))
> + return;
> +
> + info = (struct kvmgt_guest_info *)handle;
> + vgpu = info->vgpu;
> +
> + vfio_device_put(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;
> +
> + if (WARN_ON(!vgpu->vdev.vfio_device))

Doesn't seem possible, however any kvmgt_put_vfio_device() call will
set this regardless of whether that calling path retained the reference
or failed and released it, so the way vfio_device is cached here is
rather sketchy in general.

> + return -ENODEV;
> +
> + 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->handle);
> + 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_DEVICE_QUERY_PLANE) {
> + struct vfio_vgpu_query_plane plane_info;
> +
> + minsz = offsetofend(struct vfio_vgpu_query_plane, padding);
> + 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_DEVICE_CREATE_DMABUF) {
> + struct vfio_vgpu_create_dmabuf dmabuf;
> +
> + minsz = offsetofend(struct vfio_vgpu_create_dmabuf, plane_id);
> + 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 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;
> @@ -1249,6 +1361,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 (atomic_read(&vgpu->mgr_fd_opened)) {
> + gvt_vgpu_err("mgr fd already opened\n");
> + return -EPERM;
> + }

Should check what's being asked for first, if other types are created
that are not supported by kvmgt we should consistently return -EINVAL,
not -EINVAL if DMABUF_MGR isn't already opened and -EPERM if it is.

> +
> + if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
> + return -EINVAL;
> + if (type != VFIO_DEVICE_DMABUF_MGR_FD)
> + return -EINVAL;
> +
> + ret = kvmgt_get_vfio_device(vgpu->handle);
> + 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->handle);
> + gvt_vgpu_err("create dmabuf mgr fd failed\n");
> + return -EINVAL;
> + }
> + atomic_inc(&vgpu->mgr_fd_opened);

This doesn't seem like a super effective use of atomics, you could be
sure to have only one open with a cmpxchg.

> +
> + return fd;
> }
>
> return 0;
> @@ -1612,6 +1754,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..1be961e 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->handle);
> +}
> +
> +/**
> + * 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->handle);
> +}
> +
> #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);
>

2017-06-14 02:53:32

by Xiaoguang Chen

Subject: RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

>-----Original Message-----
>From: Alex Williamson [mailto:[email protected]]
>Sent: Wednesday, June 14, 2017 5:25 AM
>To: Chen, Xiaoguang <[email protected]>
>Cc: [email protected]; [email protected]; intel-
>[email protected]; [email protected];
>[email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt-
>[email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin
><[email protected]>
>Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
>
>On Fri, 9 Jun 2017 14:50:40 +0800
>Xiaoguang Chen <[email protected]> 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.
>>
>> Signed-off-by: Xiaoguang Chen <[email protected]>
>> ---
>> include/uapi/linux/vfio.h | 58
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index ae46105..24427b7 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -502,6 +502,64 @@ 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)
>> +
>> +struct vfio_vgpu_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;
>> +};
>> +
>> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */
>
>Move this #define up above vfio_vgpu_plane_info to associate it with the
>VFIO_DEVICE_GET_FD ioctl.
OK.

>
>> +
>> +/*
>> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
>> + * struct vfio_vgpu_query_plane)
>> + * Query plane information
>> + */
>> +struct vfio_vgpu_query_plane {
>> + __u32 argsz;
>> + __u32 flags;
>> + struct vfio_vgpu_plane_info plane_info;
>> + __u32 plane_id;
>> + __u32 padding;
>
>This padding doesn't make sense.
This padding is still needed if we do not move the plane_id into vfio_vgpu_plane_info. Right?

>
>> +};
>> +
>> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>> +
>> +/*
>> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
>> + * struct
>vfio_vgpu_create_dmabuf)
>> + *
>> + * Create a dma-buf for a plane
>> + */
>> +struct vfio_vgpu_create_dmabuf {
>> + __u32 argsz;
>> + __u32 flags;
>> + struct vfio_vgpu_plane_info plane_info;
>> + __s32 fd;
>> + __u32 plane_id;
>> +};
>
>Both of these have a plane_id, should plane_id simply replace the padding in
>plane_info?
Precisely speaking plane_id does not belong to the plane info. All the other information are decoded from plane except plane id.

>If not, let's at least put them in the same order so that plane_id is
>after plane_info for both structs.
Ok.

>
>> +
>> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
>
>I don't think these should be named just VFIO_DEVICE_foo, that implies they're
>ioctls on the vfio device fd, they're not. They need to be associated both in name
>and more complete descriptions as ioctls to the fd returned from a request for a
>VFIO_DEVICE_DMABUF_MGR_FD. Perhaps VFIO_DMABUF_MGR_QUERY_PLANE
>and VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using
>"vgpu" in the structure names here either, the ioctls aren't named after vgpus.
>Aren't these rather generic to graphics dmabufs, not specifically vgpus?
Make sense. I will change the names.

Thanks,
>
>Alex
>
>> +
>> /* -------- API for Type1 VFIO IOMMU -------- */
>>
>> /**

2017-06-14 03:06:16

by Alex Williamson

Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

On Wed, 14 Jun 2017 02:53:24 +0000
"Chen, Xiaoguang" <[email protected]> wrote:

> >-----Original Message-----
> >From: Alex Williamson [mailto:[email protected]]
> >Sent: Wednesday, June 14, 2017 5:25 AM
> >To: Chen, Xiaoguang <[email protected]>
> >Cc: [email protected]; [email protected]; intel-
> >[email protected]; [email protected];
> >[email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt-
> >[email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin
> ><[email protected]>
> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
> >
> >On Fri, 9 Jun 2017 14:50:40 +0800
> >Xiaoguang Chen <[email protected]> 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.
> >>
> >> Signed-off-by: Xiaoguang Chen <[email protected]>
> >> ---
> >> include/uapi/linux/vfio.h | 58
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 58 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index ae46105..24427b7 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -502,6 +502,64 @@ 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)
> >> +
> >> +struct vfio_vgpu_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;
> >> +};
> >> +
> >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */
> >
> >Move this #define up above vfio_vgpu_plane_info to associate it with the
> >VFIO_DEVICE_GET_FD ioctl.
> OK.
>
> >
> >> +
> >> +/*
> >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
> >> + * struct vfio_vgpu_query_plane)
> >> + * Query plane information
> >> + */
> >> +struct vfio_vgpu_query_plane {
> >> + __u32 argsz;
> >> + __u32 flags;
> >> + struct vfio_vgpu_plane_info plane_info;
> >> + __u32 plane_id;
> >> + __u32 padding;
> >
> >This padding doesn't make sense.
> This padding is still needed if we do not move the plane_id into vfio_vgpu_plane_info. Right?

I don't see why this padding is ever needed, can you explain? Does the
structure not being a multiple of 8 bytes affect any of the offsets
within the structure?

> >> +};
> >> +
> >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> >> +
> >> +/*
> >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
> >> + * struct
> >vfio_vgpu_create_dmabuf)
> >> + *
> >> + * Create a dma-buf for a plane
> >> + */
> >> +struct vfio_vgpu_create_dmabuf {
> >> + __u32 argsz;
> >> + __u32 flags;
> >> + struct vfio_vgpu_plane_info plane_info;
> >> + __s32 fd;
> >> + __u32 plane_id;
> >> +};
> >
> >Both of these have a plane_id, should plane_id simply replace the padding in
> >plane_info?
> Precisely speaking plane_id does not belong to the plane info. All the other information are decoded from plane except plane id.

Ok, let's keep is separate then. Thanks,

Alex

> >If not, let's at least put them in the same order so that plane_id is
> >after plane_info for both structs.
> Ok.
>
> >
> >> +
> >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
> >
> >I don't think these should be named just VFIO_DEVICE_foo, that implies they're
> >ioctls on the vfio device fd, they're not. They need to be associated both in name
> >and more complete descriptions as ioctls to the fd returned from a request for a
> >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps VFIO_DMABUF_MGR_QUERY_PLANE
> >and VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using
> >"vgpu" in the structure names here either, the ioctls aren't named after vgpus.
> >Aren't these rather generic to graphics dmabufs, not specifically vgpus?
> Make sense. I will change the names.
>
> Thanks,
> >
> >Alex
> >
> >> +
> >> /* -------- API for Type1 VFIO IOMMU -------- */
> >>
> >> /**
>

2017-06-14 03:18:47

by Xiaoguang Chen

Subject: RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

>-----Original Message-----
>From: intel-gvt-dev [mailto:[email protected]] On
>Behalf Of Alex Williamson
>Sent: Wednesday, June 14, 2017 11:06 AM
>To: Chen, Xiaoguang <[email protected]>
>Cc: Tian, Kevin <[email protected]>; [email protected]; linux-
>[email protected]; [email protected]; [email protected]; Lv,
>Zhiyuan <[email protected]>; [email protected]; Wang, Zhi
>A <[email protected]>; [email protected]
>Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
>
>On Wed, 14 Jun 2017 02:53:24 +0000
>"Chen, Xiaoguang" <[email protected]> wrote:
>
>> >-----Original Message-----
>> >From: Alex Williamson [mailto:[email protected]]
>> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >To: Chen, Xiaoguang <[email protected]>
>> >Cc: [email protected]; [email protected]; intel-
>> >[email protected]; [email protected];
>> >[email protected]; Lv, Zhiyuan <[email protected]>;
>> >intel-gvt- [email protected]; Wang, Zhi A
>> ><[email protected]>; Tian, Kevin <[email protected]>
>> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf
>> >operations
>> >
>> >On Fri, 9 Jun 2017 14:50:40 +0800
>> >Xiaoguang Chen <[email protected]> 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.
>> >>
>> >> Signed-off-by: Xiaoguang Chen <[email protected]>
>> >> ---
>> >> include/uapi/linux/vfio.h | 58
>> >> +++++++++++++++++++++++++++++++++++++++++++++++
>> >> 1 file changed, 58 insertions(+)
>> >>
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index ae46105..24427b7 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -502,6 +502,64 @@ 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)
>> >> +
>> >> +struct vfio_vgpu_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;
>> >> +};
>> >> +
>> >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */
>> >
>> >Move this #define up above vfio_vgpu_plane_info to associate it with
>> >the VFIO_DEVICE_GET_FD ioctl.
>> OK.
>>
>> >
>> >> +
>> >> +/*
>> >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
>> >> + * struct vfio_vgpu_query_plane)
>> >> + * Query plane information
>> >> + */
>> >> +struct vfio_vgpu_query_plane {
>> >> + __u32 argsz;
>> >> + __u32 flags;
>> >> + struct vfio_vgpu_plane_info plane_info;
>> >> + __u32 plane_id;
>> >> + __u32 padding;
>> >
>> >This padding doesn't make sense.
>> This padding is still needed if we do not move the plane_id into
>vfio_vgpu_plane_info. Right?
>
>I don't see why this padding is ever needed, can you explain?
I thought we add the padding to make sure the structure layout is the same in both 32bit and 64bit systems.
Am I right?

>Does the structure
>not being a multiple of 8 bytes affect any of the offsets within the structure?
No. it will not affect any offsets in the structure.

>
>> >> +};
>> >> +
>> >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>> >> +
>> >> +/*
>> >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
>> >> + * struct
>> >vfio_vgpu_create_dmabuf)
>> >> + *
>> >> + * Create a dma-buf for a plane
>> >> + */
>> >> +struct vfio_vgpu_create_dmabuf {
>> >> + __u32 argsz;
>> >> + __u32 flags;
>> >> + struct vfio_vgpu_plane_info plane_info;
>> >> + __s32 fd;
>> >> + __u32 plane_id;
>> >> +};
>> >
>> >Both of these have a plane_id, should plane_id simply replace the padding in
>> >plane_info?
>> Precisely speaking plane_id does not belong to the plane info. All the other
>information are decoded from plane except plane id.
>
>Ok, let's keep is separate then. Thanks,
>
>Alex
>
>> >If not, let's at least put them in the same order so that plane_id is
>> >after plane_info for both structs.
>> Ok.
>>
>> >
>> >> +
>> >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
>> >
>> >I don't think these should be named just VFIO_DEVICE_foo, that
>> >implies they're ioctls on the vfio device fd, they're not. They need
>> >to be associated both in name and more complete descriptions as
>> >ioctls to the fd returned from a request for a
>> >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps
>VFIO_DMABUF_MGR_QUERY_PLANE and
>> >VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using
>"vgpu" in the structure names here either, the ioctls aren't named after vgpus.
>> >Aren't these rather generic to graphics dmabufs, not specifically vgpus?
>> Make sense. I will change the names.
>>
>> Thanks,
>> >
>> >Alex
>> >
>> >> +
>> >> /* -------- API for Type1 VFIO IOMMU -------- */
>> >>
>> >> /**
>>
>
>_______________________________________________
>intel-gvt-dev mailing list
>[email protected]
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

2017-06-14 03:45:44

by Alex Williamson

Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

On Wed, 14 Jun 2017 03:18:31 +0000
"Chen, Xiaoguang" <[email protected]> wrote:

> >-----Original Message-----
> >From: intel-gvt-dev [mailto:[email protected]] On
> >Behalf Of Alex Williamson
> >Sent: Wednesday, June 14, 2017 11:06 AM
> >To: Chen, Xiaoguang <[email protected]>
> >Cc: Tian, Kevin <[email protected]>; [email protected]; linux-
> >[email protected]; [email protected]; [email protected]; Lv,
> >Zhiyuan <[email protected]>; [email protected]; Wang, Zhi
> >A <[email protected]>; [email protected]
> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
> >
> >On Wed, 14 Jun 2017 02:53:24 +0000
> >"Chen, Xiaoguang" <[email protected]> wrote:
> >
> >> >-----Original Message-----
> >> >From: Alex Williamson [mailto:[email protected]]
> >> >Sent: Wednesday, June 14, 2017 5:25 AM
> >> >To: Chen, Xiaoguang <[email protected]>
> >> >Cc: [email protected]; [email protected]; intel-
> >> >[email protected]; [email protected];
> >> >[email protected]; Lv, Zhiyuan <[email protected]>;
> >> >intel-gvt- [email protected]; Wang, Zhi A
> >> ><[email protected]>; Tian, Kevin <[email protected]>
> >> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf
> >> >operations
> >> >
> >> >On Fri, 9 Jun 2017 14:50:40 +0800
> >> >Xiaoguang Chen <[email protected]> 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.
> >> >>
> >> >> Signed-off-by: Xiaoguang Chen <[email protected]>
> >> >> ---
> >> >> include/uapi/linux/vfio.h | 58
> >> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> >> 1 file changed, 58 insertions(+)
> >> >>
> >> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> >> index ae46105..24427b7 100644
> >> >> --- a/include/uapi/linux/vfio.h
> >> >> +++ b/include/uapi/linux/vfio.h
> >> >> @@ -502,6 +502,64 @@ 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)
> >> >> +
> >> >> +struct vfio_vgpu_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;
> >> >> +};
> >> >> +
> >> >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */
> >> >
> >> >Move this #define up above vfio_vgpu_plane_info to associate it with
> >> >the VFIO_DEVICE_GET_FD ioctl.
> >> OK.
> >>
> >> >
> >> >> +
> >> >> +/*
> >> >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
> >> >> + * struct vfio_vgpu_query_plane)
> >> >> + * Query plane information
> >> >> + */
> >> >> +struct vfio_vgpu_query_plane {
> >> >> + __u32 argsz;
> >> >> + __u32 flags;
> >> >> + struct vfio_vgpu_plane_info plane_info;
> >> >> + __u32 plane_id;
> >> >> + __u32 padding;
> >> >
> >> >This padding doesn't make sense.
> >> This padding is still needed if we do not move the plane_id into
> >vfio_vgpu_plane_info. Right?
> >
> >I don't see why this padding is ever needed, can you explain?
> I thought we add the padding to make sure the structure layout is the same in both 32bit and 64bit systems.
> Am I right?

Isn't it already the same without any of the padding here? Without the
padding in vfio_vgpu_plane_info it's 4-byte aligned and we're following
it with a 4-byte field, that works the same on 32 and 64bit. Padding
the outer structure here makes no sense to me. Generally padding at
the end of the structure is to allow flexibility in expanding it within
that padding without breaking the ioctl. Here we use _IO and
argsz/flags to do that. Thanks,

Alex

> >Does the structure
> >not being a multiple of 8 bytes affect any of the offsets within the structure?
> No. it will not affect any offsets in the structure.
>
> >
> >> >> +};
> >> >> +
> >> >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
> >> >> +
> >> >> +/*
> >> >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
> >> >> + * struct
> >> >vfio_vgpu_create_dmabuf)
> >> >> + *
> >> >> + * Create a dma-buf for a plane
> >> >> + */
> >> >> +struct vfio_vgpu_create_dmabuf {
> >> >> + __u32 argsz;
> >> >> + __u32 flags;
> >> >> + struct vfio_vgpu_plane_info plane_info;
> >> >> + __s32 fd;
> >> >> + __u32 plane_id;
> >> >> +};
> >> >
> >> >Both of these have a plane_id, should plane_id simply replace the padding in
> >> >plane_info?
> >> Precisely speaking plane_id does not belong to the plane info. All the other
> >information are decoded from plane except plane id.
> >
> >Ok, let's keep is separate then. Thanks,
> >
> >Alex
> >
> >> >If not, let's at least put them in the same order so that plane_id is
> >> >after plane_info for both structs.
> >> Ok.
> >>
> >> >
> >> >> +
> >> >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16)
> >> >
> >> >I don't think these should be named just VFIO_DEVICE_foo, that
> >> >implies they're ioctls on the vfio device fd, they're not. They need
> >> >to be associated both in name and more complete descriptions as
> >> >ioctls to the fd returned from a request for a
> >> >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps
> >VFIO_DMABUF_MGR_QUERY_PLANE and
> >> >VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using
> >"vgpu" in the structure names here either, the ioctls aren't named after vgpus.
> >> >Aren't these rather generic to graphics dmabufs, not specifically vgpus?
> >> Make sense. I will change the names.
> >>
> >> Thanks,
> >> >
> >> >Alex
> >> >
> >> >> +
> >> >> /* -------- API for Type1 VFIO IOMMU -------- */
> >> >>
> >> >> /**
> >>
> >
> >_______________________________________________
> >intel-gvt-dev mailing list
> >[email protected]
> >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

2017-06-14 05:29:40

by Xiaoguang Chen

Subject: RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

>-----Original Message-----
>From: Alex Williamson [mailto:[email protected]]
>Sent: Wednesday, June 14, 2017 11:46 AM
>To: Chen, Xiaoguang <[email protected]>
>Cc: Tian, Kevin <[email protected]>; [email protected]; linux-
>[email protected]; [email protected]; [email protected]; Lv,
>Zhiyuan <[email protected]>; [email protected]; Wang, Zhi
>A <[email protected]>; [email protected]
>Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
>
>On Wed, 14 Jun 2017 03:18:31 +0000
>"Chen, Xiaoguang" <[email protected]> wrote:
>
>> >-----Original Message-----
>> >From: intel-gvt-dev
>> >[mailto:[email protected]] On Behalf Of
>> >Alex Williamson
>> >Sent: Wednesday, June 14, 2017 11:06 AM
>> >To: Chen, Xiaoguang <[email protected]>
>> >Cc: Tian, Kevin <[email protected]>;
>> >[email protected]; linux- [email protected];
>> >[email protected]; [email protected]; Lv, Zhiyuan
>> ><[email protected]>; [email protected]; Wang,
>> >Zhi A <[email protected]>; [email protected]
>> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf
>> >operations
>> >
>> >On Wed, 14 Jun 2017 02:53:24 +0000
>> >"Chen, Xiaoguang" <[email protected]> wrote:
>> >
>> >> >-----Original Message-----
>> >> >From: Alex Williamson [mailto:[email protected]]
>> >> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >> >To: Chen, Xiaoguang <[email protected]>
>> >> >Cc: [email protected]; [email protected]; intel-
>> >> >[email protected]; [email protected];
>> >> >[email protected]; Lv, Zhiyuan <[email protected]>;
>> >> >intel-gvt- [email protected]; Wang, Zhi A
>> >> ><[email protected]>; Tian, Kevin <[email protected]>
>> >> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf
>> >> >operations
>> >> >
>> >> >On Fri, 9 Jun 2017 14:50:40 +0800 Xiaoguang Chen
>> >> ><[email protected]> 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.
>> >> >>
>> >> >> Signed-off-by: Xiaoguang Chen <[email protected]>
>> >> >> ---
>> >> >> include/uapi/linux/vfio.h | 58
>> >> >> +++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> 1 file changed, 58 insertions(+)
>> >> >>
>> >> >> diff --git a/include/uapi/linux/vfio.h
>> >> >> b/include/uapi/linux/vfio.h index ae46105..24427b7 100644
>> >> >> --- a/include/uapi/linux/vfio.h
>> >> >> +++ b/include/uapi/linux/vfio.h
>> >> >> @@ -502,6 +502,64 @@ 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)
>> >> >> +
>> >> >> +struct vfio_vgpu_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;
>> >> >> +};
>> >> >> +
>> >> >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types
>*/
>> >> >
>> >> >Move this #define up above vfio_vgpu_plane_info to associate it
>> >> >with the VFIO_DEVICE_GET_FD ioctl.
>> >> OK.
>> >>
>> >> >
>> >> >> +
>> >> >> +/*
>> >> >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15,
>> >> >> + * struct
>vfio_vgpu_query_plane)
>> >> >> + * Query plane information
>> >> >> + */
>> >> >> +struct vfio_vgpu_query_plane {
>> >> >> + __u32 argsz;
>> >> >> + __u32 flags;
>> >> >> + struct vfio_vgpu_plane_info plane_info;
>> >> >> + __u32 plane_id;
>> >> >> + __u32 padding;
>> >> >
>> >> >This padding doesn't make sense.
>> >> This padding is still needed if we do not move the plane_id into
>> >vfio_vgpu_plane_info. Right?
>> >
>> >I don't see why this padding is ever needed, can you explain?
>> I thought we add the padding to make sure the structure layout is the same in
>both 32bit and 64bit systems.
>> Am I right?
>
>Isn't it already the same without any of the padding here? Without the padding in
>vfio_vgpu_plane_info it's 4-byte aligned and we're following it with a 4-byte field,
>that works the same on 32 and 64bit. Padding the outer structure here makes no
>sense to me. Generally padding at the end of the structure is to allow flexibility in
>expanding it within that padding without breaking the ioctl. Here we use _IO and
>argsz/flags to do that. Thanks,
I got your point. Yes this padding is useless.
Thanks very much : )

>
>Alex
>
>> >Does the structure
>> >not being a multiple of 8 bytes affect any of the offsets within the structure?
>> No. it will not affect any offsets in the structure.
>>
>> >
>> >> >> +};
>> >> >> +
>> >> >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>> >> >> +
>> >> >> +/*
>> >> >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16,
>> >> >> + * struct
>> >> >vfio_vgpu_create_dmabuf)
>> >> >> + *
>> >> >> + * Create a dma-buf for a plane */ struct
>> >> >> +vfio_vgpu_create_dmabuf {
>> >> >> + __u32 argsz;
>> >> >> + __u32 flags;
>> >> >> + struct vfio_vgpu_plane_info plane_info;
>> >> >> + __s32 fd;
>> >> >> + __u32 plane_id;
>> >> >> +};
>> >> >
>> >> >Both of these have a plane_id, should plane_id simply replace the
>> >> >padding in plane_info?
>> >> Precisely speaking plane_id does not belong to the plane info. All
>> >> the other
>> >information are decoded from plane except plane id.
>> >
>> >Ok, let's keep is separate then. Thanks,
>> >
>> >Alex
>> >
>> >> >If not, let's at least put them in the same order so that plane_id
>> >> >is after plane_info for both structs.
>> >> Ok.
>> >>
>> >> >
>> >> >> +
>> >> >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE +
>> >> >> +16)
>> >> >
>> >> >I don't think these should be named just VFIO_DEVICE_foo, that
>> >> >implies they're ioctls on the vfio device fd, they're not. They
>> >> >need to be associated both in name and more complete descriptions
>> >> >as ioctls to the fd returned from a request for a
>> >> >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps
>> >VFIO_DMABUF_MGR_QUERY_PLANE and
>> >> >VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using
>> >"vgpu" in the structure names here either, the ioctls aren't named after vgpus.
>> >> >Aren't these rather generic to graphics dmabufs, not specifically vgpus?
>> >> Make sense. I will change the names.
>> >>
>> >> Thanks,
>> >> >
>> >> >Alex
>> >> >
>> >> >> +
>> >> >> /* -------- API for Type1 VFIO IOMMU -------- */
>> >> >>
>> >> >> /**
>> >>
>> >
>> >_______________________________________________
>> >intel-gvt-dev mailing list
>> >[email protected]
>> >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

2017-06-14 09:43:19

by Zhenyu Wang

Subject: Re: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

On 2017.06.09 14:50:39 +0800, Xiaoguang Chen wrote:
> decode frambuffer attributes of primary, cursor and sprite plane
>
> Signed-off-by: Xiaoguang Chen <[email protected]>

...

> +/**
> + * 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 (!skl_pixel_formats[fmt].bpp && !bdw_pixel_formats[fmt].bpp) {
> + gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
> + return -EINVAL;
> + }

Is this correct? shouldn't be plane->bpp as last time comment?

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 55e3010..400759f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -161,6 +161,12 @@ extern "C" {
> #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */
> #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */
>
> +/*
> + * Intel GVT-g plane format definition
> + */
> +#define DRM_FORMAT_XRGB161616_GVT fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
> +#define DRM_FORMAT_XBGR161616_GVT fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */
> +
>

This should be a seperate patch and not need GVT postfix for format definition.

--
Open Source Technology Center, Intel ltd.

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

Attachments:

2017-06-14 09:51:21

by Xiaoguang Chen

Subject: RE: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

>-----Original Message-----
>From: Zhenyu Wang [mailto:[email protected]]
>Sent: Wednesday, June 14, 2017 5:39 PM
>To: Chen, Xiaoguang <[email protected]>
>Cc: [email protected]; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt-
>[email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin
><[email protected]>
>Subject: Re: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-
>g
>
>On 2017.06.09 14:50:39 +0800, Xiaoguang Chen wrote:
>> decode frambuffer attributes of primary, cursor and sprite plane
>>
>> Signed-off-by: Xiaoguang Chen <[email protected]>
>
>...
>
>> +/**
>> + * 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 (!skl_pixel_formats[fmt].bpp && !bdw_pixel_formats[fmt].bpp) {
>> + gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
>> + return -EINVAL;
>> + }
>
>Is this correct? shouldn't be plane->bpp as last time comment?
Yes. But use plane->bpp is more concisely. Will change.

>
>> diff --git a/include/uapi/drm/drm_fourcc.h
>> b/include/uapi/drm/drm_fourcc.h index 55e3010..400759f 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -161,6 +161,12 @@ extern "C" {
>> #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-
>subsampled Cb (1) and Cr (2) planes */
>> #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-
>subsampled Cr (1) and Cb (2) planes */
>>
>> +/*
>> + * Intel GVT-g plane format definition */ #define
>> +DRM_FORMAT_XRGB161616_GVT fourcc_code('X', 'R', '4', '8') /* [63:0]
>> +x:R:G:B 16:16:16:16 little endian */ #define
>> +DRM_FORMAT_XBGR161616_GVT fourcc_code('X', 'B', '4', '8') /* [63:0]
>> +x:B:G:R 16:16:16:16 little endian */
>> +
>>
>
>This should be a seperate patch and not need GVT postfix for format definition.
OK.

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