Re: [PATCH v14 1/5] dma-buf: Add dma-buf heaps framework

[Daniel Vetter <daniel@ffwll.ch>]

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