Re: [PATCH v4 5/5] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[Atanas Filipov <atanas.filipov@oss.qualcomm.com>]

On 7/6/2026 4:11 PM, Dmitry Baryshkov wrote:
> On Mon, Jul 06, 2026 at 10:11:13AM +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.
>>
>> The jpeg-encoder node is described as a child node of the CAMSS block
>> and is probed automatically via of_platform_populate() in camss_probe().
>>
>> Usage examples:
>>
>> - Check of related video node: v4l2-ctl --list-devices
>>    The expected result:
>>     qcom-jpeg-enc (platform:qcom-jpeg-enc):
>>          /dev/videoX
>>
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml b/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
>> index e4c16388ef07..53e83ebe4699 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
>> @@ -72,80 +72,75 @@ examples:
>>       #include <dt-bindings/interconnect/qcom,sm8250.h>
>>       #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   
>> -    jpeg-encoder@ac53000 {
>> -        compatible = "qcom,sm8250-jenc";
>> -        reg = <0xac53000 0x1000>;
> 
> What is going on here? And why?

The binding example was updated in the driver patch by mistake. v5
will have binding and driver in separate patches.

> 
>> -
>> -        interrupts = <GIC_SPI 474 IRQ_TYPE_EDGE_RISING>;
>> -
>> -        clocks = <&gcc GCC_CAMERA_HF_AXI_CLK>,
>> -                 <&gcc GCC_CAMERA_SF_AXI_CLK>,
>> -                 <&camcc CAM_CC_CORE_AHB_CLK>,
>> -                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>> -                 <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>> -                 <&camcc CAM_CC_JPEG_CLK>;
>> -        clock-names = "hf_axi",
>> -                      "sf_axi",
>> -                      "core_ahb",
>> -                      "cpas_ahb",
>> -                      "cnoc_axi",
>> -                      "jpeg";
>> -
>> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_defs.h b/drivers/media/platform/qcom/jpeg/qcom_jenc_defs.h
>> new file mode 100644
>> index 000000000000..2ab29bfb9b88
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_defs.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#ifndef QCOM_JENC_DEFS_H
>> +#define QCOM_JENC_DEFS_H
>> +
>> +#include <linux/types.h>
>> +#include <uapi/linux/v4l2-controls.h>
> 
> There includes are not necessary for this header.
> 
>> +
>> +/* Offline JPEG encoder constraints */
>> +#define QCOM_JPEG_HW_MAX_WIDTH	8192
>> +#define QCOM_JPEG_HW_MAX_HEIGHT	8192
>> +#define QCOM_JPEG_HW_MIN_WIDTH	256
>> +#define QCOM_JPEG_HW_MIN_HEIGHT	256
>> +
>> +#define QCOM_JPEG_HW_DEF_HSTEP	16
>> +#define QCOM_JPEG_HW_DEF_VSTEP	16
>> +
>> +#define QCOM_JPEG_HW_DEF_WIDTH	1920
>> +#define QCOM_JPEG_HW_DEF_HEIGHT	1088
>> +
>> +#define QCOM_JPEG_MAX_PLANES	3
>> +
>> +#define QCOM_JPEG_QUALITY_MIN	1
>> +#define QCOM_JPEG_QUALITY_DEF	98
>> +#define QCOM_JPEG_QUALITY_MAX	100
>> +#define QCOM_JPEG_QUALITY_MID	(QCOM_JPEG_QUALITY_MAX / 2)
>> +#define QCOM_JPEG_QUALITY_UNT	1
>> +
>> +#define QCOM_JPEG_FPS_MIN	1
>> +#define QCOM_JPEG_FPS_MAX	240
>> +#define QCOM_JPEG_FPS_DEF	30
>> +#define QCOM_JPEG_FPS_UNT	1
> 
> This is a collection of random defines, which are mostly used once.
> Please move them to the corresponding source file.
>

Acknowledged. v5 will remove the unnecessary includes and move the
defines to their respective source files.

>> +
>> +#endif /* QCOM_JENC_DEFS_H */
>> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
>> new file mode 100644
>> index 000000000000..ddfa84838b6b
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
>> @@ -0,0 +1,314 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +
>> +#include <media/v4l2-mem2mem.h>
>> +
>> +#include "qcom_jenc_dev.h"
>> +#include "qcom_jenc_ops.h"
>> +#include "qcom_jenc_res.h"
>> +#include "qcom_jenc_v4l2.h"
>> +
>> +enum jpeg_opp_clks_id {
>> +	JPEG_OPP_CNOC_IDX = 0,
>> +	JPEG_OPP_CORE_IDX
>> +};
>> +
>> +static const char * const opp_clk_names[] = {
>> +	[JPEG_OPP_CNOC_IDX] = "cnoc_axi",
> 
> This way your driver will force its own frequency on the CNOC_AXI clock.
> There are other clients of the clock, so there should be some kind of
> voting on it. Either use icc-clk or add CAMSS API to sum the votes on
> the CNOC_AXI.
> 

Addressed in the reply to your DTS comment.

>> +	[JPEG_OPP_CORE_IDX] = "jpeg",
>> +	NULL,
>> +};
>> +
>> +static struct dev_pm_opp_config opp_config = {
>> +	.clk_names = opp_clk_names,
>> +	.config_clks = dev_pm_opp_config_clks_simple,
>> +};
>> +
>> +static int qcom_jpeg_opp_init(struct qcom_jenc_dev *jenc)
>> +{
>> +	struct dev_pm_opp *opp;
>> +	int rc;
>> +
>> +	rc = devm_pm_opp_set_config(jenc->dev, &opp_config);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = devm_pm_opp_of_add_table(jenc->dev);
>> +	if (rc && rc != -ENODEV)
>> +		return rc;
>> +
>> +	/* initialize the maximum available frequency for the JPEG core */
>> +	jenc->max_freq = ULONG_MAX;
>> +	opp = dev_pm_opp_find_freq_floor_indexed(jenc->dev, &jenc->max_freq, JPEG_OPP_CORE_IDX);
>> +	if (IS_ERR(opp))
>> +		return PTR_ERR(opp);
>> +
>> +	dev_pm_opp_put(opp);
>> +
>> +	/* initialize the default optimized frequency for the JPEG core */
>> +	jenc->opt_freq = jenc->max_freq;
>> +
>> +	dev_dbg(jenc->dev, "JPEG max clocks is: %lu\n", jenc->max_freq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_jpeg_clk_init(struct qcom_jenc_dev *jenc)
>> +{
>> +	jenc->num_clks = devm_clk_bulk_get_all(jenc->dev, &jenc->clks);
>> +	if (jenc->num_clks < 0)
>> +		return jenc->num_clks;
>> +
>> +	return 0;
>> +}
>> +
>> +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);
>> +	if (IS_ERR(opp)) {
>> +		rc = PTR_ERR(opp);
>> +		goto err_clk_disable;
>> +	}
>> +
>> +	rc = dev_pm_opp_set_opp(jenc->dev, opp);
>> +	if (rc)
>> +		goto err_dev_pm_opp;
>> +
>> +	dev_dbg(jenc->dev, "selected OPP clocks cnoc=%lu, core=%lu\n",
>> +		dev_pm_opp_get_freq_indexed(opp, JPEG_OPP_CNOC_IDX),
>> +		dev_pm_opp_get_freq_indexed(opp, JPEG_OPP_CORE_IDX));
> 
> Drop extra debugging, you can enable debugging for OPP via
> CONFIG_DEBUG_DRIVER.
> 

Acknowledged. v5 will drop the dev_dbg.

>> +
>> +	dev_pm_opp_put(opp);
>> +
>> +	return 0;
>> +
>> +err_dev_pm_opp:
>> +	dev_pm_opp_put(opp);
> 
> Once you drop excessive debugging code, the dev_pm_opp_put() will find
> its natural place right after dev_pm_opp_set_opp(), before checking the
> rc.
> 

Acknowledged. Will be fixed in v5.

>> +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;
>> +}
>> +
>> +/* qcom_jpeg_camss_get - resume the parent CAMSS device */
>> +static int qcom_jpeg_camss_get(struct qcom_jenc_dev *jenc)
>> +{
>> +	return pm_runtime_resume_and_get(jenc->camss_dev);
> 
> Use devlinks instead.
> 

Acknowledged. v5 will model JPEG as a standalone peer node —
the camss_dev parent reference will be dropped entirely.

>> +}
>> +
>> +/* qcom_jpeg_camss_put - release the parent CAMSS device */
>> +static void qcom_jpeg_camss_put(struct qcom_jenc_dev *jenc)
>> +{
>> +	pm_runtime_put_sync(jenc->camss_dev);
>> +}
>> +
>> +static int qcom_jpeg_pm_suspend(struct device *dev)
>> +{
>> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>> +
>> +	qcom_jpeg_clk_off(jenc);
>> +	qcom_jpeg_camss_put(jenc);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_jpeg_pm_resume(struct device *dev)
>> +{
>> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = qcom_jpeg_camss_get(jenc);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = qcom_jpeg_clk_on(jenc);
>> +	if (rc) {
>> +		qcom_jpeg_camss_put(jenc);
>> +		return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_jpeg_pm_system_suspend(struct device *dev)
>> +{
>> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	v4l2_m2m_suspend(jenc->m2m_dev);
>> +
>> +	rc = pm_runtime_force_suspend(dev);
>> +	if (rc)
>> +		v4l2_m2m_resume(jenc->m2m_dev);
>> +
>> +	return rc;
>> +}
>> +
>> +static int qcom_jpeg_pm_system_resume(struct device *dev)
>> +{
>> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = pm_runtime_force_resume(dev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	v4l2_m2m_resume(jenc->m2m_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static _DEFINE_DEV_PM_OPS(qcom_jpeg_pm_ops,
>> +			  qcom_jpeg_pm_system_suspend, qcom_jpeg_pm_system_resume,
>> +			  qcom_jpeg_pm_suspend, qcom_jpeg_pm_resume, NULL);
>> +
>> +static int qcom_jpeg_probe(struct platform_device *pdev)
>> +{
>> +	const struct qcom_dev_resources *res;
>> +	struct qcom_jenc_dev *jenc;
>> +	int rc;
>> +
>> +	jenc = devm_kzalloc(&pdev->dev, sizeof(*jenc), GFP_KERNEL);
>> +	if (!jenc)
>> +		return -ENOMEM;
>> +
>> +	jenc->dev = &pdev->dev;
>> +	jenc->camss_dev = pdev->dev.parent;
>> +	platform_set_drvdata(pdev, jenc);
>> +	rc = devm_mutex_init(&pdev->dev, &jenc->dev_mutex);
>> +	if (rc)
>> +		goto err_free_jenc;
>> +	spin_lock_init(&jenc->hw_lock);
>> +	init_completion(&jenc->reset_complete);
>> +	init_completion(&jenc->stop_complete);
>> +
>> +	res = device_get_match_data(jenc->dev);
>> +	if (!res) {
>> +		rc = dev_err_probe(jenc->dev, -ENODEV, "unsupported SoC\n");
>> +		goto err_free_jenc;
> 
> drop the gotos, it's an empty label now.
> 

Acknowledged. Will be fixed in v5.

Best regards,
Atanas

>> +	}
>> +	jenc->res = res;
>> +
>> +	if (!jenc->res->hw_ops) {
>> +		rc = dev_err_probe(jenc->dev, -EINVAL, "missing hw resources\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = dma_set_mask_and_coherent(jenc->dev, DMA_BIT_MASK(32));
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to set DMA mask\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	jenc->jpeg_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(jenc->jpeg_base)) {
>> +		rc = dev_err_probe(jenc->dev, PTR_ERR(jenc->jpeg_base),
>> +				   "failed to map JPEG resource\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = qcom_jpeg_opp_init(jenc);
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to init OPP\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = qcom_jpeg_clk_init(jenc);
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to init clocks\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	jenc->irq = platform_get_irq(pdev, 0);
>> +	if (jenc->irq < 0) {
>> +		rc = dev_err_probe(jenc->dev, jenc->irq, "failed to get IRQ\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = devm_request_threaded_irq(jenc->dev, jenc->irq,
>> +				       jenc->res->hw_ops->hw_irq_top,
>> +				       jenc->res->hw_ops->hw_irq_bot,
>> +				       IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(jenc->dev), jenc);
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to request IRQ\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = v4l2_device_register(jenc->dev, &jenc->v4l2_dev);
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to register V4L2 device\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	rc = devm_add_action_or_reset(jenc->dev,
>> +				      (void (*)(void *))v4l2_device_unregister,
>> +				      &jenc->v4l2_dev);
>> +	if (rc)
>> +		goto err_free_jenc;
>> +
>> +	rc = devm_pm_runtime_enable(jenc->dev);
>> +	if (rc)
>> +		goto err_free_jenc;
>> +
>> +	rc = qcom_jpeg_v4l2_register(jenc);
>> +	if (rc) {
>> +		dev_err_probe(jenc->dev, rc, "failed to register video device\n");
>> +		goto err_free_jenc;
>> +	}
>> +
>> +	dev_dbg(jenc->dev, "Qualcomm JPEG encoder registered\n");
>> +
>> +	return 0;
>> +
>> +err_free_jenc:
>> +	return rc;
>> +}
>> +
> 
> [...]
> 
>> 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
> 
> This is offset_of(), no need to define those.
> 
> I think, you already got a review from me. Move all standard-related
> defines and code to the generic v4l2 code, unless you get an explicit
> blessing from one of V4L2 maintainers not to do so.
> 
>> +#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
>> +
>> +struct jpeg_header_buf {
>> +	u8  *ptr;
>> +	u32 size;
>> +	u32 pos;
>> +};
>> +
>