linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
To: Hans Verkuil <hverkuil@xs4all.nl>,
	ezequiel@collabora.com, p.zabel@pengutronix.de,
	mchehab@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
	festevam@gmail.com, gregkh@linuxfoundation.org,
	mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org,
	jernej.skrabec@siol.net, emil.l.velikov@gmail.com,
	andrzej.p@collabora.com
Cc: kernel@pengutronix.de, linux-imx@nxp.com,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ
Date: Thu, 3 Jun 2021 13:39:01 +0200	[thread overview]
Message-ID: <fd6b117b-f662-b448-76d6-66d5c958f1dd@collabora.com> (raw)
In-Reply-To: <1627e9d3-7008-dda5-19d1-251c4715af9e@xs4all.nl>


Le 03/06/2021 à 11:19, Hans Verkuil a écrit :
> Hi Benjamin,
>
> On 26/05/2021 14:45, Benjamin Gaignard wrote:
>> The IMX8MQ got two VPUs but until now only G1 has been enabled.
>> This series aim to add the second VPU (aka G2) and provide basic
>> HEVC decoding support.
>>
>> To be able to decode HEVC it is needed to add/update some of the
>> structures in the uapi. In addition of them one HANTRO dedicated
>> control is required to inform the driver of the number of bits to skip
>> at the beginning of the slice header.
>> The hardware require to allocate few auxiliary buffers to store the
>> references frame or tile size data.
>>
>> The driver has been tested with fluster test suite stream.
>> For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0
>>
>> version 12:
>>   - Change macro to avoid the final ';'
>>   - Made arrays static and const
>>   - Distinguish G2 generic fields (i.e these who could be reused for VP9)
>>     from HEVC dedicated fields.
> I ran the latest checkpatch/smatch/sparse etc. over this series and got a lot
> of issues.
>
> The trickiest first:
>
> I get many reports for the register definitions in hantro_g2_regs.h:
>
> drivers/staging/media/hantro/hantro_g2_regs.h:16:33: warning: 'g2_strm_start_offset' defined but not used [-Wunused-const-variable=]
>
> I think it is a bad idea to declare variables in a header, and that's really
> what is causing this.
>
> I think it should be possible to rework this to defines such as:
>
> #define G2_DEC_REG(b, s, m) \
>          (const struct hantro_reg) { \
>                  .base = G2_SWREG(b), \
>                  .shift = s, \
>                  .mask = m, \
>          }

checkpatch complains about the macro to be complex so I have to add parentheses
but that will be in v12

> #define g2_strm_start_offset G2_DEC_REG(259, 0, 0xffffffff)
>
> This allows you to do:
>
> hantro_reg_write(vpu, &g2_strm_start_offset, 0);
>
> without having to declare static variables in a header.
>
> Other warnings:
>
>  From the compiler:
>
> linux-git-arm-multi: WARNINGS
>
> drivers/staging/media/hantro/hantro_g2_hevc_dec.c: In function 'hantro_write_addr':
> drivers/staging/media/hantro/hantro_g2_hevc_dec.c:23:24: warning: right shift count >= width of type [-Wshift-count-overflow]
>     23 |  vdpu_write(vpu, (addr >> 32) & 0xffffffff, offset + 4);
>        |                        ^~
>
> Note: this builds on a 32-bit arm!
>
>  From smatch:
>
> drivers/staging/media/hantro/hantro_hevc.c:228 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_sao.cpu'
> drivers/staging/media/hantro/hantro_hevc.c:234 tile_buffer_reallocate() error: double free of 'hevc_dec->tile_bsd.cpu'

That will be fixed in v12

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

I fix this and also remove the description for not existing fields.

Thanks for your review.
Benjamin

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

      reply	other threads:[~2021-06-03 11:39 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-26 12:45 [PATCH v12 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 1/9] media: hevc: Add fields and flags for hevc PPS Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 2/9] media: hevc: Add decode params control Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 3/9] media: hantro: change hantro_codec_ops run prototype to return errors Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 4/9] media: hantro: Define HEVC codec profiles and supported features Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 5/9] media: hantro: Only use postproc when post processed formats are defined Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 6/9] media: uapi: Add a control for HANTRO driver Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 7/9] media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 8/9] media: hantro: Introduce G2/HEVC decoder Benjamin Gaignard
2021-05-26 12:45 ` [PATCH v12 9/9] media: hantro: IMX8M: add variant for G2/HEVC codec Benjamin Gaignard
2021-05-27 13:36 ` [PATCH v12 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ Benjamin Gaignard
2021-06-03  9:19 ` Hans Verkuil
2021-06-03 11:39   ` Benjamin Gaignard [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fd6b117b-f662-b448-76d6-66d5c958f1dd@collabora.com \
    --to=benjamin.gaignard@collabora.com \
    --cc=andrzej.p@collabora.com \
    --cc=emil.l.velikov@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=festevam@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hverkuil@xs4all.nl \
    --cc=jernej.skrabec@siol.net \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=mripard@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).