linux-kernel - Re: [PATCH v2] media: hantro: HEVC: Fix reference frames management

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: <YnE+1mgOnWxaVpfg@eze-laptop>
Date:   Tue, 3 May 2022 11:40:22 -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, jon@...ocrew.net, aford173@...il.com,
        kernel@...labora.com
Subject: Re: [PATCH v2] media: hantro: HEVC: Fix reference frames management

On Tue, May 03, 2022 at 03:51:38PM +0200, Benjamin Gaignard wrote:
> PoC shall be int the range of -2^31 to 2^31 -1
> (HEVC spec section 8.3.1 Decoding process for picture order count).
> The current way to know if an entry in reference picture array is free
> is to test if PoC = UNUSED_REF. Since UNUSED_REF is defined as '-1' that
> could lead to decode issue if one PoC also equal '-1'.
> PoC with value = '-1' exists in conformance test SLIST_B_Sony_9.
> 
> Change the way unused entries are managed in reference pictures array to
> avoid using PoC to detect then.
> 
> This patch doesn't change fluster HEVC score.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>

Good catch!

Thanks,

Reviewed-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>

> ---
>  .../staging/media/hantro/hantro_g2_hevc_dec.c |  6 ++---
>  drivers/staging/media/hantro/hantro_hevc.c    | 27 +++----------------
>  drivers/staging/media/hantro/hantro_hw.h      |  2 +-
>  3 files changed, 6 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> index 0a8c01ff2fa7..b7835bbf5e98 100644
> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> @@ -473,8 +473,8 @@ static int set_ref(struct hantro_ctx *ctx)
>  
>  	set_ref_pic_list(ctx);
>  
> -	/* We will only keep the references picture that are still used */
> -	ctx->hevc_dec.ref_bufs_used = 0;
> +	/* We will only keep the references pictures that are still used */
> +	hantro_hevc_ref_init(ctx);
>  
>  	/* Set up addresses of DPB buffers */
>  	dpb_longterm_e = 0;
> @@ -515,8 +515,6 @@ static int set_ref(struct hantro_ctx *ctx)
>  	hantro_write_addr(vpu, G2_OUT_CHROMA_ADDR, chroma_addr);
>  	hantro_write_addr(vpu, G2_OUT_MV_ADDR, mv_addr);
>  
> -	hantro_hevc_ref_remove_unused(ctx);
> -
>  	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);
> diff --git a/drivers/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
> index 7d4b1d72255c..7fdec50dc853 100644
> --- a/drivers/staging/media/hantro/hantro_hevc.c
> +++ b/drivers/staging/media/hantro/hantro_hevc.c
> @@ -25,15 +25,11 @@
>  #define MAX_TILE_COLS 20
>  #define MAX_TILE_ROWS 22
>  
> -#define UNUSED_REF	-1
> -
> -static void hantro_hevc_ref_init(struct hantro_ctx *ctx)
> +void hantro_hevc_ref_init(struct hantro_ctx *ctx)
>  {
>  	struct hantro_hevc_dec_hw_ctx *hevc_dec = &ctx->hevc_dec;
> -	int i;
>  
> -	for (i = 0;  i < NUM_REF_PICTURES; i++)
> -		hevc_dec->ref_bufs_poc[i] = UNUSED_REF;
> +	hevc_dec->ref_bufs_used = 0;
>  }
>  
>  dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx,
> @@ -60,7 +56,7 @@ int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr)
>  
>  	/* Add a new reference buffer */
>  	for (i = 0; i < NUM_REF_PICTURES; i++) {
> -		if (hevc_dec->ref_bufs_poc[i] == UNUSED_REF) {
> +		if (!(hevc_dec->ref_bufs_used & 1 << i)) {
>  			hevc_dec->ref_bufs_used |= 1 << i;
>  			hevc_dec->ref_bufs_poc[i] = poc;
>  			hevc_dec->ref_bufs[i].dma = addr;
> @@ -71,23 +67,6 @@ int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr)
>  	return -EINVAL;
>  }
>  
> -void hantro_hevc_ref_remove_unused(struct hantro_ctx *ctx)
> -{
> -	struct hantro_hevc_dec_hw_ctx *hevc_dec = &ctx->hevc_dec;
> -	int i;
> -
> -	/* Just tag buffer as unused, do not free them */
> -	for (i = 0;  i < NUM_REF_PICTURES; i++) {
> -		if (hevc_dec->ref_bufs_poc[i] == UNUSED_REF)
> -			continue;
> -
> -		if (hevc_dec->ref_bufs_used & (1 << i))
> -			continue;
> -
> -		hevc_dec->ref_bufs_poc[i] = UNUSED_REF;
> -	}
> -}
> -
>  static int tile_buffer_reallocate(struct hantro_ctx *ctx)
>  {
>  	struct hantro_dev *vpu = ctx->dev;
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index 9f31cce609d6..5de558386179 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -337,9 +337,9 @@ int hantro_hevc_dec_init(struct hantro_ctx *ctx);
>  void hantro_hevc_dec_exit(struct hantro_ctx *ctx);
>  int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx);
>  int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx);
> +void hantro_hevc_ref_init(struct hantro_ctx *ctx);
>  dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx, s32 poc);
>  int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr);
> -void hantro_hevc_ref_remove_unused(struct hantro_ctx *ctx);
>  
>  static inline unsigned short hantro_vp9_num_sbs(unsigned short dimension)
>  {
> -- 
> 2.32.0
>