Message-ID: <1627e9d3-7008-dda5-19d1-251c4715af9e@xs4all.nl>
Date:   Thu, 3 Jun 2021 11:19:11 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        ezequiel@...labora.com, p.zabel@...gutronix.de, mchehab@...nel.org,
        shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
        gregkh@...uxfoundation.org, mripard@...nel.org,
        paul.kocialkowski@...tlin.com, wens@...e.org,
        jernej.skrabec@...l.net, emil.l.velikov@...il.com,
        andrzej.p@...labora.com
Cc:     kernel@...gutronix.de, linux-imx@....com,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ

Hi Benjamin,

On 26/05/2021 14:45, Benjamin Gaignard wrote:
> The IMX8MQ got two VPUs but until now only G1 has been enabled.
> This series aim to add the second VPU (aka G2) and provide basic 
> HEVC decoding support.
> 
> To be able to decode HEVC it is needed to add/update some of the
> structures in the uapi. In addition of them one HANTRO dedicated
> control is required to inform the driver of the number of bits to skip
> at the beginning of the slice header.
> The hardware require to allocate few auxiliary buffers to store the
> references frame or tile size data.
> 
> The driver has been tested with fluster test suite stream.
> For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0
> 
> version 12:
>  - Change macro to avoid the final ';'
>  - Made arrays static and const
>  - Distinguish G2 generic fields (i.e these who could be reused for VP9)
>    from HEVC dedicated fields.

I ran the latest checkpatch/smatch/sparse etc. over this series and got a lot
of issues.

The trickiest first:

I get many reports for the register definitions in hantro_g2_regs.h:

drivers/staging/media/hantro/hantro_g2_regs.h:16:33: warning: 'g2_strm_start_offset' defined but not used [-Wunused-const-variable=]

I think it is a bad idea to declare variables in a header, and that's really
what is causing this.

I think it should be possible to rework this to defines such as:

#define G2_DEC_REG(b, s, m) \
        (const struct hantro_reg) { \
                .base = G2_SWREG(b), \
                .shift = s, \
                .mask = m, \
        }

#define g2_strm_start_offset G2_DEC_REG(259, 0, 0xffffffff)

This allows you to do:

hantro_reg_write(vpu, &g2_strm_start_offset, 0);

without having to declare static variables in a header.

Other warnings:

>From the compiler:

linux-git-arm-multi: WARNINGS

drivers/staging/media/hantro/hantro_g2_hevc_dec.c: In function 'hantro_write_addr':
drivers/staging/media/hantro/hantro_g2_hevc_dec.c:23:24: warning: right shift count >= width of type [-Wshift-count-overflow]
   23 |  vdpu_write(vpu, (addr >> 32) & 0xffffffff, offset + 4);
      |                        ^~

Note: this builds on a 32-bit arm!

>From smatch:

drivers/staging/media/hantro/hantro_hevc.c:228 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_sao.cpu'
drivers/staging/media/hantro/hantro_hevc.c:234 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_bsd.cpu'

>From kerneldoc:

drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs' not described in 'hantro_hevc_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_poc' not described in 'hantro_hevc_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'ref_bufs_used' not described in 'hantro_hevc_dec_hw_ctx'
drivers/staging/media/hantro/hantro_hw.h:136: warning: Function parameter or member 'num_tile_cols_allocated' not described in
'hantro_hevc_dec_hw_ctx'

Regards,

	Hans

