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.

quoted

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.

+ *

quoted

+ * 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.

quoted

+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.

quoted

+/*  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. 

quoted

+	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.

quoted

+	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