On 8/7/2017 11:13 PM, Alex Williamson wrote:
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <tina.zhang@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
