Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1632738 > unrolled thread

[RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Xiaoguang Chen <xiaoguang.chen@intel.com> - 2017-04-28 11:50 +0200
    Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Gerd Hoffmann <kraxel@redhat.com> - 2017-05-02 12:00 +0200
      RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf "Chen, Xiaoguang" <xiaoguang.chen@intel.com> - 2017-05-03 03:50 +0200
        RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf "Chen, Xiaoguang" <xiaoguang.chen@intel.com> - 2017-05-04 05:20 +0200
          Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Alex Williamson <alex.williamson@redhat.com> - 2017-05-04 18:10 +0200
            Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Gerd Hoffmann <kraxel@redhat.com> - 2017-05-05 09:00 +0200
              Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Alex Williamson <alex.williamson@redhat.com> - 2017-05-05 17:20 +0200
                RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf "Chen, Xiaoguang" <xiaoguang.chen@intel.com> - 2017-05-11 10:50 +0200
                  Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Gerd Hoffmann <kraxel@redhat.com> - 2017-05-11 15:30 +0200
                    Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf Alex Williamson <alex.williamson@redhat.com> - 2017-05-11 17:50 +0200

#1632738 — [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

GVT-g will create an anonymous fd and a vfio device region to deliver
the fd to QEMU.
QEMU can do ioctl using this fd to query/generate dmabuf on an intel vgpu.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/gvt.c   |   2 +
 drivers/gpu/drm/i915/gvt/gvt.h   |   2 +
 drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h        |   1 +
 4 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index 7dea5e5..c266d31 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -54,6 +54,8 @@
 	.vgpu_reset = intel_gvt_reset_vgpu,
 	.vgpu_activate = intel_gvt_activate_vgpu,
 	.vgpu_deactivate = intel_gvt_deactivate_vgpu,
+	.vgpu_query_dmabuf = intel_vgpu_query_dmabuf,
+	.vgpu_generate_dmabuf = intel_vgpu_generate_dmabuf,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 763a8c5..2733a69 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -467,6 +467,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_dmabuf)(struct intel_vgpu *, void *);
+	int (*vgpu_generate_dmabuf)(struct intel_vgpu *, void *);
 };
 
 
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 389f072..beb5356 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"
@@ -524,6 +525,106 @@ static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
 	return ret;
 }
 
