Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754155AbdFNCxc convert rfc822-to-8bit (ORCPT ); Tue, 13 Jun 2017 22:53:32 -0400 Received: from mga11.intel.com ([192.55.52.93]:20842 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753382AbdFNCxb (ORCPT ); Tue, 13 Jun 2017 22:53:31 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,340,1493708400"; d="scan'208";a="980551693" From: "Chen, Xiaoguang" To: Alex Williamson CC: "kraxel@redhat.com" , "chris@chris-wilson.co.uk" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "zhenyuw@linux.intel.com" , "Lv, Zhiyuan" , "intel-gvt-dev@lists.freedesktop.org" , "Wang, Zhi A" , "Tian, Kevin" Subject: RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations Thread-Topic: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations Thread-Index: AQHS4O0B+rtVVjzuBUyRMhjd8q2t06Iizw6AgADdRNA= Date: Wed, 14 Jun 2017 02:53:24 +0000 Message-ID: References: <1496991042-2265-1-git-send-email-xiaoguang.chen@intel.com> <1496991042-2265-5-git-send-email-xiaoguang.chen@intel.com> <20170613152459.6b120a7a@w520.home> In-Reply-To: <20170613152459.6b120a7a@w520.home> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4189 Lines: 134 >-----Original Message----- >From: Alex Williamson [mailto:alex.williamson@redhat.com] >Sent: Wednesday, June 14, 2017 5:25 AM >To: Chen, Xiaoguang >Cc: kraxel@redhat.com; chris@chris-wilson.co.uk; intel- >gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; >zhenyuw@linux.intel.com; Lv, Zhiyuan ; intel-gvt- >dev@lists.freedesktop.org; Wang, Zhi A ; Tian, Kevin > >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 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 >> --- >> 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? > >> +}; >> + >> +#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. >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 -------- */ >> >> /**