Message ID | 20221022000506.221933-1-sebastian.fricke@collabora.com |
---|---|
Headers | show |
Series | Wave5 codec driver | expand |
Hey all, sorry I noticed that the TODO file is still in the staging directory. Will be fixed in V11. Greetings, Sebastian On 22.10.2022 02:05, Sebastian Fricke wrote: >From: Nas Chung <nas.chung@chipsnmedia.com> > >Add a TODO file with remaining elements to be improved/added. > >Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com> >Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >--- > drivers/staging/media/wave5/TODO | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > create mode 100644 drivers/staging/media/wave5/TODO > >diff --git a/drivers/staging/media/wave5/TODO b/drivers/staging/media/wave5/TODO >new file mode 100644 >index 000000000000..6cc38d4ccbc9 >--- /dev/null >+++ b/drivers/staging/media/wave5/TODO >@@ -0,0 +1,25 @@ >+* Handle interrupts better >+ >+Currently the interrupt handling uses an unusual design employing a kfifo to >+transfer irq status to irq thread. This was done as a work around for dropped >+interrupts seen with IRQF_ONESHOT based handling. >+ >+This needs further investigation and fixing properly, with the aid of >+C&M and StarFive engineers. >+ >+* remove all struct fields, that are assigned to but not used in the code, add >+ documentation about each removed field at the macro for the register it is >+ related too. >+ >+* power management handling - add (runtime_)suspen/resume cb where the clock is enabled >+ >+* revise logic of wave5_vpu_(dec/enc)_register_framebuffer >+ >+* check if the normal kernel endianness/__swab32 routines are sufficient. (instead of the ones >+ implemented in the driver) >+ >+* Extended Codec Controls for encoder. >+ >+implement extended codec control for HEVC/H264 encoder. >+PROFILE/LEVEL/MIN_QP/MAX_QP/LOOP_FILTER/HFLIP/VFIIP/ROTATE/ >+GOP_SIZE/RC_ENABLE/I_PERIOD/BITRATE. >-- >2.25.1 >
On 02:04-20221022, Sebastian Fricke wrote: > The Wave5 codec driver is a stateful encoder/decoder. > It is found on the J721S2 SoC, JH7100 SoC, ssd202d SoC. Etc. > But current test report is based on J721S2 SoC and pre-silicon FPGA. > > The driver currently supports V4L2_PIX_FMT_HEVC, V4L2_PIX_FMT_H264. > > This driver has so far been tested on J721S2 EVM board and pre-silicon > FPGA. > > Testing on J721S2 EVM board shows it working fine both decoder and > encoder. > The driver is successfully working with gstreamer v4l2 good-plugin > without any modification. > > # v4l2-compliance -d0 > Total for wave5-dec device /dev/video0: 44, Succeeded: 44, Failed: 0, Warnings: 0 > > # v4l2-compliance -d1 > Buffer ioctls: > fail: ../../../v4l-utils-1.18.1/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): q.reqbufs(node, 1) > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > fail: ../../../v4l-utils-1.18.1/utils/v4l2-compliance/v4l2-test-buffers.cpp(753): q.reqbufs(node, 2) > test VIDIOC_EXPBUF: FAIL > test Requests: OK (Not Supported) > > Total for wave5-enc device /dev/video1: 44, Succeeded: 42, Failed: 2, Warnings: 2 > > changes since v9: > > * Move from staging to the media directory > * Move coda driver to sub-directory > > * Fixes: > * Use platform_get_irq instead of platform_get_resource to fetch the IRQ > > * General cleanups: > * Add missing error messages to error conditions > * Improve messages/variable names/comments, align parameter names across the driver > * Use macros instead of magic numbers in multiple occassions > * Reduce code duplication in multiple places > * Fix whitespace, newline and tab alignment issues > * Remove unused struct fields & commented out code > * Convert signed integers to unsigned if signed is not necessary > * Convert int/unsigned int to s32/u32, when the variable is assigned to the > return of a register read or provided as a parameter for a register write > (and vice versa) > * Fix incorrect bitwise operators where logical operators are appropriate > * Multiple smaller changes > > * Generalization: > * Add new helper file providing generalized routines for vpu-dec & vpu-enc > * Generalize luma & chroma table size calculation and stride calculation > > * Resource cleanup and error handling: > * Add error handling to all calls with ignored return codes > * Handle DMA resource cleanup properly > * Fix insufficient instance cleanup while opening dec/enc > > changes since v8: > > * add 'wave5' to DEV_NAME > * update to support Multi-stream > * update to support loop test/dynamic resolution change > * remove unnecessary memset, g_volatile, old version option > > changes since v7: > > * update v4l2-compliance test report > * fix build error on linux-kernel 5.18.0-rc4 > > changes since v6: > > * update TODO file > * get sram info from device tree > > changes since v5: > > * support NV12/NV21 pixelformat for encoder and decoder > * handle adnormal exit and EOS > > changes since v4: > > * refactor functions in wave5-hw and fix bug reported by Daniel Palmer > * rename functions and variables to better names > * change variable types such as replacing s32 with u32 and int with bool > * as appropriate > > changes since v3: > > * Fixing all issues commented by Dan Carpenter > * Change file names to have wave5- prefix > * In wave5_vpu_probe, enable the clocks before reading registers, as > * commented from Daniel Palmer > * Add more to the TODO list, > > changes since v2: > > Main fixes includes: > * change the yaml and dirver code to support up to 4 clks (instead of > * one) > * fix Kconfig format > * remove unneeded cast, > * change var types > * change var names, func names > * checkpatch fixes > > changes since v1: > > Fix changes due to comments from Ezequiel and Dan Carpenter. Main fixes > inclueds: > * move all files to one dir 'wave5' > * replace private error codes with standard error codes > * fix extra spaces > * various checkpatch fixes > * replace private 'DPRINTK' macro with standard 'dev_err/dbg ..' > * fix error handling > * add more possible fixes to the TODO file > We see issues with the patchset. The decoder output contains garbage data randomly in the image. This is seen more for higher resolutions(>=1080p). The gstreamer pipeline for encoder fails with negotiation error as stated below. "vdec 4210000.video-codec: Invalid intra refresh mode: 0 (valid: 1-4)" > Dafna Hirschfeld (1): > media: chips-media: wave5: Add the vdi layer > > Nas Chung (3): > media: chips-media: wave5: Add vpuapi layer > media: chips-media: wave5: Add the v4l2 layer > media: chips-media: wave5: Add TODO file > > Robert Beckett (2): > dt-bindings: media: wave5: add yaml devicetree bindings > media: chips-media: wave5: Add wave5 driver to maintainers file > > Sebastian Fricke (1): > media: platform: chips-media: Move Coda to separate folder > > .../devicetree/bindings/media/wave5.yml | 73 + > MAINTAINERS | 11 +- > drivers/media/platform/chips-media/Kconfig | 18 +- > drivers/media/platform/chips-media/Makefile | 6 +- > .../media/platform/chips-media/coda/Kconfig | 18 + > .../media/platform/chips-media/coda/Makefile | 6 + > .../chips-media/{ => coda}/coda-bit.c | 0 > .../chips-media/{ => coda}/coda-common.c | 0 > .../chips-media/{ => coda}/coda-gdi.c | 0 > .../chips-media/{ => coda}/coda-h264.c | 0 > .../chips-media/{ => coda}/coda-jpeg.c | 0 > .../chips-media/{ => coda}/coda-mpeg2.c | 0 > .../chips-media/{ => coda}/coda-mpeg4.c | 0 > .../platform/chips-media/{ => coda}/coda.h | 0 > .../chips-media/{ => coda}/coda_regs.h | 0 > .../chips-media/{ => coda}/imx-vdoa.c | 0 > .../chips-media/{ => coda}/imx-vdoa.h | 0 > .../platform/chips-media/{ => coda}/trace.h | 0 > .../media/platform/chips-media/wave5/Kconfig | 12 + > .../media/platform/chips-media/wave5/Makefile | 10 + > drivers/media/platform/chips-media/wave5/TODO | 25 + > .../platform/chips-media/wave5/wave5-helper.c | 175 + > .../platform/chips-media/wave5/wave5-helper.h | 28 + > .../platform/chips-media/wave5/wave5-hw.c | 3459 +++++++++++++++++ > .../chips-media/wave5/wave5-regdefine.h | 654 ++++ > .../platform/chips-media/wave5/wave5-vdi.c | 261 ++ > .../platform/chips-media/wave5/wave5-vdi.h | 67 + > .../chips-media/wave5/wave5-vpu-dec.c | 1399 +++++++ > .../chips-media/wave5/wave5-vpu-enc.c | 1454 +++++++ > .../platform/chips-media/wave5/wave5-vpu.c | 362 ++ io.h is needed for devm_ioremap to work. > .../platform/chips-media/wave5/wave5-vpu.h | 72 + > .../platform/chips-media/wave5/wave5-vpuapi.c | 1115 ++++++ > .../platform/chips-media/wave5/wave5-vpuapi.h | 1198 ++++++ > .../chips-media/wave5/wave5-vpuconfig.h | 90 + > .../chips-media/wave5/wave5-vpuerror.h | 454 +++ > .../media/platform/chips-media/wave5/wave5.h | 94 + > 36 files changed, 11040 insertions(+), 21 deletions(-) > create mode 100644 Documentation/devicetree/bindings/media/wave5.yml > create mode 100644 drivers/media/platform/chips-media/coda/Kconfig > create mode 100644 drivers/media/platform/chips-media/coda/Makefile > rename drivers/media/platform/chips-media/{ => coda}/coda-bit.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-common.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-gdi.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-h264.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-jpeg.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-mpeg2.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda-mpeg4.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda.h (100%) > rename drivers/media/platform/chips-media/{ => coda}/coda_regs.h (100%) > rename drivers/media/platform/chips-media/{ => coda}/imx-vdoa.c (100%) > rename drivers/media/platform/chips-media/{ => coda}/imx-vdoa.h (100%) > rename drivers/media/platform/chips-media/{ => coda}/trace.h (100%) > create mode 100644 drivers/media/platform/chips-media/wave5/Kconfig > create mode 100644 drivers/media/platform/chips-media/wave5/Makefile > create mode 100644 drivers/media/platform/chips-media/wave5/TODO > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-helper.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-helper.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-hw.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-regdefine.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.c > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuerror.h > create mode 100644 drivers/media/platform/chips-media/wave5/wave5.h > > -- > 2.25.1 >
Hey Prasanth, Thanks for taking a look. On 31.10.2022 23:52, Prasanth Mantena wrote: >On 02:04-20221022, Sebastian Fricke wrote: >> The Wave5 codec driver is a stateful encoder/decoder. >> It is found on the J721S2 SoC, JH7100 SoC, ssd202d SoC. Etc. >> But current test report is based on J721S2 SoC and pre-silicon FPGA. >> >> The driver currently supports V4L2_PIX_FMT_HEVC, V4L2_PIX_FMT_H264. >> >> This driver has so far been tested on J721S2 EVM board and pre-silicon >> FPGA. >> >> Testing on J721S2 EVM board shows it working fine both decoder and >> encoder. >> The driver is successfully working with gstreamer v4l2 good-plugin >> without any modification. >> >> # v4l2-compliance -d0 >> Total for wave5-dec device /dev/video0: 44, Succeeded: 44, Failed: 0, Warnings: 0 >> >> # v4l2-compliance -d1 >> Buffer ioctls: >> fail: ../../../v4l-utils-1.18.1/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): q.reqbufs(node, 1) >> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL >> fail: ../../../v4l-utils-1.18.1/utils/v4l2-compliance/v4l2-test-buffers.cpp(753): q.reqbufs(node, 2) >> test VIDIOC_EXPBUF: FAIL >> test Requests: OK (Not Supported) >> >> Total for wave5-enc device /dev/video1: 44, Succeeded: 42, Failed: 2, Warnings: 2 >> >> changes since v9: >> >> * Move from staging to the media directory >> * Move coda driver to sub-directory >> >> * Fixes: >> * Use platform_get_irq instead of platform_get_resource to fetch the IRQ >> >> * General cleanups: >> * Add missing error messages to error conditions >> * Improve messages/variable names/comments, align parameter names across the driver >> * Use macros instead of magic numbers in multiple occassions >> * Reduce code duplication in multiple places >> * Fix whitespace, newline and tab alignment issues >> * Remove unused struct fields & commented out code >> * Convert signed integers to unsigned if signed is not necessary >> * Convert int/unsigned int to s32/u32, when the variable is assigned to the >> return of a register read or provided as a parameter for a register write >> (and vice versa) >> * Fix incorrect bitwise operators where logical operators are appropriate >> * Multiple smaller changes >> >> * Generalization: >> * Add new helper file providing generalized routines for vpu-dec & vpu-enc >> * Generalize luma & chroma table size calculation and stride calculation >> >> * Resource cleanup and error handling: >> * Add error handling to all calls with ignored return codes >> * Handle DMA resource cleanup properly >> * Fix insufficient instance cleanup while opening dec/enc >> >> changes since v8: >> >> * add 'wave5' to DEV_NAME >> * update to support Multi-stream >> * update to support loop test/dynamic resolution change >> * remove unnecessary memset, g_volatile, old version option >> >> changes since v7: >> >> * update v4l2-compliance test report >> * fix build error on linux-kernel 5.18.0-rc4 >> >> changes since v6: >> >> * update TODO file >> * get sram info from device tree >> >> changes since v5: >> >> * support NV12/NV21 pixelformat for encoder and decoder >> * handle adnormal exit and EOS >> >> changes since v4: >> >> * refactor functions in wave5-hw and fix bug reported by Daniel Palmer >> * rename functions and variables to better names >> * change variable types such as replacing s32 with u32 and int with bool >> * as appropriate >> >> changes since v3: >> >> * Fixing all issues commented by Dan Carpenter >> * Change file names to have wave5- prefix >> * In wave5_vpu_probe, enable the clocks before reading registers, as >> * commented from Daniel Palmer >> * Add more to the TODO list, >> >> changes since v2: >> >> Main fixes includes: >> * change the yaml and dirver code to support up to 4 clks (instead of >> * one) >> * fix Kconfig format >> * remove unneeded cast, >> * change var types >> * change var names, func names >> * checkpatch fixes >> >> changes since v1: >> >> Fix changes due to comments from Ezequiel and Dan Carpenter. Main fixes >> inclueds: >> * move all files to one dir 'wave5' >> * replace private error codes with standard error codes >> * fix extra spaces >> * various checkpatch fixes >> * replace private 'DPRINTK' macro with standard 'dev_err/dbg ..' >> * fix error handling >> * add more possible fixes to the TODO file >> > >We see issues with the patchset. The decoder output contains garbage >data randomly in the image. This is seen more for higher >resolutions(>=1080p). Thanks for the highlight, I am currently debugging that issue as well as an issue found with the encoder, which seems to be present since V9. I'll try to keep you updated but currently I am not finished with isolating the issue. >The gstreamer pipeline for encoder fails with negotiation error as >stated below. > >"vdec 4210000.video-codec: Invalid intra refresh mode: 0 (valid: 1-4)" Yes, I have noticed this issue as well and it as already fixed in my local tree, this was a refactoring error. > > >> Dafna Hirschfeld (1): >> media: chips-media: wave5: Add the vdi layer >> >> Nas Chung (3): >> media: chips-media: wave5: Add vpuapi layer >> media: chips-media: wave5: Add the v4l2 layer >> media: chips-media: wave5: Add TODO file >> >> Robert Beckett (2): >> dt-bindings: media: wave5: add yaml devicetree bindings >> media: chips-media: wave5: Add wave5 driver to maintainers file >> >> Sebastian Fricke (1): >> media: platform: chips-media: Move Coda to separate folder >> >> .../devicetree/bindings/media/wave5.yml | 73 + >> MAINTAINERS | 11 +- >> drivers/media/platform/chips-media/Kconfig | 18 +- >> drivers/media/platform/chips-media/Makefile | 6 +- >> .../media/platform/chips-media/coda/Kconfig | 18 + >> .../media/platform/chips-media/coda/Makefile | 6 + >> .../chips-media/{ => coda}/coda-bit.c | 0 >> .../chips-media/{ => coda}/coda-common.c | 0 >> .../chips-media/{ => coda}/coda-gdi.c | 0 >> .../chips-media/{ => coda}/coda-h264.c | 0 >> .../chips-media/{ => coda}/coda-jpeg.c | 0 >> .../chips-media/{ => coda}/coda-mpeg2.c | 0 >> .../chips-media/{ => coda}/coda-mpeg4.c | 0 >> .../platform/chips-media/{ => coda}/coda.h | 0 >> .../chips-media/{ => coda}/coda_regs.h | 0 >> .../chips-media/{ => coda}/imx-vdoa.c | 0 >> .../chips-media/{ => coda}/imx-vdoa.h | 0 >> .../platform/chips-media/{ => coda}/trace.h | 0 >> .../media/platform/chips-media/wave5/Kconfig | 12 + >> .../media/platform/chips-media/wave5/Makefile | 10 + >> drivers/media/platform/chips-media/wave5/TODO | 25 + >> .../platform/chips-media/wave5/wave5-helper.c | 175 + >> .../platform/chips-media/wave5/wave5-helper.h | 28 + >> .../platform/chips-media/wave5/wave5-hw.c | 3459 +++++++++++++++++ >> .../chips-media/wave5/wave5-regdefine.h | 654 ++++ >> .../platform/chips-media/wave5/wave5-vdi.c | 261 ++ >> .../platform/chips-media/wave5/wave5-vdi.h | 67 + >> .../chips-media/wave5/wave5-vpu-dec.c | 1399 +++++++ >> .../chips-media/wave5/wave5-vpu-enc.c | 1454 +++++++ >> .../platform/chips-media/wave5/wave5-vpu.c | 362 ++ > >io.h is needed for devm_ioremap to work. Do you mean this? ``` drivers/media/platform/chips-media/wave5/wave5-vpu.c 191: dev->vdb_register = devm_platform_ioremap_resource(pdev, 0); 192- if (IS_ERR(dev->vdb_register)) 193- return PTR_ERR(dev->vdb_register); ``` Because there is no `devm_ioremap` function call in the driver. And the `devm_platform_ioremap_resource`, seems to work just fine. Any clarification is highly appreciated for this comment. Sincerely, Sebastian Fricke > >> .../platform/chips-media/wave5/wave5-vpu.h | 72 + >> .../platform/chips-media/wave5/wave5-vpuapi.c | 1115 ++++++ >> .../platform/chips-media/wave5/wave5-vpuapi.h | 1198 ++++++ >> .../chips-media/wave5/wave5-vpuconfig.h | 90 + >> .../chips-media/wave5/wave5-vpuerror.h | 454 +++ >> .../media/platform/chips-media/wave5/wave5.h | 94 + >> 36 files changed, 11040 insertions(+), 21 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/media/wave5.yml >> create mode 100644 drivers/media/platform/chips-media/coda/Kconfig >> create mode 100644 drivers/media/platform/chips-media/coda/Makefile >> rename drivers/media/platform/chips-media/{ => coda}/coda-bit.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-common.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-gdi.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-h264.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-jpeg.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-mpeg2.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda-mpeg4.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda.h (100%) >> rename drivers/media/platform/chips-media/{ => coda}/coda_regs.h (100%) >> rename drivers/media/platform/chips-media/{ => coda}/imx-vdoa.c (100%) >> rename drivers/media/platform/chips-media/{ => coda}/imx-vdoa.h (100%) >> rename drivers/media/platform/chips-media/{ => coda}/trace.h (100%) >> create mode 100644 drivers/media/platform/chips-media/wave5/Kconfig >> create mode 100644 drivers/media/platform/chips-media/wave5/Makefile >> create mode 100644 drivers/media/platform/chips-media/wave5/TODO >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-helper.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-helper.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-hw.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-regdefine.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpu.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.c >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuerror.h >> create mode 100644 drivers/media/platform/chips-media/wave5/wave5.h >> >> -- >> 2.25.1 >>