Hi Mauro and Hans, On Sun, Nov 10, 2019 at 3:42 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote: > > This is an automatic generated email to let you know that the following patch were queued: > > Subject: media: mtk-vcodec: Remove extra area allocation in an input buffer on encoding > Author: Hirokazu Honda <hiroh@chromium.org> > Date: Sun Nov 10 07:25:34 2019 +0100 I asked for some more testing on different platforms in my review reply and we got some offline signals that it might not work on some platforms, due to some hardware prefetch. (Would be better if that was reported to the mailing list instead.) We're trying to figure out the exact requirements here, because half of the frame doesn't sound like something reasonable. Let's hold off the patch for the time being. Best regards, Tomasz > > MediaTek encoder allocates non pixel data area for an input buffer every > plane. As the input buffer should be read-only, the driver should not write > anything in the buffer. Therefore, the extra data should be unnecessary. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > --- > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > index fd8de027e83e..6aad53d97d74 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > @@ -332,14 +332,12 @@ static int vidioc_try_fmt(struct v4l2_format *f, > > pix_fmt_mp->num_planes = fmt->num_planes; > pix_fmt_mp->plane_fmt[0].sizeimage = > - pix_fmt_mp->width * pix_fmt_mp->height + > - ((ALIGN(pix_fmt_mp->width, 16) * 2) * 16); > + pix_fmt_mp->width * pix_fmt_mp->height; > pix_fmt_mp->plane_fmt[0].bytesperline = pix_fmt_mp->width; > > if (pix_fmt_mp->num_planes == 2) { > pix_fmt_mp->plane_fmt[1].sizeimage = > - (pix_fmt_mp->width * pix_fmt_mp->height) / 2 + > - (ALIGN(pix_fmt_mp->width, 16) * 16); > + (pix_fmt_mp->width * pix_fmt_mp->height) / 2; > pix_fmt_mp->plane_fmt[2].sizeimage = 0; > pix_fmt_mp->plane_fmt[1].bytesperline = > pix_fmt_mp->width; > @@ -347,8 +345,7 @@ static int vidioc_try_fmt(struct v4l2_format *f, > } else if (pix_fmt_mp->num_planes == 3) { > pix_fmt_mp->plane_fmt[1].sizeimage = > pix_fmt_mp->plane_fmt[2].sizeimage = > - (pix_fmt_mp->width * pix_fmt_mp->height) / 4 + > - ((ALIGN(pix_fmt_mp->width, 16) / 2) * 16); > + (pix_fmt_mp->width * pix_fmt_mp->height) / 4; > pix_fmt_mp->plane_fmt[1].bytesperline = > pix_fmt_mp->plane_fmt[2].bytesperline = > pix_fmt_mp->width / 2;
On 11/10/19 2:13 PM, Tomasz Figa wrote: > Hi Mauro and Hans, > > On Sun, Nov 10, 2019 at 3:42 PM Mauro Carvalho Chehab > <mchehab@kernel.org> wrote: >> >> This is an automatic generated email to let you know that the following patch were queued: >> >> Subject: media: mtk-vcodec: Remove extra area allocation in an input buffer on encoding >> Author: Hirokazu Honda <hiroh@chromium.org> >> Date: Sun Nov 10 07:25:34 2019 +0100 > > I asked for some more testing on different platforms in my review > reply and we got some offline signals that it might not work on some > platforms, due to some hardware prefetch. (Would be better if that was > reported to the mailing list instead.) We're trying to figure out the > exact requirements here, because half of the frame doesn't sound like > something reasonable. Let's hold off the patch for the time being. I'll repost my last pull request, including a revert of this patch. The state of this patch in patchwork is changed back to New. I'll wait for your feedback on what to do with this patch. Regards, Hans > > Best regards, > Tomasz > >> >> MediaTek encoder allocates non pixel data area for an input buffer every >> plane. As the input buffer should be read-only, the driver should not write >> anything in the buffer. Therefore, the extra data should be unnecessary. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> >> >> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> --- >> >> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c >> index fd8de027e83e..6aad53d97d74 100644 >> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c >> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c >> @@ -332,14 +332,12 @@ static int vidioc_try_fmt(struct v4l2_format *f, >> >> pix_fmt_mp->num_planes = fmt->num_planes; >> pix_fmt_mp->plane_fmt[0].sizeimage = >> - pix_fmt_mp->width * pix_fmt_mp->height + >> - ((ALIGN(pix_fmt_mp->width, 16) * 2) * 16); >> + pix_fmt_mp->width * pix_fmt_mp->height; >> pix_fmt_mp->plane_fmt[0].bytesperline = pix_fmt_mp->width; >> >> if (pix_fmt_mp->num_planes == 2) { >> pix_fmt_mp->plane_fmt[1].sizeimage = >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 2 + >> - (ALIGN(pix_fmt_mp->width, 16) * 16); >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 2; >> pix_fmt_mp->plane_fmt[2].sizeimage = 0; >> pix_fmt_mp->plane_fmt[1].bytesperline = >> pix_fmt_mp->width; >> @@ -347,8 +345,7 @@ static int vidioc_try_fmt(struct v4l2_format *f, >> } else if (pix_fmt_mp->num_planes == 3) { >> pix_fmt_mp->plane_fmt[1].sizeimage = >> pix_fmt_mp->plane_fmt[2].sizeimage = >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 4 + >> - ((ALIGN(pix_fmt_mp->width, 16) / 2) * 16); >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 4; >> pix_fmt_mp->plane_fmt[1].bytesperline = >> pix_fmt_mp->plane_fmt[2].bytesperline = >> pix_fmt_mp->width / 2;
+Alexandre Courbot
On Sun, Nov 10, 2019 at 10:36 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 11/10/19 2:13 PM, Tomasz Figa wrote:
> > Hi Mauro and Hans,
> >
> > On Sun, Nov 10, 2019 at 3:42 PM Mauro Carvalho Chehab
> > <mchehab@kernel.org> wrote:
> >>
> >> This is an automatic generated email to let you know that the following patch were queued:
> >>
> >> Subject: media: mtk-vcodec: Remove extra area allocation in an input buffer on encoding
> >> Author: Hirokazu Honda <hiroh@chromium.org>
> >> Date: Sun Nov 10 07:25:34 2019 +0100
> >
> > I asked for some more testing on different platforms in my review
> > reply and we got some offline signals that it might not work on some
> > platforms, due to some hardware prefetch. (Would be better if that was
> > reported to the mailing list instead.) We're trying to figure out the
> > exact requirements here, because half of the frame doesn't sound like
> > something reasonable. Let's hold off the patch for the time being.
>
> I'll repost my last pull request, including a revert of this patch.
>
> The state of this patch in patchwork is changed back to New.
>
> I'll wait for your feedback on what to do with this patch.
>
> Regards,
>
> Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> MediaTek encoder allocates non pixel data area for an input buffer every
> >> plane. As the input buffer should be read-only, the driver should not write
> >> anything in the buffer. Therefore, the extra data should be unnecessary.
> >>
> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >>
> >> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 9 +++------
> >> 1 file changed, 3 insertions(+), 6 deletions(-)
> >>
> >> ---
> >>
> >> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >> index fd8de027e83e..6aad53d97d74 100644
> >> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >> @@ -332,14 +332,12 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> >>
> >> pix_fmt_mp->num_planes = fmt->num_planes;
> >> pix_fmt_mp->plane_fmt[0].sizeimage =
> >> - pix_fmt_mp->width * pix_fmt_mp->height +
> >> - ((ALIGN(pix_fmt_mp->width, 16) * 2) * 16);
> >> + pix_fmt_mp->width * pix_fmt_mp->height;
> >> pix_fmt_mp->plane_fmt[0].bytesperline = pix_fmt_mp->width;
> >>
> >> if (pix_fmt_mp->num_planes == 2) {
> >> pix_fmt_mp->plane_fmt[1].sizeimage =
> >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 2 +
> >> - (ALIGN(pix_fmt_mp->width, 16) * 16);
> >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 2;
> >> pix_fmt_mp->plane_fmt[2].sizeimage = 0;
> >> pix_fmt_mp->plane_fmt[1].bytesperline =
> >> pix_fmt_mp->width;
> >> @@ -347,8 +345,7 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> >> } else if (pix_fmt_mp->num_planes == 3) {
> >> pix_fmt_mp->plane_fmt[1].sizeimage =
> >> pix_fmt_mp->plane_fmt[2].sizeimage =
> >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 4 +
> >> - ((ALIGN(pix_fmt_mp->width, 16) / 2) * 16);
> >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 4;
> >> pix_fmt_mp->plane_fmt[1].bytesperline =
> >> pix_fmt_mp->plane_fmt[2].bytesperline =
> >> pix_fmt_mp->width / 2;
>
Hi Hans, On Tue, Dec 17, 2019 at 3:46 PM Tomasz Figa <tfiga@chromium.org> wrote: > > +Alexandre Courbot > > On Sun, Nov 10, 2019 at 10:36 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > > > On 11/10/19 2:13 PM, Tomasz Figa wrote: > > > Hi Mauro and Hans, > > > > > > On Sun, Nov 10, 2019 at 3:42 PM Mauro Carvalho Chehab > > > <mchehab@kernel.org> wrote: > > >> > > >> This is an automatic generated email to let you know that the following patch were queued: > > >> > > >> Subject: media: mtk-vcodec: Remove extra area allocation in an input buffer on encoding > > >> Author: Hirokazu Honda <hiroh@chromium.org> > > >> Date: Sun Nov 10 07:25:34 2019 +0100 > > > > > > I asked for some more testing on different platforms in my review > > > reply and we got some offline signals that it might not work on some > > > platforms, due to some hardware prefetch. (Would be better if that was > > > reported to the mailing list instead.) We're trying to figure out the > > > exact requirements here, because half of the frame doesn't sound like > > > something reasonable. Let's hold off the patch for the time being. > > > > I'll repost my last pull request, including a revert of this patch. > > > > The state of this patch in patchwork is changed back to New. > > > > I'll wait for your feedback on what to do with this patch. Let's drop this patch indeed. Apparently some implementations of this hardware do read-ahead up to 32 lines, so the extra space is needed. Best regards, Tomasz > > > > Regards, > > > > Hans > > > > > > > > Best regards, > > > Tomasz > > > > > >> > > >> MediaTek encoder allocates non pixel data area for an input buffer every > > >> plane. As the input buffer should be read-only, the driver should not write > > >> anything in the buffer. Therefore, the extra data should be unnecessary. > > >> > > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > >> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > > >> > > >> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 9 +++------ > > >> 1 file changed, 3 insertions(+), 6 deletions(-) > > >> > > >> --- > > >> > > >> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > >> index fd8de027e83e..6aad53d97d74 100644 > > >> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > >> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > >> @@ -332,14 +332,12 @@ static int vidioc_try_fmt(struct v4l2_format *f, > > >> > > >> pix_fmt_mp->num_planes = fmt->num_planes; > > >> pix_fmt_mp->plane_fmt[0].sizeimage = > > >> - pix_fmt_mp->width * pix_fmt_mp->height + > > >> - ((ALIGN(pix_fmt_mp->width, 16) * 2) * 16); > > >> + pix_fmt_mp->width * pix_fmt_mp->height; > > >> pix_fmt_mp->plane_fmt[0].bytesperline = pix_fmt_mp->width; > > >> > > >> if (pix_fmt_mp->num_planes == 2) { > > >> pix_fmt_mp->plane_fmt[1].sizeimage = > > >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 2 + > > >> - (ALIGN(pix_fmt_mp->width, 16) * 16); > > >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 2; > > >> pix_fmt_mp->plane_fmt[2].sizeimage = 0; > > >> pix_fmt_mp->plane_fmt[1].bytesperline = > > >> pix_fmt_mp->width; > > >> @@ -347,8 +345,7 @@ static int vidioc_try_fmt(struct v4l2_format *f, > > >> } else if (pix_fmt_mp->num_planes == 3) { > > >> pix_fmt_mp->plane_fmt[1].sizeimage = > > >> pix_fmt_mp->plane_fmt[2].sizeimage = > > >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 4 + > > >> - ((ALIGN(pix_fmt_mp->width, 16) / 2) * 16); > > >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 4; > > >> pix_fmt_mp->plane_fmt[1].bytesperline = > > >> pix_fmt_mp->plane_fmt[2].bytesperline = > > >> pix_fmt_mp->width / 2; > >
On Thu, Dec 19, 2019 at 6:31 PM Tomasz Figa <tfiga@chromium.org> wrote: > > Hi Hans, > > On Tue, Dec 17, 2019 at 3:46 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > > +Alexandre Courbot > > > > On Sun, Nov 10, 2019 at 10:36 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > > > > > On 11/10/19 2:13 PM, Tomasz Figa wrote: > > > > Hi Mauro and Hans, > > > > > > > > On Sun, Nov 10, 2019 at 3:42 PM Mauro Carvalho Chehab > > > > <mchehab@kernel.org> wrote: > > > >> > > > >> This is an automatic generated email to let you know that the following patch were queued: > > > >> > > > >> Subject: media: mtk-vcodec: Remove extra area allocation in an input buffer on encoding > > > >> Author: Hirokazu Honda <hiroh@chromium.org> > > > >> Date: Sun Nov 10 07:25:34 2019 +0100 > > > > > > > > I asked for some more testing on different platforms in my review > > > > reply and we got some offline signals that it might not work on some > > > > platforms, due to some hardware prefetch. (Would be better if that was > > > > reported to the mailing list instead.) We're trying to figure out the > > > > exact requirements here, because half of the frame doesn't sound like > > > > something reasonable. Let's hold off the patch for the time being. > > > > > > I'll repost my last pull request, including a revert of this patch. > > > > > > The state of this patch in patchwork is changed back to New. > > > > > > I'll wait for your feedback on what to do with this patch. > > Let's drop this patch indeed. Apparently some implementations of this > hardware do read-ahead up to 32 lines, so the extra space is needed. Sorry to revive this, but I see that this patch is currently merged: Merged a first time in 3192b2ca79b3f72fbc0eab9cd95432e3c317ab0d, Reverted in dca6b3733a4a46e63603496f544ece8ace541fde Merged again in 81735ecb62f882853a37a8c157407ec4aed44fd0 Could that last merge be an error? As far as I know the final call was that hardware read-ahead required the initial alignment, and with the patch v4l2-compliance has one more failure on G_FMT. The last merge came before Tomasz last email, and this patch is also not in the Chrome OS tree, so unless someone speaks up I will consider this should have been reverted and will do it in the next iteration of my mtk-vcodec encoder series. > > Best regards, > Tomasz > > > > > > > Regards, > > > > > > Hans > > > > > > > > > > > Best regards, > > > > Tomasz > > > > > > > >> > > > >> MediaTek encoder allocates non pixel data area for an input buffer every > > > >> plane. As the input buffer should be read-only, the driver should not write > > > >> anything in the buffer. Therefore, the extra data should be unnecessary. > > > >> > > > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > >> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > > > >> > > > >> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 9 +++------ > > > >> 1 file changed, 3 insertions(+), 6 deletions(-) > > > >> > > > >> --- > > > >> > > > >> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > >> index fd8de027e83e..6aad53d97d74 100644 > > > >> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > >> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > >> @@ -332,14 +332,12 @@ static int vidioc_try_fmt(struct v4l2_format *f, > > > >> > > > >> pix_fmt_mp->num_planes = fmt->num_planes; > > > >> pix_fmt_mp->plane_fmt[0].sizeimage = > > > >> - pix_fmt_mp->width * pix_fmt_mp->height + > > > >> - ((ALIGN(pix_fmt_mp->width, 16) * 2) * 16); > > > >> + pix_fmt_mp->width * pix_fmt_mp->height; > > > >> pix_fmt_mp->plane_fmt[0].bytesperline = pix_fmt_mp->width; > > > >> > > > >> if (pix_fmt_mp->num_planes == 2) { > > > >> pix_fmt_mp->plane_fmt[1].sizeimage = > > > >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 2 + > > > >> - (ALIGN(pix_fmt_mp->width, 16) * 16); > > > >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 2; > > > >> pix_fmt_mp->plane_fmt[2].sizeimage = 0; > > > >> pix_fmt_mp->plane_fmt[1].bytesperline = > > > >> pix_fmt_mp->width; > > > >> @@ -347,8 +345,7 @@ static int vidioc_try_fmt(struct v4l2_format *f, > > > >> } else if (pix_fmt_mp->num_planes == 3) { > > > >> pix_fmt_mp->plane_fmt[1].sizeimage = > > > >> pix_fmt_mp->plane_fmt[2].sizeimage = > > > >> - (pix_fmt_mp->width * pix_fmt_mp->height) / 4 + > > > >> - ((ALIGN(pix_fmt_mp->width, 16) / 2) * 16); > > > >> + (pix_fmt_mp->width * pix_fmt_mp->height) / 4; > > > >> pix_fmt_mp->plane_fmt[1].bytesperline = > > > >> pix_fmt_mp->plane_fmt[2].bytesperline = > > > >> pix_fmt_mp->width / 2; > > >