Hi Mirela,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on shawnguo/for-next robh/for-next linus/master v5.11-rc4 next-20210120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Mirela-Rabulea/Add-V4L2-driver-for-i-MX8-JPEG-Encoder-Decoder/20210112-033507
base: git://linuxtv.org/media_tree.git master
config: i386-randconfig-m021-20210120 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

smatch warnings:
drivers/media/platform/imx-jpeg/mxc-jpeg.c:1373 mxc_jpeg_parse() warn: inconsistent indenting

vim +1373 drivers/media/platform/imx-jpeg/mxc-jpeg.c

1286
1287 static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx,
1288 u8 *src_addr, u32 size, bool *dht_needed)
1289 {
1290 struct device *dev = ctx->mxc_jpeg->dev;
1291 struct mxc_jpeg_q_data *q_data_out, *q_data_cap;
1292 enum v4l2_buf_type cap_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
1293 struct mxc_jpeg_stream stream;
1294 bool notfound = true;
1295 bool app14 = false;
1296 bool src_chg = false;
1297 u8 app14_transform = 0;
1298 struct mxc_jpeg_sof sof, *psof = NULL;
1299 struct mxc_jpeg_sos *psos = NULL;
1300 int byte;
1301 u8 *next = NULL;
1302 enum mxc_jpeg_image_format img_fmt;
1303 u32 fourcc;
1304
1305 memset(&sof, 0, sizeof(struct mxc_jpeg_sof));
1306 stream.addr = src_addr;
1307 stream.end = size;
1308 stream.loc = 0;
1309 *dht_needed = true;
1310
1311 /* check stream starts with SOI */
1312 byte = get_byte(&stream);
1313 if (byte == -1 || byte != 0xFF)
1314 return -EINVAL;
1315 byte = get_byte(&stream);
1316 if (byte == -1 || byte != 0xD8)
1317 return -EINVAL;
1318
1319 while (notfound) {
1320 byte = get_byte(&stream);
1321 if (byte == -1)
1322 return -EINVAL;
1323 if (byte != 0xff)
1324 continue;
1325 do {
1326 byte = get_byte(&stream);
1327 } while (byte == 0xff);
1328 if (byte == -1)
1329 return false;
1330 if (byte == 0)
1331 continue;
1332 switch (byte) {
1333 case DHT:
1334 /* DHT marker present, no need to inject default one */
1335 *dht_needed = false;
1336 break;
1337 case SOF2: /* Progressive DCF frame definition */
1338 dev_err(dev,
1339 "Progressive JPEG not supported by hardware");
1340 return -EINVAL;
1341 case SOF1: /* Extended sequential DCF frame definition */
1342 case SOF0: /* Baseline sequential DCF frame definition */
1343 if (get_sof(dev, &stream, &sof) == -1)
1344 break;
1345 next = stream.addr + stream.loc;
1346 psof = (struct mxc_jpeg_sof *)next;
1347 break;
1348 case SOS:
1349 next = stream.addr + stream.loc;
1350 psos = (struct mxc_jpeg_sos *)next;
1351 notfound = false;
1352 break;
1353 case APP14:
1354 app14 = true;
1355 /*
1356 * Application Data Syntax is:
1357 * 2 bytes(APPn:0xFF,0xEE), 2 bytes(Lp), Ap1...ApLp-2
1358 * The transform flag is in Ap12
1359 * stream.loc is now on APPn-0xEE byte
1360 */
1361 app14_transform = *(stream.addr + stream.loc + 12 + 1);
1362 break;
1363 default:
1364 notfound = true;
1365 }
1366 }
1367 q_data_out = mxc_jpeg_get_q_data(ctx,
1368 V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
1369 if (q_data_out->w == 0 && q_data_out->h == 0) {
1370 dev_warn(dev, "Invalid user resolution 0x0");
1371 dev_warn(dev, "Keeping resolution from JPEG: %dx%d",
1372 sof.width, sof.height);
> 1373 q_data_out->w = sof.width;
1374 q_data_out->h = sof.height;
1375 } else if (sof.width != q_data_out->w || sof.height != q_data_out->h) {
1376 dev_err(dev,
1377 "Resolution mismatch: %dx%d (JPEG) versus %dx%d(user)",
1378 sof.width, sof.height, q_data_out->w, q_data_out->h);
1379 return -EINVAL;
1380 }
1381 if (sof.width % 8 != 0 || sof.height % 8 != 0) {
1382 dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
1383 sof.width, sof.height);
1384 return -EINVAL;
1385 }
1386 if (sof.width > MXC_JPEG_MAX_WIDTH ||
1387 sof.height > MXC_JPEG_MAX_HEIGHT) {
1388 dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
1389 sof.width, sof.height);
1390 return -EINVAL;
1391 }
1392 if (sof.width < MXC_JPEG_MIN_WIDTH ||
1393 sof.height < MXC_JPEG_MIN_HEIGHT) {
1394 dev_err(dev, "JPEG width or height should be > 64: %dx%d\n",
1395 sof.width, sof.height);
1396 return -EINVAL;
1397 }
1398 if (sof.components_no > MXC_JPEG_MAX_COMPONENTS) {
1399 dev_err(dev, "JPEG number of components should be <=%d",
1400 MXC_JPEG_MAX_COMPONENTS);
1401 return -EINVAL;
1402 }
1403 /* check and, if necessary, patch component IDs*/
1404 if (!mxc_jpeg_valid_comp_id(dev, psof, psos))
1405 dev_warn(dev, "JPEG component ids should be 0-3 or 1-4");
1406
1407 img_fmt = mxc_jpeg_get_image_format(dev, &sof);
1408 if (img_fmt == MXC_JPEG_INVALID)
1409 return -EINVAL;
1410
1411 /*
1412 * If the transform flag from APP14 marker is 0, images that are
1413 * encoded with 3 components have RGB colorspace, see Recommendation
1414 * ITU-T T.872 chapter 6.5.3 APP14 marker segment for colour encoding
1415 */
1416 if (img_fmt == MXC_JPEG_YUV444 && app14 && app14_transform == 0)
1417 img_fmt = MXC_JPEG_RGB;
1418
1419 if (mxc_jpeg_imgfmt_to_fourcc(img_fmt, &fourcc)) {
1420 dev_err(dev, "Fourcc not found for %d", img_fmt);
1421 return -EINVAL;
1422 }
1423
1424 /*
1425 * set-up the capture queue with the pixelformat and resolution
1426 * detected from the jpeg output stream
1427 */
1428 q_data_cap = mxc_jpeg_get_q_data(ctx, cap_type);
1429 if (q_data_cap->w != sof.width || q_data_cap->h != sof.height)
1430 src_chg = true;
1431 q_data_cap->w = sof.width;
1432 q_data_cap->h = sof.height;
1433 q_data_cap->fmt = mxc_jpeg_find_format(ctx, fourcc);
1434 q_data_cap->w_adjusted = q_data_cap->w;
1435 q_data_cap->h_adjusted = q_data_cap->h;
1436 /*
1437 * align up the resolution for CAST IP,
1438 * but leave the buffer resolution unchanged
1439 */
1440 v4l_bound_align_image(&q_data_cap->w_adjusted,
1441 q_data_cap->w_adjusted, /* adjust up */
1442 MXC_JPEG_MAX_WIDTH,
1443 q_data_cap->fmt->h_align,
1444 &q_data_cap->h_adjusted,
1445 q_data_cap->h_adjusted, /* adjust up */
1446 MXC_JPEG_MAX_HEIGHT,
1447 q_data_cap->fmt->v_align,
1448 0);
1449 dev_dbg(dev, "Detected jpeg res=(%dx%d)->(%dx%d), pixfmt=%c%c%c%c\n",
1450 q_data_cap->w, q_data_cap->h,
1451 q_data_cap->w_adjusted, q_data_cap->h_adjusted,
1452 (fourcc & 0xff),
1453 (fourcc >> 8) & 0xff,
1454 (fourcc >> 16) & 0xff,
1455 (fourcc >> 24) & 0xff);
1456
1457 /* setup bytesperline/sizeimage for capture queue */
1458 mxc_jpeg_bytesperline(q_data_cap, sof.precision);
1459 mxc_jpeg_sizeimage(q_data_cap);
1460
1461 /*
1462 * if the CAPTURE format was updated with new values, regardless of
1463 * whether they match the values set by the client or not, signal
1464 * a source change event
1465 */
1466 if (src_chg)
1467 notify_src_chg(ctx);
1468
1469 return 0;
1470 }
1471

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (7.88 kB)
.config.gz (31.41 kB)
Download all attachments

