DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 28043C0567A2
Date: Mon, 26 Jun 2017 11:28:57 -0600
From: Alex Williamson <alex.williamson@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: "Zhang, Tina" <tina.zhang@intel.com>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        "Chen, Xiaoguang" <xiaoguang.chen@intel.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "Lv, Zhiyuan" <zhiyuan.lv@intel.com>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>,
        "Wang, Zhenyu Z" <zhenyu.z.wang@intel.com>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
 operations
Message-ID: <20170626112857.37c2aa65@w520.home>
In-Reply-To: <1498459157.20591.6.camel@redhat.com>
References: <1497513611-2814-1-git-send-email-xiaoguang.chen@intel.com>
        <1497513611-2814-6-git-send-email-xiaoguang.chen@intel.com>
        <f19da207-a7f5-d3d3-4577-b071e7d75cf7@nvidia.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>
        <237F54289DF84E4997F34151298ABEBC7C5731B3@SHSMSX101.ccr.corp.intel.com>
        <1498459157.20591.6.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2914
Lines: 60

On Mon, 26 Jun 2017 08:39:17 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > 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.  
> > 
> > Comparing with the current patch, this would make user space a little
> > bit harder to
> > get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it
> > efficient for
> > user mode usage?  
> 
> user space has to call QUERY-PLANE first, then looks if it has a dma-
> buf for that, if not call GET-DMABUF.
> 
> Problem is the guest could have changed the plane between the QUERY-
> PLANE and GET-DMABUF ioctls.
> 
> Current patches (v8 series) just returns plane-info on GET-DMABUF too,
> so userspace can at least detect something changed.
> 
> It would be easier for userspace if GET-DMABUF throws an error in case
> the plane changed since the last QUERY-PLANE ioctl.  The generation id
> would be one way to handle it, but possibly it is easier if the kernel
> driver just keeps track internally.  So GET-DMABUF would be defined to
> return a dmabuf for the plane returned by the previous QUERY-PLANE
> ioctl (on the same file handle), or return an error in case the plane
> has changed meanwhile.

Hmm, I don't like that interface.  Can you cite examples of other
ioctls that behave this way?  It doesn't feel like an elegant user
interface; the user can get the dmabuf, but only after they query the
dmabuf, even though the get-dmabuf ioctl returns the same data as the
query-plane ioctl, but they can't get the dmabuf if the plane has
changed in the interim, which is not something the user can know.  Are
we causing our own problems with this model of cycling through dmabuf
fds?  We talked previously about an enum of plane types, primary and
cursor.  What if the user was simply able to get a dmabuf fd for each of
those and they queried the current plane information via those fds?
IOW, the fd is persistent and specific to a given plane type, but the
format within it is dynamic.  For instance, I don't have a separate
monitor on my desktop for each resolution I want to run, the monitor
adapts to the signal it gets.  I don't grasp the technical reasons why
the user can't stop using the dmabuf fd with the previous format
parameters and start using it with the new parameters.  Maybe the user
even has multiple dmabuf fds open, but they switch to only actively
using then one(s) that match the current format.  I don't know if
that's viable, but there seems to be a fundamental synchronization
issue if a given dmabuf fd only represents a transient state.  Thanks,

Alex