On Wed, 14 Jun 2017 02:53:24 +0000 "Chen, Xiaoguang" <xiaoguang.chen@xxxxxxxxx> wrote: > >-----Original Message----- > >From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx] > >Sent: Wednesday, June 14, 2017 5:25 AM > >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx> > >Cc: kraxel@xxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; intel- > >gfx@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > >zhenyuw@xxxxxxxxxxxxxxx; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; intel-gvt- > >dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; Tian, Kevin > ><kevin.tian@xxxxxxxxx> > >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations > > > >On Fri, 9 Jun 2017 14:50:40 +0800 > >Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> wrote: > > > >> Here we defined a new ioctl to create a fd for a vfio device based on > >> the input type. Now only one type is supported that is a dma-buf > >> management fd. > >> Two ioctls are defined for the dma-buf management fd: query the vfio > >> vgpu's plane information and create a dma-buf for a plane. > >> > >> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> > >> --- > >> include/uapi/linux/vfio.h | 58 > >> +++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 58 insertions(+) > >> > >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > >> index ae46105..24427b7 100644 > >> --- a/include/uapi/linux/vfio.h > >> +++ b/include/uapi/linux/vfio.h > >> @@ -502,6 +502,64 @@ struct vfio_pci_hot_reset { > >> > >> #define VFIO_DEVICE_PCI_HOT_RESET _IO(VFIO_TYPE, VFIO_BASE + 13) > >> > >> +/** > >> + * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32) > >> + * > >> + * Create a fd for a vfio device based on the input type > >> + * Vendor driver should handle this ioctl to create a fd and manage > >> +the > >> + * life cycle of this fd. > >> + * > >> + * Return: a fd if vendor support that type, -errno if not supported > >> +*/ > >> + > >> +#define VFIO_DEVICE_GET_FD _IO(VFIO_TYPE, VFIO_BASE + 14) > >> + > >> +struct vfio_vgpu_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; > >> + __u32 padding; > >> +}; > >> + > >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types */ > > > >Move this #define up above vfio_vgpu_plane_info to associate it with the > >VFIO_DEVICE_GET_FD ioctl. > OK. > > > > >> + > >> +/* > >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15, > >> + * struct vfio_vgpu_query_plane) > >> + * Query plane information > >> + */ > >> +struct vfio_vgpu_query_plane { > >> + __u32 argsz; > >> + __u32 flags; > >> + struct vfio_vgpu_plane_info plane_info; > >> + __u32 plane_id; > >> + __u32 padding; > > > >This padding doesn't make sense. > This padding is still needed if we do not move the plane_id into vfio_vgpu_plane_info. Right? I don't see why this padding is ever needed, can you explain? Does the structure not being a multiple of 8 bytes affect any of the offsets within the structure? > >> +}; > >> + > >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15) > >> + > >> +/* > >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16, > >> + * struct > >vfio_vgpu_create_dmabuf) > >> + * > >> + * Create a dma-buf for a plane > >> + */ > >> +struct vfio_vgpu_create_dmabuf { > >> + __u32 argsz; > >> + __u32 flags; > >> + struct vfio_vgpu_plane_info plane_info; > >> + __s32 fd; > >> + __u32 plane_id; > >> +}; > > > >Both of these have a plane_id, should plane_id simply replace the padding in > >plane_info? > Precisely speaking plane_id does not belong to the plane info. All the other information are decoded from plane except plane id. Ok, let's keep is separate then. Thanks, Alex > >If not, let's at least put them in the same order so that plane_id is > >after plane_info for both structs. > Ok. > > > > >> + > >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 16) > > > >I don't think these should be named just VFIO_DEVICE_foo, that implies they're > >ioctls on the vfio device fd, they're not. They need to be associated both in name > >and more complete descriptions as ioctls to the fd returned from a request for a > >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps VFIO_DMABUF_MGR_QUERY_PLANE > >and VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using > >"vgpu" in the structure names here either, the ioctls aren't named after vgpus. > >Aren't these rather generic to graphics dmabufs, not specifically vgpus? > Make sense. I will change the names. > > Thanks, > > > >Alex > > > >> + > >> /* -------- API for Type1 VFIO IOMMU -------- */ > >> > >> /** > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
