> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Alex Williamson
> Sent: Tuesday, August 8, 2017 1:43 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; kwankh...@nvidia.com; kra...@redhat.com;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
>
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <tina.zh...@intel.com> wrote:
>
> > After going through the previous discussions, here are some summaries may
> be related to the current discussion:
> > 1. How does user mode figure the device capabilities between region and
> dma-buf?
> > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.
> > Otherwise, the mdev supports dma-buf.
>
> Why do we need to make this assumption? What happens when dma-buf is
> superseded? What happens if a device supports both dma-buf and regions?
> We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to
> use
> it to identify which field, between region_index and fd, is valid? We could
> even
> put region_index and fd into a union with the flag bits indicating how to
> interpret the union, but I'm not sure everyone was onboard with this idea.
> Seems like a waste of 4 bytes not to do that though.
It seems we discussed this idea before:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html
Thanks.
Tina
>
> Thinking further, is the user ever in a situation where they query the
> graphics
> plane info and can handle either a dma-buf or a region? It seems more likely
> that the user needs to know early on which is supported and would then require
> that they continue to see compatible plane information... Should the user
> then
> be able to specify whether they want a dma-buf or a region? Perhaps these
> flag
> bits are actually input and the return should be -errno if the driver cannot
> produce something compatible.
From the previously discussion, it seems user space workflow will look quite
different
for these two cases. So once user space finds out which case is supported, it
just uses
that case, and won't change it.
Meanwhile, I'm not sure whether there will be a mdev would like to support both
region
and dma-buf cases. In my opinion, either region or dma-buf is supported by one
mdev. (Yeah,
agree, there may be other cases in future)
It's like we want to propose a general interface used to share guest's buffer
with host. And the
general interface, so far, has two choice: region and dma-buf. So each mdev
likes this interface
can implement one kind of it and gets the benefit from the general interface.
So, if we think about this, the difference in user mode should be as little as
possible.
Thanks.
Tina
>
> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION. In this
> initial implementation, DMABUF or REGION would always be set by the user to
> request that type of interface. Additionally, the QUERY bit could be set to
> probe
> compatibility, thus if PROBE and REGION are set, the vendor driver would
> return
> success only if it supports the region based interface. If PROBE and DMABUF
> are
> set, the vendor driver returns success only if the dma-buf based interface is
> supported. The value of the remainder of the structure is undefined for
> PROBE.
> Additionally setting both DMABUF and REGION is invalid. Undefined flags bits
> must be validated as zero by the drivers for future use (thus if we later
> define
> DMABUFv2, an older driver should automatically return -errno when probed or
> requested).
>
> It seems like this handles all the cases, the user can ask what's supported
> and
> specifies the interface they want on every call. The user therefore can also
> choose between region_index and fd and we can make that a union.
Agree, that's a good proposal, which can handle all the cases.
I'm just not sure about the usage case of "on every call". In previous
discussion, it seems we think static is enough.
Thanks.
Tina
>
> > 2. For dma-buf, how to differentiate unsupported vs not initialized?
> > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will
> be returned. And -errno will return when meeting other failures, like -ENOMEM.
> > If the mdev is not initialized, there won't be any returned err. Just zero
> > all the
> fields in structure vfio_device_gfx_plane_info.
>
> So we're relying on special values again :-\ For which fields is zero not a
> valid
> value? I prefer the probe interface above unless there are better ideas.
>
> > 3. The id field in structure vfio_device_gfx_plane_info So far we
> > haven't figured out the usage of this field for dma-buf usage. So, this
> > field is
> changed to "region_index" and only used for region usage.
> > In previous discussions, we thought this "id" field might be used for both
> > dma-
> buf and region usages.
> > So, we proposed some ways, like adding flags field to the structure. Another
> way to do it was to add this:
> > enum vfio_device_gfx_type {
> > VFIO_DEVICE_GFX_NONE,
> > VFIO_DEVICE_GFX_DMABUF,
> > VFIO_DEVICE_GFX_REGION,
> > };
> >
> > struct vfio_device_gfx_query_caps {
> > __u32 argsz;
> > __u32 flags;
> > enum vfio_device_gfx_type;
> > };
> > Obviously, we don't need to consider this again, unless we find the
> "region_index" means something for dma-buf usage.
>
> The usefulness of this ioctl seems really limited and once again we get into a
> situation where having two ioctls leaves doubt whether this is describing the
> current plane state. Thanks,
>
> Alex
>
> > > > > > > > > include/uapi/linux/vfio.h | 28
> > > > > > > > > ++++++++++++++++++++++++++++
> > > > > > > > > 1 file changed, 28 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230
> > > > > > > > > 100644
> > > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > > @@ -502,6 +502,34 @@ 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)
> > > > > > > > > + *
> > > > > > > > > + * Set the drm_plane_type and retrieve information
> > > > > > > > > +about the gfx
> > > > plane.
> > > > > > > > > + *
> > > > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > > > + */
> > > > > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > > > > + __u32 argsz;
> > > > > > > > > + __u32 flags;
> > > > > > > > > + /* in */
> > > > > > > > > + __u32 drm_plane_type; /* type of plane:
> > > > > > > > > DRM_PLANE_TYPE_*
> > > > */
> > > > > > > > > + /* out */
> > > > > > > > > + __u32 drm_format; /* drm format of plane */
> > > > > > > > > + __u64 drm_format_mod; /* tiled mode */
> > > > > > > > > + __u32 width; /* width of plane */
> > > > > > > > > + __u32 height; /* height of plane */
> > > > > > > > > + __u32 stride; /* stride of plane */
> > > > > > > > > + __u32 size; /* size of plane in bytes, align on
> > > > > > > > > page*/
> > > > > > > > > + __u32 x_pos; /* horizontal position of cursor plane,
> > > > > > > > > upper
> > > > left corner
> > > > > > > > in pixels */
> > > > > > > > > + __u32 y_pos; /* vertical position of cursor plane,
> > > > > > > > > upper left
> > > > corner in
> > > > > > > > lines*/
> > > > > > > > > + __u32 region_index;
> > > > > > > > > + __s32 fd; /* dma-buf fd */
> > > > > > > >
> > > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > > Can I ask for one vs the other? What are the errno values
> > > > > > > > to differentiate unsupported vs not initialized? Is there a
> > > > > > > > "probe"
> > > > > > > > flag that I can use to determine what the device supports
> > > > > > > > without allocating
> > > > > > those resources yet?
> > > > > > > Dma-buf won't use region_index, which means region_index is
> > > > > > > zero all the
> > > > > > time for dma-buf usage.
> > > > > > > As we discussed, there won't be errno if not initialized,
> > > > > > > just keep all fields
> > > > zero.
> > > > > > > I will add the comments about these in the next version.
> > > > > > > Thanks
> > > > > >
> > > > > > Zero is a valid region index.
> > > > > If zero is valid, can we find out other invalid value? How about
> > > > > 0xffffffff?
> > > >
> > > > Unlikely, but also valid. Isn't this why we have a flags field in
> > > > the structure, so we don't need to rely on implicit values as invalid.
> > > > Also, all of the previously discussed usage models needs to be
> > > > documented, either here in the header or in a Documentation/ file.
> > > According to the previously discussion, we also have the following
> > > propose:
> > > enum vfio_device_gfx_type {
> > > VFIO_DEVICE_GFX_NONE,
> > > VFIO_DEVICE_GFX_DMABUF,
> > > VFIO_DEVICE_GFX_REGION,
> > > };
> > >
> > > struct vfio_device_gfx_query_caps {
> > > __u32 argsz;
> > > __u32 flags;
> > > enum vfio_device_gfx_type;
> > > };
> > > So, we may need to add one more ioctl, e.g.
> VFIO_DEVICE_QUERY_GFX_CAPS.
> > > User mode can query this before querying the plan info, and to see
> > > which usage (dma-buf or region) is supported.
> > > Does it still make sense?
> > > Thanks.
> > So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode
> whether the region case is using, instead of introducing a new ioctl.
> > Thanks
> >
> > Tina
> > >
> > > Tina
> > >
> > >
> > > > Thanks,
> > > >
> > > > Alex
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-de...@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
