Messages in this thread | | | Subject | Re: [PATCH v4] media: mtk-vpu: Ensure alignment of 8 for DTCM buffer | From | Dafna Hirschfeld <> | Date | Mon, 29 Nov 2021 16:39:07 +0200 |
| |
On 18.10.21 04:16, Alexandre Courbot wrote: > Hi Hans! > > On Mon, Oct 4, 2021 at 6:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 20/09/2021 19:04, Dafna Hirschfeld wrote: >>> From: Alexandre Courbot <acourbot@chromium.org> >>> >>> When running memcpy_toio: >>> memcpy_toio(send_obj->share_buf, buf, len); >>> it was found that errors appear if len is not a multiple of 8: >>> >>> [58.350841] mtk-mdp 14001000.rdma: processing failed: -22 >> >> Why do errors appear? Is that due to a HW bug? Some other reason? > > MTK folks would be the best placed to answer this, but since the > failure is reported by the firmware I'd suspect either a firmware or > hardware limitation. > >> >>> >>> This patch ensures the copy of a multiple of 8 size by calling >>> round_up(len, 8) when copying >>> >>> Fixes: e6599adfad30 ("media: mtk-vpu: avoid unaligned access to DTCM buffer.") >>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>> Reviewed-by: Houlong Wei <houlong.wei@mediatek.com> >>> --- >>> changes since v3: >>> 1. multile -> multiple >>> 2. add inline doc >>> >>> changes since v2: >>> 1. do the extra copy only if len is not multiple of 8 >>> >>> changes since v1: >>> 1. change sign-off-by tags >>> 2. change values to memset >>> >>> drivers/media/platform/mtk-vpu/mtk_vpu.c | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c >>> index ec290dde59cf..1df031716c8f 100644 >>> --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c >>> +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c >>> @@ -349,7 +349,20 @@ int vpu_ipi_send(struct platform_device *pdev, >>> } >>> } while (vpu_cfg_readl(vpu, HOST_TO_VPU)); >>> >>> - memcpy_toio(send_obj->share_buf, buf, len); >>> + /* >>> + * when copying data to the vpu hardware, the memcpy_toio operation must copy >>> + * a multiple of 8. Otherwise the processing fails >> >> Same here: it needs to explain why the processing fails. >> >>> + */ >>> + if (len % 8 != 0) { >>> + unsigned char data[SHARE_BUF_SIZE]; >> >> Wouldn't it be more robust if you say: >> >> unsigned char data[sizeof(send_obj->share_buf)]; > > Definitely yes.
won't it actually be better to implement it like this: (assuming len is always multiply of 4 - which I think it must be since access must be 4 aligned)
void __iomem *to = obj->share_buf;
if (len % 8 != 0) { memcpy_toio(to, buf, len - 4); to += len - 4; buf += len - 4; writel_relaxed(*(u32 *)buf, to); } else { memcpy_toio(obj->share_buf, buf, len); } Thanks, Dafna > >> >> I also think that the SHARE_BUF_SIZE define needs a comment stating that it must be a >> multiple of 8, otherwise unexpected things can happen. >> >> You also noticed that the current SHARE_BUF_SIZE define is too low, but I saw >> no patch correcting this. Shouldn't that be fixed as well? > > AFAICT the firmware expects this exact size on its end, so I don't > believe it can be changed that easily. But maybe someone from MTK can > prove me wrong. > > Cheers, > Alex. >
| |