All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: rkvdec: Fix H264 scaling list order
@ 2020-05-22 20:21 Jonas Karlman
  2020-05-22 20:27 ` Nicolas Dufresne
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Jonas Karlman @ 2020-05-22 20:21 UTC (permalink / raw)
  To: Ezequiel Garcia, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Boris Brezillon, Tomasz Figa, Nicolas Dufresne, linux-media,
	linux-rockchip, linux-kernel, Jonas Karlman

The Rockchip Video Decoder driver is expecting that the values in a
scaling list are in zig-zag order and applies the inverse scanning process
to get the values in matrix order.

Commit 0b0393d59eb4 ("media: uapi: h264: clarify expected
scaling_list_4x4/8x8 order") clarified that the values in the scaling list
should already be in matrix order.

Fix this by removing the reordering and change to use two memcpy.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 70 +++++++---------------
 1 file changed, 22 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index cd4980d06be7..2719f0c66a4a 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -18,11 +18,16 @@
 /* Size with u32 units. */
 #define RKV_CABAC_INIT_BUFFER_SIZE	(3680 + 128)
 #define RKV_RPS_SIZE			((128 + 128) / 4)
-#define RKV_SCALING_LIST_SIZE		(6 * 16 + 6 * 64 + 128)
 #define RKV_ERROR_INFO_SIZE		(256 * 144 * 4)
 
 #define RKVDEC_NUM_REFLIST		3
 
+struct rkvdec_scaling_matrix {
+	u8 scaling_list_4x4[6][16];
+	u8 scaling_list_8x8[6][64];
+	u8 padding[128];
+};
+
 struct rkvdec_sps_pps_packet {
 	u32 info[8];
 };
@@ -86,7 +91,7 @@ struct rkvdec_ps_field {
 /* Data structure describing auxiliary buffer format. */
 struct rkvdec_h264_priv_tbl {
 	s8 cabac_table[4][464][2];
-	u8 scaling_list[RKV_SCALING_LIST_SIZE];
+	struct rkvdec_scaling_matrix scaling_list;
 	u32 rps[RKV_RPS_SIZE];
 	struct rkvdec_sps_pps_packet param_set[256];
 	u8 err_info[RKV_ERROR_INFO_SIZE];
@@ -785,56 +790,25 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 	}
 }
 
-/*
- * NOTE: The values in a scaling list are in zig-zag order, apply inverse
- * scanning process to get the values in matrix order.
- */
-static const u32 zig_zag_4x4[16] = {
-	0, 1, 4, 8, 5, 2, 3, 6, 9, 12, 13, 10, 7, 11, 14, 15
-};
-
-static const u32 zig_zag_8x8[64] = {
-	0,  1,  8, 16,  9,  2,  3, 10, 17, 24, 32, 25, 18, 11,  4,  5,
-	12, 19, 26, 33, 40, 48, 41, 34, 27, 20, 13,  6,  7, 14, 21, 28,
-	35, 42, 49, 56, 57, 50, 43, 36, 29, 22, 15, 23, 30, 37, 44, 51,
-	58, 59, 52, 45, 38, 31, 39, 46, 53, 60, 61, 54, 47, 55, 62, 63
-};
-
-static void reorder_scaling_list(struct rkvdec_ctx *ctx,
-				 struct rkvdec_h264_run *run)
+static void assemble_hw_scaling_list(struct rkvdec_ctx *ctx,
+				     struct rkvdec_h264_run *run)
 {
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling = run->scaling_matrix;
-	const size_t num_list_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4);
-	const size_t list_len_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4[0]);
-	const size_t num_list_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8);
-	const size_t list_len_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8[0]);
 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
 	struct rkvdec_h264_priv_tbl *tbl = h264_ctx->priv_tbl.cpu;
-	u8 *dst = tbl->scaling_list;
-	const u8 *src;
-	int i, j;
-
-	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_4x4) != list_len_4x4);
-	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_8x8) != list_len_8x8);
-	BUILD_BUG_ON(ARRAY_SIZE(tbl->scaling_list) <
-		     num_list_4x4 * list_len_4x4 +
-		     num_list_8x8 * list_len_8x8);
-
-	src = &scaling->scaling_list_4x4[0][0];
-	for (i = 0; i < num_list_4x4; ++i) {
-		for (j = 0; j < list_len_4x4; ++j)
-			dst[zig_zag_4x4[j]] = src[j];
-		src += list_len_4x4;
-		dst += list_len_4x4;
-	}
 
-	src = &scaling->scaling_list_8x8[0][0];
-	for (i = 0; i < num_list_8x8; ++i) {
-		for (j = 0; j < list_len_8x8; ++j)
-			dst[zig_zag_8x8[j]] = src[j];
-		src += list_len_8x8;
-		dst += list_len_8x8;
-	}
+	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_4x4) !=
+		     sizeof(scaling->scaling_list_4x4));
+	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_8x8) !=
+		     sizeof(scaling->scaling_list_8x8));
+
+	memcpy(tbl->scaling_list.scaling_list_4x4,
+	       scaling->scaling_list_4x4,
+	       sizeof(scaling->scaling_list_4x4));
+
+	memcpy(tbl->scaling_list.scaling_list_8x8,
+	       scaling->scaling_list_8x8,
+	       sizeof(scaling->scaling_list_8x8));
 }
 
 /*
@@ -1126,7 +1100,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
 				    h264_ctx->reflists.b1);
 
-	reorder_scaling_list(ctx, &run);
+	assemble_hw_scaling_list(ctx, &run);
 	assemble_hw_pps(ctx, &run);
 	assemble_hw_rps(ctx, &run);
 	config_registers(ctx, &run);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rkvdec: Fix H264 scaling list order
  2020-05-22 20:21 [PATCH] media: rkvdec: Fix H264 scaling list order Jonas Karlman
@ 2020-05-22 20:27 ` Nicolas Dufresne
  2020-05-25 11:47 ` Tomasz Figa
  2020-06-02  4:36 ` Ezequiel Garcia
  2 siblings, 0 replies; 4+ messages in thread
