[PATCH v2 0/4] Improvement around mtk_vcodec_mem_free() logging and usage

[PATCH v2 0/4] Improvement around mtk_vcodec_mem_free() logging and usage

[Fei Shao]

Hi,

This series includes some improvements around mtk_vcodec_mem_free() in
mtk_vcodec_util.c.

I noticed that mtk_vcodec_mem_free() generates a spurious error if the
target DMA buffer has been freed previously:
  mtk_vcodec_mem_free(),69: [MTK_V4L2][ERROR] 18000000.video-codec
  dma_free size=0 failed!

The warning is harmless, but it brings some confusion to our developers
and testing infra, so I sent this series to fix that with some minor
improvement for the driver alongside.

The first two patches are for aesthetic and style improvements, the
third tries to make the error logs more informative, and the last adds
back the missing checks when calling the free function.

Regards,
Fei

v1: https://lore.kernel.org/all/20231113123049.4117280-1-fshao@chromium.org/

Changes in v2:
- rebased on top of next-20231219
- revise commit message for clearer intention and rationale
- update the error messages based on the suggestion

Fei Shao (4):
  media: mediatek: vcodec: Replace dev_name in error string
  media: mediatek: vcodec: Drop unnecessary variable
  media: mediatek: vcodec: Update mtk_vcodec_mem_free() error messages
  media: mediatek: vcodec: Only free buffer VA that is not NULL

 .../mediatek/vcodec/common/mtk_vcodec_util.c  | 23 +++++++++----------
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 22 ++++++++++++------
 .../vcodec/encoder/venc/venc_h264_if.c        |  5 ++--
 3 files changed, 29 insertions(+), 21 deletions(-)

-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 1/4] media: mediatek: vcodec: Replace dev_name in error string

[Fei Shao]

mtk_v4l2_err() already uses dev_err(), so don't print the device name
again. Print function name instead.

Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---

Changes in v2:
- rebased on top of next-20231219

 .../media/platform/mediatek/vcodec/common/mtk_vcodec_util.c   | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
index 9ce34a3b5ee6..ea8c35c0e667 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
@@ -67,7 +67,7 @@ int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
 	mem->va = dma_alloc_coherent(&plat_dev->dev, size, &mem->dma_addr, GFP_KERNEL);
 	if (!mem->va) {
 		mtk_v4l2_err(plat_dev, "%s dma_alloc size=%ld failed!",
-			     dev_name(&plat_dev->dev), size);
+			     __func__, size);
 		return -ENOMEM;
 	}
 
@@ -99,7 +99,7 @@ void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
 
 	if (!mem->va) {
 		mtk_v4l2_err(plat_dev, "%s dma_free size=%ld failed!",
-			     dev_name(&plat_dev->dev), size);
+			     __func__, size);
 		return;
 	}
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 2/4] media: mediatek: vcodec: Drop unnecessary variable

[Fei Shao]

In mtk_vcodec_mem_alloc() and mtk_vcodec_mem_free(), the value of
mem->size is not expected to change before and when using the DMA APIs
and debug print, so there's no point in keeping local copies of it.

Drop the local variable "size" in the mentioned functions, and update
printk format identifiers accordingly.

This makes the code slightly more visually consistent, and retrieve a
small amount of memory that is used for no real purpose.

Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---

Changes in v2:
- revise commit message for clearer intention and rationale

 .../mediatek/vcodec/common/mtk_vcodec_util.c  | 22 +++++++++----------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
index ea8c35c0e667..23bea2702c9a 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
@@ -49,7 +49,6 @@ int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
 {
 	enum mtk_instance_type inst_type = *((unsigned int *)priv);
 	struct platform_device *plat_dev;
-	unsigned long size = mem->size;
 	int id;
 
 	if (inst_type == MTK_INST_ENCODER) {
@@ -64,15 +63,15 @@ int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
 		id = dec_ctx->id;
 	}
 
-	mem->va = dma_alloc_coherent(&plat_dev->dev, size, &mem->dma_addr, GFP_KERNEL);
+	mem->va = dma_alloc_coherent(&plat_dev->dev, mem->size, &mem->dma_addr, GFP_KERNEL);
 	if (!mem->va) {
-		mtk_v4l2_err(plat_dev, "%s dma_alloc size=%ld failed!",
-			     __func__, size);
+		mtk_v4l2_err(plat_dev, "%s dma_alloc size=0x%zx failed!",
+			     __func__, mem->size);
 		return -ENOMEM;
 	}
 
-	mtk_v4l2_debug(plat_dev, 3, "[%d] - va = %p dma = 0x%lx size = 0x%lx", id, mem->va,
-		       (unsigned long)mem->dma_addr, size);
+	mtk_v4l2_debug(plat_dev, 3, "[%d] - va = %p dma = 0x%lx size = 0x%zx", id, mem->va,
+		       (unsigned long)mem->dma_addr, mem->size);
 
 	return 0;
 }
