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

[Gerd Hoffmann]

  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