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

On 7/30/2026 6:57 PM, Dmitry Baryshkov wrote:
> 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 <[email protected]>
>> ---
> 
>> +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()?
> 

What's wrong with standart BIT_U32() when dealing with 32-bit registers? 
Or do you simply like requesting meaningless changes?!

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