Re: [PATCH v2 1/2] v4l: Add resolution change event.

[Hans Verkuil <hverkuil@xs4all.nl>]

On 04/21/2014 10:59 PM, Laurent Pinchart wrote:
> Hi Arun,
> 
> On Monday 21 April 2014 17:19:26 Arun Kumar K wrote:
>> On Mon, Apr 21, 2014 at 3:54 PM, Laurent Pinchart wrote:
>>> On Monday 21 April 2014 14:56:01 Arun Kumar K wrote:
>>>> From: Pawel Osciak <posciak@chromium.org>
>>>>
>>>> This event indicates that the decoder has reached a point in the stream,
>>>> at which the resolution changes. The userspace is expected to provide a
>>>> new
>>>> set of CAPTURE buffers for the new format before decoding can continue.
>>>> The event can also be used for more generic events involving resolution
>>>> or format changes at runtime for all kinds of video devices.
>>>>
>>>> Signed-off-by: Pawel Osciak <posciak@chromium.org>
>>>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>>>> ---
>>>>
>>>>  .../DocBook/media/v4l/vidioc-subscribe-event.xml   |   16
>>>>  ++++++++++++++++
>>>>  include/uapi/linux/videodev2.h                     |    6 ++++++
>>>>  2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
>>>> b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml index
>>>> 5c70b61..0aec831 100644
>>>> --- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
>>>> +++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
>>>> @@ -155,6 +155,22 @@
>>>>           </entry>
>>>>         </row>
>>>>         <row>
>>>> +         <entry><constant>V4L2_EVENT_SOURCE_CHANGE</constant></entry>
>>>> +         <entry>5</entry>
>>>> +         <entry>
>>>> +           <para>This event is triggered when a resolution or format
>>>> change +            is detected during runtime by the video device. It
>>>> can be a +            runtime resolution change triggered by a video
>>>> decoder or the +            format change happening on an HDMI
>>>> connector. Application may +            need to reinitialize buffers
>>>> before proceeding further.</para> +
>>>> +              <para>This event has a &v4l2-event-source-change;
>>>> associated
>>>> +           with it. This has significance only for v4l2 subdevs where
>>>> the
>>>> +           <structfield>pad_num</structfield> field will be updated with
>>>> +           the pad number on which the event is triggered.</para>
>>>> +         </entry>
>>>> +       </row>
>>>> +       <row>
>>>>
>>>>           <entry><constant>V4L2_EVENT_PRIVATE_START</constant></entry>
>>>>           <entry>0x08000000</entry>
>>>>           <entry>Base event number for driver-private events.</entry>
>>>> diff --git a/include/uapi/linux/videodev2.h
>>>> b/include/uapi/linux/videodev2.h index 6ae7bbe..12e0614 100644
>>>> --- a/include/uapi/linux/videodev2.h
>>>> +++ b/include/uapi/linux/videodev2.h
>>>> @@ -1733,6 +1733,7 @@ struct v4l2_streamparm {
>>>>  #define V4L2_EVENT_EOS                               2
>>>>  #define V4L2_EVENT_CTRL                              3
>>>>  #define V4L2_EVENT_FRAME_SYNC                        4
>>>> +#define V4L2_EVENT_SOURCE_CHANGE             5
>>>>  #define V4L2_EVENT_PRIVATE_START             0x08000000
>>>>  
>>>>  /* Payload for V4L2_EVENT_VSYNC */
>>>> @@ -1764,12 +1765,17 @@ struct v4l2_event_frame_sync {
>>>>       __u32 frame_sequence;
>>>>  };
>>>>
>>>> +struct v4l2_event_source_change {
>>>> +     __u32 pad_num;
>>>
>>> I would call the field just "pad",
>>
>> Ok.
>>
>>>> +};
>>>> +
>>>>
>>>>  struct v4l2_event {
>>>>       __u32                           type;
>>>>       union {
>>>>               struct v4l2_event_vsync         vsync;
>>>>               struct v4l2_event_ctrl          ctrl;
>>>>               struct v4l2_event_frame_sync    frame_sync;
>>>> +             struct v4l2_event_source_change source_change;
>>>>               __u8                            data[64];
>>>
>>> This looks pretty good to me, but I'm a bit concerned about future
>>> compatibility. We might need to report more information to userspace, and
>>> in particular what has been changed at the source (resolution, format,
>>> ...). In order to do so, we'll need to add a flag field to
>>> v4l2_event_source_change.
>>
>> Ok a flag can be added with bitfields for reporting specific event type.
> 
> I don't think we need to add it now. Just making sure it can be added later 
> without breaking the userspace API would be enough for me.

I would add it, I think it makes sense. It would be similar to the 'changes'
field in struct v4l2_event_ctrl.

> 
>>> The next __u32 right after the source_change field must thus be zeroed. I
>>> see two ways of doing so:
>>>
>>> - zeroing the whole data array before setting event-specific data
>>> - adding a reserved must-be-zeroed field to v4l2_event_source_change
>>>
>>> I like the former better as it's more generic, but we then need to ensure
>>> that all drivers zero the whole data field correctly. Adding a new
>>> v4l2_event_init() function would help with that.
>>
>> Is that a good idea to have an init() function just for zeroing the data
>> field? If this is agreed upon, I can add this, but it can be easily missed
>> out by drivers.
> 
> I'm not sure. On one hand it would be easier to catch missing calls if we use 
> a dedicated function, as the function could set an initialized flag in the 
> event structure that would be checked by v4l2_event_queue. On the other hand, 
> that might be overengineering, and we could just manually check that all 
> drivers memset the structure to 0 before initializing fields. In both cases 
> the goal is to make sure that the structure is properly initialized, to avoid 
> leaking kernel data to userspace, and to avoid breaking the ABI when we'll 
> need to extend the v4l2_event_source_change structure. Hans might have an 
> opinion on that.

I think a v4l2_event_init function is not a bad idea:

void v4l2_event_init(struct v4l2_event *ev, u32 type, u32 id)
{
	ev->type = type;
	ev->id = id;
	memset(ev->u, 0, sizeof(ev->u));
}

Note: all other fields, including reserved[], are set by v4l2-event.c.

Regards,

	Hans

> 
>> Also how about the drivers already using the v4l2_event. Should we
>> update those drivers too with v4l2_event_init() ?
> 
> If we decide to introduce a new function, yes. Otherwise, no :-)
> 
>>>>       } u;
>>>>       __u32                           pending;
>