Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752686AbdFPS3D (ORCPT ); Fri, 16 Jun 2017 14:29:03 -0400 Received: from hqemgate16.nvidia.com ([216.228.121.65]:14677 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750947AbdFPS3C (ORCPT ); Fri, 16 Jun 2017 14:29:02 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Fri, 16 Jun 2017 11:29:01 -0700 Subject: Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations To: Alex Williamson CC: Gerd Hoffmann , Xiaoguang Chen , , , , , , , , 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> <20170616103959.3b6f1681@t450s.home> X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <56561f81-5f83-741d-c645-0d44701deab1@nvidia.com> Date: Fri, 16 Jun 2017 23:58:46 +0530 MIME-Version: 1.0 In-Reply-To: <20170616103959.3b6f1681@t450s.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: 5238 Lines: 145 On 6/16/2017 10:09 PM, Alex Williamson wrote: > On Fri, 16 Jun 2017 19:02:30 +0530 > Kirti Wankhede wrote: > >> On 6/16/2017 2:08 AM, Alex Williamson wrote: >>> On Thu, 15 Jun 2017 18:00:38 +0200 >>> Gerd Hoffmann wrote: >>> >>>> Hi, >>>> >>>>>> +struct vfio_dmabuf_mgr_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; >>>>>> +}; >>>>>> + >>>>> >>>>> This structure is generic, can remove dmabuf from its name, >>>>> vfio_plane_info or vfio_vgpu_surface_info since this will only be >>>>> used >>>>> by vgpu. >>>> >>>> Agree. >>> >>> I'm not sure I agree regarding the vgpu statement, maybe this is not >>> dmabuf specific, but what makes it vgpu specific? We need to separate >>> our current usage plans from what it's actually describing and I don't >>> see that it describes anything vgpu specific. >>> >>>>>> +struct vfio_dmabuf_mgr_query_plane { >>>>>> + __u32 argsz; >>>>>> + __u32 flags; >>>>>> + struct vfio_dmabuf_mgr_plane_info plane_info; >>>>>> + __u32 plane_id; >>>>>> +}; >>>>>> + >>>>>> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15) >>>>>> + >>>>> >>>>> This same interface can be used to query surface/plane information >>>>> for >>>>> both, dmabuf and region, case. Here also 'DMABUF' can be removed and >>>>> define flags if you want to differentiate query for 'dmabuf' and >>>>> 'region'. >>>> >>>> Hmm, any specific reason why you want use a ioctl for that? I would >>>> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the >>>> final name will be) at the start of the region. >>> >>> Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd, >>> if you're exposing a region with the info I wouldn't think you'd want >>> the hassle of managing this separate fd when you could do something >>> like Gerd suggests with defining the first page of the regions as >>> containing the structure. >> >> My suggestion was to use vfio device fd for this ioctl and have dmabuf >> mgr fd as member in above query_plane structure, for region type it >> would be set to 0. >> Yes there is other way to query surface information as Gerd suggested, >> but my point is: if a ioctl is being add, it could be used for both >> types, dmabuf and region. > > I think this suggests abandoning the dmabuf manager fd entirely. That's > not necessarily a bad thing, but I don't think the idea of the dmabuf > manager fd stands if we push one of its primary reasons for existing > back to the device fd. Reading though previous posts, I think we > embraced the dmabuf manager as a separate fd primarily for > consolidation and the potential to use it as a notification point, the > latter being only theoretically useful. > > So perhaps this becomes: > > 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; > }; > > 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; > }; > > The flag defines the data in the id field as either referring to a > region (perhaps there could be multiple regions with only one active) > or a plane ID, which is acquired separately, such as via a dmabuf fd. > This would be retrieved via an optional VFIO_DEVICE_QUERY_GFX_PLANE > ioctl on the vfio device, implemented in the vendor driver. > > Would the above, along with the already defined mechanism for defining > device specific regions, account for NVIDIA's needs? > Yes, works for region type solution that we would go with. Thanks, Kirti > For dmabuf users, we'd still need a new ioctl to get the dmabuf fd. We > could either create a specific ioctl for this (ex. > VFIO_DEVICE_GET_DMABUF_FD) or we could create a shared, generic GET_FD > interface on the device. > >>> Maybe you could even allow mmap of that page >>> to reduce the overhead of getting the current state. >> >> Can't mmap that page to get surface information. There is no way to >> synchronize between QEMU reading this mmapped region and vendor driver >> writing it. There could be race condition in these two operations. Read >> on this page should be trapped and blocking, so that surface in that >> region is only updated when its asked for. >> >>> For the sake of >>> userspace, I'd hope we'd still use the same structure for either the >>> ioctl or region mapping. I'm not really in favor of declaring that >>> this particular ioctl might exist on the device fd when such-and-such >>> region is present otherwise it might exist on a dmabuf manager fd. >> >> Userspace will always use vfio device fd for this ioctl, it only have to >> set proper arguments to its structure based on type. > > Then we should kill off the manager fd unless there are arguments that > still give it value. Thanks, > > Alex >