Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751529AbdFTRHk (ORCPT ); Tue, 20 Jun 2017 13:07:40 -0400 Received: from hqemgate16.nvidia.com ([216.228.121.65]:6368 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750925AbdFTRHj (ORCPT ); Tue, 20 Jun 2017 13:07:39 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Tue, 20 Jun 2017 10:07:38 -0700 Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations To: Alex Williamson , Gerd Hoffmann CC: "Zhang, Tina" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "Chen, Xiaoguang" , "intel-gvt-dev@lists.freedesktop.org" , "Lv, Zhiyuan" , "Wang, Zhi A" , "Wang, Zhenyu Z" References: <1497513611-2814-1-git-send-email-xiaoguang.chen@intel.com> <1497513611-2814-6-git-send-email-xiaoguang.chen@intel.com> <1497542438.29252.1.camel@redhat.com> <20170615143833.7526351b@w520.home> <24c4880b-24f5-ea07-834c-c77d3e895c78@nvidia.com> <1497854312.4207.4.camel@redhat.com> <20170619085530.1f5e46dc@w520.home> <237F54289DF84E4997F34151298ABEBC7C56EBE0@SHSMSX101.ccr.corp.intel.com> <1497956256.16795.7.camel@redhat.com> <20170620090004.44ac7fbc@w520.home> X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <6a0d2856-46d8-1640-150f-88a34d1afd55@nvidia.com> Date: Tue, 20 Jun 2017 22:37:29 +0530 MIME-Version: 1.0 In-Reply-To: <20170620090004.44ac7fbc@w520.home> X-Originating-IP: [10.24.70.186] X-ClientProxiedBy: DRBGMAIL103.nvidia.com (10.18.16.22) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5057 Lines: 145 On 6/20/2017 8:30 PM, Alex Williamson wrote: > On Tue, 20 Jun 2017 12:57:36 +0200 > Gerd Hoffmann wrote: > >> On Tue, 2017-06-20 at 08:41 +0000, Zhang, Tina wrote: >>> Hi, >>> >>> Thanks for all the comments. Here are the summaries: >>> >>> 1. Modify the structures to make it more general. >>> struct vfio_device_gfx_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 generation; >>> }; >> >> Looks good to me. >> >>> struct vfio_device_query_gfx_plane { >>> __u32 argsz; >>> __u32 flags; >>> #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0) >>> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1) >>> struct vfio_device_gfx_plane_info plane_info; >>> __u32 id; >>> }; >> >> I'm not convinced the flags are a great idea. Whenever dmabufs or a >> region is used is a static property of the device, not of each >> individual plane. >> >> >> I think we should have this for userspace to figure: >> >> enum vfio_device_gfx_type { >> VFIO_DEVICE_GFX_NONE, >> VFIO_DEVICE_GFX_DMABUF, >> VFIO_DEVICE_GFX_REGION, >> }; >> >> struct vfio_device_gfx_query_caps { >> __u32 argsz; >> __u32 flags; >> enum vfio_device_gfx_type; >> }; > > We already have VFIO_DEVICE_GET_INFO which returns: > > struct vfio_device_info { > __u32 argsz; > __u32 flags; > #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */ > #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */ > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > }; > > We could use two flag bits to indicate dmabuf or graphics region > support. vfio_device_gfx_query_caps seems to imply a new ioctl, which > would be unnecessary. > Sounds good to me. >> Then this to query the plane: >> >> struct vfio_device_gfx_query_plane { >> __u32 argsz; >> __u32 flags; >> struct vfio_device_gfx_plane_info plane_info; /* out */ >> __u32 plane_type; /* in */ >> }; > > I'm not sure why we're using an enum for something that can currently > be defined with 2 bits, seems like this would be another good use of > flags. We could even embed an enum into the flags if we want to > leave some expansion room, 4 bits maybe? Also, I was imagining that a > device could support multiple graphics regions, that's where specifying > the "id" as a region index seemed useful. We lose that ability here > unless we go back to defining a flag bit to specify how to interpret > this last field. > Right, as I mentioned in earlier reply, we need 2 seperate fields - plane type : DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR - id : fd for dmabuf or region index for region type >> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio >> device fd. >>> VFIO_DEVICE_QUERY_GFX_PLANE : used to query >>> vfio_device_gfx_plane_info. >> >> Yes. >> >>> VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd. > > I'm not convinced this adds value, but I'll list it as an option: > > VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE) > VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD) > > The benefit is that it might help to avoid a proliferation of ioctls on > the device the pain is that we need to either define a field or section > of flags which identify what is being queried or what type of device fd > is being requested. > >> Yes. The plane might have changed between query-plane and get-dmabuf >> ioctl calls though, we must make sure we handle that somehow. Current >> patches return plane_info on get-dmabuf ioctl too, so userspace can see >> what it actually got. >> >> With the generation we can also do something different: Pass in >> plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD return >> an error in case the generation doesn't match. In that case it doesn't >> make much sense any more to have a separate plane_info struct, which >> was added so we don't have to duplicate things in query-plane and get- >> dmabuf ioctl structs. > > I'm not sure I understand how this works for a region, the region is > always the current generation, how can the user ever be sure the > plane_info matches what is exposed in the region? Thanks, > Userspace have to follow the sequence to query plane info (VFIO_DEVICE_QUERY_GFX_PLANE) and then read primary surface from the region. On kernel space side, from VFIO_DEVICE_QUERY_GFX_PLANE ioctl, driver should update surface which is being exposed by the GFX region, fill vfio_device_gfx_plane_info structure then return. GFX region surface would only get updated from this ioctl. Thanks, Kirti