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

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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?

> -
> -        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.

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

> +	[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.

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

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

> +}
> +
> +/* 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.

> +	}
> +	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;
> +};
> +

-- 
With best wishes
Dmitry