Subject: Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
To: Alex Williamson <alex.williamson@redhat.com>,
        Gerd Hoffmann <kraxel@redhat.com>
CC: Xiaoguang Chen <xiaoguang.chen@intel.com>, <chris@chris-wilson.co.uk>,
        <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <zhenyuw@linux.intel.com>, <zhiyuan.lv@intel.com>,
        <intel-gvt-dev@lists.freedesktop.org>, <zhi.a.wang@intel.com>,
        <kevin.tian@intel.com>
References: <1497513611-2814-1-git-send-email-xiaoguang.chen@intel.com>
 <1497513611-2814-6-git-send-email-xiaoguang.chen@intel.com>
 <f19da207-a7f5-d3d3-4577-b071e7d75cf7@nvidia.com>
 <1497542438.29252.1.camel@redhat.com> <20170615143833.7526351b@w520.home>
From: Kirti Wankhede <kwankhede@nvidia.com>
Message-ID: <24c4880b-24f5-ea07-834c-c77d3e895c78@nvidia.com>
Date: Fri, 16 Jun 2017 19:02:30 +0530
MIME-Version: 1.0
In-Reply-To: <20170615143833.7526351b@w520.home>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3095
Lines: 92


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.

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

Thanks,
Kirti

> Thanks,
> 
> Alex
>