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 10:11:37 +0200 |
| |
On 10.11.21 05:11, Irui Wang wrote: > Hi, Dafna, > > Thanks for the patch. > On Tue, 2021-11-09 at 10:46 +0200, Dafna Hirschfeld wrote: >> >> On 03.11.21 13:04, Dafna Hirschfeld wrote: >>> >>> >>> On 03.11.21 10:19, Irui Wang wrote: >>>> Hi, >>>> >>>> The "len" of share_buf copied should be always 8 alignment; >>>> do you have other logs to prove the len is not 8 alignment when >>>> errors >>>> appear? >>> >>> Hi, I found out that "sizeof(mdp_ipi_comm) = 20" >>> this is due to the macro #pragma pack(push, 4) in mtk_mdp_ipi.h >>> >>> see [1] >>> >>> [1] http://lkml.iu.edu/hypermail/linux/kernel/2109.2/04978.html >>> >> >> Hi Irui Wang, >> Any update regarding that patch? >> Can you give more explanation for that errors that we see >> when the buffer size is not a multiple of 8? >> >> Thanks, >> Dafna > > share_buf is a mapped memory by ioremap, it should be better use > memcpy_to/fromio instead of memcpy because of alignment. > > As for memcpy_toio, it may also have requirements for alignment, we can > also get such information from: > > https://elixir.bootlin.com/linux/v5.15/source/drivers/mtd/spi-nor/controllers/aspeed-smc.c#L207 > . > > So, it's not VPU HW bug or limitation, it's memcpy_toio requirements, > maybe we can modify IPI message to do alignment, but it need modify > both kernel and vpu firmware, which will break upstream backward > compatible, we think it's unacceptale. > > If this patch can solve the issue, we think it's OK.
In such case shouldn't we also make sure that the address of send_obj->share_buf is 8 aligend before calling memcpy_toio, and also shouldn't we make the same checks for the memcpy_fromio ?
Thanks, Dafna
> > Thanks >> >>> Thanks, >>> Dafna >>> >>>>>> [58.350841] mtk-mdp 14001000.rdma: processing failed: -22 >>>> >>>> On Wed, 2021-11-03 at 16:03 +0800, houlong wei wrote: >>>>> Add mtk-vpu driver expert irui.wang in the loop. >>>>> >>>>> On Mon, 2021-10-18 at 15:07 +0800, Dafna Hirschfeld wrote: >>>>>> >>>>>> On 18.10.21 03: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. >>>>>> >>>>>> Is writing 'due to hardware or firmware limitation' enough? >>>>>> If not, then we should wait for mediatek people's response to >>>>>> explain >>>>>> if they know more >>>>>> >>>>>>>> >>>>>>>>> + */ >>>>>>>>> + 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. >>>>>> >>>>>> I'll send v5 fixing this >>>>>> >>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>> >>>>>> I looked further and noted that the structs that are larger >>>>>> than >>>>>> 'SHARE_BUF_SIZE' >>>>>> (venc_ap_ipi_msg_enc_ext venc_ap_ipi_msg_set_param_ext) >>>>>> are used by drivers that don't use this vpu api, so actually >>>>>> SHARE_BUF_SIZE is >>>>>> not too low and as Corurbot worte probably not changeable. >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Dafna >>>>>> >>>>>>> Cheers, >>>>>>> Alex. >>>>>>> >>>>> >>>>>
| |