[PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Kirti Wankhede]

On 8/8/2017 11:37 PM, Alex Williamson wrote:
> On Tue, 8 Aug 2017 14:18:07 +0530
> Kirti Wankhede <kwankhede at nvidia.com> wrote:
> 
>> On 8/7/2017 11:13 PM, Alex Williamson wrote:
>>> On Mon, 7 Aug 2017 08:11:43 +0000
>>> "Zhang, Tina" <tina.zhang at intel.com> wrote:
>>>   
>>>> After going through the previous discussions, here are some summaries may be related to the current discussion:
>>>> 1. How does user mode figure the device capabilities between region and dma-buf?
>>>> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
>>>> Otherwise, the mdev supports dma-buf.  
>>>
>>> Why do we need to make this assumption?  
>>
>> Right, we should not make such assumption. Vendor driver might not
>> support both or disable console vnc ( for example, for performance
>> testing console VNC need to be disabled)
>>
>>>  What happens when dma-buf is
>>> superseded?  What happens if a device supports both dma-buf and
>>> regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
>>> it make sense to use it to identify which field, between region_index
>>> and fd, is valid?  We could even put region_index and fd into a union
>>> with the flag bits indicating how to interpret the union, but I'm not
>>> sure everyone was onboard with this idea.  Seems like a waste of 4
>>> bytes not to do that though.
>>>   
>>
>> Agree.
>>
>>> Thinking further, is the user ever in a situation where they query the
>>> graphics plane info and can handle either a dma-buf or a region?  It
>>> seems more likely that the user needs to know early on which is
>>> supported and would then require that they continue to see compatible
>>> plane information...  Should the user then be able to specify whether
>>> they want a dma-buf or a region?  Perhaps these flag bits are actually
>>> input and the return should be -errno if the driver cannot produce
>>> something compatible.
>>>
>>> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
>>> this initial implementation, DMABUF or REGION would always be set by
>>> the user to request that type of interface.  Additionally, the QUERY
>>> bit could be set to probe compatibility, thus if PROBE and REGION are
>>> set, the vendor driver would return success only if it supports the
>>> region based interface.  If PROBE and DMABUF are set, the vendor driver
>>> returns success only if the dma-buf based interface is supported.  The
>>> value of the remainder of the structure is undefined for PROBE.
>>> Additionally setting both DMABUF and REGION is invalid.  Undefined
>>> flags bits must be validated as zero by the drivers for future use
>>> (thus if we later define DMABUFv2, an older driver should
>>> automatically return -errno when probed or requested).
>>>   
>>
>> Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?
>>
>> Let me summarize what we need, taking QEMU as reference:
>> 1. From vfio_initfn(), for REGION case, get region info:
>> vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
>> &vdev->console_opregion)
>>
>> If above return success, setup console REGION and mmap.
>> I don't know what is required for DMABUF at this moment.
>>
>> If console VNC is supported either DMABUF or REGION, initialize console
>> and register its callback operations:
>>
>> static const GraphicHwOps vfio_console_ops = {
>>     .gfx_update  = vfio_console_update_display,
>> };
>>
>> vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
>> vdev);
> 
> I might structure it that vfio_initfn() would call
> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with the PROBE bit set with either
> DMABUF or REGION set as the interface type in the flags field.  If
> REGION is the probed interface and success is returned, then QEMU might
> go look for regions of the appropriate type, however the
> vfio_device_gfx_plane_info structure is canonical source for the region
> index, so QEMU would probably be wise to use that and only use the
> region type for consistency testing.
> 
>> 2. On above console registration, vfio_console_update_display() gets
>> called for each refresh cycle of console. In that:
>> - call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
>> - if (queried size > 0), update QEMU console surface (for REGION case
>> read mmaped region, for DMABUF read surface using fd)
> 
> The ioctl would be called with REGION or DMABUF based on the initial
> probe call, ex. we probed that REGION is supported and now we continue
> to ask for region based updates.  QEMU would need to verify the region
> index matches that already mapped, remapping a different region if
> necessary, and interpret the graphics parameters to provide the screen
> update.
>  
>> Alex, in your proposal above, my understanding is
>> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
>> step #1.
>> In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
>> flag, but either DMABUF or REGION flag based on what is returned as
>> supported by vendor driver in step #1. Is that correct?
> 
> Yes, that's the idea.  Does it make sense/provide value?
> 

Yes, sounds good to me.

Thanks,
Kirti