+static int intel_vgpu_gvtg_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	WARN_ON(1);
+
+	return 0;
+}
+
+static int intel_vgpu_gvtg_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static long intel_vgpu_gvtg_ioctl(struct file *filp,
+		unsigned int ioctl, unsigned long arg)
+{
+	struct intel_vgpu *vgpu = filp->private_data;
+	int minsz;
+	struct intel_vgpu_dmabuf dmabuf;
+	int ret;
+
+	minsz = offsetofend(struct intel_vgpu_dmabuf, y_pos);
+	if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+		return -EFAULT;
+	if (ioctl == INTEL_VGPU_QUERY_DMABUF)
+		ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
+	else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
+		ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu, &dmabuf);
+	else {
+		gvt_vgpu_err("unsupported dmabuf operation\n");
+		return -EINVAL;
+	}
+
+	if (ret != 0) {
+		gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
+		return -EINVAL;
+	}
+
+	return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : 0;
+}
+
+static const struct file_operations intel_vgpu_gvtg_ops = {
+	.release        = intel_vgpu_gvtg_release,
+	.unlocked_ioctl = intel_vgpu_gvtg_ioctl,
+	.mmap           = intel_vgpu_gvtg_mmap,
+	.llseek         = noop_llseek,
+};
+
+static size_t intel_vgpu_reg_rw_gvtg(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;
+	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+	int fd;
+
+	if (pos >= vgpu->vdev.region[i].size || iswrite) {
+		gvt_vgpu_err("invalid op or offset for Intel vgpu fd region\n");
+		return -EINVAL;
+	}
+
+	fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
+			O_RDWR | O_CLOEXEC);
+	if (fd < 0) {
+		gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
+		return -EINVAL;
+	}
+
+	count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
+	memcpy(buf, &fd, count);
+
+	return count;
+}
+
+static void intel_vgpu_reg_release_gvtg(struct intel_vgpu *vgpu,
+		struct vfio_region *region)
+{
+}
+
+static const struct intel_vgpu_regops intel_vgpu_regops_gvtg = {
+	.rw = intel_vgpu_reg_rw_gvtg,
+	.release = intel_vgpu_reg_release_gvtg,
+};
+
+static int intel_vgpu_reg_init_gvtg(struct intel_vgpu *vgpu)
+{
+	int ret;
+
+	ret = intel_vgpu_register_reg(vgpu,
+		PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+		VFIO_REGION_SUBTYPE_INTEL_IGD_GVTG,
+		&intel_vgpu_regops_gvtg, sizeof(int),
+		VFIO_REGION_INFO_FLAG_READ, NULL);
+	if (ret) {
+		gvt_vgpu_err("failed to register gvtg region:%d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -564,6 +665,14 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
 			dev_name(mdev_dev(mdev)));
 
+	ret = intel_vgpu_reg_init_gvtg(vgpu);
+	if (ret) {
+		gvt_vgpu_err("create gvtg region failed\n");
+		goto out;
+	}
+	gvt_dbg_core("create gvtg region succeeded for mdev:%s\n",
+			dev_name(mdev_dev(mdev)));
+
 	gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 519eff3..96d2c58 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION	(1)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
+#define VFIO_REGION_SUBTYPE_INTEL_IGD_GVTG	(4)
 
 /**
  * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
-- 
1.9.1

[toc] | [next] | [standalone]

#1634306

On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
> +static size_t intel_vgpu_reg_rw_gvtg(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;
> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +       int fd;
> +
> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
> region\n");
> +               return -EINVAL;
> +       }
> +
> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
> +                       O_RDWR | O_CLOEXEC);
> +       if (fd < 0) {
> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
> +               return -EINVAL;
> +       }
> +
> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> +       memcpy(buf, &fd, count);
> +
> +       return count;
> +}

Hmm, that looks like a rather strange way to return a file descriptor.

What is the reason to not use ioctls on the vfio file handle, like older
version of these patches did?

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1634692

>-----Original Message-----
>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>Sent: Tuesday, May 02, 2017 5:51 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org; intel-gvt-
>dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
>zhenyuw@linux.intel.com; linux-kernel@vger.kernel.org; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; Tian, Kevin <kevin.tian@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
>> +static size_t intel_vgpu_reg_rw_gvtg(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;
>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +       int fd;
>> +
>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
>> region\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
>> +                       O_RDWR | O_CLOEXEC);
>> +       if (fd < 0) {
>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
>> +               return -EINVAL;
>> +       }
>> +
>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
>> +       memcpy(buf, &fd, count);
>> +
>> +       return count;
>> +}
>
>Hmm, that looks like a rather strange way to return a file descriptor.
>
>What is the reason to not use ioctls on the vfio file handle, like older version of
>these patches did?
If I understood correctly that Alex prefer not to change the ioctls on the vfio file handle like the old version.
So I used this way the smallest change to general vfio framework only adding a subregion definition.

>
>cheers,
>  Gerd

[toc] | [prev] | [next] | [standalone]

#1635389

Hi Alex, do you have any comments for this interface?

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Chen, Xiaoguang
>Sent: Wednesday, May 03, 2017 9:39 AM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; alex.williamson@redhat.com;
>Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
>Zhi A <zhi.a.wang@intel.com>
>Subject: RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>
>
>>-----Original Message-----
>>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>>Sent: Tuesday, May 02, 2017 5:51 PM
>>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org;
>>intel-gvt- dev@lists.freedesktop.org; Wang, Zhi A
>><zhi.a.wang@intel.com>; zhenyuw@linux.intel.com;
>>linux-kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian,
>>Kevin <kevin.tian@intel.com>
>>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>>dmabuf
>>
>>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
>>> +static size_t intel_vgpu_reg_rw_gvtg(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;
>>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>>> +       int fd;
>>> +
>>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
>>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
>>> region\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
>>> +                       O_RDWR | O_CLOEXEC);
>>> +       if (fd < 0) {
>>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
>>> +       memcpy(buf, &fd, count);
>>> +
>>> +       return count;
>>> +}
>>
>>Hmm, that looks like a rather strange way to return a file descriptor.
>>
>>What is the reason to not use ioctls on the vfio file handle, like
>>older version of these patches did?
>If I understood correctly that Alex prefer not to change the ioctls on the vfio file
>handle like the old version.
>So I used this way the smallest change to general vfio framework only adding a
>subregion definition.
>
>>
>>cheers,
>>  Gerd
>
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

[toc] | [prev] | [next] | [standalone]

#1635865

On Thu, 4 May 2017 03:09:40 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex, do you have any comments for this interface?
> 
> >-----Original Message-----
> >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> >Behalf Of Chen, Xiaoguang
> >Sent: Wednesday, May 03, 2017 9:39 AM
> >To: Gerd Hoffmann <kraxel@redhat.com>
> >Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; alex.williamson@redhat.com;
> >Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> >Zhi A <zhi.a.wang@intel.com>
> >Subject: RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >
> >  
> >>-----Original Message-----
> >>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> >>Sent: Tuesday, May 02, 2017 5:51 PM
> >>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org;
> >>intel-gvt- dev@lists.freedesktop.org; Wang, Zhi A
> >><zhi.a.wang@intel.com>; zhenyuw@linux.intel.com;
> >>linux-kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian,
> >>Kevin <kevin.tian@intel.com>
> >>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
> >>dmabuf
> >>
> >>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:  
> >>> +static size_t intel_vgpu_reg_rw_gvtg(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;
> >>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> >>> +       int fd;
> >>> +
> >>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
> >>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
> >>> region\n");
> >>> +               return -EINVAL;
> >>> +       }
> >>> +
> >>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
> >>> +                       O_RDWR | O_CLOEXEC);
> >>> +       if (fd < 0) {
> >>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
> >>> +               return -EINVAL;
> >>> +       }
> >>> +
> >>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> >>> +       memcpy(buf, &fd, count);
> >>> +
> >>> +       return count;
> >>> +}  
> >>
> >>Hmm, that looks like a rather strange way to return a file descriptor.
> >>
> >>What is the reason to not use ioctls on the vfio file handle, like
> >>older version of these patches did?  
> >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> >handle like the old version.
> >So I used this way the smallest change to general vfio framework only adding a
> >subregion definition.

I think I was hoping we could avoid a separate file descriptor
altogether and use a vfio region instead.  However, it was explained
previously why this really needs to be a separate fd and I agree that
using a region to expose an fd is really awkward.  If we're going to
have a separate fd, let's use a device specific ioctl to get it.
Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1636203

  Hi,

> > >>Hmm, that looks like a rather strange way to return a file descriptor.
> > >>
> > >>What is the reason to not use ioctls on the vfio file handle, like
> > >>older version of these patches did?  
> > >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> > >handle like the old version.
> > >So I used this way the smallest change to general vfio framework only adding a
> > >subregion definition.
> 
> I think I was hoping we could avoid a separate file descriptor
> altogether and use a vfio region instead.

What exactly did you have in mind?  Put the framebuffer information
(struct intel_vgpu_dmabuf) into the vfio region, then access it using
read/write/mmap?

> However, it was explained
> previously why this really needs to be a separate fd and I agree that
> using a region to expose an fd is really awkward.

Now with this patchset we have *two* kinds of separate file handles.
First the anon-fd created by reading from the region.  This is then used
to run the intel ioctls on, which in turn create the other kind of file
handle (dma-buf-fd).

The dma-buf-fd really needs to be a separate fd, because it gets passed
around as handle and because this is the way dma-bufs work (guess this
is the discussion you are referring to).

I can't see a compelling reason for the anon-fd though.  I suspect this
was done due to a misunderstanding ...

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1636437

On Fri, 05 May 2017 08:55:31 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > >>Hmm, that looks like a rather strange way to return a file descriptor.
> > > >>
> > > >>What is the reason to not use ioctls on the vfio file handle, like
> > > >>older version of these patches did?    
> > > >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> > > >handle like the old version.
> > > >So I used this way the smallest change to general vfio framework only adding a
> > > >subregion definition.  
> > 
> > I think I was hoping we could avoid a separate file descriptor
> > altogether and use a vfio region instead.  
> 
> What exactly did you have in mind?  Put the framebuffer information
> (struct intel_vgpu_dmabuf) into the vfio region, then access it using
> read/write/mmap?

Yeah, that was my hope.  Adding a new file descriptor means we have one
more reference floating around complicating the life cycle of the
device, group, and container.  Furthermore this one is really only
visible to the mdev vendor driver, so we can't rely on vfio-core, the
vendor driver will need to consider the reference when releasing the
device.

> > However, it was explained
> > previously why this really needs to be a separate fd and I agree that
> > using a region to expose an fd is really awkward.  
> 
> Now with this patchset we have *two* kinds of separate file handles.
> First the anon-fd created by reading from the region.  This is then used
> to run the intel ioctls on, which in turn create the other kind of file
> handle (dma-buf-fd).
> 
> The dma-buf-fd really needs to be a separate fd, because it gets passed
> around as handle and because this is the way dma-bufs work (guess this
> is the discussion you are referring to).

Yep, we're going to need to trust the vendor driver to manage it, we
have lots of places where we need to trust the vendor driver for an
mdev device, unfortunately.
 
> I can't see a compelling reason for the anon-fd though.  I suspect this
> was done due to a misunderstanding ...

Yeah, vfio-core passes device ioctls to the vendor driver, so the
vendor driver should be able to implement a
VFIO_DEVICE_GVT_GET_DMABUF_FD ioctl direclty.  Ideally maybe this
isn't even GVT specific, and we'd s/GVT_//.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1639250

Hi Alex,

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Alex Williamson
>Sent: Friday, May 05, 2017 11:11 PM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>; intel-
>gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fri, 05 May 2017 08:55:31 +0200
>Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > > >>Hmm, that looks like a rather strange way to return a file descriptor.
>> > > >>
>> > > >>What is the reason to not use ioctls on the vfio file handle, like
>> > > >>older version of these patches did?
>> > > >If I understood correctly that Alex prefer not to change the
>> > > >ioctls on the vfio file handle like the old version.
>> > > >So I used this way the smallest change to general vfio framework
>> > > >only adding a subregion definition.
>> >
>> > I think I was hoping we could avoid a separate file descriptor
>> > altogether and use a vfio region instead.
>>
>> What exactly did you have in mind?  Put the framebuffer information
>> (struct intel_vgpu_dmabuf) into the vfio region, then access it using
>> read/write/mmap?
>
>Yeah, that was my hope.  Adding a new file descriptor means we have one more
>reference floating around complicating the life cycle of the device, group, and
>container.  Furthermore this one is really only visible to the mdev vendor driver,
>so we can't rely on vfio-core, the vendor driver will need to consider the
>reference when releasing the device.
While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
There are two methods to implement this:
1) write the plane id first and then read the framebuffer.
2) create 2 vfio regions one for primary and one for cursor.

Which method do you prefer? Or do you have other idea to handle this problem?

chenxg

[toc] | [prev] | [next] | [standalone]

#1639428

  Hi,

> While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
> There are two methods to implement this:
> 1) write the plane id first and then read the framebuffer.
> 2) create 2 vfio regions one for primary and one for cursor.

(3) Place information for both planes into one vfio region.
    Which allows to fetch both with a single read() syscall.

The question is how you'll get the file descriptor then.  If the ioctl
returns the dma-buf fd only you have a racy interface:  Things can
change between read(vfio-region) and ioctl(need-dmabuf-fd).

ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
the race, but then it is easier to go with ioctl only interface (simliar
to the orginal one from dec last year) I think.

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1639782

On Thu, 11 May 2017 15:27:53 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
> > There are two methods to implement this:
> > 1) write the plane id first and then read the framebuffer.
> > 2) create 2 vfio regions one for primary and one for cursor.  
> 
> (3) Place information for both planes into one vfio region.
>     Which allows to fetch both with a single read() syscall.
> 
> The question is how you'll get the file descriptor then.  If the ioctl
> returns the dma-buf fd only you have a racy interface:  Things can
> change between read(vfio-region) and ioctl(need-dmabuf-fd).
> 
> ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
> the race, but then it is easier to go with ioctl only interface (simliar
> to the orginal one from dec last year) I think.

If the dmabuf fd is provided by a separate mdev vendor driver specific
ioctl, I don't see how vfio regions should be involved.  Selecting
which framebuffer should be an ioctl parameter.  What sort of
information needs to be conveyed about each plane?  Is it static
information or something that needs to be read repeatedly?  Do we need
it before we get the dmabuf fd or can it be an ioctl on the dmabuf fd?
Thanks,

Alex

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web