| Message ID | 1690550624-14642-1-git-send-email-quic_vgarodia@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Qualcomm video decoder/encoder driver | expand |
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. > > This driver comes with below features: > - Centralized resource and memory management. > - Centralized management of core and instance states. > - Defines platform specific capabilities and features. As a results, it provides > a single point of control to enable/disable a given feature depending on > specific platform capabilities. > - Handles hardware interdependent configurations. For a given configuration from > client, the driver checks for hardware dependent configuration/s and updates > the same. > - Handles multiple complex video scenarios involving state transitions - DRC, > Drain, Seek, back to back DRC, DRC during Drain sequence, DRC during Seek > sequence. > - Introduces a flexible way for driver to subscribe for any property with > hardware. Hardware would inform driver with those subscribed property with any > change in value. > - Introduces performance (clock and bus) model based on new hardware > architecture. > - Introduces multi thread safe design to handle communication between client and > hardware. > - Adapts encoder quality improvements available in new hardware architecture. > - Implements asynchronous communication with hardware to achieve better > experience in low latency usecases. > - Supports multi stage hardware architecture for encode/decode. > - Output and capture planes are controlled independently. Thereby providing a > way to reconfigure individual plane. > - Hardware packetization layer supports synchronization between configuration > packet and data packet. > - Introduces a flexibility to receive a hardware response for a given command > packet. > - Native hardware support of LAST flag which is mandatory to align with port > reconfiguration and DRAIN sequence as per V4L guidelines. > - Native hardware support for drain sequence. > > I think that the driver is in good shape for mainline kernel, and I hope the > review comments will help to improve it, so please do review, and make comments. No bindings, no driver. Please post start the series from the bindings. > > Dikshita Agarwal (17): > iris: vidc: add core functions > iris: add vidc wrapper file > iris: vidc: add vb2 ops > iris: vidc: add helpers for memory management > iris: vidc: add helper functions for resource management > iris: vidc: add helper functions for power management > iris: add helpers for media format > iris: vidc: add PIL functionality for video firmware > iris: platform: add platform files > iris: platform: sm8550: add capability file for sm8550 > iris: variant: add helper functions for register handling > iris: variant: iris3: add iris3 specific ops > iris: variant: iris3: add helpers for buffer size calculations > iris: variant: iris3: add helper for bus and clock calculation > iris: variant: iris: implement the logic to compute bus bandwidth > iris: variant: iris3: implement logic to compute clock frequency > iris: enable building of iris video driver > > Vikash Garodia (16): > MAINTAINERS: Add Qualcomm Iris video accelerator driver > iris: vidc: add v4l2 wrapper file > iris: vidc: define video core and instance context > iris: iris: add video encoder files > iris: vidc: add video decoder files > iris: vidc: add control files > iris: vidc: add helper functions > iris: vidc: add helpers for state management > iris: add vidc buffer files > iris: vidc: define various structures and enum > iris: vidc: hfi: add Host Firmware Interface (HFI) > iris: vidc: hfi: add Host Firmware Interface (HFI) response handling > iris: vidc: hfi: add helpers for handling shared queues > iris: vidc: hfi: Add packetization layer > iris: vidc: hfi: defines HFI properties and enums > iris: vidc: add debug files > > MAINTAINERS | 10 + > drivers/media/platform/qcom/Kconfig | 1 + > drivers/media/platform/qcom/Makefile | 1 + > drivers/media/platform/qcom/iris/Kconfig | 15 + > drivers/media/platform/qcom/iris/Makefile | 46 + > .../iris/platform/common/inc/msm_vidc_platform.h | 305 ++ > .../iris/platform/common/src/msm_vidc_platform.c | 2499 ++++++++++++ > .../iris/platform/sm8550/inc/msm_vidc_sm8550.h | 14 + > .../iris/platform/sm8550/src/msm_vidc_sm8550.c | 1727 ++++++++ > .../iris/variant/common/inc/msm_vidc_variant.h | 22 + > .../iris/variant/common/src/msm_vidc_variant.c | 163 + > .../qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h | 1481 +++++++ > .../iris/variant/iris3/inc/msm_vidc_buffer_iris3.h | 19 + > .../qcom/iris/variant/iris3/inc/msm_vidc_iris3.h | 15 + > .../iris/variant/iris3/inc/msm_vidc_power_iris3.h | 17 + > .../iris/variant/iris3/inc/perf_static_model.h | 229 ++ > .../iris/variant/iris3/src/msm_vidc_buffer_iris3.c | 595 +++ > .../iris/variant/iris3/src/msm_vidc_bus_iris3.c | 884 ++++ > .../iris/variant/iris3/src/msm_vidc_clock_iris3.c | 627 +++ > .../qcom/iris/variant/iris3/src/msm_vidc_iris3.c | 954 +++++ > .../iris/variant/iris3/src/msm_vidc_power_iris3.c | 345 ++ > .../media/platform/qcom/iris/vidc/inc/firmware.h | 18 + > .../platform/qcom/iris/vidc/inc/hfi_command.h | 190 + > .../media/platform/qcom/iris/vidc/inc/hfi_packet.h | 52 + > .../platform/qcom/iris/vidc/inc/hfi_property.h | 666 +++ > .../platform/qcom/iris/vidc/inc/msm_media_info.h | 599 +++ > .../media/platform/qcom/iris/vidc/inc/msm_vdec.h | 40 + > .../media/platform/qcom/iris/vidc/inc/msm_venc.h | 34 + > .../media/platform/qcom/iris/vidc/inc/msm_vidc.h | 60 + > .../platform/qcom/iris/vidc/inc/msm_vidc_buffer.h | 32 + > .../platform/qcom/iris/vidc/inc/msm_vidc_control.h | 26 + > .../platform/qcom/iris/vidc/inc/msm_vidc_core.h | 165 + > .../platform/qcom/iris/vidc/inc/msm_vidc_debug.h | 186 + > .../platform/qcom/iris/vidc/inc/msm_vidc_driver.h | 352 ++ > .../platform/qcom/iris/vidc/inc/msm_vidc_inst.h | 207 + > .../qcom/iris/vidc/inc/msm_vidc_internal.h | 787 ++++ > .../platform/qcom/iris/vidc/inc/msm_vidc_memory.h | 83 + > .../platform/qcom/iris/vidc/inc/msm_vidc_power.h | 94 + > .../platform/qcom/iris/vidc/inc/msm_vidc_state.h | 102 + > .../platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h | 77 + > .../platform/qcom/iris/vidc/inc/msm_vidc_vb2.h | 39 + > .../media/platform/qcom/iris/vidc/inc/resources.h | 259 ++ > .../media/platform/qcom/iris/vidc/inc/venus_hfi.h | 66 + > .../platform/qcom/iris/vidc/inc/venus_hfi_queue.h | 89 + > .../qcom/iris/vidc/inc/venus_hfi_response.h | 26 + > .../media/platform/qcom/iris/vidc/src/firmware.c | 294 ++ > .../media/platform/qcom/iris/vidc/src/hfi_packet.c | 657 +++ > .../media/platform/qcom/iris/vidc/src/msm_vdec.c | 2091 ++++++++++ > .../media/platform/qcom/iris/vidc/src/msm_venc.c | 1484 +++++++ > .../media/platform/qcom/iris/vidc/src/msm_vidc.c | 841 ++++ > .../platform/qcom/iris/vidc/src/msm_vidc_buffer.c | 290 ++ > .../platform/qcom/iris/vidc/src/msm_vidc_control.c | 824 ++++ > .../platform/qcom/iris/vidc/src/msm_vidc_debug.c | 581 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_driver.c | 4276 ++++++++++++++++++++ > .../platform/qcom/iris/vidc/src/msm_vidc_memory.c | 448 ++ > .../platform/qcom/iris/vidc/src/msm_vidc_power.c | 560 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_probe.c | 660 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_state.c | 1607 ++++++++ > .../platform/qcom/iris/vidc/src/msm_vidc_v4l2.c | 953 +++++ > .../platform/qcom/iris/vidc/src/msm_vidc_vb2.c | 605 +++ > .../media/platform/qcom/iris/vidc/src/resources.c | 1321 ++++++ > .../media/platform/qcom/iris/vidc/src/venus_hfi.c | 1503 +++++++ > .../platform/qcom/iris/vidc/src/venus_hfi_queue.c | 537 +++ > .../qcom/iris/vidc/src/venus_hfi_response.c | 1607 ++++++++ > 64 files changed, 35357 insertions(+) > create mode 100644 drivers/media/platform/qcom/iris/Kconfig > create mode 100644 drivers/media/platform/qcom/iris/Makefile > create mode 100644 drivers/media/platform/qcom/iris/platform/common/inc/msm_vidc_platform.h > create mode 100644 drivers/media/platform/qcom/iris/platform/common/src/msm_vidc_platform.c > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/inc/msm_vidc_sm8550.h > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/src/msm_vidc_sm8550.c > create mode 100644 drivers/media/platform/qcom/iris/variant/common/inc/msm_vidc_variant.h > create mode 100644 drivers/media/platform/qcom/iris/variant/common/src/msm_vidc_variant.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_buffer_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_power_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/perf_static_model.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_buffer_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_bus_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_clock_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_power_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/firmware.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_command.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_packet.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_property.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_media_info.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vdec.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_venc.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_buffer.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_control.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_core.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_driver.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_inst.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_internal.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_memory.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_power.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_state.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_vb2.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/resources.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_queue.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_response.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/firmware.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/hfi_packet.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vdec.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_venc.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_buffer.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_control.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_debug.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_driver.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_memory.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_power.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_probe.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_state.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_vb2.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/resources.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_queue.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_response.c >
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. > > This driver comes with below features: > - Centralized resource and memory management. > - Centralized management of core and instance states. > - Defines platform specific capabilities and features. As a results, it provides > a single point of control to enable/disable a given feature depending on > specific platform capabilities. > - Handles hardware interdependent configurations. For a given configuration from > client, the driver checks for hardware dependent configuration/s and updates > the same. > - Handles multiple complex video scenarios involving state transitions - DRC, > Drain, Seek, back to back DRC, DRC during Drain sequence, DRC during Seek > sequence. > - Introduces a flexible way for driver to subscribe for any property with > hardware. Hardware would inform driver with those subscribed property with any > change in value. > - Introduces performance (clock and bus) model based on new hardware > architecture. > - Introduces multi thread safe design to handle communication between client and > hardware. > - Adapts encoder quality improvements available in new hardware architecture. > - Implements asynchronous communication with hardware to achieve better > experience in low latency usecases. > - Supports multi stage hardware architecture for encode/decode. > - Output and capture planes are controlled independently. Thereby providing a > way to reconfigure individual plane. > - Hardware packetization layer supports synchronization between configuration > packet and data packet. > - Introduces a flexibility to receive a hardware response for a given command > packet. > - Native hardware support of LAST flag which is mandatory to align with port > reconfiguration and DRAIN sequence as per V4L guidelines. > - Native hardware support for drain sequence. > > I think that the driver is in good shape for mainline kernel, and I hope the > review comments will help to improve it, so please do review, and make comments. > > Dikshita Agarwal (17): > iris: vidc: add core functions > iris: add vidc wrapper file > iris: vidc: add vb2 ops > iris: vidc: add helpers for memory management > iris: vidc: add helper functions for resource management > iris: vidc: add helper functions for power management > iris: add helpers for media format > iris: vidc: add PIL functionality for video firmware > iris: platform: add platform files > iris: platform: sm8550: add capability file for sm8550 > iris: variant: add helper functions for register handling > iris: variant: iris3: add iris3 specific ops > iris: variant: iris3: add helpers for buffer size calculations > iris: variant: iris3: add helper for bus and clock calculation > iris: variant: iris: implement the logic to compute bus bandwidth > iris: variant: iris3: implement logic to compute clock frequency > iris: enable building of iris video driver > > Vikash Garodia (16): > MAINTAINERS: Add Qualcomm Iris video accelerator driver > iris: vidc: add v4l2 wrapper file > iris: vidc: define video core and instance context > iris: iris: add video encoder files > iris: vidc: add video decoder files > iris: vidc: add control files > iris: vidc: add helper functions > iris: vidc: add helpers for state management > iris: add vidc buffer files > iris: vidc: define various structures and enum > iris: vidc: hfi: add Host Firmware Interface (HFI) > iris: vidc: hfi: add Host Firmware Interface (HFI) response handling > iris: vidc: hfi: add helpers for handling shared queues > iris: vidc: hfi: Add packetization layer > iris: vidc: hfi: defines HFI properties and enums > iris: vidc: add debug files > > MAINTAINERS | 10 + > drivers/media/platform/qcom/Kconfig | 1 + > drivers/media/platform/qcom/Makefile | 1 + > drivers/media/platform/qcom/iris/Kconfig | 15 + > drivers/media/platform/qcom/iris/Makefile | 46 + > .../iris/platform/common/inc/msm_vidc_platform.h | 305 ++ > .../iris/platform/common/src/msm_vidc_platform.c | 2499 ++++++++++++ > .../iris/platform/sm8550/inc/msm_vidc_sm8550.h | 14 + > .../iris/platform/sm8550/src/msm_vidc_sm8550.c | 1727 ++++++++ > .../iris/variant/common/inc/msm_vidc_variant.h | 22 + > .../iris/variant/common/src/msm_vidc_variant.c | 163 + > .../qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h | 1481 +++++++ > .../iris/variant/iris3/inc/msm_vidc_buffer_iris3.h | 19 + > .../qcom/iris/variant/iris3/inc/msm_vidc_iris3.h | 15 + > .../iris/variant/iris3/inc/msm_vidc_power_iris3.h | 17 + > .../iris/variant/iris3/inc/perf_static_model.h | 229 ++ > .../iris/variant/iris3/src/msm_vidc_buffer_iris3.c | 595 +++ > .../iris/variant/iris3/src/msm_vidc_bus_iris3.c | 884 ++++ > .../iris/variant/iris3/src/msm_vidc_clock_iris3.c | 627 +++ > .../qcom/iris/variant/iris3/src/msm_vidc_iris3.c | 954 +++++ > .../iris/variant/iris3/src/msm_vidc_power_iris3.c | 345 ++ > .../media/platform/qcom/iris/vidc/inc/firmware.h | 18 + > .../platform/qcom/iris/vidc/inc/hfi_command.h | 190 + > .../media/platform/qcom/iris/vidc/inc/hfi_packet.h | 52 + > .../platform/qcom/iris/vidc/inc/hfi_property.h | 666 +++ > .../platform/qcom/iris/vidc/inc/msm_media_info.h | 599 +++ > .../media/platform/qcom/iris/vidc/inc/msm_vdec.h | 40 + > .../media/platform/qcom/iris/vidc/inc/msm_venc.h | 34 + > .../media/platform/qcom/iris/vidc/inc/msm_vidc.h | 60 + > .../platform/qcom/iris/vidc/inc/msm_vidc_buffer.h | 32 + > .../platform/qcom/iris/vidc/inc/msm_vidc_control.h | 26 + > .../platform/qcom/iris/vidc/inc/msm_vidc_core.h | 165 + > .../platform/qcom/iris/vidc/inc/msm_vidc_debug.h | 186 + > .../platform/qcom/iris/vidc/inc/msm_vidc_driver.h | 352 ++ > .../platform/qcom/iris/vidc/inc/msm_vidc_inst.h | 207 + > .../qcom/iris/vidc/inc/msm_vidc_internal.h | 787 ++++ > .../platform/qcom/iris/vidc/inc/msm_vidc_memory.h | 83 + > .../platform/qcom/iris/vidc/inc/msm_vidc_power.h | 94 + > .../platform/qcom/iris/vidc/inc/msm_vidc_state.h | 102 + > .../platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h | 77 + > .../platform/qcom/iris/vidc/inc/msm_vidc_vb2.h | 39 + > .../media/platform/qcom/iris/vidc/inc/resources.h | 259 ++ > .../media/platform/qcom/iris/vidc/inc/venus_hfi.h | 66 + > .../platform/qcom/iris/vidc/inc/venus_hfi_queue.h | 89 + > .../qcom/iris/vidc/inc/venus_hfi_response.h | 26 + > .../media/platform/qcom/iris/vidc/src/firmware.c | 294 ++ > .../media/platform/qcom/iris/vidc/src/hfi_packet.c | 657 +++ > .../media/platform/qcom/iris/vidc/src/msm_vdec.c | 2091 ++++++++++ > .../media/platform/qcom/iris/vidc/src/msm_venc.c | 1484 +++++++ > .../media/platform/qcom/iris/vidc/src/msm_vidc.c | 841 ++++ > .../platform/qcom/iris/vidc/src/msm_vidc_buffer.c | 290 ++ > .../platform/qcom/iris/vidc/src/msm_vidc_control.c | 824 ++++ > .../platform/qcom/iris/vidc/src/msm_vidc_debug.c | 581 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_driver.c | 4276 ++++++++++++++++++++ > .../platform/qcom/iris/vidc/src/msm_vidc_memory.c | 448 ++ > .../platform/qcom/iris/vidc/src/msm_vidc_power.c | 560 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_probe.c | 660 +++ > .../platform/qcom/iris/vidc/src/msm_vidc_state.c | 1607 ++++++++ > .../platform/qcom/iris/vidc/src/msm_vidc_v4l2.c | 953 +++++ > .../platform/qcom/iris/vidc/src/msm_vidc_vb2.c | 605 +++ > .../media/platform/qcom/iris/vidc/src/resources.c | 1321 ++++++ > .../media/platform/qcom/iris/vidc/src/venus_hfi.c | 1503 +++++++ > .../platform/qcom/iris/vidc/src/venus_hfi_queue.c | 537 +++ > .../qcom/iris/vidc/src/venus_hfi_response.c | 1607 ++++++++ > 64 files changed, 35357 insertions(+) > create mode 100644 drivers/media/platform/qcom/iris/Kconfig > create mode 100644 drivers/media/platform/qcom/iris/Makefile > create mode 100644 drivers/media/platform/qcom/iris/platform/common/inc/msm_vidc_platform.h > create mode 100644 drivers/media/platform/qcom/iris/platform/common/src/msm_vidc_platform.c > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/inc/msm_vidc_sm8550.h > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/src/msm_vidc_sm8550.c > create mode 100644 drivers/media/platform/qcom/iris/variant/common/inc/msm_vidc_variant.h > create mode 100644 drivers/media/platform/qcom/iris/variant/common/src/msm_vidc_variant.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_buffer_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_power_iris3.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/perf_static_model.h > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_buffer_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_bus_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_clock_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_power_iris3.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/firmware.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_command.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_packet.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_property.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_media_info.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vdec.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_venc.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_buffer.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_control.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_core.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_driver.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_inst.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_internal.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_memory.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_power.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_state.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_vb2.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/resources.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_queue.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_response.h > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/firmware.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/hfi_packet.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vdec.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_venc.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_buffer.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_control.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_debug.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_driver.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_memory.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_power.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_probe.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_state.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_vb2.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/resources.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_queue.c > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_response.c >
On 28/07/2023 14: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. I think my main comment and question for this driver is; what specifically about the sm8550 requires an entirely new driver for the venus block ? We have a re-implementation of the HFI as an example. What are the specifics of the technical barriers, if any, that preclude adding or extending the existing venus codebase to enable the new silicon ? --- bod
Hi Dmitry, Le vendredi 28 juillet 2023 à 16:32 +0300, Dmitry Baryshkov a écrit : > 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. > > > > This driver comes with below features: > > - Centralized resource and memory management. > > - Centralized management of core and instance states. > > - Defines platform specific capabilities and features. As a results, it provides > > a single point of control to enable/disable a given feature depending on > > specific platform capabilities. > > - Handles hardware interdependent configurations. For a given configuration from > > client, the driver checks for hardware dependent configuration/s and updates > > the same. > > - Handles multiple complex video scenarios involving state transitions - DRC, > > Drain, Seek, back to back DRC, DRC during Drain sequence, DRC during Seek > > sequence. > > - Introduces a flexible way for driver to subscribe for any property with > > hardware. Hardware would inform driver with those subscribed property with any > > change in value. > > - Introduces performance (clock and bus) model based on new hardware > > architecture. > > - Introduces multi thread safe design to handle communication between client and > > hardware. > > - Adapts encoder quality improvements available in new hardware architecture. > > - Implements asynchronous communication with hardware to achieve better > > experience in low latency usecases. > > - Supports multi stage hardware architecture for encode/decode. > > - Output and capture planes are controlled independently. Thereby providing a > > way to reconfigure individual plane. > > - Hardware packetization layer supports synchronization between configuration > > packet and data packet. > > - Introduces a flexibility to receive a hardware response for a given command > > packet. > > - Native hardware support of LAST flag which is mandatory to align with port > > reconfiguration and DRAIN sequence as per V4L guidelines. > > - Native hardware support for drain sequence. > > > > I think that the driver is in good shape for mainline kernel, and I hope the > > review comments will help to improve it, so please do review, and make comments. > > No bindings, no driver. Please post start the series from the bindings. In your next iteration, make sure to include full v4l2-compliance report in your cover letter since we cannot assume maintainers. In addition to this, we now ask for fluster scores for each of your supported decoders. We expect the results to have no timeout, and ideally the error/failure explained (aka unsupported resolution, profile, subsampling, bit depth, etc.). Note that inter-resolution change is not possible with V4L2 today, so no need to explain why these VP9 tests fails. Fluster supports V4L2 decoding through GStreamer (gst-launch + video4linux plugin) and FFMPEG at the moment. It will run through ITU conformance vectors for HEVC and H.264, and run through libvpx and and chromium test vectors for VP9. https://github.com/fluendo/fluster regards, Nicolas > > > > > Dikshita Agarwal (17): > > iris: vidc: add core functions > > iris: add vidc wrapper file > > iris: vidc: add vb2 ops > > iris: vidc: add helpers for memory management > > iris: vidc: add helper functions for resource management > > iris: vidc: add helper functions for power management > > iris: add helpers for media format > > iris: vidc: add PIL functionality for video firmware > > iris: platform: add platform files > > iris: platform: sm8550: add capability file for sm8550 > > iris: variant: add helper functions for register handling > > iris: variant: iris3: add iris3 specific ops > > iris: variant: iris3: add helpers for buffer size calculations > > iris: variant: iris3: add helper for bus and clock calculation > > iris: variant: iris: implement the logic to compute bus bandwidth > > iris: variant: iris3: implement logic to compute clock frequency > > iris: enable building of iris video driver > > > > Vikash Garodia (16): > > MAINTAINERS: Add Qualcomm Iris video accelerator driver > > iris: vidc: add v4l2 wrapper file > > iris: vidc: define video core and instance context > > iris: iris: add video encoder files > > iris: vidc: add video decoder files > > iris: vidc: add control files > > iris: vidc: add helper functions > > iris: vidc: add helpers for state management > > iris: add vidc buffer files > > iris: vidc: define various structures and enum > > iris: vidc: hfi: add Host Firmware Interface (HFI) > > iris: vidc: hfi: add Host Firmware Interface (HFI) response handling > > iris: vidc: hfi: add helpers for handling shared queues > > iris: vidc: hfi: Add packetization layer > > iris: vidc: hfi: defines HFI properties and enums > > iris: vidc: add debug files > > > > MAINTAINERS | 10 + > > drivers/media/platform/qcom/Kconfig | 1 + > > drivers/media/platform/qcom/Makefile | 1 + > > drivers/media/platform/qcom/iris/Kconfig | 15 + > > drivers/media/platform/qcom/iris/Makefile | 46 + > > .../iris/platform/common/inc/msm_vidc_platform.h | 305 ++ > > .../iris/platform/common/src/msm_vidc_platform.c | 2499 ++++++++++++ > > .../iris/platform/sm8550/inc/msm_vidc_sm8550.h | 14 + > > .../iris/platform/sm8550/src/msm_vidc_sm8550.c | 1727 ++++++++ > > .../iris/variant/common/inc/msm_vidc_variant.h | 22 + > > .../iris/variant/common/src/msm_vidc_variant.c | 163 + > > .../qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h | 1481 +++++++ > > .../iris/variant/iris3/inc/msm_vidc_buffer_iris3.h | 19 + > > .../qcom/iris/variant/iris3/inc/msm_vidc_iris3.h | 15 + > > .../iris/variant/iris3/inc/msm_vidc_power_iris3.h | 17 + > > .../iris/variant/iris3/inc/perf_static_model.h | 229 ++ > > .../iris/variant/iris3/src/msm_vidc_buffer_iris3.c | 595 +++ > > .../iris/variant/iris3/src/msm_vidc_bus_iris3.c | 884 ++++ > > .../iris/variant/iris3/src/msm_vidc_clock_iris3.c | 627 +++ > > .../qcom/iris/variant/iris3/src/msm_vidc_iris3.c | 954 +++++ > > .../iris/variant/iris3/src/msm_vidc_power_iris3.c | 345 ++ > > .../media/platform/qcom/iris/vidc/inc/firmware.h | 18 + > > .../platform/qcom/iris/vidc/inc/hfi_command.h | 190 + > > .../media/platform/qcom/iris/vidc/inc/hfi_packet.h | 52 + > > .../platform/qcom/iris/vidc/inc/hfi_property.h | 666 +++ > > .../platform/qcom/iris/vidc/inc/msm_media_info.h | 599 +++ > > .../media/platform/qcom/iris/vidc/inc/msm_vdec.h | 40 + > > .../media/platform/qcom/iris/vidc/inc/msm_venc.h | 34 + > > .../media/platform/qcom/iris/vidc/inc/msm_vidc.h | 60 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_buffer.h | 32 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_control.h | 26 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_core.h | 165 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_debug.h | 186 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_driver.h | 352 ++ > > .../platform/qcom/iris/vidc/inc/msm_vidc_inst.h | 207 + > > .../qcom/iris/vidc/inc/msm_vidc_internal.h | 787 ++++ > > .../platform/qcom/iris/vidc/inc/msm_vidc_memory.h | 83 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_power.h | 94 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_state.h | 102 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h | 77 + > > .../platform/qcom/iris/vidc/inc/msm_vidc_vb2.h | 39 + > > .../media/platform/qcom/iris/vidc/inc/resources.h | 259 ++ > > .../media/platform/qcom/iris/vidc/inc/venus_hfi.h | 66 + > > .../platform/qcom/iris/vidc/inc/venus_hfi_queue.h | 89 + > > .../qcom/iris/vidc/inc/venus_hfi_response.h | 26 + > > .../media/platform/qcom/iris/vidc/src/firmware.c | 294 ++ > > .../media/platform/qcom/iris/vidc/src/hfi_packet.c | 657 +++ > > .../media/platform/qcom/iris/vidc/src/msm_vdec.c | 2091 ++++++++++ > > .../media/platform/qcom/iris/vidc/src/msm_venc.c | 1484 +++++++ > > .../media/platform/qcom/iris/vidc/src/msm_vidc.c | 841 ++++ > > .../platform/qcom/iris/vidc/src/msm_vidc_buffer.c | 290 ++ > > .../platform/qcom/iris/vidc/src/msm_vidc_control.c | 824 ++++ > > .../platform/qcom/iris/vidc/src/msm_vidc_debug.c | 581 +++ > > .../platform/qcom/iris/vidc/src/msm_vidc_driver.c | 4276 ++++++++++++++++++++ > > .../platform/qcom/iris/vidc/src/msm_vidc_memory.c | 448 ++ > > .../platform/qcom/iris/vidc/src/msm_vidc_power.c | 560 +++ > > .../platform/qcom/iris/vidc/src/msm_vidc_probe.c | 660 +++ > > .../platform/qcom/iris/vidc/src/msm_vidc_state.c | 1607 ++++++++ > > .../platform/qcom/iris/vidc/src/msm_vidc_v4l2.c | 953 +++++ > > .../platform/qcom/iris/vidc/src/msm_vidc_vb2.c | 605 +++ > > .../media/platform/qcom/iris/vidc/src/resources.c | 1321 ++++++ > > .../media/platform/qcom/iris/vidc/src/venus_hfi.c | 1503 +++++++ > > .../platform/qcom/iris/vidc/src/venus_hfi_queue.c | 537 +++ > > .../qcom/iris/vidc/src/venus_hfi_response.c | 1607 ++++++++ > > 64 files changed, 35357 insertions(+) > > create mode 100644 drivers/media/platform/qcom/iris/Kconfig > > create mode 100644 drivers/media/platform/qcom/iris/Makefile > > create mode 100644 drivers/media/platform/qcom/iris/platform/common/inc/msm_vidc_platform.h > > create mode 100644 drivers/media/platform/qcom/iris/platform/common/src/msm_vidc_platform.c > > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/inc/msm_vidc_sm8550.h > > create mode 100644 drivers/media/platform/qcom/iris/platform/sm8550/src/msm_vidc_sm8550.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/common/inc/msm_vidc_variant.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/common/src/msm_vidc_variant.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/hfi_buffer_iris3.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_buffer_iris3.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_iris3.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/msm_vidc_power_iris3.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/inc/perf_static_model.h > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_buffer_iris3.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_bus_iris3.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_clock_iris3.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_iris3.c > > create mode 100644 drivers/media/platform/qcom/iris/variant/iris3/src/msm_vidc_power_iris3.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/firmware.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_command.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_packet.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/hfi_property.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_media_info.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vdec.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_venc.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_buffer.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_control.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_core.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_driver.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_inst.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_internal.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_memory.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_power.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_state.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_vb2.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/resources.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_queue.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/venus_hfi_response.h > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/firmware.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/hfi_packet.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vdec.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_venc.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_buffer.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_control.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_debug.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_driver.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_memory.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_power.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_probe.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_state.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_vb2.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/resources.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_queue.c > > create mode 100644 drivers/media/platform/qcom/iris/vidc/src/venus_hfi_response.c > > >
On 28.07.2023 15:23, Vikash Garodia wrote: > This implements common helper functions for v4l2 to vidc and > vice versa conversion for different enums. > Add helpers for state checks, buffer management, locks etc. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > --- [...] > + > +#define is_odd(val) ((val) % 2 == 1) > +#define in_range(val, min, max) (((min) <= (val)) && ((val) <= (max))) > +#define COUNT_BITS(a, out) { \ hweight.* functions? [...] > + > +const char *cap_name(enum msm_vidc_inst_capability_type cap_id) > +{ > + const char *name = "UNKNOWN CAP"; Perhaps it'd be worth to include the unknown cap id here > + > + if (cap_id >= ARRAY_SIZE(cap_name_arr)) > + goto exit; > + > + name = cap_name_arr[cap_id]; > + > +exit: > + return name; > +} [...] > + > +const char *buf_name(enum msm_vidc_buffer_type type) > +{ > + const char *name = "UNKNOWN BUF"; Similarly here > + > + if (type >= ARRAY_SIZE(buf_type_name_arr)) > + goto exit; > + > + name = buf_type_name_arr[type]; > + > +exit: > + return name; > +} [...] > +const char *v4l2_type_name(u32 port) > +{ > + switch (port) { switch-case seems a bit excessive here. > + case INPUT_MPLANE: return "INPUT"; > + case OUTPUT_MPLANE: return "OUTPUT"; > + } > + > + return "UNKNOWN"; > +} [...] There's some more stuff I'd comment on, but 4500 lines in a single patch is way too much to logically follow. Couple more style suggestions: - use Reverse-Christmas-tree sorting for variable declarations - some oneliner functions could possibly become preprocessor macros - when printing giant debug messages, you may want to use loops - make sure your indentation is in order, 100 chars per line is totally fine - generally inline magic hex values are discouraged, but if they're necessary, the hex should be lowercase Konrad
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. Vikash, could elaborate more on firmware interface differences. [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
Hi Stan, On Mon, 14 Aug 2023 at 15:58, Stanimir Varbanov <stanimir.k.varbanov@gmail.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. > Vikash, could elaborate more on firmware interface differences. 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
On 7/28/2023 11:11 PM, Konrad Dybcio wrote: > On 28.07.2023 15:23, Vikash Garodia wrote: >> This implements common helper functions for v4l2 to vidc and >> vice versa conversion for different enums. >> Add helpers for state checks, buffer management, locks etc. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> --- > [...] > >> + >> +#define is_odd(val) ((val) % 2 == 1) >> +#define in_range(val, min, max) (((min) <= (val)) && ((val) <= (max))) >> +#define COUNT_BITS(a, out) { \ > hweight.* functions? > > [...] > sure, will replace with hweight. >> + >> +const char *cap_name(enum msm_vidc_inst_capability_type cap_id) >> +{ >> + const char *name = "UNKNOWN CAP"; > Perhaps it'd be worth to include the unknown cap id here > could you please elaborate more on this. >> + >> + if (cap_id >= ARRAY_SIZE(cap_name_arr)) >> + goto exit; >> + >> + name = cap_name_arr[cap_id]; >> + >> +exit: >> + return name; >> +} > [...] > >> + >> +const char *buf_name(enum msm_vidc_buffer_type type) >> +{ >> + const char *name = "UNKNOWN BUF"; > Similarly here > could you please elaborate more on this. >> + >> + if (type >= ARRAY_SIZE(buf_type_name_arr)) >> + goto exit; >> + >> + name = buf_type_name_arr[type]; >> + >> +exit: >> + return name; >> +} > [...] > >> +const char *v4l2_type_name(u32 port) >> +{ >> + switch (port) { > switch-case seems a bit excessive here. > >> + case INPUT_MPLANE: return "INPUT"; >> + case OUTPUT_MPLANE: return "OUTPUT"; >> + } >> + >> + return "UNKNOWN"; >> +} that's right, will fix in next version > [...] > > There's some more stuff I'd comment on, but 4500 lines in a single patch > is way too much to logically follow. > > Couple more style suggestions: > - use Reverse-Christmas-tree sorting for variable declarations > - some oneliner functions could possibly become preprocessor macros > - when printing giant debug messages, you may want to use loops > - make sure your indentation is in order, 100 chars per line is > totally fine > - generally inline magic hex values are discouraged, but if they're > necessary, the hex should be lowercase Nice suggestions! will take care of these comments in next version. Thanks, Dikshita > > Konrad
On 14.08.2023 21:15, Dikshita Agarwal wrote: > > > On 7/28/2023 11:11 PM, Konrad Dybcio wrote: >> On 28.07.2023 15:23, Vikash Garodia wrote: >>> This implements common helper functions for v4l2 to vidc and >>> vice versa conversion for different enums. >>> Add helpers for state checks, buffer management, locks etc. >>> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>> --- >> [...] >> >>> + >>> +#define is_odd(val) ((val) % 2 == 1) >>> +#define in_range(val, min, max) (((min) <= (val)) && ((val) <= (max))) >>> +#define COUNT_BITS(a, out) { \ >> hweight.* functions? >> >> [...] >> > sure, will replace with hweight. >>> + >>> +const char *cap_name(enum msm_vidc_inst_capability_type cap_id) >>> +{ >>> + const char *name = "UNKNOWN CAP"; >> Perhaps it'd be worth to include the unknown cap id here >> > could you please elaborate more on this. >>> + >>> + if (cap_id >= ARRAY_SIZE(cap_name_arr)) >>> + goto exit; >>> + >>> + name = cap_name_arr[cap_id]; >>> + >>> +exit: >>> + return name; >>> +} >> [...] >> >>> + >>> +const char *buf_name(enum msm_vidc_buffer_type type) >>> +{ >>> + const char *name = "UNKNOWN BUF"; >> Similarly here >> > could you please elaborate more on this. Something like "UNKNOWN BUF (0x15)" instead of just "UNKNOWN BUF" would help us better understand whether the driver or the hardware is missing something. Konrad
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@gmail.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