Re: [RFCv4 01/21] media: add request API core and UAPI

[Alexandre Courbot]

On Wed, Feb 21, 2018 at 4:29 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
>> Hi Hans,
>>
>> On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> On 02/20/18 05:44, Alexandre Courbot wrote:
>
> <snip>
>
>>>> +#define MEDIA_REQUEST_IOC(__cmd, func)                                       \
>>>> +     [_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = {                 \
>>>> +             .cmd = MEDIA_REQUEST_IOC_##__cmd,                       \
>>>> +             .fn = func,                                             \
>>>> +     }
>>>> +
>>>> +struct media_request_ioctl_info {
>>>> +     unsigned int cmd;
>>>> +     long (*fn)(struct media_request *req);
>>>> +};
>>>> +
>>>> +static const struct media_request_ioctl_info ioctl_info[] = {
>>>> +     MEDIA_REQUEST_IOC(SUBMIT, media_request_ioctl_submit),
>>>> +     MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
>>>
>>> There are only two ioctls, so there is really no need for the
>>> MEDIA_REQUEST_IOC define. Just keep it simple.
>>
>> The number of times it is used doesn't change the fact that it helps
>> with readability IMHO.
>
> But this macro just boils down to:
>
> static const struct media_request_ioctl_info ioctl_info[] = {
>         { MEDIA_REQUEST_IOC_SUBMIT, media_request_ioctl_submit },
>         { MEDIA_REQUEST_IOC_REINIT, media_request_ioctl_reinit },
> };
>
> It's absolutely identical! So it seems senseless to me.

This expands to more than that - the index needs to be offset by 0x80,
something we probably don't want to repeat every line.

>
>>
>>>
>>>> +};
>>>> +
>>>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>>>> +                             unsigned long __arg)
>>>> +{
>>>> +     struct media_request *req = filp->private_data;
>>>> +     const struct media_request_ioctl_info *info;
>>>> +
>>>> +     if ((_IOC_NR(cmd) < 0x80) ||
>>>
>>> Why start the ioctl number at 0x80? Why not just 0?
>>> It avoids all this hassle with the 0x80 offset.
>
> There is no clash with the MC ioctls, so I really don't believe the 0x80
> offset is needed.

I suppose your comment in patch 16 supersedes this one. :)

>
>>>
>>>> +          _IOC_NR(cmd) >= 0x80 + ARRAY_SIZE(ioctl_info) ||
>>>> +          ioctl_info[_IOC_NR(cmd) - 0x80].cmd != cmd)
>>>> +             return -ENOIOCTLCMD;
>>>> +
>>>> +     info = &ioctl_info[_IOC_NR(cmd) - 0x80];
>>>> +
>>>> +     return info->fn(req);
>>>> +}
>
> <snip>
>
>>>> diff --git a/include/uapi/linux/media-request.h b/include/uapi/linux/media-request.h
>>>> new file mode 100644
>>>> index 000000000000..5d30f731a442
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/media-request.h
>>>> @@ -0,0 +1,37 @@
>>>> +/*
>>>> + * Media requests UAPI
>>>> + *
>>>> + * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#ifndef __LINUX_MEDIA_REQUEST_H
>>>> +#define __LINUX_MEDIA_REQUEST_H
>>>> +
>>>> +#ifndef __KERNEL__
>>>> +#include <stdint.h>
>>>> +#endif
>>>> +#include <linux/ioctl.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/version.h>
>>>> +
>>>> +/* Only check that requests can be used, do not allocate */
>>>> +#define MEDIA_REQUEST_FLAG_TEST                      0x00000001
>>>> +
>>>> +struct media_request_new {
>>>> +     __u32 flags;
>>>> +     __s32 fd;
>>>> +} __attribute__ ((packed));
>>>> +
>>>> +#define MEDIA_REQUEST_IOC_SUBMIT       _IO('|',  128)
>>>> +#define MEDIA_REQUEST_IOC_REINIT       _IO('|',  129)
>>>> +
>>>> +#endif
>>>>
>>>
>>> I need to think a bit more on this internal API, so I might come back
>>> to this patch for more comments.
>>
>> I think I should probably elaborate on why I think it is advantageous
>> to have these ioctls handled here.
>
> Sorry for the confusion, I was not actually referring to these ioctls.
> In fact, I really like them. It was more a general comment about the
> request API core.
>
> I should have been more clear.
>
> Regards,
>
>         Hans
>
>>
>> One of the reasons if that it does not force user-space to keep track
>> of who issued the request to operate on it. Semantically, the only
>> device a request could be submitted to is the device that produced it
>> anyway, so since that argument is constant we may as well get rid of
>> it (and we also don't need to pass the request FD as argument
>> anymore).
>>
>> It also gives us more freedom when designing new request-related
>> ioctls: before, all request-related operations were multiplexed under
>> a single MEDIA_IOC_REQUEST_CMD ioctl, which cmd field indicated the
>> actual operation to perform. With this design, all the arguments must
>> fit within the media_request_cmd structure, which may cause confusion
>> as it will have to be variable-sized. I am thinking in particular
>> about a future atomic-like API to set topology, controls and buffers
>> related to a request all at the same time. Having it as a request
>> ioctl seems perfectly fitting to me.
>>
>