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

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

 



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.

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

> 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;

-- 
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