Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752850AbdFVIaS (ORCPT ); Thu, 22 Jun 2017 04:30:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38218 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751116AbdFVIaP (ORCPT ); Thu, 22 Jun 2017 04:30:15 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DE4C94025D Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=kraxel@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DE4C94025D Message-ID: <1498120215.25651.5.camel@redhat.com> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations From: Gerd Hoffmann To: Alex Williamson Cc: "Zhang, Tina" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , Kirti Wankhede , "Chen, Xiaoguang" , "intel-gvt-dev@lists.freedesktop.org" , "Lv, Zhiyuan" , "Wang, Zhi A" , "Wang, Zhenyu Z" Date: Thu, 22 Jun 2017 10:30:15 +0200 In-Reply-To: <20170621125938.1a92abda@w520.home> 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> <237F54289DF84E4997F34151298ABEBC7C56F3DC@SHSMSX101.ccr.corp.intel.com> <20170620172204.09405cf4@w520.home> <237F54289DF84E4997F34151298ABEBC7C56F843@SHSMSX101.ccr.corp.intel.com> <1498043011.5802.5.camel@redhat.com> <20170621125938.1a92abda@w520.home> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 22 Jun 2017 08:30:15 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5503 Lines: 155 Hi, > > VFIO_DEVICE_FLAGS_GFX_DMABUF? > > After proposing these, I'm kind of questioning their purpose.  In the > case of a GFX region, the user is going to learn that this is > supported > as they parse the region information and find the device specific > region identifying itself as a GFX area.  That needs to happen early > when the user is evaluating the device, so it seems rather redundant > to the flag. Indeed. > If we have dmabuf support, isn't that indicated by trying to query > the > graphics plane_info and getting back a result indicating a dmabuf fd? > Is there any point in time where a device supporting dmabuf fds would > not report this here?  Userspace could really do the same process for > a > graphics region, ie. querying the plane_info, if it exists pursuing > either the region or dmabuf path to get it. Well, you can get a dma-buf only after the guest loaded the driver and initialized the device, so a plane actually exists ... Right now the experimental intel patches throw errors in case no plane exists (yet). Maybe we should have a "bool is_enabled" field in the plane_info struct, so drivers can use that to signal whenever the guest has programmed a valid video mode or not (likewise for the cursor, which doesn't exist with fbcon, only when running xorg). With that in place using the QUERY_PLANE ioctl also for probing looks reasonable. > > generation would be increased each time one of the fields in > > vfio_device_gfx_plane_info changes, typically on mode switches > > (width/height changes) and pageflips (offset changes).  So > > userspace > > can simply compare generation instead of comparing every field to > > figure whenever something changed compared to the previous poll. > > So we have two scenarios, dmabuf and region.  When the user retrieves > a > dmabuf they get plane_info which includes the generation, so they > know > the dmabuf is for that generation.  If they query the plane_info and > get a different generation they should close the previous dmabuf and > get another. Keeping it cached is a valid choice too. > Does this promote the caching idea that a user might > maintain multiple open dmabuf fds and select the appropriate one for > the current device state?  Is it entirely the user's responsibility > to > remember the plane info for an open dmabuf fd? Yes, I'd leave that to userspace. So, when the generation changes userspace knows the guest changed the plane. It could be a configuration the guest has used before (and where userspace could have a cached dma-buf handle for), or it could be something new. > What happens to > existing dmabuf fds when the generation updates, do they stop > refreshing? Depends on what the guest is doing ;) The dma-buf is just a host-side handle for the piece of video memory where the guest stored the framebuffer. > Does it blank the framebuffer? No. > Can the dmabuf fd > transparently update to the new plane_info? No. > The other case is a region, the user queries the plane_info records > the > parameters and region info, and configures access to the region using > that information.  Meanwhile, something changed, plane_info including > generation is updated, but the user is still assuming the previous > plane_info.  How can we avoid such a race? Qemu doesn't. You might get rendering glitches in that case, due to accessing the plane with the wrong configuration. It's fundamentally the same with stdvga btw. > What is the responsibility > of the user and how are they notified to refresh the plane_info? qemu polls in regular intervals, simliar to how it checks the dirty bitmap for video memory in regular intervals with stdvga. qemu display update timer runs on 30fps usually, in case nobody is looking (no spice/vnc client connected) it reduces the update frequency though. > > plane_type should be DRM_PLANE_TYPE_PRIMARY or > > DRM_PLANE_TYPE_CURSOR > > for dmabuf. > > > > Given that nvidia doesn't support a separate cursor plane in their > > region they would support DRM_PLANE_TYPE_PRIMARY only. > > > > I can't see yet what id would be useful for. > > > > Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good > > for. > > I think we're trying to identify where this plane_info exists.  Does > the user ask for a dmabuf fd for it or use a region. But whenever a region or a dmabuf is used is a fixed property of the device, why associate that with a plane? It will be the same for all planes of a device anyway ... > If it's a region, > which region? Ok, if we want support multiple regions. Do we? Using the offset we can place multiple planes in a single region. And I'm not sure nvidia plans to use multiple planes in the first place ... > 4. Two ioctl commands > > VFIO_DEVICE_QUERY_GFX_PLANE > > VFIO_DEVICE_GET_FD > > Are we going to consider a generic VFIO_DEVICE_QUERY ioctl?  Any > opinions?  Thanks, I don't think a generic device query is that helpful. Depending on the kind of query you'll get back different stuff, and we need to handle that somehow, like this: vfio_device_query { u32 argsz; u32 flags; enum query_type; /* or use flags for that */ union { plane_info plane; /* whatever else comes up */ } query_params; }; I fail to see how this is fundamentally different from having multiple vfio_device_query_* structs (and ioctls). It only pushes the problem one level down ... Or do you have something else in mind? cheers, Gerd