RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
> -Original Message- > From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On > Behalf Of Daniel Vetter > Sent: Thursday, June 29, 2017 4:39 PM > To: Gerd Hoffmann > Cc: Wang, Zhenyu Z ; intel- > g...@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang > ; Zhang, Tina ; Alex > Williamson ; Lv, Zhiyuan > ; Kirti Wankhede ; intel-gvt- > d...@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf > operations > > On Thu, Jun 29, 2017 at 08:41:53AM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > > Does gvt track the live cycle of all dma-bufs it has handed out? > > > > > > The V9 implementation does track the dma-bufs' live cycle. The > > > original idea was that leaving the dma-bufs' live cycle management > > > to user mode. > > > > That is still the case, user space decides which dma-bufs it'll go > > keep cached. But kernel space can see what user space is doing, so > > there is no need to explicitly tell the kernel whenever a cached > > dma-buf exists or not. > > We do the same trick in drm_prime.c, keeping a cache of exported dma-buf > around for re-exporting. Since for prime sharing the use-case is almost always > re-importing as a drm gem buffer again we can then on re-import also tell > userspace whether it already has that buffer in it's userspace buffer manager, > but that's an additional optimization. With plain dma-buf we could achieve the > same by wiring up a real stat() implementation with unique inode numbers (atm > they all share the anon_inode singleton). But thus far no one asked for that. Thanks. I'm going to submit the v10 version of ABI interface. > > btw I'm lost a bit in the discussion (was on vacation), but I think all the > concerns > I've noticed with the initial rfc have been raised already, so things look > good. I'll > check the next rfc once that shows up. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > ___ > intel-gvt-dev mailing list > intel-gvt-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Thu, Jun 29, 2017 at 08:41:53AM +0200, Gerd Hoffmann wrote: > Hi, > > > > Does gvt track the live cycle of all dma-bufs it has handed out? > > > > The V9 implementation does track the dma-bufs' live cycle. The > > original idea was that leaving the dma-bufs' live cycle management to > > user mode. > > That is still the case, user space decides which dma-bufs it'll go keep > cached. But kernel space can see what user space is doing, so there is > no need to explicitly tell the kernel whenever a cached dma-buf exists > or not. We do the same trick in drm_prime.c, keeping a cache of exported dma-buf around for re-exporting. Since for prime sharing the use-case is almost always re-importing as a drm gem buffer again we can then on re-import also tell userspace whether it already has that buffer in it's userspace buffer manager, but that's an additional optimization. With plain dma-buf we could achieve the same by wiring up a real stat() implementation with unique inode numbers (atm they all share the anon_inode singleton). But thus far no one asked for that. btw I'm lost a bit in the discussion (was on vacation), but I think all the concerns I've noticed with the initial rfc have been raised already, so things look good. I'll check the next rfc once that shows up. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi, > > Does gvt track the live cycle of all dma-bufs it has handed out? > > The V9 implementation does track the dma-bufs' live cycle. The > original idea was that leaving the dma-bufs' live cycle management to > user mode. That is still the case, user space decides which dma-bufs it'll go keep cached. But kernel space can see what user space is doing, so there is no need to explicitly tell the kernel whenever a cached dma-buf exists or not. cheers, Gerd
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
> -Original Message- > From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On > Behalf Of Gerd Hoffmann > Sent: Tuesday, June 27, 2017 2:13 PM > To: Alex Williamson > Cc: Wang, Zhenyu Z ; intel- > g...@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang > ; Zhang, Tina ; Kirti > Wankhede ; Lv, Zhiyuan ; > intel-gvt-...@lists.freedesktop.org; Wang, Zhi A > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf > operations > > Hi, > > > 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. > > Will not work due to how dma-bufs are designed. > > But, yes, the QUERY then GET split is ugly for a number of reasons. > > Does gvt track the live cycle of all dma-bufs it has handed out? The V9 implementation does track the dma-bufs' live cycle. The original idea was that leaving the dma-bufs' live cycle management to user mode. > If so, then maybe we can let the kernel check whenever a dma-buf for the > current plane exists? And if that isn't the case hand out a dma- buf right > away, > without expecting userspace explicitly asking for it? I think this is a good advice. We are going to try this idea and add some tracking logic to kernel mode. > > That will simplify the interface and remove the race condition at the expense > of > some additional bookkeeping in the kernel. In this case, maybe one ioctl like QUERY_PLAN is enough. We can block it this ioctl and return it when the fd and info are ready. Thanks. Tina > > cheers, > Gerd > > ___ > intel-gvt-dev mailing list > intel-gvt-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi, > 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. Will not work due to how dma-bufs are designed. But, yes, the QUERY then GET split is ugly for a number of reasons. Does gvt track the live cycle of all dma-bufs it has handed out? If so, then maybe we can let the kernel check whenever a dma-buf for the current plane exists? And if that isn't the case hand out a dma- buf right away, without expecting userspace explicitly asking for it? That will simplify the interface and remove the race condition at the expense of some additional bookkeeping in the kernel. cheers, Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Mon, 26 Jun 2017 08:39:17 +0200 Gerd Hoffmann 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
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
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. cheers, Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi, > > So maybe a "enum plane_state" (instead of "bool is_enabled")? So > > we > > can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases? > > What's the difference between NOT_SUPPORTED and -ENOTTY on the ioctl? > Perhaps a bit in a flags field could specify EN/DIS-ABLED and leave > room for things we're forgetting. So throw error in the NOT_SUPPORTED case, otherwise set/clear a PLANE_ENABLED bit in flags? Yep, that will work too. cheers, Gerd
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Tuesday, June 20, 2017 6:58 PM
> To: Zhang, Tina ; Alex Williamson
>
> Cc: intel-...@lists.freedesktop.org; linux-kernel@vger.kernel.org; Kirti
> Wankhede ; Chen, Xiaoguang
> ; intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan
> ; Wang, Zhi A ; Wang, Zhenyu
> Z
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > Hi,
> >
> > Thanks for all the comments. Here are the summaries:
> >
> > 1. Modify the structures to make it more general.
> > struct vfio_device_gfx_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 generation;
> > };
>
> Looks good to me.
>
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > };
>
> I'm not convinced the flags are a great idea. Whenever dmabufs or a region is
> used is a static property of the device, not of each individual plane.
>
>
> I think we should have this for userspace to figure:
>
> enum vfio_device_gfx_type {
> VFIO_DEVICE_GFX_NONE,
> VFIO_DEVICE_GFX_DMABUF,
> VFIO_DEVICE_GFX_REGION,
> };
>
> struct vfio_device_gfx_query_caps {
> __u32 argsz;
> __u32 flags;
> enum vfio_device_gfx_type;
> };
>
>
> Then this to query the plane:
>
> struct vfio_device_gfx_query_plane {
> __u32 argsz;
> __u32 flags;
> struct vfio_device_gfx_plane_info plane_info; /* out */
> __u32 plane_type; /* in */
> };
>
>
> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device
> fd.
> > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > vfio_device_gfx_plane_info.
>
> Yes.
>
> > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
>
> Yes. The plane might have changed between query-plane and get-dmabuf ioctl
> calls though, we must make sure we handle that somehow. Current patches
> return plane_info on get-dmabuf ioctl too, so userspace can see what it
> actually
> got.
>
> 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?
Thanks
Tina
>
> cheers,
> Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Fri, 23 Jun 2017 09:26:59 +0200 Gerd Hoffmann wrote: > Hi, > > > Is this only going to support accelerated driver output, not basic > > VGA > > modes for BIOS interaction? > > Right now there is no vgabios or uefi support for the vgpu. > > But even with that in place there still is the problem that the display > device initialization happens before the guest runs and therefore there > isn't an plane yet ... > > > > 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. > > So maybe a "enum plane_state" (instead of "bool is_enabled")? So we > can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases? What's the difference between NOT_SUPPORTED and -ENOTTY on the ioctl? Perhaps a bit in a flags field could specify EN/DIS-ABLED and leave room for things we're forgetting. Keep in mind that we need to use explicit width fields for a uapi structure, so __u32 vs enum. Thanks, Alex
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Fri, 23 Jun 2017 10:31:28 +0200 Gerd Hoffmann wrote: > On Fri, 2017-06-23 at 15:49 +0800, Zhi Wang wrote: > > Hi: > > Thanks for the discussions! If the userspace application has > > already maintained a LRU list, it looks like we don't need > > generation > > anymore, > > generation isn't required, things are working just fine without that. > It is just a small optimization, userspace can skip the LRU lookup > altogether if the generation didn't change. > > But of couse that only pays off if the kernel doesn't has to put much > effort into maintaining the generation id. Something simple like > increasing it each time the guest writes a register which affects > plane_info. But it seems like that simple management algorithm pretty much guarantees that the kernel will never revisit a generation and therefore caching dmabuf fds is pointless. AIUI the optimization is to allow userspace to 'at a glance' test one plane_info vs another. The user could also do this with a memcmp of the plane_info structs if that's its only purpose. A randomly incremented field within that struct could actually be a hindrance to the user for such a comparison. Are there cases where the plane_info struct is otherwise identical where the user would need to get a new dmabuf fd anyway? Thanks, Alex
Re: [kra...@redhat.com: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations]
Hi Gred and Alex: Thanks for the reply! It would be better that kernel only provides functions instead of maintaining states from my point of view. If there is any existing async notification channel in vfio device fd? like reporting device events from vfio to QEMU? If so, It would be nice if we can extend that channel and notify the userspace application that guest framebuffer changed via that channel. So userspace application even doesn't need to call the ioctl periodically. :) If VM runs a single-buffered application. Thanks, Zhi. On Fri, 2017-06-23 at 15:49 +0800, Zhi Wang wrote: Hi: Thanks for the discussions! If the userspace application has already maintained a LRU list, it looks like we don't need generation anymore, generation isn't required, things are working just fine without that. It is just a small optimization, userspace can skip the LRU lookup altogether if the generation didn't change. But of couse that only pays off if the kernel doesn't has to put much effort into maintaining the generation id. Something simple like increasing it each time the guest writes a register which affects plane_info. If you think it doesn't make sense from the driver point of view we can drop the generation. cheers, Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Fri, 2017-06-23 at 15:49 +0800, Zhi Wang wrote: > Hi: > Thanks for the discussions! If the userspace application has > already maintained a LRU list, it looks like we don't need > generation > anymore, generation isn't required, things are working just fine without that. It is just a small optimization, userspace can skip the LRU lookup altogether if the generation didn't change. But of couse that only pays off if the kernel doesn't has to put much effort into maintaining the generation id. Something simple like increasing it each time the guest writes a register which affects plane_info. If you think it doesn't make sense from the driver point of view we can drop the generation. cheers, Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi:
Thanks for the discussions! If the userspace application has
already maintained a LRU list, it looks like we don't need generation
anymore, as userspace application will lookup the guest framebuffer from
the LRU list by "offset". No matter how, it would know if this is a new
guest framebuffer or not. If it's a new guest framebuffer, a new dmabuf
fd should be generated. If it's an old framebuffer, it can just show
that framebuffer.
Thanks,
Zhi.
On 06/23/17 15:26, Gerd Hoffmann wrote:
Hi,
Is this only going to support accelerated driver output, not basic
VGA
modes for BIOS interaction?
Right now there is no vgabios or uefi support for the vgpu.
But even with that in place there still is the problem that the display
device initialization happens before the guest runs and therefore there
isn't an plane yet ...
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.
So maybe a "enum plane_state" (instead of "bool is_enabled")? So we
can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?
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,
kernel wouldn't know either, only the guest knows ...
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?
Yep, this is exactly what my qemu patches are doing, keep a LRU list.
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.
Not really. Host IGD has a certain amount of memory, some of it is
assigned to the guest, guest stores the framebuffer there, the dma-buf
is a host handle (drm object, usable for rendering ops) for the guest
framebuffer. So it doesn't use much ressources. Some memory is needed
for management structs, but not for the actual data as this in the
video memory dedicated to the guest.
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.
I'd suggest to settle for one of these two. Either one region and
multiple planes inside (using offset) or one region per plane. I'd
prefer the former. When going for the latter then yes we have to
specify the region. I'd name the field region_id then to make clear
what it is.
What would be the use case for multiple planes?
cursor support? We already have plane_type for that.
multihead support? We'll need (at minimum) a head_id field for that
(for both dma-buf and region)
pageflipping support? Nothing needed, query_plane will simply return
the currently visible plane. Region only needs to be big enough to fit
the framebuffer twice. Then the driver can flip between two buffers,
point to the one qemu should display using "offset".
Do we also want to exclude that device
needs to be strictly region or dmabuf? Maybe it does both.
Very unlikely IMHO.
Or maybe
it supports dmabuf-ng (ie. whatever comes next).
Possibly happens some day, but who knows what interfaces we'll need to
support that ...
vfio_device_query {
u32 argsz;
u32 flags;
enum query_type; /* or use flags for that */
We don't have an infinite number of ioctls
The limited ioctl number space is a good reason indeed.
Ok, lets take this route then.
cheers,
Gerd
___
intel-gvt-dev mailing list
intel-gvt-...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi,
> Is this only going to support accelerated driver output, not basic
> VGA
> modes for BIOS interaction?
Right now there is no vgabios or uefi support for the vgpu.
But even with that in place there still is the problem that the display
device initialization happens before the guest runs and therefore there
isn't an plane yet ...
> > 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.
So maybe a "enum plane_state" (instead of "bool is_enabled")? So we
can clearly disturgish ENABLED, DISABLED, NOT_SUPPORTED cases?
> > 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,
kernel wouldn't know either, only the guest knows ...
> 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?
Yep, this is exactly what my qemu patches are doing, keep a LRU list.
> > > 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.
Not really. Host IGD has a certain amount of memory, some of it is
assigned to the guest, guest stores the framebuffer there, the dma-buf
is a host handle (drm object, usable for rendering ops) for the guest
framebuffer. So it doesn't use much ressources. Some memory is needed
for management structs, but not for the actual data as this in the
video memory dedicated to the guest.
> > 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.
I'd suggest to settle for one of these two. Either one region and
multiple planes inside (using offset) or one region per plane. I'd
prefer the former. When going for the latter then yes we have to
specify the region. I'd name the field region_id then to make clear
what it is.
What would be the use case for multiple planes?
cursor support? We already have plane_type for that.
multihead support? We'll need (at minimum) a head_id field for that
(for both dma-buf and region)
pageflipping support? Nothing needed, query_plane will simply return
the currently visible plane. Region only needs to be big enough to fit
the framebuffer twice. Then the driver can flip between two buffers,
point to the one qemu should display using "offset".
> Do we also want to exclude that device
> needs to be strictly region or dmabuf? Maybe it does both.
Very unlikely IMHO.
> Or maybe
> it supports dmabuf-ng (ie. whatever comes next).
Possibly happens some day, but who knows what interfaces we'll need to
support that ...
> > vfio_device_query {
> > u32 argsz;
> > u32 flags;
> > enum query_type; /* or use flags for that */
> We don't have an infinite number of ioctls
The limited ioctl number space is a good reason indeed.
Ok, lets take this route then.
cheers,
Gerd
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 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 >
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
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Wednesday, June 21, 2017 7:04 PM
> To: Zhang, Tina ; Alex Williamson
>
> Cc: Wang, Zhenyu Z ; intel-
> g...@lists.freedesktop.org; linux-kernel@vger.kernel.org; Chen, Xiaoguang
> ; Kirti Wankhede ; Lv,
> Zhiyuan ; intel-gvt-...@lists.freedesktop.org; Wang,
> Zhi A
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Wed, 2017-06-21 at 09:20 +, Zhang, Tina wrote:
> > Thanks for all the comments. I'm planning to cook the next version of
> > this patch set
>
> How about posting only this patch instead of the whole series until we've
> settled
> the interfaces?
OK.
>
> > Could the following two works?
> > #define VFIO_DEVICE_FLAGS_DMABUF (1 << 5)/* vfio-dmabuf
> > device */
>
> VFIO_DEVICE_FLAGS_GFX_DMABUF?
>
> > 2. vfio_device_gfx_plane_info
> > struct vfio_device_gfx_plane_info {
> > __u64 start;-> offset
> > __u64 drm_format_mod;
> > __u32 drm_format;
> > __u32 width;
> > __u32 height;
> > __u32 stride;
> > __u32 size;
> > __u32 x_pos;
> > __u32 y_pos;
> > };
> > > Does it make sense to have a "generation" field in the plane_info
> > > struct (which gets increased each time the struct changes) ?
>
> > Well, Gerd, can you share more details about how to use this field in
> > user mode, so that we can figure out a way to support it? Thanks.
>
> 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.
Make sense for dma-buf. Thanks.
>
> >
> > 3. vfio_device_query_gfx_plane
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > __u32 plane_type;
> > };
> > So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR.
>
>
> > If the newly added plane_type is used for this, the id field may be
> > useless in dmabuf usage. Do you have any idea about the usage of this
> > id field in dmabuf usage?
>
> 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.
>
> cheers,
> Gerd
>
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Wed, 21 Jun 2017 13:03:31 +0200
Gerd Hoffmann wrote:
> On Wed, 2017-06-21 at 09:20 +, Zhang, Tina wrote:
> > Thanks for all the comments. I'm planning to cook the next version of
> > this patch set
>
> How about posting only this patch instead of the whole series until
> we've settled the interfaces?
>
> > Could the following two works?
> > #define VFIO_DEVICE_FLAGS_DMABUF (1 << 5)/* vfio-dmabuf
> > device */
>
> 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.
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.
FWIW, I think the RESET flag in the device_info struct was a mistake,
we could have simply let the user probe for it, perhaps with a no-op
flag in the ioctl explicitly for that purpose. So I'm not in favor of
adding device_info flags that only indicate the presence of a given
ioctl, the user can try it and find out so long as it doesn't cause a
state change or we have a probe option.
> > 2. vfio_device_gfx_plane_info
> > struct vfio_device_gfx_plane_info {
> > __u64 start;-> offset
> > __u64 drm_format_mod;
> > __u32 drm_format;
> > __u32 width;
> > __u32 height;
> > __u32 stride;
> > __u32 size;
> > __u32 x_pos;
> > __u32 y_pos;
> > };
> > > Does it make sense to have a "generation" field in the plane_info
> > > struct (which gets increased each time the struct changes) ?
>
> > Well, Gerd, can you share more details about how to use this field
> > in user mode, so that we can figure out a way to support it? Thanks.
>
> 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. 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? What happens to
existing dmabuf fds when the generation updates, do they stop
refreshing? Does it blank the framebuffer? Can the dmabuf fd
transparently update to the new plane_info?
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? What is the responsibility
of the user and how are they notified to refresh the plane_info? It
seems there's no way for the user to avoid occasionally being out of
sync with the current plane_info for a region.
> >
> > 3. vfio_device_query_gfx_plane
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > __u32 plane_type;
> > };
> > So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR.
>
>
> > If the newly added plane_type is used for this, the id field may be
> > useless in dmabuf usage. Do you have any idea about the usage of this
> > id field in dmabuf usage?
>
> 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. If it's a dmabuf
fd, how might they match this to one they already know about (assuming
a generation ID is comp
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Wed, 2017-06-21 at 09:20 +, Zhang, Tina wrote:
> Thanks for all the comments. I'm planning to cook the next version of
> this patch set
How about posting only this patch instead of the whole series until
we've settled the interfaces?
> Could the following two works?
> #define VFIO_DEVICE_FLAGS_DMABUF (1 << 5)/* vfio-dmabuf
> device */
VFIO_DEVICE_FLAGS_GFX_DMABUF?
> 2. vfio_device_gfx_plane_info
> struct vfio_device_gfx_plane_info {
> __u64 start;-> offset
> __u64 drm_format_mod;
> __u32 drm_format;
> __u32 width;
> __u32 height;
> __u32 stride;
> __u32 size;
> __u32 x_pos;
> __u32 y_pos;
> };
> > Does it make sense to have a "generation" field in the plane_info
> > struct (which gets increased each time the struct changes) ?
> Well, Gerd, can you share more details about how to use this field
> in user mode, so that we can figure out a way to support it? Thanks.
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.
>
> 3. vfio_device_query_gfx_plane
> struct vfio_device_query_gfx_plane {
> __u32 argsz;
> __u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> struct vfio_device_gfx_plane_info plane_info;
> __u32 id;
> __u32 plane_type;
> };
> So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR.
> If the newly added plane_type is used for this, the id field may be
> useless in dmabuf usage. Do you have any idea about the usage of this
> id field in dmabuf usage?
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.
cheers,
Gerd
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Thanks for all the comments. I'm planning to cook the next version of this
patch set which I'd like to include all the comments and ideas. Here is the
summary:
1. How to figure the device capabilities
struct vfio_device_info {
__u32 argsz;
__u32 flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
__u32 num_regions;/* Max region index + 1 */
__u32 num_irqs; /* Max IRQ index + 1 */
};
> We could use two flag bits to indicate dmabuf or graphics region support.
Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF (1 << 5)/* vfio-dmabuf device */
#define VFIO_DEVICE_FLAGS_GFX_REGION (1 << 6)/* vfio-gfx-region
device */
2. vfio_device_gfx_plane_info
struct vfio_device_gfx_plane_info {
__u64 start;-> offset
__u64 drm_format_mod;
__u32 drm_format;
__u32 width;
__u32 height;
__u32 stride;
__u32 size;
__u32 x_pos;
__u32 y_pos;
};
> Does it make sense to have a "generation" field in the plane_info struct
> (which gets increased each time the struct changes) ?
Well, Gerd, can you share more details about how to use this field in user
mode, so that we can figure out a way to support it? Thanks.
3. vfio_device_query_gfx_plane
struct vfio_device_query_gfx_plane {
__u32 argsz;
__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
struct vfio_device_gfx_plane_info plane_info;
__u32 id;
__u32 plane_type;
};
So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR. If
the newly added plane_type is used for this, the id field may be useless in
dmabuf usage. Do you have any idea about the usage of this id field in dmabuf
usage?
4. Two ioctl commands
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD
5. > Then we should kill off the manager fd unless there are arguments that
still give it value.
Agree.
If there is anything I miss, please tell us.
Thanks.
Tina
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, June 21, 2017 7:22 AM
> To: Zhang, Tina
> Cc: Gerd Hoffmann ; intel-...@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; Kirti Wankhede ;
> Chen, Xiaoguang ; intel-gvt-
> d...@lists.freedesktop.org; Lv, Zhiyuan ; Wang, Zhi A
> ; Wang, Zhenyu Z
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Tue, 20 Jun 2017 23:01:53 +
> "Zhang, Tina" wrote:
>
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Tuesday, June 20, 2017 11:00 PM
> > > To: Gerd Hoffmann
> > > Cc: Zhang, Tina ;
> > > intel-...@lists.freedesktop.org; linux- ker...@vger.kernel.org;
> > > Kirti Wankhede ; Chen, Xiaoguang
> > > ; intel-gvt-...@lists.freedesktop.org;
> > > Lv, Zhiyuan ; Wang, Zhi A
> > > ; Wang, Zhenyu Z
> > > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based
> > > dma-buf operations
> > >
> > > On Tue, 20 Jun 2017 12:57:36 +0200
> > > Gerd Hoffmann wrote:
> > >
> > > > On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > > > > Hi,
> > > > >
> > > > > Thanks for all the comments. Here are the summaries:
> > > > >
> > > > > 1. Modify the structures to make it more general.
> > > > > struct vfio_device_gfx_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 generation;
> > > > > };
> > > >
> > > > Looks good to me.
> > > >
> > > > > struct vfio_device_query_gfx_plane {
> > > > > __u32 argsz;
> > > > > __u32 flags;
> > > > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID(1 << 0)
> > > > > #define VFIO_GFX_PLANE_F
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi,
> We already have VFIO_DEVICE_GET_INFO which returns:
>
> struct vfio_device_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports
> reset */
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform
> device */
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device
> */
> #define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
> __u32 num_regions;/* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
>
> We could use two flag bits to indicate dmabuf or graphics region
> support.
That works too.
> > Then this to query the plane:
> >
> > struct vfio_device_gfx_query_plane {
> > __u32 argsz;
> > __u32 flags;
> > struct vfio_device_gfx_plane_info plane_info; /* out */
> > __u32 plane_type; /* in */
> > };
>
> I'm not sure why we're using an enum for something that can currently
> be defined with 2 bits,
We can reuse the DRM_PLANE_TYPE_* then.
> seems like this would be another good use of
> flags. We could even embed an enum into the flags if we want to
> leave some expansion room, 4 bits maybe? Also, I was imagining that
> a
> device could support multiple graphics regions, that's where
> specifying
> the "id" as a region index seemed useful.
Hmm, yes, possibly for multihead configurations. But I guess for
proper multihead support we would need more than just an region id.
Or do you have something else in mind?
> > 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.
>
> I'm not sure I understand how this works for a region, the region is
> always the current generation, how can the user ever be sure the
> plane_info matches what is exposed in the region?
generation will change each time the plane configuration (not content)
changes. Typically that will be on video mode switches. In the dmabuf
case also on pageflips.
cheers,
Gerd
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Tue, 20 Jun 2017 23:01:53 +
"Zhang, Tina" wrote:
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Tuesday, June 20, 2017 11:00 PM
> > To: Gerd Hoffmann
> > Cc: Zhang, Tina ; intel-...@lists.freedesktop.org;
> > linux-
> > ker...@vger.kernel.org; Kirti Wankhede ; Chen,
> > Xiaoguang ; intel-gvt-...@lists.freedesktop.org;
> > Lv, Zhiyuan ; Wang, Zhi A ;
> > Wang, Zhenyu Z
> > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> > operations
> >
> > On Tue, 20 Jun 2017 12:57:36 +0200
> > Gerd Hoffmann wrote:
> >
> > > On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > > > Hi,
> > > >
> > > > Thanks for all the comments. Here are the summaries:
> > > >
> > > > 1. Modify the structures to make it more general.
> > > > struct vfio_device_gfx_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 generation;
> > > > };
> > >
> > > Looks good to me.
> > >
> > > > struct vfio_device_query_gfx_plane {
> > > > __u32 argsz;
> > > > __u32 flags;
> > > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
> > > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > > > struct vfio_device_gfx_plane_info plane_info;
> > > > __u32 id;
> > > > };
> > >
> > > I'm not convinced the flags are a great idea. Whenever dmabufs or a
> > > region is used is a static property of the device, not of each
> > > individual plane.
> > >
> > >
> > > I think we should have this for userspace to figure:
> > >
> > > enum vfio_device_gfx_type {
> > > VFIO_DEVICE_GFX_NONE,
> > > VFIO_DEVICE_GFX_DMABUF,
> > > VFIO_DEVICE_GFX_REGION,
> > > };
> > >
> > > struct vfio_device_gfx_query_caps {
> > > __u32 argsz;
> > > __u32 flags;
> > > enum vfio_device_gfx_type;
> > > };
> >
> > We already have VFIO_DEVICE_GET_INFO which returns:
> >
> > struct vfio_device_info {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
> > #define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
> > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device */
> > #define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
> > __u32 num_regions;/* Max region index + 1 */
> > __u32 num_irqs; /* Max IRQ index + 1 */
> > };
> >
> > We could use two flag bits to indicate dmabuf or graphics region support.
> > vfio_device_gfx_query_caps seems to imply a new ioctl, which would be
> > unnecessary.
> >
> > > Then this to query the plane:
> > >
> > > struct vfio_device_gfx_query_plane {
> > > __u32 argsz;
> > > __u32 flags;
> > > struct vfio_device_gfx_plane_info plane_info; /* out */
> > > __u32 plane_type; /* in */
> > > };
> >
> > I'm not sure why we're using an enum for something that can currently be
> > defined with 2 bits, seems like this would be another good use of flags. We
> > could even embed an enum into the flags if we want to leave some expansion
> > room, 4 bits maybe? Also, I was imagining that a device could support
> > multiple
> > graphics regions, that's where specifying the "id" as a region index seemed
> > useful. We lose that ability here unless we go back to defining a flag bit
> > to
> > specify how to interpret this last field.
> >
> > > 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> > > device fd.
> > > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > > vfio_device_gfx_plane_info.
> > >
> > &
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, June 20, 2017 11:00 PM
> To: Gerd Hoffmann
> Cc: Zhang, Tina ; intel-...@lists.freedesktop.org;
> linux-
> ker...@vger.kernel.org; Kirti Wankhede ; Chen,
> Xiaoguang ; intel-gvt-...@lists.freedesktop.org;
> Lv, Zhiyuan ; Wang, Zhi A ;
> Wang, Zhenyu Z
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Tue, 20 Jun 2017 12:57:36 +0200
> Gerd Hoffmann wrote:
>
> > On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > > Hi,
> > >
> > > Thanks for all the comments. Here are the summaries:
> > >
> > > 1. Modify the structures to make it more general.
> > > struct vfio_device_gfx_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 generation;
> > > };
> >
> > Looks good to me.
> >
> > > struct vfio_device_query_gfx_plane {
> > > __u32 argsz;
> > > __u32 flags;
> > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID(1 << 0)
> > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > > struct vfio_device_gfx_plane_info plane_info;
> > > __u32 id;
> > > };
> >
> > I'm not convinced the flags are a great idea. Whenever dmabufs or a
> > region is used is a static property of the device, not of each
> > individual plane.
> >
> >
> > I think we should have this for userspace to figure:
> >
> > enum vfio_device_gfx_type {
> > VFIO_DEVICE_GFX_NONE,
> > VFIO_DEVICE_GFX_DMABUF,
> > VFIO_DEVICE_GFX_REGION,
> > };
> >
> > struct vfio_device_gfx_query_caps {
> > __u32 argsz;
> > __u32 flags;
> > enum vfio_device_gfx_type;
> > };
>
> We already have VFIO_DEVICE_GET_INFO which returns:
>
> struct vfio_device_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
> __u32 num_regions;/* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
>
> We could use two flag bits to indicate dmabuf or graphics region support.
> vfio_device_gfx_query_caps seems to imply a new ioctl, which would be
> unnecessary.
>
> > Then this to query the plane:
> >
> > struct vfio_device_gfx_query_plane {
> > __u32 argsz;
> > __u32 flags;
> > struct vfio_device_gfx_plane_info plane_info; /* out */
> > __u32 plane_type; /* in */
> > };
>
> I'm not sure why we're using an enum for something that can currently be
> defined with 2 bits, seems like this would be another good use of flags. We
> could even embed an enum into the flags if we want to leave some expansion
> room, 4 bits maybe? Also, I was imagining that a device could support
> multiple
> graphics regions, that's where specifying the "id" as a region index seemed
> useful. We lose that ability here unless we go back to defining a flag bit to
> specify how to interpret this last field.
>
> > 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> > device fd.
> > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > vfio_device_gfx_plane_info.
> >
> > Yes.
> >
> > > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
>
> I'm not convinced this adds value, but I'll list it as an option:
>
> VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
>
> The benefit is that it might help to avoid a proliferation of ioctls on the
> device the
> pain is that we need to either define a field or section of flags which
> identify
> what is being queried or what type of device fd is being requested.
I didn't understand here. The patch introduces three ioctl commands:
VFIO_DEVICE_GET_FD, VFIO_DMABUF_MGR_QUERY_PLANE, VFIO_DMABUF_MGR_CREATE_DMABUF.
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On 6/20/2017 8:30 PM, Alex Williamson wrote:
> On Tue, 20 Jun 2017 12:57:36 +0200
> Gerd Hoffmann wrote:
>
>> On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
>>> Hi,
>>>
>>> Thanks for all the comments. Here are the summaries:
>>>
>>> 1. Modify the structures to make it more general.
>>> struct vfio_device_gfx_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 generation;
>>> };
>>
>> Looks good to me.
>>
>>> struct vfio_device_query_gfx_plane {
>>> __u32 argsz;
>>> __u32 flags;
>>> #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
>>> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
>>> struct vfio_device_gfx_plane_info plane_info;
>>> __u32 id;
>>> };
>>
>> I'm not convinced the flags are a great idea. Whenever dmabufs or a
>> region is used is a static property of the device, not of each
>> individual plane.
>>
>>
>> I think we should have this for userspace to figure:
>>
>> enum vfio_device_gfx_type {
>> VFIO_DEVICE_GFX_NONE,
>> VFIO_DEVICE_GFX_DMABUF,
>> VFIO_DEVICE_GFX_REGION,
>> };
>>
>> struct vfio_device_gfx_query_caps {
>> __u32 argsz;
>> __u32 flags;
>> enum vfio_device_gfx_type;
>> };
>
> We already have VFIO_DEVICE_GET_INFO which returns:
>
> struct vfio_device_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
> __u32 num_regions;/* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
>
> We could use two flag bits to indicate dmabuf or graphics region
> support. vfio_device_gfx_query_caps seems to imply a new ioctl, which
> would be unnecessary.
>
Sounds good to me.
>> Then this to query the plane:
>>
>> struct vfio_device_gfx_query_plane {
>> __u32 argsz;
>> __u32 flags;
>> struct vfio_device_gfx_plane_info plane_info; /* out */
>> __u32 plane_type; /* in */
>> };
>
> I'm not sure why we're using an enum for something that can currently
> be defined with 2 bits, seems like this would be another good use of
> flags. We could even embed an enum into the flags if we want to
> leave some expansion room, 4 bits maybe? Also, I was imagining that a
> device could support multiple graphics regions, that's where specifying
> the "id" as a region index seemed useful. We lose that ability here
> unless we go back to defining a flag bit to specify how to interpret
> this last field.
>
Right, as I mentioned in earlier reply, we need 2 seperate fields
- plane type : DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR
- id : fd for dmabuf or region index for region type
>> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
>> device fd.
>>> VFIO_DEVICE_QUERY_GFX_PLANE : used to query
>>> vfio_device_gfx_plane_info.
>>
>> Yes.
>>
>>> VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
>
> I'm not convinced this adds value, but I'll list it as an option:
>
> VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
>
> The benefit is that it might help to avoid a proliferation of ioctls on
> the device the pain is that we need to either define a field or section
> of flags which identify what is being queried or what type of device fd
> is being requested.
>
>> Yes. The plane might have changed between query-plane and get-dmabuf
>> ioctl calls though, we must make sure we handle that somehow. Current
>> patches return plane_info on get-dmabuf ioctl too, so userspace can see
>> what it actually got.
>>
>> 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.
>
> I'm not sure I understand how this works for a region, the region is
> always the current generation, how can the user ever be sure the
> plane_info matches what is exposed in the region? Thanks,
>
Userspace have to follow the sequence to query plane info
(VFIO_DEVICE_QUERY_GFX_PLANE) and then read primary surface from the region.
On kernel space side, from VFIO_DEVICE_QUERY_GFX_PLANE ioctl, driver
should update surface which is being exposed by the GFX region, fill
vfio_device_gfx_pl
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Tue, 20 Jun 2017 12:57:36 +0200
Gerd Hoffmann wrote:
> On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > Hi,
> >
> > Thanks for all the comments. Here are the summaries:
> >
> > 1. Modify the structures to make it more general.
> > struct vfio_device_gfx_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 generation;
> > };
>
> Looks good to me.
>
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > };
>
> I'm not convinced the flags are a great idea. Whenever dmabufs or a
> region is used is a static property of the device, not of each
> individual plane.
>
>
> I think we should have this for userspace to figure:
>
> enum vfio_device_gfx_type {
> VFIO_DEVICE_GFX_NONE,
> VFIO_DEVICE_GFX_DMABUF,
> VFIO_DEVICE_GFX_REGION,
> };
>
> struct vfio_device_gfx_query_caps {
> __u32 argsz;
> __u32 flags;
> enum vfio_device_gfx_type;
> };
We already have VFIO_DEVICE_GET_INFO which returns:
struct vfio_device_info {
__u32 argsz;
__u32 flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI (1 << 1)/* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA (1 << 3)/* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW (1 << 4)/* vfio-ccw device */
__u32 num_regions;/* Max region index + 1 */
__u32 num_irqs; /* Max IRQ index + 1 */
};
We could use two flag bits to indicate dmabuf or graphics region
support. vfio_device_gfx_query_caps seems to imply a new ioctl, which
would be unnecessary.
> Then this to query the plane:
>
> struct vfio_device_gfx_query_plane {
> __u32 argsz;
> __u32 flags;
> struct vfio_device_gfx_plane_info plane_info; /* out */
> __u32 plane_type; /* in */
> };
I'm not sure why we're using an enum for something that can currently
be defined with 2 bits, seems like this would be another good use of
flags. We could even embed an enum into the flags if we want to
leave some expansion room, 4 bits maybe? Also, I was imagining that a
device could support multiple graphics regions, that's where specifying
the "id" as a region index seemed useful. We lose that ability here
unless we go back to defining a flag bit to specify how to interpret
this last field.
> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> device fd.
> > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > vfio_device_gfx_plane_info.
>
> Yes.
>
> > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
I'm not convinced this adds value, but I'll list it as an option:
VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
The benefit is that it might help to avoid a proliferation of ioctls on
the device the pain is that we need to either define a field or section
of flags which identify what is being queried or what type of device fd
is being requested.
> Yes. The plane might have changed between query-plane and get-dmabuf
> ioctl calls though, we must make sure we handle that somehow. Current
> patches return plane_info on get-dmabuf ioctl too, so userspace can see
> what it actually got.
>
> 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.
I'm not sure I understand how this works for a region, the region is
always the current generation, how can the user ever be sure the
plane_info matches what is exposed in the region? Thanks,
Alex
Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> Hi,
>
> Thanks for all the comments. Here are the summaries:
>
> 1. Modify the structures to make it more general.
> struct vfio_device_gfx_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 generation;
> };
Looks good to me.
> struct vfio_device_query_gfx_plane {
> __u32 argsz;
> __u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> struct vfio_device_gfx_plane_info plane_info;
> __u32 id;
> };
I'm not convinced the flags are a great idea. Whenever dmabufs or a
region is used is a static property of the device, not of each
individual plane.
I think we should have this for userspace to figure:
enum vfio_device_gfx_type {
VFIO_DEVICE_GFX_NONE,
VFIO_DEVICE_GFX_DMABUF,
VFIO_DEVICE_GFX_REGION,
};
struct vfio_device_gfx_query_caps {
__u32 argsz;
__u32 flags;
enum vfio_device_gfx_type;
};
Then this to query the plane:
struct vfio_device_gfx_query_plane {
__u32 argsz;
__u32 flags;
struct vfio_device_gfx_plane_info plane_info; /* out */
__u32 plane_type; /* in */
};
2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
device fd.
> VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> vfio_device_gfx_plane_info.
Yes.
> VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
Yes. The plane might have changed between query-plane and get-dmabuf
ioctl calls though, we must make sure we handle that somehow. Current
patches return plane_info on get-dmabuf ioctl too, so userspace can see
what it actually got.
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.
cheers,
Gerd
RE: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Hi,
Thanks for all the comments. Here are the summaries:
1. Modify the structures to make it more general.
struct vfio_device_gfx_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 generation;
};
struct vfio_device_query_gfx_plane {
__u32 argsz;
__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID (1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
struct vfio_device_gfx_plane_info plane_info;
__u32 id;
};
2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.
VFIO_DEVICE_QUERY_GFX_PLANE : used to query vfio_device_gfx_plane_info.
VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
Am I correct? Thanks.
Tina
> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Monday, June 19, 2017 10:56 PM
> To: Gerd Hoffmann
> Cc: intel-...@lists.freedesktop.org; linux-kernel@vger.kernel.org; Kirti
> Wankhede ; Chen, Xiaoguang
> ; intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan
>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
>
> On Mon, 19 Jun 2017 08:38:32 +0200
> Gerd Hoffmann wrote:
>
> > Hi,
> >
> > > 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.
> >
> > Region type should be DRM_PLANE_TYPE_PRIMARY
> >
> > > 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.
> >
> > Does it make sense to have a "generation" field in the plane_info
> > struct (which gets increased each time the struct changes) ?
>
> It seems less cumbersome than checking each field to see if it has changed.
> Thanks,
>
> Alex
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
