Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Kirti Wankhede]

On 7/12/2017 1:10 PM, Daniel Vetter wrote:
> On Wed, Jul 12, 2017 at 02:31:40AM +0000, Zhang, Tina wrote:
>>
>>
>>> -----Original Message-----
>>> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On
>>> Behalf Of Daniel Vetter
>>> Sent: Tuesday, July 11, 2017 5:13 PM
>>> To: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>> Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; intel-
>>> gfx@xxxxxxxxxxxxxxxxxxxxx; alex.williamson@xxxxxxxxxx;
>>> zhenyuw@xxxxxxxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; Kirti Wankhede
>>> <kwankhede@xxxxxxxxxx>; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>;
>>> daniel@xxxxxxxx; Zhang, Tina <tina.zhang@xxxxxxxxx>; intel-gvt-
>>> dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>
>>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
>>>
>>> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
>>>>   Hi,
>>>>
>>>>>> +struct vfio_device_query_gfx_plane {
>>>>>> +	__u32 argsz;
>>>>>> +	__u32 flags;
>>>>>> +	struct vfio_device_gfx_plane_info plane_info;
>>>>>> +	__u32 plane_type;
>>>>>> +	__s32 fd; /* dma-buf fd */
>>>>>> +	__u32 plane_id;
>>>>>> +};
>>>>>> +
>>>>>
>>>>> It would be better to have comment here about what are expected
>>>>> values for plane_type and plane_id.
>>>>
>>>> plane_type is DRM_PLANE_TYPE_*.
>>>>
>>>> yes, a comment saying so would be good, same for drm_format which is
>>>> DRM_FORMAT_*.  While looking at these two: renaming plane_type to
>>>> drm_plane_type (for consistency) is probably a good idea too.
>>>>
>>>> plane_id needs a specification.
>>>
>>> Why do you need plane_type? With universal planes the plane_id along is
>>> sufficient to identify a plane on a given drm device instance. I'd just remove it.
>>> -Daniel
>> The plane_type here, is to ask the mdev vendor driver to return the information according to the value in field plane_type. So, it's a input field.
>> The values in plane_type field is the same of drm_plane_type. And yes, it's better to use drm_plane_type instead of plane_id. 
> 
> I have no idea what you mean here, I guess that just shows that discussing
> an ioctl struct without solid definitions of what field does what and why
> is not all that useful. What exactly it plane_id for then?
> 

plane type could be DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR.

In case when VFIO region is used to provide surface to QEMU, plane_id
would be region index, for example region 10 could be used for primary
surface and region 11 could be used for cursor surface. So in that case,
mdev vendor driver should return plane_type and its corresponding plane_id.

Thanks,
Kirti

> This just confused me more ...
> -Daniel
>