Re: [PATCH v7 2/4] media: rockchip: Add JPEG decoder driver

[Sascha Hauer]

Hi Torsten,

On 2026-10-22 19:00, Torsten Duwe wrote:
> Hi Sascha,
> 
> first: thank you for driving this :)
> 
> On Thu, 22 Oct 2026 13:06:40 +0200
> Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
> 
> > Add a driver for the JPEG hardware decoder Rockchip integrates into a
> > number of its SoCs. 
> 
> [...]
> 
> > +static int rkjpegd_enum_fmt_vid_out(struct file *file, void *priv,
> > +				    struct v4l2_fmtdesc *f)
> > +{
> > +	if (f->index)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = V4L2_PIX_FMT_JPEG;
> > +	f->flags |= V4L2_FMT_FLAG_DYN_RESOLUTION;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rkjpegd_enum_framesizes(struct file *file, void *priv,
> > +				   struct v4l2_frmsizeenum *fsize)
> > +{
> > +	/* The decoded format follows the bitstream, nothing to enumerate. */
> > +	if (fsize->pixel_format == V4L2_PIX_FMT_NV12 ||
> > +	    fsize->pixel_format == V4L2_PIX_FMT_GREY)
> > +		return -ENOTTY;
> > +
> > +	if (fsize->pixel_format != V4L2_PIX_FMT_JPEG)
> > +		return -EINVAL;
> > +
> > +	if (fsize->index)
> > +		return -EINVAL;
> > +
> > +	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > +	fsize->stepwise.min_width = RKJPEGD_MIN_WIDTH;
> > +	fsize->stepwise.max_width = RKJPEGD_MAX_SIZE;
> > +	fsize->stepwise.step_width = RKJPEGD_CODED_STEP;
> > +	fsize->stepwise.min_height = RKJPEGD_MIN_HEIGHT;
> > +	fsize->stepwise.max_height = RKJPEGD_MAX_SIZE;
> > +	fsize->stepwise.step_height = RKJPEGD_CODED_STEP;
> > +
> > +	return 0;
> > +}
> 
> Can it do JPEG, MJPEG or maybe both? AFAICT the difference is minimal,
> so maybe try to be more permissive?

It can do both. Gstreamer uses image/jpeg und thus V4L2_PIX_FMT_JPEG for
both, so it just works. It seems most jpeg decoders in Linux do not have
need to support V4L2_PIX_FMT_MJPEG.

> 
> [...]
> 
> > +static const struct v4l2_ioctl_ops rkjpegd_ioctl_ops = {
> > +	.vidioc_querycap = rkjpegd_querycap,
> > +	.vidioc_enum_framesizes = rkjpegd_enum_framesizes,
> > +
> > +	.vidioc_enum_fmt_vid_cap = rkjpegd_enum_fmt_vid_cap,
> > +	.vidioc_g_fmt_vid_cap_mplane = rkjpegd_g_fmt_vid_cap,
> > +	.vidioc_try_fmt_vid_cap_mplane = rkjpegd_try_fmt_vid_cap,
> > +	.vidioc_s_fmt_vid_cap_mplane = rkjpegd_s_fmt_vid_cap,
> > +
> > +	.vidioc_enum_fmt_vid_out = rkjpegd_enum_fmt_vid_out,
> > +	.vidioc_g_fmt_vid_out_mplane = rkjpegd_g_fmt_vid_out,
> > +	.vidioc_try_fmt_vid_out_mplane = rkjpegd_try_fmt_vid_out,
> > +	.vidioc_s_fmt_vid_out_mplane = rkjpegd_s_fmt_vid_out,
> > 
> 
> You are populating only the *_out_mplane members and not the single
> plane ones? So a greyscale JPEG needs to go _mplane with 1 plane?

Yes.

> 
> And for color, the colorspace on the decoded side should be
> V4L2_COLORSPACE_JPEG, not sRGB, I think.

from colorspace-details.rst:

   The JPEG standard does not actually store colorspace
   information. So if something other than sRGB is used, then the driver
   will have to set that information explicitly. Effectively
   ``V4L2_COLORSPACE_JPEG`` can be considered to be an abbreviation for
   ``V4L2_COLORSPACE_SRGB``, ``V4L2_XFER_FUNC_SRGB``, ``V4L2_YCBCR_ENC_601``
   and ``V4L2_QUANTIZATION_FULL_RANGE``.

These are the values the driver sets on the capture side. mxc-jpeg for
example does the same.

> 
> Would you mind to give a hint about your test setup?

I tested with Gstreamer, like different variants of:

for format in I420 Y42B Y444 Y41B GRAY8; do x=1920; y=1088; GST_DEBUG=2 \
        gst-launch-1.0 videotestsrc num-buffers=100 ! \
        video/x-raw,format=$format,width=$x,height=$y ! \
        jpegenc ! jpegparse ! v4l2jpegdec ! waylandsink sync=false; done

I also tested several single frame jpegs with various defects/oddities
in them, also all with gstreamer.

Sascha

--
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |