Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Alex Williamson]

On Wed, 21 Jun 2017 13:03:31 +0200
Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:

> On Wed, 2017-06-21 at 09:20 +0000, 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 compatible with such a notion)?  If it's a region,
which region?

Cut off here, but from Tina's previous reply:

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

Alex