On 6/16/2017 10:09 PM, Alex Williamson wrote:
> On Fri, 16 Jun 2017 19:02:30 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>
>> On 6/16/2017 2:08 AM, Alex Williamson wrote:
>>> On Thu, 15 Jun 2017 18:00:38 +0200
>>> Gerd Hoffmann <kraxel@xxxxxxxxxx> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
