Messages in this thread | | | Subject | Re: [PATCH v4] media: mtk-vpu: Ensure alignment of 8 for DTCM buffer | From | Dafna Hirschfeld <> | Date | Tue, 9 Nov 2021 10:46:50 +0200 |
| |
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
> 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. >>>>> >>> >>>
| |