Re: [PATCH RFT 5/7] media: hantro: jpeg: Add COM segment to JPEG header to align image scan

[Chen-Yu Tsai <wenst@chromium.org>]

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