Message-ID: <1498120215.25651.5.camel@redhat.com>
Date:   Thu, 22 Jun 2017 10:30:15 +0200
From:   Gerd Hoffmann <kraxel@...hat.com>
To:     Alex Williamson <alex.williamson@...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

  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

Powered by blists - more mailing lists