[PATCH 0/2] hantro: postproc related fixes

[PATCH 0/2] hantro: postproc related fixes

[Ezequiel Garcia]

Doing some more extended testing, we found a few
issues when the post-processor is enabled (currently
for color conversion).

The two patches below should fix these issues.

Ezequiel Garcia (2):
  hantro: h264: Get the correct fallback reference buffer
  hantro: postproc: Fix motion vector space allocation

 drivers/staging/media/hantro/hantro_h264.c     | 2 +-
 drivers/staging/media/hantro/hantro_hw.h       | 4 ++--
 drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
 3 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/2] hantro: h264: Get the correct fallback reference buffer

[Ezequiel Garcia]

If the bitstream and the application are incorrectly configuring
the reference pictures, the hardware will need to fallback
to using some other reference picture.

When the post-processor is enabled, the fallback buffer
should be a shadow buffer (postproc.dec_q), and not a
CAPTURE queue buffer, since the latter is post-processed
and not really the output of the decoder core.

Fixes: 8c2d66b036c77 ("media: hantro: Support color conversion via post-processing")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_h264.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 194d05848077..6dcd47bd9ed3 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -325,7 +325,7 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
 		 */
 		dst_buf = hantro_get_dst_buf(ctx);
 		buf = &dst_buf->vb2_buf;
-		dma_addr = vb2_dma_contig_plane_dma_addr(buf, 0);
+		dma_addr = hantro_get_dec_buf_addr(ctx, buf);
 	}
 
 	return dma_addr;
-- 
2.27.0

[PATCH 2/2] hantro: postproc: Fix motion vector space allocation

[Ezequiel Garcia]

When the post-processor is enabled, the driver allocates
"shadow buffers" which are used for the decoder core,
and exposes the post-processed buffers to userspace.

For this reason, extra motion vector space has to
be allocated on the shadow buffers, which the driver
wasn't doing. Fix this.

This fix removes artifacts on high profile bitstreams,
found on certain platforms.

While here, fix the MV layout comment, since the multicore
(MC) word is found before the MV buffer.

Fixes: 8c2d66b036c77 ("media: hantro: Support color conversion via post-processing")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_hw.h       | 4 ++--
 drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index f066de6b592d..989564485ca1 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -200,10 +200,10 @@ hantro_h264_mv_size(unsigned int width, unsigned int height)
 	 * +---------------------------+
 	 * | UV-plane  128 bytes x MBs |
 	 * +---------------------------+
-	 * | MV buffer  64 bytes x MBs |
-	 * +---------------------------+
 	 * | MC sync          32 bytes |
 	 * +---------------------------+
+	 * | MV buffer  64 bytes x MBs |
+	 * +---------------------------+
 	 */
 	return 64 * MB_WIDTH(width) * MB_WIDTH(height) + 32;
 }
diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
index 44062ffceaea..6d2a8f2a8f0b 100644
--- a/drivers/staging/media/hantro/hantro_postproc.c
+++ b/drivers/staging/media/hantro/hantro_postproc.c
@@ -118,7 +118,9 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
 	unsigned int num_buffers = cap_queue->num_buffers;
 	unsigned int i, buf_size;
 
