devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: imx-jpeg: Correct some definition according specification
@ 2022-05-27  7:54 Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Disable some unused interrupt Ming Qian
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Ming Qian @ 2022-05-27  7:54 UTC (permalink / raw)
  To: mchehab, mirela.rabulea, hverkuil-cisco
  Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
	linux-kernel, devicetree, linux-arm-kernel

the register CAST_NOMFRSIZE_LO should be equal to CAST_STATUS16
the register CAST_NOMFRSIZE_HI should be equal to CAST_STATUS17
the register CAST_OFBSIZE_LO should be equal to CAST_STATUS18
the register CAST_OFBSIZE_HI should be equal to CAST_STATUS19

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 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 e7e8954754b1..07655502f4bd 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
@@ -53,10 +53,10 @@
 #define CAST_REC_REGS_SEL		CAST_STATUS4
 #define CAST_LUMTH			CAST_STATUS5
 #define CAST_CHRTH			CAST_STATUS6
-#define CAST_NOMFRSIZE_LO		CAST_STATUS7
-#define CAST_NOMFRSIZE_HI		CAST_STATUS8
-#define CAST_OFBSIZE_LO			CAST_STATUS9
-#define CAST_OFBSIZE_HI			CAST_STATUS10
+#define CAST_NOMFRSIZE_LO		CAST_STATUS16
+#define CAST_NOMFRSIZE_HI		CAST_STATUS17
+#define CAST_OFBSIZE_LO			CAST_STATUS18
+#define CAST_OFBSIZE_HI			CAST_STATUS19
 
 #define MXC_MAX_SLOTS	1 /* TODO use all 4 slots*/
 /* JPEG-Decoder Wrapper Slot Registers 0..3 */
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] media: imx-jpeg: Disable some unused interrupt
  2022-05-27  7:54 [PATCH] media: imx-jpeg: Correct some definition according specification Ming Qian
@ 2022-05-27  7:54 ` Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Leave a blank space before the configuration data Ming Qian
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Ming Qian @ 2022-05-27  7:54 UTC (permalink / raw)
  To: mchehab, mirela.rabulea, hverkuil-cisco
  Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
	linux-kernel, devicetree, linux-arm-kernel

The interrupt STMBUF_HALF may be triggered after frame done.
It may led to system hang if driver try to access the register after
power off.
And interrupt STMBUF_HALF and STMBUF_RTND have no other effect.
So disable them and the unused interrupts.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 c482228262a3..258fbee7ab66 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
@@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void __iomem *reg)
 
 void mxc_jpeg_enable_irq(void __iomem *reg, int slot)
 {
-	writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN));
+	writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN));
 }
 
 void mxc_jpeg_sw_reset(void __iomem *reg)
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] media: imx-jpeg: Leave a blank space before the configuration data
  2022-05-27  7:54 [PATCH] media: imx-jpeg: Correct some definition according specification Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Disable some unused interrupt Ming Qian
@ 2022-05-27  7:54 ` Ming Qian
  2022-05-27  9:38   ` Tommaso Merciai
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Align upwards buffer size Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Implement drain using v4l2-mem2mem helpers Ming Qian
  3 siblings, 1 reply; 9+ messages in thread
From: Ming Qian @ 2022-05-27  7:54 UTC (permalink / raw)
  To: mchehab, mirela.rabulea, hverkuil-cisco
  Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
	linux-kernel, devicetree, linux-arm-kernel

There is a hardware bug that it will load
the first 128 bytes of configuration data twice,
it will led to some configure error.
so shift the configuration data 128 bytes,
and make the first 128 bytes all zero,
then hardware will load the 128 zero twice,
and ignore them as garbage.
then the configuration data can be loaded correctly

Signed-off-by: Ming Qian <ming.qian@nxp.com>
Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 734e1b65fbc7..ad4213e020f3 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
 				     GFP_ATOMIC);
 	if (!cfg_stm)
 		goto err;
+	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
 	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
 
 skip_alloc:
@@ -755,7 +756,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
 					      u32 fourcc,
 					      u16 w, u16 h)
 {
-	unsigned int offset = 0;
+	unsigned int offset = 0x80;
 	u8 *cfg = (u8 *)cfg_stream_vaddr;
 	struct mxc_jpeg_sof *sof;
 	struct mxc_jpeg_sos *sos;
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] media: imx-jpeg: Align upwards buffer size
  2022-05-27  7:54 [PATCH] media: imx-jpeg: Correct some definition according specification Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Disable some unused interrupt Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Leave a blank space before the configuration data Ming Qian
@ 2022-05-27  7:54 ` Ming Qian
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Implement drain using v4l2-mem2mem helpers Ming Qian
  3 siblings, 0 replies; 9+ messages in thread
From: Ming Qian @ 2022-05-27  7:54 UTC (permalink / raw)
  To: mchehab, mirela.rabulea, hverkuil-cisco
  Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
	linux-kernel, devicetree, linux-arm-kernel

The hardware can support any image size WxH,
with arbitrary W (image width) and H (image height) dimensions.

Align upwards buffer size for both encoder and decoder.
and leave the picture resolution unchanged.

For decoder, the risk of memory out of bounds can be avoided.
For both encoder and decoder, the driver will lift the limitation of
resolution alignment.

For example, the decoder can support jpeg whose resolution is 227x149
the encoder can support nv12 1080P, won't change it to 1920x1072.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 88 ++++++++-----------
 1 file changed, 37 insertions(+), 51 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index ad4213e020f3..9719eab7bc83 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -888,8 +888,8 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
 	jpeg->slot_data[slot].cfg_stream_size =
 			mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
 						  q_data->fmt->fourcc,
-						  q_data->w_adjusted,
-						  q_data->h_adjusted);
+						  q_data->w,
+						  q_data->h);
 
 	/* chain the config descriptor with the encoding descriptor */
 	cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
@@ -971,7 +971,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
 				      &q_data_cap->h_adjusted,
 				      q_data_cap->h_adjusted, /* adjust up */
 				      MXC_JPEG_MAX_HEIGHT,
-				      q_data_cap->fmt->v_align,
+				      0,
 				      0);
 
 		/* setup bytesperline/sizeimage for capture queue */
@@ -1155,18 +1155,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
 {
 	struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
 	struct mxc_jpeg_q_data *q_data = NULL;
+	struct mxc_jpeg_q_data tmp_q;
 	int i;
 
 	q_data = mxc_jpeg_get_q_data(ctx, q->type);
 	if (!q_data)
 		return -EINVAL;
 
+	tmp_q.fmt = q_data->fmt;
+	tmp_q.w = q_data->w_adjusted;
+	tmp_q.h = q_data->h_adjusted;
+	for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
+		tmp_q.bytesperline[i] = q_data->bytesperline[i];
+		tmp_q.sizeimage[i] = q_data->sizeimage[i];
+	}
+	mxc_jpeg_sizeimage(&tmp_q);
+	for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
+		tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
+
 	/* Handle CREATE_BUFS situation - *nplanes != 0 */
 	if (*nplanes) {
 		if (*nplanes != q_data->fmt->colplanes)
 			return -EINVAL;
 		for (i = 0; i < *nplanes; i++) {
-			if (sizes[i] < q_data->sizeimage[i])
+			if (sizes[i] < tmp_q.sizeimage[i])
 				return -EINVAL;
 		}
 		return 0;
@@ -1175,7 +1187,7 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
 	/* Handle REQBUFS situation */
 	*nplanes = q_data->fmt->colplanes;
 	for (i = 0; i < *nplanes; i++)
-		sizes[i] = q_data->sizeimage[i];
+		sizes[i] = tmp_q.sizeimage[i];
 
 	return 0;
 }
@@ -1375,11 +1387,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
 	}
 	q_data_out->w = header.frame.width;
 	q_data_out->h = header.frame.height;
-	if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
-		dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
-			header.frame.width, header.frame.height);
-		return -EINVAL;
-	}
 	if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
 	    header.frame.height > MXC_JPEG_MAX_HEIGHT) {
 		dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
@@ -1742,22 +1749,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fm
 	pix_mp->num_planes = fmt->colplanes;
 	pix_mp->pixelformat = fmt->fourcc;
 
-	/*
-	 * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
-	 * alignment, to loosen up the alignment to multiple of 8,
-	 * otherwise NV12-1080p fails as 1080 is not a multiple of 16
-	 */
+	pix_mp->width = w;
+	pix_mp->height = h;
 	v4l_bound_align_image(&w,
-			      MXC_JPEG_MIN_WIDTH,
-			      w, /* adjust downwards*/
+			      w, /* adjust upwards*/
+			      MXC_JPEG_MAX_WIDTH,
 			      fmt->h_align,
 			      &h,
-			      MXC_JPEG_MIN_HEIGHT,
-			      h, /* adjust downwards*/
-			      MXC_JPEG_H_ALIGN,
+			      h, /* adjust upwards*/
+			      MXC_JPEG_MAX_HEIGHT,
+			      0,
 			      0);
-	pix_mp->width = w; /* negotiate the width */
-	pix_mp->height = h; /* negotiate the height */
 
 	/* get user input into the tmp_q */
 	tmp_q.w = w;
@@ -1883,35 +1885,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx *ctx,
 
 	q_data->w_adjusted = q_data->w;
 	q_data->h_adjusted = q_data->h;
-	if (jpeg->mode == MXC_JPEG_DECODE) {
-		/*
-		 * align up the resolution for CAST IP,
-		 * but leave the buffer resolution unchanged
-		 */
-		v4l_bound_align_image(&q_data->w_adjusted,
-				      q_data->w_adjusted,  /* adjust upwards */
-				      MXC_JPEG_MAX_WIDTH,
-				      q_data->fmt->h_align,
-				      &q_data->h_adjusted,
-				      q_data->h_adjusted, /* adjust upwards */
-				      MXC_JPEG_MAX_HEIGHT,
-				      q_data->fmt->v_align,
-				      0);
-	} else {
-		/*
-		 * align down the resolution for CAST IP,
-		 * but leave the buffer resolution unchanged
-		 */
-		v4l_bound_align_image(&q_data->w_adjusted,
-				      MXC_JPEG_MIN_WIDTH,
-				      q_data->w_adjusted, /* adjust downwards*/
-				      q_data->fmt->h_align,
-				      &q_data->h_adjusted,
-				      MXC_JPEG_MIN_HEIGHT,
-				      q_data->h_adjusted, /* adjust downwards*/
-				      q_data->fmt->v_align,
-				      0);
-	}
+	/*
+	 * align up the resolution for CAST IP,
+	 * but leave the buffer resolution unchanged
+	 */
+	v4l_bound_align_image(&q_data->w_adjusted,
+			      q_data->w_adjusted,  /* adjust upwards */
+			      MXC_JPEG_MAX_WIDTH,
+			      q_data->fmt->h_align,
+			      &q_data->h_adjusted,
+			      q_data->h_adjusted, /* adjust upwards */
+			      MXC_JPEG_MAX_HEIGHT,
+			      q_data->fmt->v_align,
+			      0);
 
 	for (i = 0; i < pix_mp->num_planes; i++) {
 		q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] media: imx-jpeg: Implement drain using v4l2-mem2mem helpers
  2022-05-27  7:54 [PATCH] media: imx-jpeg: Correct some definition according specification Ming Qian
                   ` (2 preceding siblings ...)
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Align upwards buffer size Ming Qian
@ 2022-05-27  7:54 ` Ming Qian
  3 siblings, 0 replies; 9+ messages in thread
From: Ming Qian @ 2022-05-27  7:54 UTC (permalink / raw)
  To: mchehab, mirela.rabulea, hverkuil-cisco
  Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
	linux-kernel, devicetree, linux-arm-kernel

v4l2 m2m has supplied some helper function to handle drain,
so the driver can use the helper function directly.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 155 +++++++++---------
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |   2 -
 2 files changed, 73 insertions(+), 84 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 9719eab7bc83..521560c6ba83 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -558,6 +558,18 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg,
 	jpeg->slot_data[slot].used = false;
 }
 
+static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
+					       struct vb2_v4l2_buffer *src_buf,
+					       struct vb2_v4l2_buffer *dst_buf)
+{
+	if (v4l2_m2m_is_last_draining_src_buf(ctx->fh.m2m_ctx, src_buf)) {
+		dst_buf->flags |= V4L2_BUF_FLAG_LAST;
+		v4l2_m2m_mark_stopped(ctx->fh.m2m_ctx);
+		notify_eos(ctx);
+		ctx->header_parsed = false;
+	}
+}
+
 static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 {
 	struct mxc_jpeg_dev *jpeg = priv;
@@ -633,6 +645,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
 		goto job_unlock;
 	}
+
 	if (jpeg->mode == MXC_JPEG_ENCODE) {
 		payload = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_BUF_PTR));
 		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, payload);
@@ -661,6 +674,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 
 buffers_done:
 	jpeg->slot_data[slot].used = false; /* unused, but don't free */
+	mxc_jpeg_check_and_set_last_buffer(ctx, src_buf, dst_buf);
 	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
 	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
 	v4l2_m2m_buf_done(src_buf, buf_state);
@@ -1028,6 +1042,7 @@ static void mxc_jpeg_device_run(void *priv)
 		jpeg_src_buf->jpeg_parse_error = true;
 	}
 	if (jpeg_src_buf->jpeg_parse_error) {
+		mxc_jpeg_check_and_set_last_buffer(ctx, src_buf, dst_buf);
 		v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
 		v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
 		v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
@@ -1078,45 +1093,33 @@ static void mxc_jpeg_device_run(void *priv)
 	spin_unlock_irqrestore(&ctx->mxc_jpeg->hw_lock, flags);
 }
 
-static void mxc_jpeg_set_last_buffer_dequeued(struct mxc_jpeg_ctx *ctx)
-{
-	struct vb2_queue *q;
-
-	ctx->stopped = 1;
-	q = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx);
-	if (!list_empty(&q->done_list))
-		return;
-
-	q->last_buffer_dequeued = true;
-	wake_up(&q->done_wq);
-	ctx->stopped = 0;
-	ctx->header_parsed = false;
-}
-
 static int mxc_jpeg_decoder_cmd(struct file *file, void *priv,
 				struct v4l2_decoder_cmd *cmd)
 {
 	struct v4l2_fh *fh = file->private_data;
 	struct mxc_jpeg_ctx *ctx = mxc_jpeg_fh_to_ctx(fh);
-	struct device *dev = ctx->mxc_jpeg->dev;
 	int ret;
 
 	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, cmd);
 	if (ret < 0)
 		return ret;
 
-	if (cmd->cmd == V4L2_DEC_CMD_STOP) {
-		dev_dbg(dev, "Received V4L2_DEC_CMD_STOP");
-		if (v4l2_m2m_num_src_bufs_ready(fh->m2m_ctx) == 0) {
-			/* No more src bufs, notify app EOS */
-			notify_eos(ctx);
-			mxc_jpeg_set_last_buffer_dequeued(ctx);
-		} else {
-			/* will send EOS later*/
-			ctx->stopping = 1;
-		}
+	if (!vb2_is_streaming(v4l2_m2m_get_src_vq(fh->m2m_ctx)))
+		return 0;
+
+	ret = v4l2_m2m_ioctl_decoder_cmd(file, priv, cmd);
+	if (ret < 0)
+		return ret;
+
+	if (cmd->cmd == V4L2_DEC_CMD_STOP &&
+	    v4l2_m2m_has_stopped(fh->m2m_ctx)) {
+		notify_eos(ctx);
+		ctx->header_parsed = false;
 	}
 
+	if (cmd->cmd == V4L2_DEC_CMD_START &&
+	    v4l2_m2m_has_stopped(fh->m2m_ctx))
+		vb2_clear_last_buffer_dequeued(&fh->m2m_ctx->cap_q_ctx.q);
 	return 0;
 }
 
@@ -1125,24 +1128,27 @@ static int mxc_jpeg_encoder_cmd(struct file *file, void *priv,
 {
 	struct v4l2_fh *fh = file->private_data;
 	struct mxc_jpeg_ctx *ctx = mxc_jpeg_fh_to_ctx(fh);
-	struct device *dev = ctx->mxc_jpeg->dev;
 	int ret;
 
 	ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd);
 	if (ret < 0)
 		return ret;
 
-	if (cmd->cmd == V4L2_ENC_CMD_STOP) {
-		dev_dbg(dev, "Received V4L2_ENC_CMD_STOP");
-		if (v4l2_m2m_num_src_bufs_ready(fh->m2m_ctx) == 0) {
-			/* No more src bufs, notify app EOS */
-			notify_eos(ctx);
-			mxc_jpeg_set_last_buffer_dequeued(ctx);
-		} else {
-			/* will send EOS later*/
-			ctx->stopping = 1;
-		}
-	}
+	if (!vb2_is_streaming(v4l2_m2m_get_src_vq(fh->m2m_ctx)) ||
+	    !vb2_is_streaming(v4l2_m2m_get_dst_vq(fh->m2m_ctx)))
+		return 0;
+
+	ret = v4l2_m2m_ioctl_encoder_cmd(file, fh, cmd);
+	if (ret < 0)
+		return 0;
+
+	if (cmd->cmd == V4L2_ENC_CMD_STOP &&
+	    v4l2_m2m_has_stopped(fh->m2m_ctx))
+		notify_eos(ctx);
+
+	if (cmd->cmd == V4L2_ENC_CMD_START &&
+	    v4l2_m2m_has_stopped(fh->m2m_ctx))
+		vb2_clear_last_buffer_dequeued(&fh->m2m_ctx->cap_q_ctx.q);
 
 	return 0;
 }
@@ -1198,6 +1204,8 @@ static int mxc_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct mxc_jpeg_q_data *q_data = mxc_jpeg_get_q_data(ctx, q->type);
 	int ret;
 
+	v4l2_m2m_update_start_streaming_state(ctx->fh.m2m_ctx, q);
+
 	if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE && V4L2_TYPE_IS_CAPTURE(q->type))
 		ctx->source_change = 0;
 	dev_dbg(ctx->mxc_jpeg->dev, "Start streaming ctx=%p", ctx);
@@ -1229,11 +1237,15 @@ static void mxc_jpeg_stop_streaming(struct vb2_queue *q)
 			break;
 		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
 	}
-	pm_runtime_put_sync(&ctx->mxc_jpeg->pdev->dev);
-	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
-		ctx->stopping = 0;
-		ctx->stopped = 0;
+
+	v4l2_m2m_update_stop_streaming_state(ctx->fh.m2m_ctx, q);
+	if (V4L2_TYPE_IS_OUTPUT(q->type) &&
+	    v4l2_m2m_has_stopped(ctx->fh.m2m_ctx)) {
+		notify_eos(ctx);
+		ctx->header_parsed = false;
 	}
+
+	pm_runtime_put_sync(&ctx->mxc_jpeg->pdev->dev);
 }
 
 static int mxc_jpeg_valid_comp_id(struct device *dev,
@@ -1432,6 +1444,20 @@ static void mxc_jpeg_buf_queue(struct vb2_buffer *vb)
 	struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
 	struct mxc_jpeg_src_buf *jpeg_src_buf;
 
+	if (V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) &&
+	    vb2_is_streaming(vb->vb2_queue) &&
+	    v4l2_m2m_dst_buf_is_last(ctx->fh.m2m_ctx)) {
+		struct mxc_jpeg_q_data *q_data;
+
+		q_data = mxc_jpeg_get_q_data(ctx, vb->vb2_queue->type);
+		vbuf->field = V4L2_FIELD_NONE;
+		vbuf->sequence = q_data->sequence++;
+		v4l2_m2m_last_buffer_done(ctx->fh.m2m_ctx, vbuf);
+		notify_eos(ctx);
+		ctx->header_parsed = false;
+		return;
+	}
+
 	if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
 		goto end;
 
@@ -1480,24 +1506,11 @@ static int mxc_jpeg_buf_prepare(struct vb2_buffer *vb)
 			return -EINVAL;
 		}
 	}
-	return 0;
-}
-
-static void mxc_jpeg_buf_finish(struct vb2_buffer *vb)
-{
-	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
-	struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
-	struct vb2_queue *q = vb->vb2_queue;
-
-	if (V4L2_TYPE_IS_OUTPUT(vb->type))
-		return;
-	if (!ctx->stopped)
-		return;
-	if (list_empty(&q->done_list)) {
-		vbuf->flags |= V4L2_BUF_FLAG_LAST;
-		ctx->stopped = 0;
-		ctx->header_parsed = false;
+	if (V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type)) {
+		vb2_set_plane_payload(vb, 0, 0);
+		vb2_set_plane_payload(vb, 1, 0);
 	}
+	return 0;
 }
 
 static const struct vb2_ops mxc_jpeg_qops = {
@@ -1506,7 +1519,6 @@ static const struct vb2_ops mxc_jpeg_qops = {
 	.wait_finish		= vb2_ops_wait_finish,
 	.buf_out_validate	= mxc_jpeg_buf_out_validate,
 	.buf_prepare		= mxc_jpeg_buf_prepare,
-	.buf_finish             = mxc_jpeg_buf_finish,
 	.start_streaming	= mxc_jpeg_start_streaming,
 	.stop_streaming		= mxc_jpeg_stop_streaming,
 	.buf_queue		= mxc_jpeg_buf_queue,
@@ -2009,27 +2021,6 @@ static int mxc_jpeg_subscribe_event(struct v4l2_fh *fh,
 	}
 }
 
-static int mxc_jpeg_dqbuf(struct file *file, void *priv,
-			  struct v4l2_buffer *buf)
-{
-	struct v4l2_fh *fh = file->private_data;
-	struct mxc_jpeg_ctx *ctx = mxc_jpeg_fh_to_ctx(priv);
-	struct device *dev = ctx->mxc_jpeg->dev;
-	int num_src_ready = v4l2_m2m_num_src_bufs_ready(fh->m2m_ctx);
-	int ret;
-
-	dev_dbg(dev, "DQBUF type=%d, index=%d", buf->type, buf->index);
-	if (ctx->stopping == 1 && num_src_ready == 0) {
-		/* No more src bufs, notify app EOS */
-		notify_eos(ctx);
-		ctx->stopping = 0;
-		mxc_jpeg_set_last_buffer_dequeued(ctx);
-	}
-
-	ret = v4l2_m2m_dqbuf(file, fh->m2m_ctx, buf);
-	return ret;
-}
-
 static const struct v4l2_ioctl_ops mxc_jpeg_ioctl_ops = {
 	.vidioc_querycap		= mxc_jpeg_querycap,
 	.vidioc_enum_fmt_vid_cap	= mxc_jpeg_enum_fmt_vid_cap,
@@ -2053,7 +2044,7 @@ static const struct v4l2_ioctl_ops mxc_jpeg_ioctl_ops = {
 	.vidioc_encoder_cmd		= mxc_jpeg_encoder_cmd,
 
 	.vidioc_qbuf			= v4l2_m2m_ioctl_qbuf,
-	.vidioc_dqbuf			= mxc_jpeg_dqbuf,
+	.vidioc_dqbuf			= v4l2_m2m_ioctl_dqbuf,
 
 	.vidioc_create_bufs		= v4l2_m2m_ioctl_create_bufs,
 	.vidioc_prepare_buf		= v4l2_m2m_ioctl_prepare_buf,
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
index d439be6e4acf..c508d41a906f 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
@@ -92,8 +92,6 @@ struct mxc_jpeg_ctx {
 	struct mxc_jpeg_q_data		cap_q;
 	struct v4l2_fh			fh;
 	enum mxc_jpeg_enc_state		enc_state;
-	unsigned int			stopping;
-	unsigned int			stopped;
 	unsigned int			slot;
 	unsigned int			source_change;
 	bool				header_parsed;
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: imx-jpeg: Leave a blank space before the configuration data
  2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Leave a blank space before the configuration data Ming Qian
@ 2022-05-27  9:38   ` Tommaso Merciai
  2022-05-27  9:52     ` [EXT] " Ming Qian
  0 siblings, 1 reply; 9+ messages in thread
From: Tommaso Merciai @ 2022-05-27  9:38 UTC (permalink / raw)
  To: Ming Qian
  Cc: mchehab, mirela.rabulea, hverkuil-cisco, shawnguo, s.hauer,
	kernel, festevam, linux-imx, linux-media, linux-kernel,
	devicetree, linux-arm-kernel

Hi Ming,
I think have some comments on the code for this would be nice for the
future

On Fri, May 27, 2022 at 03:54:35PM +0800, Ming Qian wrote:
> There is a hardware bug that it will load
> the first 128 bytes of configuration data twice,
> it will led to some configure error.
> so shift the configuration data 128 bytes,
> and make the first 128 bytes all zero,
> then hardware will load the 128 zero twice,

From what I've understood you initialize cfg_stm with zeros then
you start to write the configuration from 0x80 (128 bytes), avoiding the hw issue right?

> and ignore them as garbage.
> then the configuration data can be loaded correctly
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 734e1b65fbc7..ad4213e020f3 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
>  				     GFP_ATOMIC);
>  	if (!cfg_stm)
>  		goto err;
> +	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
>  	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
>  
>  skip_alloc:
> @@ -755,7 +756,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
>  					      u32 fourcc,
>  					      u16 w, u16 h)
>  {
> -	unsigned int offset = 0;
> +	unsigned int offset = 0x80;
>  	u8 *cfg = (u8 *)cfg_stream_vaddr;
>  	struct mxc_jpeg_sof *sof;
>  	struct mxc_jpeg_sos *sos;
> -- 
> 2.36.1
> 

Thanks,
Tommaso

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the configuration data
  2022-05-27  9:38   ` Tommaso Merciai
@ 2022-05-27  9:52     ` Ming Qian
  2022-05-27 10:05       ` Tommaso Merciai
  0 siblings, 1 reply; 9+ messages in thread
From: Ming Qian @ 2022-05-27  9:52 UTC (permalink / raw)
  To: Tommaso Merciai
  Cc: mchehab, Mirela Rabulea (OSS),
	hverkuil-cisco, shawnguo, s.hauer, kernel, festevam,
	dl-linux-imx, linux-media, linux-kernel, devicetree,
	linux-arm-kernel

> From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Sent: 2022年5月27日 17:38
> To: Ming Qian <ming.qian@nxp.com>
> Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> Hi Ming,
> I think have some comments on the code for this would be nice for the future
> 
> On Fri, May 27, 2022 at 03:54:35PM +0800, Ming Qian wrote:
> > There is a hardware bug that it will load the first 128 bytes of
> > configuration data twice, it will led to some configure error.
> > so shift the configuration data 128 bytes, and make the first 128
> > bytes all zero, then hardware will load the 128 zero twice,
> 
> From what I've understood you initialize cfg_stm with zeros then you start to
> write the configuration from 0x80 (128 bytes), avoiding the hw issue right?
> 

HI Tommaso,
    Yes, you're right, there is a hardware bug.
I want to write the configuration data from the offset 0x80,
And set the first 128 bytes to zero.
Then the hardware bug can be avoided.

Ming.


> > and ignore them as garbage.
> > then the configuration data can be loaded correctly
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > ---
> >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > index 734e1b65fbc7..ad4213e020f3 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct
> mxc_jpeg_dev *jpeg,
> >                                    GFP_ATOMIC);
> >       if (!cfg_stm)
> >               goto err;
> > +     memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
> >       jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
> >
> >  skip_alloc:
> > @@ -755,7 +756,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void
> *cfg_stream_vaddr,
> >                                             u32 fourcc,
> >                                             u16 w, u16 h)  {
> > -     unsigned int offset = 0;
> > +     unsigned int offset = 0x80;
> >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> >       struct mxc_jpeg_sof *sof;
> >       struct mxc_jpeg_sos *sos;
> > --
> > 2.36.1
> >
> 
> Thanks,
> Tommaso
> 
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> info@amarulasolutions.com
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> OJzhFaZlow%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the configuration data
  2022-05-27  9:52     ` [EXT] " Ming Qian
@ 2022-05-27 10:05       ` Tommaso Merciai
  2022-05-27 10:15         ` Ming Qian
  0 siblings, 1 reply; 9+ messages in thread
From: Tommaso Merciai @ 2022-05-27 10:05 UTC (permalink / raw)
  To: Ming Qian
  Cc: mchehab, Mirela Rabulea (OSS),
	hverkuil-cisco, shawnguo, s.hauer, kernel, festevam,
	dl-linux-imx, linux-media, linux-kernel, devicetree,
	linux-arm-kernel

On Fri, May 27, 2022 at 09:52:48AM +0000, Ming Qian wrote:
> > From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > Sent: 2022年5月27日 17:38
> > To: Ming Qian <ming.qian@nxp.com>
> > Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> > <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> > configuration data
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > I think have some comments on the code for this would be nice for the future
> > 
> > On Fri, May 27, 2022 at 03:54:35PM +0800, Ming Qian wrote:
> > > There is a hardware bug that it will load the first 128 bytes of
> > > configuration data twice, it will led to some configure error.
> > > so shift the configuration data 128 bytes, and make the first 128
> > > bytes all zero, then hardware will load the 128 zero twice,
> > 
> > From what I've understood you initialize cfg_stm with zeros then you start to
> > write the configuration from 0x80 (128 bytes), avoiding the hw issue right?
> > 
> 

Hi Ming,

> HI Tommaso,
>     Yes, you're right, there is a hardware bug.
> I want to write the configuration data from the offset 0x80,
> And set the first 128 bytes to zero.
> Then the hardware bug can be avoided.

Thanks for your clarification!
Unfortunately I can't test this on a real board but the implementation looks
good to me. Only thing is missing I think we need some notes to keep
track of this known issue (for the sake of clarity just a note on why we
start to write from 0x80)

Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

Thanks,
Tommaso

> 
> Ming.
> 
> 
> > > and ignore them as garbage.
> > > then the configuration data can be loaded correctly
> > >
> > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > ---
> > >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > index 734e1b65fbc7..ad4213e020f3 100644
> > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct
> > mxc_jpeg_dev *jpeg,
> > >                                    GFP_ATOMIC);
> > >       if (!cfg_stm)
> > >               goto err;
> > > +     memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
> > >       jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
> > >
> > >  skip_alloc:
> > > @@ -755,7 +756,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void
> > *cfg_stream_vaddr,
> > >                                             u32 fourcc,
> > >                                             u16 w, u16 h)  {
> > > -     unsigned int offset = 0;
> > > +     unsigned int offset = 0x80;
> > >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> > >       struct mxc_jpeg_sof *sof;
> > >       struct mxc_jpeg_sos *sos;
> > > --
> > > 2.36.1
> > >
> > 
> > Thanks,
> > Tommaso
> > 
> > --
> > Tommaso Merciai
> > Embedded Linux Engineer
> > tommaso.merciai@amarulasolutions.com
> > __________________________________
> > 
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> > info@amarulasolutions.com
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> > arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> > d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> > C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> > OJzhFaZlow%3D&amp;reserved=0

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the configuration data
  2022-05-27 10:05       ` Tommaso Merciai
@ 2022-05-27 10:15         ` Ming Qian
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Qian @ 2022-05-27 10:15 UTC (permalink / raw)
  To: Tommaso Merciai
  Cc: mchehab, Mirela Rabulea (OSS),
	hverkuil-cisco, shawnguo, s.hauer, kernel, festevam,
	dl-linux-imx, linux-media, linux-kernel, devicetree,
	linux-arm-kernel

> From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Sent: 2022年5月27日 18:06
> To: Ming Qian <ming.qian@nxp.com>
> Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> On Fri, May 27, 2022 at 09:52:48AM +0000, Ming Qian wrote:
> > > From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > Sent: 2022年5月27日 17:38
> > > To: Ming Qian <ming.qian@nxp.com>
> > > Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> > > <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space
> > > before the configuration data
> > >
> > > Caution: EXT Email
> > >
> > > Hi Ming,
> > > I think have some comments on the code for this would be nice for
> > > the future
> > >
> > > On Fri, May 27, 2022 at 03:54:35PM +0800, Ming Qian wrote:
> > > > There is a hardware bug that it will load the first 128 bytes of
> > > > configuration data twice, it will led to some configure error.
> > > > so shift the configuration data 128 bytes, and make the first 128
> > > > bytes all zero, then hardware will load the 128 zero twice,
> > >
> > > From what I've understood you initialize cfg_stm with zeros then you
> > > start to write the configuration from 0x80 (128 bytes), avoiding the hw
> issue right?
> > >
> >
> 
> Hi Ming,
> 
> > HI Tommaso,
> >     Yes, you're right, there is a hardware bug.
> > I want to write the configuration data from the offset 0x80, And set
> > the first 128 bytes to zero.
> > Then the hardware bug can be avoided.
> 
> Thanks for your clarification!
> Unfortunately I can't test this on a real board but the implementation looks
> good to me. Only thing is missing I think we need some notes to keep track of
> this known issue (for the sake of clarity just a note on why we start to write
> from 0x80)
> 
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> 
> Thanks,
> Tommaso
> 

I'll add some comments in code, thanks for your suggestion

> >
> > Ming.
> >
> >
> > > > and ignore them as garbage.
> > > > then the configuration data can be loaded correctly
> > > >
> > > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > ---
> > > >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > index 734e1b65fbc7..ad4213e020f3 100644
> > > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct
> > > mxc_jpeg_dev *jpeg,
> > > >                                    GFP_ATOMIC);
> > > >       if (!cfg_stm)
> > > >               goto err;
> > > > +     memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
> > > >       jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
> > > >
> > > >  skip_alloc:
> > > > @@ -755,7 +756,7 @@ static unsigned int
> > > > mxc_jpeg_setup_cfg_stream(void
> > > *cfg_stream_vaddr,
> > > >                                             u32 fourcc,
> > > >                                             u16 w, u16 h)  {
> > > > -     unsigned int offset = 0;
> > > > +     unsigned int offset = 0x80;
> > > >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> > > >       struct mxc_jpeg_sof *sof;
> > > >       struct mxc_jpeg_sos *sos;
> > > > --
> > > > 2.36.1
> > > >
> > >
> > > Thanks,
> > > Tommaso
> > >
> > > --
> > > Tommaso Merciai
> > > Embedded Linux Engineer
> > > tommaso.merciai@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions SRL
> > > Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> > > info@amarulasolutions.com
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> > > .am%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C75180f2a39b
> b437bb0b6
> > >
> 08da3fc87e42%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378
> 924275
> > >
> 86457594%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzI
> > >
> iLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PDj76
> PwhSl
> > > cOyexz%2BR7ljkwx%2FEG7YTB%2BffIbz8atgFs%3D&amp;reserved=0
> > >
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> > >
> d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > >
> 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> > >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> > >
> C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> > > OJzhFaZlow%3D&amp;reserved=0
> 
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> info@amarulasolutions.com
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C7
> 5180f2a39bb437bb0b608da3fc87e42%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637892427586457594%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&amp;sdata=NW%2FGyXSBrpkfzW%2Fqdh4q9JyZ8rTX8x2
> M7r%2FMJiRH7tQ%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-27 10:15 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27  7:54 [PATCH] media: imx-jpeg: Correct some definition according specification Ming Qian
2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Disable some unused interrupt Ming Qian
2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Leave a blank space before the configuration data Ming Qian
2022-05-27  9:38   ` Tommaso Merciai
2022-05-27  9:52     ` [EXT] " Ming Qian
2022-05-27 10:05       ` Tommaso Merciai
2022-05-27 10:15         ` Ming Qian
2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Align upwards buffer size Ming Qian
2022-05-27  7:54 ` [PATCH] media: imx-jpeg: Implement drain using v4l2-mem2mem helpers Ming Qian

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).