[PATCH 0/4] Hantro VPU JPEG encoder fixes

[PATCH 0/4] Hantro VPU JPEG encoder fixes

[Andrzej Pietrasiewicz]

This series addresses quality issues in encoded JPEG images.

The first patch actually restores the intention of the original submission
of this driver: due to a typo the helper variables were unused and then
have been removed in some cleanup done by Mauro.

The second patch aligns the driver's luma quantization table with
the one in the ITU-T.81 standard.

The third patch changes the order in which quantization tables are
written to the resulting file and to the hardware. The file expects
a zig-zag order, while the hardware wants some special order, neither
linear nor zig-zag. In other words, hardware-wise it rearranges which
parts of quantization tables go into which 4-byte registers - in a hardware
specific order rather than linear or zig-zag. It also affects rk3288 and
hasn't been tested with it.

The fourth patch then rearranges the sequence of register writes.
The whole luma quantization table must be written first, and then the
chroma quantization is written. In other words, while patch 3/4
changes what goes into which register, this patch changes when each
register is written to. It also affects rk3288 and hasn't been
tested with it.

Andrzej Pietrasiewicz (4):
  media: hantro: Read be32 words starting at every fourth byte
  media: hantro: Use standard luma quantization table
  media: hantro: Write the quantization tables in proper order
  media: hantro: Write quantization table registers in increasing
    addresses order

 .../staging/media/hantro/hantro_h1_jpeg_enc.c | 19 ++++-
 drivers/staging/media/hantro/hantro_jpeg.c    | 76 ++++++++++++++-----
 drivers/staging/media/hantro/hantro_jpeg.h    |  2 +-
 .../media/hantro/rk3399_vpu_hw_jpeg_enc.c     | 24 ++++--
 4 files changed, 89 insertions(+), 32 deletions(-)

-- 
2.17.1

[PATCH 1/4] media: hantro: Read be32 words starting at every fourth byte

[Andrzej Pietrasiewicz]

Since (luma/chroma)_qtable is an array of unsigned char, indexing it
returns consecutive byte locations, but we are supposed to read the arrays
in four-byte words. Consequently, we should be pointing
get_unaligned_be32() at consecutive word locations instead.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/staging/media/hantro/hantro_h1_jpeg_enc.c     | 9 +++++++--
 drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c | 9 +++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
index 938b48d4d3d9..be787a045c7e 100644
--- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
+++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
@@ -67,12 +67,17 @@ hantro_h1_jpeg_enc_set_qtable(struct hantro_dev *vpu,
 			      unsigned char *chroma_qtable)
 {
 	u32 reg, i;
+	__be32 *luma_qtable_p;
+	__be32 *chroma_qtable_p;
+
+	luma_qtable_p = (__be32 *)luma_qtable;
+	chroma_qtable_p = (__be32 *)chroma_qtable;
 
 	for (i = 0; i < H1_JPEG_QUANT_TABLE_COUNT; i++) {
-		reg = get_unaligned_be32(&luma_qtable[i]);
+		reg = get_unaligned_be32(&luma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_LUMA_QUAT(i));
 
-		reg = get_unaligned_be32(&chroma_qtable[i]);
+		reg = get_unaligned_be32(&chroma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_CHROMA_QUAT(i));
 	}
 }
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
index 067892345b5d..bdb95652d6a8 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
@@ -98,12 +98,17 @@ rk3399_vpu_jpeg_enc_set_qtable(struct hantro_dev *vpu,
 			       unsigned char *chroma_qtable)
 {
 	u32 reg, i;
+	__be32 *luma_qtable_p;
+	__be32 *chroma_qtable_p;
+
+	luma_qtable_p = (__be32 *)luma_qtable;
+	chroma_qtable_p = (__be32 *)chroma_qtable;
 
 	for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
-		reg = get_unaligned_be32(&luma_qtable[i]);
+		reg = get_unaligned_be32(&luma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
 
-		reg = get_unaligned_be32(&chroma_qtable[i]);
+		reg = get_unaligned_be32(&chroma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_CHROMA_QUAT(i));
 	}
 }
-- 
2.17.1

Re: [PATCH 1/4] media: hantro: Read be32 words starting at every fourth byte

[Ezequiel Garcia]

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> Since (luma/chroma)_qtable is an array of unsigned char, indexing it
> returns consecutive byte locations, but we are supposed to read the arrays
> in four-byte words. Consequently, we should be pointing
> get_unaligned_be32() at consecutive word locations instead.
> 

