>-----Original Message----- >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On >Behalf Of Alex Williamson >Sent: Wednesday, June 14, 2017 11:06 AM >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx> >Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; Lv, >Zhiyuan <zhiyuan.lv@xxxxxxxxx>; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi >A <zhi.a.wang@xxxxxxxxx>; kraxel@xxxxxxxxxx >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations > >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? I thought we add the padding to make sure the structure layout is the same in both 32bit and 64bit systems. Am I right? >Does the structure >not being a multiple of 8 bytes affect any of the offsets within the structure? No. it will not affect any offsets in 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-gvt-dev mailing list >intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