> 
> version 11:
>  - Rebased on media_stage.
>  - Fix minor typo/remarks.
> 
> version 10:
>  - Shorter version of the previous series without ctrl block patches
>    and no DT modifications.
>    The scope of this series is limited to HEVC support.
> 
> version 9:
>  - Corrections in commits messages.
>  - Define the dedicated control in hevc-controls.h
>  - Add note in documentation.
>  - Change max value of the dedicated control.
>  - Rebased on media_tree/master branch.
> 
> version 8:
>  - Add reviewed-by and ack-by tags 
>  - Fix the warnings reported by kernel test robot
>  - Only patch 9 (adding dedicated control), patch 11 (HEVC support) and
>    patch 13 (DT changes) are still missing of review/ack tag.
> 
> version 7:
>  - Remove 'q' from syscon phandle name to make usable for iMX8MM too.
>    Update the bindings documentation.
>  - Add review/ack tags.
>  - Rebase on top of media_tree/master
>  - Be more accurate when computing the size of the memory needed motion
>    vectors.
>  - Explain why the all clocks need to set in the both DT node.
> 
> version 6:
>  - fix the errors reported by kernel test robot
> 
> version 5:
>  - use syscon instead of VPU reset driver.
>  - Do not break kernel/DT backward compatibility.
>  - Add documentation for dedicated Hantro control.
>  - Fix the remarks done by Ezequeil (typo, comments, unused function)
>  - Run v4l2-compliance without errors (see below).
>  - Do not add field to distinguish version, check postproc reg instead
> 
> version 4:
> - Split the changes in hevc controls in 2 commits to make them easier to
>   review.
> - Change hantro_codec_ops run() prototype to return errors   
> - Hantro v4l2 dedicated control is now only an integer
> - rebase on top of VPU reset changes posted here:
>   https://www.spinics.net/lists/arm-kernel/msg878440.html
> - Various fix from previous remarks
> - Limit the modifications in API to what the driver needs
> 
> version 3:
> - Fix typo in Hantro v4l2 dedicated control
> - Add documentation for the new structures and fields
> - Rebased on top of media_tree for-linus-5.12-rc1 tag
> 
> version 2:
> - remove all change related to scaling
> - squash commits to a coherent split
> - be more verbose about the added fields
> - fix the comments done by Ezequiel about dma_alloc_coherent usage
> - fix Dan's comments about control copy, reverse the test logic
> in tile_buffer_reallocate, rework some goto and return cases.
> - be more verbose about why I change the bindings
> - remove all sign-off expect mime since it is confusing
> - remove useless clocks in VPUs nodes
> 
> Benjamin Gaignard (9):
>   media: hevc: Add fields and flags for hevc PPS
>   media: hevc: Add decode params control
>   media: hantro: change hantro_codec_ops run prototype to return errors
>   media: hantro: Define HEVC codec profiles and supported features
>   media: hantro: Only use postproc when post processed formats are
>     defined
>   media: uapi: Add a control for HANTRO driver
>   media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control
>   media: hantro: Introduce G2/HEVC decoder
>   media: hantro: IMX8M: add variant for G2/HEVC codec
> 
>  .../userspace-api/media/drivers/hantro.rst    |  19 +
>  .../userspace-api/media/drivers/index.rst     |   1 +
>  .../media/v4l/ext-ctrls-codec.rst             | 108 +++-
>  .../media/v4l/vidioc-queryctrl.rst            |   6 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  21 +-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |   4 +
>  drivers/staging/media/hantro/Makefile         |   2 +
>  drivers/staging/media/hantro/hantro.h         |  13 +-
>  drivers/staging/media/hantro/hantro_drv.c     |  99 ++-
>  .../staging/media/hantro/hantro_g1_h264_dec.c |  10 +-
>  .../media/hantro/hantro_g1_mpeg2_dec.c        |   4 +-
>  .../staging/media/hantro/hantro_g1_vp8_dec.c  |   6 +-
>  .../staging/media/hantro/hantro_g2_hevc_dec.c | 587 ++++++++++++++++++
>  drivers/staging/media/hantro/hantro_g2_regs.h | 206 ++++++
>  .../staging/media/hantro/hantro_h1_jpeg_enc.c |   4 +-
>  drivers/staging/media/hantro/hantro_hevc.c    | 327 ++++++++++
>  drivers/staging/media/hantro/hantro_hw.h      |  69 +-
>  .../staging/media/hantro/hantro_postproc.c    |  14 +
>  drivers/staging/media/hantro/hantro_v4l2.c    |   5 +-
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   |  96 ++-
>  .../media/hantro/rk3399_vpu_hw_jpeg_enc.c     |   4 +-
>  .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |   4 +-
>  .../media/hantro/rk3399_vpu_hw_vp8_dec.c      |   6 +-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   6 +
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |   1 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   2 +
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  |  12 +-
>  include/media/hevc-ctrls.h                    |  46 +-
>  28 files changed, 1613 insertions(+), 69 deletions(-)
>  create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_regs.h
>  create mode 100644 drivers/staging/media/hantro/hantro_hevc.c
> 

Powered by blists - more mailing lists