Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Kirti Wankhede <kwankhede@nvidia.com>]

On 6/16/2017 10:09 PM, Alex Williamson wrote:
> On Fri, 16 Jun 2017 19:02:30 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/16/2017 2:08 AM, Alex Williamson wrote:
>>> On Thu, 15 Jun 2017 18:00:38 +0200
>>> Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>   
>>>>   Hi,
>>>>  
>>>>>> +struct vfio_dmabuf_mgr_plane_info {
>>>>>> +	__u64 start;
>>>>>> +	__u64 drm_format_mod;
>>>>>> +	__u32 drm_format;
>>>>>> +	__u32 width;
>>>>>> +	__u32 height;
>>>>>> +	__u32 stride;
>>>>>> +	__u32 size;
>>>>>> +	__u32 x_pos;
>>>>>> +	__u32 y_pos;
>>>>>> +	__u32 padding;
>>>>>> +};
>>>>>> +    
>>>>>
>>>>> This structure is generic, can remove dmabuf from its name,
>>>>> vfio_plane_info or vfio_vgpu_surface_info since this will only be
>>>>> used
>>>>> by vgpu.    
>>>>
>>>> Agree.  
>>>
>>> I'm not sure I agree regarding the vgpu statement, maybe this is not
>>> dmabuf specific, but what makes it vgpu specific?  We need to separate
>>> our current usage plans from what it's actually describing and I don't
>>> see that it describes anything vgpu specific.
>>>    
>>>>>> +struct vfio_dmabuf_mgr_query_plane {
>>>>>> +	__u32 argsz;
>>>>>> +	__u32 flags;
>>>>>> +	struct vfio_dmabuf_mgr_plane_info plane_info;
>>>>>> +	__u32 plane_id;
>>>>>> +};
>>>>>> +
>>>>>> +#define VFIO_DMABUF_MGR_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15)
>>>>>> +    
>>>>>
>>>>> This same interface can be used to query surface/plane information
>>>>> for
>>>>> both, dmabuf and region, case. Here also 'DMABUF' can be removed and
>>>>> define flags if you want to differentiate query for 'dmabuf' and
>>>>> 'region'.    
>>>>
>>>> Hmm, any specific reason why you want use a ioctl for that?  I would
>>>> simply place a "struct vfio_dmabuf_mgr_plane_info" (or whatever the
>>>> final name will be) at the start of the region.  
>>>
>>> Right, these are ioctls on the dmabuf mgr fd, not the vfio device fd,
>>> if you're exposing a region with the info I wouldn't think you'd want
>>> the hassle of managing this separate fd when you could do something
>>> like Gerd suggests with defining the first page of the regions as
>>> containing the structure.  
>>
>> My suggestion was to use vfio device fd for this ioctl and have dmabuf
>> mgr fd as member in above query_plane structure, for region type it
>> would be set to 0.
>> Yes there is other way to query surface information as Gerd suggested,
>> but my point is: if a ioctl is being add, it could be used for both
>> types, dmabuf and region.
> 
> I think this suggests abandoning the dmabuf manager fd entirely.  That's
> not necessarily a bad thing, but I don't think the idea of the dmabuf
> manager fd stands if we push one of its primary reasons for existing
> back to the device fd.  Reading though previous posts, I think we
> embraced the dmabuf manager as a separate fd primarily for
> consolidation and the potential to use it as a notification point, the
> latter being only theoretically useful.
> 
> So perhaps this becomes:
> 
> struct vfio_device_gfx_plane_info {
> 	__u64 start;
> 	__u64 drm_format_mod;
> 	__u32 drm_format;
> 	__u32 width;
> 	__u32 height;
> 	__u32 stride;
> 	__u32 size;
> 	__u32 x_pos;
> 	__u32 y_pos;
> };
> 
> struct vfio_device_query_gfx_plane {
> 	__u32 argsz;
> 	__u32 flags;
> #define VFIO_GFX_PLANE_FLAGS_REGION_ID		(1 << 0)
> #define VFIO_GFX_PLANE_FLAGS_PLANE_ID		(1 << 1)
> 	struct vfio_device_gfx_plane_info plane_info;
> 	__u32 id; 
> };
> 
> The flag defines the data in the id field as either referring to a
> region (perhaps there could be multiple regions with only one active)
> or a plane ID, which is acquired separately, such as via a dmabuf fd.
> This would be retrieved via an optional VFIO_DEVICE_QUERY_GFX_PLANE
> ioctl on the vfio device, implemented in the vendor driver.
> 
> Would the above, along with the already defined mechanism for defining
> device specific regions, account for NVIDIA's needs?
> 

Yes, works for region type solution that we would go with.

Thanks,
Kirti

> For dmabuf users, we'd still need a new ioctl to get the dmabuf fd.  We
> could either create a specific ioctl for this (ex.
> VFIO_DEVICE_GET_DMABUF_FD) or we could create a shared, generic GET_FD
> interface on the device.
> 
>>>  Maybe you could even allow mmap of that page
>>> to reduce the overhead of getting the current state.    
>>
>> Can't mmap that page to get surface information. There is no way to
>> synchronize between QEMU reading this mmapped region and vendor driver
>> writing it. There could be race condition in these two operations. Read
>> on this page should be trapped and blocking, so that surface in that
>> region is only updated when its asked for.
>>
>>> For the sake of
>>> userspace, I'd hope we'd still use the same structure for either the
>>> ioctl or region mapping.  I'm not really in favor of declaring that
>>> this particular ioctl might exist on the device fd when such-and-such
>>> region is present otherwise it might exist on a dmabuf manager fd.  
>>
>> Userspace will always use vfio device fd for this ioctl, it only have to
>> set proper arguments to its structure based on type.
> 
> Then we should kill off the manager fd unless there are arguments that
> still give it value.  Thanks,
> 
> Alex
>