2021-01-20 20:28:18

On 10/03/2021 13:33, Mirela Rabulea wrote:
> Hi Hans,
>
> On Thu, 2021-03-04 at 14:03 +0100, Hans Verkuil wrote:
>> Caution: EXT Email
>>
>> On 22/02/2021 20:09, Mirela Rabulea wrote:
>>> Hi Hans,
>>> appologies for my late response, please see below 2 comments.
>>
>> Replies below:
>>
>>>
>>> On Tue, 2021-01-19 at 11:31 +0100, Hans Verkuil wrote:
>>>> Caution: EXT Email
>>>>
>>>> On 11/01/2021 20:28, Mirela Rabulea wrote:
>>>>> From: Mirela Rabulea <[email protected]>
>>>>>
>>>>> V4L2 driver for the JPEG encoder/decoder from i.MX8QXP/i.MX8QM
>>>>> application
>>>>> processors.
>>>>> The multi-planar buffers API is used.
>>>>>
>>>>> Baseline and extended sequential jpeg decoding is supported.
>>>>> Progressive jpeg decoding is not supported by the IP.
>>>>> Supports encode and decode of various formats:
>>>>> YUV444, YUV422, YUV420, RGB, ARGB, Gray
>>>>> YUV420 is the only multi-planar format supported.
>>>>> Minimum resolution is 64 x 64, maximum 8192 x 8192.
>>>>> The alignment requirements for the resolution depend on the
>>>>> format,
>>>>> multiple of 16 resolutions should work for all formats.
>>>>>
>>>>> v4l2-compliance tests are passing, including the
>>>>> streaming tests, "v4l2-compliance -s"
>>>>>
>>>>> Signed-off-by: Mirela Rabulea <[email protected]>
>>>>> ---
>>>>> Changes in v7:
>>>>> Add print_mxc_buf() to replace print_buf_preview() and
>>>>> print_nbuf_to_eoi(),
>>>>> and inside, use the print_hex_dump() from printk.h, also,
>>>>> print
>>>>> all the planes.
>>>>>
>>>>> drivers/media/platform/Kconfig | 2 +
>>>>> drivers/media/platform/Makefile | 1 +
>>>>> drivers/media/platform/imx-jpeg/Kconfig | 10 +
>>>>> drivers/media/platform/imx-jpeg/Makefile | 3 +
>>>>> drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c | 168 ++
>>>>> drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h | 140 +
>>>>> drivers/media/platform/imx-jpeg/mxc-jpeg.c | 2289
>>>>> +++++++++++++++++
>>>>> drivers/media/platform/imx-jpeg/mxc-jpeg.h | 184 ++
>>>>> 8 files changed, 2797 insertions(+)
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/Kconfig
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/Makefile
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-
>>>>> hw.c
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-
>>>>> hw.h
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.c
>>>>> create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.h
>>>>
>>>> One high-level comment: why introduce the driver in patch 3/9 and
>>>> then
>>>> change it again in 9/9? I would very much prefer to have just a
>>>> single
>>>> patch that adds this driver, i.e. merge patch 3 and 9 into a
>>>> single
>>>> patch.
>>>
>>> I can squash patch 9 into patch 3, but please note that patch 9
>>> depends
>>> on jpeg helper patches 6,7,8, so these patches will also have to
>>> move
>>> before patch 3. Let me know yout thought and I'll do this in v9, in
>>> v8
>>> version I only addressed the rest of your feedback and some more
>>> from
>>> Philipp.
>>
>> Yes, just move the jpeg helper to earlier in the patch series.
>>
>>>
>>>>
>>
>> <snip>
>>
>>>>> + /* fix colorspace information to sRGB for both output &
>>>>> capture */
>>>>> + pix_mp->colorspace = V4L2_COLORSPACE_SRGB;
>>>>> + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>>>> + pix_mp->xfer_func = V4L2_XFER_FUNC_SRGB;
>>>>> + pix_mp->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>>>
>>>> So YUV formats are expected to be full range as well? Both when
>>>> encoding
>>>> and decoding?
>>>
>>> I set the colorspace information like that based on this comment:
>>> /*
>>> * Effectively shorthand for V4L2_COLORSPACE_SRGB,
>>> V4L2_YCBCR_ENC_601
>>> * and V4L2_QUANTIZATION_FULL_RANGE. To be used for (Motion-
>>> )JPEG.
>>> */
>>> V4L2_COLORSPACE_JPEG = 7,
>>>
>>
>> Inside a JPEG the YUV quantization is using full range. But when you
>> *decode* a JPEG the YUV quantization in the raw decoded image is
>> normally limited range again (the default for YUV).
>>
>> It depends on what this decoder does: most will decode to limited
>> range YUV, some decode to full range YUV (in which case this code
>> would be
>> correct), and some support both.
>
> Experimentally, I saw the decoder outputs full-range values, but I was
> not sure about limited-range support, so I asked our IP owner, and I
> got this answer:
> "The decoder does not alter the range of the data in any way.
> It outputs the same full or limited range data that was given to the
> encoder."
>
> So, surely our encoder/decoder cannot change the range of the samples,
> but it could process both full or half range.
> I was thinking about a possibility to allow a half-range streams
> scenario (half range raw format -> encoder -> jpeg with half range???
> -> decoder -> half range raw format).
> That could be achieved by allowing user to choose (specify) the
> quantization for the output stream, and the driver would set the same
> for the capture stream, but this would result in a JPEG with half range
> for the capture stream.
> So, you mentioned inside JPEG the YUV quantization is using full range,
> experience confirms it (I decoded a jpeg produced with gimp and one
> with ms-paint, and I saw full range values), and v4l2-compliance fails
> if the pixel format is JPEG and quantization is not full.
> I'm not sure if the standard allows half-range JPEG (it would be a
> subset of full-range, so theoretically possible).
>
> So, I will just add a comment to clarify the mxc-jpeg driver will
> always use full-range.
>
> The mxc-jpeg driver will not support CSC, therefor it is not setting
> V4L2_FMT_FLAG_CSC_COLORSPACE in v4l2_fmtdesc during enumeration.
> So, the application cannot request a specific colorspace for the
> capture stream. No change needed here.
>
> Let me know if this sounds ok, and I'll send v9 with the added coment
> and the squash.

Yes, just add a comment that the JPEG codec always uses full range YUV
for the uncompressed frames.

Regards,

Hans

>
> Thanks,
> Mirela
>
>>
>> In the latter case you want to support the V4L2_PIX_FMT_FLAG_SET_CSC
>> flag:
>>
>>
>