All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chen-Yu Tsai <wenst@chromium.org>
To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Philipp Zabel <p.zabel@pengutronix.de>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	 Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 Tomasz Figa <tfiga@chromium.org>,
	linux-media@vger.kernel.org,  linux-rockchip@lists.infradead.org,
	linux-staging@lists.linux.dev,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFT 5/7] media: hantro: jpeg: Add COM segment to JPEG header to align image scan
Date: Mon, 27 Dec 2021 15:33:48 +0800	[thread overview]
Message-ID: <CAGXv+5GJzZE1xDxxOqzV3Bq2XfuG2-aFuu-6hNxJ9S2YXFM_og@mail.gmail.com> (raw)
In-Reply-To: <YciZZ2hA4uMveN2l@eze-laptop>

On Mon, Dec 27, 2021 at 12:33 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Hi,
>
> On Fri, Dec 24, 2021 at 04:42:46PM +0800, Chen-Yu Tsai wrote:
> > The JPEG header size is not 64-bit aligned. This makes the driver
> > require a bounce buffer for the encoded JPEG image scan output.
> >
> > Add a COM (comment) segment to the JPEG header so that the header size
> > is a multiple of 64 bits. This will then allow dropping the use of the
> > bounce buffer, and instead have the hardware write out to the capture
> > buffer directly.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/staging/media/hantro/hantro_jpeg.c | 3 +++
> >  drivers/staging/media/hantro/hantro_jpeg.h | 2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_jpeg.c b/drivers/staging/media/hantro/hantro_jpeg.c
> > index 7d4018bd6876..51e67e5cf86f 100644
> > --- a/drivers/staging/media/hantro/hantro_jpeg.c
> > +++ b/drivers/staging/media/hantro/hantro_jpeg.c
> > @@ -247,6 +247,9 @@ static const unsigned char hantro_jpeg_header[JPEG_HEADER_SIZE] = {
> >       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >
> > +     /* COM */
> > +     0xff, 0xfe, 0x00, 0x03, 0x00,
> > +
> >       /* SOS */
> >       0xff, 0xda, 0x00, 0x0c, 0x03, 0x01, 0x00, 0x02,
> >       0x11, 0x03, 0x11, 0x00, 0x3f, 0x00,
> > diff --git a/drivers/staging/media/hantro/hantro_jpeg.h b/drivers/staging/media/hantro/hantro_jpeg.h
> > index f33c492134e4..0b49d0b82caa 100644
> > --- a/drivers/staging/media/hantro/hantro_jpeg.h
> > +++ b/drivers/staging/media/hantro/hantro_jpeg.h
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0+ */
> >
> > -#define JPEG_HEADER_SIZE     619
> > +#define JPEG_HEADER_SIZE     624
>
> Can we add some compile-time check for the 8-byte alignment,
> so this is always enforced?

Ack.

> Perhaps getting rid of the JPEG_HEADER_SIZE macro,
> something like this....

I don't think that's doable. The other parts of the driver need to know
how large the header is, and we can't use "sizeof(hantro_jpeg_header)"
in those places unless the size is predetermined in the header declaration,
or we move the definition into the header file. Otherwise we need to
keep the macro and have another static assertion to check that
JPEG_HEADER_SIZE == sizeof(hantro_jpeg_header).

> @@ -140,7 +140,7 @@ static const unsigned char chroma_ac_table[] = {
>   * and we'll use fixed offsets to change the width, height
>   * quantization tables, etc.
>   */
> -static const unsigned char hantro_jpeg_header[JPEG_HEADER_SIZE] = {
> +static const unsigned char hantro_jpeg_header[] = {
>         /* SOI */
>         0xff, 0xd8,
>
> @@ -304,8 +304,13 @@ void hantro_jpeg_header_assemble(struct hantro_jpeg_ctx *ctx)
>  {
>         char *buf = ctx->buffer;
>
> -       memcpy(buf, hantro_jpeg_header,
> -              sizeof(hantro_jpeg_header));
> +       /*
> +        * THE JPEG buffer is prepended with the JPEG header,
> +        * so 64-bit alignment is needed for DMA.
> +        */
> +       BUILD_BUG_ON(!IS_ALIGNED(sizeof(hantro_jpeg_header), 8));

Probably bikeshedding, but I was thinking more of a static assert just
beneath hantro_jpeg_header[], along with some comments.


ChenYu

> +
> +       memcpy(buf, hantro_jpeg_header, sizeof(hantro_jpeg_header));
>
> Thanks,
> Ezequiel

WARNING: multiple messages have this Message-ID
From: Chen-Yu Tsai <wenst@chromium.org>
To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Philipp Zabel <p.zabel@pengutronix.de>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	 Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 Tomasz Figa <tfiga@chromium.org>,
	linux-media@vger.kernel.org,  linux-rockchip@lists.infradead.org,
	linux-staging@lists.linux.dev,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFT 5/7] media: hantro: jpeg: Add COM segment to JPEG header to align image scan
Date: Mon, 27 Dec 2021 15:33:48 +0800	[thread overview]
Message-ID: <CAGXv+5GJzZE1xDxxOqzV3Bq2XfuG2-aFuu-6hNxJ9S2YXFM_og@mail.gmail.com> (raw)
In-Reply-To: <YciZZ2hA4uMveN2l@eze-laptop>

On Mon, Dec 27, 2021 at 12:33 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Hi,
>
> On Fri, Dec 24, 2021 at 04:42:46PM +0800, Chen-Yu Tsai wrote:
> > The JPEG header size is not 64-bit aligned. This makes the driver
> > require a bounce buffer for the encoded JPEG image scan output.
> >
> > Add a COM (comment) segment to the JPEG header so that the header size
> > is a multiple of 64 bits. This will then allow dropping the use of the
> > bounce buffer, and instead have the hardware write out to the capture
> > buffer directly.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/staging/media/hantro/hantro_jpeg.c | 3 +++
> >  drivers/staging/media/hantro/hantro_jpeg.h | 2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_jpeg.c b/drivers/staging/media/hantro/hantro_jpeg.c
> > index 7d4018bd6876..51e67e5cf86f 100644
> > --- a/drivers/staging/media/hantro/hantro_jpeg.c
> > +++ b/drivers/staging/media/hantro/hantro_jpeg.c
> > @@ -247,6 +247,9 @@ static const unsigned char hantro_jpeg_header[JPEG_HEADER_SIZE] = {
> >       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >
> > +     /* COM */
> > +     0xff, 0xfe, 0x00, 0x03, 0x00,
> > +
> >       /* SOS */
> >       0xff, 0xda, 0x00, 0x0c, 0x03, 0x01, 0x00, 0x02,
> >       0x11, 0x03, 0x11, 0x00, 0x3f, 0x00,
> > diff --git a/drivers/staging/media/hantro/hantro_jpeg.h b/drivers/staging/media/hantro/hantro_jpeg.h
> > index f33c492134e4..0b49d0b82caa 100644
> > --- a/drivers/staging/media/hantro/hantro_jpeg.h
> > +++ b/drivers/staging/media/hantro/hantro_jpeg.h
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0+ */
> >
> > -#define JPEG_HEADER_SIZE     619
> > +#define JPEG_HEADER_SIZE     624
>
> Can we add some compile-time check for the 8-byte alignment,
> so this is always enforced?

Ack.

> Perhaps getting rid of the JPEG_HEADER_SIZE macro,
> something like this....

I don't think that's doable. The other parts of the driver need to know
how large the header is, and we can't use "sizeof(hantro_jpeg_header)"
in those places unless the size is predetermined in the header declaration,
or we move the definition into the header file. Otherwise we need to
keep the macro and have another static assertion to check that
JPEG_HEADER_SIZE == sizeof(hantro_jpeg_header).

> @@ -140,7 +140,7 @@ static const unsigned char chroma_ac_table[] = {
>   * and we'll use fixed offsets to change the width, height
>   * quantization tables, etc.
>   */
> -static const unsigned char hantro_jpeg_header[JPEG_HEADER_SIZE] = {
> +static const unsigned char hantro_jpeg_header[] = {
>         /* SOI */
>         0xff, 0xd8,
>
> @@ -304,8 +304,13 @@ void hantro_jpeg_header_assemble(struct hantro_jpeg_ctx *ctx)
>  {
>         char *buf = ctx->buffer;
>
> -       memcpy(buf, hantro_jpeg_header,
> -              sizeof(hantro_jpeg_header));
> +       /*
> +        * THE JPEG buffer is prepended with the JPEG header,
> +        * so 64-bit alignment is needed for DMA.
> +        */
> +       BUILD_BUG_ON(!IS_ALIGNED(sizeof(hantro_jpeg_header), 8));

Probably bikeshedding, but I was thinking more of a static assert just
beneath hantro_jpeg_header[], along with some comments.


ChenYu

> +
> +       memcpy(buf, hantro_jpeg_header, sizeof(hantro_jpeg_header));
>
> Thanks,
> Ezequiel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

  reply	other threads:[~2021-12-27  7:34 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-24  8:42 [PATCH RFT 0/7] media: hantro: jpeg: Various improvements Chen-Yu Tsai
2021-12-24  8:42 ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 1/7] media: hantro: jpeg: Relax register writes before write starting hardware Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 2/7] media: hantro: Fix overfill bottom register field name Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 3/7] media: hantro: Support cropping visible area for encoders Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 4/7] media: hantro: jpeg: Add JFIF APP0 segment to JPEG encoder output Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 5/7] media: hantro: jpeg: Add COM segment to JPEG header to align image scan Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-26 16:33   ` Ezequiel Garcia
2021-12-26 16:33     ` Ezequiel Garcia
2021-12-27  7:33     ` Chen-Yu Tsai [this message]
2021-12-27  7:33       ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 6/7] media: hantro: Implement V4L2_CID_JPEG_ACTIVE_MARKER control Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai
2021-12-24  8:42 ` [PATCH RFT 7/7] media: hantro: output encoded JPEG content directly to capture buffers Chen-Yu Tsai
2021-12-24  8:42   ` Chen-Yu Tsai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXv+5GJzZE1xDxxOqzV3Bq2XfuG2-aFuu-6hNxJ9S2YXFM_og@mail.gmail.com \
    --to=wenst@chromium.org \
    --cc=ezequiel@vanguardiasur.com.ar \
    --cc=gregkh@linuxfoundation.org \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=mchehab@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=tfiga@chromium.org \
    --subject='Re: [PATCH RFT 5/7] media: hantro: jpeg: Add COM segment to JPEG header to align image scan' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.