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

[Greg KH <greg@kroah.com>]

On Mon, Jun 06, 2022 at 04:10:09PM +0100, Robin Murphy wrote:
> On 2022-06-02 07:47, 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.
> 
> Just a drive-by observation, so apologies if I'm overlooking something
> obvious, but it sounds like the ideal compromise would be to expose a sysfs
> file which behaves as a dummy exported dma-buf. That way userspace could
> just open() it and try ioctl() directly - assuming that supported operations
> can fail distinctly from unsupported ones, or succeed as a no-op - which
> seems even simpler still.

ioctl() will not work on a sysfs file, sorry.