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


> 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,

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,

2. On above console registration, vfio_console_update_display() gets
called for each refresh cycle of console. In that:
- 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.


>> 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 {
>>  };
>>  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)  
>>>>>>>>> +/**
>>>> +  
>>>>>> 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 {
>>> };
>>> 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  
dri-devel mailing list

Reply via email to