[PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Justin Green]

Set allow_cache_hints to 1 for the vb2_queue source and destination queues
in the mediatek vcodec V4L2 driver. This allows us to allocate buffers
with the V4L2_MEMORY_FLAG_NON_COHERENT set. On Mediatek SoCs, this enables
caching for this memory, which vastly improves performance when being read
from CPU. Read performance for these buffers is in turn important for
detiling MM21 video frames in software.

This change should be safe from race conditions since videobuf2 already
invalidates or flushes the appropriate cache lines in its prepare() and
finish() methods.

Tested on a MT8183 SoC. Resulted in both correct detiling and a 10X
speedup.

Signed-off-by: Justin Green <greenjustin@chromium.org>
---
 .../platform/mediatek/vcodec/mtk_vcodec_dec.c | 38 ++++++++++---------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index 52e5d36aa912..f093aa715e1f 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
vb2_queue *src_vq,

  mtk_v4l2_debug(3, "[%d]", ctx->id);

- src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
- src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- src_vq->drv_priv = ctx;
- src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- src_vq->mem_ops = &vb2_dma_contig_memops;
- src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- src_vq->lock = &ctx->dev->dev_mutex;
- src_vq->dev             = &ctx->dev->plat_dev->dev;
+ src_vq->type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+ src_vq->io_modes   = VB2_DMABUF | VB2_MMAP;
+ src_vq->drv_priv   = ctx;
+ src_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
+ src_vq->ops   = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ src_vq->mem_ops   = &vb2_dma_contig_memops;
+ src_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ src_vq->lock   = &ctx->dev->dev_mutex;
+ src_vq->dev               = &ctx->dev->plat_dev->dev;
+ src_vq->allow_cache_hints = 1;

  ret = vb2_queue_init(src_vq);
  if (ret) {
  mtk_v4l2_err("Failed to initialize videobuf2 queue(output)");
  return ret;
  }
- dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- dst_vq->drv_priv = ctx;
- dst_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- dst_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- dst_vq->mem_ops = &vb2_dma_contig_memops;
- dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- dst_vq->lock = &ctx->dev->dev_mutex;
- dst_vq->dev             = &ctx->dev->plat_dev->dev;
+ dst_vq->type   = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ dst_vq->io_modes   = VB2_DMABUF | VB2_MMAP;
+ dst_vq->drv_priv   = ctx;
+ dst_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
+ dst_vq->ops   = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ dst_vq->mem_ops   = &vb2_dma_contig_memops;
+ dst_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ dst_vq->lock   = &ctx->dev->dev_mutex;
+ dst_vq->dev               = &ctx->dev->plat_dev->dev;
+ dst_vq->allow_cache_hints = 1;

  ret = vb2_queue_init(dst_vq);
  if (ret)
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Sergey Senozhatsky]

On (22/05/31 17:10), Justin Green wrote:
> Set allow_cache_hints to 1 for the vb2_queue source and destination queues
> in the mediatek vcodec V4L2 driver. This allows us to allocate buffers
> with the V4L2_MEMORY_FLAG_NON_COHERENT set. On Mediatek SoCs, this enables
> caching for this memory, which vastly improves performance when being read
> from CPU. Read performance for these buffers is in turn important for
> detiling MM21 video frames in software.
> 
> This change should be safe from race conditions since videobuf2 already
> invalidates or flushes the appropriate cache lines in its prepare() and
> finish() methods.
> 
> Tested on a MT8183 SoC. Resulted in both correct detiling and a 10X
> speedup.

Hi Justin,

It seems that something has happened to tabs and code formatting,
could you double check and resend?

> @@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
> vb2_queue *src_vq,
> 
>   mtk_v4l2_debug(3, "[%d]", ctx->id);
> 
> - src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> - src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> - src_vq->drv_priv = ctx;
> - src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
> - src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
> - src_vq->mem_ops = &vb2_dma_contig_memops;
> - src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> - src_vq->lock = &ctx->dev->dev_mutex;
> - src_vq->dev             = &ctx->dev->plat_dev->dev;
> + src_vq->type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + src_vq->io_modes   = VB2_DMABUF | VB2_MMAP;
> + src_vq->drv_priv   = ctx;
> + src_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
> + src_vq->ops   = ctx->dev->vdec_pdata->vdec_vb2_ops;
> + src_vq->mem_ops   = &vb2_dma_contig_memops;
> + src_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + src_vq->lock   = &ctx->dev->dev_mutex;
> + src_vq->dev               = &ctx->dev->plat_dev->dev;
> + src_vq->allow_cache_hints = 1;

I guess it should look something like this

-	src_vq->type		= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
-	src_vq->io_modes	= VB2_DMABUF | VB2_MMAP;
-	src_vq->drv_priv	= ctx;
-	src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
-	src_vq->ops		= ctx->dev->vdec_pdata->vdec_vb2_ops;
-	src_vq->mem_ops		= &vb2_dma_contig_memops;
-	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
-	src_vq->lock		= &ctx->dev->dev_mutex;
-	src_vq->dev             = &ctx->dev->plat_dev->dev;
+	src_vq->type			= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+	src_vq->io_modes		= VB2_DMABUF | VB2_MMAP;
+	src_vq->drv_priv		= ctx;
+	src_vq->buf_struct_size		= sizeof(struct mtk_video_dec_buf);
+	src_vq->ops			= ctx->dev->vdec_pdata->vdec_vb2_ops;
+	src_vq->mem_ops			= &vb2_dma_contig_memops;
+	src_vq->timestamp_flags		= V4L2_BUF_FLAG_TIMESTAMP_COPY;
+	src_vq->lock			= &ctx->dev->dev_mutex;
+	src_vq->dev			= &ctx->dev->plat_dev->dev;
+	src_vq->allow_cache_hints	= 1;

Re: [PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Justin Green]

Sure thing! Sorry about that, I think something got messed up with the
tabs. I've switched the "=" padding to spaces to spacing to make sure
everything is consistent. I think the removals part of the diff might
still look odd on some clients because of the tabs though.


diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index 52e5d36aa912..6a47b34c5bc9 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
vb2_queue *src_vq,

  mtk_v4l2_debug(3, "[%d]", ctx->id);

- src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
- src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- src_vq->drv_priv = ctx;
- src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- src_vq->mem_ops = &vb2_dma_contig_memops;
- src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- src_vq->lock = &ctx->dev->dev_mutex;
- src_vq->dev             = &ctx->dev->plat_dev->dev;
+ src_vq->type              = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+ src_vq->io_modes          = VB2_DMABUF | VB2_MMAP;
+ src_vq->drv_priv          = ctx;
+ src_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
+ src_vq->ops               = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ src_vq->mem_ops           = &vb2_dma_contig_memops;
+ src_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ src_vq->lock              = &ctx->dev->dev_mutex;
+ src_vq->dev               = &ctx->dev->plat_dev->dev;
+ src_vq->allow_cache_hints = 1;

  ret = vb2_queue_init(src_vq);
  if (ret) {
  mtk_v4l2_err("Failed to initialize videobuf2 queue(output)");
  return ret;
  }
- dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- dst_vq->drv_priv = ctx;
- dst_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- dst_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- dst_vq->mem_ops = &vb2_dma_contig_memops;
- dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- dst_vq->lock = &ctx->dev->dev_mutex;
- dst_vq->dev             = &ctx->dev->plat_dev->dev;
+ dst_vq->type              = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ dst_vq->io_modes          = VB2_DMABUF | VB2_MMAP;
+ dst_vq->drv_priv          = ctx;
+ dst_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
+ dst_vq->ops               = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ dst_vq->mem_ops           = &vb2_dma_contig_memops;
+ dst_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ dst_vq->lock              = &ctx->dev->dev_mutex;
+ dst_vq->dev               = &ctx->dev->plat_dev->dev;
+ dst_vq->allow_cache_hints = 1;

  ret = vb2_queue_init(dst_vq);
  if (ret)
-- 
2.36.1.255.ge46751e96f-goog

On Tue, May 31, 2022 at 9:38 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (22/05/31 17:10), Justin Green wrote:
> > Set allow_cache_hints to 1 for the vb2_queue source and destination queues
> > in the mediatek vcodec V4L2 driver. This allows us to allocate buffers
> > with the V4L2_MEMORY_FLAG_NON_COHERENT set. On Mediatek SoCs, this enables
> > caching for this memory, which vastly improves performance when being read
> > from CPU. Read performance for these buffers is in turn important for
> > detiling MM21 video frames in software.
> >
> > This change should be safe from race conditions since videobuf2 already
> > invalidates or flushes the appropriate cache lines in its prepare() and
> > finish() methods.
> >
> > Tested on a MT8183 SoC. Resulted in both correct detiling and a 10X
> > speedup.
>
> Hi Justin,
>
> It seems that something has happened to tabs and code formatting,
> could you double check and resend?
>
> > @@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
> > vb2_queue *src_vq,
> >
> >   mtk_v4l2_debug(3, "[%d]", ctx->id);
> >
> > - src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > - src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > - src_vq->drv_priv = ctx;
> > - src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
> > - src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
> > - src_vq->mem_ops = &vb2_dma_contig_memops;
> > - src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > - src_vq->lock = &ctx->dev->dev_mutex;
> > - src_vq->dev             = &ctx->dev->plat_dev->dev;
> > + src_vq->type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes   = VB2_DMABUF | VB2_MMAP;
> > + src_vq->drv_priv   = ctx;
> > + src_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
> > + src_vq->ops   = ctx->dev->vdec_pdata->vdec_vb2_ops;
> > + src_vq->mem_ops   = &vb2_dma_contig_memops;
> > + src_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock   = &ctx->dev->dev_mutex;
> > + src_vq->dev               = &ctx->dev->plat_dev->dev;
> > + src_vq->allow_cache_hints = 1;
>
> I guess it should look something like this
>
> -       src_vq->type            = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> -       src_vq->io_modes        = VB2_DMABUF | VB2_MMAP;
> -       src_vq->drv_priv        = ctx;
> -       src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
> -       src_vq->ops             = ctx->dev->vdec_pdata->vdec_vb2_ops;
> -       src_vq->mem_ops         = &vb2_dma_contig_memops;
> -       src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> -       src_vq->lock            = &ctx->dev->dev_mutex;
> -       src_vq->dev             = &ctx->dev->plat_dev->dev;
> +       src_vq->type                    = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +       src_vq->io_modes                = VB2_DMABUF | VB2_MMAP;
> +       src_vq->drv_priv                = ctx;
> +       src_vq->buf_struct_size         = sizeof(struct mtk_video_dec_buf);
> +       src_vq->ops                     = ctx->dev->vdec_pdata->vdec_vb2_ops;
> +       src_vq->mem_ops                 = &vb2_dma_contig_memops;
> +       src_vq->timestamp_flags         = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +       src_vq->lock                    = &ctx->dev->dev_mutex;
> +       src_vq->dev                     = &ctx->dev->plat_dev->dev;
> +       src_vq->allow_cache_hints       = 1;

Re: [PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Nicolas Dufresne]

Hi Justin,

Le mercredi 01 juin 2022 à 10:00 -0400, Justin Green a écrit :
> Sure thing! Sorry about that, I think something got messed up with the
> tabs. I've switched the "=" padding to spaces to spacing to make sure
> everything is consistent. I think the removals part of the diff might
> still look odd on some clients because of the tabs though.

Best practice to to not mix style and functional changes, unless trivial. So if
you want to change the style, add a second patch. Otherwise just maintain the
original style (my recommendation). By the way you can add:

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

I'd be very interested to learn from Sergey on why this feature wasn't enable
more broadly. I notice though the begin/end access bits have not been
implemented, so when used with DMABuf, this isn't going to behave quite right by
default. I also notice that the code make no use of the attached device
dma_coherent flag, so another case were this feature would get some help before
being generalized.

> 
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> index 52e5d36aa912..6a47b34c5bc9 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> @@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
> vb2_queue *src_vq,
> 
>   mtk_v4l2_debug(3, "[%d]", ctx->id);
> 
> - src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> - src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> - src_vq->drv_priv = ctx;
> - src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
> - src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
> - src_vq->mem_ops = &vb2_dma_contig_memops;
> - src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> - src_vq->lock = &ctx->dev->dev_mutex;
> - src_vq->dev             = &ctx->dev->plat_dev->dev;
> + src_vq->type              = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + src_vq->io_modes          = VB2_DMABUF | VB2_MMAP;
> + src_vq->drv_priv          = ctx;
> + src_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
> + src_vq->ops               = ctx->dev->vdec_pdata->vdec_vb2_ops;
> + src_vq->mem_ops           = &vb2_dma_contig_memops;
> + src_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + src_vq->lock              = &ctx->dev->dev_mutex;
> + src_vq->dev               = &ctx->dev->plat_dev->dev;
> + src_vq->allow_cache_hints = 1;

