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

[Brian Starkey]

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@xxxxxxx> wrote:
> >
> > Hi John,
> >
> > On Fri, Nov 01, 2019 at 09:42:34PM +0000, John Stultz wrote:
> > > From: "Andrew F. Davis" <afd@xxxxxx>
> > >
> > > 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?

> 
> 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.