From: Nicolas Dufresne @ 2020-05-22 20:27 UTC (permalink / raw)
  To: Jonas Karlman, Ezequiel Garcia, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Boris Brezillon, Tomasz Figa, linux-media, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5461 bytes --]

Le vendredi 22 mai 2020 à 20:21 +0000, Jonas Karlman a écrit :
> The Rockchip Video Decoder driver is expecting that the values in a
> scaling list are in zig-zag order and applies the inverse scanning process
> to get the values in matrix order.
> 
> Commit 0b0393d59eb4 ("media: uapi: h264: clarify expected
> scaling_list_4x4/8x8 order") clarified that the values in the scaling list
> should already be in matrix order.
> 
> Fix this by removing the reordering and change to use two memcpy.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

I've tested it over GStreamer implementation. Note that in addition to
this patch, we should now fix the documentation. The documentation says
that the scaling list order should be that same as in the bitstream
(that means zigzag order). But I believe it make sense to move to
raster order as we know have the knowledge that this is what all the HW
we supports uses and is what one would pass to VAAPI, NVDEC and DXVA2
too.

Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 70 +++++++---------------
>  1 file changed, 22 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index cd4980d06be7..2719f0c66a4a 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -18,11 +18,16 @@
>  /* Size with u32 units. */
>  #define RKV_CABAC_INIT_BUFFER_SIZE	(3680 + 128)
>  #define RKV_RPS_SIZE			((128 + 128) / 4)
> -#define RKV_SCALING_LIST_SIZE		(6 * 16 + 6 * 64 + 128)
>  #define RKV_ERROR_INFO_SIZE		(256 * 144 * 4)
>  
>  #define RKVDEC_NUM_REFLIST		3
>  
> +struct rkvdec_scaling_matrix {
> +	u8 scaling_list_4x4[6][16];
> +	u8 scaling_list_8x8[6][64];
> +	u8 padding[128];
> +};
> +
>  struct rkvdec_sps_pps_packet {
>  	u32 info[8];
>  };
> @@ -86,7 +91,7 @@ struct rkvdec_ps_field {
>  /* Data structure describing auxiliary buffer format. */
>  struct rkvdec_h264_priv_tbl {
>  	s8 cabac_table[4][464][2];
> -	u8 scaling_list[RKV_SCALING_LIST_SIZE];
> +	struct rkvdec_scaling_matrix scaling_list;
>  	u32 rps[RKV_RPS_SIZE];
>  	struct rkvdec_sps_pps_packet param_set[256];
>  	u8 err_info[RKV_ERROR_INFO_SIZE];
> @@ -785,56 +790,25 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  	}
>  }
>  
> -/*
> - * NOTE: The values in a scaling list are in zig-zag order, apply inverse
> - * scanning process to get the values in matrix order.
> - */
> -static const u32 zig_zag_4x4[16] = {
> -	0, 1, 4, 8, 5, 2, 3, 6, 9, 12, 13, 10, 7, 11, 14, 15
> -};
> -
> -static const u32 zig_zag_8x8[64] = {
> -	0,  1,  8, 16,  9,  2,  3, 10, 17, 24, 32, 25, 18, 11,  4,  5,
> -	12, 19, 26, 33, 40, 48, 41, 34, 27, 20, 13,  6,  7, 14, 21, 28,
> -	35, 42, 49, 56, 57, 50, 43, 36, 29, 22, 15, 23, 30, 37, 44, 51,
> -	58, 59, 52, 45, 38, 31, 39, 46, 53, 60, 61, 54, 47, 55, 62, 63
> -};
> -
> -static void reorder_scaling_list(struct rkvdec_ctx *ctx,
> -				 struct rkvdec_h264_run *run)
> +static void assemble_hw_scaling_list(struct rkvdec_ctx *ctx,
> +				     struct rkvdec_h264_run *run)
>  {
>  	const struct v4l2_ctrl_h264_scaling_matrix *scaling = run->scaling_matrix;
> -	const size_t num_list_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4);
> -	const size_t list_len_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4[0]);
> -	const size_t num_list_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8);
> -	const size_t list_len_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8[0]);
>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>  	struct rkvdec_h264_priv_tbl *tbl = h264_ctx->priv_tbl.cpu;
> -	u8 *dst = tbl->scaling_list;
> -	const u8 *src;
> -	int i, j;
> -
> -	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_4x4) != list_len_4x4);
> -	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_8x8) != list_len_8x8);
> -	BUILD_BUG_ON(ARRAY_SIZE(tbl->scaling_list) <
> -		     num_list_4x4 * list_len_4x4 +
> -		     num_list_8x8 * list_len_8x8);
> -
> -	src = &scaling->scaling_list_4x4[0][0];
> -	for (i = 0; i < num_list_4x4; ++i) {
> -		for (j = 0; j < list_len_4x4; ++j)
> -			dst[zig_zag_4x4[j]] = src[j];
> -		src += list_len_4x4;
> -		dst += list_len_4x4;
> -	}
>  
> -	src = &scaling->scaling_list_8x8[0][0];
> -	for (i = 0; i < num_list_8x8; ++i) {
> -		for (j = 0; j < list_len_8x8; ++j)
> -			dst[zig_zag_8x8[j]] = src[j];
> -		src += list_len_8x8;
> -		dst += list_len_8x8;
> -	}
> +	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_4x4) !=
> +		     sizeof(scaling->scaling_list_4x4));
> +	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_8x8) !=
> +		     sizeof(scaling->scaling_list_8x8));
> +
> +	memcpy(tbl->scaling_list.scaling_list_4x4,
> +	       scaling->scaling_list_4x4,
> +	       sizeof(scaling->scaling_list_4x4));
> +
> +	memcpy(tbl->scaling_list.scaling_list_8x8,
> +	       scaling->scaling_list_8x8,
> +	       sizeof(scaling->scaling_list_8x8));
>  }
>  
>  /*
> @@ -1126,7 +1100,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
>  				    h264_ctx->reflists.b1);
>  
> -	reorder_scaling_list(ctx, &run);
> +	assemble_hw_scaling_list(ctx, &run);
>  	assemble_hw_pps(ctx, &run);
>  	assemble_hw_rps(ctx, &run);
>  	config_registers(ctx, &run);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rkvdec: Fix H264 scaling list order
  2020-05-22 20:21 [PATCH] media: rkvdec: Fix H264 scaling list order Jonas Karlman
  2020-05-22 20:27 ` Nicolas Dufresne
@ 2020-05-25 11:47 ` Tomasz Figa
  2020-06-02  4:36 ` Ezequiel Garcia
  2 siblings, 0 replies; 4+ messages in thread
From: Tomasz Figa @ 2020-05-25 11:47 UTC (permalink / raw)
  To: Jonas Karlman, Alexandre Courbot, Jeffrey Kardatzke
  Cc: Ezequiel Garcia, Hans Verkuil, Mauro Carvalho Chehab,
	Boris Brezillon, Nicolas Dufresne, Linux Media Mailing List,
	open list:ARM/Rockchip SoC...,
	Linux Kernel Mailing List

On Fri, May 22, 2020 at 10:21 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> The Rockchip Video Decoder driver is expecting that the values in a
> scaling list are in zig-zag order and applies the inverse scanning process
> to get the values in matrix order.
>
> Commit 0b0393d59eb4 ("media: uapi: h264: clarify expected
> scaling_list_4x4/8x8 order") clarified that the values in the scaling list
> should already be in matrix order.
>
> Fix this by removing the reordering and change to use two memcpy.
>
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 70 +++++++---------------
>  1 file changed, 22 insertions(+), 48 deletions(-)
>

+Alexandre Courbot +Jeffrey Kardatzke for visibility

> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index cd4980d06be7..2719f0c66a4a 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -18,11 +18,16 @@
>  /* Size with u32 units. */
>  #define RKV_CABAC_INIT_BUFFER_SIZE     (3680 + 128)
>  #define RKV_RPS_SIZE                   ((128 + 128) / 4)
> -#define RKV_SCALING_LIST_SIZE          (6 * 16 + 6 * 64 + 128)
>  #define RKV_ERROR_INFO_SIZE            (256 * 144 * 4)
>
>  #define RKVDEC_NUM_REFLIST             3
>
> +struct rkvdec_scaling_matrix {
> +       u8 scaling_list_4x4[6][16];
> +       u8 scaling_list_8x8[6][64];
> +       u8 padding[128];
> +};
> +
>  struct rkvdec_sps_pps_packet {
>         u32 info[8];
>  };
> @@ -86,7 +91,7 @@ struct rkvdec_ps_field {
>  /* Data structure describing auxiliary buffer format. */
>  struct rkvdec_h264_priv_tbl {
>         s8 cabac_table[4][464][2];
> -       u8 scaling_list[RKV_SCALING_LIST_SIZE];
> +       struct rkvdec_scaling_matrix scaling_list;
>         u32 rps[RKV_RPS_SIZE];
>         struct rkvdec_sps_pps_packet param_set[256];
>         u8 err_info[RKV_ERROR_INFO_SIZE];
> @@ -785,56 +790,25 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>         }
>  }
>
> -/*
> - * NOTE: The values in a scaling list are in zig-zag order, apply inverse
> - * scanning process to get the values in matrix order.
> - */
> -static const u32 zig_zag_4x4[16] = {
> -       0, 1, 4, 8, 5, 2, 3, 6, 9, 12, 13, 10, 7, 11, 14, 15
> -};
> -
> -static const u32 zig_zag_8x8[64] = {
> -       0,  1,  8, 16,  9,  2,  3, 10, 17, 24, 32, 25, 18, 11,  4,  5,
> -       12, 19, 26, 33, 40, 48, 41, 34, 27, 20, 13,  6,  7, 14, 21, 28,
> -       35, 42, 49, 56, 57, 50, 43, 36, 29, 22, 15, 23, 30, 37, 44, 51,
> -       58, 59, 52, 45, 38, 31, 39, 46, 53, 60, 61, 54, 47, 55, 62, 63
> -};
> -
> -static void reorder_scaling_list(struct rkvdec_ctx *ctx,
> -                                struct rkvdec_h264_run *run)
> +static void assemble_hw_scaling_list(struct rkvdec_ctx *ctx,
> +                                    struct rkvdec_h264_run *run)
>  {
>         const struct v4l2_ctrl_h264_scaling_matrix *scaling = run->scaling_matrix;
> -       const size_t num_list_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4);
> -       const size_t list_len_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4[0]);
> -       const size_t num_list_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8);
> -       const size_t list_len_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8[0]);
>         struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>         struct rkvdec_h264_priv_tbl *tbl = h264_ctx->priv_tbl.cpu;
> -       u8 *dst = tbl->scaling_list;
> -       const u8 *src;
> -       int i, j;
> -
> -       BUILD_BUG_ON(ARRAY_SIZE(zig_zag_4x4) != list_len_4x4);
> -       BUILD_BUG_ON(ARRAY_SIZE(zig_zag_8x8) != list_len_8x8);
> -       BUILD_BUG_ON(ARRAY_SIZE(tbl->scaling_list) <
> -                    num_list_4x4 * list_len_4x4 +
> -                    num_list_8x8 * list_len_8x8);
> -
> -       src = &scaling->scaling_list_4x4[0][0];
> -       for (i = 0; i < num_list_4x4; ++i) {
> -               for (j = 0; j < list_len_4x4; ++j)
> -                       dst[zig_zag_4x4[j]] = src[j];
> -               src += list_len_4x4;
> -               dst += list_len_4x4;
> -       }
>
> -       src = &scaling->scaling_list_8x8[0][0];
> -       for (i = 0; i < num_list_8x8; ++i) {
> -               for (j = 0; j < list_len_8x8; ++j)
> -                       dst[zig_zag_8x8[j]] = src[j];
> -               src += list_len_8x8;
> -               dst += list_len_8x8;
> -       }
> +       BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_4x4) !=
> +                    sizeof(scaling->scaling_list_4x4));
> +       BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_8x8) !=
> +                    sizeof(scaling->scaling_list_8x8));
> +
> +       memcpy(tbl->scaling_list.scaling_list_4x4,
> +              scaling->scaling_list_4x4,
> +              sizeof(scaling->scaling_list_4x4));
> +
> +       memcpy(tbl->scaling_list.scaling_list_8x8,
> +              scaling->scaling_list_8x8,
> +              sizeof(scaling->scaling_list_8x8));
>  }
>
>  /*
> @@ -1126,7 +1100,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>         v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
>                                     h264_ctx->reflists.b1);
>
> -       reorder_scaling_list(ctx, &run);
> +       assemble_hw_scaling_list(ctx, &run);
>         assemble_hw_pps(ctx, &run);
>         assemble_hw_rps(ctx, &run);
>         config_registers(ctx, &run);
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rkvdec: Fix H264 scaling list order
  2020-05-22 20:21 [PATCH] media: rkvdec: Fix H264 scaling list order Jonas Karlman
  2020-05-22 20:27 ` Nicolas Dufresne
  2020-05-25 11:47 ` Tomasz Figa
@ 2020-06-02  4:36 ` Ezequiel Garcia
  2 siblings, 0 replies; 4+ messages in thread
From: Ezequiel Garcia @ 2020-06-02  4:36 UTC (permalink / raw)
  To: Jonas Karlman, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Jeffrey Kardatzke, Alexandre Courbot, Boris Brezillon,
	Tomasz Figa, Nicolas Dufresne, linux-media, linux-rockchip,
	linux-kernel

Hi Jonas,

Thanks a lot for the fix! Indeed it fixes
a few bitstream that had artifacts :)

On Fri, 2020-05-22 at 20:21 +0000, Jonas Karlman wrote:
> The Rockchip Video Decoder driver is expecting that the values in a
> scaling list are in zig-zag order and applies the inverse scanning process
> to get the values in matrix order.
> 
> Commit 0b0393d59eb4 ("media: uapi: h264: clarify expected
> scaling_list_4x4/8x8 order") clarified that the values in the scaling list
> should already be in matrix order.
> 
> Fix this by removing the reordering and change to use two memcpy.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 70 +++++++---------------
>  1 file changed, 22 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index cd4980d06be7..2719f0c66a4a 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -18,11 +18,16 @@
>  /* Size with u32 units. */
>  #define RKV_CABAC_INIT_BUFFER_SIZE	(3680 + 128)
>  #define RKV_RPS_SIZE			((128 + 128) / 4)
> -#define RKV_SCALING_LIST_SIZE		(6 * 16 + 6 * 64 + 128)
>  #define RKV_ERROR_INFO_SIZE		(256 * 144 * 4)
>  
>  #define RKVDEC_NUM_REFLIST		3
>  
> +struct rkvdec_scaling_matrix {

How about we call this rkvdec_scaling_list
or even rkvdec_h264_scaling_list?

It'll make code more readable and easier to grep.

> +	u8 scaling_list_4x4[6][16];
> +	u8 scaling_list_8x8[6][64];
> +	u8 padding[128];

Oops, something is wrong here, maybe some
mistake when porting code around.

Rockchip MPP defines the scaling list size as:

#define RKV_SCALING_LIST_SIZE (6*16 + 2*64 + 128)

Consistently with the fact that 4:4:4 sampling
is not supported (chroma_format_idc == 3),
only the first two scaling matrices are used.

Moreover, given all these buffers are specified separately
(see below), I bet not even the 128 padding bytes
are needed.
 
> +};
> +
>  struct rkvdec_sps_pps_packet {
>  	u32 info[8];
>  };
> @@ -86,7 +91,7 @@ struct rkvdec_ps_field {
>  /* Data structure describing auxiliary buffer format. */
>  struct rkvdec_h264_priv_tbl {
>  	s8 cabac_table[4][464][2];

As can be seen, all these buffers can be allocated
independently, which perhaps could reduce the
pressure on the DMA allocator.

> -	u8 scaling_list[RKV_SCALING_LIST_SIZE];
> +	struct rkvdec_scaling_matrix scaling_list;
>  	u32 rps[RKV_RPS_SIZE];
>  	struct rkvdec_sps_pps_packet param_set[256];
>  	u8 err_info[RKV_ERROR_INFO_SIZE];

In particular, I wonder if the error info stuff
is really required.

I guess we'll want to merge a simple fix,
so no need to address any of these questions
for now.

With the struct rkvdec_scaling_matrix rename:

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

Thanks,
Ezequiel

> @@ -785,56 +790,25 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  	}
>  }
>  
> -/*
> - * NOTE: The values in a scaling list are in zig-zag order, apply inverse
> - * scanning process to get the values in matrix order.
> - */
> -static const u32 zig_zag_4x4[16] = {
> -	0, 1, 4, 8, 5, 2, 3, 6, 9, 12, 13, 10, 7, 11, 14, 15
> -};
> -
> -static const u32 zig_zag_8x8[64] = {
> -	0,  1,  8, 16,  9,  2,  3, 10, 17, 24, 32, 25, 18, 11,  4,  5,
> -	12, 19, 26, 33, 40, 48, 41, 34, 27, 20, 13,  6,  7, 14, 21, 28,
> -	35, 42, 49, 56, 57, 50, 43, 36, 29, 22, 15, 23, 30, 37, 44, 51,
> -	58, 59, 52, 45, 38, 31, 39, 46, 53, 60, 61, 54, 47, 55, 62, 63
> -};
> -
> -static void reorder_scaling_list(struct rkvdec_ctx *ctx,
> -				 struct rkvdec_h264_run *run)
> +static void assemble_hw_scaling_list(struct rkvdec_ctx *ctx,
> +				     struct rkvdec_h264_run *run)
>  {
>  	const struct v4l2_ctrl_h264_scaling_matrix *scaling = run->scaling_matrix;
> -	const size_t num_list_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4);
> -	const size_t list_len_4x4 = ARRAY_SIZE(scaling->scaling_list_4x4[0]);
> -	const size_t num_list_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8);
> -	const size_t list_len_8x8 = ARRAY_SIZE(scaling->scaling_list_8x8[0]);
>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>  	struct rkvdec_h264_priv_tbl *tbl = h264_ctx->priv_tbl.cpu;
> -	u8 *dst = tbl->scaling_list;
> -	const u8 *src;
> -	int i, j;
> -
> -	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_4x4) != list_len_4x4);
> -	BUILD_BUG_ON(ARRAY_SIZE(zig_zag_8x8) != list_len_8x8);
> -	BUILD_BUG_ON(ARRAY_SIZE(tbl->scaling_list) <
> -		     num_list_4x4 * list_len_4x4 +
> -		     num_list_8x8 * list_len_8x8);
> -
> -	src = &scaling->scaling_list_4x4[0][0];
> -	for (i = 0; i < num_list_4x4; ++i) {
> -		for (j = 0; j < list_len_4x4; ++j)
> -			dst[zig_zag_4x4[j]] = src[j];
> -		src += list_len_4x4;
> -		dst += list_len_4x4;
> -	}
>  
> -	src = &scaling->scaling_list_8x8[0][0];
> -	for (i = 0; i < num_list_8x8; ++i) {
> -		for (j = 0; j < list_len_8x8; ++j)
> -			dst[zig_zag_8x8[j]] = src[j];
> -		src += list_len_8x8;
> -		dst += list_len_8x8;
> -	}
> +	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_4x4) !=
> +		     sizeof(scaling->scaling_list_4x4));
> +	BUILD_BUG_ON(sizeof(tbl->scaling_list.scaling_list_8x8) !=
> +		     sizeof(scaling->scaling_list_8x8));
> +
> +	memcpy(tbl->scaling_list.scaling_list_4x4,
> +	       scaling->scaling_list_4x4,
> +	       sizeof(scaling->scaling_list_4x4));
> +
> +	memcpy(tbl->scaling_list.scaling_list_8x8,
> +	       scaling->scaling_list_8x8,
> +	       sizeof(scaling->scaling_list_8x8));
>  }
>  
>  /*
> @@ -1126,7 +1100,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
>  				    h264_ctx->reflists.b1);
>  
> -	reorder_scaling_list(ctx, &run);
> +	assemble_hw_scaling_list(ctx, &run);
>  	assemble_hw_pps(ctx, &run);
>  	assemble_hw_rps(ctx, &run);
>  	config_registers(ctx, &run);



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-06-02  4:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22 20:21 [PATCH] media: rkvdec: Fix H264 scaling list order Jonas Karlman
2020-05-22 20:27 ` Nicolas Dufresne
2020-05-25 11:47 ` Tomasz Figa
2020-06-02  4:36 ` Ezequiel Garcia

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.