| Message ID | 1398072362-24962-2-git-send-email-arun.kk@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pawel, On Mon, Apr 21, 2014 at 2:58 PM, Pawel Osciak <posciak@chromium.org> wrote: > Arun, I think it'd better if you attributed this patch to yourself, I'm not > the author of it. I only wrote patch 2/2 in this series. > Thanks, > Pawel > > Sure I will change it. I kept your name as it was a rework of your patch from chrome tree. Will update the author name and post v3 if there are no other comments on this. Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arun, Thank you for the patch. 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", > +}; > + > 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. 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. > } u; > __u32 pending;
Hi Laurent, Thank you for the review. On Mon, Apr 21, 2014 at 3:54 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Arun, > > Thank you for the patch. > > 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. > 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-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > 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;
Hi Pawel, > From: Pawel Osciak [mailto:posciak@chromium.org] > Sent: Monday, April 21, 2014 12:27 PM > To: Arun Kumar K > Cc: linux-media@vger.kernel.org; linux-samsung-soc; Kamil Debski; > Sylwester Nawrocki; Hans Verkuil; Laurent Pinchart > Subject: Re: [PATCH v2 1/2] v4l: Add resolution change event. > > As a side note, because this is not really codified in the API, I would > like this event to indicate not only resolution change mid-stream, but > also detection of initial resolution, which should be a subset of > resolution change. I think this would make sense for the codec > interface: > > Video decode: > 1. S_FMT to given codec on OUTPUT queue. > 2. REQBUFS(n) and STREAMON on OUTPUT queue. > 3. Keep QBUFing until we get an resolution change event on the CAPTURE > queue; until then, the driver/codec HW will operate on the OUTPUT queue > only and try to detect relevant headers in the OUTPUT buffers, and will > send resolution change event once it finds resolution, profile, etc. > info). DQEVENT. > 4. G_FMT on CAPTURE to get the discovered output format (resolution), > REQBUFS and STREAMON on the CAPTURE queue. > 5. Normal mem-to-mem decoding. > 6. If a resolution change event arrives on CAPTURE queue, DQEVENT, > STREAMOFF, REQBUFS(0) only on CAPTURE queue, and goto 4. OUTPUT queue > operates completely independently of this. > > Also, this event should invariably indicate all of the below: > - all output buffers from before resolution change are already ready on > the CAPTURE queue to DQBUF (so it's ready to REQBUFS(0) after DQBUFs), > and > - there will be no more new ready buffers on the CAPTURE queue until > the streamoff-reqbufs(0)-g_fmt-reqbufs()-streamon is performed, and > - OUTPUT queue is completely independent of all of the above and can be > still used as normal, i.e. stream buffers can still keep being queued > at any stage of the resolution change and they will be decoded after > resolution change sequence is finished; > > If we all agree to the above, I will prepare a subsequent patch for the > documentation to include the above. If I understand correctly this will keep the old application working. By this I mean application that do not use events and rely on the current mechanism to detect initial header parsing and resolution change. If backward compatibility is kept I am all for the changes proposed by you. > > Thanks, > Pawel Best wishes,
Hi Pawel, > From: Pawel Osciak [mailto:posciak@chromium.org] > Sent: Tuesday, April 22, 2014 9:46 AM > To: Kamil Debski > Cc: Arun Kumar K; linux-media@vger.kernel.org; linux-samsung-soc; > Sylwester Nawrocki; Hans Verkuil; Laurent Pinchart > Subject: Re: [PATCH v2 1/2] v4l: Add resolution change event. > > Hi Kamil, > > On Tue, Apr 22, 2014 at 4:34 PM, Kamil Debski <k.debski@samsung.com> > wrote: > > > Hi Pawel, > > > From: Pawel Osciak [mailto:posciak@chromium.org] > > Sent: Monday, April 21, 2014 12:27 PM > > To: Arun Kumar K > > Cc: linux-media@vger.kernel.org; linux-samsung-soc; Kamil > Debski; > > Sylwester Nawrocki; Hans Verkuil; Laurent Pinchart > > Subject: Re: [PATCH v2 1/2] v4l: Add resolution change event. > > > > > As a side note, because this is not really codified in the API, > I would > > like this event to indicate not only resolution change mid- > stream, but > > also detection of initial resolution, which should be a subset > of > > resolution change. I think this would make sense for the codec > > interface: > > > > Video decode: > > 1. S_FMT to given codec on OUTPUT queue. > > 2. REQBUFS(n) and STREAMON on OUTPUT queue. > > 3. Keep QBUFing until we get an resolution change event on the > CAPTURE > > queue; until then, the driver/codec HW will operate on the > OUTPUT queue > > only and try to detect relevant headers in the OUTPUT buffers, > and will > > send resolution change event once it finds resolution, profile, > etc. > > info). DQEVENT. > > 4. G_FMT on CAPTURE to get the discovered output format > (resolution), > > REQBUFS and STREAMON on the CAPTURE queue. > > 5. Normal mem-to-mem decoding. > > 6. If a resolution change event arrives on CAPTURE queue, > DQEVENT, > > STREAMOFF, REQBUFS(0) only on CAPTURE queue, and goto 4. OUTPUT > queue > > operates completely independently of this. > > > > Also, this event should invariably indicate all of the below: > > - all output buffers from before resolution change are already > ready on > > the CAPTURE queue to DQBUF (so it's ready to REQBUFS(0) after > DQBUFs), > > and > > - there will be no more new ready buffers on the CAPTURE queue > until > > the streamoff-reqbufs(0)-g_fmt-reqbufs()-streamon is performed, > and > > - OUTPUT queue is completely independent of all of the above > and can be > > still used as normal, i.e. stream buffers can still keep being > queued > > at any stage of the resolution change and they will be decoded > after > > resolution change sequence is finished; > > > > If we all agree to the above, I will prepare a subsequent patch > for the > > documentation to include the above. > > > If I understand correctly this will keep the old application > working. > By this I mean application that do not use events and rely on the > current > mechanism to detect initial header parsing and resolution change. > > If backward compatibility is kept I am all for the changes > proposed by you. > > > > MFC codec depends on the userspace to parse the stream and pass the > stream header with resolution info before calling G_FMT. So if it > ignores the events but keeps doing this, things should keep working I > think, as the G_FMT should still work as before. > > This event will help userspace that doesn't want to bother itself with > parsing the stream to know if the right header was queued and rely on > events to know when to call G_FMT instead. > > What do you think? Am I missing something? Do you think this will work with all MFC versions? I guess that your focus is on the newer MFC version such as v6 or even v7. I had worked mostly with the v5 version and I am not sure how well it will handle the situation when it does not receive the header as the first frame. Do you have a target with v5 of MFC at hand? I really like your idea. If you haven't got the possibility to check how it works on v5 then I suggest you go ahead with patches and later I will do some testing on MFC v5. Best wishes,
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/21/2014 11:26 AM, 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; > +}; > + > 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]; > } u; > __u32 pending; > This needs to be done differently. The problem is that you really have multiple events that you want to subscribe to: one for each pad (or input: see the way VIDIOC_G/S_EDID maps pad to an input or output index when used with a video node, we have to support that for this event as well). What you want to do is similar to what is done for control events: there you can subscribe for a specific control and get notified when that control changes. The way that works in the event code is that the 'id' field in the v4l2_event struct contains that control ID, or, in this case, the pad/input/output index. In the application you will subscribe to the SOURCE_CHANGE event for a specific pad/input/output index. In other words, the pad_num field should be dropped and the id field used instead. Assuming we will also add a 'changes' field (see my reply to the other post on that topic), then you should also provide replace and merge ops (see v4l2-ctrls.c). That way it is sufficient to use just one element when calling v4l2_event_subscribe(): you will never loose information since if multiple events arrive before the application can dequeue them, the 'changes' information will be the ORed value of all those intermediate events. It's all more work, but it is critical to ensure that the application never misses events. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On 04/22/14 18:22, Hans Verkuil wrote: > On 04/21/2014 11:26 AM, 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; >> +}; >> + >> 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]; >> } u; >> __u32 pending; >> > > This needs to be done differently. The problem is that you really have multiple > events that you want to subscribe to: one for each pad (or input: see the way > VIDIOC_G/S_EDID maps pad to an input or output index when used with a video node, > we have to support that for this event as well). > > What you want to do is similar to what is done for control events: there you can > subscribe for a specific control and get notified when that control changes. > > The way that works in the event code is that the 'id' field in the v4l2_event > struct contains that control ID, or, in this case, the pad/input/output index. > As I am new to v4l2-events itself, I might have some noob questions. I understood filling pad index into id field. But for video nodes, the application is supposed to put v4l2_buf_type in the id field? > In the application you will subscribe to the SOURCE_CHANGE event for a specific > pad/input/output index. > > In other words, the pad_num field should be dropped and the id field used > instead. > > Assuming we will also add a 'changes' field (see my reply to the other post > on that topic), then you should also provide replace and merge ops (see > v4l2-ctrls.c). That way it is sufficient to use just one element when calling > v4l2_event_subscribe(): you will never loose information since if multiple > events arrive before the application can dequeue them, the 'changes' information > will be the ORed value of all those intermediate events. > If I understand correctly, the implementation should go somewhat in the following lines: +void v4l2_event_src_replace(struct v4l2_event *old, const struct v4l2_event *new) +{ + u32 old_changes = old->u.src_change.changes; + + old->u.src_change = new->u.src_change; + old->u.src_change.changes |= old_changes; +} + +void v4l2_event_src_merge(const struct v4l2_event *old, struct v4l2_event *new) +{ + new->u.src_change.changes |= old->u.src_change.changes; +} + +const struct v4l2_subscribed_event_ops v4l2_event_src_ch_ops = { + .replace = v4l2_event_src_replace, + .merge = v4l2_event_src_merge, +}; + +int v4l2_src_change_event_subscribe(struct v4l2_fh *fh, + const struct v4l2_event_subscription *sub) +{ + if (sub->type == V4L2_EVENT_SOURCE_CHANGE) + return v4l2_event_subscribe(fh, sub, 0, &v4l2_event_src_ch_ops); + return -EINVAL; +} And v4l2-event.c is the right place to put it? And in the videodev2.h file: +#define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) +#define V4L2_EVENT_SRC_CH_FORMAT (1 << 1) + +struct v4l2_event_src_change { + __u32 changes; +}; + 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_src_change src_change; __u8 data[64]; } u; __u32 pending; Please correct me if my understanding is totally wrong here? Regards Arun > It's all more work, but it is critical to ensure that the application never > misses events. > > Regards, > > Hans > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/30/2014 12:38 PM, Arun Kumar K wrote: > Hi Hans, > > > On 04/22/14 18:22, Hans Verkuil wrote: >> On 04/21/2014 11:26 AM, 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; >>> +}; >>> + >>> 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]; >>> } u; >>> __u32 pending; >>> >> >> This needs to be done differently. The problem is that you really have multiple >> events that you want to subscribe to: one for each pad (or input: see the way >> VIDIOC_G/S_EDID maps pad to an input or output index when used with a video node, >> we have to support that for this event as well). >> >> What you want to do is similar to what is done for control events: there you can >> subscribe for a specific control and get notified when that control changes. >> >> The way that works in the event code is that the 'id' field in the v4l2_event >> struct contains that control ID, or, in this case, the pad/input/output index. >> > > As I am new to v4l2-events itself, I might have some noob questions. > I understood filling pad index into id field. But for video nodes, > the application is supposed to put v4l2_buf_type in the id field? No, a capture video node can have multiple inputs (those are enumerated with VIDIOC_ENUMINPUTS), and each input can generate a SOURCE_CHANGE event, even if it is not the currently active input. Hence the need of setting id to the input index. v4l2_buf_type makes no sense here. > >> In the application you will subscribe to the SOURCE_CHANGE event for a specific >> pad/input/output index. >> >> In other words, the pad_num field should be dropped and the id field used >> instead. >> >> Assuming we will also add a 'changes' field (see my reply to the other post >> on that topic), then you should also provide replace and merge ops (see >> v4l2-ctrls.c). That way it is sufficient to use just one element when calling >> v4l2_event_subscribe(): you will never loose information since if multiple >> events arrive before the application can dequeue them, the 'changes' information >> will be the ORed value of all those intermediate events. >> > > If I understand correctly, the implementation should go somewhat in the > following lines: > > +void v4l2_event_src_replace(struct v4l2_event *old, const struct > v4l2_event *new) > +{ > + u32 old_changes = old->u.src_change.changes; > + > + old->u.src_change = new->u.src_change; > + old->u.src_change.changes |= old_changes; > +} > + > +void v4l2_event_src_merge(const struct v4l2_event *old, struct > v4l2_event *new) > +{ > + new->u.src_change.changes |= old->u.src_change.changes; > +} > + > +const struct v4l2_subscribed_event_ops v4l2_event_src_ch_ops = { > + .replace = v4l2_event_src_replace, > + .merge = v4l2_event_src_merge, > +}; > + > +int v4l2_src_change_event_subscribe(struct v4l2_fh *fh, > + const struct v4l2_event_subscription *sub) > +{ > + if (sub->type == V4L2_EVENT_SOURCE_CHANGE) > + return v4l2_event_subscribe(fh, sub, 0, > &v4l2_event_src_ch_ops); > + return -EINVAL; > +} > > And v4l2-event.c is the right place to put it? Correct. Note that the replace and merge functions and v4l2_event_src_ch_ops can all be static, only v4l2_src_change_event_subscribe should be exported. > > > And in the videodev2.h file: > > +#define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) > +#define V4L2_EVENT_SRC_CH_FORMAT (1 << 1) > + > +struct v4l2_event_src_change { > + __u32 changes; > +}; > + > 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_src_change src_change; > __u8 data[64]; > } u; > __u32 pending; > > Please correct me if my understanding is totally wrong here? No, this is correct. I do think that CH_FORMAT is too vague. 'format' is such a general concept and I would prefer something that is more specific as to what has changed. Do you actually need that at the moment? Regards, Hans > > Regards > Arun > >> It's all more work, but it is critical to ensure that the application never >> misses events. >> >> Regards, >> >> Hans >> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Wed, Apr 30, 2014 at 8:03 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 04/30/2014 12:38 PM, Arun Kumar K wrote: >> Hi Hans, >> >> >> On 04/22/14 18:22, Hans Verkuil wrote: >>> On 04/21/2014 11:26 AM, 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; >>>> +}; >>>> + >>>> 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]; >>>> } u; >>>> __u32 pending; >>>> >>> >>> This needs to be done differently. The problem is that you really have multiple >>> events that you want to subscribe to: one for each pad (or input: see the way >>> VIDIOC_G/S_EDID maps pad to an input or output index when used with a video node, >>> we have to support that for this event as well). >>> >>> What you want to do is similar to what is done for control events: there you can >>> subscribe for a specific control and get notified when that control changes. >>> >>> The way that works in the event code is that the 'id' field in the v4l2_event >>> struct contains that control ID, or, in this case, the pad/input/output index. >>> >> >> As I am new to v4l2-events itself, I might have some noob questions. >> I understood filling pad index into id field. But for video nodes, >> the application is supposed to put v4l2_buf_type in the id field? > > No, a capture video node can have multiple inputs (those are enumerated with > VIDIOC_ENUMINPUTS), and each input can generate a SOURCE_CHANGE event, even if it > is not the currently active input. Hence the need of setting id to the input > index. v4l2_buf_type makes no sense here. > Ok. But in a simple m2m device like mfc, the userspace has to subscribe to the event say on the CAPTURE plane where buffers has to be re-allocated on resolution change. So in such a case, what should be put in id field? >> >>> In the application you will subscribe to the SOURCE_CHANGE event for a specific >>> pad/input/output index. >>> >>> In other words, the pad_num field should be dropped and the id field used >>> instead. >>> >>> Assuming we will also add a 'changes' field (see my reply to the other post >>> on that topic), then you should also provide replace and merge ops (see >>> v4l2-ctrls.c). That way it is sufficient to use just one element when calling >>> v4l2_event_subscribe(): you will never loose information since if multiple >>> events arrive before the application can dequeue them, the 'changes' information >>> will be the ORed value of all those intermediate events. >>> >> >> If I understand correctly, the implementation should go somewhat in the >> following lines: >> >> +void v4l2_event_src_replace(struct v4l2_event *old, const struct >> v4l2_event *new) >> +{ >> + u32 old_changes = old->u.src_change.changes; >> + >> + old->u.src_change = new->u.src_change; >> + old->u.src_change.changes |= old_changes; >> +} >> + >> +void v4l2_event_src_merge(const struct v4l2_event *old, struct >> v4l2_event *new) >> +{ >> + new->u.src_change.changes |= old->u.src_change.changes; >> +} >> + >> +const struct v4l2_subscribed_event_ops v4l2_event_src_ch_ops = { >> + .replace = v4l2_event_src_replace, >> + .merge = v4l2_event_src_merge, >> +}; >> + >> +int v4l2_src_change_event_subscribe(struct v4l2_fh *fh, >> + const struct v4l2_event_subscription *sub) >> +{ >> + if (sub->type == V4L2_EVENT_SOURCE_CHANGE) >> + return v4l2_event_subscribe(fh, sub, 0, >> &v4l2_event_src_ch_ops); >> + return -EINVAL; >> +} >> >> And v4l2-event.c is the right place to put it? > > Correct. Note that the replace and merge functions and v4l2_event_src_ch_ops can > all be static, only v4l2_src_change_event_subscribe should be exported. > Ok will do that. >> >> >> And in the videodev2.h file: >> >> +#define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) >> +#define V4L2_EVENT_SRC_CH_FORMAT (1 << 1) >> + >> +struct v4l2_event_src_change { >> + __u32 changes; >> +}; >> + >> 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_src_change src_change; >> __u8 data[64]; >> } u; >> __u32 pending; >> >> Please correct me if my understanding is totally wrong here? > > No, this is correct. > > I do think that CH_FORMAT is too vague. 'format' is such a general concept and > I would prefer something that is more specific as to what has changed. Do you > actually need that at the moment? > No I dont need it. I will keep only the CH_RESOLUTION for now. Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/05/2014 11:50 AM, Arun Kumar K wrote: > Hi Hans, > > On Wed, Apr 30, 2014 at 8:03 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 04/30/2014 12:38 PM, Arun Kumar K wrote: >>> Hi Hans, >>> >>> >>> On 04/22/14 18:22, Hans Verkuil wrote: >>>> On 04/21/2014 11:26 AM, 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; >>>>> +}; >>>>> + >>>>> 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]; >>>>> } u; >>>>> __u32 pending; >>>>> >>>> >>>> This needs to be done differently. The problem is that you really have multiple >>>> events that you want to subscribe to: one for each pad (or input: see the way >>>> VIDIOC_G/S_EDID maps pad to an input or output index when used with a video node, >>>> we have to support that for this event as well). >>>> >>>> What you want to do is similar to what is done for control events: there you can >>>> subscribe for a specific control and get notified when that control changes. >>>> >>>> The way that works in the event code is that the 'id' field in the v4l2_event >>>> struct contains that control ID, or, in this case, the pad/input/output index. >>>> >>> >>> As I am new to v4l2-events itself, I might have some noob questions. >>> I understood filling pad index into id field. But for video nodes, >>> the application is supposed to put v4l2_buf_type in the id field? >> >> No, a capture video node can have multiple inputs (those are enumerated with >> VIDIOC_ENUMINPUTS), and each input can generate a SOURCE_CHANGE event, even if it >> is not the currently active input. Hence the need of setting id to the input >> index. v4l2_buf_type makes no sense here. >> > > Ok. But in a simple m2m device like mfc, the userspace has to subscribe to > the event say on the CAPTURE plane where buffers has to be re-allocated on > resolution change. So in such a case, what should be put in id field? M2M devices will only have one input and one output, so the ID will be 0. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +}; + 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]; } u; __u32 pending;