As a side effect, you'll only have this line added, no noise around it.

> 
>   ret = vb2_queue_init(src_vq);
>   if (ret) {
>   mtk_v4l2_err("Failed to initialize videobuf2 queue(output)");
>   return ret;
>   }
> - dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> - dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> - dst_vq->drv_priv = ctx;
> - dst_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
> - dst_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
> - dst_vq->mem_ops = &vb2_dma_contig_memops;
> - dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> - dst_vq->lock = &ctx->dev->dev_mutex;
> - dst_vq->dev             = &ctx->dev->plat_dev->dev;
> + dst_vq->type              = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + dst_vq->io_modes          = VB2_DMABUF | VB2_MMAP;
> + dst_vq->drv_priv          = ctx;
> + dst_vq->buf_struct_size   = sizeof(struct mtk_video_dec_buf);
> + dst_vq->ops               = ctx->dev->vdec_pdata->vdec_vb2_ops;
> + dst_vq->mem_ops           = &vb2_dma_contig_memops;
> + dst_vq->timestamp_flags   = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + dst_vq->lock              = &ctx->dev->dev_mutex;
> + dst_vq->dev               = &ctx->dev->plat_dev->dev;
> + dst_vq->allow_cache_hints = 1;
> 
>   ret = vb2_queue_init(dst_vq);
>   if (ret)

Re: [PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Sergey Senozhatsky]

On (22/06/01 11:13), Nicolas Dufresne wrote:
> I'd be very interested to learn from Sergey on why this feature wasn't enable
> more broadly.

Well, we needed this for one particular device and we didn't plan on
expanding this to drivers that we cannot test, verify, etc. and for
the hardware that we don't run.

> I notice though the begin/end access bits have not been
> implemented, so when used with DMABuf, this isn't going to behave quite right by
> default.

We cared only for MMAP buffers. Got time to cook patches?

Re: [PATCH] mediatek/vcodec: Enable incoherent buffer allocation

[Nicolas Dufresne]

Le mardi 07 juin 2022 à 18:57 +0900, Sergey Senozhatsky a écrit :
> On (22/06/01 11:13), Nicolas Dufresne wrote:
> > I'd be very interested to learn from Sergey on why this feature wasn't enable
> > more broadly.
> 
> Well, we needed this for one particular device and we didn't plan on
> expanding this to drivers that we cannot test, verify, etc. and for
> the hardware that we don't run.

Fair enough, should have been better documented, as folks may expect more then a
single driver supporting it. I'll see if I can improve this. Justin's use case
it to improve performance of software conversion of HW decoded frames. So he's
only reading the buffer, so dmabuf/mmap is not as relevant here.

> 
> > I notice though the begin/end access bits have not been
> > implemented, so when used with DMABuf, this isn't going to behave quite right by
> > default.
> 
> We cared only for MMAP buffers. Got time to cook patches?

Eventually, my ultimate goal is try and complete the DMABuf support in V4L2,
this code you added in the contiguous allocator also exist in the SG and vmalloc
one in some form (and is pretty generic), and both are also missing the
begin/end ops for DMABuf (only works for MMAP), including tracking the attached
device coherency. If that code was complete, I think we could eventually make
this feature available by default (but not turned on, as I suspect flush
overhead will regress some use cases).

The memory synchronization support we have now is all for MMAP, and is in
general very inefficient for DMABuf were we know more about the importer. Most
devs seems to have avoided this problem. I felt like your hint work was part of
it, but of course, if you were not using DMABuf, those hints are needed in
replacement to DMABuf Sync API (which can be used for the same purpose). Perhaps
not great that we duplicate modern API into legacy kind of APIs, but as long as
V4L2 supports mmap, there is little choice but to do that.

Nicolas