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

Groups > linux.kernel > #1681923 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v10] vfio: ABI for mdev display dma-buf operation Tina Zhang <tina.zhang@intel.com> - 2017-07-06 00:40 +0200
    Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Kirti Wankhede <kwankhede@nvidia.com> - 2017-07-06 16:10 +0200
      Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-11 08:20 +0200
        Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Daniel Vetter <daniel@ffwll.ch> - 2017-07-11 11:20 +0200
          RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation "Zhang, Tina" <tina.zhang@intel.com> - 2017-07-12 04:40 +0200
            Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Daniel Vetter <daniel@ffwll.ch> - 2017-07-12 09:50 +0200
              Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Kirti Wankhede <kwankhede@nvidia.com> - 2017-07-12 14:50 +0200
                RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation "Zhang, Tina" <tina.zhang@intel.com> - 2017-07-14 03:40 +0200
                  Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Kirti Wankhede <kwankhede@nvidia.com> - 2017-07-14 12:20 +0200
                    Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-14 14:10 +0200
                      Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Kirti Wankhede <kwankhede@nvidia.com> - 2017-07-14 14:20 +0200
                        Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-17 13:10 +0200
                Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-14 12:10 +0200
                  Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Kirti Wankhede <kwankhede@nvidia.com> - 2017-07-14 12:20 +0200
                    Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-14 14:20 +0200
          Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Zhenyu Wang <zhenyuw@linux.intel.com> - 2017-07-12 05:30 +0200
      RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation "Zhang, Tina" <tina.zhang@intel.com> - 2017-07-12 04:30 +0200
    Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Gerd Hoffmann <kraxel@redhat.com> - 2017-07-11 08:10 +0200
      RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation "Zhang, Tina" <tina.zhang@intel.com> - 2017-07-12 04:50 +0200
    Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation Daniel Vetter <daniel@ffwll.ch> - 2017-07-11 11:20 +0200
      RE: [PATCH v10] vfio: ABI for mdev display dma-buf operation "Zhang, Tina" <tina.zhang@intel.com> - 2017-07-12 05:20 +0200

Page 1 of 2  [1] 2  Next page →

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

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

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

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

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

[toc] | [next] | [standalone]

#1682458

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

Above structure looks good to me.

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

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

Thanks,
Kirti

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

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

#1684795

  Hi,

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

plane_type is DRM_PLANE_TYPE_*.

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

plane_id needs a specification.

cheers,
  Gerd

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

#1684908

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

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

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

#1685469

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

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

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

#1685579

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

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

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

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

#1685748

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

plane type could be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

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

Thanks,
Kirti

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

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

#1687011

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

Tina

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

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

#1687234

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

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

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

Thanks,
Kirti


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

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

#1687290

  Hi,

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

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

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

cheers,
  Gerd

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

#1687329

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

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

Thanks,
Kirti

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

#1688928

  Hi,

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

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

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

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

cheers,
  Gerd

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

#1687228

  Hi,

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

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

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

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

cheers,
  Gerd

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

#1687233

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

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

Thanks,
Kirti

> cheers,
>   Gerd
> 

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

#1687301

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

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

cheers,
  Gerd

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

#1685484

[Multipart message — attachments visible in raw view] — view raw

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

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

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

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

-- 
Open Source Technology Center, Intel ltd.

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

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

#1685462

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

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

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

#1684793

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

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

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

What is plane_id?

cheers,
  Gerd

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

#1685471

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

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

Tina

> 
> cheers,
>   Gerd

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

#1684909

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

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

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

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

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

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

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web