[PATCH v3 0/5] media: hantro: H264 fixes and improvements

[PATCH v3 0/5] media: hantro: H264 fixes and improvements

[Jonas Karlman]

This series contains fixes and improvements for the hantro H264 decoder.

Patch 1 and 5 fixes the motion vector buffer offset calculation for field encoded
and monochrome content and makes it possible to decode a sample from [1].

Patch 2 removes overallocation for the motion vector buffer,
only half of the extra size is needed.

Patch 3 changes to use the same source for width and height as is used for
motion vector buffer offset calculation.

The RFC patches that added bits to handle field encoded content have
been dropped and will be resent in a separate series.

The following sample from [1] is now playable with this series
- H264_1080i-25-interlace_Kaesescheibchen.mkv

This series has been tested using ffmpeg v4l2 request hwaccel at [2]

[1] http://kwiboo.libreelec.tv/test/samples/
[2] https://github.com/Kwiboo/FFmpeg/compare/4.0.4-Leia-18.4...v4l2-request-hwaccel-4.0.4-hantro

Changes in v3:
  - rebased on for-v5.5q
  - drop RFC patches
  - src_fmt instead of dst_fmt is used for width/height
  - address feedback from Boris

Changes in v2:
  - scaling list changes split to its own series
  - address feedback from Philipp and Ezequiel

Regards,
Jonas

Jonas Karlman (5):
  media: hantro: Fix H264 motion vector buffer offset
  media: hantro: Reduce H264 extra space for motion vectors
  media: hantro: Use output buffer width and height for H264 decoding
  media: hantro: Remove now unused H264 pic_size
  media: hantro: Set H264 FIELDPIC_FLAG_E flag correctly

 .../staging/media/hantro/hantro_g1_h264_dec.c | 37 +++++++++++++------
 drivers/staging/media/hantro/hantro_h264.c    |  5 ---
 drivers/staging/media/hantro/hantro_hw.h      |  3 --
 drivers/staging/media/hantro/hantro_v4l2.c    | 20 +++++++++-
 4 files changed, 43 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] media: hantro: Fix H264 motion vector buffer offset

[Jonas Karlman]

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock and is laid out in memory as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+

The motion vector buffer offset is currently correct for 4:2:0 because the
extra space for motion vectors is overallocated with an extra 64 bytes x MBs.

Wrong offset for both destination and motion vector buffer are used
for the bottom field of field encoded content, wrong offset is
also used for 4:0:0 (monochrome) content.

Fix this by setting the motion vector address to the expected 384 bytes x MBs
offset for 4:2:0 and 256 bytes x MBs offset for 4:0:0 content.

Also use correct destination and motion vector buffer offset
for the bottom field of field encoded content.

While at it also extend the check for 4:0:0 (monochrome) to include an
additional check for High Profile (100).

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v3:
  * address remarks from Boris
  - use src_fmt instead of dst_fmt
Changes in v2:
  * address remarks from Philipp and Ezequiel
  - update commit message
  - rename offset to bytes_per_mb
  - remove MV_OFFSET macros
  - move PIC_MB_WIDTH/HEIGHT_P change to separate patch
---
 .../staging/media/hantro/hantro_g1_h264_dec.c | 31 +++++++++++++------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 70a6b5b26477..30d977c3d529 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -81,7 +81,7 @@ static void set_params(struct hantro_ctx *ctx)
 		reg |= G1_REG_DEC_CTRL4_CABAC_E;
 	if (sps->flags & V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE)
 		reg |= G1_REG_DEC_CTRL4_DIR_8X8_INFER_E;
-	if (sps->chroma_format_idc == 0)
+	if (sps->profile_idc >= 100 && sps->chroma_format_idc == 0)
 		reg |= G1_REG_DEC_CTRL4_BLACKWHITE_E;
 	if (pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED)
 		reg |= G1_REG_DEC_CTRL4_WEIGHT_PRED_E;
@@ -234,6 +234,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	struct hantro_dev *vpu = ctx->dev;
 	dma_addr_t src_dma, dst_dma;
