Re: [EXT] Re: [PATCH v7 3/9] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Hans Verkuil]

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 <mirela.rabulea@xxxxxxx>
>>>
>>> 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 <mirela.rabulea@xxxxxxx>
>>> ---
>>> 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.

In the latter case you want to support the V4L2_PIX_FMT_FLAG_SET_CSC
flag:

https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/pixfmt-v4l2.html#c.v4l2_pix_format

That would allow userspace to specify the desired quantization range.

Regards,

	Hans

> Also, I looked at mtk_jpeg_set_default_params for example.
> Let me know if you have a different suggestion.
> 
> Thanks,
> Mirela