On Tue, 20 Jun 2017 12:57:36 +0200 Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote: > On Tue, 2017-06-20 at 08:41 +0000, 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx