[PATCH 0/6] media: ti: cal: misc fixes

[PATCH 0/6] media: ti: cal: misc fixes

[Tomi Valkeinen]

Hi,

Here are some fixes for CAL that I've been carrying in my streams
branch, but as they don't depend on the streams work they can be merged
separately.

 Tomi

Tomi Valkeinen (6):
  media: ti: cal: fix error paths in cal_camerarx_create()
  media: ti: cal: fix useless variable init
  media: ti: cal: rename sd_state to state
  media: ti: cal: use CSI-2 frame number for seq number
  media: ti: cal: combine wdma irq handling
  media: ti: cal: fix wdma irq for metadata

 drivers/media/platform/ti/cal/cal-camerarx.c |  46 +++---
 drivers/media/platform/ti/cal/cal-video.c    |   5 +-
 drivers/media/platform/ti/cal/cal.c          | 139 ++++++++++++++-----
 drivers/media/platform/ti/cal/cal.h          |   7 +-
 4 files changed, 136 insertions(+), 61 deletions(-)

-- 
2.25.1

[PATCH 1/6] media: ti: cal: fix error paths in cal_camerarx_create()

[Tomi Valkeinen]

The error paths are not correct: media_entity_cleanup() should not be
called unless media_entity_pads_init() has been called. Fix this.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-camerarx.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index 6b43a1525b45..a41941fa819a 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -854,7 +854,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	if (IS_ERR(phy->base)) {
 		cal_err(cal, "failed to ioremap\n");
 		ret = PTR_ERR(phy->base);
-		goto error;
+		goto err_free_phy;
 	}
 
 	cal_dbg(1, cal, "ioresource %s at %pa - %pa\n",
@@ -862,11 +862,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 
 	ret = cal_camerarx_regmap_init(cal, phy);
 	if (ret)
-		goto error;
+		goto err_free_phy;
 
 	ret = cal_camerarx_parse_dt(phy);
 	if (ret)
-		goto error;
+		goto err_free_phy;
 
 	/* Initialize the V4L2 subdev and media entity. */
 	sd = &phy->subdev;
@@ -883,20 +883,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads),
 				     phy->pads);
 	if (ret)
-		goto error;
+		goto err_free_phy;
 
 	ret = cal_camerarx_sd_init_cfg(sd, NULL);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
 	ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
 	return phy;
 
-error:
+err_entity_cleanup:
 	media_entity_cleanup(&phy->subdev.entity);
+err_free_phy:
 	kfree(phy);
 	return ERR_PTR(ret);
 }
-- 
2.25.1

[PATCH 2/6] media: ti: cal: fix useless variable init

[Tomi Valkeinen]

'ret' is initialized needlessly in cal_legacy_try_fmt_vid_cap(). We can
also move the variable to the for block, which is the only place where
it is used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
index 3e936a2ca36c..1cbd9bda1892 100644
--- a/drivers/media/platform/ti/cal/cal-video.c
+++ b/drivers/media/platform/ti/cal/cal-video.c
@@ -195,7 +195,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
 	struct cal_ctx *ctx = video_drvdata(file);
 	const struct cal_format_info *fmtinfo;
 	struct v4l2_subdev_frame_size_enum fse;
-	int ret, found;
+	int found;
 
 	fmtinfo = find_format_by_pix(ctx, f->fmt.pix.pixelformat);
 	if (!fmtinfo) {
@@ -210,12 +210,13 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
 	f->fmt.pix.field = ctx->v_fmt.fmt.pix.field;
 
 	/* check for/find a valid width/height */
-	ret = 0;
 	found = false;
 	fse.pad = 0;
 	fse.code = fmtinfo->code;
 	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	for (fse.index = 0; ; fse.index++) {
+		int ret;
+
 		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
 				       NULL, &fse);
 		if (ret)
-- 
2.25.1

[PATCH 3/6] media: ti: cal: rename sd_state to state

[Tomi Valkeinen]

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-camerarx.c | 30 ++++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index a41941fa819a..34b8542133b6 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -595,12 +595,12 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
 
 static struct v4l2_mbus_framefmt *
 cal_camerarx_get_pad_format(struct cal_camerarx *phy,
-			    struct v4l2_subdev_state *sd_state,
+			    struct v4l2_subdev_state *state,
 			    unsigned int pad, u32 which)
 {
 	switch (which) {
 	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_format(&phy->subdev, sd_state, pad);
+		return v4l2_subdev_get_try_format(&phy->subdev, state, pad);
 	case V4L2_SUBDEV_FORMAT_ACTIVE:
 		return &phy->formats[pad];
 	default:
@@ -626,7 +626,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
 }
 
 static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
-					  struct v4l2_subdev_state *sd_state,
+					  struct v4l2_subdev_state *state,
 					  struct v4l2_subdev_mbus_code_enum *code)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
@@ -643,7 +643,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
 			goto out;
 		}
 
-		fmt = cal_camerarx_get_pad_format(phy, sd_state,
+		fmt = cal_camerarx_get_pad_format(phy, state,
 						  CAL_CAMERARX_PAD_SINK,
 						  code->which);
 		code->code = fmt->code;
@@ -663,7 +663,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
 }
 
 static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
-					   struct v4l2_subdev_state *sd_state,
+					   struct v4l2_subdev_state *state,
 					   struct v4l2_subdev_frame_size_enum *fse)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
@@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 	if (cal_rx_pad_is_source(fse->pad)) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		fmt = cal_camerarx_get_pad_format(phy, sd_state,
+		fmt = cal_camerarx_get_pad_format(phy, state,
 						  CAL_CAMERARX_PAD_SINK,
 						  fse->which);
 		if (fse->code != fmt->code) {
@@ -711,7 +711,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 }
 
 static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_state *sd_state,
+				   struct v4l2_subdev_state *state,
 				   struct v4l2_subdev_format *format)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
@@ -719,7 +719,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
 
 	mutex_lock(&phy->mutex);
 
-	fmt = cal_camerarx_get_pad_format(phy, sd_state, format->pad,
+	fmt = cal_camerarx_get_pad_format(phy, state, format->pad,
 					  format->which);
 	format->format = *fmt;
 
@@ -729,7 +729,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
 }
 
 static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_state *sd_state,
+				   struct v4l2_subdev_state *state,
 				   struct v4l2_subdev_format *format)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
@@ -739,7 +739,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 
 	/* No transcoding, source and sink formats must match. */
 	if (cal_rx_pad_is_source(format->pad))
-		return cal_camerarx_sd_get_fmt(sd, sd_state, format);
+		return cal_camerarx_sd_get_fmt(sd, state, format);
 
 	/*
 	 * Default to the first format if the requested media bus code isn't
@@ -765,12 +765,12 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 
 	mutex_lock(&phy->mutex);
 
-	fmt = cal_camerarx_get_pad_format(phy, sd_state,
+	fmt = cal_camerarx_get_pad_format(phy, state,
 					  CAL_CAMERARX_PAD_SINK,
 					  format->which);
 	*fmt = format->format;
 
-	fmt = cal_camerarx_get_pad_format(phy, sd_state,
+	fmt = cal_camerarx_get_pad_format(phy, state,
 					  CAL_CAMERARX_PAD_FIRST_SOURCE,
 					  format->which);
 	*fmt = format->format;
@@ -781,10 +781,10 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 }
 
 static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
-				    struct v4l2_subdev_state *sd_state)
+				    struct v4l2_subdev_state *state)
 {
 	struct v4l2_subdev_format format = {
-		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
+		.which = state ? V4L2_SUBDEV_FORMAT_TRY
 		: V4L2_SUBDEV_FORMAT_ACTIVE,
 		.pad = CAL_CAMERARX_PAD_SINK,
 		.format = {
@@ -799,7 +799,7 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
 		},
 	};
 
-	return cal_camerarx_sd_set_fmt(sd, sd_state, &format);
+	return cal_camerarx_sd_set_fmt(sd, state, &format);
 }
 
 static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
-- 
2.25.1

[PATCH 4/6] media: ti: cal: use CSI-2 frame number for seq number

[Tomi Valkeinen]

The userspace needs a way to match received metadata buffers to pixel
data buffers. The obvious way to do this is to use the CSI-2 frame
number, as both the metadata and the pixel data have the same frame
number as they come from the same frame.

However, we don't have means to convey the frame number to userspace. We
do have the 'sequence' field, which with a few tricks can be used for
this purpose.

To achieve this, track the frame number for each virtual channel and
increase the sequence for each virtual channel by frame-number -
previous-frame-number, also taking into account the eventual wrap of the
CSI-2 frame number.

This way we get a monotonically increasing sequence number which is
common to all streams using the same virtual channel.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-camerarx.c |  1 +
 drivers/media/platform/ti/cal/cal.c          | 57 +++++++++++++++++++-
 drivers/media/platform/ti/cal/cal.h          |  7 ++-
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index 34b8542133b6..96adbf7d8b65 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -844,6 +844,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	phy->cal = cal;
 	phy->instance = instance;
 
+	spin_lock_init(&phy->vc_lock);
 	mutex_init(&phy->mutex);
 
 	phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
index 4a4a6c5983f7..783ce5a8cd79 100644
--- a/drivers/media/platform/ti/cal/cal.c
+++ b/drivers/media/platform/ti/cal/cal.c
@@ -496,7 +496,22 @@ void cal_ctx_unprepare(struct cal_ctx *ctx)
 
 void cal_ctx_start(struct cal_ctx *ctx)
 {
-	ctx->sequence = 0;
+	struct cal_camerarx *phy = ctx->phy;
+
+	/*
+	 * Reset the frame number & sequence number, but only if the
+	 * virtual channel is not already in use.
+	 */
+
+	spin_lock(&phy->vc_lock);
+
+	if (phy->vc_enable_count[ctx->vc]++ == 0) {
+		phy->vc_frame_number[ctx->vc] = 0;
+		phy->vc_sequence[ctx->vc] = 0;
+	}
+
+	spin_unlock(&phy->vc_lock);
+
 	ctx->dma.state = CAL_DMA_RUNNING;
 
 	/* Configure the CSI-2, pixel processing and write DMA contexts. */
