Re: [PATCH 00/30] TI Video Decoder driver upstreaming to v5.14-rc6 kernel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sidraya,

On 18/08/2021 16:10, sidraya.bj@xxxxxxxxxxxxxxxxxxx wrote:
> From: Sidraya <sidraya.bj@xxxxxxxxxxxxxxxxxxx>
> 
> This series of patches implements v4l2 based Decoder driver for H264,
> H265 and MJPEG decoding standards.This Driver is for D5520 H/W decoder on
> DRA8x SOC of J721e platform.
> This driver has been tested on v5.14-rc6 kernel for following
> decoding standards on v4l2 based Gstreamer 1.16 plug-in.
> 1. H264
> 2. H265
> 3. MJPEG
> 
> Note:
> Currently Driver uses list, map and queue custom data structure APIs
> implementation and IOMMU custom framework.We are working on replacing
> customised APIs with Linux kernel generic framework APIs.
> Meanwhile would like to address review comments from
> reviewers before merging to main media/platform subsystem.

OK, so I consider this an RFC series rather than an actual driver submission
and I'll mark it as such in patchwork.

First of all, patch 13/30 never made it to the linux-media mailinglist, so the
series as a whole will not apply. Can you repost 13/30? One possible reason why
13/30 might have been dropped is if it it a really large patch. You may have to
split it up in that case.

