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

[Robin Murphy <robin.murphy@arm.com>]

On 2022-06-06 16:22, Greg KH wrote:
> 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.

Ah, fair enough - TBH I should have just said "a file", since I presume 
some sort of /dev/dma-buf might also be an option, if a bit more work to 
implement.

I'll scuttle back to my low-level DMA corner now :)

Cheers,
Robin.