Re: [PATCH v4 5/5] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder
From: Atanas Filipov
Date: Mon Jul 13 2026 - 06:59:59 EST
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;
+};
+