@@ -82,7 +81,6 @@ void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
 {
 	enum mtk_instance_type inst_type = *((unsigned int *)priv);
 	struct platform_device *plat_dev;
-	unsigned long size = mem->size;
 	int id;
 
 	if (inst_type == MTK_INST_ENCODER) {
@@ -98,15 +96,15 @@ void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
 	}
 
 	if (!mem->va) {
-		mtk_v4l2_err(plat_dev, "%s dma_free size=%ld failed!",
-			     __func__, size);
+		mtk_v4l2_err(plat_dev, "%s dma_free size=0x%zx failed!",
+			     __func__, mem->size);
 		return;
 	}
 
-	mtk_v4l2_debug(plat_dev, 3, "[%d] - va = %p dma = 0x%lx size = 0x%lx", id, mem->va,
-		       (unsigned long)mem->dma_addr, size);
+	mtk_v4l2_debug(plat_dev, 3, "[%d] - va = %p dma = 0x%lx size = 0x%zx", id, mem->va,
+		       (unsigned long)mem->dma_addr, mem->size);
 
-	dma_free_coherent(&plat_dev->dev, size, mem->va, mem->dma_addr);
+	dma_free_coherent(&plat_dev->dev, mem->size, mem->va, mem->dma_addr);
 	mem->va = NULL;
 	mem->dma_addr = 0;
 	mem->size = 0;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 3/4] media: mediatek: vcodec: Update mtk_vcodec_mem_free() error messages

[Fei Shao]

In mtk_vcodec_mem_free(), there are two cases where a NULL VA is passed:
- mem->size == 0: we are called to free no memory. This may happen when
  we call mtk_vcodec_mem_free() twice or the memory has never been
  allocated.
- mem->size > 0: we are called to free memory but without VA. This means
  that we failed to free the memory for real.

Both cases are not expected to happen, and we want to have clearer error
messages to describe which one we just encountered.
Update the error messages to include more information for that purpose.

Signed-off-by: Fei Shao <fshao@chromium.org>
---

Changes in v2:
- update the error messages based on the suggestion

 .../media/platform/mediatek/vcodec/common/mtk_vcodec_util.c  | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
index 23bea2702c9a..c60e4c193b25 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
@@ -96,8 +96,9 @@ void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
 	}
 
 	if (!mem->va) {
-		mtk_v4l2_err(plat_dev, "%s dma_free size=0x%zx failed!",
-			     __func__, mem->size);
+		mtk_v4l2_err(plat_dev, "%s: Tried to free a NULL VA", __func__);
+		if (mem->size)
+			mtk_v4l2_err(plat_dev, "Failed to free %zu bytes", mem->size);
 		return;
 	}
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 4/4] media: mediatek: vcodec: Only free buffer VA that is not NULL

[Fei Shao]

In the MediaTek vcodec driver, while mtk_vcodec_mem_free() is mostly
called only when the buffer to free exists, there are some instances
that didn't do the check and triggered warnings in practice.

We believe those checks were forgotten unintentionally. Add the checks
back to fix the warnings.

Signed-off-by: Fei Shao <fshao@chromium.org>
---
I had discussion with Yunfei, and he also thinks that the developers
just forgot to wrap mtk_vcodec_mem_free() with the checks back then.
We believe it's safe to add those checks.


 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 22 +++++++++++++------
 .../vcodec/encoder/venc/venc_h264_if.c        |  5 +++--
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
index 2b6a5adbc419..b0e2e59f61b5 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
@@ -1023,18 +1023,26 @@ static void vdec_av1_slice_free_working_buffer(struct vdec_av1_slice_instance *i
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(instance->mv); i++)
-		mtk_vcodec_mem_free(ctx, &instance->mv[i]);
+		if (instance->mv[i].va)
+			mtk_vcodec_mem_free(ctx, &instance->mv[i]);
 
 	for (i = 0; i < ARRAY_SIZE(instance->seg); i++)
