On 1/28/19 11:41 AM, Sakari Ailus wrote:
> Hi Hans,
>
> On Mon, Jan 28, 2019 at 10:52:40AM +0100, Hans Verkuil wrote:
>> On 1/28/19 10:21 AM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the patch.
>>>
>>> On Sat, Jan 26, 2019 at 12:06:19PM +0100, Hans Verkuil wrote:
>>>> This patch adds an extended version of VIDIOC_DQEVENT that:
>>>>
>>>> 1) is Y2038 safe by using a __u64 for the timestamp
>>>> 2) needs no compat32 conversion code
>>>> 3) is able to handle control events from 64-bit control types
>>>> by changing the type of the minimum, maximum, step and default_value
>>>> field to __u64
>>>>
>>>> All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
>>>> only place where the old event ioctl and structs are used.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> ---
>>>> Please let me know if there are additional requests for such a new ioctl.
>>>>
>>>> Note that I am using number 104 for the ioctl, but perhaps it would be better to
>>>> use an unused ioctl number like 1 or 3. There are quite a few holes in the
>>>> ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
>>>> ---
>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>> index 9a920f071ff9..969e775b8c25 100644
>>>> --- a/include/uapi/linux/videodev2.h
>>>> +++ b/include/uapi/linux/videodev2.h
>>>> @@ -2303,6 +2303,37 @@ struct v4l2_event {
>>>> __u32 reserved[8];
>>>> };
>>>>
>>>> +struct v4l2_event_ext_ctrl {
>>>> + __u32 changes;
>>>> + __u32 type;
>>>> + union {
>>>> + __s32 value;
>>>> + __s64 value64;
>>>> + };
>>>> + __s64 minimum;
>>>> + __s64 maximum;
>>>> + __s64 step;
>>>> + __s64 default_value;
>>>> + __u32 flags;
>>>> +};
>>>> +
>>>> +struct v4l2_ext_event {
>>>> + __u32 type;
>>>> + __u32 id;
>>>> + union {
>>>> + struct v4l2_event_vsync vsync;
>>>> + struct v4l2_event_ext_ctrl ctrl;
>>>> + struct v4l2_event_frame_sync frame_sync;
>>>> + struct v4l2_event_src_change src_change;
>>>> + struct v4l2_event_motion_det motion_det;
>>>> + __u8 data[64];
>>>> + } u;
>>>
>>> If I'd change something in the event IOCTL, I'd probably put the reserved
>>> fields here. That'd allow later taking some for the use of the event data
>>> if needed.
>>
>> Good point, I'll do that.
>>
>>> I might also increase the size of the event data. 64 bytes is not that
>>> much. But you indeed end up copying it around all the time... So it's a
>>> trade-off.
>>
>> I decided to leave this alone. I think by putting the reserved array after
>> the union (nice idea) we allow for such future extension should it be
>> necessary.
>
> Agreed.
>
>>
>>>
>>>> + __u64 timestamp;
>>>> + __u32 pending;
>>>> + __u32 sequence;
>>>> + __u32 reserved[8];
>>>> +};
>>>> +
>>>> #define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0)
>>>> #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK (1 << 1)
>>>>
>>>> @@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
>>>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>>>
>>>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>>>> +#define VIDIOC_DQEXTEVENT _IOR('V', 104, struct v4l2_ext_event)
>>>
>>> How do you plan to name the new buffer handling IOCTLs? I.e. with or
>>> without underscores around "EXT"?
>>
>> It's a good question. In my old patch I named them VIDIOC_EXT_QBUF etc. See:
>> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3
>>
>> So I think I should probably rename this to VIDIOC_EXT_DQEVENT.
>>
>> Alternatively, perhaps we should ditch the _ext_ usage and instead use a
>> version suffix: VIDIOC_DQEVENT_V2.
>>
>> The problem with EXT is that if you want to make a newer version of such a
>> control, you can't just name it EXT_EXT, that would be silly. But naming it
>
> You could use "EXT2" as well, I think that'd be fine, too. Think of ext4fs,
> for instance. :-)
>
>> _V3 would be fine.
>
> We have such a pattern on MC. But I'd still favour "EXT" since we already
> use that in V4L2.
>
>>
>> Frankly, the extended control ioctls have that problem, also due to awful
>> 64 bit alignment issues. It would be really nice to have _V3 versions of
>> those ioctls that do not require compat32 code.
>
> VIDIOC_G_EXT_CTRLS_V3 or VIDIOC_G_EXT2_CTRLS?
That might work, I hadn't thought about that. It's probably better than
introducing a new naming scheme.
>
> It'd be nice to hear other opinions, too..
>
Indeed.
Regards,
Hans