Ouch!

Seems we were too fast on that cleanup. Please add:

Cc: stable@vger.kernel.org
Fixes: 00c30f42c7595f "media: rockchip vpu: remove some unused vars"
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_h1_jpeg_enc.c     | 9 +++++++--
>  drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c | 9 +++++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> index 938b48d4d3d9..be787a045c7e 100644
> --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> @@ -67,12 +67,17 @@ hantro_h1_jpeg_enc_set_qtable(struct hantro_dev *vpu,
>  			      unsigned char *chroma_qtable)
>  {
>  	u32 reg, i;
> +	__be32 *luma_qtable_p;
> +	__be32 *chroma_qtable_p;
> +
> +	luma_qtable_p = (__be32 *)luma_qtable;
> +	chroma_qtable_p = (__be32 *)chroma_qtable;
>  
>  	for (i = 0; i < H1_JPEG_QUANT_TABLE_COUNT; i++) {
> -		reg = get_unaligned_be32(&luma_qtable[i]);
> +		reg = get_unaligned_be32(&luma_qtable_p[i]);
>  		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_LUMA_QUAT(i));
>  
> -		reg = get_unaligned_be32(&chroma_qtable[i]);
> +		reg = get_unaligned_be32(&chroma_qtable_p[i]);
>  		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_CHROMA_QUAT(i));
>  	}
>  }
> diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> index 067892345b5d..bdb95652d6a8 100644
> --- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> @@ -98,12 +98,17 @@ rk3399_vpu_jpeg_enc_set_qtable(struct hantro_dev *vpu,
>  			       unsigned char *chroma_qtable)
>  {
>  	u32 reg, i;
> +	__be32 *luma_qtable_p;
> +	__be32 *chroma_qtable_p;
> +
> +	luma_qtable_p = (__be32 *)luma_qtable;
> +	chroma_qtable_p = (__be32 *)chroma_qtable;
>  
>  	for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
> -		reg = get_unaligned_be32(&luma_qtable[i]);
> +		reg = get_unaligned_be32(&luma_qtable_p[i]);
>  		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
>  
> -		reg = get_unaligned_be32(&chroma_qtable[i]);
> +		reg = get_unaligned_be32(&chroma_qtable_p[i]);
>  		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_CHROMA_QUAT(i));
>  	}
>  }
> -- 
> 2.17.1
> 
> 

[PATCH 2/4] media: hantro: Use standard luma quantization table

[Andrzej Pietrasiewicz]