@@ -516,8 +531,15 @@ void cal_ctx_start(struct cal_ctx *ctx)
 
 void cal_ctx_stop(struct cal_ctx *ctx)
 {
+	struct cal_camerarx *phy = ctx->phy;
 	long timeout;
 
+	WARN_ON(phy->vc_enable_count[ctx->vc] == 0);
+
+	spin_lock(&phy->vc_lock);
+	phy->vc_enable_count[ctx->vc]--;
+	spin_unlock(&phy->vc_lock);
+
 	/*
 	 * Request DMA stop and wait until it completes. If completion times
 	 * out, forcefully disable the DMA.
@@ -554,6 +576,34 @@ void cal_ctx_stop(struct cal_ctx *ctx)
  * ------------------------------------------------------------------
  */
 
+/*
+ * Track a sequence number for each virtual channel, which is shared by
+ * all contexts using the same virtual channel. This is done using the
+ * CSI-2 frame number as a base.
+ */
+static void cal_update_seq_number(struct cal_ctx *ctx)
+{
+	struct cal_dev *cal = ctx->cal;
+	struct cal_camerarx *phy = ctx->phy;
+	u16 prev_frame_num, frame_num;
+	u8 vc = ctx->vc;
+
+	frame_num =
+		cal_read(cal, CAL_CSI2_STATUS(phy->instance, ctx->csi2_ctx)) &
+		0xffff;
+
+	if (phy->vc_frame_number[vc] != frame_num) {
+		prev_frame_num = phy->vc_frame_number[vc];
+
+		if (prev_frame_num >= frame_num)
+			phy->vc_sequence[vc] += 1;
+		else
+			phy->vc_sequence[vc] += frame_num - prev_frame_num;
+
+		phy->vc_frame_number[vc] = frame_num;
+	}
+}
+
 static inline void cal_irq_wdma_start(struct cal_ctx *ctx)
 {
 	spin_lock(&ctx->dma.lock);
@@ -584,6 +634,8 @@ static inline void cal_irq_wdma_start(struct cal_ctx *ctx)
 	}
 
 	spin_unlock(&ctx->dma.lock);
+
+	cal_update_seq_number(ctx);
 }
 
 static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
@@ -610,7 +662,8 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
 	if (buf) {
 		buf->vb.vb2_buf.timestamp = ktime_get_ns();
 		buf->vb.field = ctx->v_fmt.fmt.pix.field;
-		buf->vb.sequence = ctx->sequence++;
+		buf->vb.sequence = ctx->phy->vc_sequence[ctx->vc];
+
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 	}
 }
diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
index 527e22d022f3..dfb628cd3bdd 100644
--- a/drivers/media/platform/ti/cal/cal.h
+++ b/drivers/media/platform/ti/cal/cal.h
@@ -180,6 +180,12 @@ struct cal_camerarx {
 	struct media_pad	pads[CAL_CAMERARX_NUM_PADS];
 	struct v4l2_mbus_framefmt	formats[CAL_CAMERARX_NUM_PADS];
 
+	/* protects the vc_* fields below */
+	spinlock_t		vc_lock;
+	u8			vc_enable_count[4];
+	u16			vc_frame_number[4];
+	u32			vc_sequence[4];
+
 	/*
 	 * Lock for camerarx ops. Protects:
 	 * - formats
@@ -242,7 +248,6 @@ struct cal_ctx {
 	const struct cal_format_info	**active_fmt;
 	unsigned int		num_active_fmt;
 
-	unsigned int		sequence;
 	struct vb2_queue	vb_vidq;
 	u8			dma_ctx;
 	u8			cport;
-- 
2.25.1

[PATCH 5/6] media: ti: cal: combine wdma irq handling

[Tomi Valkeinen]

Instead of handling the WDMA START and END interrupts separately, we
need to handle both at the same time to better manage the inherent race
conditions related to CAL interrupts.

Change the code so that we have a single function,
cal_irq_handle_wdma(), which gets two booleans, start and end, as
parameters, which allows us to manage the race conditions in the
following patch.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal.c | 59 ++++++++++++-----------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
index 783ce5a8cd79..e4355f266c58 100644
--- a/drivers/media/platform/ti/cal/cal.c
+++ b/drivers/media/platform/ti/cal/cal.c
@@ -668,22 +668,33 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
 	}
 }
 
+static void cal_irq_handle_wdma(struct cal_ctx *ctx, bool start, bool end)
+{
+	if (end)
+		cal_irq_wdma_end(ctx);
+
+	if (start)
+		cal_irq_wdma_start(ctx);
+}
+
 static irqreturn_t cal_irq(int irq_cal, void *data)
 {
 	struct cal_dev *cal = data;
-	u32 status;
-
-	status = cal_read(cal, CAL_HL_IRQSTATUS(0));
-	if (status) {
-		unsigned int i;
+	u32 status[3];
+	unsigned int i;
 
-		cal_write(cal, CAL_HL_IRQSTATUS(0), status);
+	for (i = 0; i < 3; ++i) {
+		status[i] = cal_read(cal, CAL_HL_IRQSTATUS(i));
+		if (status[i])
+			cal_write(cal, CAL_HL_IRQSTATUS(i), status[i]);
+	}
 
-		if (status & CAL_HL_IRQ_OCPO_ERR_MASK)
+	if (status[0]) {
+		if (status[0] & CAL_HL_IRQ_OCPO_ERR_MASK)
 			dev_err_ratelimited(cal->dev, "OCPO ERROR\n");
 
 		for (i = 0; i < cal->data->num_csi2_phy; ++i) {
-			if (status & CAL_HL_IRQ_CIO_MASK(i)) {
+			if (status[0] & CAL_HL_IRQ_CIO_MASK(i)) {
 				u32 cio_stat = cal_read(cal,
 							CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
 
@@ -694,7 +705,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 					  cio_stat);
 			}
 
-			if (status & CAL_HL_IRQ_VC_MASK(i)) {
+			if (status[0] & CAL_HL_IRQ_VC_MASK(i)) {
 				u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i));
 
 				dev_err_ratelimited(cal->dev,
@@ -706,32 +717,12 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 		}
 	}
 
-	/* Check which DMA just finished */
-	status = cal_read(cal, CAL_HL_IRQSTATUS(1));
-	if (status) {
-		unsigned int i;
-
-		/* Clear Interrupt status */
-		cal_write(cal, CAL_HL_IRQSTATUS(1), status);
-
-		for (i = 0; i < cal->num_contexts; ++i) {
-			if (status & CAL_HL_IRQ_WDMA_END_MASK(i))
-				cal_irq_wdma_end(cal->ctx[i]);
-		}
-	}
-
-	/* Check which DMA just started */
-	status = cal_read(cal, CAL_HL_IRQSTATUS(2));
-	if (status) {
-		unsigned int i;
-
-		/* Clear Interrupt status */
-		cal_write(cal, CAL_HL_IRQSTATUS(2), status);
+	for (i = 0; i < cal->num_contexts; ++i) {
+		bool end = !!(status[1] & CAL_HL_IRQ_WDMA_END_MASK(i));
+		bool start = !!(status[2] & CAL_HL_IRQ_WDMA_START_MASK(i));
 
-		for (i = 0; i < cal->num_contexts; ++i) {
-			if (status & CAL_HL_IRQ_WDMA_START_MASK(i))
-				cal_irq_wdma_start(cal->ctx[i]);
-		}
+		if (start || end)
+			cal_irq_handle_wdma(cal->ctx[i], start, end);
 	}
 
 	return IRQ_HANDLED;
-- 
2.25.1

[PATCH 6/6] media: ti: cal: fix wdma irq for metadata

[Tomi Valkeinen]

CAL HW interrupts are inherently racy. If we get both start and end
interrupts, we don't know what has happened: did the DMA for a single
frame start and end, or did one frame end and a new frame start?

Usually for normal pixel frames we get the interrupts separately. If
we do get both, we have to guess. The assumption in the code is that the
active vertical area is larger than the blanking vertical area, and thus
it is more likely that we get the end of the old frame and the start of
a new frame.

However, for embedded data, which is only a few lines high, we always
get both interrupts. Here the assumption is that we get both for the
same frame.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal.c | 33 ++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
index e4355f266c58..12de9171e353 100644
--- a/drivers/media/platform/ti/cal/cal.c
+++ b/drivers/media/platform/ti/cal/cal.c
@@ -670,11 +670,34 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
 
 static void cal_irq_handle_wdma(struct cal_ctx *ctx, bool start, bool end)
 {
-	if (end)
-		cal_irq_wdma_end(ctx);
-
-	if (start)
-		cal_irq_wdma_start(ctx);
+	/*
+	 * CAL HW interrupts are inherently racy. If we get both start and end
+	 * interrupts, we don't know what has happened: did the DMA for a single
+	 * frame start and end, or did one frame end and a new frame start?
+	 *
+	 * Usually for normal pixel frames we get the interrupts separately. If
+	 * we do get both, we have to guess. The assumption in the code below is
+	 * that the active vertical area is larger than the blanking vertical
+	 * area, and thus it is more likely that we get the end of the old frame
+	 * and the start of a new frame.
+	 *
+	 * However, for embedded data, which is only a few lines high, we always
+	 * get both interrupts. Here the assumption is that we get both for the
+	 * same frame.
+	 */
+	if (ctx->v_fmt.fmt.pix.height < 10) {
+		if (start)
+			cal_irq_wdma_start(ctx);
+
+		if (end)
+			cal_irq_wdma_end(ctx);
+	} else {
+		if (end)
+			cal_irq_wdma_end(ctx);
+
+		if (start)
+			cal_irq_wdma_start(ctx);
+	}
 }
 
 static irqreturn_t cal_irq(int irq_cal, void *data)
-- 
2.25.1

Re: [PATCH 1/6] media: ti: cal: fix error paths in cal_camerarx_create()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 21, 2022 at 05:34:44PM +0300, Tomi Valkeinen wrote:
> The error paths are not correct: media_entity_cleanup() should not be
> called unless media_entity_pads_init() has been called. Fix this.

See commit 443bf23d0048 ("media: media-entity: Clarify
media_entity_cleanup() usage") :-)

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti/cal/cal-camerarx.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index 6b43a1525b45..a41941fa819a 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -854,7 +854,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	if (IS_ERR(phy->base)) {
>  		cal_err(cal, "failed to ioremap\n");
>  		ret = PTR_ERR(phy->base);
> -		goto error;
> +		goto err_free_phy;
>  	}
>  
>  	cal_dbg(1, cal, "ioresource %s at %pa - %pa\n",
> @@ -862,11 +862,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  
>  	ret = cal_camerarx_regmap_init(cal, phy);
>  	if (ret)
> -		goto error;
> +		goto err_free_phy;
>  
>  	ret = cal_camerarx_parse_dt(phy);
>  	if (ret)
> -		goto error;
> +		goto err_free_phy;
>  
>  	/* Initialize the V4L2 subdev and media entity. */
>  	sd = &phy->subdev;
> @@ -883,20 +883,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads),
>  				     phy->pads);
>  	if (ret)
> -		goto error;
> +		goto err_free_phy;
>  
>  	ret = cal_camerarx_sd_init_cfg(sd, NULL);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>  
>  	ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>  
>  	return phy;
>  
> -error:
> +err_entity_cleanup:
>  	media_entity_cleanup(&phy->subdev.entity);
> +err_free_phy:
>  	kfree(phy);
>  	return ERR_PTR(ret);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: ti: cal: fix useless variable init

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 21, 2022 at 05:34:45PM +0300, Tomi Valkeinen wrote:
> 'ret' is initialized needlessly in cal_legacy_try_fmt_vid_cap(). We can
> also move the variable to the for block, which is the only place where
> it is used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/ti/cal/cal-video.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> index 3e936a2ca36c..1cbd9bda1892 100644
> --- a/drivers/media/platform/ti/cal/cal-video.c
> +++ b/drivers/media/platform/ti/cal/cal-video.c
> @@ -195,7 +195,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>  	struct cal_ctx *ctx = video_drvdata(file);
>  	const struct cal_format_info *fmtinfo;
>  	struct v4l2_subdev_frame_size_enum fse;
> -	int ret, found;
> +	int found;
>  
>  	fmtinfo = find_format_by_pix(ctx, f->fmt.pix.pixelformat);
>  	if (!fmtinfo) {
> @@ -210,12 +210,13 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>  	f->fmt.pix.field = ctx->v_fmt.fmt.pix.field;
>  
>  	/* check for/find a valid width/height */
> -	ret = 0;
>  	found = false;
>  	fse.pad = 0;
>  	fse.code = fmtinfo->code;
>  	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	for (fse.index = 0; ; fse.index++) {
> +		int ret;
> +
>  		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
>  				       NULL, &fse);
>  		if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: ti: cal: rename sd_state to state

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 21, 2022 at 05:34:46PM +0300, Tomi Valkeinen wrote:

Maybe with a commit message ?

"There is no other state than the subdev state, shorten lines by
renaming the 'sd_state' function parameters to 'state'."

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/ti/cal/cal-camerarx.c | 30 ++++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index a41941fa819a..34b8542133b6 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -595,12 +595,12 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
>  
>  static struct v4l2_mbus_framefmt *
>  cal_camerarx_get_pad_format(struct cal_camerarx *phy,
> -			    struct v4l2_subdev_state *sd_state,
> +			    struct v4l2_subdev_state *state,
>  			    unsigned int pad, u32 which)
>  {
>  	switch (which) {
>  	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_get_try_format(&phy->subdev, sd_state, pad);
> +		return v4l2_subdev_get_try_format(&phy->subdev, state, pad);
>  	case V4L2_SUBDEV_FORMAT_ACTIVE:
>  		return &phy->formats[pad];
>  	default:
> @@ -626,7 +626,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
>  }
>  
>  static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
> -					  struct v4l2_subdev_state *sd_state,
> +					  struct v4l2_subdev_state *state,
>  					  struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> @@ -643,7 +643,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>  			goto out;
>  		}
>  
> -		fmt = cal_camerarx_get_pad_format(phy, sd_state,
> +		fmt = cal_camerarx_get_pad_format(phy, state,
>  						  CAL_CAMERARX_PAD_SINK,
>  						  code->which);
>  		code->code = fmt->code;
> @@ -663,7 +663,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>  }
>  
>  static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
> -					   struct v4l2_subdev_state *sd_state,
> +					   struct v4l2_subdev_state *state,
>  					   struct v4l2_subdev_frame_size_enum *fse)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> @@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  	if (cal_rx_pad_is_source(fse->pad)) {
>  		struct v4l2_mbus_framefmt *fmt;
>  
> -		fmt = cal_camerarx_get_pad_format(phy, sd_state,
> +		fmt = cal_camerarx_get_pad_format(phy, state,
>  						  CAL_CAMERARX_PAD_SINK,
>  						  fse->which);
>  		if (fse->code != fmt->code) {
> @@ -711,7 +711,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  }
>  
>  static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *sd_state,
> +				   struct v4l2_subdev_state *state,
>  				   struct v4l2_subdev_format *format)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> @@ -719,7 +719,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
>  
>  	mutex_lock(&phy->mutex);
>  
> -	fmt = cal_camerarx_get_pad_format(phy, sd_state, format->pad,
> +	fmt = cal_camerarx_get_pad_format(phy, state, format->pad,
>  					  format->which);
>  	format->format = *fmt;
>  
> @@ -729,7 +729,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
>  }
>  
>  static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *sd_state,
> +				   struct v4l2_subdev_state *state,
>  				   struct v4l2_subdev_format *format)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> @@ -739,7 +739,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  
>  	/* No transcoding, source and sink formats must match. */
>  	if (cal_rx_pad_is_source(format->pad))
> -		return cal_camerarx_sd_get_fmt(sd, sd_state, format);
> +		return cal_camerarx_sd_get_fmt(sd, state, format);
>  
>  	/*
>  	 * Default to the first format if the requested media bus code isn't
> @@ -765,12 +765,12 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  
>  	mutex_lock(&phy->mutex);
>  
> -	fmt = cal_camerarx_get_pad_format(phy, sd_state,
> +	fmt = cal_camerarx_get_pad_format(phy, state,
>  					  CAL_CAMERARX_PAD_SINK,
>  					  format->which);
>  	*fmt = format->format;
>  
> -	fmt = cal_camerarx_get_pad_format(phy, sd_state,
> +	fmt = cal_camerarx_get_pad_format(phy, state,
>  					  CAL_CAMERARX_PAD_FIRST_SOURCE,
>  					  format->which);
>  	*fmt = format->format;
> @@ -781,10 +781,10 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  }
>  
>  static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
> -				    struct v4l2_subdev_state *sd_state)
> +				    struct v4l2_subdev_state *state)
>  {
>  	struct v4l2_subdev_format format = {
> -		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> +		.which = state ? V4L2_SUBDEV_FORMAT_TRY
>  		: V4L2_SUBDEV_FORMAT_ACTIVE,
>  		.pad = CAL_CAMERARX_PAD_SINK,
>  		.format = {
> @@ -799,7 +799,7 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
>  		},
>  	};
>  
> -	return cal_camerarx_sd_set_fmt(sd, sd_state, &format);
> +	return cal_camerarx_sd_set_fmt(sd, state, &format);
>  }
>  
>  static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: ti: cal: use CSI-2 frame number for seq number

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote:
> The userspace needs a way to match received metadata buffers to pixel
> data buffers. The obvious way to do this is to use the CSI-2 frame
> number, as both the metadata and the pixel data have the same frame
> number as they come from the same frame.
> 
> However, we don't have means to convey the frame number to userspace. We
> do have the 'sequence' field, which with a few tricks can be used for
> this purpose.
> 
> To achieve this, track the frame number for each virtual channel and
> increase the sequence for each virtual channel by frame-number -
> previous-frame-number, also taking into account the eventual wrap of the
> CSI-2 frame number.
> 
> This way we get a monotonically increasing sequence number which is
> common to all streams using the same virtual channel.

I'd agree in principle, if it wasn't for the fact that sensors are not
required to produce a frame number :-S

Quoting CSI-2 v1.01.00:

For FS and FE synchronization packets the Short Packet Data Field shall
contain a 16-bit frame number. This frame number shall be the same for
the FS and FE synchronization packets corresponding to a given frame.

The 16-bit frame number, when used, shall be non-zero to distinguish it
from the use-case where frame number is inoperative and remains set to
zero.

The behavior of the 16-bit frame number shall be as one of the following

- Frame number is always zero – frame number is inoperative.

- Frame number increments by 1 for every FS packet with the same Virtual
  Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1, 2, 1, 2
  or 1, 2, 3, 4, 1, 2, 3, 4

The frame number must be a non-zero value.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti/cal/cal-camerarx.c |  1 +
>  drivers/media/platform/ti/cal/cal.c          | 57 +++++++++++++++++++-
>  drivers/media/platform/ti/cal/cal.h          |  7 ++-
>  3 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index 34b8542133b6..96adbf7d8b65 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -844,6 +844,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	phy->cal = cal;
>  	phy->instance = instance;
>  
> +	spin_lock_init(&phy->vc_lock);
>  	mutex_init(&phy->mutex);
>  
>  	phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
> index 4a4a6c5983f7..783ce5a8cd79 100644
> --- a/drivers/media/platform/ti/cal/cal.c
> +++ b/drivers/media/platform/ti/cal/cal.c
> @@ -496,7 +496,22 @@ void cal_ctx_unprepare(struct cal_ctx *ctx)
>  
>  void cal_ctx_start(struct cal_ctx *ctx)
>  {
> -	ctx->sequence = 0;
> +	struct cal_camerarx *phy = ctx->phy;
> +
> +	/*
> +	 * Reset the frame number & sequence number, but only if the
> +	 * virtual channel is not already in use.
> +	 */
> +
> +	spin_lock(&phy->vc_lock);
> +
> +	if (phy->vc_enable_count[ctx->vc]++ == 0) {
> +		phy->vc_frame_number[ctx->vc] = 0;
> +		phy->vc_sequence[ctx->vc] = 0;
> +	}
> +
> +	spin_unlock(&phy->vc_lock);
> +
>  	ctx->dma.state = CAL_DMA_RUNNING;
>  
>  	/* Configure the CSI-2, pixel processing and write DMA contexts. */
> @@ -516,8 +531,15 @@ void cal_ctx_start(struct cal_ctx *ctx)
>  
>  void cal_ctx_stop(struct cal_ctx *ctx)
>  {
> +	struct cal_camerarx *phy = ctx->phy;
>  	long timeout;
>  
> +	WARN_ON(phy->vc_enable_count[ctx->vc] == 0);
> +
> +	spin_lock(&phy->vc_lock);
> +	phy->vc_enable_count[ctx->vc]--;
> +	spin_unlock(&phy->vc_lock);
> +
>  	/*
>  	 * Request DMA stop and wait until it completes. If completion times
>  	 * out, forcefully disable the DMA.
> @@ -554,6 +576,34 @@ void cal_ctx_stop(struct cal_ctx *ctx)
>   * ------------------------------------------------------------------
>   */
>  
> +/*
> + * Track a sequence number for each virtual channel, which is shared by
> + * all contexts using the same virtual channel. This is done using the
> + * CSI-2 frame number as a base.
> + */
> +static void cal_update_seq_number(struct cal_ctx *ctx)
> +{
> +	struct cal_dev *cal = ctx->cal;
> +	struct cal_camerarx *phy = ctx->phy;
> +	u16 prev_frame_num, frame_num;
> +	u8 vc = ctx->vc;
> +
> +	frame_num =
> +		cal_read(cal, CAL_CSI2_STATUS(phy->instance, ctx->csi2_ctx)) &
> +		0xffff;
> +
> +	if (phy->vc_frame_number[vc] != frame_num) {
> +		prev_frame_num = phy->vc_frame_number[vc];
> +
> +		if (prev_frame_num >= frame_num)
> +			phy->vc_sequence[vc] += 1;
> +		else
> +			phy->vc_sequence[vc] += frame_num - prev_frame_num;
> +
> +		phy->vc_frame_number[vc] = frame_num;
> +	}
> +}
> +
>  static inline void cal_irq_wdma_start(struct cal_ctx *ctx)
>  {
>  	spin_lock(&ctx->dma.lock);
> @@ -584,6 +634,8 @@ static inline void cal_irq_wdma_start(struct cal_ctx *ctx)
>  	}
>  
>  	spin_unlock(&ctx->dma.lock);
> +
> +	cal_update_seq_number(ctx);
>  }
>  
>  static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
> @@ -610,7 +662,8 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
>  	if (buf) {
>  		buf->vb.vb2_buf.timestamp = ktime_get_ns();
>  		buf->vb.field = ctx->v_fmt.fmt.pix.field;
> -		buf->vb.sequence = ctx->sequence++;
> +		buf->vb.sequence = ctx->phy->vc_sequence[ctx->vc];
> +
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  	}
>  }
> diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
> index 527e22d022f3..dfb628cd3bdd 100644
> --- a/drivers/media/platform/ti/cal/cal.h
> +++ b/drivers/media/platform/ti/cal/cal.h
> @@ -180,6 +180,12 @@ struct cal_camerarx {
>  	struct media_pad	pads[CAL_CAMERARX_NUM_PADS];
>  	struct v4l2_mbus_framefmt	formats[CAL_CAMERARX_NUM_PADS];
>  
> +	/* protects the vc_* fields below */
> +	spinlock_t		vc_lock;
> +	u8			vc_enable_count[4];
> +	u16			vc_frame_number[4];
> +	u32			vc_sequence[4];
> +
>  	/*
>  	 * Lock for camerarx ops. Protects:
>  	 * - formats
> @@ -242,7 +248,6 @@ struct cal_ctx {
>  	const struct cal_format_info	**active_fmt;
>  	unsigned int		num_active_fmt;
>  
> -	unsigned int		sequence;
>  	struct vb2_queue	vb_vidq;
>  	u8			dma_ctx;
>  	u8			cport;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: ti: cal: combine wdma irq handling

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 21, 2022 at 05:34:48PM +0300, Tomi Valkeinen wrote:
> Instead of handling the WDMA START and END interrupts separately, we
> need to handle both at the same time to better manage the inherent race
> conditions related to CAL interrupts.
> 
> Change the code so that we have a single function,
> cal_irq_handle_wdma(), which gets two booleans, start and end, as
> parameters, which allows us to manage the race conditions in the
> following patch.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/ti/cal/cal.c | 59 ++++++++++++-----------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
> index 783ce5a8cd79..e4355f266c58 100644
> --- a/drivers/media/platform/ti/cal/cal.c
> +++ b/drivers/media/platform/ti/cal/cal.c
> @@ -668,22 +668,33 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
>  	}
>  }
>  
> +static void cal_irq_handle_wdma(struct cal_ctx *ctx, bool start, bool end)
> +{
> +	if (end)
> +		cal_irq_wdma_end(ctx);
> +
> +	if (start)
> +		cal_irq_wdma_start(ctx);
> +}
> +
>  static irqreturn_t cal_irq(int irq_cal, void *data)
>  {
>  	struct cal_dev *cal = data;
> -	u32 status;
> -
> -	status = cal_read(cal, CAL_HL_IRQSTATUS(0));
> -	if (status) {
> -		unsigned int i;
> +	u32 status[3];
> +	unsigned int i;
>  
> -		cal_write(cal, CAL_HL_IRQSTATUS(0), status);
> +	for (i = 0; i < 3; ++i) {
> +		status[i] = cal_read(cal, CAL_HL_IRQSTATUS(i));
> +		if (status[i])
> +			cal_write(cal, CAL_HL_IRQSTATUS(i), status[i]);
> +	}
>  
> -		if (status & CAL_HL_IRQ_OCPO_ERR_MASK)
> +	if (status[0]) {
> +		if (status[0] & CAL_HL_IRQ_OCPO_ERR_MASK)
>  			dev_err_ratelimited(cal->dev, "OCPO ERROR\n");
>  
>  		for (i = 0; i < cal->data->num_csi2_phy; ++i) {
> -			if (status & CAL_HL_IRQ_CIO_MASK(i)) {
> +			if (status[0] & CAL_HL_IRQ_CIO_MASK(i)) {
>  				u32 cio_stat = cal_read(cal,
>  							CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
>  
> @@ -694,7 +705,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  					  cio_stat);
>  			}
>  
> -			if (status & CAL_HL_IRQ_VC_MASK(i)) {
> +			if (status[0] & CAL_HL_IRQ_VC_MASK(i)) {
>  				u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i));
>  
>  				dev_err_ratelimited(cal->dev,
> @@ -706,32 +717,12 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  		}
>  	}
>  
> -	/* Check which DMA just finished */
> -	status = cal_read(cal, CAL_HL_IRQSTATUS(1));
> -	if (status) {
> -		unsigned int i;
> -
> -		/* Clear Interrupt status */
> -		cal_write(cal, CAL_HL_IRQSTATUS(1), status);
> -
> -		for (i = 0; i < cal->num_contexts; ++i) {
> -			if (status & CAL_HL_IRQ_WDMA_END_MASK(i))
> -				cal_irq_wdma_end(cal->ctx[i]);
> -		}
> -	}
> -
> -	/* Check which DMA just started */
> -	status = cal_read(cal, CAL_HL_IRQSTATUS(2));
> -	if (status) {
> -		unsigned int i;
> -
> -		/* Clear Interrupt status */
> -		cal_write(cal, CAL_HL_IRQSTATUS(2), status);
> +	for (i = 0; i < cal->num_contexts; ++i) {
> +		bool end = !!(status[1] & CAL_HL_IRQ_WDMA_END_MASK(i));
> +		bool start = !!(status[2] & CAL_HL_IRQ_WDMA_START_MASK(i));
>  
> -		for (i = 0; i < cal->num_contexts; ++i) {
> -			if (status & CAL_HL_IRQ_WDMA_START_MASK(i))
> -				cal_irq_wdma_start(cal->ctx[i]);
> -		}
> +		if (start || end)
> +			cal_irq_handle_wdma(cal->ctx[i], start, end);
>  	}
>  
>  	return IRQ_HANDLED;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: ti: cal: use CSI-2 frame number for seq number

[Tomi Valkeinen]

On 22/04/2022 02:52, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote:
>> The userspace needs a way to match received metadata buffers to pixel
>> data buffers. The obvious way to do this is to use the CSI-2 frame
>> number, as both the metadata and the pixel data have the same frame
>> number as they come from the same frame.
>>
>> However, we don't have means to convey the frame number to userspace. We
>> do have the 'sequence' field, which with a few tricks can be used for
>> this purpose.
>>
>> To achieve this, track the frame number for each virtual channel and
>> increase the sequence for each virtual channel by frame-number -
>> previous-frame-number, also taking into account the eventual wrap of the
>> CSI-2 frame number.
>>
>> This way we get a monotonically increasing sequence number which is
>> common to all streams using the same virtual channel.
> 
> I'd agree in principle, if it wasn't for the fact that sensors are not
> required to produce a frame number :-S

In that case the CAL hardware will increment the register every frame. 
 From CAL doc:

Frame number.
Matches the frame number sent by the camera when the
camera transmits it.
Otherwise, incremented by one on every FS short packet
for this context.
Reset when the context is enabled.

I'll add a note about that to the desc.

  Tomi

Re: [PATCH 1/6] media: ti: cal: fix error paths in cal_camerarx_create()

[Tomi Valkeinen]

On 21/04/2022 23:01, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Apr 21, 2022 at 05:34:44PM +0300, Tomi Valkeinen wrote:
>> The error paths are not correct: media_entity_cleanup() should not be
>> called unless media_entity_pads_init() has been called. Fix this.
> 
> See commit 443bf23d0048 ("media: media-entity: Clarify
> media_entity_cleanup() usage") :-)

Ok, I'll drop this.

  Tomi

Re: [PATCH 4/6] media: ti: cal: use CSI-2 frame number for seq number

[Laurent Pinchart]

Hi Tomi,

On Fri, Apr 22, 2022 at 09:41:51AM +0300, Tomi Valkeinen wrote:
> On 22/04/2022 02:52, Laurent Pinchart wrote:
> > On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote:
> >> The userspace needs a way to match received metadata buffers to pixel
> >> data buffers. The obvious way to do this is to use the CSI-2 frame
> >> number, as both the metadata and the pixel data have the same frame
> >> number as they come from the same frame.
> >>
> >> However, we don't have means to convey the frame number to userspace. We
> >> do have the 'sequence' field, which with a few tricks can be used for
> >> this purpose.
> >>
> >> To achieve this, track the frame number for each virtual channel and
> >> increase the sequence for each virtual channel by frame-number -
> >> previous-frame-number, also taking into account the eventual wrap of the
> >> CSI-2 frame number.
> >>
> >> This way we get a monotonically increasing sequence number which is
> >> common to all streams using the same virtual channel.
> > 
> > I'd agree in principle, if it wasn't for the fact that sensors are not
> > required to produce a frame number :-S
> 
> In that case the CAL hardware will increment the register every frame. 
>  From CAL doc:
> 
> Frame number.
> Matches the frame number sent by the camera when the
> camera transmits it.
> Otherwise, incremented by one on every FS short packet
> for this context.

Is that only when the FS packet contains a frame number equal to 0 ? How
about the extreme case where the frame number counts up to 2 and resets
to 1, effectively toggling between 1 and 2 ? Does your patch support
this correctly ?

> Reset when the context is enabled.
> 
> I'll add a note about that to the desc.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: ti: cal: use CSI-2 frame number for seq number

[Tomi Valkeinen]

On 22/04/2022 19:53, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, Apr 22, 2022 at 09:41:51AM +0300, Tomi Valkeinen wrote:
>> On 22/04/2022 02:52, Laurent Pinchart wrote:
>>> On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote:
>>>> The userspace needs a way to match received metadata buffers to pixel
>>>> data buffers. The obvious way to do this is to use the CSI-2 frame
>>>> number, as both the metadata and the pixel data have the same frame
>>>> number as they come from the same frame.
>>>>
>>>> However, we don't have means to convey the frame number to userspace. We
>>>> do have the 'sequence' field, which with a few tricks can be used for
>>>> this purpose.
>>>>
>>>> To achieve this, track the frame number for each virtual channel and
>>>> increase the sequence for each virtual channel by frame-number -
>>>> previous-frame-number, also taking into account the eventual wrap of the
>>>> CSI-2 frame number.
>>>>
>>>> This way we get a monotonically increasing sequence number which is
>>>> common to all streams using the same virtual channel.
>>>
>>> I'd agree in principle, if it wasn't for the fact that sensors are not
>>> required to produce a frame number :-S
>>
>> In that case the CAL hardware will increment the register every frame.
>>   From CAL doc:
>>
>> Frame number.
>> Matches the frame number sent by the camera when the
>> camera transmits it.
>> Otherwise, incremented by one on every FS short packet
>> for this context.
> 
> Is that only when the FS packet contains a frame number equal to 0 ? How

I think so, although the doc doesn't explicitly say it.

> about the extreme case where the frame number counts up to 2 and resets
> to 1, effectively toggling between 1 and 2 ? Does your patch support
> this correctly ?

I think so, but I didn't try that one. I do have a setup where the 
counts go up to 4 (or was it 3), and it seems to work fine.

The code increments the seq number by new_frame_number - 
prev_frame_number, except when the frame number has wrapped, in which 
case the seq number is incremented by 1.

  Tomi