Re: [PATCH v6 2/3] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[Dmitry Baryshkov]

On Thu, Jul 30, 2026 at 06:05:23PM +0300, Atanas Filipov wrote:
> Add a Qualcomm JPEG encoder driver implemented on top of the
> V4L2 mem2mem framework.
> 
> The driver wires vb2 queue handling, format negotiation, JPEG header
> handling, interrupt-driven job completion, and runtime PM/clock/ICC
> integration for the standalone JPEG encode hardware block.
> 
> This series targets SM8250 (Kona) platforms.
> 
> Signed-off-by: Atanas Filipov <atanas.filipov@xxxxxxxxxxxxxxxx>
> ---

> +static int qcom_jpeg_clk_on(struct qcom_jenc_dev *jenc)
> +{
> +	struct dev_pm_opp *opp;
> +	int rc;
> +
> +	rc = clk_bulk_prepare_enable(jenc->num_clks, jenc->clks);
> +	if (rc)
> +		return rc;
> +
> +	/* setup the OPP according to the calculated optimal frequency */
> +	opp = dev_pm_opp_find_freq_ceil_indexed(jenc->dev, &jenc->opt_freq, JPEG_OPP_CORE_IDX);

clk_bulk_prepare_enable starts ticking on a certain freq. Why does it
happen _before_ setting the performance points (including voltage rails)
which those clocks require?

> +	if (IS_ERR(opp)) {
> +		rc = PTR_ERR(opp);
> +		goto err_clk_disable;
> +	}
> +
> +	rc = dev_pm_opp_set_opp(jenc->dev, opp);
> +	dev_pm_opp_put(opp);
> +	if (rc)
> +		goto err_clk_disable;
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_bulk_disable_unprepare(jenc->num_clks, jenc->clks);
> +
> +	return rc;
> +}
> +
> +static void qcom_jpeg_clk_off(struct qcom_jenc_dev *jenc)
> +{
> +	dev_pm_opp_set_opp(jenc->dev, NULL);
> +	clk_bulk_disable_unprepare(jenc->num_clks, jenc->clks);
> +	jenc->opt_freq = jenc->max_freq;
> +}
> +

[...]

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_hdr.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_hdr.c
> new file mode 100644
> index 000000000000..c9959518c64d
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_hdr.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +
> +#include <media/jpeg.h>
> +#include <media/v4l2-jpeg.h>
> +
> +#include "qcom_jenc_dev.h"
> +#include "qcom_jenc_hdr.h"
> +
> +/*
> + * The elements defined in this header are specified
> + * in the ITU-T T.81 / JPEG specification.
> + *
> + * https://www.w3.org/Graphics/JPEG/itu-t81.pdf
> + */
> +
> +#define JFIF_HEADER_WIDTH_OFFS		0x07
> +#define JFIF_HEADER_HEIGHT_OFFS		0x05

And you've ignored feedback here. PLEASE move all standard-related
defines and code to the common helpers. Are there any other drivers
which construct JPEG files manually? If not, you are lucky and you can
just push you code. If they are, find a way to unify the codebase.

At the very least, it would make you split this commit into at least
two, making them more readable.

> +#define JFIF_APP0_LENGTH_HI		0x00
> +#define JFIF_APP0_LENGTH_LO		0x10
> +#define JFIF_IDENT_TERM		0x00
> +#define JFIF_VERSION_MAJOR		0x01
> +#define JFIF_VERSION_MINOR		0x01
> +#define JFIF_DENSITY_HI			0x00
> +#define JFIF_DENSITY_LO			0x01
> +#define JFIF_THUMBNAIL_SIZE		0x00
> +
> +#define JPEG_SEG_LEN_HI			0x00
> +#define JPEG_LEN_DQT_LUMA_LO		0x43
> +#define JPEG_LEN_DQT_CHROMA_LO		0x43
> +#define JPEG_LEN_SOF0_MONO_LO		0x0b
> +#define JPEG_LEN_SOF0_COLOR_LO		0x11
> +#define JPEG_LEN_DHT_MONO_LO		0xd2
> +#define JPEG_LEN_DHT_COLOR_HI		0x01
> +#define JPEG_LEN_DHT_COLOR_LO		0xa2
> +#define JPEG_LEN_SOS_MONO_LO		0x08
> +#define JPEG_LEN_SOS_COLOR_LO		0x0c
> +

[...]

> +
> +static inline u32 jpeg_io_read(struct qcom_jenc_dev *jenc, u32 offset)
> +{
> +	return readl(jenc->jpeg_base + offset);
> +}

Ugh.