-	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
+	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage +
+		   hantro_h264_mv_size(ctx->dst_fmt.width,
+				       ctx->dst_fmt.height);
 
 	for (i = 0; i < num_buffers; ++i) {
 		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
-- 
2.27.0

Re: [PATCH 2/2] hantro: postproc: Fix motion vector space allocation

[Jonas Karlman]

On 2020-07-25 17:52, Ezequiel Garcia wrote:
> When the post-processor is enabled, the driver allocates
> "shadow buffers" which are used for the decoder core,
> and exposes the post-processed buffers to userspace.
> 
> For this reason, extra motion vector space has to
> be allocated on the shadow buffers, which the driver
> wasn't doing. Fix this.
> 
> This fix removes artifacts on high profile bitstreams,
> found on certain platforms.
> 
> While here, fix the MV layout comment, since the multicore
> (MC) word is found before the MV buffer.

If this is the case then the mv offset currently used is probably
wrong for multicore hantro blocks.

The imx-vpu-hantro reference code mentions the following for H.264,

  allocate 32 bytes for multicore status fields
  locate it after picture and direct MV

and for VP8 it locate it directly after picture.

The Rockchip hantro devices I have is not multicore (to my knowledge),
is the iMX8M hantro a multicore block?

Best regards,
Jonas

> 
> Fixes: 8c2d66b036c77 ("media: hantro: Support color conversion via post-processing")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_hw.h       | 4 ++--
>  drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index f066de6b592d..989564485ca1 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -200,10 +200,10 @@ hantro_h264_mv_size(unsigned int width, unsigned int height)
>  	 * +---------------------------+
>  	 * | UV-plane  128 bytes x MBs |
>  	 * +---------------------------+
> -	 * | MV buffer  64 bytes x MBs |
> -	 * +---------------------------+
>  	 * | MC sync          32 bytes |
>  	 * +---------------------------+
> +	 * | MV buffer  64 bytes x MBs |
> +	 * +---------------------------+
>  	 */
>  	return 64 * MB_WIDTH(width) * MB_WIDTH(height) + 32;
>  }
> diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
> index 44062ffceaea..6d2a8f2a8f0b 100644
> --- a/drivers/staging/media/hantro/hantro_postproc.c
> +++ b/drivers/staging/media/hantro/hantro_postproc.c
> @@ -118,7 +118,9 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
>  	unsigned int num_buffers = cap_queue->num_buffers;
>  	unsigned int i, buf_size;
>  
> -	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> +	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage +
> +		   hantro_h264_mv_size(ctx->dst_fmt.width,
> +				       ctx->dst_fmt.height);
>  
>  	for (i = 0; i < num_buffers; ++i) {
>  		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> 

Re: [PATCH 2/2] hantro: postproc: Fix motion vector space allocation

[Ezequiel Garcia]

On Sun, 26 Jul 2020 at 04:42, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2020-07-25 17:52, Ezequiel Garcia wrote:
> > When the post-processor is enabled, the driver allocates
> > "shadow buffers" which are used for the decoder core,
> > and exposes the post-processed buffers to userspace.
> >
> > For this reason, extra motion vector space has to
> > be allocated on the shadow buffers, which the driver
> > wasn't doing. Fix this.
> >
> > This fix removes artifacts on high profile bitstreams,
> > found on certain platforms.
> >
> > While here, fix the MV layout comment, since the multicore
> > (MC) word is found before the MV buffer.
>
> If this is the case then the mv offset currently used is probably
> wrong for multicore hantro blocks.
>

Oh, yes, my bad.

> The imx-vpu-hantro reference code mentions the following for H.264,
>
>   allocate 32 bytes for multicore status fields
>   locate it after picture and direct MV
>
> and for VP8 it locate it directly after picture.
>
> The Rockchip hantro devices I have is not multicore (to my knowledge),
> is the iMX8M hantro a multicore block?
>

Newer cores (newer than G1 and G2) specify the MC status before the
MV buffer, and I was too quick to assume it was the case also for G1.

I'll get a v2.

Thanks for the catch,
Ezequiel

> Best regards,
> Jonas
>
> >
> > Fixes: 8c2d66b036c77 ("media: hantro: Support color conversion via post-processing")
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_hw.h       | 4 ++--
> >  drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index f066de6b592d..989564485ca1 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -200,10 +200,10 @@ hantro_h264_mv_size(unsigned int width, unsigned int height)
> >        * +---------------------------+
> >        * | UV-plane  128 bytes x MBs |
> >        * +---------------------------+
> > -      * | MV buffer  64 bytes x MBs |
> > -      * +---------------------------+
> >        * | MC sync          32 bytes |
> >        * +---------------------------+
> > +      * | MV buffer  64 bytes x MBs |
> > +      * +---------------------------+
> >        */
> >       return 64 * MB_WIDTH(width) * MB_WIDTH(height) + 32;
> >  }
> > diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
> > index 44062ffceaea..6d2a8f2a8f0b 100644
> > --- a/drivers/staging/media/hantro/hantro_postproc.c
> > +++ b/drivers/staging/media/hantro/hantro_postproc.c
> > @@ -118,7 +118,9 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> >       unsigned int num_buffers = cap_queue->num_buffers;
> >       unsigned int i, buf_size;
> >
> > -     buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> > +     buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage +
> > +                hantro_h264_mv_size(ctx->dst_fmt.width,
> > +                                    ctx->dst_fmt.height);
> >
> >       for (i = 0; i < num_buffers; ++i) {
> >               struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> >