linux-kernel - Re: [PATCH v2] media: hantro: Auto generate the AXI ID to avoid conflicts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YWfyiSUqNHnsGzzp@fedora>
Date:   Thu, 14 Oct 2021 06:04:09 -0300
From:   Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc:     p.zabel@...gutronix.de, mchehab@...nel.org,
        gregkh@...uxfoundation.org, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>
Subject: Re: [PATCH v2] media: hantro: Auto generate the AXI ID to avoid
 conflicts

Hi Benjamin,

Thanks for picking up the patch.

On Fri, Sep 24, 2021 at 03:24:47PM +0200, Benjamin Gaignard wrote:
> The AXI ID is an AXI bus configuration for improve bus performance. 
> If read and write operations use different ID the operations can be paralleled,
> whereas when they have the same ID the operations will be serialized. 
> Right now, the write ID is fixed to 0 but we can set it to 0xff to get auto
> generated ID to avoid possible conflicts.
> 
> This change has no functional changes, but seems reasonable to let the
> hardware to autogenerate the ID instead of hardcoding in software.
> 

Well, it may not have functional changes, but it is expected to have
performance impact on a contended AXI bus.

> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
> changes in version 2:
> - Add a macro with comment about the value.
> - Make VP8 and H264 codecs use the macro.
> - fluster tests on the both codecs show no regressions.
>   ./fluster.py run -ts VP8-TEST-VECTORS -d GStreamer-VP8-V4L2SL-Gst1.0
>   ./fluster.py run -ts JVT-AVC_V1 -d GStreamer-H.264-V4L2SL-Gst1.0
> - The both codec write other bits in the same configuration register
>   so the simplest solution is to use the macro in the both cases.
> 

Please describe the performance test that you did to make
sure it's not regressing.

>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>  drivers/staging/media/hantro/hantro_g1_regs.h     | 2 ++
>  drivers/staging/media/hantro/hantro_g1_vp8_dec.c  | 3 ++-

This covers VP8 and H264 on some of the cores (i.MX8 and Rockchip VPU1).
I believe you are missing the VPU2 cores (RK3399), see for instance
rockchip_vpu2_hw_h264_dec.c.

What about MPEG-2? If the tests show no performance impact,
then please do the change for MPEG-2 as well.

Thanks,
Ezequiel

>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 236ce24ca00c..f49dbfb8a843 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -29,7 +29,7 @@ static void set_params(struct hantro_ctx *ctx, struct vb2_v4l2_buffer *src_buf)
>  	u32 reg;
>  
>  	/* Decoder control register 0. */
> -	reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0x0);
> +	reg = G1_REG_DEC_CTRL0_DEC_AXI_AUTO;
>  	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>  		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>  	if (sps->profile_idc > 66) {
> diff --git a/drivers/staging/media/hantro/hantro_g1_regs.h b/drivers/staging/media/hantro/hantro_g1_regs.h
> index c1756e3d5391..c623b3b0be18 100644
> --- a/drivers/staging/media/hantro/hantro_g1_regs.h
> +++ b/drivers/staging/media/hantro/hantro_g1_regs.h
> @@ -68,6 +68,8 @@
>  #define     G1_REG_DEC_CTRL0_PICORD_COUNT_E		BIT(9)
>  #define     G1_REG_DEC_CTRL0_DEC_AHB_HLOCK_E		BIT(8)
>  #define     G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(x)		(((x) & 0xff) << 0)
> +/* Setting AXI ID to 0xff to get auto generated ID to avoid possible conflicts */
> +#define     G1_REG_DEC_CTRL0_DEC_AXI_AUTO		G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0xff)
>  #define G1_REG_DEC_CTRL1				0x010
>  #define     G1_REG_DEC_CTRL1_PIC_MB_WIDTH(x)		(((x) & 0x1ff) << 23)
>  #define     G1_REG_DEC_CTRL1_MB_WIDTH_OFF(x)		(((x) & 0xf) << 19)
> diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> index 6180b23e7d94..851eb67f19f5 100644
> --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> @@ -463,7 +463,8 @@ int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
>  	      G1_REG_CONFIG_DEC_MAX_BURST(16);
>  	vdpu_write_relaxed(vpu, reg, G1_REG_CONFIG);
>  
> -	reg = G1_REG_DEC_CTRL0_DEC_MODE(10);
> +	reg = G1_REG_DEC_CTRL0_DEC_MODE(10) |
> +	      G1_REG_DEC_CTRL0_DEC_AXI_AUTO;
>  	if (!V4L2_VP8_FRAME_IS_KEY_FRAME(hdr))
>  		reg |= G1_REG_DEC_CTRL0_PIC_INTER_E;
>  	if (!(hdr->flags & V4L2_VP8_FRAME_FLAG_MB_NO_SKIP_COEFF))
> -- 
> 2.30.2
>