| Message ID | 20210920170408.1561-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] media: mtk-vpu: Ensure alignment of 8 for DTCM buffer | expand |
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? > > 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)]; 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? Regards, Hans > + > + memset(data + len, 0, sizeof(data) - len); > + memcpy(data, buf, len); > + memcpy_toio(send_obj->share_buf, data, round_up(len, 8)); > + } else { > + memcpy_toio(send_obj->share_buf, buf, len); > + } > + > writel(len, &send_obj->len); > writel(id, &send_obj->id); > >
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. > > 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.
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. >
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. > >
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? >> [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. > > > > >
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 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. >>>> >> >>
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. >>>>> >>> >>>
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. 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. > > > > > > > > > > > > > >
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. >>>>>>> >>>>> >>>>>
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. >
On Mon, Nov 29, 2021 at 11:39 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > > > 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); > } Not sure if avoiding that stack allocation is worth the extra complexity and requirement for len being a multiple of 4. Also I'd like to test it on real hardware to confirm it is indeed ok.
Il 20/09/21 19:04, Dafna Hirschfeld ha scritto: > 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 > > 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> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> I acknowledge that this patch is getting very old and stale, but after some testing on MT8173, we were able to verify that it is essential for MediaTek MDP functionality, along with some other patches. Without this one, it won't work. Keeping this in mind, I would propose to get this one picked before it gets completely stale and requiring a rewrite. Cheers, Angelo
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 + */ + if (len % 8 != 0) { + unsigned char data[SHARE_BUF_SIZE]; + + memset(data + len, 0, sizeof(data) - len); + memcpy(data, buf, len); + memcpy_toio(send_obj->share_buf, data, round_up(len, 8)); + } else { + memcpy_toio(send_obj->share_buf, buf, len); + } + writel(len, &send_obj->len); writel(id, &send_obj->id);