diff mbox series

[1/2] media: cedrus: Fix decoding for some HEVC videos

Message ID 20191213161516.54688-2-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show
Series media: cedrus: hevc: Minor fixes | expand

Commit Message

Jernej Škrabec Dec. 13, 2019, 4:15 p.m. UTC 
It seems that for some HEVC videos at least one bitstream parsing
trigger must be called in order to be decoded correctly. There is no
explanation why this helps, but it was observed that several videos
with this fix are now decoded correctly and there is no regression with
others.

Without this fix, those same videos totally crash HEVC decoder (other
decoder engines are unaffected). After decoding those problematic
videos, HEVC decoder always returns only green image (all zeros).
Only complete HW reset helps.

This fix is similar to that for H264.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 25 ++++++++++++++++---
 .../staging/media/sunxi/cedrus/cedrus_regs.h  |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

Comments

Paul Kocialkowski Dec. 18, 2019, 8:40 a.m. UTC | #1 
Hi,

On Fri 13 Dec 19, 17:15, Jernej Skrabec wrote:
> It seems that for some HEVC videos at least one bitstream parsing
> trigger must be called in order to be decoded correctly. There is no
> explanation why this helps, but it was observed that several videos
> with this fix are now decoded correctly and there is no regression with
> others.
> 
> Without this fix, those same videos totally crash HEVC decoder (other
> decoder engines are unaffected). After decoding those problematic
> videos, HEVC decoder always returns only green image (all zeros).
> Only complete HW reset helps.
> 
> This fix is similar to that for H264.

Thanks for the fix, interesting that the same issue shows up on HEVC!
I suspect that Allwinner folks never really tested the engine without
using it for bitstream parsing.

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 25 ++++++++++++++++---
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  1 +
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 109d3289418c..5a207f1e137c 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -7,6 +7,7 @@
>   * Copyright (C) 2018 Bootlin
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/types.h>
>  
>  #include <media/videobuf2-dma-contig.h>
> @@ -283,6 +284,23 @@ static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
>  		}
>  }
>  
> +static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num)
> +{
> +	int count = 0;
> +
> +	while (count < num) {
> +		int tmp = min(num - count, 32);
> +
> +		cedrus_write(dev, VE_DEC_H265_TRIGGER,
> +			     VE_DEC_H265_TRIGGER_FLUSH_BITS |
> +			     VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp));
> +		while (cedrus_read(dev, VE_DEC_H265_STATUS) & VE_DEC_H265_STATUS_VLD_BUSY)
> +			udelay(1);
> +
> +		count += tmp;
> +	}
> +}
> +
>  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  			      struct cedrus_run *run)
>  {
> @@ -347,10 +365,9 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  
>  	/* Source offset and length in bits. */
>  
> -	reg = slice_params->data_bit_offset;
> -	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, reg);
> +	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0);
>  
> -	reg = slice_params->bit_size - slice_params->data_bit_offset;
> +	reg = slice_params->bit_size;
>  	cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg);
>  
>  	/* Source beginning and end addresses. */
> @@ -385,6 +402,8 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  	/* Initialize bitstream access. */
>  	cedrus_write(dev, VE_DEC_H265_TRIGGER, VE_DEC_H265_TRIGGER_INIT_SWDEC);
>  
> +	cedrus_h265_skip_bits(dev, slice_params->data_bit_offset);
> +
>  	/* Bitstream parameters. */
>  
>  	reg = VE_DEC_H265_DEC_NAL_HDR_NAL_UNIT_TYPE(slice_params->nal_unit_type) |
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> index 0d9449fe2b28..df1cceef8d93 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> @@ -424,6 +424,7 @@
>  
>  #define VE_DEC_H265_TRIGGER			(VE_ENGINE_DEC_H265 + 0x34)
>  
> +#define VE_DEC_H265_TRIGGER_TYPE_N_BITS(x)	(((x) & 0x3f) << 8)
>  #define VE_DEC_H265_TRIGGER_STCD_VC1		(0x02 << 4)
>  #define VE_DEC_H265_TRIGGER_STCD_AVS		(0x01 << 4)
>  #define VE_DEC_H265_TRIGGER_STCD_HEVC		(0x00 << 4)
> -- 
> 2.24.0
>
Jernej Škrabec Dec. 18, 2019, 4:40 p.m. UTC | #2 
Hi!

Dne sreda, 18. december 2019 ob 09:40:47 CET je Paul Kocialkowski napisal(a):
> Hi,
> 
> On Fri 13 Dec 19, 17:15, Jernej Skrabec wrote:
> > It seems that for some HEVC videos at least one bitstream parsing
> > trigger must be called in order to be decoded correctly. There is no
> > explanation why this helps, but it was observed that several videos
> > with this fix are now decoded correctly and there is no regression with
> > others.
> > 
> > Without this fix, those same videos totally crash HEVC decoder (other
> > decoder engines are unaffected). After decoding those problematic
> > videos, HEVC decoder always returns only green image (all zeros).
> > Only complete HW reset helps.
> > 
> > This fix is similar to that for H264.
> 
> Thanks for the fix, interesting that the same issue shows up on HEVC!
> I suspect that Allwinner folks never really tested the engine without
> using it for bitstream parsing.

That thought also crossed my mind. It's even worse with VP8. There you can't 
have proper decoding at all without calling one specific bitstream parsing 
function.

> 
> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Thanks!

Best regards,
Jernej

