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

[Jason Ekstrand <jason.ekstrand@collabora.com>]

On Tue, 2022-06-07 at 12:55 +0200, Greg KH wrote:
> 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?

Because we need a file descriptor to call that ioctl on.  Currently,
that file descriptor is the dma-buf itself.  We could move it to be a
DRM IOCTL but then it ends up unnecessarily tied to DRM even if
someone's trying to use it with V4L or some other dma-buf
producer/consumer.  IDK if that's a real problem in practice but there
didn't seem to be a good reason to tie it to things outside dma-buf.

--Jason