The table is actually different in the document than in this file, so align
this file with the document.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/staging/media/hantro/hantro_jpeg.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_jpeg.c b/drivers/staging/media/hantro/hantro_jpeg.c
index 125eb41f2ede..d3b381d00b23 100644
--- a/drivers/staging/media/hantro/hantro_jpeg.c
+++ b/drivers/staging/media/hantro/hantro_jpeg.c
@@ -23,17 +23,17 @@
 #define HUFF_CHROMA_AC_OFF	409
 
 /* Default tables from JPEG ITU-T.81
- * (ISO/IEC 10918-1) Annex K.3, I
+ * (ISO/IEC 10918-1) Annex K, tables K.1 and K.2
  */
 static const unsigned char luma_q_table[] = {
-	0x10, 0x0b, 0x0a, 0x10, 0x7c, 0x8c, 0x97, 0xa1,
-	0x0c, 0x0c, 0x0e, 0x13, 0x7e, 0x9e, 0xa0, 0x9b,
-	0x0e, 0x0d, 0x10, 0x18, 0x8c, 0x9d, 0xa9, 0x9c,
-	0x0e, 0x11, 0x16, 0x1d, 0x97, 0xbb, 0xb4, 0xa2,
-	0x12, 0x16, 0x25, 0x38, 0xa8, 0x6d, 0x67, 0xb1,
-	0x18, 0x23, 0x37, 0x40, 0xb5, 0x68, 0x71, 0xc0,
+	0x10, 0x0b, 0x0a, 0x10, 0x18, 0x28, 0x33, 0x3d,
+	0x0c, 0x0c, 0x0e, 0x13, 0x1a, 0x3a, 0x3c, 0x37,
+	0x0e, 0x0d, 0x10, 0x18, 0x28, 0x39, 0x45, 0x38,
+	0x0e, 0x11, 0x16, 0x1d, 0x33, 0x57, 0x50, 0x3e,
+	0x12, 0x16, 0x25, 0x38, 0x44, 0x6d, 0x67, 0x4d,
+	0x18, 0x23, 0x37, 0x40, 0x51, 0x68, 0x71, 0x5c,
 	0x31, 0x40, 0x4e, 0x57, 0x67, 0x79, 0x78, 0x65,
-	0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0xc7,
+	0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0x63
 };
 
 static const unsigned char chroma_q_table[] = {
-- 
2.17.1

Re: [PATCH 2/4] media: hantro: Use standard luma quantization table

[Ezequiel Garcia]

Hi Andrzej,

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> The table is actually different in the document than in this file, so align
> this file with the document.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_jpeg.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_jpeg.c b/drivers/staging/media/hantro/hantro_jpeg.c
> index 125eb41f2ede..d3b381d00b23 100644
> --- a/drivers/staging/media/hantro/hantro_jpeg.c
> +++ b/drivers/staging/media/hantro/hantro_jpeg.c
> @@ -23,17 +23,17 @@
>  #define HUFF_CHROMA_AC_OFF	409
>  
>  /* Default tables from JPEG ITU-T.81
> - * (ISO/IEC 10918-1) Annex K.3, I
> + * (ISO/IEC 10918-1) Annex K, tables K.1 and K.2
>   */

I wonder if we shouldn't just have these tables
in decimal instead of hexa, so they look exactly
like the ones in the spec.

Thanks,
Ezequiel

>  static const unsigned char luma_q_table[] = {
> -	0x10, 0x0b, 0x0a, 0x10, 0x7c, 0x8c, 0x97, 0xa1,
> -	0x0c, 0x0c, 0x0e, 0x13, 0x7e, 0x9e, 0xa0, 0x9b,
> -	0x0e, 0x0d, 0x10, 0x18, 0x8c, 0x9d, 0xa9, 0x9c,
> -	0x0e, 0x11, 0x16, 0x1d, 0x97, 0xbb, 0xb4, 0xa2,
> -	0x12, 0x16, 0x25, 0x38, 0xa8, 0x6d, 0x67, 0xb1,
> -	0x18, 0x23, 0x37, 0x40, 0xb5, 0x68, 0x71, 0xc0,
> +	0x10, 0x0b, 0x0a, 0x10, 0x18, 0x28, 0x33, 0x3d,
> +	0x0c, 0x0c, 0x0e, 0x13, 0x1a, 0x3a, 0x3c, 0x37,
> +	0x0e, 0x0d, 0x10, 0x18, 0x28, 0x39, 0x45, 0x38,
> +	0x0e, 0x11, 0x16, 0x1d, 0x33, 0x57, 0x50, 0x3e,
> +	0x12, 0x16, 0x25, 0x38, 0x44, 0x6d, 0x67, 0x4d,
> +	0x18, 0x23, 0x37, 0x40, 0x51, 0x68, 0x71, 0x5c,
>  	0x31, 0x40, 0x4e, 0x57, 0x67, 0x79, 0x78, 0x65,
> -	0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0xc7,
> +	0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0x63
>  };
>  
>  static const unsigned char chroma_q_table[] = {
> -- 
> 2.17.1
> 
> 

[PATCH 3/4] media: hantro: Write the quantization tables in proper order

[Andrzej Pietrasiewicz]

The quantization tables as defined in the file (luma_q_table,
chroma_q_table) are in fact in linear order. The JPEG file header, which is
not generated by the hardware, but must be programatically created with the
CPU, expects the table in zigzag order. On the other hand, the hardware
doesn't expect neither linear, nor zigzag order. Instead it expects the
quantization tables in vertical groups of four quantization parameters,
and the groups are organized in blocks of two vertically adjacent groups.
On top of that the blocks must be provided to the hardware in this order:
leftmost top block, leftmost bottom block, second leftmost top block,
second leftmost bottom block and so on. So, if this is the quantization
table in linear order:

0x10, 0x0b, 0x0a, 0x10, 0x18, 0x28, 0x33, 0x3d,
0x0c, 0x0c, 0x0e, 0x13, 0x1a, 0x3a, 0x3c, 0x37,
0x0e, 0x0d, 0x10, 0x18, 0x28, 0x39, 0x45, 0x38,
0x0e, 0x11, 0x16, 0x1d, 0x33, 0x57, 0x50, 0x3e,
0x12, 0x16, 0x25, 0x38, 0x44, 0x6d, 0x67, 0x4d,
0x18, 0x23, 0x37, 0x40, 0x51, 0x68, 0x71, 0x5c,
0x31, 0x40, 0x4e, 0x57, 0x67, 0x79, 0x78, 0x65,
0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0x63

then the hardware expects this in its consecutive registers:

0x100c0e0e,
0x0b0c0d11,
0x12183148,
0x1623405c,
0x0a0e1016,
0x1013181d,
0x25374e5f,
0x38405762,

and so on.

Consequently, the same area of memory cannot be used both for dumping it
into the JPEG file header and writing its contents to the hardware
registers. Instead, a separate pair of arrays is added for properly
reordered quantization tables, to be read with get_unaligned_be32()
and linearly written to the registers.

The "ctx" parameter is not needed any more for hantro_jpeg_get_qtable().

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 .../staging/media/hantro/hantro_h1_jpeg_enc.c |  4 +-
 drivers/staging/media/hantro/hantro_jpeg.c    | 60 +++++++++++++++----
 drivers/staging/media/hantro/hantro_jpeg.h    |  2 +-
 .../media/hantro/rk3399_vpu_hw_jpeg_enc.c     |  9 ++-
 4 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
index be787a045c7e..bd05aea1bd71 100644
--- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
+++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
@@ -108,8 +108,8 @@ void hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx)
 	hantro_h1_set_src_img_ctrl(vpu, ctx);
 	hantro_h1_jpeg_enc_set_buffers(vpu, ctx, &src_buf->vb2_buf);
 	hantro_h1_jpeg_enc_set_qtable(vpu,
-				      hantro_jpeg_get_qtable(&jpeg_ctx, 0),
-				      hantro_jpeg_get_qtable(&jpeg_ctx, 1));
+				      hantro_jpeg_get_qtable(0),
+				      hantro_jpeg_get_qtable(1));
 
 	reg = H1_REG_AXI_CTRL_OUTPUT_SWAP16
 		| H1_REG_AXI_CTRL_INPUT_SWAP16
