[PATCH v2 0/2] Add resolution change event

[PATCH v2 0/2] Add resolution change event

[Arun Kumar K]

This patchset adds the resolution change event to the MFC
decoder. This will be used for triggering the runtime resolution
change.

Changes from v1
---------------
- Addressed review comments from Hans and Laurent
  https://patchwork.kernel.org/patch/4000951/

Pawel Osciak (2):
  v4l: Add resolution change event.
  [media] s5p-mfc: Add support for resolution change event

 .../DocBook/media/v4l/vidioc-subscribe-event.xml   |   16 ++++++++++++++++
 drivers/media/platform/s5p-mfc/s5p_mfc.c           |    6 ++++++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       |    2 ++
 include/uapi/linux/videodev2.h                     |    6 ++++++
 4 files changed, 30 insertions(+)

-- 
1.7.9.5

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

[Arun Kumar K]

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

[PATCH v2 2/2] [media] s5p-mfc: Add support for resolution change event

[Arun Kumar K]

From: Pawel Osciak <posciak@chromium.org>

When a resolution change point is reached, queue an event to signal the
userspace that a new set of buffers is required before decoding can
continue.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c     |    6 ++++++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c |    2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 4ee5a02..5152a23 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -308,6 +308,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
 	struct s5p_mfc_buf *src_buf;
 	unsigned long flags;
 	unsigned int res_change;
+	struct v4l2_event ev;
 
 	dst_frame_status = s5p_mfc_hw_call(dev->mfc_ops, get_dspl_status, dev)
 				& S5P_FIMV_DEC_STATUS_DECODING_STATUS_MASK;
@@ -339,6 +340,11 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
 		if (ctx->state == MFCINST_RES_CHANGE_FLUSH) {
 			s5p_mfc_handle_frame_all_extracted(ctx);
 			ctx->state = MFCINST_RES_CHANGE_END;
+
+			memset(&ev, 0, sizeof(struct v4l2_event));
+			ev.type = V4L2_EVENT_SOURCE_CHANGE;
+			v4l2_event_queue_fh(&ctx->fh, &ev);
+
 			goto leave_handle_frame;
 		} else {
 			s5p_mfc_handle_frame_all_extracted(ctx);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 4586186..069d354 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -851,6 +851,8 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
 	switch (sub->type) {
 	case V4L2_EVENT_EOS:
 		return v4l2_event_subscribe(fh, sub, 2, NULL);
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_event_subscribe(fh, sub, 2, NULL);
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

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

[Arun Kumar K]

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

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

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

[Arun Kumar K]

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
>

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

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

[Kamil Debski]

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,
-- 
Kamil Debski
Samsung R&D Institute Poland

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

[Kamil Debski]

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,
-- 
Kamil Debski
Samsung R&D Institute Poland

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

[Hans Verkuil]

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

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

[Hans Verkuil]

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

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

[Arun Kumar K]

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
> 

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

[Hans Verkuil]

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

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

[Arun Kumar K]

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

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

[Hans Verkuil]

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