>-----Original Message----- >From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx] >Sent: Wednesday, June 14, 2017 11:46 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 03:18:31 +0000 >"Chen, Xiaoguang" <xiaoguang.chen@xxxxxxxxx> wrote: > >> >-----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? > >Isn't it already the same without any of the padding here? Without the padding in >vfio_vgpu_plane_info it's 4-byte aligned and we're following it with a 4-byte field, >that works the same on 32 and 64bit. Padding the outer structure here makes no >sense to me. Generally padding at the end of the structure is to allow flexibility in >expanding it within that padding without breaking the ioctl. Here we use _IO and >argsz/flags to do that. Thanks, I got your point. Yes this padding is useless. Thanks very much : ) > >Alex > >> >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