+	size_t offset = 0;
 
 	src_buf = hantro_get_src_buf(ctx);
 	dst_buf = hantro_get_dst_buf(ctx);
@@ -244,18 +245,30 @@ static void set_buffers(struct hantro_ctx *ctx)
 
 	/* Destination (decoded frame) buffer. */
 	dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
-	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
+	/* Adjust dma addr to start at second line for bottom field */
+	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+		offset = ALIGN(ctx->src_fmt.width, MB_DIM);
+	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
 
 	/* Higher profiles require DMV buffer appended to reference frames. */
 	if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
-		size_t pic_size = ctx->h264_dec.pic_size;
-		size_t mv_offset = round_up(pic_size, 8);
-
+		unsigned int bytes_per_mb = 384;
+
+		/* DMV buffer for monochrome start directly after Y-plane */
+		if (ctrls->sps->profile_idc >= 100 &&
+		    ctrls->sps->chroma_format_idc == 0)
+			bytes_per_mb = 256;
+		offset = bytes_per_mb * MB_WIDTH(ctx->src_fmt.width) *
+			 MB_HEIGHT(ctx->src_fmt.height);
+
+		/*
+		 * DMV buffer is split in two for field encoded frames,
+		 * adjust offset for bottom field
+		 */
 		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
-			mv_offset += 32 * MB_WIDTH(ctx->dst_fmt.width);
-
-		vdpu_write_relaxed(vpu, dst_dma + mv_offset,
-				   G1_REG_ADDR_DIR_MV);
+			offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
+				  MB_HEIGHT(ctx->src_fmt.height);
+		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
 	}
 
 	/* Auxiliary buffer prepared in hantro_g1_h264_dec_prepare_table(). */
-- 
2.17.1

Re: [PATCH v3 1/5] media: hantro: Fix H264 motion vector buffer offset

[Tomasz Figa]

Hi Jonas,

On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> macroblock and is laid out in memory as follow:
>
> +---------------------------+
> | Y-plane   256 bytes x MBs |
> +---------------------------+
> | UV-plane  128 bytes x MBs |
> +---------------------------+
> | MV buffer  64 bytes x MBs |
> +---------------------------+
>
> The motion vector buffer offset is currently correct for 4:2:0 because the
> extra space for motion vectors is overallocated with an extra 64 bytes x MBs.
>
> Wrong offset for both destination and motion vector buffer are used
> for the bottom field of field encoded content, wrong offset is
> also used for 4:0:0 (monochrome) content.
>
> Fix this by setting the motion vector address to the expected 384 bytes x MBs
> offset for 4:2:0 and 256 bytes x MBs offset for 4:0:0 content.
>
> Also use correct destination and motion vector buffer offset
> for the bottom field of field encoded content.
>
> While at it also extend the check for 4:0:0 (monochrome) to include an
> additional check for High Profile (100).
>
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v3:
>   * address remarks from Boris
>   - use src_fmt instead of dst_fmt
> Changes in v2:
>   * address remarks from Philipp and Ezequiel
>   - update commit message
>   - rename offset to bytes_per_mb
>   - remove MV_OFFSET macros
>   - move PIC_MB_WIDTH/HEIGHT_P change to separate patch
> ---
>  .../staging/media/hantro/hantro_g1_h264_dec.c | 31 +++++++++++++------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>

First of all, thanks for the patches! Good to see more members of the
community contributing to the driver.

Please find my comments inline.

> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 70a6b5b26477..30d977c3d529 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -81,7 +81,7 @@ static void set_params(struct hantro_ctx *ctx)
>                 reg |= G1_REG_DEC_CTRL4_CABAC_E;
>         if (sps->flags & V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE)
>                 reg |= G1_REG_DEC_CTRL4_DIR_8X8_INFER_E;
> -       if (sps->chroma_format_idc == 0)
> +       if (sps->profile_idc >= 100 && sps->chroma_format_idc == 0)

I'd rather make this a separate patch with proper explanation in commit message.

