Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753669AbdFPNcp (ORCPT ); Fri, 16 Jun 2017 09:32:45 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:12370 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753291AbdFPNcn (ORCPT ); Fri, 16 Jun 2017 09:32:43 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Fri, 16 Jun 2017 06:32:42 -0700 Subject: Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations To: Alex Williamson , Gerd Hoffmann CC: 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> From: Kirti Wankhede X-Nvconfidentiality: public Message-ID: <24c4880b-24f5-ea07-834c-c77d3e895c78@nvidia.com> Date: Fri, 16 Jun 2017 19:02:30 +0530 MIME-Version: 1.0 In-Reply-To: <20170615143833.7526351b@w520.home> X-Originating-IP: [10.24.205.12] 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: 3095 Lines: 92 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. > 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. Thanks, Kirti > Thanks, > > Alex >