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: <814252fe-6fa9-595b-92b3-8ef8ef4c0187@xs4all.nl>
Date:   Wed, 15 Jan 2020 16:03:45 +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/10/20 11:54 AM, Stanimir Varbanov wrote:
> 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.
> 

My main problem regarding semantics is what should be done if you disconnect
and reconnect an HDMI (for example) connector. You get a V4L2_EVENT_SOURCE_CHANGE
when the new signal is detected, but should it just set V4L2_EVENT_SRC_CH_RESOLUTION
(as it does today), or also V4L2_EVENT_SRC_CH_COLOR_DEPTH?

In my view V4L2_EVENT_SRC_CH_COLOR_DEPTH only makes sense if the resolution
stays the same, but only the color depth changes.

BTW, one thing that will be added here as well in the future is a
V4L2_EVENT_SRC_CH_COLORIMETRY for when the colorspace etc. information changes,
but nothing else. In that case there is no need to renegotiate the format etc.,
it's just the interpretation of the video data that changes.

An alternative approach is to define a V4L2_EVENT_SRC_CH_ALL bit mask, OR-ing
all the V4L2_EVENT_SRC_CH_* defines, and change all drivers that use
V4L2_EVENT_SRC_CH_RESOLUTION at the moment to use V4L2_EVENT_SRC_CH_ALL instead,
and only drivers that detect that only one of these changes took place will
use a specific V4L2_EVENT_SRC_CH_ flag. This will be primarily codec drivers,
I believe.

There aren't that many of those, so this shouldn't be too difficult to do.

Perhaps this is the cleanest approach to this problem...

Regards,

	Hans