>                 reg |= G1_REG_DEC_CTRL4_BLACKWHITE_E;
>         if (pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED)
>                 reg |= G1_REG_DEC_CTRL4_WEIGHT_PRED_E;
> @@ -234,6 +234,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>         struct vb2_v4l2_buffer *src_buf, *dst_buf;
>         struct hantro_dev *vpu = ctx->dev;
>         dma_addr_t src_dma, dst_dma;
> +       size_t offset = 0;
>
>         src_buf = hantro_get_src_buf(ctx);
>         dst_buf = hantro_get_dst_buf(ctx);
> @@ -244,18 +245,30 @@ static void set_buffers(struct hantro_ctx *ctx)
>
>         /* Destination (decoded frame) buffer. */
>         dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> -       vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
> +       /* Adjust dma addr to start at second line for bottom field */
> +       if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +               offset = ALIGN(ctx->src_fmt.width, MB_DIM);

Isn't ctx->src_fmt.width already aligned to MB_DIM?

Also, offset is in bytes, so should we rather use the bytesperline field?

> +       vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
>
>         /* Higher profiles require DMV buffer appended to reference frames. */
>         if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
> -               size_t pic_size = ctx->h264_dec.pic_size;
> -               size_t mv_offset = round_up(pic_size, 8);
> -
> +               unsigned int bytes_per_mb = 384;
> +
> +               /* DMV buffer for monochrome start directly after Y-plane */
> +               if (ctrls->sps->profile_idc >= 100 &&
> +                   ctrls->sps->chroma_format_idc == 0)
> +                       bytes_per_mb = 256;

nit: Adding a blank line here would make it much easier to read.

> +               offset = bytes_per_mb * MB_WIDTH(ctx->src_fmt.width) *
> +                        MB_HEIGHT(ctx->src_fmt.height);

It's kind of difficult to follow with this idea of bytes_per_mb IMHO.
Would it perhaps make sense to rewrite the code as below?

luma_size = ctx->src_fmt.planes[0].bytesperline * ctx->src_fmt.height;

if (ctrls->sps->profile_idc >= 100 &&
    ctrls->sps->chroma_format_idc == 0)
        chroma_size = 0;
else
        chroma_size = ctx->src_fmt.planes[0].bytesperline *
ctx->src_fmt.height / 4;

offset = luma_size + chroma_size;

Also, the code only handles 4:2:0 and 4:0:0. How about 4:2:2?

Best regards,
Tomasz

> +
> +               /*
> +                * DMV buffer is split in two for field encoded frames,
> +                * adjust offset for bottom field
> +                */
>                 if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> -                       mv_offset += 32 * MB_WIDTH(ctx->dst_fmt.width);
> -
> -               vdpu_write_relaxed(vpu, dst_dma + mv_offset,
> -                                  G1_REG_ADDR_DIR_MV);
> +                       offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
> +                                 MB_HEIGHT(ctx->src_fmt.height);
> +               vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
>         }
>
>         /* Auxiliary buffer prepared in hantro_g1_h264_dec_prepare_table(). */
> --
> 2.17.1
>

[PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Jonas Karlman]

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock with additional 32 bytes on multi-core variants.

Memory layout is as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+
| MC sync          32 bytes |
+---------------------------+

Reduce the extra space allocated now that motion vector buffer offset no
longer is based on the extra space.

Only allocate extra space for 64 bytes x MBs of motion vector buffer
and 32 bytes for multi-core sync.

Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v3:
  - add memory layout to code comment (Boris)
Changes in v2:
  - updated commit message
---
 drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 3dae52abb96c..c8c896a06f58 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -240,14 +240,30 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
 		v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width,
 				    pix_mp->height);
 		/*
+		 * A decoded 8-bit 4:2:0 NV12 frame may need memory for up to
+		 * 448 bytes per macroblock with additional 32 bytes on
+		 * multi-core variants.
+		 *
 		 * The H264 decoder needs extra space on the output buffers
 		 * to store motion vectors. This is needed for reference
 		 * frames.
+		 *
+		 * Memory layout is as follow:
+		 *
+		 * +---------------------------+
+		 * | Y-plane   256 bytes x MBs |
+		 * +---------------------------+
+		 * | UV-plane  128 bytes x MBs |
+		 * +---------------------------+
+		 * | MV buffer  64 bytes x MBs |
+		 * +---------------------------+
+		 * | MC sync          32 bytes |
+		 * +---------------------------+
 		 */
 		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
 			pix_mp->plane_fmt[0].sizeimage +=
