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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thank you for the review.

On Mon, Apr 21, 2014 at 3:54 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Arun,
>
> Thank you for the patch.
>
> 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.

> 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.
Also how about the drivers already using the v4l2_event. Should we
update those drivers too with v4l2_event_init() ?

Regards
Arun

>>       } u;
>>       __u32                           pending;
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux