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

[Greg KH <greg@kroah.com>]

On Tue, Jun 07, 2022 at 09:52:56AM -0500, Jason Ekstrand wrote:
> 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.

So you all have an api that is so hard to use, you can't use it until
you know what the capabilities of that api is before you call it.  So
you want an "out-of-band" way to know if this api is present or not.

Sounds like your api is the problem here, not the capability issue...

Again, I understand your problem, but sysfs is NOT the place for this.
Linux has a well-defined way to tell userspace if an ioctl is present or
not.  Just because it is "hard" to get to does not mean you get to
create a-brand-new-way-to-do-things for this, sorry.

Kernel API design and evolution is hard, that's the job description of a
kernel developer.  Boiler-plate code that you write once to do this
"does the kernel support this ioctl for a dma-buf" will be able to be
used by everyone for forever.  Best of all, it will already be done for
when you have this issue for the next ioctl you create :)

greg k-h