Message-ID: <aXzFnXmvZIcqWxZ4@lizhi-Precision-Tower-5810>
Date: Fri, 30 Jan 2026 09:52:13 -0500
From: Frank Li <Frank.li@....com>
To: ming.qian@....nxp.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 v2] media: imx-jpeg: Add support for encoder v1
 descriptor configuration

On Fri, Jan 30, 2026 at 02:22:33PM +0800, ming.qian@....nxp.com wrote:
> From: Ming Qian <ming.qian@....nxp.com>
>
> 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.
>
> 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
>
> Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>
> ---
> v2
> - Improve commit message
> - Use GENMASK_U32
> - make mxc_jpeg_get_version() static
> - Check version in probe()
> - Remove noise that update copyright years
> ---
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |   1 +
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 104 +++++++++++++++---
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  22 ++++
>  3 files changed, 113 insertions(+), 14 deletions(-)
>
> 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..0d78443cb270 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/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(GENMASK_U32(19, 16), r)
>
>  /* COM_STAUS fields */
>  #define COM_STATUS_DEC_ONGOING(r)		(((r) & (1 << 31)) >> 31)
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index b558700d1d96..71f4a1d292ac 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -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;

this change and memset belong code cleanup, it'd better use seperate patch.

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

If I do this, I prefer mxc_jpeg_enc_start_config_manually() and
mxc_jpeg_enc_finish_config_manually() as patch, just do code re-org.

Then base that, add mxc_jpeg_enc_configure_desc() will straight forward.

Some maintainer accept this if change is not bigger.  The squash patches
is trivials by maintainers.

Frank
> +
> +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);
> @@ -2842,6 +2895,14 @@ static int mxc_jpeg_attach_pm_domains(struct mxc_jpeg_dev *jpeg)
>  	return ret;
>  }
>
> +static int mxc_jpeg_get_version(void __iomem *reg)
> +{
> +	u32 regval;
> +
> +	regval = readl(reg + GLB_CTRL);
> +	return GLB_CTRL_CUR_VERSION(regval);
> +}
> +
>  static int mxc_jpeg_probe(struct platform_device *pdev)
>  {
>  	struct mxc_jpeg_dev *jpeg;
> @@ -2976,8 +3037,23 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, jpeg);
>  	pm_runtime_enable(dev);
>
> +	if (mode == MXC_JPEG_ENCODE) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0)
> +			goto err_check_version;
> +
> +		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;
> +
> +		pm_runtime_put_sync(dev);
> +	}
> +
>  	return 0;
>
> +err_check_version:
> +	pm_runtime_disable(&pdev->dev);
>  err_vdev_register:
>  	video_device_release(jpeg->dec_vdev);
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 9c5b4f053ded..c00c13549746 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.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;
>  } __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);
> +};
> +
>  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