DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DAA0661D37
Message-ID: <1498030461.5802.3.camel@redhat.com>
Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
 operations
From: Gerd Hoffmann <kraxel@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: "Zhang, Tina" <tina.zhang@intel.com>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        "Chen, Xiaoguang" <xiaoguang.chen@intel.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "Lv, Zhiyuan" <zhiyuan.lv@intel.com>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>,
        "Wang, Zhenyu Z" <zhenyu.z.wang@intel.com>
Date: Wed, 21 Jun 2017 09:34:21 +0200
In-Reply-To: <20170620090004.44ac7fbc@w520.home>
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>
         <1497854312.4207.4.camel@redhat.com> <20170619085530.1f5e46dc@w520.home>
         <237F54289DF84E4997F34151298ABEBC7C56EBE0@SHSMSX101.ccr.corp.intel.com>
         <1497956256.16795.7.camel@redhat.com> <20170620090004.44ac7fbc@w520.home>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2606
Lines: 72

  Hi,

> We already have VFIO_DEVICE_GET_INFO which returns:
> 
> struct vfio_device_info {
>         __u32   argsz;
>         __u32   flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports
> reset */
> #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform
> device */
> #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device
> */
> #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
>         __u32   num_regions;    /* Max region index + 1 */
>         __u32   num_irqs;       /* Max IRQ index + 1 */
> };
> 
> We could use two flag bits to indicate dmabuf or graphics region
> support.

That works too.

> > Then this to query the plane:
> > 
> > struct vfio_device_gfx_query_plane {
> >         __u32 argsz;
> >         __u32 flags;
> >         struct vfio_device_gfx_plane_info plane_info;  /* out */
> >         __u32 plane_type;                              /* in  */
> > };
> 
> I'm not sure why we're using an enum for something that can currently
> be defined with 2 bits,

We can reuse the DRM_PLANE_TYPE_* then.

>  seems like this would be another good use of
> flags.  We could even embed an enum into the flags if we want to
> leave some expansion room, 4 bits maybe?  Also, I was imagining that
> a
> device could support multiple graphics regions, that's where
> specifying
> the "id" as a region index seemed useful.

Hmm, yes, possibly for multihead configurations.  But I guess for
proper multihead support we would need more than just an region id.

Or do you have something else in mind?

> > With the generation we can also do something different:  Pass in
> > plane_type and generation, and have VFIO_DEVICE_GET_DMABUF_FD
> > return
> > an error in case the generation doesn't match.  In that case it
> > doesn't
> > make much sense any more to have a separate plane_info struct,
> > which
> > was added so we don't have to duplicate things in query-plane and
> > get-
> > dmabuf ioctl structs.
> 
> I'm not sure I understand how this works for a region, the region is
> always the current generation, how can the user ever be sure the
> plane_info matches what is exposed in the region?

generation will change each time the plane configuration (not content)
changes.  Typically that will be on video mode switches.  In the dmabuf
case also on pageflips.

cheers,
  Gerd