diff --git a/drivers/staging/media/hantro/hantro_jpeg.c b/drivers/staging/media/hantro/hantro_jpeg.c
index d3b381d00b23..36c140fc6a36 100644
--- a/drivers/staging/media/hantro/hantro_jpeg.c
+++ b/drivers/staging/media/hantro/hantro_jpeg.c
@@ -36,6 +36,8 @@ static const unsigned char luma_q_table[] = {
 	0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0x63
 };
 
+static unsigned char luma_q_table_reordered[ARRAY_SIZE(luma_q_table)];
+
 static const unsigned char chroma_q_table[] = {
 	0x11, 0x12, 0x18, 0x2f, 0x63, 0x63, 0x63, 0x63,
 	0x12, 0x15, 0x1a, 0x42, 0x63, 0x63, 0x63, 0x63,
@@ -47,6 +49,30 @@ static const unsigned char chroma_q_table[] = {
 	0x63, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63
 };
 
+static unsigned char chroma_q_table_reordered[ARRAY_SIZE(chroma_q_table)];
+
+static const unsigned char zigzag[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 const u32 hw_reorder[64] = {
+	 0,  8, 16, 24,  1,  9, 17, 25,
+	32, 40, 48, 56, 33, 41, 49, 57,
+	 2, 10, 18, 26,  3, 11, 19, 27,
+	34, 42, 50, 58, 35, 43, 51, 59,
+	 4, 12, 20, 28,  5, 13, 21, 29,
+	36, 44, 52, 60, 37, 45, 53, 61,
+	 6, 14, 22, 30,  7, 15, 23, 31,
+	38, 46, 54, 62, 39, 47, 55, 63
+};
+
 /* Huffman tables are shared with CODA */
 static const unsigned char luma_dc_table[] = {
 	0x00, 0x01, 0x05, 0x01, 0x01, 0x01, 0x01, 0x01,
@@ -225,20 +251,29 @@ static const unsigned char hantro_jpeg_header[JPEG_HEADER_SIZE] = {
 	0x11, 0x03, 0x11, 0x00, 0x3f, 0x00,
 };
 
+static unsigned char jpeg_scale_qp(const unsigned char qp, int scale)
+{
+	unsigned int temp;
+
+	temp = DIV_ROUND_CLOSEST((unsigned int)qp * scale, 100);
+	if (temp <= 0)
+		temp = 1;
+	if (temp > 255)
+		temp = 255;
+
+	return (unsigned char)temp;
+}
+
 static void
-jpeg_scale_quant_table(unsigned char *q_tab,
+jpeg_scale_quant_table(unsigned char *file_q_tab,
+		       unsigned char *reordered_q_tab,
 		       const unsigned char *tab, int scale)
 {
-	unsigned int temp;
 	int i;
 
 	for (i = 0; i < 64; i++) {
-		temp = DIV_ROUND_CLOSEST((unsigned int)tab[i] * scale, 100);
-		if (temp <= 0)
-			temp = 1;
-		if (temp > 255)
-			temp = 255;
-		q_tab[i] = (unsigned char)temp;
+		file_q_tab[i] = jpeg_scale_qp(tab[zigzag[i]], scale);
+		reordered_q_tab[i] = jpeg_scale_qp(tab[hw_reorder[i]], scale);
 	}
 }
 
@@ -256,17 +291,18 @@ static void jpeg_set_quality(unsigned char *buffer, int quality)
 		scale = 200 - 2 * quality;
 
 	jpeg_scale_quant_table(buffer + LUMA_QUANT_OFF,
+			       luma_q_table_reordered,
 			       luma_q_table, scale);
 	jpeg_scale_quant_table(buffer + CHROMA_QUANT_OFF,
+			       chroma_q_table_reordered,
 			       chroma_q_table, scale);
 }
 
-unsigned char *
-hantro_jpeg_get_qtable(struct hantro_jpeg_ctx *ctx, int index)
+unsigned char *hantro_jpeg_get_qtable(int index)
 {
 	if (index == 0)
-		return ctx->buffer + LUMA_QUANT_OFF;
-	return ctx->buffer + CHROMA_QUANT_OFF;
+		return luma_q_table_reordered;
+	return chroma_q_table_reordered;
 }
 
 void hantro_jpeg_header_assemble(struct hantro_jpeg_ctx *ctx)
diff --git a/drivers/staging/media/hantro/hantro_jpeg.h b/drivers/staging/media/hantro/hantro_jpeg.h
index 9e8397c71388..9474a00277f8 100644
--- a/drivers/staging/media/hantro/hantro_jpeg.h
+++ b/drivers/staging/media/hantro/hantro_jpeg.h
@@ -9,5 +9,5 @@ struct hantro_jpeg_ctx {
 	unsigned char *buffer;
 };
 
-unsigned char *hantro_jpeg_get_qtable(struct hantro_jpeg_ctx *ctx, int index);
+unsigned char *hantro_jpeg_get_qtable(int index);
 void hantro_jpeg_header_assemble(struct hantro_jpeg_ctx *ctx);
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
index bdb95652d6a8..a0cf34073235 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
@@ -18,9 +18,8 @@
  *
  * Quantization luma table values are written to registers
  * VEPU_swreg_0-VEPU_swreg_15, and chroma table values to
- * VEPU_swreg_16-VEPU_swreg_31.
- *
- * JPEG zigzag order is expected on the quantization tables.
+ * VEPU_swreg_16-VEPU_swreg_31. A special order is needed, neither
+ * zigzag, nor linear.
  */
 
 #include <asm/unaligned.h>
@@ -139,8 +138,8 @@ void rk3399_vpu_jpeg_enc_run(struct hantro_ctx *ctx)
 	rk3399_vpu_set_src_img_ctrl(vpu, ctx);
 	rk3399_vpu_jpeg_enc_set_buffers(vpu, ctx, &src_buf->vb2_buf);
 	rk3399_vpu_jpeg_enc_set_qtable(vpu,
-				       hantro_jpeg_get_qtable(&jpeg_ctx, 0),
-				       hantro_jpeg_get_qtable(&jpeg_ctx, 1));
+				       hantro_jpeg_get_qtable(0),
+				       hantro_jpeg_get_qtable(1));
 
 	reg = VEPU_REG_OUTPUT_SWAP32
 		| VEPU_REG_OUTPUT_SWAP16
-- 
2.17.1

[PATCH 4/4] media: hantro: Write quantization table registers in increasing addresses order

[Andrzej Pietrasiewicz]

Luma and chroma qtables need to be written into two 16-register blocks,
each table consisting of 64 bytes total. The blocks are contiguous and
start at offset 0 for luma and at offset 0x40 for chroma.

The seemingly innocent optimization of writing the two blocks using one
loop causes side effects which result in improper values of quantization
tables being used by the hardware during encoding. Visually this results
in macroblocking artifacts around contrasting edges in encoded images. The
artifacts look like horizontally flipped shadows of the said edges.
Changing the write operations to non-relaxed variant doesn't help.

This patch removes this premature optimization and after this change the
macroblocking artifacts around contrasting edges are gone.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/staging/media/hantro/hantro_h1_jpeg_enc.c     | 6 ++++++
 drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
index bd05aea1bd71..fb43ec770e9e 100644
--- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
+++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
@@ -73,10 +73,16 @@ hantro_h1_jpeg_enc_set_qtable(struct hantro_dev *vpu,
 	luma_qtable_p = (__be32 *)luma_qtable;
 	chroma_qtable_p = (__be32 *)chroma_qtable;
 
+	/*
+	 * Quantization table registers must be written in contiguous blocks.
+	 * DO NOT collapse the below two "for" loops into one.
+	 */
 	for (i = 0; i < H1_JPEG_QUANT_TABLE_COUNT; i++) {
 		reg = get_unaligned_be32(&luma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_LUMA_QUAT(i));
+	}
 
+	for (i = 0; i < H1_JPEG_QUANT_TABLE_COUNT; i++) {
 		reg = get_unaligned_be32(&chroma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, H1_REG_JPEG_CHROMA_QUAT(i));
 	}
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
index a0cf34073235..f4dbffda0be7 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
@@ -103,10 +103,16 @@ rk3399_vpu_jpeg_enc_set_qtable(struct hantro_dev *vpu,
 	luma_qtable_p = (__be32 *)luma_qtable;
 	chroma_qtable_p = (__be32 *)chroma_qtable;
 
+	/*
+	 * Quantization table registers must be written in contiguous blocks.
+	 * DO NOT collapse the below two "for" loops into one.
+	 */
 	for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
 		reg = get_unaligned_be32(&luma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
+	}
 
+	for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
 		reg = get_unaligned_be32(&chroma_qtable_p[i]);
 		vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_CHROMA_QUAT(i));
 	}
-- 
2.17.1

Re: [PATCH 0/4] Hantro VPU JPEG encoder fixes

[Ezequiel Garcia]

Hi Andrzej,

Thanks a lot for the fixes!

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> This series addresses quality issues in encoded JPEG images.
> 
> The first patch actually restores the intention of the original submission
> of this driver: due to a typo the helper variables were unused and then
> have been removed in some cleanup done by Mauro.
> 
> The second patch aligns the driver's luma quantization table with
> the one in the ITU-T.81 standard.
> 
> The third patch changes the order in which quantization tables are
> written to the resulting file and to the hardware. The file expects
> a zig-zag order, while the hardware wants some special order, neither
> linear nor zig-zag. In other words, hardware-wise it rearranges which
> parts of quantization tables go into which 4-byte registers - in a hardware
> specific order rather than linear or zig-zag. It also affects rk3288 and
> hasn't been tested with it.
> 
> The fourth patch then rearranges the sequence of register writes.
> The whole luma quantization table must be written first, and then the
> chroma quantization is written. In other words, while patch 3/4
> changes what goes into which register, this patch changes when each
> register is written to. It also affects rk3288 and hasn't been
> tested with it.
> 

I've just tested RK3288, and this series is indeed fixing
these issues. So for all patches:

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

Thanks,
Ezequiel

> Andrzej Pietrasiewicz (4):
>   media: hantro: Read be32 words starting at every fourth byte
>   media: hantro: Use standard luma quantization table
>   media: hantro: Write the quantization tables in proper order
>   media: hantro: Write quantization table registers in increasing
>     addresses order
> 
>  .../staging/media/hantro/hantro_h1_jpeg_enc.c | 19 ++++-
>  drivers/staging/media/hantro/hantro_jpeg.c    | 76 ++++++++++++++-----
>  drivers/staging/media/hantro/hantro_jpeg.h    |  2 +-
>  .../media/hantro/rk3399_vpu_hw_jpeg_enc.c     | 24 ++++--
>  4 files changed, 89 insertions(+), 32 deletions(-)
> 
> -- 
> 2.17.1
> 
> 

Re: [PATCH 0/4] Hantro VPU JPEG encoder fixes

[Andrzej Pietrasiewicz]

Hi All,

<snip>

> I've just tested RK3288, and this series is indeed fixing
> these issues. So for all patches:
> 
> Tested-by: Ezequiel Garcia <ezequiel@collabora.com>

A kind reminder.

The series fixes serious encoding quality problems in both rk3399 and rk3288,
so it seems it should be included. A review is needed, though, at least for
patches 2-4.

Thank you,

Andrzej