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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
mtk_v4l2_err() already uses dev_err(), so don't print the device name
again. Print function name instead.
Signed-off-by: Fei Shao <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
On Thu, Dec 21, 2023 at 5:23 PM Fei Shao <[email protected]> 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/[email protected]/
>
> 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
>
Hey Fei,
On 27.03.2024 17:18, Fei Shao wrote:
>On Thu, Dec 21, 2023 at 5:23 PM Fei Shao <[email protected]> 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/[email protected]/
>>
>> 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
>>
>