| Message ID | 20200725155208.897908-3-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hantro: postproc related fixes | expand |
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]; >
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]; > >
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];
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(-)