DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 97F0FC04B950
Date: Fri, 16 Jun 2017 10:39:59 -0600
From: Alex Williamson <alex.williamson@redhat.com>
To: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
        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>
Subject: Re: [PATCH v9 5/7] vfio: Define vfio based dma-buf operations
Message-ID: <20170616103959.3b6f1681@t450s.home>
In-Reply-To: <24c4880b-24f5-ea07-834c-c77d3e895c78@nvidia.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>
        <24c4880b-24f5-ea07-834c-c77d3e895c78@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4991
Lines: 135

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?

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