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

[Asahi Lina]

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@xxxxxxxxxxxxx>
>> ---
>>   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)...

Basically I'm not sure if it makes sense to fully support noncontiguous
ioctl numbers automagically, or just say drivers need to explicitly list
gaps. I'd love to hear the opinion of other DRM folks about this!

~~ Lina