Message-ID: <20170622125458.01c953ab@w520.home>
Date:   Thu, 22 Jun 2017 12:54:58 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Gerd Hoffmann <kraxel@...hat.com>
Cc:     "Zhang, Tina" <tina.zhang@...el.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kirti Wankhede <kwankhede@...dia.com>,
        "Chen, Xiaoguang" <xiaoguang.chen@...el.com>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>,
        "Lv, Zhiyuan" <zhiyuan.lv@...el.com>,
        "Wang, Zhi A" <zhi.a.wang@...el.com>,
        "Wang, Zhenyu Z" <zhenyu.z.wang@...el.com>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
 operations

On Thu, 22 Jun 2017 10:30:15 +0200
Gerd Hoffmann <kraxel@...hat.com> wrote:

>   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 ...

Is this only going to support accelerated driver output, not basic VGA
modes for BIOS interaction?
 
> 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.

Sure, or -ENOTTY for ioctl not implemented vs -EINVAL for no available
plane, but then that might not help the user know how a plane would be
available if it were available.

> > > 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.

So generation is just intended to be a unique handle, like a uuid but
cheaper.  Generally I think of a generation field only to track what's
current.  Userspace might assume a "generation" never goes backwards
(until it wraps).

> > 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.

But userspace also doesn't know that a dmabuf generation will ever be
visited again, so they're bound to have some stale descriptors.  Are
we thinking userspace would have some LRU list of dmabufs so that they
don't collect too many?  Each uses some resources,  do we just rely on
open file handles to set an upper limit?
 
> > 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.

So the resources the user is holding if they don't release their dmabuf
are potentially non-trivial.  The user could also have this video
memory mmap'd, which makes it harder to recover from the user.  This
seems like a problem.
 
> > Does it blank the framebuffer?  
> 
> No.
> 
> > Can the dmabuf fd
> > transparently update to the new plane_info?  
> 
> No.

So the user hold a reference to video memory with no idea whether it
will be reused, we have no way to tell them to release that reference
or mechanism to force them to do so... something is wrong here.

> > 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 ...

I don't want to take a driver ioctl interface as a throw away, one time
use exercise.  If we can think of such questions now, let's define how
they work.  A device could have multiple graphics regions with multiple
planes within each region.  Do we also want to exclude that device
needs to be strictly region or dmabuf?  Maybe it does both.  Or maybe
it supports dmabuf-ng (ie. whatever comes next).

> > 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?

We don't have an infinite number of ioctls and each ioctl defines a
user interface that needs to be supported and maintain an ABI.  So if
we define something like above where we can screw up some of the query
types without throwing away the entire ioctl, I think that's a good
thing.  Thanks,

Alex

Powered by blists - more mailing lists