Re: [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction

["Maíra Canal" <mcanal@igalia.com>]

On 3/9/23 03:15, Dave Airlie wrote:
> On Thu, 9 Mar 2023 at 15:32, Asahi Lina <lina@asahilina.net> wrote:
>>
>> On 08/03/2023 00.32, Maíra Canal wrote:
>>> On 3/7/23 11:25, Asahi Lina wrote:
>>>> DRM drivers need to be able to declare which driver-specific ioctls they
>>>> support. This abstraction adds the required types and a helper macro to
>>>> generate the ioctl definition inside the DRM driver.
>>>>
>>>> Note that this macro is not usable until further bits of the
>>>> abstraction are in place (but it will not fail to compile on its own, if
>>>> not called).
>>>>
>>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>>> ---
>>>>    drivers/gpu/drm/Kconfig         |   7 ++
>>>>    rust/bindings/bindings_helper.h |   2 +
>>>>    rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>>>>    rust/kernel/drm/mod.rs          |   5 ++
>>>>    rust/kernel/lib.rs              |   2 +
>>>>    5 files changed, 163 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>>> index dc0f94f02a82..dab8f0f9aa96 100644
>>>> --- a/drivers/gpu/drm/Kconfig
>>>> +++ b/drivers/gpu/drm/Kconfig
>>>> @@ -27,6 +27,13 @@ menuconfig DRM
>>>>         details.  You should also select and configure AGP
>>>>         (/dev/agpgart) support if it is available for your platform.
>>>>
>>>
>>> [...]
>>>
>>>> +
>>>> +/// Declare the DRM ioctls for a driver.
>>>> +///
>>>> +/// Each entry in the list should have the form:
>>>> +///
>>>> +/// `(ioctl_number, argument_type, flags, user_callback),`
>>>> +///
>>>> +/// `argument_type` is the type name within the `bindings` crate.
>>>> +/// `user_callback` should have the following prototype:
>>>> +///
>>>> +/// ```
>>>> +/// fn foo(device: &kernel::drm::device::Device<Self>,
>>>> +///        data: &mut bindings::argument_type,
>>>> +///        file: &kernel::drm::file::File<Self::File>,
>>>> +/// )
>>>> +/// ```
>>>> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```
>>>> +/// kernel::declare_drm_ioctls! {
>>>> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +#[macro_export]
>>>> +macro_rules! declare_drm_ioctls {
>>>> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
>>>> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
>>>> +            const _:() = {
>>>> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
>>>> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
>>>> +                // and that the sizeof of the specified type is correct.
>>>
>>> I believe that not necessarily the IOCTLs need to be in the right order and
>>> with no gaps. For example, armada_drm.h has a gap in between 0x00 and
>>> 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
>>> virtgpu, start their IOCTLs with 0x01.
>>
>> Yeah, we talked about this a bit... do you have any ideas about how to
>> design this? I think it should be possible with a const function
>> initializing an array entry by entry, we just need a two-pass macro
>> (once to determine the max ioctl number, then again to actually output
>> the implementation).
>>
>> I'm not sure why drivers would have gaps in the ioctl numbers though...
>> my idea was that new drivers shouldn't need that as far as I can tell
>> (you can't remove APIs after the fact due to UAPI stability guarantees,
>> so as long as you don't have gaps to begin with...). But I guess if
>> we're reimplementing existing drivers in Rust we'll need this... though
>> maybe it makes sense to just say it's not supported and require
>> reimplementations that have holes to just explicitly add dummy ioctls
>> that return EINVAL? We could even provide such a dummy generic ioctl
>> handler on the abstraction side, so drivers just have to add it to the
>> list, or make the macro take a special token that is used for
>> placeholder ioctls that don't exist (which then creates the NULL
>> function pointer that the drm core interprets as invalid)...
> 
> I can think of two reason for gaps having appeared:
> 
> a) developers wanted to group new uapis at a nice base number.
> This is never essential it's just makes things easier to read, and
> allows slotting other ioctls into the gaps later.
> 
> b) parallel feature development ends up conflicting then one thread never lands.
> I've got two-three devs each adding a uAPI, we assign them 0x10, 0x11,
> 0x12 while they work, then 0x11 never lands because it was a bad idea.
> 
> However I think you should be fine enforcing a non-sparse space here
> unless we want to handle replacing current drivers, as long as it's
> hard to screw up so you know early.

I guess it would be nice to support old UAPIs for cases of reimplementations.
Currently, I'm working on a reimplementation of vgem and I ended up having to
create a dummy IOCTL to deal with the sparse number space. Although creating
dummy IOCTLs works, I don't believe it is a nice practice.

Moreover, I believe that if we keep developing new drivers with Rust, cases
(a) and (b) will end up happening, and maybe the Rust abstractions should
work like DRM and allow it to happen.

Best Regards,
- Maíra Canal

> 
> Dave.