-				128 * DIV_ROUND_UP(pix_mp->width, 16) *
-				      DIV_ROUND_UP(pix_mp->height, 16);
+				64 * MB_WIDTH(pix_mp->width) *
+				     MB_WIDTH(pix_mp->height) + 32;
 	} else if (!pix_mp->plane_fmt[0].sizeimage) {
 		/*
 		 * For coded formats the application can specify
-- 
2.17.1

Re: [PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Tomasz Figa]

Hi Jonas,

On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> macroblock with additional 32 bytes on multi-core variants.
>
> Memory layout is as follow:
>
> +---------------------------+
> | Y-plane   256 bytes x MBs |
> +---------------------------+
> | UV-plane  128 bytes x MBs |
> +---------------------------+
> | MV buffer  64 bytes x MBs |
> +---------------------------+
> | MC sync          32 bytes |
> +---------------------------+
>
> Reduce the extra space allocated now that motion vector buffer offset no
> longer is based on the extra space.
>
> Only allocate extra space for 64 bytes x MBs of motion vector buffer
> and 32 bytes for multi-core sync.
>
> Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v3:
>   - add memory layout to code comment (Boris)
> Changes in v2:
>   - updated commit message
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>

Thanks for the patch!

What platform did you test it on and how? Was it tested with IOMMU enabled?

Best regards,
Tomasz

> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 3dae52abb96c..c8c896a06f58 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -240,14 +240,30 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
>                 v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width,
>                                     pix_mp->height);
>                 /*
> +                * A decoded 8-bit 4:2:0 NV12 frame may need memory for up to
> +                * 448 bytes per macroblock with additional 32 bytes on
> +                * multi-core variants.
> +                *
>                  * The H264 decoder needs extra space on the output buffers
>                  * to store motion vectors. This is needed for reference
>                  * frames.
> +                *
> +                * Memory layout is as follow:
> +                *
> +                * +---------------------------+
> +                * | Y-plane   256 bytes x MBs |
> +                * +---------------------------+
> +                * | UV-plane  128 bytes x MBs |
> +                * +---------------------------+
> +                * | MV buffer  64 bytes x MBs |
> +                * +---------------------------+
> +                * | MC sync          32 bytes |
> +                * +---------------------------+
>                  */
>                 if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
>                         pix_mp->plane_fmt[0].sizeimage +=
> -                               128 * DIV_ROUND_UP(pix_mp->width, 16) *
> -                                     DIV_ROUND_UP(pix_mp->height, 16);
> +                               64 * MB_WIDTH(pix_mp->width) *
> +                                    MB_WIDTH(pix_mp->height) + 32;
>         } else if (!pix_mp->plane_fmt[0].sizeimage) {
>                 /*
>                  * For coded formats the application can specify
> --
> 2.17.1
>

Re: [PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Ezequiel Garcia]

On Wed, 2019-11-20 at 21:44 +0900, Tomasz Figa wrote:
> Hi Jonas,
> 
> On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> > A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> > macroblock with additional 32 bytes on multi-core variants.
> > 
> > Memory layout is as follow:
> > 
> > +---------------------------+
> > > Y-plane   256 bytes x MBs |
> > +---------------------------+
> > > UV-plane  128 bytes x MBs |
> > +---------------------------+
> > > MV buffer  64 bytes x MBs |
> > +---------------------------+
> > > MC sync          32 bytes |
> > +---------------------------+
> > 
> > Reduce the extra space allocated now that motion vector buffer offset no
> > longer is based on the extra space.
> > 
> > Only allocate extra space for 64 bytes x MBs of motion vector buffer
> > and 32 bytes for multi-core sync.
> > 
> > Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Changes in v3:
> >   - add memory layout to code comment (Boris)
> > Changes in v2:
> >   - updated commit message
> > ---
> >  drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> 
> Thanks for the patch!
> 
> What platform did you test it on and how? Was it tested with IOMMU enabled?

Hello Tomasz,

Please note that this series has been picked-up and is merged
in v5.5-rc1.

IIRC, we tested these patches on RK3399 and RK3288 (that means
with an IOMMU). I've just ran some more extensive tests on RK3288,
on media/master; and I plan to test some more on RK3399 later this week.

Do you have any specific concern in mind?

Thanks,
Ezequiel

Re: [PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Tomasz Figa]

On Tue, Dec 10, 2019 at 3:11 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> On Wed, 2019-11-20 at 21:44 +0900, Tomasz Figa wrote:
> > Hi Jonas,
> >
> > On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> > > A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> > > macroblock with additional 32 bytes on multi-core variants.
> > >
> > > Memory layout is as follow:
> > >
> > > +---------------------------+
> > > > Y-plane   256 bytes x MBs |
> > > +---------------------------+
> > > > UV-plane  128 bytes x MBs |
> > > +---------------------------+
> > > > MV buffer  64 bytes x MBs |
> > > +---------------------------+
> > > > MC sync          32 bytes |
> > > +---------------------------+
> > >
> > > Reduce the extra space allocated now that motion vector buffer offset no
> > > longer is based on the extra space.
> > >
> > > Only allocate extra space for 64 bytes x MBs of motion vector buffer
> > > and 32 bytes for multi-core sync.
> > >
> > > Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > > Changes in v3:
> > >   - add memory layout to code comment (Boris)
> > > Changes in v2:
> > >   - updated commit message
> > > ---
> > >  drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> >
> > Thanks for the patch!
> >
> > What platform did you test it on and how? Was it tested with IOMMU enabled?
>
> Hello Tomasz,
>
> Please note that this series has been picked-up and is merged
> in v5.5-rc1.
>
> IIRC, we tested these patches on RK3399 and RK3288 (that means
> with an IOMMU). I've just ran some more extensive tests on RK3288,
> on media/master; and I plan to test some more on RK3399 later this week.
>
> Do you have any specific concern in mind?

I specifically want to know whether we're 100% sure that those sizes
are correct. The IOMMU still works on page granularity so it's
possible that the allocation could be just big enough by luck.

Best regards,
Tomasz

Re: [PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Jonas Karlman]

On 2020-01-08 13:59, Tomasz Figa wrote:
> On Tue, Dec 10, 2019 at 3:11 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>
>> On Wed, 2019-11-20 at 21:44 +0900, Tomasz Figa wrote:
>>> Hi Jonas,
>>>
>>> On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>>>> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
>>>> macroblock with additional 32 bytes on multi-core variants.
>>>>
>>>> Memory layout is as follow:
>>>>
>>>> +---------------------------+
>>>>> Y-plane   256 bytes x MBs |
>>>> +---------------------------+
>>>>> UV-plane  128 bytes x MBs |
>>>> +---------------------------+
>>>>> MV buffer  64 bytes x MBs |
>>>> +---------------------------+
>>>>> MC sync          32 bytes |
>>>> +---------------------------+
>>>>
>>>> Reduce the extra space allocated now that motion vector buffer offset no
>>>> longer is based on the extra space.
>>>>
>>>> Only allocate extra space for 64 bytes x MBs of motion vector buffer
>>>> and 32 bytes for multi-core sync.
>>>>
>>>> Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
>>>> ---
>>>> Changes in v3:
>>>>   - add memory layout to code comment (Boris)
>>>> Changes in v2:
>>>>   - updated commit message
>>>> ---
>>>>  drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
>>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Thanks for the patch!
>>>
>>> What platform did you test it on and how? Was it tested with IOMMU enabled?
>>
>> Hello Tomasz,
>>
>> Please note that this series has been picked-up and is merged
>> in v5.5-rc1.
>>
>> IIRC, we tested these patches on RK3399 and RK3288 (that means
>> with an IOMMU). I've just ran some more extensive tests on RK3288,
>> on media/master; and I plan to test some more on RK3399 later this week.
>>
>> Do you have any specific concern in mind?
> 
> I specifically want to know whether we're 100% sure that those sizes
> are correct. The IOMMU still works on page granularity so it's
> possible that the allocation could be just big enough by luck.

One of my RK3288 TRM [1] contains the following:

Direct mode motion vector write:
  Its base addr is right after decode output picture data
  Its length is mbwidth*mbheight*64

Also both the mpp library and imx-vpu-hantro code both use mbwidth*mbheight*64.
So I feel confident that the buffer size is correct.

[1] Rockchip RK3288TRM V1.1 Part3-Graphic and multi-media.pdf

Regards,
Jonas

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v3 2/5] media: hantro: Reduce H264 extra space for motion vectors

[Tomasz Figa]

On Thu, Jan 9, 2020 at 12:10 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2020-01-08 13:59, Tomasz Figa wrote:
> > On Tue, Dec 10, 2019 at 3:11 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >>
> >> On Wed, 2019-11-20 at 21:44 +0900, Tomasz Figa wrote:
> >>> Hi Jonas,
> >>>
> >>> On Thu, Nov 7, 2019 at 7:34 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>>> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> >>>> macroblock with additional 32 bytes on multi-core variants.
> >>>>
> >>>> Memory layout is as follow:
> >>>>
> >>>> +---------------------------+
> >>>>> Y-plane   256 bytes x MBs |
> >>>> +---------------------------+
> >>>>> UV-plane  128 bytes x MBs |
> >>>> +---------------------------+
> >>>>> MV buffer  64 bytes x MBs |
> >>>> +---------------------------+
> >>>>> MC sync          32 bytes |
> >>>> +---------------------------+
> >>>>
> >>>> Reduce the extra space allocated now that motion vector buffer offset no
> >>>> longer is based on the extra space.
> >>>>
> >>>> Only allocate extra space for 64 bytes x MBs of motion vector buffer
> >>>> and 32 bytes for multi-core sync.
> >>>>
> >>>> Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
> >>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> >>>> ---
> >>>> Changes in v3:
> >>>>   - add memory layout to code comment (Boris)
> >>>> Changes in v2:
> >>>>   - updated commit message
> >>>> ---
> >>>>  drivers/staging/media/hantro/hantro_v4l2.c | 20 ++++++++++++++++++--
> >>>>  1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>
> >>>
> >>> Thanks for the patch!
> >>>
> >>> What platform did you test it on and how? Was it tested with IOMMU enabled?
> >>
> >> Hello Tomasz,
> >>
> >> Please note that this series has been picked-up and is merged
> >> in v5.5-rc1.
> >>
> >> IIRC, we tested these patches on RK3399 and RK3288 (that means
> >> with an IOMMU). I've just ran some more extensive tests on RK3288,
> >> on media/master; and I plan to test some more on RK3399 later this week.
> >>
> >> Do you have any specific concern in mind?
> >
> > I specifically want to know whether we're 100% sure that those sizes
> > are correct. The IOMMU still works on page granularity so it's
> > possible that the allocation could be just big enough by luck.
>
> One of my RK3288 TRM [1] contains the following:
>
> Direct mode motion vector write:
>   Its base addr is right after decode output picture data
>   Its length is mbwidth*mbheight*64
>
> Also both the mpp library and imx-vpu-hantro code both use mbwidth*mbheight*64.
> So I feel confident that the buffer size is correct.
>
> [1] Rockchip RK3288TRM V1.1 Part3-Graphic and multi-media.pdf

Fair enough. Thanks!

Best regards,
Tomasz

[PATCH v3 3/5] media: hantro: Use output buffer width and height for H264 decoding

[Jonas Karlman]

Calculations for motion vector buffer offset is based on width and height
from the configured output format, lets use the same values for macroblock
width and height hw regs.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v3:
  - change to use src_fmt instead of dst_fmt (Boris)
Changes in v2:
  - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 30d977c3d529..27d40d8d3728 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
 
 	/* Decoder control register 1. */
-	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
-	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
+	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
+	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
 	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
 
-- 
2.17.1

Re: [PATCH v3 3/5] media: hantro: Use output buffer width and height for H264 decoding

[Boris Brezillon]

On Wed, 6 Nov 2019 22:34:22 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> Calculations for motion vector buffer offset is based on width and height
> from the configured output format, lets use the same values for macroblock
> width and height hw regs.

Still don't see what was the problem with
sps->pic_{width,height}_in_mbs_minus1, but okay.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")

Is this really fixing a bug? Do you have cases where
->pic_{width,height}_in_mbs_minus1 and
MB_{WIDTH,HEIGHT}(src_fmt.{width,height}) do not match?

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v3:
>   - change to use src_fmt instead of dst_fmt (Boris)
> Changes in v2:
>   - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 30d977c3d529..27d40d8d3728 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>  
>  	/* Decoder control register 1. */
> -	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
> -	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
> +	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
> +	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
>  	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
>  

[PATCH v3 4/5] media: hantro: Remove now unused H264 pic_size

[Jonas Karlman]

pic_size in hantro_h264_dec_hw_ctx struct is no longer used,
lets remove it.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/staging/media/hantro/hantro_h264.c | 5 -----
 drivers/staging/media/hantro/hantro_hw.h   | 3 ---
 2 files changed, 8 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 694a330f508e..568640eab3a6 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -618,7 +618,6 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
 	struct hantro_h264_dec_hw_ctx *h264_dec = &ctx->h264_dec;
 	struct hantro_aux_buf *priv = &h264_dec->priv;
 	struct hantro_h264_dec_priv_tbl *tbl;
-	struct v4l2_pix_format_mplane pix_mp;
 
 	priv->cpu = dma_alloc_coherent(vpu->dev, sizeof(*tbl), &priv->dma,
 				       GFP_KERNEL);
@@ -629,9 +628,5 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
 	tbl = priv->cpu;
 	memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table));
 
