On Wed, 11 Oct 2017 01:46:37 +0000 "Zhang, Tina" <tina.zhang@xxxxxxxxx> wrote: > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx] > > Sent: Wednesday, October 11, 2017 3:17 AM > > To: Zhang, Tina <tina.zhang@xxxxxxxxx> > > Cc: kraxel@xxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx; > > Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; > > Tian, Kevin <kevin.tian@xxxxxxxxx>; daniel@xxxxxxxx; kwankhede@xxxxxxxxxx; > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; linux- > > kernel@xxxxxxxxxxxxxxx; Daniel Vetter <daniel.vetter@xxxxxxxx> > > 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@xxxxxxxxx> 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@xxxxxxxxx> > > > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx> > > > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > --- > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
