Re: [PATCH v4] dma-buf: Add a capabilities directory

[Greg KH <greg@kroah.com>]

On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote:
> On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote:
> >
> > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote:
> >
> > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote:
> > >
> > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote:
> > > >
> > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote:
> > > > >
> > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no
> > > > > > choice but to try to perform the IOCTL on an existing DMA-BUF.
> > > > >
> > > > > Which is correct and how all kernel features work (sorry I missed the
> > > > > main goal of this patch earlier and focused only on the sysfs stuff).
> > > > >
> > > > > > However, user-space may want to figure out whether or not the
> > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at
> > > > > > initialization time in a Wayland compositor.
> > > > >
> > > > > Why not just do the ioctl in a test way? That's how we determine kernel
> > > > > features, we do not poke around in sysfs to determine what is, or is
> > > > > not, present at runtime.
> > > > >
> > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF
> > > > > > subsystem to advertise supported features. Add a
> > > > > > sync_file_import_export entry which indicates that importing and
> > > > > > exporting sync_files from/to DMA-BUFs is supported.
> > > > >
> > > > > No, sorry, this is not a sustainable thing to do for all kernel features
> > > > > over time. Please just do the ioctl and go from there. sysfs is not
> > > > > for advertising what is and is not enabled/present in a kernel with
> > > > > regards to functionality or capabilities of the system.
> > > > >
> > > > > If sysfs were to export this type of thing, it would have to do it for
> > > > > everything, not just some random tiny thing of one kernel driver.
> > > >
> > > > I'd argue that DMA-BUF is a special case here.
> > >
> > > So this is special and unique just like everything else? :)
> > >
> > > > To check whether the import/export IOCTLs are available, user-space
> > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF,
> > > > user-space needs to enumerate GPUs, pick one at random, load GBM or
> > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the
> > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown
> > > > all of this. There is no other way.
> > > >
> > > > This sounds like a roundabout way to answer the simple question "is the
> > > > IOCTL available?". Do you have another suggestion to address this
> > > > problem?
> > >
> > > What does userspace do differently if the ioctl is present or not?
> >
> > Globally enable a synchronization API for Wayland clients, for instance
> > in the case of a Wayland compositor.
> >
> > > And why is this somehow more special than of the tens of thousands of
> > > other ioctl calls where you have to do exactly the same thing you list
> > > above to determine if it is present or not?
> >
> > For other IOCTLs it's not as complicated to obtain a FD to do the test
> > with.
> 
> Two expand on this:
> 
> - compositor opens the drm render /dev node
> - compositor initializes the opengl or vulkan userspace driver on top of that
> - compositor asks that userspace driver to allocate some buffer, which
> can be pretty expensive
> - compositor asks the userspace driver to export that buffer into a dma-buf
> - compositor can finally do the test ioctl, realizes support isn't
> there and tosses the entire thing
> 
> read() on a sysfs file is so much more reasonable it's not even funny.

I agree it seems trivial and "simple", but that is NOT how to determine
what is, and is not, a valid ioctl command for a device node.

The only sane way to do this is like we have been doing for the past 30+
years, make the ioctl and look at the return value.

Now if we want to come up with a new generic "here's the
capabilities/ioctls/whatever" that the kernel currently supports at this
point in time api, wonderful, but PLEASE do not overload sysfs to do
something like this as that is not what it is for at this moment in
time.

Don't just do this for one specific ioctl as there really is nothing
special about it at all ("it's special and unique just like all other
ioctls...")

> Plan B we discussed is to add a getparam to signify this to the drm
> ioctl interface, but that has the design problem that a feature in the
> dma-buf subsystem is announced in a totally different subsystem (ok
> same maintainers), and if that ever gets out of sync your userspace
> breaks. So really no good.

getparam makes sense in a way, if it doesn't change over time (i.e. if
you call it now, will it be the same if you call it again if some kernel
module is added/removed in the meantime?)  Also be aware of
suspend/resume where you can swap out the kernel underneath running
userspace and that kernel might have different capabilities now.  So you
can't just not check error values of ioctl commands (not that you are
saying you want to here, just that it's more complex than might
originally seem.)

> So if sysfs also isn't the right approach, and the getparam ioctl on
> drm is defo worse, what is the right approach? Ideally without setting
> up the entire userspace render driver and doing some expensive-ish
> (depending upon driver at least) buffer allocations just to check for
> a feature.
> 
> Note that some compositors want to gate their "should I use vk for
> rendering and expose some additional features to client" decision on
> this, so setting up the wrong renderer just to test whether that would
> work is not a very great approach.
> 
> Also the last time we added a feature to dma-buf was in 3.17, so I
> guess everyone just hardcodes nowadays that all dma-buf features are
> present. Which isn't great, and that's why we're trying to fix here.

Why can't you call the test ioctl with an invalid value to see if it is
present or not (-ENOTTY vs. -EINVAL) right at the beginning before you
set up anything?

thanks,

greg k-h