| 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
| ||
|
Date: Wed, 15 May 2024 13:31:24 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>,
ezequiel@...guardiasur.com.ar, p.zabel@...gutronix.de, mchehab@...nel.org
Cc: linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH] media: verisilicon: Add reference buffers compression
Hi,
Le mardi 14 mai 2024 à 18:20 +0200, Benjamin Gaignard a écrit :
> Reference frame compression is a feature added in G2 decoder to compress
> frame buffers so that the bandwidth of storing/loading reference frames
> can be reduced, especially when the resolution of decoded stream is of
> high definition.
>
> The impact of compressed frames is confirmed when using perf to monitor
> the number of memory accesses with or without compression feature.
> The following command
> perf stat -a -e imx8_ddr0/cycles/,imx8_ddr0/read-cycles/,imx8_ddr0/write-cycles/ gst-launch-1.0 filesrc location=Jockey_3840x2160_120fps_420_8bit_HEVC_RAW.hevc ! queue ! h265parse ! v4l2slh265dec ! fakesink
>
> give us these results
> without compression feature:
> Performance counter stats for 'system wide':
>
> 1711300345 imx8_ddr0/cycles/
> 892207924 imx8_ddr0/read-cycles/
> 1291785864 imx8_ddr0/write-cycles/
>
> 13.760048353 seconds time elapsed
>
> with compression feature:
> Performance counter stats for 'system wide':
>
> 274526799 imx8_ddr0/cycles/
> 453120194 imx8_ddr0/read-cycles/
> 833391434 imx8_ddr0/write-cycles/
>
> 18.257831534 seconds time elapsed
On my own testing the duration remained the same. Perhaps you had something else
running during your test ? I would suggest to always runs multiple time such a
test before sharing to avoid snapshot effect and possibly wrong interpretation.
regards,
Nicolas
>
> As expected the number of read/write cycles are really lower when compression
> is used.
>
> Since storing compression data requires more memory a module
> parameter named 'hevc_use_compression' is used to enable/disable
> this feature and, by default, compression isn't used.
Perhaps a KConfig to change the default ?
>
> Enabling compression feature means that decoder output frames
> are stored with a specific compression pixel format. Since this
> pixel format is unknown, this patch restrain compression feature
> usage to the cases where post-processor pixels formats (NV12 or NV15)
> are selected by the applications.
>
> Fluster compliance HEVC test suite score is still 141/147 after this patch.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> ---
> .../media/platform/verisilicon/hantro_g2.c | 35 +++++++++++++++++
> .../platform/verisilicon/hantro_g2_hevc_dec.c | 20 ++++++++--
> .../platform/verisilicon/hantro_g2_regs.h | 4 ++
> .../media/platform/verisilicon/hantro_hevc.c | 8 ++++
> .../media/platform/verisilicon/hantro_hw.h | 39 +++++++++++++++++++
> .../platform/verisilicon/hantro_postproc.c | 6 ++-
> 6 files changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_g2.c b/drivers/media/platform/verisilicon/hantro_g2.c
> index b880a6849d58..62ca91427360 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2.c
> @@ -9,6 +9,12 @@
> #include "hantro_g2_regs.h"
>
> #define G2_ALIGN 16
> +#define CBS_SIZE 16 /* compression table size in bytes */
> +#define CBS_LUMA 8 /* luminance CBS is composed of 1 8x8 coded block */
> +#define CBS_CHROMA_W (8 * 2) /* chrominance CBS is composed of two 8x4 coded
> + * blocks, with Cb CB first then Cr CB following
> + */
> +#define CBS_CHROMA_H 4
>
> void hantro_g2_check_idle(struct hantro_dev *vpu)
> {
> @@ -56,3 +62,32 @@ size_t hantro_g2_motion_vectors_offset(struct hantro_ctx *ctx)
>
> return ALIGN((cr_offset * 3) / 2, G2_ALIGN);
> }
> +
> +static size_t hantro_g2_mv_size(struct hantro_ctx *ctx)
> +{
> + const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
> + const struct v4l2_ctrl_hevc_sps *sps = ctrls->sps;
> + unsigned int pic_width_in_ctbs, pic_height_in_ctbs;
> + unsigned int max_log2_ctb_size;
> +
> + max_log2_ctb_size = sps->log2_min_luma_coding_block_size_minus3 + 3 +
> + sps->log2_diff_max_min_luma_coding_block_size;
> + pic_width_in_ctbs = (sps->pic_width_in_luma_samples +
> + (1 << max_log2_ctb_size) - 1) >> max_log2_ctb_size;
> + pic_height_in_ctbs = (sps->pic_height_in_luma_samples + (1 << max_log2_ctb_size) - 1)
> + >> max_log2_ctb_size;
> +
> + return pic_width_in_ctbs * pic_height_in_ctbs * (1 << (2 * (max_log2_ctb_size - 4))) * 16;
> +}
> +
> +size_t hantro_g2_luma_compress_offset(struct hantro_ctx *ctx)
> +{
> + return hantro_g2_motion_vectors_offset(ctx) +
> + hantro_g2_mv_size(ctx);
> +}
> +
> +size_t hantro_g2_chroma_compress_offset(struct hantro_ctx *ctx)
> +{
> + return hantro_g2_luma_compress_offset(ctx) +
> + hantro_hevc_luma_compressed_size(ctx->dst_fmt.width, ctx->dst_fmt.height);
> +}
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> index d3f8c33eb16c..85a44143b378 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> @@ -367,11 +367,14 @@ static int set_ref(struct hantro_ctx *ctx)
> const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params;
> const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb;
> dma_addr_t luma_addr, chroma_addr, mv_addr = 0;
> + dma_addr_t compress_luma_addr, compress_chroma_addr = 0;
> struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *vb2_dst;
> struct hantro_decoded_buffer *dst;
> size_t cr_offset = hantro_g2_chroma_offset(ctx);
> size_t mv_offset = hantro_g2_motion_vectors_offset(ctx);
> + size_t compress_luma_offset = hantro_g2_luma_compress_offset(ctx);
> + size_t compress_chroma_offset = hantro_g2_chroma_compress_offset(ctx);
> u32 max_ref_frames;
> u16 dpb_longterm_e;
> static const struct hantro_reg cur_poc[] = {
> @@ -445,6 +448,8 @@ static int set_ref(struct hantro_ctx *ctx)
>
> chroma_addr = luma_addr + cr_offset;
> mv_addr = luma_addr + mv_offset;
> + compress_luma_addr = luma_addr + compress_luma_offset;
> + compress_chroma_addr = luma_addr + compress_chroma_offset;
>
> if (dpb[i].flags & V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
> dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
> @@ -452,6 +457,8 @@ static int set_ref(struct hantro_ctx *ctx)
> hantro_write_addr(vpu, G2_REF_LUMA_ADDR(i), luma_addr);
> hantro_write_addr(vpu, G2_REF_CHROMA_ADDR(i), chroma_addr);
> hantro_write_addr(vpu, G2_REF_MV_ADDR(i), mv_addr);
> + hantro_write_addr(vpu, G2_REF_COMP_LUMA_ADDR(i), compress_luma_addr);
> + hantro_write_addr(vpu, G2_REF_COMP_CHROMA_ADDR(i), compress_chroma_addr);
> }
>
> vb2_dst = hantro_get_dst_buf(ctx);
> @@ -465,19 +472,27 @@ static int set_ref(struct hantro_ctx *ctx)
>
> chroma_addr = luma_addr + cr_offset;
> mv_addr = luma_addr + mv_offset;
> + compress_luma_addr = luma_addr + compress_luma_offset;
> + compress_chroma_addr = luma_addr + compress_chroma_offset;
>
> hantro_write_addr(vpu, G2_REF_LUMA_ADDR(i), luma_addr);
> hantro_write_addr(vpu, G2_REF_CHROMA_ADDR(i), chroma_addr);
> - hantro_write_addr(vpu, G2_REF_MV_ADDR(i++), mv_addr);
> + hantro_write_addr(vpu, G2_REF_MV_ADDR(i), mv_addr);
> + hantro_write_addr(vpu, G2_REF_COMP_LUMA_ADDR(i), compress_luma_addr);
> + hantro_write_addr(vpu, G2_REF_COMP_CHROMA_ADDR(i++), compress_chroma_addr);
>
> hantro_write_addr(vpu, G2_OUT_LUMA_ADDR, luma_addr);
> hantro_write_addr(vpu, G2_OUT_CHROMA_ADDR, chroma_addr);
> hantro_write_addr(vpu, G2_OUT_MV_ADDR, mv_addr);
> + hantro_write_addr(vpu, G2_OUT_COMP_LUMA_ADDR, compress_luma_addr);
> + hantro_write_addr(vpu, G2_OUT_COMP_CHROMA_ADDR, compress_chroma_addr);
>
> for (; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) {
> hantro_write_addr(vpu, G2_REF_LUMA_ADDR(i), 0);
> hantro_write_addr(vpu, G2_REF_CHROMA_ADDR(i), 0);
> hantro_write_addr(vpu, G2_REF_MV_ADDR(i), 0);
> + hantro_write_addr(vpu, G2_REF_COMP_LUMA_ADDR(i), 0);
> + hantro_write_addr(vpu, G2_REF_COMP_CHROMA_ADDR(i), 0);
> }
>
> hantro_reg_write(vpu, &g2_refer_lterm_e, dpb_longterm_e);
> @@ -594,8 +609,7 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
> /* Don't disable output */
> hantro_reg_write(vpu, &g2_out_dis, 0);
>
> - /* Don't compress buffers */
> - hantro_reg_write(vpu, &g2_ref_compress_bypass, 1);
> + hantro_reg_write(vpu, &g2_ref_compress_bypass, !ctx->hevc_dec.use_compression);
>
> /* Bus width and max burst */
> hantro_reg_write(vpu, &g2_buswidth, BUS_WIDTH_128);
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_regs.h b/drivers/media/platform/verisilicon/hantro_g2_regs.h
> index 82606783591a..b943b1816db7 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_regs.h
> +++ b/drivers/media/platform/verisilicon/hantro_g2_regs.h
> @@ -318,6 +318,10 @@
> #define G2_TILE_BSD_ADDR (G2_SWREG(183))
> #define G2_DS_DST (G2_SWREG(186))
> #define G2_DS_DST_CHR (G2_SWREG(188))
> +#define G2_OUT_COMP_LUMA_ADDR (G2_SWREG(190))
> +#define G2_REF_COMP_LUMA_ADDR(i) (G2_SWREG(192) + ((i) * 0x8))
> +#define G2_OUT_COMP_CHROMA_ADDR (G2_SWREG(224))
> +#define G2_REF_COMP_CHROMA_ADDR(i) (G2_SWREG(226) + ((i) * 0x8))
>
> #define g2_strm_buffer_len G2_DEC_REG(258, 0, 0xffffffff)
> #define g2_strm_start_offset G2_DEC_REG(259, 0, 0xffffffff)
> diff --git a/drivers/media/platform/verisilicon/hantro_hevc.c b/drivers/media/platform/verisilicon/hantro_hevc.c
> index 2c14330bc562..895dc0c76c74 100644
> --- a/drivers/media/platform/verisilicon/hantro_hevc.c
> +++ b/drivers/media/platform/verisilicon/hantro_hevc.c
> @@ -25,6 +25,11 @@
> #define MAX_TILE_COLS 20
> #define MAX_TILE_ROWS 22
>
> +bool hevc_use_compression;
> +module_param_named(hevc_use_compression, hevc_use_compression, bool, 0644);
> +MODULE_PARM_DESC(hevc_use_compression,
> + "Use reference frame compression for HEVC");
> +
> void hantro_hevc_ref_init(struct hantro_ctx *ctx)
> {
> struct hantro_hevc_dec_hw_ctx *hevc_dec = &ctx->hevc_dec;
> @@ -275,5 +280,8 @@ int hantro_hevc_dec_init(struct hantro_ctx *ctx)
>
> hantro_hevc_ref_init(ctx);
>
> + hevc_dec->use_compression =
> + hevc_use_compression & hantro_needs_postproc(ctx, ctx->vpu_dst_fmt);
> +
> return 0;
> }
> diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
> index 7737320cc8cc..43d4ff637376 100644
> --- a/drivers/media/platform/verisilicon/hantro_hw.h
> +++ b/drivers/media/platform/verisilicon/hantro_hw.h
> @@ -42,6 +42,14 @@
>
> #define MAX_POSTPROC_BUFFERS 64
>
> +#define G2_ALIGN 16
> +#define CBS_SIZE 16 /* compression table size in bytes */
> +#define CBS_LUMA 8 /* luminance CBS is composed of 1 8x8 coded block */
> +#define CBS_CHROMA_W (8 * 2) /* chrominance CBS is composed of two 8x4 coded
> + * blocks, with Cb CB first then Cr CB following
> + */
> +#define CBS_CHROMA_H 4
> +
> struct hantro_dev;
> struct hantro_ctx;
> struct hantro_buf;
> @@ -144,6 +152,7 @@ struct hantro_hevc_dec_ctrls {
> * @ref_bufs_used: Bitfield of used reference buffers
> * @ctrls: V4L2 controls attached to a run
> * @num_tile_cols_allocated: number of allocated tiles
> + * @use_compression: use reference buffer compression
> */
> struct hantro_hevc_dec_hw_ctx {
> struct hantro_aux_buf tile_sizes;
> @@ -156,6 +165,7 @@ struct hantro_hevc_dec_hw_ctx {
> u32 ref_bufs_used;
> struct hantro_hevc_dec_ctrls ctrls;
> unsigned int num_tile_cols_allocated;
> + bool use_compression;
> };
>
> /**
> @@ -510,6 +520,33 @@ hantro_hevc_mv_size(unsigned int width, unsigned int height)
> return width * height / 16;
> }
>
> +static inline size_t
> +hantro_hevc_luma_compressed_size(unsigned int width, unsigned int height)
> +{
> + u32 pic_width_in_cbsy =
> + round_up((width + CBS_LUMA - 1) / CBS_LUMA, CBS_SIZE);
> + u32 pic_height_in_cbsy = (height + CBS_LUMA - 1) / CBS_LUMA;
> +
> + return round_up(pic_width_in_cbsy * pic_height_in_cbsy, CBS_SIZE);
> +}
> +
> +static inline size_t
> +hantro_hevc_chroma_compressed_size(unsigned int width, unsigned int height)
> +{
> + u32 pic_width_in_cbsc =
> + round_up((width + CBS_CHROMA_W - 1) / CBS_CHROMA_W, CBS_SIZE);
> + u32 pic_height_in_cbsc = (height / 2 + CBS_CHROMA_H - 1) / CBS_CHROMA_H;
> +
> + return round_up(pic_width_in_cbsc * pic_height_in_cbsc, CBS_SIZE);
> +}
> +
> +static inline size_t
> +hantro_hevc_compressed_size(unsigned int width, unsigned int height)
> +{
> + return hantro_hevc_luma_compressed_size(width, height) +
> + hantro_hevc_chroma_compressed_size(width, height);
> +}
> +
> static inline unsigned short hantro_av1_num_sbs(unsigned short dimension)
> {
> return DIV_ROUND_UP(dimension, 64);
> @@ -525,6 +562,8 @@ hantro_av1_mv_size(unsigned int width, unsigned int height)
>
> size_t hantro_g2_chroma_offset(struct hantro_ctx *ctx);
> size_t hantro_g2_motion_vectors_offset(struct hantro_ctx *ctx);
> +size_t hantro_g2_luma_compress_offset(struct hantro_ctx *ctx);
> +size_t hantro_g2_chroma_compress_offset(struct hantro_ctx *ctx);
>
> int hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx);
> int rockchip_vpu2_mpeg2_dec_run(struct hantro_ctx *ctx);
> diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c
> index 41e93176300b..232c93eea7ee 100644
> --- a/drivers/media/platform/verisilicon/hantro_postproc.c
> +++ b/drivers/media/platform/verisilicon/hantro_postproc.c
> @@ -213,9 +213,13 @@ static unsigned int hantro_postproc_buffer_size(struct hantro_ctx *ctx)
> else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
> buf_size += hantro_vp9_mv_size(pix_mp.width,
> pix_mp.height);
> - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
> + else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE) {
> buf_size += hantro_hevc_mv_size(pix_mp.width,
> pix_mp.height);
> + if (ctx->hevc_dec.use_compression)
> + buf_size += hantro_hevc_compressed_size(pix_mp.width,
> + pix_mp.height);
> + }
> else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_AV1_FRAME)
> buf_size += hantro_av1_mv_size(pix_mp.width,
> pix_mp.height);
Powered by blists - more mailing lists