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

[Kirti Wankhede <kwankhede@nvidia.com>]

On 8/7/2017 11:13 PM, Alex Williamson wrote:
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <tina.zhang@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);

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)


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?


> It seems like this handles all the cases, the user can ask what's
> supported and specifies the interface they want on every call.  The
> user therefore can also choose between region_index and fd and we can
> make that a union.
>
>> 2. For dma-buf, how to differentiate unsupported vs not initialized?
>> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
>> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.
> 
> So we're relying on special values again :-\  For which fields is zero
> not a valid value?  I prefer the probe interface above unless there are
> better ideas.
> 

PROBE will be called during vdev initialization and after that
vfio_console_update_display() gets called at every console refresh
cycle. But until driver in guest is loaded, mmaped buffer/ DMABUF will
not be populated with correct surface data. In that case for QUERY,
vendor driver should return (atleast) size=0 which means there is
nothing to copy. Once guest driver is loaded and surface is created by
guest driver, QUERY interface should return valid size.

Thanks,
Kirti

>> 3. The id field in structure vfio_device_gfx_plane_info
>> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
>> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
>> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
>> 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;
>>  };
>> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.
> 
> The usefulness of this ioctl seems really limited and once again we get
> into a situation where having two ioctls leaves doubt whether this is
> describing the current plane state.  Thanks,
> 
> Alex
> 
>>>>>>>>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>>>>>>>>>  1 file changed, 28 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/uapi/linux/vfio.h
>>>>>>>>> b/include/uapi/linux/vfio.h index ae46105..827a230 100644
>>>>>>>>> --- a/include/uapi/linux/vfio.h
>>>>>>>>> +++ b/include/uapi/linux/vfio.h
>>>>>>>>> @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
>>>>>>>>>
>>>>>>>>>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,  
>>> VFIO_BASE +  
>>>>>>>> 13)  
>>>>>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,  
>>> VFIO_BASE  
>>>> +  
>>>>>> 14,  
>>>>>>>>> +struct vfio_device_query_gfx_plane)
>>>>>>>>> + *
>>>>>>>>> + * Set the drm_plane_type and retrieve information about
>>>>>>>>> +the gfx  
>>>> plane.  
>>>>>>>>> + *
>>>>>>>>> + * Return: 0 on success, -errno on failure.
>>>>>>>>> + */
>>>>>>>>> +struct vfio_device_gfx_plane_info {
>>>>>>>>> +	__u32 argsz;
>>>>>>>>> +	__u32 flags;
>>>>>>>>> +	/* in */
>>>>>>>>> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*  
>>>> */  
>>>>>>>>> +	/* out */
>>>>>>>>> +	__u32 drm_format;	/* drm format of plane */
>>>>>>>>> +	__u64 drm_format_mod;   /* tiled mode */
>>>>>>>>> +	__u32 width;	/* width of plane */
>>>>>>>>> +	__u32 height;	/* height of plane */
>>>>>>>>> +	__u32 stride;	/* stride of plane */
>>>>>>>>> +	__u32 size;	/* size of plane in bytes, align on page*/
>>>>>>>>> +	__u32 x_pos;	/* horizontal position of cursor plane, upper  
>>>> left corner  
>>>>>>>> in pixels */  
>>>>>>>>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
>>>> corner in  
>>>>>>>> lines*/  
>>>>>>>>> +	__u32 region_index;
>>>>>>>>> +	__s32 fd;	/* dma-buf fd */  
>>>>>>>>
>>>>>>>> How do I know which of these is valid, region_index or fd?
>>>>>>>> Can I ask for one vs the other?  What are the errno values to
>>>>>>>> differentiate unsupported vs not initialized?  Is there a "probe"
>>>>>>>> flag that I can use to determine what the device supports
>>>>>>>> without allocating  
>>>>>> those resources yet?  
>>>>>>> Dma-buf won't use region_index, which means region_index is zero
>>>>>>> all the  
>>>>>> time for dma-buf usage.  
>>>>>>> As we discussed, there won't be errno if not initialized, just
>>>>>>> keep all fields  
>>>> zero.  
>>>>>>> I will add the comments about these in the next version. Thanks  
>>>>>>
>>>>>> Zero is a valid region index.  
>>>>> If zero is valid, can we find out other invalid value? How about 0xffffffff?  
>>>>
>>>> Unlikely, but also valid.  Isn't this why we have a flags field in the
>>>> structure, so we don't need to rely on implicit values as invalid.
>>>> Also, all of the previously discussed usage models needs to be
>>>> documented, either here in the header or in a Documentation/ file.  
>>> According to the previously discussion, we also have the following propose:
>>> 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;
>>> };
>>> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
>>> User mode can query this before querying the plan info, and to see which usage
>>> (dma-buf or region) is supported.
>>> Does it still make sense?
>>> Thanks.  
>> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
>> Thanks
>>
>> Tina
>>>
>>> Tina
>>>
>>>   
>>>> Thanks,
>>>>
>>>> Alex
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx