Message ID | 20210526124516.2039434-1-benjamin.gaignard@collabora.com (mailing list archive) |
---|---|
Headers | show |
Series | Add HANTRO G2/HEVC decoder support for IMX8MQ | expand |
Le 26/05/2021 à 14:45, Benjamin Gaignard a écrit : > The IMX8MQ got two VPUs but until now only G1 has been enabled. > This series aim to add the second VPU (aka G2) and provide basic > HEVC decoding support. > > To be able to decode HEVC it is needed to add/update some of the > structures in the uapi. In addition of them one HANTRO dedicated > control is required to inform the driver of the number of bits to skip > at the beginning of the slice header. > The hardware require to allocate few auxiliary buffers to store the > references frame or tile size data. > > The driver has been tested with fluster test suite stream. > For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0 > > version 12: > - Change macro to avoid the final ';' > - Made arrays static and const > - Distinguish G2 generic fields (i.e these who could be reused for VP9) > from HEVC dedicated fields. Hi Hans, Could you merge this series ? The documentation for the compatible string will come when we will reach a conclusion about the power domain like describe in this thread: https://lore.kernel.org/linux-media/20210407073534.376722-1-benjamin.gaignard@collabora.com/T/#m6bd571fee94b1b1fa5c5d3fa6315ae5c9f4c8fad Regards, Benjamin > version 11: > - Rebased on media_stage. > - Fix minor typo/remarks. > > version 10: > - Shorter version of the previous series without ctrl block patches > and no DT modifications. > The scope of this series is limited to HEVC support. > > version 9: > - Corrections in commits messages. > - Define the dedicated control in hevc-controls.h > - Add note in documentation. > - Change max value of the dedicated control. > - Rebased on media_tree/master branch. > > version 8: > - Add reviewed-by and ack-by tags > - Fix the warnings reported by kernel test robot > - Only patch 9 (adding dedicated control), patch 11 (HEVC support) and > patch 13 (DT changes) are still missing of review/ack tag. > > version 7: > - Remove 'q' from syscon phandle name to make usable for iMX8MM too. > Update the bindings documentation. > - Add review/ack tags. > - Rebase on top of media_tree/master > - Be more accurate when computing the size of the memory needed motion > vectors. > - Explain why the all clocks need to set in the both DT node. > > version 6: > - fix the errors reported by kernel test robot > > version 5: > - use syscon instead of VPU reset driver. > - Do not break kernel/DT backward compatibility. > - Add documentation for dedicated Hantro control. > - Fix the remarks done by Ezequeil (typo, comments, unused function) > - Run v4l2-compliance without errors (see below). > - Do not add field to distinguish version, check postproc reg instead > > version 4: > - Split the changes in hevc controls in 2 commits to make them easier to > review. > - Change hantro_codec_ops run() prototype to return errors > - Hantro v4l2 dedicated control is now only an integer > - rebase on top of VPU reset changes posted here: > https://www.spinics.net/lists/arm-kernel/msg878440.html > - Various fix from previous remarks > - Limit the modifications in API to what the driver needs > > version 3: > - Fix typo in Hantro v4l2 dedicated control > - Add documentation for the new structures and fields > - Rebased on top of media_tree for-linus-5.12-rc1 tag > > version 2: > - remove all change related to scaling > - squash commits to a coherent split > - be more verbose about the added fields > - fix the comments done by Ezequiel about dma_alloc_coherent usage > - fix Dan's comments about control copy, reverse the test logic > in tile_buffer_reallocate, rework some goto and return cases. > - be more verbose about why I change the bindings > - remove all sign-off expect mime since it is confusing > - remove useless clocks in VPUs nodes > > Benjamin Gaignard (9): > media: hevc: Add fields and flags for hevc PPS > media: hevc: Add decode params control > media: hantro: change hantro_codec_ops run prototype to return errors > media: hantro: Define HEVC codec profiles and supported features > media: hantro: Only use postproc when post processed formats are > defined > media: uapi: Add a control for HANTRO driver > media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control > media: hantro: Introduce G2/HEVC decoder > media: hantro: IMX8M: add variant for G2/HEVC codec > > .../userspace-api/media/drivers/hantro.rst | 19 + > .../userspace-api/media/drivers/index.rst | 1 + > .../media/v4l/ext-ctrls-codec.rst | 108 +++- > .../media/v4l/vidioc-queryctrl.rst | 6 + > drivers/media/v4l2-core/v4l2-ctrls-core.c | 21 +- > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 + > drivers/staging/media/hantro/Makefile | 2 + > drivers/staging/media/hantro/hantro.h | 13 +- > drivers/staging/media/hantro/hantro_drv.c | 99 ++- > .../staging/media/hantro/hantro_g1_h264_dec.c | 10 +- > .../media/hantro/hantro_g1_mpeg2_dec.c | 4 +- > .../staging/media/hantro/hantro_g1_vp8_dec.c | 6 +- > .../staging/media/hantro/hantro_g2_hevc_dec.c | 587 ++++++++++++++++++ > drivers/staging/media/hantro/hantro_g2_regs.h | 206 ++++++ > .../staging/media/hantro/hantro_h1_jpeg_enc.c | 4 +- > drivers/staging/media/hantro/hantro_hevc.c | 327 ++++++++++ > drivers/staging/media/hantro/hantro_hw.h | 69 +- > .../staging/media/hantro/hantro_postproc.c | 14 + > drivers/staging/media/hantro/hantro_v4l2.c | 5 +- > drivers/staging/media/hantro/imx8m_vpu_hw.c | 96 ++- > .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 4 +- > .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 4 +- > .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 6 +- > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 + > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 + > .../staging/media/sunxi/cedrus/cedrus_h265.c | 12 +- > include/media/hevc-ctrls.h | 46 +- > 28 files changed, 1613 insertions(+), 69 deletions(-) > create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst > create mode 100644 drivers/staging/media/hantro/hantro_g2_hevc_dec.c > create mode 100644 drivers/staging/media/hantro/hantro_g2_regs.h > create mode 100644 drivers/staging/media/hantro/hantro_hevc.c >
Hi Benjamin, On 26/05/2021 14:45, Benjamin Gaignard wrote: > The IMX8MQ got two VPUs but until now only G1 has been enabled. > This series aim to add the second VPU (aka G2) and provide basic > HEVC decoding support. > > To be able to decode HEVC it is needed to add/update some of the > structures in the uapi. In addition of them one HANTRO dedicated > control is required to inform the driver of the number of bits to skip > at the beginning of the slice header. > The hardware require to allocate few auxiliary buffers to store the > references frame or tile size data. > > The driver has been tested with fluster test suite stream. > For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0 > > version 12: > - Change macro to avoid the final ';' > - Made arrays static and const > - Distinguish G2 generic fields (i.e these who could be reused for VP9) > from HEVC dedicated fields. I ran the latest checkpatch/smatch/sparse etc. over this series and got a lot of issues. The trickiest first: I get many reports for the register definitions in hantro_g2_regs.h: drivers/staging/media/hantro/hantro_g2_regs.h:16:33: warning: 'g2_strm_start_offset' defined but not used [-Wunused-const-variable=] I think it is a bad idea to declare variables in a header, and that's really what is causing this. I think it should be possible to rework this to defines such as: #define G2_DEC_REG(b, s, m) \ (const struct hantro_reg) { \ .base = G2_SWREG(b), \ .shift = s, \ .mask = m, \ } #define g2_strm_start_offset G2_DEC_REG(259, 0, 0xffffffff) This allows you to do: hantro_reg_write(vpu, &g2_strm_start_offset, 0); without having to declare static variables in a header. Other warnings: From the compiler: linux-git-arm-multi: WARNINGS drivers/staging/media/hantro/hantro_g2_hevc_dec.c: In function 'hantro_write_addr': drivers/staging/media/hantro/hantro_g2_hevc_dec.c:23:24: warning: right shift count >= width of type [-Wshift-count-overflow] 23 | vdpu_write(vpu, (addr >> 32) & 0xffffffff, offset + 4); | ^~ Note: this builds on a 32-bit arm! From smatch: drivers/staging/media/hantro/hantro_hevc.c:228 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_sao.cpu' drivers/staging/media/hantro/hantro_hevc.c:234 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_bsd.cpu' From kerneldoc: drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs' not described in 'hantro_hevc_dec_hw_ctx' drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_poc' not described in 'hantro_hevc_dec_hw_ctx' drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_used' not described in 'hantro_hevc_dec_hw_ctx' drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'num_tile_cols_allocated' not described in 'hantro_hevc_dec_hw_ctx' Regards, Hans > > version 11: > - Rebased on media_stage. > - Fix minor typo/remarks. > > version 10: > - Shorter version of the previous series without ctrl block patches > and no DT modifications. > The scope of this series is limited to HEVC support. > > version 9: > - Corrections in commits messages. > - Define the dedicated control in hevc-controls.h > - Add note in documentation. > - Change max value of the dedicated control. > - Rebased on media_tree/master branch. > > version 8: > - Add reviewed-by and ack-by tags > - Fix the warnings reported by kernel test robot > - Only patch 9 (adding dedicated control), patch 11 (HEVC support) and > patch 13 (DT changes) are still missing of review/ack tag. > > version 7: > - Remove 'q' from syscon phandle name to make usable for iMX8MM too. > Update the bindings documentation. > - Add review/ack tags. > - Rebase on top of media_tree/master > - Be more accurate when computing the size of the memory needed motion > vectors. > - Explain why the all clocks need to set in the both DT node. > > version 6: > - fix the errors reported by kernel test robot > > version 5: > - use syscon instead of VPU reset driver. > - Do not break kernel/DT backward compatibility. > - Add documentation for dedicated Hantro control. > - Fix the remarks done by Ezequeil (typo, comments, unused function) > - Run v4l2-compliance without errors (see below). > - Do not add field to distinguish version, check postproc reg instead > > version 4: > - Split the changes in hevc controls in 2 commits to make them easier to > review. > - Change hantro_codec_ops run() prototype to return errors > - Hantro v4l2 dedicated control is now only an integer > - rebase on top of VPU reset changes posted here: > https://www.spinics.net/lists/arm-kernel/msg878440.html > - Various fix from previous remarks > - Limit the modifications in API to what the driver needs > > version 3: > - Fix typo in Hantro v4l2 dedicated control > - Add documentation for the new structures and fields > - Rebased on top of media_tree for-linus-5.12-rc1 tag > > version 2: > - remove all change related to scaling > - squash commits to a coherent split > - be more verbose about the added fields > - fix the comments done by Ezequiel about dma_alloc_coherent usage > - fix Dan's comments about control copy, reverse the test logic > in tile_buffer_reallocate, rework some goto and return cases. > - be more verbose about why I change the bindings > - remove all sign-off expect mime since it is confusing > - remove useless clocks in VPUs nodes > > Benjamin Gaignard (9): > media: hevc: Add fields and flags for hevc PPS > media: hevc: Add decode params control > media: hantro: change hantro_codec_ops run prototype to return errors > media: hantro: Define HEVC codec profiles and supported features > media: hantro: Only use postproc when post processed formats are > defined > media: uapi: Add a control for HANTRO driver > media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control > media: hantro: Introduce G2/HEVC decoder > media: hantro: IMX8M: add variant for G2/HEVC codec > > .../userspace-api/media/drivers/hantro.rst | 19 + > .../userspace-api/media/drivers/index.rst | 1 + > .../media/v4l/ext-ctrls-codec.rst | 108 +++- > .../media/v4l/vidioc-queryctrl.rst | 6 + > drivers/media/v4l2-core/v4l2-ctrls-core.c | 21 +- > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 + > drivers/staging/media/hantro/Makefile | 2 + > drivers/staging/media/hantro/hantro.h | 13 +- > drivers/staging/media/hantro/hantro_drv.c | 99 ++- > .../staging/media/hantro/hantro_g1_h264_dec.c | 10 +- > .../media/hantro/hantro_g1_mpeg2_dec.c | 4 +- > .../staging/media/hantro/hantro_g1_vp8_dec.c | 6 +- > .../staging/media/hantro/hantro_g2_hevc_dec.c | 587 ++++++++++++++++++ > drivers/staging/media/hantro/hantro_g2_regs.h | 206 ++++++ > .../staging/media/hantro/hantro_h1_jpeg_enc.c | 4 +- > drivers/staging/media/hantro/hantro_hevc.c | 327 ++++++++++ > drivers/staging/media/hantro/hantro_hw.h | 69 +- > .../staging/media/hantro/hantro_postproc.c | 14 + > drivers/staging/media/hantro/hantro_v4l2.c | 5 +- > drivers/staging/media/hantro/imx8m_vpu_hw.c | 96 ++- > .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 4 +- > .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 4 +- > .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 6 +- > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 + > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 + > .../staging/media/sunxi/cedrus/cedrus_h265.c | 12 +- > include/media/hevc-ctrls.h | 46 +- > 28 files changed, 1613 insertions(+), 69 deletions(-) > create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst > create mode 100644 drivers/staging/media/hantro/hantro_g2_hevc_dec.c > create mode 100644 drivers/staging/media/hantro/hantro_g2_regs.h > create mode 100644 drivers/staging/media/hantro/hantro_hevc.c >
Le 03/06/2021 à 11:19, Hans Verkuil a écrit : > Hi Benjamin, > > On 26/05/2021 14:45, Benjamin Gaignard wrote: >> The IMX8MQ got two VPUs but until now only G1 has been enabled. >> This series aim to add the second VPU (aka G2) and provide basic >> HEVC decoding support. >> >> To be able to decode HEVC it is needed to add/update some of the >> structures in the uapi. In addition of them one HANTRO dedicated >> control is required to inform the driver of the number of bits to skip >> at the beginning of the slice header. >> The hardware require to allocate few auxiliary buffers to store the >> references frame or tile size data. >> >> The driver has been tested with fluster test suite stream. >> For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0 >> >> version 12: >> - Change macro to avoid the final ';' >> - Made arrays static and const >> - Distinguish G2 generic fields (i.e these who could be reused for VP9) >> from HEVC dedicated fields. > I ran the latest checkpatch/smatch/sparse etc. over this series and got a lot > of issues. > > The trickiest first: > > I get many reports for the register definitions in hantro_g2_regs.h: > > drivers/staging/media/hantro/hantro_g2_regs.h:16:33: warning: 'g2_strm_start_offset' defined but not used [-Wunused-const-variable=] > > I think it is a bad idea to declare variables in a header, and that's really > what is causing this. > > I think it should be possible to rework this to defines such as: > > #define G2_DEC_REG(b, s, m) \ > (const struct hantro_reg) { \ > .base = G2_SWREG(b), \ > .shift = s, \ > .mask = m, \ > } checkpatch complains about the macro to be complex so I have to add parentheses but that will be in v12 > #define g2_strm_start_offset G2_DEC_REG(259, 0, 0xffffffff) > > This allows you to do: > > hantro_reg_write(vpu, &g2_strm_start_offset, 0); > > without having to declare static variables in a header. > > Other warnings: > > From the compiler: > > linux-git-arm-multi: WARNINGS > > drivers/staging/media/hantro/hantro_g2_hevc_dec.c: In function 'hantro_write_addr': > drivers/staging/media/hantro/hantro_g2_hevc_dec.c:23:24: warning: right shift count >= width of type [-Wshift-count-overflow] > 23 | vdpu_write(vpu, (addr >> 32) & 0xffffffff, offset + 4); > | ^~ > > Note: this builds on a 32-bit arm! > > From smatch: > > drivers/staging/media/hantro/hantro_hevc.c:228 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_sao.cpu' > drivers/staging/media/hantro/hantro_hevc.c:234 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_bsd.cpu' That will be fixed in v12 > > From kerneldoc: > > drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs' not described in 'hantro_hevc_dec_hw_ctx' > drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_poc' not described in 'hantro_hevc_dec_hw_ctx' > drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_used' not described in 'hantro_hevc_dec_hw_ctx' > drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'num_tile_cols_allocated' not described in > 'hantro_hevc_dec_hw_ctx' I fix this and also remove the description for not existing fields. Thanks for your review. Benjamin > > Regards, > > Hans > >> version 11: >> - Rebased on media_stage. >> - Fix minor typo/remarks. >> >> version 10: >> - Shorter version of the previous series without ctrl block patches >> and no DT modifications. >> The scope of this series is limited to HEVC support. >> >> version 9: >> - Corrections in commits messages. >> - Define the dedicated control in hevc-controls.h >> - Add note in documentation. >> - Change max value of the dedicated control. >> - Rebased on media_tree/master branch. >> >> version 8: >> - Add reviewed-by and ack-by tags >> - Fix the warnings reported by kernel test robot >> - Only patch 9 (adding dedicated control), patch 11 (HEVC support) and >> patch 13 (DT changes) are still missing of review/ack tag. >> >> version 7: >> - Remove 'q' from syscon phandle name to make usable for iMX8MM too. >> Update the bindings documentation. >> - Add review/ack tags. >> - Rebase on top of media_tree/master >> - Be more accurate when computing the size of the memory needed motion >> vectors. >> - Explain why the all clocks need to set in the both DT node. >> >> version 6: >> - fix the errors reported by kernel test robot >> >> version 5: >> - use syscon instead of VPU reset driver. >> - Do not break kernel/DT backward compatibility. >> - Add documentation for dedicated Hantro control. >> - Fix the remarks done by Ezequeil (typo, comments, unused function) >> - Run v4l2-compliance without errors (see below). >> - Do not add field to distinguish version, check postproc reg instead >> >> version 4: >> - Split the changes in hevc controls in 2 commits to make them easier to >> review. >> - Change hantro_codec_ops run() prototype to return errors >> - Hantro v4l2 dedicated control is now only an integer >> - rebase on top of VPU reset changes posted here: >> https://www.spinics.net/lists/arm-kernel/msg878440.html >> - Various fix from previous remarks >> - Limit the modifications in API to what the driver needs >> >> version 3: >> - Fix typo in Hantro v4l2 dedicated control >> - Add documentation for the new structures and fields >> - Rebased on top of media_tree for-linus-5.12-rc1 tag >> >> version 2: >> - remove all change related to scaling >> - squash commits to a coherent split >> - be more verbose about the added fields >> - fix the comments done by Ezequiel about dma_alloc_coherent usage >> - fix Dan's comments about control copy, reverse the test logic >> in tile_buffer_reallocate, rework some goto and return cases. >> - be more verbose about why I change the bindings >> - remove all sign-off expect mime since it is confusing >> - remove useless clocks in VPUs nodes >> >> Benjamin Gaignard (9): >> media: hevc: Add fields and flags for hevc PPS >> media: hevc: Add decode params control >> media: hantro: change hantro_codec_ops run prototype to return errors >> media: hantro: Define HEVC codec profiles and supported features >> media: hantro: Only use postproc when post processed formats are >> defined >> media: uapi: Add a control for HANTRO driver >> media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control >> media: hantro: Introduce G2/HEVC decoder >> media: hantro: IMX8M: add variant for G2/HEVC codec >> >> .../userspace-api/media/drivers/hantro.rst | 19 + >> .../userspace-api/media/drivers/index.rst | 1 + >> .../media/v4l/ext-ctrls-codec.rst | 108 +++- >> .../media/v4l/vidioc-queryctrl.rst | 6 + >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 21 +- >> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 + >> drivers/staging/media/hantro/Makefile | 2 + >> drivers/staging/media/hantro/hantro.h | 13 +- >> drivers/staging/media/hantro/hantro_drv.c | 99 ++- >> .../staging/media/hantro/hantro_g1_h264_dec.c | 10 +- >> .../media/hantro/hantro_g1_mpeg2_dec.c | 4 +- >> .../staging/media/hantro/hantro_g1_vp8_dec.c | 6 +- >> .../staging/media/hantro/hantro_g2_hevc_dec.c | 587 ++++++++++++++++++ >> drivers/staging/media/hantro/hantro_g2_regs.h | 206 ++++++ >> .../staging/media/hantro/hantro_h1_jpeg_enc.c | 4 +- >> drivers/staging/media/hantro/hantro_hevc.c | 327 ++++++++++ >> drivers/staging/media/hantro/hantro_hw.h | 69 +- >> .../staging/media/hantro/hantro_postproc.c | 14 + >> drivers/staging/media/hantro/hantro_v4l2.c | 5 +- >> drivers/staging/media/hantro/imx8m_vpu_hw.c | 96 ++- >> .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 4 +- >> .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 4 +- >> .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 6 +- >> drivers/staging/media/sunxi/cedrus/cedrus.c | 6 + >> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + >> .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 + >> .../staging/media/sunxi/cedrus/cedrus_h265.c | 12 +- >> include/media/hevc-ctrls.h | 46 +- >> 28 files changed, 1613 insertions(+), 69 deletions(-) >> create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst >> create mode 100644 drivers/staging/media/hantro/hantro_g2_hevc_dec.c >> create mode 100644 drivers/staging/media/hantro/hantro_g2_regs.h >> create mode 100644 drivers/staging/media/hantro/hantro_hevc.c >>