On 01/08/2018 21:33, Maxime Jourdan wrote: > This is a Request for Comments for the amlogic (meson) video decoder driver. > It is written around the V4L2 M2M framework without using the Request > API as there are a hardware bitstream parser and firmwares. > > It features decoding for: > - MPEG 1/2/4, H.263, H.264, MJPEG, HEVC 8-bit (partial) > > Even though they are supported in hardware, it doesn't leverage support for: > - HEVC 10-bit, VP9, VC1 (all those are in TODOs) > > The output is multiplanar NV12 (V4L2_PIX_FMT_NV12M). > Supported SoCs are: GXBB (S905), GXL (S905X/W/D), GXM (S912) > It was tested primarily with FFmpeg, GStreamer and kodi. > > The file hierarchy can be boiled down to: > > | codec_h264.c > | codec_mjpeg.c > | codec_mpeg4.c > | vdec_1.c -->| codec_mpeg12.c > vdec.c -->| vdec_hevc.c -->| codec_hevc.c > | esparser.c > > The V4L2 code is handled mostly in vdec.c. > Each VDEC and CODEC unit is accessed via ops structs to facilitate the code. > > The arrangement between vdecs and codecs can be seen in vdec_platform.c > This file also declares things like pixfmts, min/max buffers and firmware paths > for each SoC. > > Specific questions about the code: > > - While I do use the platform's general clks and resets tied to the vdec in > a nice way (dts + clock/reset controller with clk/reset frameworks), > there are some subclocks and resets that I use in the driver by writing > directly to registers. e.g: > > - writel_relaxed((1<<7) | (1<<6), core->dos_base + DOS_SW_RESET0); This seems to be internal resets > - writel_relaxed(0x3ff, core->dos_base + DOS_GCLK_EN0); This seems to be internal gates If it's in the VDEC/DO registers space, you can keep this internaly, no need to use the clk or reset framework. Neil > > and a few other instances where that happens. > > Is it okay to not create specific controllers for those ? The main issue is > the lack of documentation so I don't know which resets/clocks are impacted by > those writes. > The only thing I'm certain of is that they only apply to the vdec/esparser. > > - I tend to call vdec_* functions from the codec handlers. > > For instance, codec_h264 will call vdec_dst_buf_done_idx to DONE > a capture buffer. vdec_dst_buf_done_idx is as such a public symbol. > > Should I use an ops struct for those instead, so that the codec handlers > don't depend directly on the vdec general code ? > > - Naming: my public symbols either start with vdec_* or esparser_* > > Should I change that to something meson/amlogic specific ? > > - I have a _lot_ of writel_relaxed calls. > > Can I leave them be or is there a nicer way to do it ? > > - Since the decoder is single instance, I only allow one _open at a time. > > However the v4l2 compliance suite complains about this. > How should I safely make it single instance ? Not allowing multiple start_streaming ? > > - I am getting these 2 fails, but unsure what they are about: > > Buffer ioctls: > fail: ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(428): node->node2 == NULL > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > fail: ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(571): q.has_expbuf(node) > test VIDIOC_EXPBUF: FAIL > > > > And of course, I will gladly accept any kind of other feedback you would have. > > Thanks! > > > Maxime Jourdan (4): > media: meson: add v4l2 m2m video decoder driver > ARM64: dts: meson-gx: add vdec entry > ARM64: dts: meson: add vdec entries > dt-bindings: media: add Amlogic Meson Video Decoder Bindings > > .../bindings/media/amlogic,meson-vdec.txt | 60 + > arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 14 + > arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 8 + > arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 8 + > arch/arm64/boot/dts/amlogic/meson-gxm.dtsi | 4 + > drivers/media/platform/Kconfig | 10 + > drivers/media/platform/meson/Makefile | 1 + > drivers/media/platform/meson/vdec/Makefile | 7 + > drivers/media/platform/meson/vdec/canvas.c | 69 + > drivers/media/platform/meson/vdec/canvas.h | 42 + > .../media/platform/meson/vdec/codec_h264.c | 376 +++++ > .../media/platform/meson/vdec/codec_h264.h | 13 + > .../media/platform/meson/vdec/codec_helpers.c | 45 + > .../media/platform/meson/vdec/codec_helpers.h | 8 + > .../media/platform/meson/vdec/codec_hevc.c | 1383 +++++++++++++++++ > .../media/platform/meson/vdec/codec_hevc.h | 13 + > .../media/platform/meson/vdec/codec_mjpeg.c | 203 +++ > .../media/platform/meson/vdec/codec_mjpeg.h | 13 + > .../media/platform/meson/vdec/codec_mpeg12.c | 183 +++ > .../media/platform/meson/vdec/codec_mpeg12.h | 13 + > .../media/platform/meson/vdec/codec_mpeg4.c | 213 +++ > .../media/platform/meson/vdec/codec_mpeg4.h | 13 + > drivers/media/platform/meson/vdec/esparser.c | 320 ++++ > drivers/media/platform/meson/vdec/esparser.h | 16 + > drivers/media/platform/meson/vdec/hevc_regs.h | 742 +++++++++ > drivers/media/platform/meson/vdec/vdec.c | 1009 ++++++++++++ > drivers/media/platform/meson/vdec/vdec.h | 152 ++ > drivers/media/platform/meson/vdec/vdec_1.c | 266 ++++ > drivers/media/platform/meson/vdec/vdec_1.h | 13 + > drivers/media/platform/meson/vdec/vdec_hevc.c | 188 +++ > drivers/media/platform/meson/vdec/vdec_hevc.h | 22 + > .../media/platform/meson/vdec/vdec_platform.c | 273 ++++ > .../media/platform/meson/vdec/vdec_platform.h | 29 + > 33 files changed, 5729 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/amlogic,meson-vdec.txt > create mode 100644 drivers/media/platform/meson/vdec/Makefile > create mode 100644 drivers/media/platform/meson/vdec/canvas.c > create mode 100644 drivers/media/platform/meson/vdec/canvas.h > create mode 100644 drivers/media/platform/meson/vdec/codec_h264.c > create mode 100644 drivers/media/platform/meson/vdec/codec_h264.h > create mode 100644 drivers/media/platform/meson/vdec/codec_helpers.c > create mode 100644 drivers/media/platform/meson/vdec/codec_helpers.h > create mode 100644 drivers/media/platform/meson/vdec/codec_hevc.c > create mode 100644 drivers/media/platform/meson/vdec/codec_hevc.h > create mode 100644 drivers/media/platform/meson/vdec/codec_mjpeg.c > create mode 100644 drivers/media/platform/meson/vdec/codec_mjpeg.h > create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg12.c > create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg12.h > create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg4.c > create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg4.h > create mode 100644 drivers/media/platform/meson/vdec/esparser.c > create mode 100644 drivers/media/platform/meson/vdec/esparser.h > create mode 100644 drivers/media/platform/meson/vdec/hevc_regs.h > create mode 100644 drivers/media/platform/meson/vdec/vdec.c > create mode 100644 drivers/media/platform/meson/vdec/vdec.h > create mode 100644 drivers/media/platform/meson/vdec/vdec_1.c > create mode 100644 drivers/media/platform/meson/vdec/vdec_1.h > create mode 100644 drivers/media/platform/meson/vdec/vdec_hevc.c > create mode 100644 drivers/media/platform/meson/vdec/vdec_hevc.h > create mode 100644 drivers/media/platform/meson/vdec/vdec_platform.c > create mode 100644 drivers/media/platform/meson/vdec/vdec_platform.h >
