Message-ID: <8c97d866-1cab-0106-4ab3-3ca070945ef7@quicinc.com>
Date:   Thu, 24 Aug 2023 20:53:08 +0530
From:   Vikash Garodia <quic_vgarodia@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Stanimir Varbanov <stanimir.k.varbanov@...il.com>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <mchehab@...nel.org>,
        <hans.verkuil@...co.com>, <linux-kernel@...r.kernel.org>,
        <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <quic_dikshita@...cinc.com>
Subject: Re: [PATCH 00/33] Qualcomm video decoder/encoder driver

Hi Dmitry,

On 8/14/2023 8:30 PM, Dmitry Baryshkov wrote:
> Hi Stan,
> 
> On Mon, 14 Aug 2023 at 15:58, Stanimir Varbanov
> <stanimir.k.varbanov@...il.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 28.07.23 г. 17:01 ч., Dmitry Baryshkov wrote:
>>> On 28/07/2023 16:23, Vikash Garodia wrote:
>>>> This patch series introduces support for Qualcomm new video acceleration
>>>> hardware architecture, used for video stream decoding/encoding. This
>>>> driver
>>>> is based on new communication protocol between video hardware and
>>>> application
>>>> processor.
>>>>
>>>> This driver comes with below capabilities:
>>>> - V4L2 complaint video driver with M2M and STREAMING capability.
>>>> - Supports H264, H265, VP9 decoders.
>>>> - Supports H264, H265 encoders.
>>>
>>> Please describe, why is it impossible to support this hardware in the
>>> venus driver. We do not usually add new drivers for the new generations
>>> of the hardware, unless it is fully incompatible with the previous
>>> generations. Let me point you to camss or drm/msm drivers. They have
>>> successfully solved the issue of supporting multiple generations of the
>>> hardware in the same driver.
>>>
>>> Unless the "iris3" is completely different from all the previous
>>> generations, I strongly suggest spending time on restructuring existing
>>> venus driver and then adding support for the new hardware there instead
>>> of dumping out something completely new.
>>
>> AFAIK the major differences are HW IP and firmware interface (by
>> firmware interface I mean a protocol, API and API behavior). The
>> firmware and its interface has been re-written to align closely with the
>> current v4l2 specs for encoders/decoders state machines [1][2]. On the
>> other side current mainline Venus driver firmware is following interface
>> similar to OpenMAX.
>>
>> There are incompatibilities between both firmware interfaces which
>> cannot easily combined in a common driver. Even if there is a
>> possibility to do that it will lead us to a unreadable driver source
>> code and maintenance burden.
> 
> Thank you for your explanation!
> 
> If the hardware is more or less the same, then the existing venus
> driver should be refactored and split into hardware driver and the
> firmware interface. Then iris3 can come up as a second driver
> implementing support for new firmware interface but utilising common
> hardware-related code.

Its not just about supporting the new firmware interface because if that was the
case, it would have been a simple change. Its also about how the new firmware
interface affects the rest of the video sub-modules and state handling.
We incrementally evaluated whether putting the pieces one by one would make
sense but it doesn’t as every layer got affected and as a whole we decided to go
with this approach.
To elaborate more, let me try to put one of sequence which can provide info on
firmware interface and its handling across different video layers.

>> Vikash, could elaborate more on firmware interface differences.

Many new interfaces are added. Explained below one such video usecase of
handling dynamic resolution change (DRC) during drain. Illustrated a pseudo code
on how this will look if we fit this in venus driver.