> +
> +static inline void jpeg_io_write(struct qcom_jenc_dev *jenc, u32 offset, u32 value)
> +{
> +	writel(value, jenc->jpeg_base + offset);
> +}
> +
> +/*
> + * Runtime bitfield helpers (for non-constant masks).

Why are the masks non-constant? They typically are, for register access.
Use FIELD_GET and FIELD_PREP.

> + *
> + * Requirements:
> + *  - mask must be non-zero
> + *  - mask must be contiguous (e.g. 0x7u << n)
> + */
> +
> +static inline u32 jpeg_bits_get(u32 mask, u32 val)
> +{
> +	/* __ffs(0) is undefined; fail-safe on invalid masks. */
> +	if (WARN_ON_ONCE(!mask))
> +		return 0;
> +
> +	return (val & mask) >> __ffs(mask);
> +}
> +
> +static inline u32 jpeg_bits_set(u32 mask, u32 val)
> +{
> +	/* __ffs(0) is undefined; fail-safe on invalid masks. */
> +	if (WARN_ON_ONCE(!mask))
> +		return 0;
> +
> +	return (val << __ffs(mask)) & mask;
> +}
> +
> +static inline u32 jpeg_rd_bits(struct qcom_jenc_dev *jenc, u32 offs, u32 mask)
> +{
> +	u32 reg = jpeg_io_read(jenc, offs);
> +
> +	return jpeg_bits_get(mask, reg);
> +}
> +
> +/*
> + * Read-modify-write (for R/W registers)
> + */
> +static inline void jpeg_rw_bits(struct qcom_jenc_dev *jenc, u32 offs, u32 mask, u32 val)
> +{
> +	u32 reg = jpeg_io_read(jenc, offs);
> +
> +	reg &= ~mask;
> +	reg |= jpeg_bits_set(mask, val);
> +
> +	jpeg_io_write(jenc, offs, reg);

And if you switch to regmap, you'd have gotten a nice function for this
too.

> +}
> +
> +/*
> + * Write-only variant (for write only registers)
> + */
> +static inline void jpeg_wo_bits(struct qcom_jenc_dev *jenc, u32 offs, u32 mask, u32 val)
> +{
> +	jpeg_io_write(jenc, offs, jpeg_bits_set(mask, val));

Ugh.

> +}
> +
> +

[...]

> +const struct qcom_jpeg_hw_ops qcom_jpeg_default_ops = {
> +	.hw_get_cap	= op_jpeg_get_hw_caps,
> +	.hw_acquire	= op_jpeg_acquire,
> +	.hw_release	= op_jpeg_release,
> +	.hw_prepare	= op_jpeg_prepare,
> +	.get_queue	= op_jpeg_get_buff_queue,
> +	.queue_setup	= op_jpeg_queue_setup,
> +	.src_fmt_update	= op_jpeg_src_fmt_update,
> +	.buf_prepare	= op_jpeg_buffer_prepare,
> +	.process_exec	= op_jpeg_process_exec,
> +	.hw_irq_top	= op_jpeg_irq_top,
> +	.hw_irq_bot	= op_jpeg_irq_bot

Function names should start with qcom_jpeg_

> +};
> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.h b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.h
> new file mode 100644
> index 000000000000..5a64e33a28b0
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef QCOM_JENC_OPS_H
> +#define QCOM_JENC_OPS_H
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +#include <media/videobuf2-core.h>
> +
> +#include "qcom_jenc_dev.h"
> +
> +/*
> + * JENC encoder hardware operations.
> + */
> +struct qcom_jpeg_hw_ops {
> +	void (*hw_get_cap)
> +		(struct qcom_jenc_dev *jenc_dev, u32 *hw_caps);
> +
> +	int (*hw_acquire)
> +		(struct jenc_context *ectx, struct vb2_queue *queue);
> +
> +	int (*hw_release)
> +		(struct jenc_context *ectx, struct vb2_queue *queue);
> +
> +	int (*hw_prepare)
> +		(struct qcom_jenc_dev *jenc);
> +
> +	struct qcom_jenc_queue * (*get_queue)
> +		(struct jenc_context *ectx, enum qcom_enc_qid id);
> +
> +	int (*queue_setup)
> +		(struct jenc_context *ectx, enum qcom_enc_qid id);
> +
> +	int (*src_fmt_update)
> +		(struct jenc_context *ectx, u32 old_fourcc, u32 new_fourcc);
> +
> +	int (*buf_prepare)
> +		(struct jenc_context *ectx, struct vb2_buffer *vb2);
> +
> +	int (*process_exec)
> +		(struct qcom_jenc_dev *jenc, struct jenc_context *ectx, struct vb2_buffer *vb2);
> +
> +	irqreturn_t (*hw_irq_top)(int irq_num, void *data);
> +	irqreturn_t (*hw_irq_bot)(int irq_num, void *data);

How many non-default platforms do you support? Zero?

Drop the call table.

> +};
> +
> +extern const struct qcom_jpeg_hw_ops qcom_jpeg_default_ops;
> +
> +#endif /* QCOM_JENC_OPS_H */
> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h
> new file mode 100644
> index 000000000000..ebf69128cc2b
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h
> @@ -0,0 +1,410 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef QCOM_JENC_V420_HW_INFO_H
> +#define QCOM_JENC_V420_HW_INFO_H
> +
> +#include "qcom_jenc_defs.h"
> +
> +#define JPEG_V420_HW_VER_STEP_POS 0
> +#define JPEG_V420_HW_VER_STEP_MSK \
> +	(0xffffu << JPEG_V420_HW_VER_STEP_POS)

