2016-11-14 11:36:56

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH v4 7/8] v4l: Add signal lock status to source change events

On 08/03/2016 08:03 PM, Steve Longerbeam wrote:
> Add a signal lock status change to the source changes bitmask.
> This indicates there was a signal lock or unlock event detected
> at the input of a video decoder.
>
> Signed-off-by: Steve Longerbeam <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
>
> ---
>
> v4:
> - converted to rst from DocBook
>
> v3: no changes
> v2: no changes
> ---
> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 9 +++++++++
> Documentation/media/videodev2.h.rst.exceptions | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> index 73c0d5b..7d8a053 100644
> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> @@ -564,6 +564,15 @@ call.
> an input. This can come from an input connector or from a video
> decoder.
>
> + - .. row 2
> +
> + - ``V4L2_EVENT_SRC_CH_LOCK_STATUS``
> +
> + - 0x0002
> +
> + - This event gets triggered when there is a signal lock or
> + unlock detected at the input of a video decoder.
> +
>
> Return Value
> ============
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index 9bb9a6c..f412cc8 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -453,6 +453,7 @@ replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags
> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags
>
> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags
> +replace define V4L2_EVENT_SRC_CH_LOCK_STATUS src-changes-flags
>
> replace define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ v4l2-event-motion-det
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 724f43e..08a153f 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2078,6 +2078,7 @@ struct v4l2_event_frame_sync {
> };
>
> #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0)
> +#define V4L2_EVENT_SRC_CH_LOCK_STATUS (1 << 1)
>
> struct v4l2_event_src_change {
> __u32 changes;
>

Quoting from an old (July) conversation about this:

>> > I'm not entirely sure I like this. Typically losing lock means that this event
>> > is triggered with the V4L2_EVENT_SRC_CH_RESOLUTION flag set, and userspace has
>> > to check the new timings etc., which will fail if there is no lock anymore.
>> >
>> > This information is also available through ENUMINPUT.
>> >
>> > I would need to know more about why you think this is needed, because I don't
>> > see what this adds.
>
> Hi Hans,
>
> At least on the ADV718x, a source resolution change (from an
> autodetected video
> standard change) and a signal lock status change are distinct events.
> For example
> there can be a temporary loss of input signal lock without a change in
> detected
> input video standard/resolution.

OK, but what can the application do with that event? If the glitch didn't
affect the video, then it is pointless.

If the lock is lost, then normally you loose video as well. If not, then
applications are not interested in the event.

So I am not convinced...

Regards,

Hans


2016-11-14 14:16:15

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [PATCH v4 7/8] v4l: Add signal lock status to source change events

> OK, but what can the application do with that event? If the glitch didn't
> affect the video, then it is pointless.
>
> If the lock is lost, then normally you loose video as well. If not, then
> applications are not interested in the event.

What about free running mode (where some decoders delivers blue or
black video with no signal present)? In that case it might still be
useful to inform the application so it can show a message that says
something like "No Signal".

Devin


--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

2016-11-14 19:19:15

by Steve Longerbeam

[permalink] [raw]
Subject: Re: [PATCH v4 7/8] v4l: Add signal lock status to source change events



On 11/14/2016 03:36 AM, Hans Verkuil wrote:
> On 08/03/2016 08:03 PM, Steve Longerbeam wrote:
>> Add a signal lock status change to the source changes bitmask.
>> This indicates there was a signal lock or unlock event detected
>> at the input of a video decoder.
>>
>> Signed-off-by: Steve Longerbeam <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>>
>> ---
>>
>> v4:
>> - converted to rst from DocBook
>>
>> v3: no changes
>> v2: no changes
>> ---
>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 9 +++++++++
>> Documentation/media/videodev2.h.rst.exceptions | 1 +
>> include/uapi/linux/videodev2.h | 1 +
>> 3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> index 73c0d5b..7d8a053 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> @@ -564,6 +564,15 @@ call.
>> an input. This can come from an input connector or from a video
>> decoder.
>>
>> + - .. row 2
>> +
>> + - ``V4L2_EVENT_SRC_CH_LOCK_STATUS``
>> +
>> + - 0x0002
>> +
>> + - This event gets triggered when there is a signal lock or
>> + unlock detected at the input of a video decoder.
>> +
>>
>> Return Value
>> ============
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>> index 9bb9a6c..f412cc8 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -453,6 +453,7 @@ replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags
>> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags
>>
>> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags
>> +replace define V4L2_EVENT_SRC_CH_LOCK_STATUS src-changes-flags
>>
>> replace define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ v4l2-event-motion-det
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 724f43e..08a153f 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2078,6 +2078,7 @@ struct v4l2_event_frame_sync {
>> };
>>
>> #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0)
>> +#define V4L2_EVENT_SRC_CH_LOCK_STATUS (1 << 1)
>>
>> struct v4l2_event_src_change {
>> __u32 changes;
>>
> Quoting from an old (July) conversation about this:
>
>>>> I'm not entirely sure I like this. Typically losing lock means that this event
>>>> is triggered with the V4L2_EVENT_SRC_CH_RESOLUTION flag set, and userspace has
>>>> to check the new timings etc., which will fail if there is no lock anymore.
>>>>
>>>> This information is also available through ENUMINPUT.
>>>>
>>>> I would need to know more about why you think this is needed, because I don't
>>>> see what this adds.
>> Hi Hans,
>>
>> At least on the ADV718x, a source resolution change (from an
>> autodetected video
>> standard change) and a signal lock status change are distinct events.
>> For example
>> there can be a temporary loss of input signal lock without a change in
>> detected
>> input video standard/resolution.
> OK, but what can the application do with that event? If the glitch didn't
> affect the video, then it is pointless.

Hi Hans, that's just it, for i.mx6 it does affect video. On i.mx6 a
temporary
loss of signal from the adv7180 often results in a "split image", or rolling
image from captured frame to the next, and the only way to recover
from this failure is to restart the pipeline (stream off -- stream on). So
the application needs to be informed of this temporary loss of signal
event in order to restart streaming.


>
> If the lock is lost, then normally you loose video as well. If not, then
> applications are not interested in the event.

Yes, the lost lock causes a temporary or permanent loss of video,
but with no other indications from the adv7180 (such as a detected
video standard change). And on i.mx6, the lost lock often (actually
usually) causes a split or rolling image.

Steve

2016-11-14 19:32:24

by Steve Longerbeam

[permalink] [raw]
Subject: Re: [PATCH v4 7/8] v4l: Add signal lock status to source change events



On 11/14/2016 11:19 AM, Steve Longerbeam wrote:
>
>
> On 11/14/2016 03:36 AM, Hans Verkuil wrote:
>> On 08/03/2016 08:03 PM, Steve Longerbeam wrote:
>>> Add a signal lock status change to the source changes bitmask.
>>> This indicates there was a signal lock or unlock event detected
>>> at the input of a video decoder.
>>>
>>> Signed-off-by: Steve Longerbeam <[email protected]>
>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>
>>> ---
>>>
>>> v4:
>>> - converted to rst from DocBook
>>>
>>> v3: no changes
>>> v2: no changes
>>> ---
>>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 9 +++++++++
>>> Documentation/media/videodev2.h.rst.exceptions | 1 +
>>> include/uapi/linux/videodev2.h | 1 +
>>> 3 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> index 73c0d5b..7d8a053 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> @@ -564,6 +564,15 @@ call.
>>> an input. This can come from an input connector or from a video
>>> decoder.
>>> + - .. row 2
>>> +
>>> + - ``V4L2_EVENT_SRC_CH_LOCK_STATUS``
>>> +
>>> + - 0x0002
>>> +
>>> + - This event gets triggered when there is a signal lock or
>>> + unlock detected at the input of a video decoder.
>>> +
>>> Return Value
>>> ============
>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions
>>> b/Documentation/media/videodev2.h.rst.exceptions
>>> index 9bb9a6c..f412cc8 100644
>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>> @@ -453,6 +453,7 @@ replace define V4L2_EVENT_CTRL_CH_FLAGS
>>> ctrl-changes-flags
>>> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags
>>> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags
>>> +replace define V4L2_EVENT_SRC_CH_LOCK_STATUS src-changes-flags
>>> replace define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ
>>> v4l2-event-motion-det
>>> diff --git a/include/uapi/linux/videodev2.h
>>> b/include/uapi/linux/videodev2.h
>>> index 724f43e..08a153f 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -2078,6 +2078,7 @@ struct v4l2_event_frame_sync {
>>> };
>>> #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0)
>>> +#define V4L2_EVENT_SRC_CH_LOCK_STATUS (1 << 1)
>>> struct v4l2_event_src_change {
>>> __u32 changes;
>>>
>> Quoting from an old (July) conversation about this:
>>
>>>>> I'm not entirely sure I like this. Typically losing lock means
>>>>> that this event
>>>>> is triggered with the V4L2_EVENT_SRC_CH_RESOLUTION flag set, and
>>>>> userspace has
>>>>> to check the new timings etc., which will fail if there is no lock
>>>>> anymore.
>>>>>
>>>>> This information is also available through ENUMINPUT.
>>>>>
>>>>> I would need to know more about why you think this is needed,
>>>>> because I don't
>>>>> see what this adds.
>>> Hi Hans,
>>>
>>> At least on the ADV718x, a source resolution change (from an
>>> autodetected video
>>> standard change) and a signal lock status change are distinct events.
>>> For example
>>> there can be a temporary loss of input signal lock without a change in
>>> detected
>>> input video standard/resolution.
>> OK, but what can the application do with that event? If the glitch
>> didn't
>> affect the video, then it is pointless.
>
> Hi Hans, that's just it, for i.mx6 it does affect video. On i.mx6 a
> temporary
> loss of signal from the adv7180 often results in a "split image", or
> rolling
> image from captured frame to the next, and the only way to recover
> from this failure is to restart the pipeline (stream off -- stream
> on). So
> the application needs to be informed of this temporary loss of signal
> event in order to restart streaming.
>
>
>>
>> If the lock is lost, then normally you loose video as well. If not, then
>> applications are not interested in the event.
>
> Yes, the lost lock causes a temporary or permanent loss of video,
> but with no other indications from the adv7180 (such as a detected
> video standard change). And on i.mx6, the lost lock often (actually
> usually) causes a split or rolling image.
>

Also it seems the sequence of actions from the application due to a
SRC_CH_LOCK_STATUS event would be in line with the action due to
SRC_CH_RESOLUTION. In other words for SRC_CH_RESOLUTION,
the app requests an update on the detected video standard via
QUERYSTD, and for SRC_CH_LOCK_STATUS, it requests an update
on input status (ENUMINPUTS).

Steve