Did you run v4l2-compliance for this driver? Always compile v4l2-compliance
(part of https://git.linuxtv.org/v4l-utils.git/) from the git repo sources
to make sure you have most recent tests.

I need to see the output of 'v4l2-compliance' as part of the cover letter.
There shouldn't be any failures.

I see a lot of pointless #ifdef DEBUG_DECODER_DRIVER lines. Either just drop
the debug code or use dev_dbg/pr_debug. Ditto for VDEC_ASSERT(). This really
pollutes the code.

Can you provide a high-level description of the hardware? It seems like this
driver is a lot more complex than other decoder drivers, but it is not immediately
clear why that is. One reason might be that the hardware/firmware is a stateless
decoder, while the driver exposes a stateful decoder API. See the m2m interface
documentation for the differences between the two:

https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/dev-mem2mem.html

If that's the case, then the driver should really be a stateless driver, that
will likely make things much easier.

In any case, this driver clearly needs a lot more work.

Regards,

	Hans

> 
> Sidraya (30):
>   dt-bindings: Add binding for img,d5500-vxd for DRA8x
>   v4l: vxd-dec: Create mmu programming helper library
>   v4l: vxd-dec: Create vxd_dec Mem Manager helper library
>   v4l: vxd-dec: Add vxd helper library
>   v4l: vxd-dec: Add IMG VXD Video Decoder mem to mem drive
>   v4l: vxd-dec: Add hardware control modules
>   v4l: vxd-dec: Add vxd core module
>   v4l: vxd-dec: Add translation control modules
>   v4l: vxd-dec: Add idgen api modules
>   v4l: vxd-dec: Add utility modules
>   v4l: vxd-dec: Add TALMMU module
>   v4l: vxd-dec: Add VDEC MMU wrapper
>   v4l: vxd-dec: Add Bistream Preparser (BSPP) module
>   v4l: vxd-dec: Add common headers
>   v4l: vxd-dec: Add firmware interface headers
>   v4l: vxd-dec: Add pool api modules
>   v4l: vxd-dec: This patch implements resource manage component
>   v4l: vxd-dec: This patch implements pixel processing library
>   v4l:vxd-dec:vdecdd utility library
>   v4l:vxd-dec:Decoder resource component
>   v4l:vxd-dec:Decoder Core Component
>   v4l:vxd-dec:vdecdd headers added
>   v4l:vxd-dec:Decoder Component
>   v4l:vxd-dec: Add resource manager
>   v4l: videodev2: Add 10bit definitions for NV12 and NV16 color formats
>   media: Kconfig: Add Video decoder kconfig and Makefile entries
>   media: platform: vxd: Kconfig: Add Video decoder Kconfig and Makefile
>   IMG DEC V4L2 Interface function implementations
>   arm64: dts: dra82: Add v4l2 vxd_dec device node
>   ARM64: ti_sdk_arm64_release_defconfig: Enable d5520 video decoder
>     driver
> 
>  .../bindings/media/img,d5520-vxd.yaml         |   52 +
>  MAINTAINERS                                   |  114 +
>  arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     |    9 +
>  .../configs/ti_sdk_arm64_release_defconfig    | 7407 +++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c          |    2 +
>  drivers/staging/media/Kconfig                 |    2 +
>  drivers/staging/media/Makefile                |    1 +
>  drivers/staging/media/vxd/common/addr_alloc.c |  499 ++
>  drivers/staging/media/vxd/common/addr_alloc.h |  238 +
>  drivers/staging/media/vxd/common/dq.c         |  248 +
>  drivers/staging/media/vxd/common/dq.h         |   36 +
>  drivers/staging/media/vxd/common/hash.c       |  481 ++
>  drivers/staging/media/vxd/common/hash.h       |   86 +
>  drivers/staging/media/vxd/common/idgen_api.c  |  449 +
>  drivers/staging/media/vxd/common/idgen_api.h  |   59 +
>  drivers/staging/media/vxd/common/img_errors.h |  104 +
>  drivers/staging/media/vxd/common/img_mem.h    |   43 +
>  .../staging/media/vxd/common/img_mem_man.c    | 1124 +++
>  .../staging/media/vxd/common/img_mem_man.h    |  231 +
>  .../media/vxd/common/img_mem_unified.c        |  276 +
>  drivers/staging/media/vxd/common/imgmmu.c     |  782 ++
>  drivers/staging/media/vxd/common/imgmmu.h     |  180 +
>  drivers/staging/media/vxd/common/lst.c        |  119 +
>  drivers/staging/media/vxd/common/lst.h        |   37 +
>  drivers/staging/media/vxd/common/pool.c       |  228 +
>  drivers/staging/media/vxd/common/pool.h       |   66 +
>  drivers/staging/media/vxd/common/pool_api.c   |  709 ++
>  drivers/staging/media/vxd/common/pool_api.h   |  113 +
>  drivers/staging/media/vxd/common/ra.c         |  972 +++
>  drivers/staging/media/vxd/common/ra.h         |  200 +
>  drivers/staging/media/vxd/common/resource.c   |  576 ++
>  drivers/staging/media/vxd/common/resource.h   |   66 +
>  drivers/staging/media/vxd/common/rman_api.c   |  620 ++
>  drivers/staging/media/vxd/common/rman_api.h   |   66 +
>  drivers/staging/media/vxd/common/talmmu_api.c |  753 ++
>  drivers/staging/media/vxd/common/talmmu_api.h |  246 +
>  drivers/staging/media/vxd/common/vid_buf.h    |   42 +
>  drivers/staging/media/vxd/common/work_queue.c |  188 +
>  drivers/staging/media/vxd/common/work_queue.h |   66 +
>  drivers/staging/media/vxd/decoder/Kconfig     |   13 +
>  drivers/staging/media/vxd/decoder/Makefile    |  129 +
>  drivers/staging/media/vxd/decoder/bspp.c      | 2479 ++++++
>  drivers/staging/media/vxd/decoder/bspp.h      |  363 +
>  drivers/staging/media/vxd/decoder/bspp_int.h  |  514 ++
>  drivers/staging/media/vxd/decoder/core.c      | 3656 ++++++++
>  drivers/staging/media/vxd/decoder/core.h      |   72 +
>  .../staging/media/vxd/decoder/dec_resources.c |  554 ++
>  .../staging/media/vxd/decoder/dec_resources.h |   46 +
>  drivers/staging/media/vxd/decoder/decoder.c   | 4622 ++++++++++
>  drivers/staging/media/vxd/decoder/decoder.h   |  375 +
>  .../staging/media/vxd/decoder/fw_interface.h  |  818 ++
>  drivers/staging/media/vxd/decoder/h264_idx.h  |   60 +
>  .../media/vxd/decoder/h264_secure_parser.c    | 3051 +++++++
>  .../media/vxd/decoder/h264_secure_parser.h    |  278 +
>  drivers/staging/media/vxd/decoder/h264_vlc.h  |  604 ++
>  .../staging/media/vxd/decoder/h264fw_data.h   |  652 ++
>  .../media/vxd/decoder/h264fw_data_shared.h    |  759 ++
>  .../media/vxd/decoder/hevc_secure_parser.c    | 2895 +++++++
>  .../media/vxd/decoder/hevc_secure_parser.h    |  455 +
>  .../staging/media/vxd/decoder/hevcfw_data.h   |  472 ++
>  .../media/vxd/decoder/hevcfw_data_shared.h    |  767 ++
>  .../staging/media/vxd/decoder/hw_control.c    | 1211 +++
>  .../staging/media/vxd/decoder/hw_control.h    |  144 +
>  .../media/vxd/decoder/img_dec_common.h        |  278 +
>  .../media/vxd/decoder/img_msvdx_cmds.h        |  279 +
>  .../media/vxd/decoder/img_msvdx_core_regs.h   |   22 +
>  .../media/vxd/decoder/img_msvdx_vdmc_regs.h   |   26 +
>  .../media/vxd/decoder/img_msvdx_vec_regs.h    |   60 +
>  .../staging/media/vxd/decoder/img_pixfmts.h   |  195 +
>  .../media/vxd/decoder/img_profiles_levels.h   |   33 +
>  .../media/vxd/decoder/img_pvdec_core_regs.h   |   60 +
>  .../media/vxd/decoder/img_pvdec_pixel_regs.h  |   35 +
>  .../media/vxd/decoder/img_pvdec_test_regs.h   |   39 +
>  .../media/vxd/decoder/img_vdec_fw_msg.h       |  192 +
>  .../vxd/decoder/img_video_bus4_mmu_regs.h     |  120 +
>  .../media/vxd/decoder/jpeg_secure_parser.c    |  645 ++
>  .../media/vxd/decoder/jpeg_secure_parser.h    |   37 +
>  .../staging/media/vxd/decoder/jpegfw_data.h   |   83 +
>  .../media/vxd/decoder/jpegfw_data_shared.h    |   84 +
>  drivers/staging/media/vxd/decoder/mem_io.h    |   42 +
>  drivers/staging/media/vxd/decoder/mmu_defs.h  |   42 +
>  drivers/staging/media/vxd/decoder/pixel_api.c |  895 ++
>  drivers/staging/media/vxd/decoder/pixel_api.h |  152 +
>  .../media/vxd/decoder/pvdec_entropy_regs.h    |   33 +
>  drivers/staging/media/vxd/decoder/pvdec_int.h |   82 +
>  .../media/vxd/decoder/pvdec_vec_be_regs.h     |   35 +
>  drivers/staging/media/vxd/decoder/reg_io2.h   |   74 +
>  .../staging/media/vxd/decoder/scaler_setup.h  |   59 +
>  drivers/staging/media/vxd/decoder/swsr.c      | 1657 ++++
>  drivers/staging/media/vxd/decoder/swsr.h      |  278 +
>  .../media/vxd/decoder/translation_api.c       | 1725 ++++
>  .../media/vxd/decoder/translation_api.h       |   42 +
>  drivers/staging/media/vxd/decoder/vdec_defs.h |  548 ++
>  .../media/vxd/decoder/vdec_mmu_wrapper.c      |  829 ++
>  .../media/vxd/decoder/vdec_mmu_wrapper.h      |  174 +
>  .../staging/media/vxd/decoder/vdecdd_defs.h   |  446 +
>  .../staging/media/vxd/decoder/vdecdd_utils.c  |   95 +
>  .../staging/media/vxd/decoder/vdecdd_utils.h  |   93 +
>  .../media/vxd/decoder/vdecdd_utils_buf.c      |  897 ++
>  .../staging/media/vxd/decoder/vdecfw_share.h  |   36 +
>  .../staging/media/vxd/decoder/vdecfw_shared.h |  893 ++
>  drivers/staging/media/vxd/decoder/vxd_core.c  | 1683 ++++
>  drivers/staging/media/vxd/decoder/vxd_dec.c   |  185 +
>  drivers/staging/media/vxd/decoder/vxd_dec.h   |  477 ++
>  drivers/staging/media/vxd/decoder/vxd_ext.h   |   74 +
>  drivers/staging/media/vxd/decoder/vxd_int.c   | 1137 +++
>  drivers/staging/media/vxd/decoder/vxd_int.h   |  128 +
>  .../staging/media/vxd/decoder/vxd_mmu_defs.h  |   30 +
>  drivers/staging/media/vxd/decoder/vxd_props.h |   80 +
>  drivers/staging/media/vxd/decoder/vxd_pvdec.c | 1745 ++++
>  .../media/vxd/decoder/vxd_pvdec_priv.h        |  126 +
>  .../media/vxd/decoder/vxd_pvdec_regs.h        |  779 ++
>  drivers/staging/media/vxd/decoder/vxd_v4l2.c  | 2129 +++++
>  include/uapi/linux/videodev2.h                |    2 +
>  114 files changed, 62369 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/img,d5520-vxd.yaml
>  create mode 100644 arch/arm64/configs/ti_sdk_arm64_release_defconfig
>  create mode 100644 drivers/staging/media/vxd/common/addr_alloc.c
>  create mode 100644 drivers/staging/media/vxd/common/addr_alloc.h
>  create mode 100644 drivers/staging/media/vxd/common/dq.c
>  create mode 100644 drivers/staging/media/vxd/common/dq.h
>  create mode 100644 drivers/staging/media/vxd/common/hash.c
>  create mode 100644 drivers/staging/media/vxd/common/hash.h
>  create mode 100644 drivers/staging/media/vxd/common/idgen_api.c
>  create mode 100644 drivers/staging/media/vxd/common/idgen_api.h
>  create mode 100644 drivers/staging/media/vxd/common/img_errors.h
>  create mode 100644 drivers/staging/media/vxd/common/img_mem.h
>  create mode 100644 drivers/staging/media/vxd/common/img_mem_man.c
>  create mode 100644 drivers/staging/media/vxd/common/img_mem_man.h
>  create mode 100644 drivers/staging/media/vxd/common/img_mem_unified.c
>  create mode 100644 drivers/staging/media/vxd/common/imgmmu.c
>  create mode 100644 drivers/staging/media/vxd/common/imgmmu.h
>  create mode 100644 drivers/staging/media/vxd/common/lst.c
>  create mode 100644 drivers/staging/media/vxd/common/lst.h
>  create mode 100644 drivers/staging/media/vxd/common/pool.c
>  create mode 100644 drivers/staging/media/vxd/common/pool.h
>  create mode 100644 drivers/staging/media/vxd/common/pool_api.c
>  create mode 100644 drivers/staging/media/vxd/common/pool_api.h
>  create mode 100644 drivers/staging/media/vxd/common/ra.c
>  create mode 100644 drivers/staging/media/vxd/common/ra.h
>  create mode 100644 drivers/staging/media/vxd/common/resource.c
>  create mode 100644 drivers/staging/media/vxd/common/resource.h
>  create mode 100644 drivers/staging/media/vxd/common/rman_api.c
>  create mode 100644 drivers/staging/media/vxd/common/rman_api.h
>  create mode 100644 drivers/staging/media/vxd/common/talmmu_api.c
>  create mode 100644 drivers/staging/media/vxd/common/talmmu_api.h
>  create mode 100644 drivers/staging/media/vxd/common/vid_buf.h
>  create mode 100644 drivers/staging/media/vxd/common/work_queue.c
>  create mode 100644 drivers/staging/media/vxd/common/work_queue.h
>  create mode 100644 drivers/staging/media/vxd/decoder/Kconfig
>  create mode 100644 drivers/staging/media/vxd/decoder/Makefile
>  create mode 100644 drivers/staging/media/vxd/decoder/bspp.c
>  create mode 100644 drivers/staging/media/vxd/decoder/bspp.h
>  create mode 100644 drivers/staging/media/vxd/decoder/bspp_int.h
>  create mode 100644 drivers/staging/media/vxd/decoder/core.c
>  create mode 100644 drivers/staging/media/vxd/decoder/core.h
>  create mode 100644 drivers/staging/media/vxd/decoder/dec_resources.c
>  create mode 100644 drivers/staging/media/vxd/decoder/dec_resources.h
>  create mode 100644 drivers/staging/media/vxd/decoder/decoder.c
>  create mode 100644 drivers/staging/media/vxd/decoder/decoder.h
>  create mode 100644 drivers/staging/media/vxd/decoder/fw_interface.h
>  create mode 100644 drivers/staging/media/vxd/decoder/h264_idx.h
>  create mode 100644 drivers/staging/media/vxd/decoder/h264_secure_parser.c
>  create mode 100644 drivers/staging/media/vxd/decoder/h264_secure_parser.h
>  create mode 100644 drivers/staging/media/vxd/decoder/h264_vlc.h
>  create mode 100644 drivers/staging/media/vxd/decoder/h264fw_data.h
>  create mode 100644 drivers/staging/media/vxd/decoder/h264fw_data_shared.h
>  create mode 100644 drivers/staging/media/vxd/decoder/hevc_secure_parser.c
>  create mode 100644 drivers/staging/media/vxd/decoder/hevc_secure_parser.h
>  create mode 100644 drivers/staging/media/vxd/decoder/hevcfw_data.h
>  create mode 100644 drivers/staging/media/vxd/decoder/hevcfw_data_shared.h
>  create mode 100644 drivers/staging/media/vxd/decoder/hw_control.c
>  create mode 100644 drivers/staging/media/vxd/decoder/hw_control.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_dec_common.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_msvdx_cmds.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_msvdx_core_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_msvdx_vdmc_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_msvdx_vec_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_pixfmts.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_profiles_levels.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_pvdec_core_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_pvdec_pixel_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_pvdec_test_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_vdec_fw_msg.h
>  create mode 100644 drivers/staging/media/vxd/decoder/img_video_bus4_mmu_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/jpeg_secure_parser.c
>  create mode 100644 drivers/staging/media/vxd/decoder/jpeg_secure_parser.h
>  create mode 100644 drivers/staging/media/vxd/decoder/jpegfw_data.h
>  create mode 100644 drivers/staging/media/vxd/decoder/jpegfw_data_shared.h
>  create mode 100644 drivers/staging/media/vxd/decoder/mem_io.h
>  create mode 100644 drivers/staging/media/vxd/decoder/mmu_defs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/pixel_api.c
>  create mode 100644 drivers/staging/media/vxd/decoder/pixel_api.h
>  create mode 100644 drivers/staging/media/vxd/decoder/pvdec_entropy_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/pvdec_int.h
>  create mode 100644 drivers/staging/media/vxd/decoder/pvdec_vec_be_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/reg_io2.h
>  create mode 100644 drivers/staging/media/vxd/decoder/scaler_setup.h
>  create mode 100644 drivers/staging/media/vxd/decoder/swsr.c
>  create mode 100644 drivers/staging/media/vxd/decoder/swsr.h
>  create mode 100644 drivers/staging/media/vxd/decoder/translation_api.c
>  create mode 100644 drivers/staging/media/vxd/decoder/translation_api.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdec_defs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdec_mmu_wrapper.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vdec_mmu_wrapper.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecdd_defs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecdd_utils.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecdd_utils.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecdd_utils_buf.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecfw_share.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vdecfw_shared.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_core.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_dec.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_dec.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_ext.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_int.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_int.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_mmu_defs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_props.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_pvdec.c
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_pvdec_priv.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_pvdec_regs.h
>  create mode 100644 drivers/staging/media/vxd/decoder/vxd_v4l2.c
> 





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux