linux-kernel - Re: [PATCH v4 04/12] v4l: Add source event change for bit-depth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fb27b5cc-0eef-a7b4-f45b-a3986b77c4c9@xs4all.nl>
Date:   Thu, 9 Jan 2020 09:57:39 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org
Cc:     Vikash Garodia <vgarodia@...eaurora.org>, dikshita@...eaurora.org
Subject: Re: [PATCH v4 04/12] v4l: Add source event change for bit-depth

On 1/9/20 8:41 AM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 1/8/20 6:09 PM, Hans Verkuil wrote:
>> On 1/6/20 4:49 PM, Stanimir Varbanov wrote:
>>> This event indicate that the source color bit-depth is changed
>>> during run-time. The client must get the new format and re-allocate
>>> buffers for it. This can usually happens with video decoder (encoders)
>>> when the bit-stream color bit-depth is changed from 8 to 10bits
>>> or vice versa.
>>>
>>> Acked-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>>> ---
>>>  Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++-
>>>  Documentation/media/videodev2.h.rst.exceptions  | 1 +
>>>  include/uapi/linux/videodev2.h                  | 1 +
>>>  3 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> index 42659a3d1705..fad853d440cf 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> @@ -402,7 +402,13 @@ call.
>>>  	that many Video Capture devices are not able to recover from a temporary
>>>  	loss of signal and so restarting streaming I/O is required in order for
>>>  	the hardware to synchronize to the video signal.
>>> -
>>> +    * - ``V4L2_EVENT_SRC_CH_COLOR_DEPTH``
>>> +      - 0x0002
>>> +      - This event gets triggered when color bit-depth change is detected
>>> +	from a video decoder. Applications will have to query the new pixel
>>> +	format and re-negotiate the queue. In most cases the streaming must be
>>> +	stopped and restarted (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>`
>>> +	followed by :ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`).
>>
>> I think this is too specific for decoders. Something similar to the
>> CH_RESOLUTION description would be more appropriate:
>>
>>       - This event gets triggered when a color bit-depth change (but not a
>> 	resolution change!) is detected	at an input. This can come from an
> 
> What you mean by "but not a resolution change" here? Resolution change
> and bit-depth change cannot occur on the same time, or something else.

What I was trying to say is that a resolution change implies a possible bit-depth
change as well, whereas V4L2_EVENT_SRC_CH_COLOR_DEPTH is only set if there is
a bit-depth change but no resolution change.

V4L2_EVENT_SRC_CH_RESOLUTION requires that userspace does a full resync to the
new format, CH_COLOR_DEPTH implies that only the bit depth changed.

Which actually makes me wonder: is there a difference between the two change flags
w.r.t. userspace behavior? If there is, then that should be carefully documented,
if there isn't, then is this new flag really needed?

Regards,

	Hans

> 
> I would say that for Venus (and probably others) on initialization time
> both could be changed on the same time, because we cannot predict the
> resolution and bit-depth before parsing bitstream headers.
> 
>> 	input connector or from a video decoder. Applications will have to query
>> 	the new pixel format and re-negotiate the queue.
>>
>> 	For stateful decoders follow the guidelines in :ref:`decoder`.
>> 	Video capture devices will in most cases have to stop and restart
>> 	streaming (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by
>> 	:ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`).
>>
>> And update dev-decoder.rst where needed with this new event flag.
>>
>> As to your question on irc: once I've acked this patch it can be merged
>> via a venus PR.
>>
>> Regards,
>>
>> 	Hans
>>
>>>  
>>>  Return Value
>>>  ============
>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>>> index cb6ccf91776e..209709114378 100644
>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>> @@ -490,6 +490,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_COLOR_DEPTH src-changes-flags
>>>  
>>>  replace define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ :c:type:`v4l2_event_motion_det`
>>>  
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 5f9357dcb060..1d349c9d57a7 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -2332,6 +2332,7 @@ struct v4l2_event_frame_sync {
>>>  };
>>>  
>>>  #define V4L2_EVENT_SRC_CH_RESOLUTION		(1 << 0)
>>> +#define V4L2_EVENT_SRC_CH_COLOR_DEPTH		(1 << 1)
>>>  
>>>  struct v4l2_event_src_change {
>>>  	__u32 changes;
>>>
>>
>