-	v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat,
-			    ctx->dst_fmt.width, ctx->dst_fmt.height);
-	h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage;
-
 	return 0;
 }
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 69b88f4d3fb3..fa91dd1848b7 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -80,15 +80,12 @@ struct hantro_h264_dec_reflists {
  * @dpb:	DPB
  * @reflists:	P/B0/B1 reflists
  * @ctrls:	V4L2 controls attached to a run
- * @pic_size:	Size in bytes of decoded picture, this is needed
- *		to pass the location of motion vectors.
  */
 struct hantro_h264_dec_hw_ctx {
 	struct hantro_aux_buf priv;
 	struct v4l2_h264_dpb_entry dpb[HANTRO_H264_DPB_SIZE];
 	struct hantro_h264_dec_reflists reflists;
 	struct hantro_h264_dec_ctrls ctrls;
-	size_t pic_size;
 };
 
 /**
-- 
2.17.1

[PATCH v3 5/5] media: hantro: Set H264 FIELDPIC_FLAG_E flag correctly

[Jonas Karlman]

The FIELDPIC_FLAG_E bit should be set when field_pic_flag exists in stream,
it is currently set based on field_pic_flag of current frame.
The PIC_FIELDMODE_E bit is correctly set based on the field_pic_flag.

Fix this by setting the FIELDPIC_FLAG_E bit when frame_mbs_only is not set.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 27d40d8d3728..3cd40a8f0daa 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -63,7 +63,7 @@ static void set_params(struct hantro_ctx *ctx)
 	/* always use the matrix sent from userspace */
 	reg |= G1_REG_DEC_CTRL2_TYPE1_QUANT_E;
 
-	if (slices[0].flags &  V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
 		reg |= G1_REG_DEC_CTRL2_FIELDPIC_FLAG_E;
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL2);
 
-- 
2.17.1