| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <69a3d328-748c-445d-bcbd-823b5b314543@oss.nxp.com>
Date: Thu, 29 Jan 2026 09:42:39 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Frank Li <Frank.li@....com>
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl, mirela.rabulea@....nxp.com,
nicolas@...fresne.ca, shawnguo@...nel.org, s.hauer@...gutronix.de,
kernel@...gutronix.de, festevam@...il.com, xiahong.bao@....com,
eagle.zhou@....com, linux-imx@....com, imx@...ts.linux.dev,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: imx-jpeg: Add encoder V1 support for i.MX952
Hi Frank,
On 1/29/2026 4:48 AM, Frank Li wrote:
> On Tue, Jan 27, 2026 at 03:37:00PM +0800, ming.qian@....nxp.com wrote:
>> From: Ming Qian <ming.qian@....nxp.com>
>>
>> The i.MX952 SoC features an upgraded JPEG encoder (version 1) with
>> enhanced descriptor-based configuration capabilities.
>>
>> The hardware version can be determined by reading the
>> version register.
>>
>> The v1 encoder uses an expanded descriptor format that allows
>> configuring all encoding parameters, including JPEG quality,
>> directly in the descriptor. This eliminates the manual
>> configuration phase required by v0 and reduces the interrupt
>> count from two to one per frame.
>>
>> V0 encoding flow:
>> 1. Write quality to registers -> trigger config interrupt
>> 2. Start encoding -> trigger completion interrupt
>>
>> V1 encoding flow:
>> 1. Configure descriptor with all parameters including quality
>> 2. Start encoding -> trigger completion interrupt
>
> AI polished commit message
>
> jpeg: imx: add support for JPEG encoder v1 descriptor configuration
>
> Support the upgraded JPEG encoder v1 found on i.MX952 SoC.
>
> Detect the encoder hardware version via the version register.
>
> The v1 encoder uses an expanded descriptor format that allows all encoding
> parameters, including JPEG quality, to be configured directly in the
> descriptor.
>
> This removes the manual register-based configuration step required by v0
> and reduces the interrupt count from two to one per frame.
>
OK, will apply this improvement in V2.
>>
>> Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>> ---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 10 +-
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h | 4 +-
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 92 ++++++++++++++++---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.h | 24 ++++-
>> 4 files changed, 112 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
>> index 9a6e8b332e12..97a6e1426ba2 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
>> @@ -2,7 +2,7 @@
>> /*
>> * i.MX8QXP/i.MX8QM JPEG encoder/decoder v4l2 driver
>> *
>> - * Copyright 2018-2019 NXP
>> + * Copyright 2018-2026 NXP
>> */
>>
>> #include <linux/delay.h>
>> @@ -189,3 +189,11 @@ void mxc_jpeg_clr_desc(void __iomem *reg, int slot)
>> {
>> writel(0, reg + MXC_SLOT_OFFSET(slot, SLOT_NXT_DESCPT_PTR));
>> }
>> +
>> +int mxc_jpeg_get_version(void __iomem *reg)
>> +{
>> + u32 regval;
>> +
>> + regval = readl(reg + GLB_CTRL);
>> + return GLB_CTRL_CUR_VERSION(regval);
>> +}
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> index adb93e977be9..e9c7573f0fe4 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> @@ -2,7 +2,7 @@
>> /*
>> * i.MX8QXP/i.MX8QM JPEG encoder/decoder v4l2 driver
>> *
>> - * Copyright 2018-2019 NXP
>> + * Copyright 2018-2026 NXP
>> */
>>
>> #ifndef _MXC_JPEG_HW_H
>> @@ -73,6 +73,7 @@
>> #define GLB_CTRL_DEC_GO (0x1 << 2)
>> #define GLB_CTRL_L_ENDIAN(le) ((le) << 3)
>> #define GLB_CTRL_SLOT_EN(slot) (0x1 << ((slot) + 4))
>> +#define GLB_CTRL_CUR_VERSION(r) FIELD_GET(0xF0000, r)
>
> 0xF0000 use GEN_MASK(16, 23)
>
OK, will fix it in V2
>>
>> /* COM_STAUS fields */
>> #define COM_STATUS_DEC_ONGOING(r) (((r) & (1 << 31)) >> 31)
>> @@ -129,4 +130,5 @@ void mxc_jpeg_set_res(struct mxc_jpeg_desc *desc, u16 w, u16 h);
>> void mxc_jpeg_set_line_pitch(struct mxc_jpeg_desc *desc, u32 line_pitch);
>> void mxc_jpeg_set_desc(u32 desc, void __iomem *reg, int slot);
>> void mxc_jpeg_clr_desc(void __iomem *reg, int slot);
>> +int mxc_jpeg_get_version(void __iomem *reg);
>
> Only use at mxc_jpeg_runtime_resume(), it can be removed here and it can
> be static in C file.
>
But it's defined and used in different c file.
I will move it to mxc-jpeg.c in V2.
>> #endif
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index b558700d1d96..9624eea2450d 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -37,7 +37,7 @@
>> *
>> * This is inspired by the drivers/media/platform/samsung/s5p-jpeg driver
>> *
>> - * Copyright 2018-2019 NXP
>> + * Copyright 2018-2026 NXP
>> */
>>
>> #include <linux/kernel.h>
>> @@ -64,6 +64,12 @@
>> #include "mxc-jpeg-hw.h"
>> #include "mxc-jpeg.h"
>>
>> +#define call_void_jpeg_enc_ops(jpeg, op, args...) \
>> + do { \
>> + if ((jpeg)->enc_cfg_ops && (jpeg)->enc_cfg_ops->op) \
>> + (jpeg)->enc_cfg_ops->op(args); \
>> + } while (0)
>> +
>> static const struct mxc_jpeg_fmt mxc_formats[] = {
>> {
>> .name = "JPEG",
>> @@ -1030,11 +1036,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>
>> if (jpeg->mode == MXC_JPEG_ENCODE &&
>> ctx->enc_state == MXC_JPEG_ENC_CONF) {
>> - q_data = mxc_jpeg_get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>> - ctx->enc_state = MXC_JPEG_ENCODING;
>> - dev_dbg(dev, "Encoder config finished. Start encoding...\n");
>> - mxc_jpeg_enc_set_quality(dev, reg, ctx->jpeg_quality);
>> - mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>> + call_void_jpeg_enc_ops(jpeg, exit_config_mode, ctx);
>> goto job_unlock;
>> }
>> if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>> @@ -1272,6 +1274,7 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf,
>>
>> jpeg_src_buf = vb2_to_mxc_buf(src_buf);
>>
>> + ctx->extseq = mxc_jpeg_is_extended_sequential(jpeg_src_buf->fmt);
>> /* setup the decoding descriptor */
>> desc->next_descpt_ptr = 0; /* end of chain */
>> q_data_cap = mxc_jpeg_get_q_data(ctx, cap_type);
>> @@ -1335,9 +1338,15 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
>> struct mxc_jpeg_q_data *q_data;
>> enum mxc_jpeg_image_format img_fmt;
>> int w, h;
>> + bool extseq;
>>
>> q_data = mxc_jpeg_get_q_data(ctx, src_buf->vb2_queue->type);
>> + extseq = mxc_jpeg_is_extended_sequential(q_data->fmt);
>> +
>> + ctx->extseq = extseq;
>>
>> + memset(desc, 0, sizeof(struct mxc_jpeg_desc));
>> + memset(cfg_desc, 0, sizeof(struct mxc_jpeg_desc));
>> jpeg->slot_data.cfg_stream_size =
>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>> q_data->fmt->fourcc,
>> @@ -1348,11 +1357,6 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
>>
>> cfg_desc->buf_base0 = jpeg->slot_data.cfg_stream_handle;
>> - cfg_desc->buf_base1 = 0;
>> - cfg_desc->line_pitch = 0;
>> - cfg_desc->stm_bufbase = 0; /* no output expected */
>> - cfg_desc->stm_bufsize = 0x0;
>> - cfg_desc->imgsize = 0;
>> cfg_desc->stm_ctrl = STM_CTRL_CONFIG_MOD(1);
>> cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
>>
>> @@ -1372,11 +1376,14 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
>> desc->stm_ctrl = STM_CTRL_CONFIG_MOD(0) |
>> STM_CTRL_IMAGE_FORMAT(img_fmt);
>> desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
>> - if (mxc_jpeg_is_extended_sequential(q_data->fmt))
>> + if (extseq)
>> desc->stm_ctrl |= STM_CTRL_PIXEL_PRECISION;
>> else
>> desc->stm_ctrl &= ~STM_CTRL_PIXEL_PRECISION;
>> mxc_jpeg_addrs(desc, src_buf, dst_buf, 0);
>> +
>> + call_void_jpeg_enc_ops(jpeg, setup_desc, ctx);
>> +
>> dev_dbg(jpeg->dev, "cfg_desc:\n");
>> print_descriptor_info(jpeg->dev, cfg_desc);
>> dev_dbg(jpeg->dev, "enc desc:\n");
>> @@ -1388,6 +1395,54 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
>> mxc_jpeg_set_desc(cfg_desc_handle, reg, slot);
>> }
>>
>> +static void mxc_jpeg_enc_start_config_manually(struct mxc_jpeg_ctx *ctx)
>> +{
>> + struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> + void __iomem *reg = jpeg->base_reg;
>> + struct device *dev = jpeg->dev;
>> +
>> + ctx->enc_state = MXC_JPEG_ENC_CONF;
>> + mxc_jpeg_enc_mode_conf(dev, reg, ctx->extseq);
>> +}
>> +
>> +static void mxc_jpeg_enc_finish_config_manually(struct mxc_jpeg_ctx *ctx)
>> +{
>> + struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> + void __iomem *reg = jpeg->base_reg;
>> + struct device *dev = jpeg->dev;
>> +
>> + ctx->enc_state = MXC_JPEG_ENCODING;
>> + dev_dbg(dev, "Encoder config finished. Start encoding...\n");
>> + mxc_jpeg_enc_set_quality(dev, reg, ctx->jpeg_quality);
>> + mxc_jpeg_enc_mode_go(dev, reg, ctx->extseq);
>> +}
>> +
>> +static void mxc_jpeg_enc_configure_desc(struct mxc_jpeg_ctx *ctx)
>> +{
>> + struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> + struct mxc_jpeg_desc *desc = jpeg->slot_data.desc;
>> + struct mxc_jpeg_desc *cfg_desc = jpeg->slot_data.cfg_desc;
>> +
>> + ctx->enc_state = MXC_JPEG_ENCODING;
>> + cfg_desc->mode = (ctx->extseq) ? 0xb0 : 0xa0;
>> + cfg_desc->cfg_mode = 0x3ff;
>> +
>> + desc->mode = (ctx->extseq) ? 0x150 : 0x140;
>> + desc->cfg_mode = 0x3ff;
>> + desc->quality = ctx->jpeg_quality;
>> + desc->lumth = 0xffff;
>> + desc->chrth = 0xffff;
>> +}
>> +
>> +static const struct mxc_jpeg_enc_ops mxc_jpeg_enc_cfg_ops_v0 = {
>> + .enter_config_mode = mxc_jpeg_enc_start_config_manually,
>> + .exit_config_mode = mxc_jpeg_enc_finish_config_manually
>> +};
>> +
>> +static const struct mxc_jpeg_enc_ops mxc_jpeg_enc_cfg_ops_v1 = {
>> + .setup_desc = mxc_jpeg_enc_configure_desc
>> +};
>> +
>> static const struct mxc_jpeg_fmt *mxc_jpeg_get_sibling_format(const struct mxc_jpeg_fmt *fmt)
>> {
>> int i;
>> @@ -1593,12 +1648,10 @@ static void mxc_jpeg_device_run(void *priv)
>>
>> if (jpeg->mode == MXC_JPEG_ENCODE) {
>> dev_dbg(dev, "Encoding on slot %d\n", ctx->slot);
>> - ctx->enc_state = MXC_JPEG_ENC_CONF;
>> mxc_jpeg_config_enc_desc(&dst_buf->vb2_buf, ctx,
>> &src_buf->vb2_buf, &dst_buf->vb2_buf);
>> /* start config phase */
>> - mxc_jpeg_enc_mode_conf(dev, reg,
>> - mxc_jpeg_is_extended_sequential(q_data_out->fmt));
>> + call_void_jpeg_enc_ops(jpeg, enter_config_mode, ctx);
>> } else {
>> dev_dbg(dev, "Decoding on slot %d\n", ctx->slot);
>> print_mxc_buf(jpeg, &src_buf->vb2_buf, 0);
>> @@ -3006,6 +3059,15 @@ static int mxc_jpeg_runtime_resume(struct device *dev)
>> return ret;
>> }
>>
>> + if (jpeg->mode == MXC_JPEG_ENCODE) {
>> + if (!jpeg->enc_cfg_ops) {
>> + if (mxc_jpeg_get_version(jpeg->base_reg) == 0)
>> + jpeg->enc_cfg_ops = &mxc_jpeg_enc_cfg_ops_v0;
>> + else
>> + jpeg->enc_cfg_ops = &mxc_jpeg_enc_cfg_ops_v1;
>
> This should only do once at probe.
>
OK, will fix it in v2
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> index 9c5b4f053ded..8e68dcde9613 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> @@ -2,7 +2,7 @@
>> /*
>> * i.MX8QXP/i.MX8QM JPEG encoder/decoder v4l2 driver
>> *
>> - * Copyright 2018-2019 NXP
>> + * Copyright 2018-2026 NXP
>> */
>>
>> #include <media/v4l2-ctrls.h>
>> @@ -81,6 +81,17 @@ struct mxc_jpeg_desc {
>> u32 stm_bufsize;
>> u32 imgsize;
>> u32 stm_ctrl;
>> + /* below parameters are valid for v1 */
>> + u32 mode;
>> + u32 cfg_mode;
>> + u32 quality;
>> + u32 rc_regs_sel;
>> + u32 lumth;
>> + u32 chrth;
>> + u32 nomfrsize_lo;
>> + u32 nomfrsize_hi;
>> + u32 ofbsize_lo;
>> + u32 ofbsize_hi;
>
> Do you only use one desc each time? if desc is array based, it will impact
> v0.
>
The desc is not array based, each descriptor is assigned a separate DMA
buffer.
For v0, properties added at the end will not be read. Therefore, it
should have no effect.
>> } __packed;
>>
>> struct mxc_jpeg_q_data {
>> @@ -105,6 +116,7 @@ struct mxc_jpeg_ctx {
>> unsigned int source_change;
>> bool need_initial_source_change_evt;
>> bool header_parsed;
>> + bool extseq;
>> struct v4l2_ctrl_handler ctrl_handler;
>> u8 jpeg_quality;
>> struct delayed_work task_timer;
>> @@ -125,6 +137,15 @@ struct mxc_jpeg_slot_data {
>> dma_addr_t cfg_dec_daddr;
>> };
>>
>> +struct mxc_jpeg_enc_ops {
>> + /* Manual configuration (v0 hardware) - two-phase process */
>> + void (*enter_config_mode)(struct mxc_jpeg_ctx *ctx);
>> + void (*exit_config_mode)(struct mxc_jpeg_ctx *ctx);
>> +
>> + /* Descriptor-based configuration (v1 hardware) - single-phase */
>> + void (*setup_desc)(struct mxc_jpeg_ctx *ctx);
>
> You use callback, suppose callback name should be the same for v0 and v1.
>
> for example:
>
> void (*setup_config)()
> void (*clean_config)()
>
> v0 enter_config_mode() -> .setup_config
> exit_config_mode() -> .clean_config
>
> v1
> setup_desc -> .setup_config
> NULL -> .clean_config.
>
> Frank
>
However, the flow for calling the callback in v0 and v1 are different.
If use the same callback name, then additional checks will be required
during the call.
Therefore, I prefer to use different names to handle the different flow.
Regards,
Ming
>> +};
>> +
>> struct mxc_jpeg_dev {
>> spinlock_t hw_lock; /* hardware access lock */
>> unsigned int mode;
>> @@ -142,6 +163,7 @@ struct mxc_jpeg_dev {
>> struct device **pd_dev;
>> struct device_link **pd_link;
>> struct gen_pool *sram_pool;
>> + const struct mxc_jpeg_enc_ops *enc_cfg_ops;
>> };
>>
>> /**
>>
>> base-commit: c824345288d11e269ce41b36c105715bc2286050
>> prerequisite-patch-id: 0000000000000000000000000000000000000000
>> --
>> 2.52.0
>>
Powered by blists - more mailing lists