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

[Hans Verkuil]

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.

> 
>>
>>> +};
>>> +
>>> +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.

>>
>>> +          _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.
>