No need to define _POS. Use GENMASK to define bitfields.

> +
> +#define JPEG_V420_HW_VER_MINOR_POS 16
> +#define JPEG_V420_HW_VER_MINOR_MSK \
> +	(0x0fffu << JPEG_V420_HW_VER_MINOR_POS)
> +
> +#define JPEG_V420_HW_VER_MAJOR_POS 28
> +#define JPEG_V420_HW_VER_MAJOR_MSK \
> +	(0xfu << JPEG_V420_HW_VER_MAJOR_POS)
> +
> +#define JPEG_V420_HW_CAP_ENCODE_MSK BIT_U32(0)
> +#define JPEG_V420_HW_CAP_DECODE_MSK BIT_U32(1)
> +
> +#define JPEG_V420_HW_CAP_UPSCALE_POS 4
> +#define JPEG_V420_HW_CAP_UPSCALE_MSK \
> +	(0x7u << JPEG_V420_HW_CAP_UPSCALE_POS)
> +
> +#define JPEG_V420_HW_CAP_DOWNSCALE_POS 8
> +#define JPEG_V420_HW_CAP_DOWNSCALE_MSK \
> +	(0x7u << JPEG_V420_HW_CAP_DOWNSCALE_POS)
> +
> +#define JPEG_V420_RST_CMD_FE_RESET_MSK			BIT_U32(0)

What is wrong with the standard BIT()?

> +#define JPEG_V420_RST_CMD_WE_RESET_MSK			BIT_U32(1)
> +#define JPEG_V420_RST_CMD_ENCODER_RESET_MSK		BIT_U32(4)
> +#define JPEG_V420_RST_CMD_DECODER_RESET_MSK		BIT_U32(5)
> +#define JPEG_V420_RST_CMD_BLOCK_FORMATTER_RST_MSK	BIT_U32(6)
> +#define JPEG_V420_RST_CMD_SCALE_RESET_MSK		BIT_U32(7)
> +#define JPEG_V420_RST_CMD_REGISTER_RESET_MSK		BIT_U32(13)
> +#define JPEG_V420_RST_CMD_MISR_RESET_MSK		BIT_U32(16)
> +#define JPEG_V420_RST_CMD_CORE_RESET_MSK		BIT_U32(17)
> +#define JPEG_V420_RST_CMD_JPEG_V420_DOMAIN_RESET_MSK	BIT_U32(29)
> +#define JPEG_V420_RST_CMD_RESET_BYPASS_MSK		BIT_U32(31)
> +

> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index affec0ab4781..6408d3da29d4 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -234,6 +234,27 @@ enum v4l2_colorfx {
>   */
>  #define V4L2_CID_USER_MALI_C55_BASE		(V4L2_CID_USER_BASE + 0x1230)
>  
> +/* Qualcomm JPEG encoder controls */
> +#define V4L2_CID_USER_QCOM_JENC_BASE		(V4L2_CID_USER_BASE + 0x1240)

Separate these to their own commit, making it easier for others to
review uAPI additions. How are these being handled by the other drivers?

> +
> +/*
> + * V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO - enable adaptive performance scaling.
> + *
> + * When set to 1 the driver selects the core clock OPP level based on the
> + * encoded frame resolution and fps target. When set to 0 (default) the
> + * driver always runs at NOMINAL (highest) OPP level.
> + */
> +#define V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO	(V4L2_CID_USER_QCOM_JENC_BASE + 0)
> +
> +/*
> + * V4L2_CID_QCOM_JPEG_FPS_TARGET - target encode rate in frames per second.
> + *
> + * Used together with V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO to select the lowest
> + * OPP level whose throughput is sufficient for the requested frame rate.
> + * Has no effect when perf_level_auto is 0. Range: 1-240, default: 30.

I assume 1-240 is only applicable to your driver.

> + */
> +#define V4L2_CID_QCOM_JPEG_FPS_TARGET		(V4L2_CID_USER_QCOM_JENC_BASE + 1)
> +
>  /* MPEG-class control IDs */
>  /* The MPEG controls are applicable to all codec controls
>   * and the 'MPEG' part of the define is historical */
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry