Re: [PATCH v4 04/11] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Laurentiu,

On Mon, 2020-11-02 at 18:20 +0200, Laurentiu Palcu wrote:
> Hi Mirela,
> 
> I wanted to give it a more in-depth look but I then saw that patch 11
> deletes a lot of code from this file. So, the review on the deleted
> parts would be pointless... :/
> 
> I suggest you squash 4 and 11 together.

Sorry about that, I refrained from squashing 4 & 11 for 2 reasons:

1. On patch 4 I did not make significant changes since previous version
(just enough to keep it compiling) I hoped it will be easier to review
like this, tried to explain in cover letter.

2. After using the jpeg helpers, the mxc_jpeg_parse() is somewhere
between 2-8% slower, depending on the measuring method, so I was
thinking it would be nice to have the previous method in the history,
as a reference. Now, I have done more measurements on the overall
impact, an it is insignificant, ~0.01% for a streaming case (1 1080p
RGB24 buffer enqueued 1000 times).

I can definitely  squash patch 7 into 4, together with other changes
from this version review. I'm waiting for more opinions about squashing
11 into 4.

> 
> > v4l2-compliance tests are passing, including the
> > streaming tests, "v4l2-compliance -s"
> > 
> > V3 Replaced GRABBER
> 
> What does this mean?

Removed, my bad, that info was added in cover letter.

> + *
> > + * A module parameter is available for debug purpose
> > (jpeg_tracing), to enable
> > + * it, enable dynamic debug for this module and:
> > + * echo 1 > /sys/module/mxc_jpeg_encdec/parameters/jpeg_tracing
> 
> It looks like this it's trying to achieve the same thing that's
> already
> offered by v4l2_dbg() and the 'debug' module parameter. The advantage
> of
> the latter is that you don't need to recompile the kernel to enable
> dynamic debug... Maybe it's worth reusing it?

I replaced jpeg_tracing module parameter with debug module parameter in
the next version, so, it will be shared with what v4l2_dbg is using.

> > +0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3, 0xC4,
> > +0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2,
> > +0xD3, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8, 0xD9,
> > +0xDA, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7,
> > +0xE8, 0xE9, 0xEA, 0xF2, 0xF3, 0xF4, 0xF5,
> > +0xF6, 0xF7, 0xF8, 0xF9, 0xFA};
> > +static const unsigned char jpeg_dri[] = {0xFF, 0xDD,
> > +0x00, 0x04, 0x00, 0x20};
> > +static const unsigned char jpeg_sos_maximal[] = {0xFF, 0xDA,
> > +0x00, 0x0C, 0x04, 0x01, 0x00, 0x02, 0x11, 0x03,
> > +0x11, 0x04, 0x11, 0x00, 0x3F, 0x00,};
> > +static const unsigned char jpeg_eoi[] = {0xFF, 0xD9};
> 
> These data blocks above should be properly indented, one tab to the
> right.

Done, for the next version.

> > +/*  Print Four-character-code (FOURCC) */
> > +static char *fourcc_to_str(u32 format)
> > +{
> > +	char *buf = kmalloc(32, GFP_KERNEL);
> 
> I'm not sure this is worth it. Either you make it a static array:
> 
> static char buf[5];
> 
> And return a pointer to it, unless this is called from different
> contexts. Or you could make the caller pass a pointer to the buffer.
> Using kmalloc() every time you want to print 4 characters might
> introduce some unnecessary overhead if this is called too often.
> 
> However, my sugestion is to get rid of this fourcc_to_str() helper
> completely and print the format directly where you need it. Here's a
> list of places where this is done, in case you have second thoughts:
> 
> git grep "\(v4l2_dbg\|pr_cont\|dev_\).*%c%c%c%c"
> 

Removed fourcc_to_str(), used %c%c%c%c instead, the amount of lines of
code is similar, so, it's ok, no loss. 

> > +	struct mxc_jpeg_sof sof, *psof = 0;
> > +	struct mxc_jpeg_sos *psos = 0;
> > +	u8 byte, *next = 0;
> 
> Don't use 0 for NULL pointers. Use NULL. :)

Done, it will be in the next version.

> 
> > +	enum mxc_jpeg_image_format img_fmt;
> > +	u32 fourcc;
> > +
> > +	memset(&sof, 0, sizeof(struct mxc_jpeg_sof));
> > +	stream.addr = src_addr;
> > +	stream.end = size;
> > +	stream.loc = 0;
> > +	*dht_needed = true;
> > +	while (notfound) {
> > +		byte = get_byte(&stream);
> > +		if (byte == -1)
> 
> byte is u8. The above doesn't make much sense.

This is handled differently now in the upstream jpeg helpers
(v4l2_jpeg_parse_header() and jpeg_next_marker()).
However, in the above, there is a problem, the end of the stream is not
detected properly. I'll modify get_byte to return int, not u8, because
-1 is for the end of stream, and 0xFF is
for markers (in u8 representation both were 0xFF). This passed
undetected because, for a valid jpeg the parsing was done only up to
SOS. So, a problematic case would be a corrupted jpeg, with SOI, but
without SOS. I'll try to add some corrupted jpegs in my testsuite.

Thanks,
Mirela