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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Jan 2020 12:54:03 +0200
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Hans Verkuil <hverkuil@...all.nl>,
        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

Hi Hans,

On 1/9/20 10:57 AM, Hans Verkuil wrote:
> 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.

CH_COLOR_DEPTH implies format re-negotiation as well. In Venus case
8->10bit change will change the format of OPB buffers (now it is not
possible because of lack of v4l modifiers) and DPB buffers becomes
compressed raw buffers (to optimize bandwidth).

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

Looking into semantics of v4l events, CH_COLOR_DEPTH makes sense because
it describes what actually changed (similar to CH_RESOLUTION). I would
say that v4l2_event::type V4L2_EVENT_SOURCE_CHANGE implies format
re-negotiation and v4l2_event::src_change just informs userland what
exactly is changed.

I'll postpone this patch until we have clear vision what will be the
usage in user-space.

-- 
regards,
Stan