- Client issues a STOP command. The command goes through state-wise command
handling, which also checks for cases like back to back drain. Vidc layer, which
handles common encoder and decoder functionality, routes it to decoder stop.
Decoder and driver layer then submits the command "HFI_CMD_DRAIN" to hardware
and moves the sub state to "MSM_VIDC_DRAIN".
- Now before drain is completed, there is a resolution change in one of the
frame queued before drain. Driver receives "HFI_CMD_SETTINGS_CHANGE" in hfi
response layer. The response goes through state check if received in intended
state and if good, changes the state to "MSM_VIDC_DRC | MSM_VIDC_INPUT_PAUSE".
Any further input processing remain paused at this point. The decoder layer then
parses all the bitstream parameters which were subscribed by the driver.
V4L2_EVENT_SOURCE_CHANGE event is raised to client.
- Hardware respond with HFI_INFO_HFI_FLAG_PSC_LAST to indicate LAST frame with
old sequence. Driver substate is added with "MSM_VIDC_DRC_LAST_BUFFER |
MSM_VIDC_OUTPUT_PAUSE". At this point, driver is in state, STREAMING and
substate - MSM_VIDC_DRAIN | MSM_VIDC_DRC | MSM_VIDC_INPUT_PAUSE |
MSM_VIDC_DRC_LAST_BUFFER | MSM_VIDC_OUTPUT_PAUSE. This is when both hardware as
well as driver is paused while waiting for further instructions.
- Client issues START cmd. Vidc layer routes it to decoder layer which checks
for sub states and then allocates/queues internal buffers. At this point, DRC
sequence is completed and substates "MSM_VIDC_DRC | MSM_VIDC_DRC_LAST_BUFFER"
are cleared and both input and output planes are resumed with HFI
"HFI_CMD_RESUME". substate "MSM_VIDC_INPUT_PAUSE" and "MSM_VIDC_OUTPUT_PAUSE" is
cleared as well. So driver is in streaming state with sub state as "MSM_VIDC_DRAIN"
- Hardware issues a response to "HFI_CMD_DRAIN". As part of handling of this,
driver adds to sub state "MSM_VIDC_INPUT_PAUSE". This is done to avoid any
further input processing. Once all the frames are processed, hardware raises HFI
"HFI_INFO_HFI_FLAG_DRAIN_LAST". After doing state check, further sub states
"MSM_VIDC_DRAIN_LAST_BUFFER | MSM_VIDC_OUTPUT_PAUSE" are added. So at this
point, the sub states are MSM_VIDC_DRAIN, MSM_VIDC_DRAIN_LAST_BUFFER,
MSM_VIDC_INPUT_PAUSE and MSM_VIDC_OUTPUT_PAUSE.
- Any pair calls like VIDIOC_STREAMON()/VIDIOC_STREAMOFF() on output or capture
queue, resets the substate to stream again.

If the same needs to be added in venus driver

- Client issues a STOP cmd to initiate drain. Decoder layer for stop handling
needs to be updated something like below //pseudo code  if (old interface)
   send dummy buffer
   change state to VENUS_DEC_STATE_DRAIN
   drain_active = true
 else
   statewise validation of STOP cmd
   state check for back to back drain
   issue HFI_CMD_DRAIN to hardware
   change sub state = MSM_VIDC_DRAIN

- DRC is issued by hardware
  if (old interface) //vdec and hfi response layer
   HFI_EVENT_SESSION_SEQUENCE_CHANGED with type
HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES
   changes state to VENUS_DEC_STATE_DRC
   next_buf_last = true
   flush (output)
   reconfig = true
   raise V4L2_EVENT_SOURCE_CHANGE event to client
  else
   HFI_CMD_SETTINGS_CHANGE // in hfi response layer
   state validation for intended state // in state handling layer
   sub state |= MSM_VIDC_DRC | MSM_VIDC_INPUT_PAUSE
   raise V4L2_EVENT_SOURCE_CHANGE event to client

- LAST flag handling
  if (old interface)
    No LAST flag HFI from hardware
    in qbuf_capture
      if (next_buf_last) associated LAST flag
  else
    handle HFI_INFO_HFI_FLAG_PSC_LAST //in response layer
    sub state |= MSM_VIDC_DRC_LAST_BUFFER | MSM_VIDC_OUTPUT_PAUSE

- Client issues START cmd
  if (old interface)
    does not handle VENUS_DEC_STATE_DRC state
  else
    sub state &= ~(MSM_VIDC_DRC | MSM_VIDC_DRC_LAST_BUFFER)
    allocates and queue internal buffer
    call HFI_CMD_RESUME for input and output
    sub state &= ~(MSM_VIDC_INPUT_PAUSE|MSM_VIDC_OUTPUT_PAUSE)

- Hardware response for HFI_CMD_DRAIN
  if (old interface)
    Nothing to do.
  else
    sub state |= MSM_VIDC_INPUT_PAUSE

- Handling for drain LAST flag
  if (old interface)
    receives dummy buffer with EOS
    converts to LAST and send to client
  else
    process HFI_INFO_HFI_FLAG_DRAIN_LAST
    sub state |= MSM_VIDC_DRAIN_LAST_BUFFER | MSM_VIDC_OUTPUT_PAUSE

There are many such complex sequences which would add to complexity if we try to
fit them into existing driver.

> Do we have any details on firmware versions that implement older
> (OpenMAX-like) interface vs versions implementing new (v4l2-like)
> interface?
> 
>> [1]
>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-decoder.html
>>
>> [2]
>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-encoder.html

Thanks,
Vikash

Powered by blists - more mailing lists