| Message ID | 1507629007-3183-6-git-send-email-tina.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-10-10 at 17:50 +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. This ioctl can be invoked > with: > 1) either flag DMABUF or REGION is set. Vendor driver returns success > and > the plane_info only when the specific kind of buffer is supported. > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver > returns > success only when the specific kind of buffer is supported. > > 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 needs to create new resource according > to > the new fd or just use the existed resource related to the old fd. Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> cheers, Gerd
On Tue, 10 Oct 2017 17:50:05 +0800 Tina Zhang <tina.zhang@intel.com> wrote: > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and > get the plan and its related information. This ioctl can be invoked with: s/plan/plane/ > 1) either flag DMABUF or REGION is set. Vendor driver returns success and > the plane_info only when the specific kind of buffer is supported. > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver returns > success only when the specific kind of buffer is supported. > > 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 needs to create new resource according to > the new fd or just use the existed resource related to the old fd. > > v15: > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd) > > v14: > - add PROBE, DMABUF and REGION flags. (Alex) > > v12: > - add drm_format_mod back. (Gerd and Zhenyu) > - add region_index. (Gerd) > > v11: > - rename plane_type to drm_plane_type. (Gerd) > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. > (Gerd) > - remove drm_format_mod, start fields. (Daniel) > - remove plane_id. > > v10: > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd) > > v3: > - 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. (Gerd) > > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > include/uapi/linux/vfio.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index ae46105..fdf9a9c 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -502,6 +502,68 @@ 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 flags, then retrieve the gfx plane info. > + * > + * flags supported: > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF are set > + * to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no > + * support for dma-buf. > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION are set > + * to ask if the mdev supports region. 0 on support, -EINVAL on no > + * support for region. > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION is set > + * with each call to query the plane info. So dmabuf_id is effectively just a token that can be fed into GET_GFX_DMABUF to get the fd. The implementation of the token is vendor specific, but can be thought of as some sort of sequence ID or generation ID (but not necessarily monotonically increasing), so GET_GFX_DMABUF may fail if the previously provided dmabuf_id is no longer valid. Do I have this correct? > + * - Others are invalid and return -EINVAL. And I see that in patch 7/7 that i915 is checking explicitly for only these flag combinations, great! > + * > + * Return: 0 on success, -ENODEV with all out fields zero on mdev > + * device initialization, -errno on other failure. > + */ > +struct vfio_device_gfx_plane_info { > + __u32 argsz; > + __u32 flags; > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) > +#define VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) > +#define VFIO_GFX_PLANE_TYPE_REGION (1 << 2) > + /* 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 */ > + __u32 y_pos; /* vertical position of cursor plane*/ > + union { > + __u32 region_index; /* region index */ > + __s32 dmabuf_id; /* dma-buf fd */ "dma-buf fd", but it's not an fd. Why is this signed since it's no longer an fd? > + }; > +}; > + > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14) > + > +/** > + * VFIO_DEVICE_GET_GFX_DMABUF - _IOW(VFIO_TYPE, VFIO_BASE + 15, > + * struct vfio_device_gfx_dmabuf_fd) > + * > + * Return: 0 on success, -errno on failure. > + */ So given a dmabuf_id, return a dmabuf_fd, which may be the same as a fd previously returned to the user. In the latter case, can we assume that there's no new reference added to the existing fd? Is the user permitted to cache the dmabuf_id to dmabuf_fd mapping? Similarly, if a user already has a dmabuf_fd, can calling GET_GFX_DMABUF ever fail or return a different result for the same dmabuf_id which provided that fd? Maybe we could define some of these behaviors in the header file to keep vendor implementations consistent. > +struct vfio_device_gfx_dmabuf_fd { > + __u32 argsz; > + __u32 flags; Patch 7/7 however fails to validate this flags field. It needs to return -EINVAL if any unsupported flags are set, which at this point is all of them. > + /* in */ > + __u32 dmabuf_id; This unsigned dmabuf_id is probably correct, but it doesn't match the signed one provided above. > + /* out */ > + __s32 dmabuf_fd; > +}; VFIO_GROUP_GET_DEVICE_FD returns the fd through the return code, is there a reason to do it differently here? GET_DEVICE_FD also only takes a char parameter without the extra future proofing, I wonder if this ioctl is simple enough to do the same. > + > +#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /** Thanks, Alex
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Wednesday, October 11, 2017 3:17 AM > To: Zhang, Tina <tina.zhang@intel.com> > Cc: 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; kwankhede@nvidia.com; > intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; linux- > kernel@vger.kernel.org; Daniel Vetter <daniel.vetter@ffwll.ch> > Subject: Re: [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation > > On Tue, 10 Oct 2017 17:50:05 +0800 > Tina Zhang <tina.zhang@intel.com> wrote: > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query > > and get the plan and its related information. This ioctl can be invoked with: > > s/plan/plane/ Sorry about this typo :). > > > 1) either flag DMABUF or REGION is set. Vendor driver returns success > > and the plane_info only when the specific kind of buffer is supported. > > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver > > returns success only when the specific kind of buffer is supported. > > > > 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 needs to create new resource according to > > the new fd or just use the existed resource related to the old fd. > > > > v15: > > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd) > > > > v14: > > - add PROBE, DMABUF and REGION flags. (Alex) > > > > v12: > > - add drm_format_mod back. (Gerd and Zhenyu) > > - add region_index. (Gerd) > > > > v11: > > - rename plane_type to drm_plane_type. (Gerd) > > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. > > (Gerd) > > - remove drm_format_mod, start fields. (Daniel) > > - remove plane_id. > > > > v10: > > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd) > > > > v3: > > - 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. (Gerd) > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Alex Williamson <alex.williamson@redhat.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > include/uapi/linux/vfio.h | 62 > > +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > index ae46105..fdf9a9c 100644 > > --- a/include/uapi/linux/vfio.h > > +++ b/include/uapi/linux/vfio.h > > @@ -502,6 +502,68 @@ 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 flags, then retrieve the gfx plane info. > > + * > > + * flags supported: > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF > are set > > + * to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no > > + * support for dma-buf. > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION > are set > > + * to ask if the mdev supports region. 0 on support, -EINVAL on no > > + * support for region. > > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION > is set > > + * with each call to query the plane info. > > So dmabuf_id is effectively just a token that can be fed into GET_GFX_DMABUF > to get the fd. The implementation of the token is vendor specific, but can be > thought of as some sort of sequence ID or generation ID (but not necessarily > monotonically increasing), so GET_GFX_DMABUF may fail if the previously > provided dmabuf_id is no longer valid. Do I have this correct? Exactly. GET_GFX_DMABUF may fail if the dmabuf_id is no longer valid. -EINVAL will be returned in that case. And dmabuf_id is invalid when: 1) user space has closed all dma-buf fds which are exposed with the dmabuf_id, 2) or the dmabuf_id isn't returned from QUERY_GFX_PLANE. I will add the comments. Thanks. > > > + * - Others are invalid and return -EINVAL. > > And I see that in patch 7/7 that i915 is checking explicitly for only these flag > combinations, great! > > > + * > > + * Return: 0 on success, -ENODEV with all out fields zero on mdev > > + * device initialization, -errno on other failure. > > + */ > > +struct vfio_device_gfx_plane_info { > > + __u32 argsz; > > + __u32 flags; > > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) #define > > +VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) #define > > +VFIO_GFX_PLANE_TYPE_REGION (1 << 2) > > + /* 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 */ > > + __u32 y_pos; /* vertical position of cursor plane*/ > > + union { > > + __u32 region_index; /* region index */ > > + __s32 dmabuf_id; /* dma-buf fd */ > > "dma-buf fd", but it's not an fd. Why is this signed since it's no longer an fd? Again, sorry about the typo :) > > > + }; > > +}; > > + > > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14) > > + > > +/** > > + * VFIO_DEVICE_GET_GFX_DMABUF - _IOW(VFIO_TYPE, VFIO_BASE + 15, > > + * struct vfio_device_gfx_dmabuf_fd) > > + * > > + * Return: 0 on success, -errno on failure. > > + */ > > So given a dmabuf_id, return a dmabuf_fd, which may be the same as a fd > previously returned to the user. In the latter case, can we assume that there's > no new reference added to the existing fd? Is the user permitted to cache the > dmabuf_id to dmabuf_fd mapping? Similarly, if a user already has a dmabuf_fd, > can calling GET_GFX_DMABUF ever fail or return a different result for the same > dmabuf_id which provided that fd? Maybe we could define some of these > behaviors in the header file to keep vendor implementations consistent. Actually, a new fd is returned with each time GET_GFX_DMABUF invoked, even with the same dmabuf_id. The benefit is to leave the dma-buf's life cycle management to userspace. The dma-buf will be released when all its fds in userspace are closed. Then, I think I need to add some comments like this: "dmabuf_fd will return a new fd with each time GET_GFX_DMABUF being invoked" > > > +struct vfio_device_gfx_dmabuf_fd { > > + __u32 argsz; > > + __u32 flags; > > Patch 7/7 however fails to validate this flags field. It needs to return -EINVAL if > any unsupported flags are set, which at this point is all of them. Indeed. > > > + /* in */ > > + __u32 dmabuf_id; > > This unsigned dmabuf_id is probably correct, but it doesn't match the signed > one provided above. Will make them both unsigned :) > > > + /* out */ > > + __s32 dmabuf_fd; > > +}; > > VFIO_GROUP_GET_DEVICE_FD returns the fd through the return code, is there > a reason to do it differently here? GET_DEVICE_FD also only takes a char > parameter without the extra future proofing, I wonder if this ioctl is simple > enough to do the same. So that means we don't need the "flages" field and change dmabuf_id to a character string. Maybe it's fine for the current usage. But, I'm not sure if it's suitable for future. Gerd, Zhenyu and Zhi, how do you think about it? Thanks. Tina > > > + > > +#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15) > > + > > /* -------- API for Type1 VFIO IOMMU -------- */ > > > > /** > > Thanks, > Alex
On Wed, 11 Oct 2017 01:46:37 +0000 "Zhang, Tina" <tina.zhang@intel.com> wrote: > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Wednesday, October 11, 2017 3:17 AM > > To: Zhang, Tina <tina.zhang@intel.com> > > Cc: 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; kwankhede@nvidia.com; > > intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; linux- > > kernel@vger.kernel.org; Daniel Vetter <daniel.vetter@ffwll.ch> > > Subject: Re: [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation > > > > On Tue, 10 Oct 2017 17:50:05 +0800 > > Tina Zhang <tina.zhang@intel.com> wrote: > > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query > > > and get the plan and its related information. This ioctl can be invoked with: > > > > s/plan/plane/ > Sorry about this typo :). > > > > > > 1) either flag DMABUF or REGION is set. Vendor driver returns success > > > and the plane_info only when the specific kind of buffer is supported. > > > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver > > > returns success only when the specific kind of buffer is supported. > > > > > > 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 needs to create new resource according to > > > the new fd or just use the existed resource related to the old fd. > > > > > > v15: > > > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd) > > > > > > v14: > > > - add PROBE, DMABUF and REGION flags. (Alex) > > > > > > v12: > > > - add drm_format_mod back. (Gerd and Zhenyu) > > > - add region_index. (Gerd) > > > > > > v11: > > > - rename plane_type to drm_plane_type. (Gerd) > > > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. > > > (Gerd) > > > - remove drm_format_mod, start fields. (Daniel) > > > - remove plane_id. > > > > > > v10: > > > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd) > > > > > > v3: > > > - 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. (Gerd) > > > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > > Cc: Alex Williamson <alex.williamson@redhat.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > include/uapi/linux/vfio.h | 62 > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 62 insertions(+) > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > index ae46105..fdf9a9c 100644 > > > --- a/include/uapi/linux/vfio.h > > > +++ b/include/uapi/linux/vfio.h > > > @@ -502,6 +502,68 @@ 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 flags, then retrieve the gfx plane info. > > > + * > > > + * flags supported: > > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF > > are set > > > + * to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no > > > + * support for dma-buf. > > > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION > > are set > > > + * to ask if the mdev supports region. 0 on support, -EINVAL on no > > > + * support for region. > > > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION > > is set > > > + * with each call to query the plane info. > > > > So dmabuf_id is effectively just a token that can be fed into GET_GFX_DMABUF > > to get the fd. The implementation of the token is vendor specific, but can be > > thought of as some sort of sequence ID or generation ID (but not necessarily > > monotonically increasing), so GET_GFX_DMABUF may fail if the previously > > provided dmabuf_id is no longer valid. Do I have this correct? > Exactly. GET_GFX_DMABUF may fail if the dmabuf_id is no longer valid. -EINVAL will be > returned in that case. > And dmabuf_id is invalid when: > 1) user space has closed all dma-buf fds which are exposed with the dmabuf_id, > 2) or the dmabuf_id isn't returned from QUERY_GFX_PLANE. > I will add the comments. Thanks. > > > > > > + * - Others are invalid and return -EINVAL. > > > > And I see that in patch 7/7 that i915 is checking explicitly for only these flag > > combinations, great! > > > > > + * > > > + * Return: 0 on success, -ENODEV with all out fields zero on mdev > > > + * device initialization, -errno on other failure. > > > + */ > > > +struct vfio_device_gfx_plane_info { > > > + __u32 argsz; > > > + __u32 flags; > > > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) #define > > > +VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) #define > > > +VFIO_GFX_PLANE_TYPE_REGION (1 << 2) > > > + /* 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 */ > > > + __u32 y_pos; /* vertical position of cursor plane*/ > > > + union { > > > + __u32 region_index; /* region index */ > > > + __s32 dmabuf_id; /* dma-buf fd */ > > > > "dma-buf fd", but it's not an fd. Why is this signed since it's no longer an fd? > Again, sorry about the typo :) > > > > > > + }; > > > +}; > > > + > > > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14) > > > + > > > +/** > > > + * VFIO_DEVICE_GET_GFX_DMABUF - _IOW(VFIO_TYPE, VFIO_BASE + 15, > > > + * struct vfio_device_gfx_dmabuf_fd) > > > + * > > > + * Return: 0 on success, -errno on failure. > > > + */ > > > > So given a dmabuf_id, return a dmabuf_fd, which may be the same as a fd > > previously returned to the user. In the latter case, can we assume that there's > > no new reference added to the existing fd? Is the user permitted to cache the > > dmabuf_id to dmabuf_fd mapping? Similarly, if a user already has a dmabuf_fd, > > can calling GET_GFX_DMABUF ever fail or return a different result for the same > > dmabuf_id which provided that fd? Maybe we could define some of these > > behaviors in the header file to keep vendor implementations consistent. > > Actually, a new fd is returned with each time GET_GFX_DMABUF invoked, even > with the same dmabuf_id. The benefit is to leave the dma-buf's life cycle management > to userspace. The dma-buf will be released when all its fds in userspace are closed. Ok, so userspace could actually have multiple dmabuf_fds open at the same time for the same dmabuf_id if they were sloppy with management of those fds. > Then, I think I need to add some comments like this: > "dmabuf_fd will return a new fd with each time GET_GFX_DMABUF being invoked" Yes, that would be great. > > > > > +struct vfio_device_gfx_dmabuf_fd { > > > + __u32 argsz; > > > + __u32 flags; > > > > Patch 7/7 however fails to validate this flags field. It needs to return -EINVAL if > > any unsupported flags are set, which at this point is all of them. > Indeed. > > > > > > + /* in */ > > > + __u32 dmabuf_id; > > > > This unsigned dmabuf_id is probably correct, but it doesn't match the signed > > one provided above. > Will make them both unsigned :) > > > > > > + /* out */ > > > + __s32 dmabuf_fd; > > > +}; > > > > VFIO_GROUP_GET_DEVICE_FD returns the fd through the return code, is there > > a reason to do it differently here? GET_DEVICE_FD also only takes a char > > parameter without the extra future proofing, I wonder if this ioctl is simple > > enough to do the same. > So that means we don't need the "flages" field and change dmabuf_id to a character string. > Maybe it's fine for the current usage. But, I'm not sure if it's suitable for future. > Gerd, Zhenyu and Zhi, how do you think about it? No, the parameter wouldn't be a char, you'd use an __u32 for the dmabuf_id. I'm just referencing the structure of the GET_DEVICE_FD as an ioctl which returns an fd through the return value and takes a single parameter. I don't mean to imply matching the type of that parameter. Gerd, what are your thoughts on that, it'd make it slightly easier to call GET_GFX_DMABUF, but it limits us to file descriptor for the dmabuf whereas with a flag and expandable structure we could use some future mechanism to return the dmabuf to userspace. Thanks, Alex
Hi, > No, the parameter wouldn't be a char, you'd use an __u32 for the > dmabuf_id. I'm just referencing the structure of the GET_DEVICE_FD > as > an ioctl which returns an fd through the return value and takes a > single parameter. I don't mean to imply matching the type of that > parameter. > Gerd, what are your thoughts on that, it'd make it slightly > easier to call GET_GFX_DMABUF, but it limits us to file descriptor > for > the dmabuf whereas with a flag and expandable structure we could use > some future mechanism to return the dmabuf to userspace. It's fine with me. The point of using fd's to refer to dmabufs is that they can be passed around easily, so I doubt this is going to change. cheers, Gerd
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index ae46105..fdf9a9c 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -502,6 +502,68 @@ 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 flags, then retrieve the gfx plane info. + * + * flags supported: + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF are set + * to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no + * support for dma-buf. + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION are set + * to ask if the mdev supports region. 0 on support, -EINVAL on no + * support for region. + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION is set + * with each call to query the plane info. + * - Others are invalid and return -EINVAL. + * + * Return: 0 on success, -ENODEV with all out fields zero on mdev + * device initialization, -errno on other failure. + */ +struct vfio_device_gfx_plane_info { + __u32 argsz; + __u32 flags; +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) +#define VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) +#define VFIO_GFX_PLANE_TYPE_REGION (1 << 2) + /* 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 */ + __u32 y_pos; /* vertical position of cursor plane*/ + union { + __u32 region_index; /* region index */ + __s32 dmabuf_id; /* dma-buf fd */ + }; +}; + +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14) + +/** + * VFIO_DEVICE_GET_GFX_DMABUF - _IOW(VFIO_TYPE, VFIO_BASE + 15, + * struct vfio_device_gfx_dmabuf_fd) + * + * Return: 0 on success, -errno on failure. + */ +struct vfio_device_gfx_dmabuf_fd { + __u32 argsz; + __u32 flags; + /* in */ + __u32 dmabuf_id; + /* out */ + __s32 dmabuf_fd; +}; + +#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15) + /* -------- API for Type1 VFIO IOMMU -------- */ /**
Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and get the plan and its related information. This ioctl can be invoked with: 1) either flag DMABUF or REGION is set. Vendor driver returns success and the plane_info only when the specific kind of buffer is supported. 2) flag PROBE is set with either DMABUF or REGION. Vendor driver returns success only when the specific kind of buffer is supported. 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 needs to create new resource according to the new fd or just use the existed resource related to the old fd. v15: - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd) v14: - add PROBE, DMABUF and REGION flags. (Alex) v12: - add drm_format_mod back. (Gerd and Zhenyu) - add region_index. (Gerd) v11: - rename plane_type to drm_plane_type. (Gerd) - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. (Gerd) - remove drm_format_mod, start fields. (Daniel) - remove plane_id. v10: - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd) v3: - 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. (Gerd) Signed-off-by: Tina Zhang <tina.zhang@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- include/uapi/linux/vfio.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)