mbox series

[v7,00/11] VP9 codec V4L2 control interface

Message ID 20210929160439.6601-1-andrzej.p@collabora.com (mailing list archive)
Headers show
Series VP9 codec V4L2 control interface | expand

Message

Andrzej Pietrasiewicz Sept. 29, 2021, 4:04 p.m. UTC
Dear all,

This patch series adds VP9 codec V4L2 control interface and two drivers
using the new controls. It is a follow-up of previous v6 series [1].

In this iteration, we've implemented VP9 hardware decoding on two devices:
Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
The i.MX8M driver needs proper power domains support, though, which is a
subject of a different effort, but in all 3 cases we were able to run the
drivers.

GStreamer support is also available, the needed changes have been submitted
by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
VP9 V4L2 controls to be merged and released.

Both rkvdec and hantro drivers are passing a significant number of VP9 tests
using Fluster[3]. There are still a few tests that are not passing, due to
dynamic frame resize (not yet supported by V4L2) and small size videos
(due to IP block limitations).

The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
merged without passing through staging, as agreed[4]. The ABI has been checked
for padding and verified to contain no holes.

[1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
[2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
[3] https://github.com/fluendo/fluster
[4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/

The series depends on the YUV tiled format support prepared by Ezequiel:
https://www.spinics.net/lists/linux-media/msg197047.html

Rebased onto latest media_tree.

Changes related to v6:
- moved setting tile filter and tile bsd auxiliary buffer addresses so
that they are always set, even if no tiles are used (thanks, Jernej)
- added a comment near the place where the 32-bit DMA mask is applied
  (thanks, Nicolas)
- improved consistency in register names (thanks, Nicolas)

Changes related to v5:
- improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
- improved pdf output of documentation
- added Benjamin's Reviewed-by (thanks, Benjamin)

Changes related to v4:
- removed unused enum v4l2_vp9_intra_prediction_mode
- converted remaining enums to defines to follow the convention
- improved the documentation, in particular better documented how to use segmentation 
features

Changes related to v3:

Apply suggestions from Jernej's review (thanks, Jernej):
- renamed a control and two structs:
	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
	v4l2_ctrl_vp9_compressed_hdr_probs =>
		v4l2_ctrl_vp9_compressed_hdr
	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
- moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
- fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
- explicitly assigned values to all other vp9 enums

Apply suggestion from Nicolas's review (thanks, Nicolas):
- explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
and implemented only by drivers which need it

Changes related to the RFC v2:

- added another driver including a postprocessor to de-tile
        codec-specific tiling
- reworked uAPI structs layout to follow VP8 style
- changed validation of loop filter params
- changed validation of segmentation params
- changed validation of VP9 frame params
- removed level lookup array from loop filter struct
        (can be computed by drivers)
- renamed some enum values to match the spec more closely
- V4L2 VP9 library changed the 'eob' member of
        'struct v4l2_vp9_frame_symbol_counts' so that it is an array
        of pointers instead of an array of pointers to arrays
        (IPs such as g2 creatively pass parts of the 'eob' counts in
        the 'coeff' counts)
- factored out several repeated portions of code
- minor nitpicks and cleanups

Andrzej Pietrasiewicz (6):
  media: uapi: Add VP9 stateless decoder controls
  media: Add VP9 v4l2 library
  media: hantro: Rename registers
  media: hantro: Prepare for other G2 codecs
  media: hantro: Support VP9 on the G2 core
  media: hantro: Support NV12 on the G2 core

Boris Brezillon (1):
  media: rkvdec: Add the VP9 backend

Ezequiel Garcia (4):
  hantro: postproc: Fix motion vector space size
  hantro: postproc: Introduce struct hantro_postproc_ops
  hantro: Simplify postprocessor
  hantro: Add quirk for NV12/NV12_4L4 capture format

 .../userspace-api/media/v4l/biblio.rst        |   10 +
 .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
 .../media/v4l/pixfmt-compressed.rst           |   15 +
 .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
 .../media/v4l/vidioc-queryctrl.rst            |   12 +
 .../media/videodev2.h.rst.exceptions          |    2 +
 drivers/media/v4l2-core/Kconfig               |    4 +
 drivers/media/v4l2-core/Makefile              |    1 +
 drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
 drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
 drivers/staging/media/hantro/Kconfig          |    1 +
 drivers/staging/media/hantro/Makefile         |    7 +-
 drivers/staging/media/hantro/hantro.h         |   40 +-
 drivers/staging/media/hantro/hantro_drv.c     |   23 +-
 drivers/staging/media/hantro/hantro_g2.c      |   27 +
 .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
 drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
 .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
 drivers/staging/media/hantro/hantro_hw.h      |   83 +-
 .../staging/media/hantro/hantro_postproc.c    |   79 +-
 drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
 drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
 drivers/staging/media/hantro/hantro_vp9.h     |  103 +
 drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
 .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
 .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
 drivers/staging/media/rkvdec/Kconfig          |    1 +
 drivers/staging/media/rkvdec/Makefile         |    2 +-
 drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
 drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
 drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
 include/media/v4l2-ctrls.h                    |    4 +
 include/media/v4l2-vp9.h                      |  182 ++
 include/uapi/linux/v4l2-controls.h            |  284 +++
 include/uapi/linux/videodev2.h                |    6 +
 37 files changed, 6033 insertions(+), 104 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
 create mode 100644 drivers/staging/media/hantro/hantro_g2.c
 create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
 create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
 create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
 create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
 create mode 100644 include/media/v4l2-vp9.h


base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82

Comments

Ezequiel Garcia Oct. 19, 2021, 5:55 p.m. UTC | #1
Hi everyone,

On Wed, 29 Sept 2021 at 12:04, Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Dear all,
>
> This patch series adds VP9 codec V4L2 control interface and two drivers
> using the new controls. It is a follow-up of previous v6 series [1].
>
> In this iteration, we've implemented VP9 hardware decoding on two devices:
> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
> The i.MX8M driver needs proper power domains support, though, which is a
> subject of a different effort, but in all 3 cases we were able to run the
> drivers.
>
> GStreamer support is also available, the needed changes have been submitted
> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
> VP9 V4L2 controls to be merged and released.
>
> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
> using Fluster[3]. There are still a few tests that are not passing, due to
> dynamic frame resize (not yet supported by V4L2) and small size videos
> (due to IP block limitations).
>
> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
> merged without passing through staging, as agreed[4]. The ABI has been checked
> for padding and verified to contain no holes.
>

I took another look at this, and I'm fairly happy with it.

I'd just like to have an A-b or R-b from Nicolas Dufresne and
Daniel Almeida, given they've done a lot of work on the client side
of the API.

Another option would be to wait until Jernej finishes the work on
Allwinner H6, so we have another hardware supported.

Thanks,
Ezequiel

> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
> [3] https://github.com/fluendo/fluster
> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>
> The series depends on the YUV tiled format support prepared by Ezequiel:
> https://www.spinics.net/lists/linux-media/msg197047.html
>
> Rebased onto latest media_tree.
>
> Changes related to v6:
> - moved setting tile filter and tile bsd auxiliary buffer addresses so
> that they are always set, even if no tiles are used (thanks, Jernej)
> - added a comment near the place where the 32-bit DMA mask is applied
>   (thanks, Nicolas)
> - improved consistency in register names (thanks, Nicolas)
>
> Changes related to v5:
> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
> - improved pdf output of documentation
> - added Benjamin's Reviewed-by (thanks, Benjamin)
>
> Changes related to v4:
> - removed unused enum v4l2_vp9_intra_prediction_mode
> - converted remaining enums to defines to follow the convention
> - improved the documentation, in particular better documented how to use segmentation
> features
>
> Changes related to v3:
>
> Apply suggestions from Jernej's review (thanks, Jernej):
> - renamed a control and two structs:
>         V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>                 V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>         v4l2_ctrl_vp9_compressed_hdr_probs =>
>                 v4l2_ctrl_vp9_compressed_hdr
>         v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
> - explicitly assigned values to all other vp9 enums
>
> Apply suggestion from Nicolas's review (thanks, Nicolas):
> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
> and implemented only by drivers which need it
>
> Changes related to the RFC v2:
>
> - added another driver including a postprocessor to de-tile
>         codec-specific tiling
> - reworked uAPI structs layout to follow VP8 style
> - changed validation of loop filter params
> - changed validation of segmentation params
> - changed validation of VP9 frame params
> - removed level lookup array from loop filter struct
>         (can be computed by drivers)
> - renamed some enum values to match the spec more closely
> - V4L2 VP9 library changed the 'eob' member of
>         'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>         of pointers instead of an array of pointers to arrays
>         (IPs such as g2 creatively pass parts of the 'eob' counts in
>         the 'coeff' counts)
> - factored out several repeated portions of code
> - minor nitpicks and cleanups
>
> Andrzej Pietrasiewicz (6):
>   media: uapi: Add VP9 stateless decoder controls
>   media: Add VP9 v4l2 library
>   media: hantro: Rename registers
>   media: hantro: Prepare for other G2 codecs
>   media: hantro: Support VP9 on the G2 core
>   media: hantro: Support NV12 on the G2 core
>
> Boris Brezillon (1):
>   media: rkvdec: Add the VP9 backend
>
> Ezequiel Garcia (4):
>   hantro: postproc: Fix motion vector space size
>   hantro: postproc: Introduce struct hantro_postproc_ops
>   hantro: Simplify postprocessor
>   hantro: Add quirk for NV12/NV12_4L4 capture format
>
>  .../userspace-api/media/v4l/biblio.rst        |   10 +
>  .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>  .../media/v4l/pixfmt-compressed.rst           |   15 +
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>  .../media/v4l/vidioc-queryctrl.rst            |   12 +
>  .../media/videodev2.h.rst.exceptions          |    2 +
>  drivers/media/v4l2-core/Kconfig               |    4 +
>  drivers/media/v4l2-core/Makefile              |    1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>  drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>  drivers/staging/media/hantro/Kconfig          |    1 +
>  drivers/staging/media/hantro/Makefile         |    7 +-
>  drivers/staging/media/hantro/hantro.h         |   40 +-
>  drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>  drivers/staging/media/hantro/hantro_g2.c      |   27 +
>  .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>  drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>  drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>  .../staging/media/hantro/hantro_postproc.c    |   79 +-
>  drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>  drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>  drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>  .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>  .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>  drivers/staging/media/rkvdec/Kconfig          |    1 +
>  drivers/staging/media/rkvdec/Makefile         |    2 +-
>  drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>  drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>  drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>  include/media/v4l2-ctrls.h                    |    4 +
>  include/media/v4l2-vp9.h                      |  182 ++
>  include/uapi/linux/v4l2-controls.h            |  284 +++
>  include/uapi/linux/videodev2.h                |    6 +
>  37 files changed, 6033 insertions(+), 104 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>  create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>  create mode 100644 include/media/v4l2-vp9.h
>
>
> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
> --
> 2.17.1
>
Hans Verkuil Nov. 11, 2021, 2:44 p.m. UTC | #2
Hi all,

Andrzej, Jernej, Nicolas, if none of you (or anyone else for that matter)
objects, then I'll make a PR for this early next week.

Regards,

	Hans

On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
> Dear all,
> 
> This patch series adds VP9 codec V4L2 control interface and two drivers
> using the new controls. It is a follow-up of previous v6 series [1].
> 
> In this iteration, we've implemented VP9 hardware decoding on two devices:
> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
> The i.MX8M driver needs proper power domains support, though, which is a
> subject of a different effort, but in all 3 cases we were able to run the
> drivers.
> 
> GStreamer support is also available, the needed changes have been submitted
> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
> VP9 V4L2 controls to be merged and released.
> 
> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
> using Fluster[3]. There are still a few tests that are not passing, due to
> dynamic frame resize (not yet supported by V4L2) and small size videos
> (due to IP block limitations).
> 
> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
> merged without passing through staging, as agreed[4]. The ABI has been checked
> for padding and verified to contain no holes.
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
> [3] https://github.com/fluendo/fluster
> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
> 
> The series depends on the YUV tiled format support prepared by Ezequiel:
> https://www.spinics.net/lists/linux-media/msg197047.html
> 
> Rebased onto latest media_tree.
> 
> Changes related to v6:
> - moved setting tile filter and tile bsd auxiliary buffer addresses so
> that they are always set, even if no tiles are used (thanks, Jernej)
> - added a comment near the place where the 32-bit DMA mask is applied
>   (thanks, Nicolas)
> - improved consistency in register names (thanks, Nicolas)
> 
> Changes related to v5:
> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
> - improved pdf output of documentation
> - added Benjamin's Reviewed-by (thanks, Benjamin)
> 
> Changes related to v4:
> - removed unused enum v4l2_vp9_intra_prediction_mode
> - converted remaining enums to defines to follow the convention
> - improved the documentation, in particular better documented how to use segmentation 
> features
> 
> Changes related to v3:
> 
> Apply suggestions from Jernej's review (thanks, Jernej):
> - renamed a control and two structs:
> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
> 		v4l2_ctrl_vp9_compressed_hdr
> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
> - explicitly assigned values to all other vp9 enums
> 
> Apply suggestion from Nicolas's review (thanks, Nicolas):
> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
> and implemented only by drivers which need it
> 
> Changes related to the RFC v2:
> 
> - added another driver including a postprocessor to de-tile
>         codec-specific tiling
> - reworked uAPI structs layout to follow VP8 style
> - changed validation of loop filter params
> - changed validation of segmentation params
> - changed validation of VP9 frame params
> - removed level lookup array from loop filter struct
>         (can be computed by drivers)
> - renamed some enum values to match the spec more closely
> - V4L2 VP9 library changed the 'eob' member of
>         'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>         of pointers instead of an array of pointers to arrays
>         (IPs such as g2 creatively pass parts of the 'eob' counts in
>         the 'coeff' counts)
> - factored out several repeated portions of code
> - minor nitpicks and cleanups
> 
> Andrzej Pietrasiewicz (6):
>   media: uapi: Add VP9 stateless decoder controls
>   media: Add VP9 v4l2 library
>   media: hantro: Rename registers
>   media: hantro: Prepare for other G2 codecs
>   media: hantro: Support VP9 on the G2 core
>   media: hantro: Support NV12 on the G2 core
> 
> Boris Brezillon (1):
>   media: rkvdec: Add the VP9 backend
> 
> Ezequiel Garcia (4):
>   hantro: postproc: Fix motion vector space size
>   hantro: postproc: Introduce struct hantro_postproc_ops
>   hantro: Simplify postprocessor
>   hantro: Add quirk for NV12/NV12_4L4 capture format
> 
>  .../userspace-api/media/v4l/biblio.rst        |   10 +
>  .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>  .../media/v4l/pixfmt-compressed.rst           |   15 +
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>  .../media/v4l/vidioc-queryctrl.rst            |   12 +
>  .../media/videodev2.h.rst.exceptions          |    2 +
>  drivers/media/v4l2-core/Kconfig               |    4 +
>  drivers/media/v4l2-core/Makefile              |    1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>  drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>  drivers/staging/media/hantro/Kconfig          |    1 +
>  drivers/staging/media/hantro/Makefile         |    7 +-
>  drivers/staging/media/hantro/hantro.h         |   40 +-
>  drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>  drivers/staging/media/hantro/hantro_g2.c      |   27 +
>  .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>  drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>  drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>  .../staging/media/hantro/hantro_postproc.c    |   79 +-
>  drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>  drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>  drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>  .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>  .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>  drivers/staging/media/rkvdec/Kconfig          |    1 +
>  drivers/staging/media/rkvdec/Makefile         |    2 +-
>  drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>  drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>  drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>  include/media/v4l2-ctrls.h                    |    4 +
>  include/media/v4l2-vp9.h                      |  182 ++
>  include/uapi/linux/v4l2-controls.h            |  284 +++
>  include/uapi/linux/videodev2.h                |    6 +
>  37 files changed, 6033 insertions(+), 104 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>  create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>  create mode 100644 include/media/v4l2-vp9.h
> 
> 
> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>
Nicolas Dufresne Nov. 12, 2021, 3:27 p.m. UTC | #3
Hi Hans,

Le jeudi 11 novembre 2021 à 15:44 +0100, Hans Verkuil a écrit :
> Hi all,
> 
> Andrzej, Jernej, Nicolas, if none of you (or anyone else for that matter)
> objects, then I'll make a PR for this early next week.

I have no objection. I've myself delayed replying as we have been digging a lot
into our compliance failures, but I believe we have explained most of them by
now and nothing seems to be related to the API.

regards,
Nicolas

> 
> Regards,
> 
> 	Hans
> 
> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
> > Dear all,
> > 
> > This patch series adds VP9 codec V4L2 control interface and two drivers
> > using the new controls. It is a follow-up of previous v6 series [1].
> > 
> > In this iteration, we've implemented VP9 hardware decoding on two devices:
> > Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
> > The i.MX8M driver needs proper power domains support, though, which is a
> > subject of a different effort, but in all 3 cases we were able to run the
> > drivers.
> > 
> > GStreamer support is also available, the needed changes have been submitted
> > by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
> > VP9 V4L2 controls to be merged and released.
> > 
> > Both rkvdec and hantro drivers are passing a significant number of VP9 tests
> > using Fluster[3]. There are still a few tests that are not passing, due to
> > dynamic frame resize (not yet supported by V4L2) and small size videos
> > (due to IP block limitations).
> > 
> > The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
> > merged without passing through staging, as agreed[4]. The ABI has been checked
> > for padding and verified to contain no holes.
> > 
> > [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
> > [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
> > [3] https://github.com/fluendo/fluster
> > [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
> > 
> > The series depends on the YUV tiled format support prepared by Ezequiel:
> > https://www.spinics.net/lists/linux-media/msg197047.html
> > 
> > Rebased onto latest media_tree.
> > 
> > Changes related to v6:
> > - moved setting tile filter and tile bsd auxiliary buffer addresses so
> > that they are always set, even if no tiles are used (thanks, Jernej)
> > - added a comment near the place where the 32-bit DMA mask is applied
> >   (thanks, Nicolas)
> > - improved consistency in register names (thanks, Nicolas)
> > 
> > Changes related to v5:
> > - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
> > - improved pdf output of documentation
> > - added Benjamin's Reviewed-by (thanks, Benjamin)
> > 
> > Changes related to v4:
> > - removed unused enum v4l2_vp9_intra_prediction_mode
> > - converted remaining enums to defines to follow the convention
> > - improved the documentation, in particular better documented how to use segmentation 
> > features
> > 
> > Changes related to v3:
> > 
> > Apply suggestions from Jernej's review (thanks, Jernej):
> > - renamed a control and two structs:
> > 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
> > 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
> > 	v4l2_ctrl_vp9_compressed_hdr_probs =>
> > 		v4l2_ctrl_vp9_compressed_hdr
> > 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
> > - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
> > - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
> > - explicitly assigned values to all other vp9 enums
> > 
> > Apply suggestion from Nicolas's review (thanks, Nicolas):
> > - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
> > and implemented only by drivers which need it
> > 
> > Changes related to the RFC v2:
> > 
> > - added another driver including a postprocessor to de-tile
> >         codec-specific tiling
> > - reworked uAPI structs layout to follow VP8 style
> > - changed validation of loop filter params
> > - changed validation of segmentation params
> > - changed validation of VP9 frame params
> > - removed level lookup array from loop filter struct
> >         (can be computed by drivers)
> > - renamed some enum values to match the spec more closely
> > - V4L2 VP9 library changed the 'eob' member of
> >         'struct v4l2_vp9_frame_symbol_counts' so that it is an array
> >         of pointers instead of an array of pointers to arrays
> >         (IPs such as g2 creatively pass parts of the 'eob' counts in
> >         the 'coeff' counts)
> > - factored out several repeated portions of code
> > - minor nitpicks and cleanups
> > 
> > Andrzej Pietrasiewicz (6):
> >   media: uapi: Add VP9 stateless decoder controls
> >   media: Add VP9 v4l2 library
> >   media: hantro: Rename registers
> >   media: hantro: Prepare for other G2 codecs
> >   media: hantro: Support VP9 on the G2 core
> >   media: hantro: Support NV12 on the G2 core
> > 
> > Boris Brezillon (1):
> >   media: rkvdec: Add the VP9 backend
> > 
> > Ezequiel Garcia (4):
> >   hantro: postproc: Fix motion vector space size
> >   hantro: postproc: Introduce struct hantro_postproc_ops
> >   hantro: Simplify postprocessor
> >   hantro: Add quirk for NV12/NV12_4L4 capture format
> > 
> >  .../userspace-api/media/v4l/biblio.rst        |   10 +
> >  .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
> >  .../media/v4l/pixfmt-compressed.rst           |   15 +
> >  .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
> >  .../media/v4l/vidioc-queryctrl.rst            |   12 +
> >  .../media/videodev2.h.rst.exceptions          |    2 +
> >  drivers/media/v4l2-core/Kconfig               |    4 +
> >  drivers/media/v4l2-core/Makefile              |    1 +
> >  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
> >  drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
> >  drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
> >  drivers/staging/media/hantro/Kconfig          |    1 +
> >  drivers/staging/media/hantro/Makefile         |    7 +-
> >  drivers/staging/media/hantro/hantro.h         |   40 +-
> >  drivers/staging/media/hantro/hantro_drv.c     |   23 +-
> >  drivers/staging/media/hantro/hantro_g2.c      |   27 +
> >  .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
> >  drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
> >  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
> >  drivers/staging/media/hantro/hantro_hw.h      |   83 +-
> >  .../staging/media/hantro/hantro_postproc.c    |   79 +-
> >  drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
> >  drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
> >  drivers/staging/media/hantro/hantro_vp9.h     |  103 +
> >  drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
> >  .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
> >  .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
> >  drivers/staging/media/rkvdec/Kconfig          |    1 +
> >  drivers/staging/media/rkvdec/Makefile         |    2 +-
> >  drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
> >  drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
> >  drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
> >  include/media/v4l2-ctrls.h                    |    4 +
> >  include/media/v4l2-vp9.h                      |  182 ++
> >  include/uapi/linux/v4l2-controls.h            |  284 +++
> >  include/uapi/linux/videodev2.h                |    6 +
> >  37 files changed, 6033 insertions(+), 104 deletions(-)
> >  create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
> >  create mode 100644 drivers/staging/media/hantro/hantro_g2.c
> >  create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> >  create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
> >  create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
> >  create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
> >  create mode 100644 include/media/v4l2-vp9.h
> > 
> > 
> > base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
> > 
>
Andrzej Pietrasiewicz Nov. 15, 2021, 12:56 p.m. UTC | #4
Hi Hans,

W dniu 12.11.2021 o 16:27, Nicolas Dufresne pisze:
> Hi Hans,
> 
> Le jeudi 11 novembre 2021 à 15:44 +0100, Hans Verkuil a écrit :
>> Hi all,
>>
>> Andrzej, Jernej, Nicolas, if none of you (or anyone else for that matter)
>> objects, then I'll make a PR for this early next week.
> 
> I have no objection. I've myself delayed replying as we have been digging a lot
> into our compliance failures, but I believe we have explained most of them by
> now and nothing seems to be related to the API.
> 
> regards,
> Nicolas
I'm fine with making a PR, too.

Andrzej

> 
>>
>> Regards,
>>
>> 	Hans
>>
>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>> Dear all,
>>>
>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>
>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>> The i.MX8M driver needs proper power domains support, though, which is a
>>> subject of a different effort, but in all 3 cases we were able to run the
>>> drivers.
>>>
>>> GStreamer support is also available, the needed changes have been submitted
>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>> VP9 V4L2 controls to be merged and released.
>>>
>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>> (due to IP block limitations).
>>>
>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>> for padding and verified to contain no holes.
>>>
>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>> [3] https://github.com/fluendo/fluster
>>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>>
>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>
>>> Rebased onto latest media_tree.
>>>
>>> Changes related to v6:
>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>    (thanks, Nicolas)
>>> - improved consistency in register names (thanks, Nicolas)
>>>
>>> Changes related to v5:
>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>> - improved pdf output of documentation
>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>
>>> Changes related to v4:
>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>> - converted remaining enums to defines to follow the convention
>>> - improved the documentation, in particular better documented how to use segmentation
>>> features
>>>
>>> Changes related to v3:
>>>
>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>> - renamed a control and two structs:
>>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>>> 		v4l2_ctrl_vp9_compressed_hdr
>>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>>> - explicitly assigned values to all other vp9 enums
>>>
>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>> and implemented only by drivers which need it
>>>
>>> Changes related to the RFC v2:
>>>
>>> - added another driver including a postprocessor to de-tile
>>>          codec-specific tiling
>>> - reworked uAPI structs layout to follow VP8 style
>>> - changed validation of loop filter params
>>> - changed validation of segmentation params
>>> - changed validation of VP9 frame params
>>> - removed level lookup array from loop filter struct
>>>          (can be computed by drivers)
>>> - renamed some enum values to match the spec more closely
>>> - V4L2 VP9 library changed the 'eob' member of
>>>          'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>          of pointers instead of an array of pointers to arrays
>>>          (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>          the 'coeff' counts)
>>> - factored out several repeated portions of code
>>> - minor nitpicks and cleanups
>>>
>>> Andrzej Pietrasiewicz (6):
>>>    media: uapi: Add VP9 stateless decoder controls
>>>    media: Add VP9 v4l2 library
>>>    media: hantro: Rename registers
>>>    media: hantro: Prepare for other G2 codecs
>>>    media: hantro: Support VP9 on the G2 core
>>>    media: hantro: Support NV12 on the G2 core
>>>
>>> Boris Brezillon (1):
>>>    media: rkvdec: Add the VP9 backend
>>>
>>> Ezequiel Garcia (4):
>>>    hantro: postproc: Fix motion vector space size
>>>    hantro: postproc: Introduce struct hantro_postproc_ops
>>>    hantro: Simplify postprocessor
>>>    hantro: Add quirk for NV12/NV12_4L4 capture format
>>>
>>>   .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>   .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>   .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>   .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>   .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>   .../media/videodev2.h.rst.exceptions          |    2 +
>>>   drivers/media/v4l2-core/Kconfig               |    4 +
>>>   drivers/media/v4l2-core/Makefile              |    1 +
>>>   drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>   drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>   drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>   drivers/staging/media/hantro/Kconfig          |    1 +
>>>   drivers/staging/media/hantro/Makefile         |    7 +-
>>>   drivers/staging/media/hantro/hantro.h         |   40 +-
>>>   drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>   drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>   .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>   drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>   .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>   drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>   .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>   drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>   drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>   drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>   drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>   .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>   .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>   drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>   drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>   drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>   drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>   drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>   include/media/v4l2-ctrls.h                    |    4 +
>>>   include/media/v4l2-vp9.h                      |  182 ++
>>>   include/uapi/linux/v4l2-controls.h            |  284 +++
>>>   include/uapi/linux/videodev2.h                |    6 +
>>>   37 files changed, 6033 insertions(+), 104 deletions(-)
>>>   create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>   create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>   create mode 100644 include/media/v4l2-vp9.h
>>>
>>>
>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>
>>
>
Andrzej Pietrasiewicz Nov. 15, 2021, 1:09 p.m. UTC | #5
Hi Hans,

Let me clarify:

W dniu 15.11.2021 o 13:56, Andrzej Pietrasiewicz pisze:
> Hi Hans,
> 
> W dniu 12.11.2021 o 16:27, Nicolas Dufresne pisze:
>> Hi Hans,
>>
>> Le jeudi 11 novembre 2021 à 15:44 +0100, Hans Verkuil a écrit :
>>> Hi all,
>>>
>>> Andrzej, Jernej, Nicolas, if none of you (or anyone else for that matter)
>>> objects, then I'll make a PR for this early next week.
>>
>> I have no objection. I've myself delayed replying as we have been digging a lot
>> into our compliance failures, but I believe we have explained most of them by
>> now and nothing seems to be related to the API.
>>
>> regards,
>> Nicolas
> I'm fine with making a PR, too.
What I meant was this: "I'm fine with you making a PR."


> 
> Andrzej
> 
>>
>>>
>>> Regards,
>>>
>>>     Hans
>>>
>>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>>> Dear all,
>>>>
>>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>>
>>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>>> The i.MX8M driver needs proper power domains support, though, which is a
>>>> subject of a different effort, but in all 3 cases we were able to run the
>>>> drivers.
>>>>
>>>> GStreamer support is also available, the needed changes have been submitted
>>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>>> VP9 V4L2 controls to be merged and released.
>>>>
>>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>>> (due to IP block limitations).
>>>>
>>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>>> for padding and verified to contain no holes.
>>>>
>>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>>> [2] 
>>>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>>> [3] https://github.com/fluendo/fluster
>>>> [4] 
>>>> https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/ 
>>>>
>>>>
>>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>>
>>>> Rebased onto latest media_tree.
>>>>
>>>> Changes related to v6:
>>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>>    (thanks, Nicolas)
>>>> - improved consistency in register names (thanks, Nicolas)
>>>>
>>>> Changes related to v5:
>>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>>> - improved pdf output of documentation
>>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>>
>>>> Changes related to v4:
>>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>>> - converted remaining enums to defines to follow the convention
>>>> - improved the documentation, in particular better documented how to use 
>>>> segmentation
>>>> features
>>>>
>>>> Changes related to v3:
>>>>
>>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>>> - renamed a control and two structs:
>>>>     V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>>>         V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>>>     v4l2_ctrl_vp9_compressed_hdr_probs =>
>>>>         v4l2_ctrl_vp9_compressed_hdr
>>>>     v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a 
>>>> bitfield)
>>>> - explicitly assigned values to all other vp9 enums
>>>>
>>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>>> and implemented only by drivers which need it
>>>>
>>>> Changes related to the RFC v2:
>>>>
>>>> - added another driver including a postprocessor to de-tile
>>>>          codec-specific tiling
>>>> - reworked uAPI structs layout to follow VP8 style
>>>> - changed validation of loop filter params
>>>> - changed validation of segmentation params
>>>> - changed validation of VP9 frame params
>>>> - removed level lookup array from loop filter struct
>>>>          (can be computed by drivers)
>>>> - renamed some enum values to match the spec more closely
>>>> - V4L2 VP9 library changed the 'eob' member of
>>>>          'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>>          of pointers instead of an array of pointers to arrays
>>>>          (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>>          the 'coeff' counts)
>>>> - factored out several repeated portions of code
>>>> - minor nitpicks and cleanups
>>>>
>>>> Andrzej Pietrasiewicz (6):
>>>>    media: uapi: Add VP9 stateless decoder controls
>>>>    media: Add VP9 v4l2 library
>>>>    media: hantro: Rename registers
>>>>    media: hantro: Prepare for other G2 codecs
>>>>    media: hantro: Support VP9 on the G2 core
>>>>    media: hantro: Support NV12 on the G2 core
>>>>
>>>> Boris Brezillon (1):
>>>>    media: rkvdec: Add the VP9 backend
>>>>
>>>> Ezequiel Garcia (4):
>>>>    hantro: postproc: Fix motion vector space size
>>>>    hantro: postproc: Introduce struct hantro_postproc_ops
>>>>    hantro: Simplify postprocessor
>>>>    hantro: Add quirk for NV12/NV12_4L4 capture format
>>>>
>>>>   .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>>   .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>>   .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>>   .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>>   .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>>   .../media/videodev2.h.rst.exceptions          |    2 +
>>>>   drivers/media/v4l2-core/Kconfig               |    4 +
>>>>   drivers/media/v4l2-core/Makefile              |    1 +
>>>>   drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>>   drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>>   drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>>   drivers/staging/media/hantro/Kconfig          |    1 +
>>>>   drivers/staging/media/hantro/Makefile         |    7 +-
>>>>   drivers/staging/media/hantro/hantro.h         |   40 +-
>>>>   drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>>   drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>>   .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>>   drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>>   .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>>   drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>>   .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>>   drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>>   drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>>   drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>>   drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>>   .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>>   .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>>   drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>>   drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>>   drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>>   drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>>   drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>>   include/media/v4l2-ctrls.h                    |    4 +
>>>>   include/media/v4l2-vp9.h                      |  182 ++
>>>>   include/uapi/linux/v4l2-controls.h            |  284 +++
>>>>   include/uapi/linux/videodev2.h                |    6 +
>>>>   37 files changed, 6033 insertions(+), 104 deletions(-)
>>>>   create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>>   create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>>   create mode 100644 include/media/v4l2-vp9.h
>>>>
>>>>
>>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>>
>>>
>>
>
Hans Verkuil Nov. 15, 2021, 3:07 p.m. UTC | #6
Andrzej,

Can you rebase this series on top of the master branch of
https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
manner.

Regards,

	Hans

On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
> Dear all,
> 
> This patch series adds VP9 codec V4L2 control interface and two drivers
> using the new controls. It is a follow-up of previous v6 series [1].
> 
> In this iteration, we've implemented VP9 hardware decoding on two devices:
> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
> The i.MX8M driver needs proper power domains support, though, which is a
> subject of a different effort, but in all 3 cases we were able to run the
> drivers.
> 
> GStreamer support is also available, the needed changes have been submitted
> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
> VP9 V4L2 controls to be merged and released.
> 
> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
> using Fluster[3]. There are still a few tests that are not passing, due to
> dynamic frame resize (not yet supported by V4L2) and small size videos
> (due to IP block limitations).
> 
> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
> merged without passing through staging, as agreed[4]. The ABI has been checked
> for padding and verified to contain no holes.
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
> [3] https://github.com/fluendo/fluster
> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
> 
> The series depends on the YUV tiled format support prepared by Ezequiel:
> https://www.spinics.net/lists/linux-media/msg197047.html
> 
> Rebased onto latest media_tree.
> 
> Changes related to v6:
> - moved setting tile filter and tile bsd auxiliary buffer addresses so
> that they are always set, even if no tiles are used (thanks, Jernej)
> - added a comment near the place where the 32-bit DMA mask is applied
>   (thanks, Nicolas)
> - improved consistency in register names (thanks, Nicolas)
> 
> Changes related to v5:
> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
> - improved pdf output of documentation
> - added Benjamin's Reviewed-by (thanks, Benjamin)
> 
> Changes related to v4:
> - removed unused enum v4l2_vp9_intra_prediction_mode
> - converted remaining enums to defines to follow the convention
> - improved the documentation, in particular better documented how to use segmentation 
> features
> 
> Changes related to v3:
> 
> Apply suggestions from Jernej's review (thanks, Jernej):
> - renamed a control and two structs:
> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
> 		v4l2_ctrl_vp9_compressed_hdr
> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
> - explicitly assigned values to all other vp9 enums
> 
> Apply suggestion from Nicolas's review (thanks, Nicolas):
> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
> and implemented only by drivers which need it
> 
> Changes related to the RFC v2:
> 
> - added another driver including a postprocessor to de-tile
>         codec-specific tiling
> - reworked uAPI structs layout to follow VP8 style
> - changed validation of loop filter params
> - changed validation of segmentation params
> - changed validation of VP9 frame params
> - removed level lookup array from loop filter struct
>         (can be computed by drivers)
> - renamed some enum values to match the spec more closely
> - V4L2 VP9 library changed the 'eob' member of
>         'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>         of pointers instead of an array of pointers to arrays
>         (IPs such as g2 creatively pass parts of the 'eob' counts in
>         the 'coeff' counts)
> - factored out several repeated portions of code
> - minor nitpicks and cleanups
> 
> Andrzej Pietrasiewicz (6):
>   media: uapi: Add VP9 stateless decoder controls
>   media: Add VP9 v4l2 library
>   media: hantro: Rename registers
>   media: hantro: Prepare for other G2 codecs
>   media: hantro: Support VP9 on the G2 core
>   media: hantro: Support NV12 on the G2 core
> 
> Boris Brezillon (1):
>   media: rkvdec: Add the VP9 backend
> 
> Ezequiel Garcia (4):
>   hantro: postproc: Fix motion vector space size
>   hantro: postproc: Introduce struct hantro_postproc_ops
>   hantro: Simplify postprocessor
>   hantro: Add quirk for NV12/NV12_4L4 capture format
> 
>  .../userspace-api/media/v4l/biblio.rst        |   10 +
>  .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>  .../media/v4l/pixfmt-compressed.rst           |   15 +
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>  .../media/v4l/vidioc-queryctrl.rst            |   12 +
>  .../media/videodev2.h.rst.exceptions          |    2 +
>  drivers/media/v4l2-core/Kconfig               |    4 +
>  drivers/media/v4l2-core/Makefile              |    1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>  drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>  drivers/staging/media/hantro/Kconfig          |    1 +
>  drivers/staging/media/hantro/Makefile         |    7 +-
>  drivers/staging/media/hantro/hantro.h         |   40 +-
>  drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>  drivers/staging/media/hantro/hantro_g2.c      |   27 +
>  .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>  drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>  drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>  .../staging/media/hantro/hantro_postproc.c    |   79 +-
>  drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>  drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>  drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>  .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>  .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>  drivers/staging/media/rkvdec/Kconfig          |    1 +
>  drivers/staging/media/rkvdec/Makefile         |    2 +-
>  drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>  drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>  drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>  include/media/v4l2-ctrls.h                    |    4 +
>  include/media/v4l2-vp9.h                      |  182 ++
>  include/uapi/linux/v4l2-controls.h            |  284 +++
>  include/uapi/linux/videodev2.h                |    6 +
>  37 files changed, 6033 insertions(+), 104 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>  create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>  create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>  create mode 100644 include/media/v4l2-vp9.h
> 
> 
> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>
Andrzej Pietrasiewicz Nov. 15, 2021, 5:14 p.m. UTC | #7
Hi Hans,

W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
> Andrzej,
> 
> Can you rebase this series on top of the master branch of
> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
> manner.

This is a branch for you:

https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi

Regards,

Andrzej


> 
> Regards,
> 
> 	Hans
> 
> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>> Dear all,
>>
>> This patch series adds VP9 codec V4L2 control interface and two drivers
>> using the new controls. It is a follow-up of previous v6 series [1].
>>
>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>> The i.MX8M driver needs proper power domains support, though, which is a
>> subject of a different effort, but in all 3 cases we were able to run the
>> drivers.
>>
>> GStreamer support is also available, the needed changes have been submitted
>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>> VP9 V4L2 controls to be merged and released.
>>
>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>> using Fluster[3]. There are still a few tests that are not passing, due to
>> dynamic frame resize (not yet supported by V4L2) and small size videos
>> (due to IP block limitations).
>>
>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>> merged without passing through staging, as agreed[4]. The ABI has been checked
>> for padding and verified to contain no holes.
>>
>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>> [3] https://github.com/fluendo/fluster
>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>
>> The series depends on the YUV tiled format support prepared by Ezequiel:
>> https://www.spinics.net/lists/linux-media/msg197047.html
>>
>> Rebased onto latest media_tree.
>>
>> Changes related to v6:
>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>> that they are always set, even if no tiles are used (thanks, Jernej)
>> - added a comment near the place where the 32-bit DMA mask is applied
>>    (thanks, Nicolas)
>> - improved consistency in register names (thanks, Nicolas)
>>
>> Changes related to v5:
>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>> - improved pdf output of documentation
>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>
>> Changes related to v4:
>> - removed unused enum v4l2_vp9_intra_prediction_mode
>> - converted remaining enums to defines to follow the convention
>> - improved the documentation, in particular better documented how to use segmentation
>> features
>>
>> Changes related to v3:
>>
>> Apply suggestions from Jernej's review (thanks, Jernej):
>> - renamed a control and two structs:
>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>> 		v4l2_ctrl_vp9_compressed_hdr
>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>> - explicitly assigned values to all other vp9 enums
>>
>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>> and implemented only by drivers which need it
>>
>> Changes related to the RFC v2:
>>
>> - added another driver including a postprocessor to de-tile
>>          codec-specific tiling
>> - reworked uAPI structs layout to follow VP8 style
>> - changed validation of loop filter params
>> - changed validation of segmentation params
>> - changed validation of VP9 frame params
>> - removed level lookup array from loop filter struct
>>          (can be computed by drivers)
>> - renamed some enum values to match the spec more closely
>> - V4L2 VP9 library changed the 'eob' member of
>>          'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>          of pointers instead of an array of pointers to arrays
>>          (IPs such as g2 creatively pass parts of the 'eob' counts in
>>          the 'coeff' counts)
>> - factored out several repeated portions of code
>> - minor nitpicks and cleanups
>>
>> Andrzej Pietrasiewicz (6):
>>    media: uapi: Add VP9 stateless decoder controls
>>    media: Add VP9 v4l2 library
>>    media: hantro: Rename registers
>>    media: hantro: Prepare for other G2 codecs
>>    media: hantro: Support VP9 on the G2 core
>>    media: hantro: Support NV12 on the G2 core
>>
>> Boris Brezillon (1):
>>    media: rkvdec: Add the VP9 backend
>>
>> Ezequiel Garcia (4):
>>    hantro: postproc: Fix motion vector space size
>>    hantro: postproc: Introduce struct hantro_postproc_ops
>>    hantro: Simplify postprocessor
>>    hantro: Add quirk for NV12/NV12_4L4 capture format
>>
>>   .../userspace-api/media/v4l/biblio.rst        |   10 +
>>   .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>   .../media/v4l/pixfmt-compressed.rst           |   15 +
>>   .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>   .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>   .../media/videodev2.h.rst.exceptions          |    2 +
>>   drivers/media/v4l2-core/Kconfig               |    4 +
>>   drivers/media/v4l2-core/Makefile              |    1 +
>>   drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>   drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>   drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>   drivers/staging/media/hantro/Kconfig          |    1 +
>>   drivers/staging/media/hantro/Makefile         |    7 +-
>>   drivers/staging/media/hantro/hantro.h         |   40 +-
>>   drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>   drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>   .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>   drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>   .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>   drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>   .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>   drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>   drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>   drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>   drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>   .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>   .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>   drivers/staging/media/rkvdec/Kconfig          |    1 +
>>   drivers/staging/media/rkvdec/Makefile         |    2 +-
>>   drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>   drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>   drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>   include/media/v4l2-ctrls.h                    |    4 +
>>   include/media/v4l2-vp9.h                      |  182 ++
>>   include/uapi/linux/v4l2-controls.h            |  284 +++
>>   include/uapi/linux/videodev2.h                |    6 +
>>   37 files changed, 6033 insertions(+), 104 deletions(-)
>>   create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>   create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>   create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>   create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>   create mode 100644 include/media/v4l2-vp9.h
>>
>>
>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>
>
Hans Verkuil Nov. 15, 2021, 9:16 p.m. UTC | #8
On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>> Andrzej,
>>
>> Can you rebase this series on top of the master branch of
>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>> manner.
> 
> This is a branch for you:
> 
> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi

I'm getting a bunch of sparse/smatch warnings:

sparse:
rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?

smatch:
rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'adaptive->inter_mode[i]' too small (4 vs 21)
hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'probs->inter_mode[i]' too small (3 vs 21

Also a bunch of kerneldoc warnings:

include/media/v4l2-vp9.h:30: warning: Function parameter or member 'joint' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_bit' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_fr' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'fr' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:30: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_mv_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx8' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx16' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx32' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'coef' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'inter_mode' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'interp_filter' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'is_inter' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_mode' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:58: warning: Function parameter or member 'mv' not described in 'v4l2_vp9_frame_context'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'intra_inter' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx32p' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx16p' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx8p' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'filter' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_joint' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_fp' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'fp' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'coeff' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:93: warning: Function parameter or member 'eob' not described in 'v4l2_vp9_frame_symbol_counts'
include/media/v4l2-vp9.h:166: warning: expecting prototype for v4l2_vp9_adapt_coef_probs(). Prototype was for v4l2_vp9_adapt_noncoef_probs()
instead
drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'vp_clk_pol' not described in 'isp_ccp2_cfg'
drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'lanecfg' not described in 'isp_ccp2_cfg'
drivers/media/platform/qcom/venus/core.h:202: warning: Function parameter or member 'sys_err_done' not described in 'venus_core'
drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'fw_min_cnt' not described in 'venus_inst'
drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'flags' not described in 'venus_inst'
drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'dpb_ids' not described in 'venus_inst'
drivers/staging/media/hantro/hantro.h:115: warning: Enum value 'HANTRO_MODE_VP9_DEC' not described in enum 'hantro_codec_mode'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_edge' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'misc' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cnts' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'probability_tables' not described in
'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'frame_context' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cur' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'bsd_ctrl_offset' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map_size' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'ctx_counters_offset' not described in
'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_info_offset' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_r_info' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_c_info' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_r' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_c' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_r' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_c' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'active_segment' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_enabled' not described in 'hantro_vp9_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_data' not described in 'hantro_vp9_dec_hw_ctx'

You can test kerneldoc yourself with: scripts/kernel-doc -none include/media/v4l2-vp9.h

Regards,

	Hans

> 
> Regards,
> 
> Andrzej
> 
> 
>>
>> Regards,
>>
>> 	Hans
>>
>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>> Dear all,
>>>
>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>
>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>> The i.MX8M driver needs proper power domains support, though, which is a
>>> subject of a different effort, but in all 3 cases we were able to run the
>>> drivers.
>>>
>>> GStreamer support is also available, the needed changes have been submitted
>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>> VP9 V4L2 controls to be merged and released.
>>>
>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>> (due to IP block limitations).
>>>
>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>> for padding and verified to contain no holes.
>>>
>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>> [3] https://github.com/fluendo/fluster
>>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>>
>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>
>>> Rebased onto latest media_tree.
>>>
>>> Changes related to v6:
>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>    (thanks, Nicolas)
>>> - improved consistency in register names (thanks, Nicolas)
>>>
>>> Changes related to v5:
>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>> - improved pdf output of documentation
>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>
>>> Changes related to v4:
>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>> - converted remaining enums to defines to follow the convention
>>> - improved the documentation, in particular better documented how to use segmentation
>>> features
>>>
>>> Changes related to v3:
>>>
>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>> - renamed a control and two structs:
>>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>>> 		v4l2_ctrl_vp9_compressed_hdr
>>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>>> - explicitly assigned values to all other vp9 enums
>>>
>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>> and implemented only by drivers which need it
>>>
>>> Changes related to the RFC v2:
>>>
>>> - added another driver including a postprocessor to de-tile
>>>          codec-specific tiling
>>> - reworked uAPI structs layout to follow VP8 style
>>> - changed validation of loop filter params
>>> - changed validation of segmentation params
>>> - changed validation of VP9 frame params
>>> - removed level lookup array from loop filter struct
>>>          (can be computed by drivers)
>>> - renamed some enum values to match the spec more closely
>>> - V4L2 VP9 library changed the 'eob' member of
>>>          'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>          of pointers instead of an array of pointers to arrays
>>>          (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>          the 'coeff' counts)
>>> - factored out several repeated portions of code
>>> - minor nitpicks and cleanups
>>>
>>> Andrzej Pietrasiewicz (6):
>>>    media: uapi: Add VP9 stateless decoder controls
>>>    media: Add VP9 v4l2 library
>>>    media: hantro: Rename registers
>>>    media: hantro: Prepare for other G2 codecs
>>>    media: hantro: Support VP9 on the G2 core
>>>    media: hantro: Support NV12 on the G2 core
>>>
>>> Boris Brezillon (1):
>>>    media: rkvdec: Add the VP9 backend
>>>
>>> Ezequiel Garcia (4):
>>>    hantro: postproc: Fix motion vector space size
>>>    hantro: postproc: Introduce struct hantro_postproc_ops
>>>    hantro: Simplify postprocessor
>>>    hantro: Add quirk for NV12/NV12_4L4 capture format
>>>
>>>   .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>   .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>   .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>   .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>   .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>   .../media/videodev2.h.rst.exceptions          |    2 +
>>>   drivers/media/v4l2-core/Kconfig               |    4 +
>>>   drivers/media/v4l2-core/Makefile              |    1 +
>>>   drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>   drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>   drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>   drivers/staging/media/hantro/Kconfig          |    1 +
>>>   drivers/staging/media/hantro/Makefile         |    7 +-
>>>   drivers/staging/media/hantro/hantro.h         |   40 +-
>>>   drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>   drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>   .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>   drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>   .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>   drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>   .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>   drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>   drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>   drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>   drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>   .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>   .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>   drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>   drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>   drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>   drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>   drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>   include/media/v4l2-ctrls.h                    |    4 +
>>>   include/media/v4l2-vp9.h                      |  182 ++
>>>   include/uapi/linux/v4l2-controls.h            |  284 +++
>>>   include/uapi/linux/videodev2.h                |    6 +
>>>   37 files changed, 6033 insertions(+), 104 deletions(-)
>>>   create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>   create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>   create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>   create mode 100644 include/media/v4l2-vp9.h
>>>
>>>
>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>
>>
>
Andrzej Pietrasiewicz Nov. 16, 2021, 8:09 a.m. UTC | #9
Hi Hans,

W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>> Hi Hans,
>>
>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>> Andrzej,
>>>
>>> Can you rebase this series on top of the master branch of
>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>> manner.
>>
>> This is a branch for you:
>>
>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
> 
> I'm getting a bunch of sparse/smatch warnings:
> 

Thanks for finding this, I will re-create the branch and let you know on irc.
Some of the below are "false positives, namely:

drivers/media/platform/omap3isp/omap3isp.h
drivers/media/platform/qcom/venus/core.h

which are not touched by the series.

Regards,

Andrzej

> sparse:
> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
> 
> smatch:
> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'adaptive->inter_mode[i]' too small (4 vs 21)
> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'probs->inter_mode[i]' too small (3 vs 21
> 
> Also a bunch of kerneldoc warnings:
> 
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'joint' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_bit' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_fr' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'fr' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_mv_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx8' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx16' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx32' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'coef' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'inter_mode' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'interp_filter' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'is_inter' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_mode' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'mv' not described in 'v4l2_vp9_frame_context'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'intra_inter' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx32p' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx16p' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx8p' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'filter' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_joint' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_fp' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'fp' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'coeff' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'eob' not described in 'v4l2_vp9_frame_symbol_counts'
> include/media/v4l2-vp9.h:166: warning: expecting prototype for v4l2_vp9_adapt_coef_probs(). Prototype was for v4l2_vp9_adapt_noncoef_probs()
> instead
> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'vp_clk_pol' not described in 'isp_ccp2_cfg'
> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'lanecfg' not described in 'isp_ccp2_cfg'
> drivers/media/platform/qcom/venus/core.h:202: warning: Function parameter or member 'sys_err_done' not described in 'venus_core'
> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'fw_min_cnt' not described in 'venus_inst'
> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'flags' not described in 'venus_inst'
> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'dpb_ids' not described in 'venus_inst'
> drivers/staging/media/hantro/hantro.h:115: warning: Enum value 'HANTRO_MODE_VP9_DEC' not described in enum 'hantro_codec_mode'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_edge' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'misc' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cnts' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'probability_tables' not described in
> 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'frame_context' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cur' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'bsd_ctrl_offset' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map_size' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'ctx_counters_offset' not described in
> 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_info_offset' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_r_info' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_c_info' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_r' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_c' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_r' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_c' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'active_segment' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_enabled' not described in 'hantro_vp9_dec_hw_ctx'
> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_data' not described in 'hantro_vp9_dec_hw_ctx'
> 
> You can test kerneldoc yourself with: scripts/kernel-doc -none include/media/v4l2-vp9.h
> 
> Regards,
> 
> 	Hans
> 
>>
>> Regards,
>>
>> Andrzej
>>
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>>> Dear all,
>>>>
>>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>>
>>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>>> The i.MX8M driver needs proper power domains support, though, which is a
>>>> subject of a different effort, but in all 3 cases we were able to run the
>>>> drivers.
>>>>
>>>> GStreamer support is also available, the needed changes have been submitted
>>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>>> VP9 V4L2 controls to be merged and released.
>>>>
>>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>>> (due to IP block limitations).
>>>>
>>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>>> for padding and verified to contain no holes.
>>>>
>>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>>> [3] https://github.com/fluendo/fluster
>>>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>>>
>>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>>
>>>> Rebased onto latest media_tree.
>>>>
>>>> Changes related to v6:
>>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>>     (thanks, Nicolas)
>>>> - improved consistency in register names (thanks, Nicolas)
>>>>
>>>> Changes related to v5:
>>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>>> - improved pdf output of documentation
>>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>>
>>>> Changes related to v4:
>>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>>> - converted remaining enums to defines to follow the convention
>>>> - improved the documentation, in particular better documented how to use segmentation
>>>> features
>>>>
>>>> Changes related to v3:
>>>>
>>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>>> - renamed a control and two structs:
>>>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>>>> 		v4l2_ctrl_vp9_compressed_hdr
>>>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>>>> - explicitly assigned values to all other vp9 enums
>>>>
>>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>>> and implemented only by drivers which need it
>>>>
>>>> Changes related to the RFC v2:
>>>>
>>>> - added another driver including a postprocessor to de-tile
>>>>           codec-specific tiling
>>>> - reworked uAPI structs layout to follow VP8 style
>>>> - changed validation of loop filter params
>>>> - changed validation of segmentation params
>>>> - changed validation of VP9 frame params
>>>> - removed level lookup array from loop filter struct
>>>>           (can be computed by drivers)
>>>> - renamed some enum values to match the spec more closely
>>>> - V4L2 VP9 library changed the 'eob' member of
>>>>           'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>>           of pointers instead of an array of pointers to arrays
>>>>           (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>>           the 'coeff' counts)
>>>> - factored out several repeated portions of code
>>>> - minor nitpicks and cleanups
>>>>
>>>> Andrzej Pietrasiewicz (6):
>>>>     media: uapi: Add VP9 stateless decoder controls
>>>>     media: Add VP9 v4l2 library
>>>>     media: hantro: Rename registers
>>>>     media: hantro: Prepare for other G2 codecs
>>>>     media: hantro: Support VP9 on the G2 core
>>>>     media: hantro: Support NV12 on the G2 core
>>>>
>>>> Boris Brezillon (1):
>>>>     media: rkvdec: Add the VP9 backend
>>>>
>>>> Ezequiel Garcia (4):
>>>>     hantro: postproc: Fix motion vector space size
>>>>     hantro: postproc: Introduce struct hantro_postproc_ops
>>>>     hantro: Simplify postprocessor
>>>>     hantro: Add quirk for NV12/NV12_4L4 capture format
>>>>
>>>>    .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>>    .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>>    .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>>    .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>>    .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>>    .../media/videodev2.h.rst.exceptions          |    2 +
>>>>    drivers/media/v4l2-core/Kconfig               |    4 +
>>>>    drivers/media/v4l2-core/Makefile              |    1 +
>>>>    drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>>    drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>>    drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>>    drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>>    drivers/staging/media/hantro/Kconfig          |    1 +
>>>>    drivers/staging/media/hantro/Makefile         |    7 +-
>>>>    drivers/staging/media/hantro/hantro.h         |   40 +-
>>>>    drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>>    drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>>    drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>>    .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>>    drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>>    .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>>    drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>>    drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>>    drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>>    .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>>    .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>>    drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>>    drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>>    drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>>    drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>>    drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>>    include/media/v4l2-ctrls.h                    |    4 +
>>>>    include/media/v4l2-vp9.h                      |  182 ++
>>>>    include/uapi/linux/v4l2-controls.h            |  284 +++
>>>>    include/uapi/linux/videodev2.h                |    6 +
>>>>    37 files changed, 6033 insertions(+), 104 deletions(-)
>>>>    create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>>    create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>>    create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>>    create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>>    create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>>    create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>>    create mode 100644 include/media/v4l2-vp9.h
>>>>
>>>>
>>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>>
>>>
>>
>
Hans Verkuil Nov. 16, 2021, 8:21 a.m. UTC | #10
On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>> Hi Hans,
>>>
>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>> Andrzej,
>>>>
>>>> Can you rebase this series on top of the master branch of
>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>> manner.
>>>
>>> This is a branch for you:
>>>
>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>
>> I'm getting a bunch of sparse/smatch warnings:
>>
> 
> Thanks for finding this, I will re-create the branch and let you know on irc.
> Some of the below are "false positives, namely:
> 
> drivers/media/platform/omap3isp/omap3isp.h
> drivers/media/platform/qcom/venus/core.h

Ah, sorry, I though I had filtered those out. Obviously you can ignore those.

Please post a v8. That way the series is archived on lore. And it works better
with patchwork.

Regards,

	Hans

> 
> which are not touched by the series.
> 
> Regards,
> 
> Andrzej
> 
>> sparse:
>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>
>> smatch:
>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
>> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'adaptive->inter_mode[i]' too small (4 vs 21)
>> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'probs->inter_mode[i]' too small (3 vs 21
>>
>> Also a bunch of kerneldoc warnings:
>>
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'joint' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_bit' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_fr' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'fr' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_mv_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx8' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx16' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx32' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'coef' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'inter_mode' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'interp_filter' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'is_inter' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_mode' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'mv' not described in 'v4l2_vp9_frame_context'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'intra_inter' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx32p' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx16p' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx8p' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'filter' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_joint' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_fp' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'fp' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'coeff' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'eob' not described in 'v4l2_vp9_frame_symbol_counts'
>> include/media/v4l2-vp9.h:166: warning: expecting prototype for v4l2_vp9_adapt_coef_probs(). Prototype was for v4l2_vp9_adapt_noncoef_probs()
>> instead
>> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'vp_clk_pol' not described in 'isp_ccp2_cfg'
>> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'lanecfg' not described in 'isp_ccp2_cfg'
>> drivers/media/platform/qcom/venus/core.h:202: warning: Function parameter or member 'sys_err_done' not described in 'venus_core'
>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'fw_min_cnt' not described in 'venus_inst'
>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'flags' not described in 'venus_inst'
>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'dpb_ids' not described in 'venus_inst'
>> drivers/staging/media/hantro/hantro.h:115: warning: Enum value 'HANTRO_MODE_VP9_DEC' not described in enum 'hantro_codec_mode'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_edge' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'misc' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cnts' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'probability_tables' not described in
>> 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'frame_context' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cur' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'bsd_ctrl_offset' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map_size' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'ctx_counters_offset' not described in
>> 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_info_offset' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_r_info' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_c_info' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_r' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_c' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_r' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_c' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'active_segment' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_enabled' not described in 'hantro_vp9_dec_hw_ctx'
>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_data' not described in 'hantro_vp9_dec_hw_ctx'
>>
>> You can test kerneldoc yourself with: scripts/kernel-doc -none include/media/v4l2-vp9.h
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Regards,
>>>
>>> Andrzej
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>>>> Dear all,
>>>>>
>>>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>>>
>>>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>>>> The i.MX8M driver needs proper power domains support, though, which is a
>>>>> subject of a different effort, but in all 3 cases we were able to run the
>>>>> drivers.
>>>>>
>>>>> GStreamer support is also available, the needed changes have been submitted
>>>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>>>> VP9 V4L2 controls to be merged and released.
>>>>>
>>>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>>>> (due to IP block limitations).
>>>>>
>>>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>>>> for padding and verified to contain no holes.
>>>>>
>>>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>>>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>>>> [3] https://github.com/fluendo/fluster
>>>>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>>>>
>>>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>>>
>>>>> Rebased onto latest media_tree.
>>>>>
>>>>> Changes related to v6:
>>>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>>>     (thanks, Nicolas)
>>>>> - improved consistency in register names (thanks, Nicolas)
>>>>>
>>>>> Changes related to v5:
>>>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>>>> - improved pdf output of documentation
>>>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>>>
>>>>> Changes related to v4:
>>>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>>>> - converted remaining enums to defines to follow the convention
>>>>> - improved the documentation, in particular better documented how to use segmentation
>>>>> features
>>>>>
>>>>> Changes related to v3:
>>>>>
>>>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>>>> - renamed a control and two structs:
>>>>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>>>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>>>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>>>>> 		v4l2_ctrl_vp9_compressed_hdr
>>>>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>>>>> - explicitly assigned values to all other vp9 enums
>>>>>
>>>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>>>> and implemented only by drivers which need it
>>>>>
>>>>> Changes related to the RFC v2:
>>>>>
>>>>> - added another driver including a postprocessor to de-tile
>>>>>           codec-specific tiling
>>>>> - reworked uAPI structs layout to follow VP8 style
>>>>> - changed validation of loop filter params
>>>>> - changed validation of segmentation params
>>>>> - changed validation of VP9 frame params
>>>>> - removed level lookup array from loop filter struct
>>>>>           (can be computed by drivers)
>>>>> - renamed some enum values to match the spec more closely
>>>>> - V4L2 VP9 library changed the 'eob' member of
>>>>>           'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>>>           of pointers instead of an array of pointers to arrays
>>>>>           (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>>>           the 'coeff' counts)
>>>>> - factored out several repeated portions of code
>>>>> - minor nitpicks and cleanups
>>>>>
>>>>> Andrzej Pietrasiewicz (6):
>>>>>     media: uapi: Add VP9 stateless decoder controls
>>>>>     media: Add VP9 v4l2 library
>>>>>     media: hantro: Rename registers
>>>>>     media: hantro: Prepare for other G2 codecs
>>>>>     media: hantro: Support VP9 on the G2 core
>>>>>     media: hantro: Support NV12 on the G2 core
>>>>>
>>>>> Boris Brezillon (1):
>>>>>     media: rkvdec: Add the VP9 backend
>>>>>
>>>>> Ezequiel Garcia (4):
>>>>>     hantro: postproc: Fix motion vector space size
>>>>>     hantro: postproc: Introduce struct hantro_postproc_ops
>>>>>     hantro: Simplify postprocessor
>>>>>     hantro: Add quirk for NV12/NV12_4L4 capture format
>>>>>
>>>>>    .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>>>    .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>>>    .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>>>    .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>>>    .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>>>    .../media/videodev2.h.rst.exceptions          |    2 +
>>>>>    drivers/media/v4l2-core/Kconfig               |    4 +
>>>>>    drivers/media/v4l2-core/Makefile              |    1 +
>>>>>    drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>>>    drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>>>    drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>>>    drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>>>    drivers/staging/media/hantro/Kconfig          |    1 +
>>>>>    drivers/staging/media/hantro/Makefile         |    7 +-
>>>>>    drivers/staging/media/hantro/hantro.h         |   40 +-
>>>>>    drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>>>    drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>>>    drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>>>    .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>>>    drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>>>    .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>>>    drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>>>    drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>>>    drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>>>    .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>>>    .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>>>    drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>>>    drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>>>    drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>>>    drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>>>    drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>>>    include/media/v4l2-ctrls.h                    |    4 +
>>>>>    include/media/v4l2-vp9.h                      |  182 ++
>>>>>    include/uapi/linux/v4l2-controls.h            |  284 +++
>>>>>    include/uapi/linux/videodev2.h                |    6 +
>>>>>    37 files changed, 6033 insertions(+), 104 deletions(-)
>>>>>    create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>>>    create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>>>    create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>>>    create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>>>    create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>>>    create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>>>    create mode 100644 include/media/v4l2-vp9.h
>>>>>
>>>>>
>>>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>>>
>>>>
>>>
>>
>
Andrzej Pietrasiewicz Nov. 16, 2021, 1:14 p.m. UTC | #11
Hi,

W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>> Hi Hans,
>>
>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>> Hi Hans,
>>>>
>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>> Andrzej,
>>>>>
>>>>> Can you rebase this series on top of the master branch of
>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>> manner.
>>>>
>>>> This is a branch for you:
>>>>
>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>
>>> I'm getting a bunch of sparse/smatch warnings:
>>>
>>
>> Thanks for finding this, I will re-create the branch and let you know on irc.
>> Some of the below are "false positives, namely:
>>
>> drivers/media/platform/omap3isp/omap3isp.h
>> drivers/media/platform/qcom/venus/core.h
> 
> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
> 
> Please post a v8. That way the series is archived on lore. And it works better
> with patchwork.

Sure, no problem. Also please see below.

> 
> Regards,
> 
> 	Hans
> 
>>
>> which are not touched by the series.
>>
>> Regards,
>>
>> Andrzej
>>
>>> sparse:
>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>
>>> smatch:
>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91

this looks a false positive.

A portion of memory pointed to by ptr is indexed with i * 23 + m,
where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
So the largest index value we compute equals 89 (3 * 23 + 20).
Because ptr points to something that is at least 90 bytes large,
89 is a valid index and no greater index will be ever computed.

>>> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'adaptive->inter_mode[i]' too small (4 vs 21)
>>> hantro/hantro_g2_vp9_dec.c: hantro/hantro_g2_vp9_dec.c:670 config_probs() error: memcpy() 'probs->inter_mode[i]' too small (3 vs 21
>>>
>>> Also a bunch of kerneldoc warnings:
>>>
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'joint' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_bit' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_fr' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'fr' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:30: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_mv_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx8' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx16' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'tx32' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'coef' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'inter_mode' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'interp_filter' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'is_inter' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_mode' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:58: warning: Function parameter or member 'mv' not described in 'v4l2_vp9_frame_context'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'partition' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'skip' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'intra_inter' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx32p' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx16p' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'tx8p' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'y_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'uv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'comp_ref' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'single_ref' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_mode' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'filter' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'mv_joint' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'sign' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'classes' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'bits' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_fp' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'fp' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'class0_hp' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'hp' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'coeff' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:93: warning: Function parameter or member 'eob' not described in 'v4l2_vp9_frame_symbol_counts'
>>> include/media/v4l2-vp9.h:166: warning: expecting prototype for v4l2_vp9_adapt_coef_probs(). Prototype was for v4l2_vp9_adapt_noncoef_probs()
>>> instead
>>> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'vp_clk_pol' not described in 'isp_ccp2_cfg'
>>> drivers/media/platform/omap3isp/omap3isp.h:107: warning: Function parameter or member 'lanecfg' not described in 'isp_ccp2_cfg'
>>> drivers/media/platform/qcom/venus/core.h:202: warning: Function parameter or member 'sys_err_done' not described in 'venus_core'
>>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'fw_min_cnt' not described in 'venus_inst'
>>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'flags' not described in 'venus_inst'
>>> drivers/media/platform/qcom/venus/core.h:462: warning: Function parameter or member 'dpb_ids' not described in 'venus_inst'
>>> drivers/staging/media/hantro/hantro.h:115: warning: Enum value 'HANTRO_MODE_VP9_DEC' not described in enum 'hantro_codec_mode'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_edge' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'misc' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cnts' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'probability_tables' not described in
>>> 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'frame_context' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'cur' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'bsd_ctrl_offset' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'segment_map_size' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'ctx_counters_offset' not described in
>>> 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_info_offset' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_r_info' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'tile_c_info' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_r' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_tile_c' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_r' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'last_sbs_c' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'active_segment' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_enabled' not described in 'hantro_vp9_dec_hw_ctx'
>>> drivers/staging/media/hantro/hantro_hw.h:211: warning: Function parameter or member 'feature_data' not described in 'hantro_vp9_dec_hw_ctx'
>>>
>>> You can test kerneldoc yourself with: scripts/kernel-doc -none include/media/v4l2-vp9.h
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>> Regards,
>>>>
>>>> Andrzej
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>>
>>>>> On 29/09/2021 18:04, Andrzej Pietrasiewicz wrote:
>>>>>> Dear all,
>>>>>>
>>>>>> This patch series adds VP9 codec V4L2 control interface and two drivers
>>>>>> using the new controls. It is a follow-up of previous v6 series [1].
>>>>>>
>>>>>> In this iteration, we've implemented VP9 hardware decoding on two devices:
>>>>>> Rockchip VDEC and Hantro G2, and tested on RK3399, i.MX8MQ and i.MX8MP.
>>>>>> The i.MX8M driver needs proper power domains support, though, which is a
>>>>>> subject of a different effort, but in all 3 cases we were able to run the
>>>>>> drivers.
>>>>>>
>>>>>> GStreamer support is also available, the needed changes have been submitted
>>>>>> by Daniel Almeida [2]. This MR is ready to be merged, and just needs the
>>>>>> VP9 V4L2 controls to be merged and released.
>>>>>>
>>>>>> Both rkvdec and hantro drivers are passing a significant number of VP9 tests
>>>>>> using Fluster[3]. There are still a few tests that are not passing, due to
>>>>>> dynamic frame resize (not yet supported by V4L2) and small size videos
>>>>>> (due to IP block limitations).
>>>>>>
>>>>>> The series adds the VP9 codec V4L2 control API as uAPI, so it aims at being
>>>>>> merged without passing through staging, as agreed[4]. The ABI has been checked
>>>>>> for padding and verified to contain no holes.
>>>>>>
>>>>>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=6377
>>>>>> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2144
>>>>>> [3] https://github.com/fluendo/fluster
>>>>>> [4] https://lore.kernel.org/linux-media/b8f83c93-67fd-09f5-9314-15746cbfdc61@xs4all.nl/
>>>>>>
>>>>>> The series depends on the YUV tiled format support prepared by Ezequiel:
>>>>>> https://www.spinics.net/lists/linux-media/msg197047.html
>>>>>>
>>>>>> Rebased onto latest media_tree.
>>>>>>
>>>>>> Changes related to v6:
>>>>>> - moved setting tile filter and tile bsd auxiliary buffer addresses so
>>>>>> that they are always set, even if no tiles are used (thanks, Jernej)
>>>>>> - added a comment near the place where the 32-bit DMA mask is applied
>>>>>>      (thanks, Nicolas)
>>>>>> - improved consistency in register names (thanks, Nicolas)
>>>>>>
>>>>>> Changes related to v5:
>>>>>> - improved the doc comments as per Ezequiel's review (thanks, Ezequiel)
>>>>>> - improved pdf output of documentation
>>>>>> - added Benjamin's Reviewed-by (thanks, Benjamin)
>>>>>>
>>>>>> Changes related to v4:
>>>>>> - removed unused enum v4l2_vp9_intra_prediction_mode
>>>>>> - converted remaining enums to defines to follow the convention
>>>>>> - improved the documentation, in particular better documented how to use segmentation
>>>>>> features
>>>>>>
>>>>>> Changes related to v3:
>>>>>>
>>>>>> Apply suggestions from Jernej's review (thanks, Jernej):
>>>>>> - renamed a control and two structs:
>>>>>> 	V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR_PROBS =>
>>>>>> 		V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR
>>>>>> 	v4l2_ctrl_vp9_compressed_hdr_probs =>
>>>>>> 		v4l2_ctrl_vp9_compressed_hdr
>>>>>> 	v4l2_vp9_mv_compressed_hdr_probs => v4l2_vp9_mv_probs
>>>>>> - moved tx_mode to v4l2_ctrl_vp9_compressed_hdr
>>>>>> - fixed enum v4l2_vp9_ref_frame_sign_bias values (which are used to test a bitfield)
>>>>>> - explicitly assigned values to all other vp9 enums
>>>>>>
>>>>>> Apply suggestion from Nicolas's review (thanks, Nicolas):
>>>>>> - explicitly stated that the v4l2_ctrl_vp9_compressed_hdr control is optional
>>>>>> and implemented only by drivers which need it
>>>>>>
>>>>>> Changes related to the RFC v2:
>>>>>>
>>>>>> - added another driver including a postprocessor to de-tile
>>>>>>            codec-specific tiling
>>>>>> - reworked uAPI structs layout to follow VP8 style
>>>>>> - changed validation of loop filter params
>>>>>> - changed validation of segmentation params
>>>>>> - changed validation of VP9 frame params
>>>>>> - removed level lookup array from loop filter struct
>>>>>>            (can be computed by drivers)
>>>>>> - renamed some enum values to match the spec more closely
>>>>>> - V4L2 VP9 library changed the 'eob' member of
>>>>>>            'struct v4l2_vp9_frame_symbol_counts' so that it is an array
>>>>>>            of pointers instead of an array of pointers to arrays
>>>>>>            (IPs such as g2 creatively pass parts of the 'eob' counts in
>>>>>>            the 'coeff' counts)
>>>>>> - factored out several repeated portions of code
>>>>>> - minor nitpicks and cleanups
>>>>>>
>>>>>> Andrzej Pietrasiewicz (6):
>>>>>>      media: uapi: Add VP9 stateless decoder controls
>>>>>>      media: Add VP9 v4l2 library
>>>>>>      media: hantro: Rename registers
>>>>>>      media: hantro: Prepare for other G2 codecs
>>>>>>      media: hantro: Support VP9 on the G2 core
>>>>>>      media: hantro: Support NV12 on the G2 core
>>>>>>
>>>>>> Boris Brezillon (1):
>>>>>>      media: rkvdec: Add the VP9 backend
>>>>>>
>>>>>> Ezequiel Garcia (4):
>>>>>>      hantro: postproc: Fix motion vector space size
>>>>>>      hantro: postproc: Introduce struct hantro_postproc_ops
>>>>>>      hantro: Simplify postprocessor
>>>>>>      hantro: Add quirk for NV12/NV12_4L4 capture format
>>>>>>
>>>>>>     .../userspace-api/media/v4l/biblio.rst        |   10 +
>>>>>>     .../media/v4l/ext-ctrls-codec-stateless.rst   |  573 +++++
>>>>>>     .../media/v4l/pixfmt-compressed.rst           |   15 +
>>>>>>     .../media/v4l/vidioc-g-ext-ctrls.rst          |    8 +
>>>>>>     .../media/v4l/vidioc-queryctrl.rst            |   12 +
>>>>>>     .../media/videodev2.h.rst.exceptions          |    2 +
>>>>>>     drivers/media/v4l2-core/Kconfig               |    4 +
>>>>>>     drivers/media/v4l2-core/Makefile              |    1 +
>>>>>>     drivers/media/v4l2-core/v4l2-ctrls-core.c     |  180 ++
>>>>>>     drivers/media/v4l2-core/v4l2-ctrls-defs.c     |    8 +
>>>>>>     drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>>>>>>     drivers/media/v4l2-core/v4l2-vp9.c            | 1850 +++++++++++++++++
>>>>>>     drivers/staging/media/hantro/Kconfig          |    1 +
>>>>>>     drivers/staging/media/hantro/Makefile         |    7 +-
>>>>>>     drivers/staging/media/hantro/hantro.h         |   40 +-
>>>>>>     drivers/staging/media/hantro/hantro_drv.c     |   23 +-
>>>>>>     drivers/staging/media/hantro/hantro_g2.c      |   27 +
>>>>>>     .../staging/media/hantro/hantro_g2_hevc_dec.c |   69 +-
>>>>>>     drivers/staging/media/hantro/hantro_g2_regs.h |  132 +-
>>>>>>     .../staging/media/hantro/hantro_g2_vp9_dec.c  |  980 +++++++++
>>>>>>     drivers/staging/media/hantro/hantro_hw.h      |   83 +-
>>>>>>     .../staging/media/hantro/hantro_postproc.c    |   79 +-
>>>>>>     drivers/staging/media/hantro/hantro_v4l2.c    |   20 +
>>>>>>     drivers/staging/media/hantro/hantro_vp9.c     |  240 +++
>>>>>>     drivers/staging/media/hantro/hantro_vp9.h     |  103 +
>>>>>>     drivers/staging/media/hantro/imx8m_vpu_hw.c   |   38 +-
>>>>>>     .../staging/media/hantro/rockchip_vpu_hw.c    |    7 +-
>>>>>>     .../staging/media/hantro/sama5d4_vdec_hw.c    |    3 +-
>>>>>>     drivers/staging/media/rkvdec/Kconfig          |    1 +
>>>>>>     drivers/staging/media/rkvdec/Makefile         |    2 +-
>>>>>>     drivers/staging/media/rkvdec/rkvdec-vp9.c     | 1078 ++++++++++
>>>>>>     drivers/staging/media/rkvdec/rkvdec.c         |   52 +-
>>>>>>     drivers/staging/media/rkvdec/rkvdec.h         |   12 +-
>>>>>>     include/media/v4l2-ctrls.h                    |    4 +
>>>>>>     include/media/v4l2-vp9.h                      |  182 ++
>>>>>>     include/uapi/linux/v4l2-controls.h            |  284 +++
>>>>>>     include/uapi/linux/videodev2.h                |    6 +
>>>>>>     37 files changed, 6033 insertions(+), 104 deletions(-)
>>>>>>     create mode 100644 drivers/media/v4l2-core/v4l2-vp9.c
>>>>>>     create mode 100644 drivers/staging/media/hantro/hantro_g2.c
>>>>>>     create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c
>>>>>>     create mode 100644 drivers/staging/media/hantro/hantro_vp9.c
>>>>>>     create mode 100644 drivers/staging/media/hantro/hantro_vp9.h
>>>>>>     create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>>>>>>     create mode 100644 include/media/v4l2-vp9.h
>>>>>>
>>>>>>
>>>>>> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
>>>>>>
>>>>>
>>>>
>>>
>>
>
Hans Verkuil Nov. 17, 2021, 9:59 a.m. UTC | #12
On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
> Hi,
> 
> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>> Hi Hans,
>>>
>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>> Hi Hans,
>>>>>
>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>> Andrzej,
>>>>>>
>>>>>> Can you rebase this series on top of the master branch of
>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>>> manner.
>>>>>
>>>>> This is a branch for you:
>>>>>
>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>
>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>
>>>
>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>> Some of the below are "false positives, namely:
>>>
>>> drivers/media/platform/omap3isp/omap3isp.h
>>> drivers/media/platform/qcom/venus/core.h
>>
>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>
>> Please post a v8. That way the series is archived on lore. And it works better
>> with patchwork.
> 
> Sure, no problem. Also please see below.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> which are not touched by the series.
>>>
>>> Regards,
>>>
>>> Andrzej
>>>
>>>> sparse:
>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>
>>>> smatch:
>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
> 
> this looks a false positive.
> 
> A portion of memory pointed to by ptr is indexed with i * 23 + m,
> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
> So the largest index value we compute equals 89 (3 * 23 + 20).
> Because ptr points to something that is at least 90 bytes large,
> 89 is a valid index and no greater index will be ever computed.

But we do need to get rid of this smatch warning, otherwise it will pollute the
list of smatch warnings.

I was looking at the code and wonder if it wouldn't make more sense to
move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:

        for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
                rkprobs->intra_mode[i / 23].uv_mode[i % 23] = v4l2_vp9_kf_uv_mode_prob[i];

Wouldn't that do the same as the current code? It looks simpler as well.

Regards,

	Hans
Andrzej Pietrasiewicz Nov. 17, 2021, 10:49 a.m. UTC | #13
Hi,

W dniu 17.11.2021 o 10:59, Hans Verkuil pisze:
> On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
>> Hi,
>>
>> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>>> Hi Hans,
>>>>
>>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>>> Andrzej,
>>>>>>>
>>>>>>> Can you rebase this series on top of the master branch of
>>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>>>> manner.
>>>>>>
>>>>>> This is a branch for you:
>>>>>>
>>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>>
>>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>>
>>>>
>>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>>> Some of the below are "false positives, namely:
>>>>
>>>> drivers/media/platform/omap3isp/omap3isp.h
>>>> drivers/media/platform/qcom/venus/core.h
>>>
>>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>>
>>> Please post a v8. That way the series is archived on lore. And it works better
>>> with patchwork.
>>
>> Sure, no problem. Also please see below.
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>> which are not touched by the series.
>>>>
>>>> Regards,
>>>>
>>>> Andrzej
>>>>
>>>>> sparse:
>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>>
>>>>> smatch:
>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
>>
>> this looks a false positive.
>>
>> A portion of memory pointed to by ptr is indexed with i * 23 + m,
>> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
>> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
>> So the largest index value we compute equals 89 (3 * 23 + 20).
>> Because ptr points to something that is at least 90 bytes large,
>> 89 is a valid index and no greater index will be ever computed.
> 
> But we do need to get rid of this smatch warning, otherwise it will pollute the
> list of smatch warnings.
> 
> I was looking at the code and wonder if it wouldn't make more sense to
> move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:
> 
>          for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
>                  rkprobs->intra_mode[i / 23].uv_mode[i % 23] = v4l2_vp9_kf_uv_mode_prob[i];
> 
> Wouldn't that do the same as the current code? It looks simpler as well.
> 

I think it would, but I would slightly change the loop:

	for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++) {
		const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob;

		rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i];
	}

because v4l2_vp9_kf_uv_mode_prob is actually a u8[10][9].

I will make such a change locally and test whether it causes regressions.

Once I confirm it works (and I expect I will) would you like me to post a v9,
only reply to the changed patch with its updated version or do you want to make 
this change yourself?

Andrzej
Andrzej Pietrasiewicz Nov. 17, 2021, 10:51 a.m. UTC | #14
Hi again,

W dniu 17.11.2021 o 11:49, Andrzej Pietrasiewicz pisze:
> Hi,
> 
> W dniu 17.11.2021 o 10:59, Hans Verkuil pisze:
>> On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
>>> Hi,
>>>
>>> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>>>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>>>> Hi Hans,
>>>>>
>>>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>>>> Andrzej,
>>>>>>>>
>>>>>>>> Can you rebase this series on top of the master branch of
>>>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>>>>> manner.
>>>>>>>
>>>>>>> This is a branch for you:
>>>>>>>
>>>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>>>
>>>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>>>
>>>>>
>>>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>>>> Some of the below are "false positives, namely:
>>>>>
>>>>> drivers/media/platform/omap3isp/omap3isp.h
>>>>> drivers/media/platform/qcom/venus/core.h
>>>>
>>>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>>>
>>>> Please post a v8. That way the series is archived on lore. And it works better
>>>> with patchwork.
>>>
>>> Sure, no problem. Also please see below.
>>>
>>>>
>>>> Regards,
>>>>
>>>>     Hans
>>>>
>>>>>
>>>>> which are not touched by the series.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Andrzej
>>>>>
>>>>>> sparse:
>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: 
>>>>>> symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>>>
>>>>>> smatch:
>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() 
>>>>>> error: buffer overflow 'ptr' 90 <= 91
>>>
>>> this looks a false positive.
>>>
>>> A portion of memory pointed to by ptr is indexed with i * 23 + m,
>>> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
>>> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
>>> So the largest index value we compute equals 89 (3 * 23 + 20).
>>> Because ptr points to something that is at least 90 bytes large,
>>> 89 is a valid index and no greater index will be ever computed.
>>
>> But we do need to get rid of this smatch warning, otherwise it will pollute the
>> list of smatch warnings.
>>
>> I was looking at the code and wonder if it wouldn't make more sense to
>> move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:
>>
>>          for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
>>                  rkprobs->intra_mode[i / 23].uv_mode[i % 23] = 
>> v4l2_vp9_kf_uv_mode_prob[i];
>>
>> Wouldn't that do the same as the current code? It looks simpler as well.
>>
> 
> I think it would, but I would slightly change the loop:
>
>      for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++) {

actually, sizeof(v4l2_vp9_kf_uv_mode_prob)



>          const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob;
> 
>          rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i];
>      }
> 
> because v4l2_vp9_kf_uv_mode_prob is actually a u8[10][9].
> 
> I will make such a change locally and test whether it causes regressions.
> 
> Once I confirm it works (and I expect I will) would you like me to post a v9,
> only reply to the changed patch with its updated version or do you want to make 
> this change yourself?
> 
> Andrzej
Andrzej Pietrasiewicz Nov. 17, 2021, 11:33 a.m. UTC | #15
Hi Hans,

W dniu 17.11.2021 o 11:51, Andrzej Pietrasiewicz pisze:
> Hi again,
> 
> W dniu 17.11.2021 o 11:49, Andrzej Pietrasiewicz pisze:
>> Hi,
>>
>> W dniu 17.11.2021 o 10:59, Hans Verkuil pisze:
>>> On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
>>>> Hi,
>>>>
>>>> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>>>>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>>>>> Hi Hans,
>>>>>>>>
>>>>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>>>>> Andrzej,
>>>>>>>>>
>>>>>>>>> Can you rebase this series on top of the master branch of
>>>>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a 
>>>>>>>>> non-trivial
>>>>>>>>> manner.
>>>>>>>>
>>>>>>>> This is a branch for you:
>>>>>>>>
>>>>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>>>>
>>>>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>>>>
>>>>>>
>>>>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>>>>> Some of the below are "false positives, namely:
>>>>>>
>>>>>> drivers/media/platform/omap3isp/omap3isp.h
>>>>>> drivers/media/platform/qcom/venus/core.h
>>>>>
>>>>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>>>>
>>>>> Please post a v8. That way the series is archived on lore. And it works better
>>>>> with patchwork.
>>>>
>>>> Sure, no problem. Also please see below.
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>>     Hans
>>>>>
>>>>>>
>>>>>> which are not touched by the series.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Andrzej
>>>>>>
>>>>>>> sparse:
>>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>>> used [-Wunused-but-set-variable]
>>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>>> used [-Wunused-but-set-variable]
>>>>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: 
>>>>>>> symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>>>>
>>>>>>> smatch:
>>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>>> used [-Wunused-but-set-variable]
>>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>>> used [-Wunused-but-set-variable]
>>>>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() 
>>>>>>> error: buffer overflow 'ptr' 90 <= 91
>>>>
>>>> this looks a false positive.
>>>>
>>>> A portion of memory pointed to by ptr is indexed with i * 23 + m,
>>>> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
>>>> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
>>>> So the largest index value we compute equals 89 (3 * 23 + 20).
>>>> Because ptr points to something that is at least 90 bytes large,
>>>> 89 is a valid index and no greater index will be ever computed.
>>>
>>> But we do need to get rid of this smatch warning, otherwise it will pollute the
>>> list of smatch warnings.
>>>
>>> I was looking at the code and wonder if it wouldn't make more sense to
>>> move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:
>>>
>>>          for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
>>>                  rkprobs->intra_mode[i / 23].uv_mode[i % 23] = 
>>> v4l2_vp9_kf_uv_mode_prob[i];
>>>
>>> Wouldn't that do the same as the current code? It looks simpler as well.
>>>
>>
>> I think it would, but I would slightly change the loop:
>>
>>      for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++) {
> 
> actually, sizeof(v4l2_vp9_kf_uv_mode_prob)
> 
> 
> 
>>          const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob;
>>
>>          rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i];
>>      }
>>
>> because v4l2_vp9_kf_uv_mode_prob is actually a u8[10][9].
>>
>> I will make such a change locally and test whether it causes regressions.

This worked, no regressions:

	for (i = 0; i < sizeof(v4l2_vp9_kf_uv_mode_prob); ++i) {
		const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob;

		rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i];
	}

Andrzej