> 
> Cheers,
> 
> Paul
> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 25 ++++++++++++++++---
> >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  1 +
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > 109d3289418c..5a207f1e137c 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -7,6 +7,7 @@
> > 
> >   * Copyright (C) 2018 Bootlin
> >   */
> > 
> > +#include <linux/delay.h>
> > 
> >  #include <linux/types.h>
> >  
> >  #include <media/videobuf2-dma-contig.h>
> > 
> > @@ -283,6 +284,23 @@ static void cedrus_h265_write_scaling_list(struct
> > cedrus_ctx *ctx,> 
> >  		}
> >  
> >  }
> > 
> > +static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num)
> > +{
> > +	int count = 0;
> > +
> > +	while (count < num) {
> > +		int tmp = min(num - count, 32);
> > +
> > +		cedrus_write(dev, VE_DEC_H265_TRIGGER,
> > +			     VE_DEC_H265_TRIGGER_FLUSH_BITS |
> > +			     VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp));
> > +		while (cedrus_read(dev, VE_DEC_H265_STATUS) &
> > VE_DEC_H265_STATUS_VLD_BUSY) +			udelay(1);
> > +
> > +		count += tmp;
> > +	}
> > +}
> > +
> > 
> >  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> >  
> >  			      struct cedrus_run *run)
> >  
> >  {
> > 
> > @@ -347,10 +365,9 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > 
> >  	/* Source offset and length in bits. */
> > 
> > -	reg = slice_params->data_bit_offset;
> > -	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, reg);
> > +	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0);
> > 
> > -	reg = slice_params->bit_size - slice_params->data_bit_offset;
> > +	reg = slice_params->bit_size;
> > 
> >  	cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg);
> >  	
> >  	/* Source beginning and end addresses. */
> > 
> > @@ -385,6 +402,8 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > 
> >  	/* Initialize bitstream access. */
> >  	cedrus_write(dev, VE_DEC_H265_TRIGGER, 
VE_DEC_H265_TRIGGER_INIT_SWDEC);
> > 
> > +	cedrus_h265_skip_bits(dev, slice_params->data_bit_offset);
> > +
> > 
> >  	/* Bitstream parameters. */
> >  	
> >  	reg = VE_DEC_H265_DEC_NAL_HDR_NAL_UNIT_TYPE(slice_params-
>nal_unit_type)
> >  	|
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > 0d9449fe2b28..df1cceef8d93 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > @@ -424,6 +424,7 @@
> > 
> >  #define VE_DEC_H265_TRIGGER			(VE_ENGINE_DEC_H265 + 
0x34)
> > 
> > +#define VE_DEC_H265_TRIGGER_TYPE_N_BITS(x)	(((x) & 0x3f) << 8)
> > 
> >  #define VE_DEC_H265_TRIGGER_STCD_VC1		(0x02 << 4)
> >  #define VE_DEC_H265_TRIGGER_STCD_AVS		(0x01 << 4)
> >  #define VE_DEC_H265_TRIGGER_STCD_HEVC		(0x00 << 4)
diff mbox series

Patch

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 109d3289418c..5a207f1e137c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -7,6 +7,7 @@ 
  * Copyright (C) 2018 Bootlin
  */
 
+#include <linux/delay.h>
 #include <linux/types.h>
 
 #include <media/videobuf2-dma-contig.h>
@@ -283,6 +284,23 @@  static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
 		}
 }
 
+static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num)
+{
+	int count = 0;
+
+	while (count < num) {
+		int tmp = min(num - count, 32);
+
+		cedrus_write(dev, VE_DEC_H265_TRIGGER,
+			     VE_DEC_H265_TRIGGER_FLUSH_BITS |
+			     VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp));
+		while (cedrus_read(dev, VE_DEC_H265_STATUS) & VE_DEC_H265_STATUS_VLD_BUSY)
+			udelay(1);
+
+		count += tmp;
+	}
+}
+
 static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 			      struct cedrus_run *run)
 {
@@ -347,10 +365,9 @@  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 
 	/* Source offset and length in bits. */
 
-	reg = slice_params->data_bit_offset;
-	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, reg);
+	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0);
 
-	reg = slice_params->bit_size - slice_params->data_bit_offset;
+	reg = slice_params->bit_size;
 	cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg);
 
 	/* Source beginning and end addresses. */
@@ -385,6 +402,8 @@  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	/* Initialize bitstream access. */
 	cedrus_write(dev, VE_DEC_H265_TRIGGER, VE_DEC_H265_TRIGGER_INIT_SWDEC);
 
+	cedrus_h265_skip_bits(dev, slice_params->data_bit_offset);
+
 	/* Bitstream parameters. */
 
 	reg = VE_DEC_H265_DEC_NAL_HDR_NAL_UNIT_TYPE(slice_params->nal_unit_type) |
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
index 0d9449fe2b28..df1cceef8d93 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
@@ -424,6 +424,7 @@ 
 
 #define VE_DEC_H265_TRIGGER			(VE_ENGINE_DEC_H265 + 0x34)
 
+#define VE_DEC_H265_TRIGGER_TYPE_N_BITS(x)	(((x) & 0x3f) << 8)
 #define VE_DEC_H265_TRIGGER_STCD_VC1		(0x02 << 4)
 #define VE_DEC_H265_TRIGGER_STCD_AVS		(0x01 << 4)
 #define VE_DEC_H265_TRIGGER_STCD_HEVC		(0x00 << 4)