Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 4 Nov 2019 19:30:23 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Brian Starkey <Brian.Starkey@arm.com>
Cc:     Dave Airlie <airlied@gmail.com>, Hillf Danton <hdanton@sina.com>,
        Sudipto Paul <Sudipto.Paul@arm.com>,
        Sandeep Patil <sspatil@google.com>,
        Alistair Strachan <astrachan@google.com>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        Chenbo Feng <fengc@google.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Liam Mark <lmark@codeaurora.org>,
        "Andrew F. Davis" <afd@ti.com>,
        Christoph Hellwig <hch@infradead.org>, nd <nd@arm.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Hridya Valsaraju <hridya@google.com>,
        Pratik Patel <pratikp@codeaurora.org>
Subject: Re: [PATCH v14 1/5] dma-buf: Add dma-buf heaps framework
Message-ID: <20191104183023.GT10326@phenom.ffwll.local>
Mail-Followup-To: Brian Starkey <Brian.Starkey@arm.com>,
        Dave Airlie <airlied@gmail.com>, Hillf Danton <hdanton@sina.com>,
        Sudipto Paul <Sudipto.Paul@arm.com>,
        Sandeep Patil <sspatil@google.com>,
        Alistair Strachan <astrachan@google.com>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        Chenbo Feng <fengc@google.com>, lkml <linux-kernel@vger.kernel.org>,
        Liam Mark <lmark@codeaurora.org>, "Andrew F. Davis" <afd@ti.com>,
        Christoph Hellwig <hch@infradead.org>, nd <nd@arm.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Hridya Valsaraju <hridya@google.com>,
        Pratik Patel <pratikp@codeaurora.org>
References: <20191101214238.78015-1-john.stultz@linaro.org>
 <20191101214238.78015-2-john.stultz@linaro.org>
 <20191104102410.66wlyoln5ahlgkem@DESKTOP-E1NTVVP.localdomain>
 <CAPM=9tydXxV-6++HkkA+JX9GPWE1sN_8CGVCVn-Mwfgfzcn=hg@mail.gmail.com>
 <20191104174341.m6hjzog2vibc3ek3@DESKTOP-E1NTVVP.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191104174341.m6hjzog2vibc3ek3@DESKTOP-E1NTVVP.localdomain>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Nov 04, 2019 at 05:43:51PM +0000, Brian Starkey wrote:
> Hi Dave,
> 
> On Tue, Nov 05, 2019 at 02:58:17AM +1000, Dave Airlie wrote:
> > On Mon, 4 Nov 2019 at 20:24, Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > Hi John,
> > >
> > > On Fri, Nov 01, 2019 at 09:42:34PM +0000, John Stultz wrote:
> > > > From: "Andrew F. Davis" <afd@ti.com>
> > > >
> > > > This framework allows a unified userspace interface for dma-buf
> > > > exporters, allowing userland to allocate specific types of memory
> > > > for use in dma-buf sharing.
> > > >
> > > > Each heap is given its own device node, which a user can allocate
> > > > a dma-buf fd from using the DMA_HEAP_IOC_ALLOC.
> > > >
> > > > Additionally should the interface grow in the future, we have a
> > > > DMA_HEAP_IOC_GET_FEATURES ioctl which can return future feature
> > > > flags.
> > >
> > > The userspace patch doesn't use this - and there's no indication of
> > > what it's intended to allow in the future. I missed the discussion
> > > about it, do you have a link?
> > >
> > > I thought the preferred approach was to add the new ioctl only when we
> > > need it, and new userspace on old kernels will get "ENOSYS" to know
> > > that the kernel doesn't support it.
> > 
> > This works once, expand the interface 3 or 4 times with no features
> > ioctl, and you start being hostile to userspace, which feature does
> > ENOSYS mean isn't supported etc.
> 
> Sorry, perhaps I wasn't clear (or I misunderstand what you're saying
> about working only once). I'm not against adding a get_features ioctl,
> I just don't see why we'd add it before we have any features?
> 
> When we gain the first "feature", can't we add the get_features ioctl
> then? For Future SW that knows about Future features, "ENOSYS" will
> always mean "no features". If it doesn't get ENOSYS, then it can use
> the ioctl to find out what features it has.
> 
> Otherwise we're adding an ioctl which doesn't do anything, based on
> the assumption that in the future it _will_ do something... but we
> don't know that that is yet... but we're pretty sure whatever it will
> do can be described with a u64?
> 
> Why not wait until we know what we want it for, and then implement it
> with an interface which we know is appropriate at that time?

Yeah I'm with Brian, adding the get_feature ioctl when we need it.
Otherwise it's going to be broken somehow and we'll immediately ref to
get_features2 :-)
-Daniel

> > Next userspace starts to rely on kernel version, but then someone
> > backports a feature, down the rabbit hole we go.
> > 
> 
> I suppose that adding the feature ioctl later would mean that it might
> be possible to backport a feature without also backporting the ioctl,
> depending on how the patches are split up. I think it would be pretty
> hard to do accidentally though.
> 
> Thanks,
> -Brian
> 
> > Be nice to userspace give them a features ioctl they can use to figure
> > out in advance which of the 4 uapis the kernel supports.
> > 
> > Dave.
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch