linux-kernel - Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250405153732.sitanh6tucpzvkoe@basti-XPS-13-9310>
Date: Sat, 5 Apr 2025 17:37:32 +0200
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: ming.qian@....nxp.com
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl,
	mirela.rabulea@....nxp.com, 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 3/3] media: imx-jpeg: Check decoding is ongoing for
 motion-jpeg

Hey Ming,

On 28.03.2025 14:30, ming.qian@....nxp.com wrote:
>From: Ming Qian <ming.qian@....nxp.com>
>
>To support decoding motion-jpeg without DHT, driver will try to decode a
>pattern jpeg before actual jpeg frame by use of linked descriptors
>(This is called "repeat mode"), then the DHT in the pattern jpeg can be
>used for decoding the motion-jpeg.

Hmm do we need to repeat the description from the previous patch?
>
>In other words, 2 frame done interrupts will be triggered, driver will
>ignore the first interrupt, and wait for the second interrupt.
>If the resolution is small, and the 2 interrupts may be too close,
>when driver is handling the first interrupt, two frames are done, then
>driver will fail to wait for the second interrupt.

Okay this first part is a bit hard to understand, how about:

As the first frame in "repeat-mode" is the pattern, the first interrupt
is ignored by the driver. With small resolution bitstreams, the
interrupts might fire too quickly and thus the driver might miss the
second interrupt from the first actual frame.

Is that what you mean?

>
>In such case, driver can check whether the decoding is still ongoing,
>if not, just done the current decoding.

That doesn't answer to me why this solves the issue of missing the
second interrupt, can you elaborate your solution a bit so that the
reader of the commit description understands why this is needed?

Regards,
Sebastian

>
>Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>---
> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
>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 d579c804b047..adb93e977be9 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>@@ -89,6 +89,7 @@
> /* SLOT_STATUS fields for slots 0..3 */
> #define SLOT_STATUS_FRMDONE			(0x1 << 3)
> #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>+#define SLOT_STATUS_ONGOING			(0x1 << 31)
>
> /* SLOT_IRQ_EN fields TBD */
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>index 45705c606769..e6bb45633a19 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>@@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
> 	return size;
> }
>
>+static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>+{
>+	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>+	u32 curr_desc;
>+	u32 slot_status;
>+
>+	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>+	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>+
>+	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>+		return true;
>+	if (slot_status & SLOT_STATUS_ONGOING)
>+		return true;
>+
>+	return false;
>+}
>+
> static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> {
> 	struct mxc_jpeg_dev *jpeg = priv;
>@@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
> 		goto job_unlock;
> 	}
>-	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>+	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>+	    mxc_dec_is_ongoing(ctx)) {
> 		jpeg_src_buf->dht_needed = false;
> 		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
> 		goto job_unlock;
>-- 
>2.43.0-rc1
>
>