-		mtk_vcodec_mem_free(ctx, &instance->seg[i]);
+		if (instance->seg[i].va)
+			mtk_vcodec_mem_free(ctx, &instance->seg[i]);
 
 	for (i = 0; i < ARRAY_SIZE(instance->cdf); i++)
-		mtk_vcodec_mem_free(ctx, &instance->cdf[i]);
+		if (instance->cdf[i].va)
+			mtk_vcodec_mem_free(ctx, &instance->cdf[i]);
+
 
-	mtk_vcodec_mem_free(ctx, &instance->tile);
-	mtk_vcodec_mem_free(ctx, &instance->cdf_temp);
-	mtk_vcodec_mem_free(ctx, &instance->cdf_table);
-	mtk_vcodec_mem_free(ctx, &instance->iq_table);
+	if (instance->tile.va)
+		mtk_vcodec_mem_free(ctx, &instance->tile);
+	if (instance->cdf_temp.va)
+		mtk_vcodec_mem_free(ctx, &instance->cdf_temp);
+	if (instance->cdf_table.va)
+		mtk_vcodec_mem_free(ctx, &instance->cdf_table);
+	if (instance->iq_table.va)
+		mtk_vcodec_mem_free(ctx, &instance->iq_table);
 
 	instance->level = AV1_RES_NONE;
 }
diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
index a68dac72c4e4..f8145998fcaf 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
@@ -301,11 +301,12 @@ static void h264_enc_free_work_buf(struct venc_h264_inst *inst)
 	 * other buffers need to be freed by AP.
 	 */
 	for (i = 0; i < VENC_H264_VPU_WORK_BUF_MAX; i++) {
-		if (i != VENC_H264_VPU_WORK_BUF_SKIP_FRAME)
+		if (i != VENC_H264_VPU_WORK_BUF_SKIP_FRAME && inst->work_bufs[i].va)
 			mtk_vcodec_mem_free(inst->ctx, &inst->work_bufs[i]);
 	}
 
-	mtk_vcodec_mem_free(inst->ctx, &inst->pps_buf);
+	if (inst->pps_buf.va)
+		mtk_vcodec_mem_free(inst->ctx, &inst->pps_buf);
 }
 
 static int h264_enc_alloc_work_buf(struct venc_h264_inst *inst, bool is_34bit)
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH v2 3/4] media: mediatek: vcodec: Update mtk_vcodec_mem_free() error messages

[AngeloGioacchino Del Regno]

Il 21/12/23 10:17, Fei Shao ha scritto:
> In mtk_vcodec_mem_free(), there are two cases where a NULL VA is passed:
> - mem->size == 0: we are called to free no memory. This may happen when
>    we call mtk_vcodec_mem_free() twice or the memory has never been
>    allocated.
> - mem->size > 0: we are called to free memory but without VA. This means
>    that we failed to free the memory for real.
> 
> Both cases are not expected to happen, and we want to have clearer error
> messages to describe which one we just encountered.
> Update the error messages to include more information for that purpose.
> 
> Signed-off-by: Fei Shao <fshao@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 4/4] media: mediatek: vcodec: Only free buffer VA that is not NULL

[AngeloGioacchino Del Regno]

Il 21/12/23 10:17, Fei Shao ha scritto:
> In the MediaTek vcodec driver, while mtk_vcodec_mem_free() is mostly
> called only when the buffer to free exists, there are some instances
> that didn't do the check and triggered warnings in practice.
> 
> We believe those checks were forgotten unintentionally. Add the checks
> back to fix the warnings.
> 
> Signed-off-by: Fei Shao <fshao@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 0/4] Improvement around mtk_vcodec_mem_free() logging and usage

[Fei Shao]

On Thu, Dec 21, 2023 at 5:23 PM Fei Shao <fshao@chromium.org> wrote:
>
> Hi,
>
> This series includes some improvements around mtk_vcodec_mem_free() in
> mtk_vcodec_util.c.
>
> I noticed that mtk_vcodec_mem_free() generates a spurious error if the
> target DMA buffer has been freed previously:
>   mtk_vcodec_mem_free(),69: [MTK_V4L2][ERROR] 18000000.video-codec
>   dma_free size=0 failed!
>
> The warning is harmless, but it brings some confusion to our developers
> and testing infra, so I sent this series to fix that with some minor
> improvement for the driver alongside.
>
> The first two patches are for aesthetic and style improvements, the
> third tries to make the error logs more informative, and the last adds
> back the missing checks when calling the free function.
>
> Regards,
> Fei
>
> v1: https://lore.kernel.org/all/20231113123049.4117280-1-fshao@chromium.org/
>
> Changes in v2:
> - rebased on top of next-20231219
> - revise commit message for clearer intention and rationale
> - update the error messages based on the suggestion
>
> Fei Shao (4):
>   media: mediatek: vcodec: Replace dev_name in error string
>   media: mediatek: vcodec: Drop unnecessary variable
>   media: mediatek: vcodec: Update mtk_vcodec_mem_free() error messages
>   media: mediatek: vcodec: Only free buffer VA that is not NULL
>
>  .../mediatek/vcodec/common/mtk_vcodec_util.c  | 23 +++++++++----------
>  .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 22 ++++++++++++------
>  .../vcodec/encoder/venc/venc_h264_if.c        |  5 ++--
>  3 files changed, 29 insertions(+), 21 deletions(-)

Hi Hans,

I'd like to get some attention for this series in case it has been
overlooked. It still applies to next-20240327 without conflict.
Please let me know if you have any feedback or concerns on this.

Thank you,
Fei
>
> --
> 2.43.0.472.g3155946c3a-goog
>

Re: [PATCH v2 0/4] Improvement around mtk_vcodec_mem_free() logging and usage

[Sebastian Fricke]

Hey Fei,

On 27.03.2024 17:18, Fei Shao wrote:
>On Thu, Dec 21, 2023 at 5:23 PM Fei Shao <fshao@chromium.org> wrote:
>>
>> Hi,
>>
>> This series includes some improvements around mtk_vcodec_mem_free() in
>> mtk_vcodec_util.c.
>>
>> I noticed that mtk_vcodec_mem_free() generates a spurious error if the
>> target DMA buffer has been freed previously:
>>   mtk_vcodec_mem_free(),69: [MTK_V4L2][ERROR] 18000000.video-codec
>>   dma_free size=0 failed!
>>
>> The warning is harmless, but it brings some confusion to our developers
>> and testing infra, so I sent this series to fix that with some minor
>> improvement for the driver alongside.
>>
>> The first two patches are for aesthetic and style improvements, the
>> third tries to make the error logs more informative, and the last adds
>> back the missing checks when calling the free function.
>>
>> Regards,
>> Fei
>>
>> v1: https://lore.kernel.org/all/20231113123049.4117280-1-fshao@chromium.org/
>>
>> Changes in v2:
>> - rebased on top of next-20231219
>> - revise commit message for clearer intention and rationale
>> - update the error messages based on the suggestion
>>
>> Fei Shao (4):
>>   media: mediatek: vcodec: Replace dev_name in error string
>>   media: mediatek: vcodec: Drop unnecessary variable
>>   media: mediatek: vcodec: Update mtk_vcodec_mem_free() error messages
>>   media: mediatek: vcodec: Only free buffer VA that is not NULL
>>
>>  .../mediatek/vcodec/common/mtk_vcodec_util.c  | 23 +++++++++----------
>>  .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 22 ++++++++++++------
>>  .../vcodec/encoder/venc/venc_h264_if.c        |  5 ++--
>>  3 files changed, 29 insertions(+), 21 deletions(-)
>
>Hi Hans,
>
>I'd like to get some attention for this series in case it has been
>overlooked. It still applies to next-20240327 without conflict.
>Please let me know if you have any feedback or concerns on this.

Sorry this seems to have slipped through my fingers, I'll put it on my
list. Thanks for the notification.

>
>Thank you,
>Fei

Greetings,
Sebastian

>>
>> --
>> 2.43.0.472.g3155946c3a-goog
>>
>