DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C28F080C04
Date: Thu, 15 Jun 2017 14:38:33 -0600
From: Alex Williamson <alex.williamson@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.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: <20170615143833.7526351b@w520.home>
In-Reply-To: <1497542438.29252.1.camel@redhat.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>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2220
Lines: 64

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.  Maybe you could even allow mmap of that page
to reduce the overhead of getting the current state.  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.
Thanks,

Alex