Date:   Wed, 15 Apr 2020 17:34:01 +0300
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/15/20 5:03 PM, Hans Verkuil wrote:
> 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?

I think disconnect -> connect should imply that everything is changed
i.e. source resolution, color depth and colorimetry. We cannot guess is
it the same HDMI source or not.

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

I can imagine a bitstream which changing resolution and color-depth at
the same time. And I guess h264|265 doesn't denied this to happen.

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

In case of colorimetry, maybe the buffer format will not change but
colorspace, quantization, ycbcr_encoding and transfer function will
change. And this imply that the userspace have to streamoff -> gfmt ->
do-some-action -> streamon ?

I have to figure out what is for example Android is doing when the
colorimentry is changed.
If someone knows, please shed some light on that subject.

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

I can do that as an RFC.

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

-- 
regards,
Stan

Powered by blists - more mailing lists