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@xxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxx>
>>>> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
>>>> ---
>>>>
>>>> .../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;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
