[PATCH v2 00/15] media: rzg2l-cru: Rework slot programming for V2H/G3E

[PATCH v2 00/15] media: rzg2l-cru: Rework slot programming for V2H/G3E

[Jacopo Mondi]

This patch series starts by collecting a patch sent from Dan in the past
which improves the HW slot programming on V2H(P) to avoid losing frames
under heavy system load conditions.

Tommaso also sent a series a few months ago for the CRU from which I
collected the first two patches.

Around it, I've reworked a bit the locking in the driver which is a bit
coarse and causes lost of frames under heavy system load conditions.

Along with these, bit of drive-by cometic changes here and there to
modernize the driver code.

I've tested on V2H(P) but I've also modified the G2L IRQ handler, so if
anyone could test on G2L and G3E it would be great!

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Changes in v2:
- Add tags
- Remove now unused macros for FrameStart interrupt handling
- Do not include cleanup.h
- Add a patch to remove dev_dbg() from irq handlers
- Move lockdep assertions to later in the series
- Minor cosmetics
- Link to v1: https://lore.kernel.org/r/20260327-b4-cru-rework-v1-0-3b7d0430f538@ideasonboard.com

---
Daniel Scally (1):
      media: rzg2l-cru: Rework rzg2l_cru_fill_hw_slot()

Jacopo Mondi (12):
      media: rzg2l-cru: Modernize locking usage with guards
      media: rzg2l-cru: Use proper guard() in irq handler
      media: rzg2l-cru: Remove locking from start/stop routines
      media: rzg2l-cru: Do not use irqsave when not needed
      media: rzg2l-cru: Remove wrong locking comment
      media: rz2gl-cru: Introduce a spinlock for hw operations
      media: rzg2l-cru: Split hw locking from buffers
      media: rzg2l-cru: Manually track active slot number
      media: rz2gl-cru: Return pending buffers in order
      media: rzg2l-cru: Remove the 'state' variable
      media: rzg2l-cru: Remove debug printouts from irq
      media: rzg2l-cru: Simplify irq return value handling

Tommaso Merciai (2):
      media: rzg2l-cru: Skip ICnMC configuration when ICnSVC is used
      media: rzg2l-cru: Use only frame end interrupts

 .../platform/renesas/rzg2l-cru/rzg2l-cru-regs.h    |   4 +-
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   |  29 +-
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 326 ++++++++-------------
 3 files changed, 137 insertions(+), 222 deletions(-)
---
base-commit: 4fbeef21f5387234111b5d52924e77757626faa5
change-id: 20260326-b4-cru-rework-ba3b712bc715

Best regards,
-- 
Jacopo Mondi <jacopo.mondi@ideasonboard.com>

[PATCH v2 01/15] media: rzg2l-cru: Skip ICnMC configuration when ICnSVC is used

[Jacopo Mondi]

From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

When the CRU is configured to use ICnSVC for virtual channel mapping,
as on the RZ/{G3E, V2H/P} SoC, the ICnMC register must not be
programmed.

Return early after setting up ICnSVC to avoid overriding the ICnMC
register, which is not applicable in this mode.

This prevents unintended register programming when ICnSVC is enabled.

Fixes: 3c5ca0a48bb0 ("media: rzg2l-cru: Drop function pointer to configure CSI")
Cc: stable@vger.kernel.org
Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
[Rework to not break image format programming]
Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h   |  1 +
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c  | 17 +++++++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
index a5a57369ef0e..10e62f2646d0 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
@@ -60,6 +60,7 @@
 #define ICnMC_CSCTHR			BIT(5)
 #define ICnMC_INF(x)			((x) << 16)
 #define ICnMC_VCSEL(x)			((x) << 22)
+#define ICnMC_VCSEL_MASK		GENMASK(23, 22)
 #define ICnMC_INF_MASK			GENMASK(21, 16)
 
 #define ICnMS_IA			BIT(2)
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 162e2ace6931..6aea7c244df1 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -262,19 +262,24 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
 				 u8 csi_vc)
 {
 	const struct rzg2l_cru_info *info = cru->info;
-	u32 icnmc = ICnMC_INF(ip_fmt->datatype);
+	u32 icnmc = rzg2l_cru_read(cru, info->image_conv) & ~(ICnMC_INF_MASK |
+							      ICnMC_VCSEL_MASK);
+	icnmc |= ICnMC_INF(ip_fmt->datatype);
 
+	/*
+	 * VC filtering goes through SVC register on G3E/V2H.
+	 *
+	 * FIXME: virtual channel filtering is likely broken and only VC=0
+	 * works.
+	 */
 	if (cru->info->regs[ICnSVC]) {
 		rzg2l_cru_write(cru, ICnSVCNUM, csi_vc);
 		rzg2l_cru_write(cru, ICnSVC, ICnSVC_SVC0(0) | ICnSVC_SVC1(1) |
 				ICnSVC_SVC2(2) | ICnSVC_SVC3(3));
+	} else {
+		icnmc |= ICnMC_VCSEL(csi_vc);
 	}
 
-	icnmc |= rzg2l_cru_read(cru, info->image_conv) & ~ICnMC_INF_MASK;
-
-	/* Set virtual channel CSI2 */
-	icnmc |= ICnMC_VCSEL(csi_vc);
-
 	rzg2l_cru_write(cru, info->image_conv, icnmc);
 }
 

-- 
2.53.0

[PATCH v2 02/15] media: rzg2l-cru: Use only frame end interrupts

[Jacopo Mondi]

From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

On RZ/G3E the CRU driver relies on the frame end interrupt to detect the
completion of an active frame transfer when stopping DMA.

Update the driver to enable only frame end interrupts (CRUnIE2_FExE),
dropping the usage of the frame start interrupts, which is not required
for this operations flow.

Fix the interrupt status handling in the DMA stopping state by checking
the correct frame end status bits (FExS) instead of the frame start one
(FSxS). Add a dedicated CRUnINTS2_FExS() macro to reflect the actual
register bit layout and drop the now unused CRUnIE2_FSxE() and
CRUnINTS2_FSxS() macros.

This ensures that DMA stopping is triggered by the intended frame end
events and avoids incorrect interrupt handling.

Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h | 3 +--
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c    | 9 ++++-----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
index 10e62f2646d0..23cb50ee8e57 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
@@ -14,12 +14,11 @@
 
 #define CRUnIE_EFE			BIT(17)
 
-#define CRUnIE2_FSxE(x)			BIT(((x) * 3))
 #define CRUnIE2_FExE(x)			BIT(((x) * 3) + 1)
 
 #define CRUnINTS_SFS			BIT(16)
 
-#define CRUnINTS2_FSxS(x)		BIT(((x) * 3))
+#define CRUnINTS2_FExS(x)		BIT(((x) * 3) + 1)
 
 #define CRUnRST_VRESETN			BIT(0)
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 6aea7c244df1..98b6afbc708d 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -440,7 +440,6 @@ static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
 
 void rzg3e_cru_enable_interrupts(struct rzg2l_cru_dev *cru)
 {
-	rzg2l_cru_write(cru, CRUnIE2, CRUnIE2_FSxE(cru->svc_channel));
 	rzg2l_cru_write(cru, CRUnIE2, CRUnIE2_FExE(cru->svc_channel));
 }
 
@@ -700,10 +699,10 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 		}
 
 		if (cru->state == RZG2L_CRU_DMA_STOPPING) {
-			if (irq_status & CRUnINTS2_FSxS(0) ||
-			    irq_status & CRUnINTS2_FSxS(1) ||
-			    irq_status & CRUnINTS2_FSxS(2) ||
-			    irq_status & CRUnINTS2_FSxS(3))
+			if (irq_status & CRUnINTS2_FExS(0) ||
+			    irq_status & CRUnINTS2_FExS(1) ||
+			    irq_status & CRUnINTS2_FExS(2) ||
+			    irq_status & CRUnINTS2_FExS(3))
 				dev_dbg(cru->dev, "IRQ while state stopping\n");
 			return IRQ_HANDLED;
 		}

-- 
2.53.0

[PATCH v2 03/15] media: rzg2l-cru: Modernize locking usage with guards

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

Use more modern lock guard constructs to express the locking sequences
in the rzg2l driver. Also include spinlock.h and mutex.h that were
previously missing.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   |  2 ++
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 31 +++++++---------------
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 3a200db15730..be4a9a4953d1 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -9,7 +9,9 @@
 #define __RZG2L_CRU__
 
 #include <linux/irqreturn.h>
+#include <linux/mutex.h>
 #include <linux/reset.h>
+#include <linux/spinlock.h>
 
 #include <media/v4l2-async.h>
 #include <media/v4l2-dev.h>
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 98b6afbc708d..0739169f4439 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -110,10 +110,10 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 				  enum vb2_buffer_state state)
 {
 	struct rzg2l_cru_buffer *buf, *node;
-	unsigned long flags;
 	unsigned int i;
 
-	spin_lock_irqsave(&cru->qlock, flags);
+	guard(spinlock_irqsave)(&cru->qlock);
+
 	for (i = 0; i < cru->num_buf; i++) {
 		if (cru->queue_buf[i]) {
 			vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
@@ -126,7 +126,6 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 		vb2_buffer_done(&buf->vb.vb2_buf, state);
 		list_del(&buf->list);
 	}
-	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
 static int rzg2l_cru_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
@@ -165,13 +164,9 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vb->vb2_queue);
-	unsigned long flags;
-
-	spin_lock_irqsave(&cru->qlock, flags);
 
+	guard(spinlock_irqsave)(&cru->qlock);
 	list_add_tail(to_buf_list(vbuf), &cru->buf_list);
-
-	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
 static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
@@ -465,7 +460,6 @@ void rzg2l_cru_disable_interrupts(struct rzg2l_cru_dev *cru)
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 {
 	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
-	unsigned long flags;
 	u8 csi_vc;
 	int ret;
 
@@ -475,7 +469,7 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	csi_vc = ret;
 	cru->svc_channel = csi_vc;
 
-	spin_lock_irqsave(&cru->qlock, flags);
+	guard(spinlock_irqsave)(&cru->qlock);
 
 	/* Select a video input */
 	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
@@ -492,7 +486,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	/* Initialize image convert */
 	ret = rzg2l_cru_initialize_image_conv(cru, fmt, csi_vc);
 	if (ret) {
-		spin_unlock_irqrestore(&cru->qlock, flags);
 		return ret;
 	}
 
@@ -502,8 +495,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	/* Enable image processing reception */
 	rzg2l_cru_write(cru, ICnEN, ICnEN_ICEN);
 
-	spin_unlock_irqrestore(&cru->qlock, flags);
-
 	return 0;
 }
 
@@ -573,16 +564,15 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 {
 	struct rzg2l_cru_dev *cru = data;
 	unsigned int handled = 0;
-	unsigned long flags;
 	u32 irq_status;
 	u32 amnmbs;
 	int slot;
 
-	spin_lock_irqsave(&cru->qlock, flags);
+	guard(spinlock_irqsave)(&cru->qlock);
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS);
 	if (!irq_status)
-		goto done;
+		return IRQ_RETVAL(handled);
 
 	handled = 1;
 
@@ -591,14 +581,14 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
 	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
 		dev_dbg(cru->dev, "IRQ while state stopped\n");
-		goto done;
+		return IRQ_RETVAL(handled);
 	}
 
 	/* Increase stop retries if capture status is 'RZG2L_CRU_DMA_STOPPING' */
 	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
 		if (irq_status & CRUnINTS_SFS)
 			dev_dbg(cru->dev, "IRQ while state stopping\n");
-		goto done;
+		return IRQ_RETVAL(handled);
 	}
 
 	/* Prepare for capture and update state */
@@ -621,7 +611,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	if (cru->state == RZG2L_CRU_DMA_STARTING) {
 		if (slot != 0) {
 			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
-			goto done;
+			return IRQ_RETVAL(handled);
 		}
 
 		dev_dbg(cru->dev, "Capture start synced!\n");
@@ -646,9 +636,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	/* Prepare for next frame */
 	rzg2l_cru_fill_hw_slot(cru, slot);
 
-done:
-	spin_unlock_irqrestore(&cru->qlock, flags);
-
 	return IRQ_RETVAL(handled);
 }
 

-- 
2.53.0

[PATCH v2 04/15] media: rzg2l-cru: Use proper guard() in irq handler

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The irq handler uses a scoped_guard() that covers the whole function
body.

Replace it with a more appropriate guard() and reduce the indentation.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 104 ++++++++++-----------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 0739169f4439..75928b0f48be 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -670,70 +670,70 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	u32 irq_status;
 	int slot;
 
-	scoped_guard(spinlock, &cru->qlock) {
-		irq_status = rzg2l_cru_read(cru, CRUnINTS2);
-		if (!irq_status)
-			return IRQ_NONE;
+	guard(spinlock)(&cru->qlock);
 
-		dev_dbg(cru->dev, "CRUnINTS2 0x%x\n", irq_status);
+	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
+	if (!irq_status)
+		return IRQ_NONE;
 
-		rzg2l_cru_write(cru, CRUnINTS2, rzg2l_cru_read(cru, CRUnINTS2));
+	dev_dbg(cru->dev, "CRUnINTS2 0x%x\n", irq_status);
 
-		/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
-		if (cru->state == RZG2L_CRU_DMA_STOPPED) {
-			dev_dbg(cru->dev, "IRQ while state stopped\n");
-			return IRQ_HANDLED;
-		}
+	rzg2l_cru_write(cru, CRUnINTS2, rzg2l_cru_read(cru, CRUnINTS2));
 
-		if (cru->state == RZG2L_CRU_DMA_STOPPING) {
-			if (irq_status & CRUnINTS2_FExS(0) ||
-			    irq_status & CRUnINTS2_FExS(1) ||
-			    irq_status & CRUnINTS2_FExS(2) ||
-			    irq_status & CRUnINTS2_FExS(3))
-				dev_dbg(cru->dev, "IRQ while state stopping\n");
-			return IRQ_HANDLED;
-		}
+	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
+	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
+		dev_dbg(cru->dev, "IRQ while state stopped\n");
+		return IRQ_HANDLED;
+	}
 
-		slot = rzg3e_cru_get_current_slot(cru);
-		if (slot < 0)
-			return IRQ_HANDLED;
+	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
+		if (irq_status & CRUnINTS2_FExS(0) ||
+		    irq_status & CRUnINTS2_FExS(1) ||
+		    irq_status & CRUnINTS2_FExS(2) ||
+		    irq_status & CRUnINTS2_FExS(3))
+			dev_dbg(cru->dev, "IRQ while state stopping\n");
+		return IRQ_HANDLED;
+	}
 
-		dev_dbg(cru->dev, "Current written slot: %d\n", slot);
-		cru->buf_addr[slot] = 0;
-
-		/*
-		 * To hand buffers back in a known order to userspace start
-		 * to capture first from slot 0.
-		 */
-		if (cru->state == RZG2L_CRU_DMA_STARTING) {
-			if (slot != 0) {
-				dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
-				return IRQ_HANDLED;
-			}
-			dev_dbg(cru->dev, "Capture start synced!\n");
-			cru->state = RZG2L_CRU_DMA_RUNNING;
-		}
+	slot = rzg3e_cru_get_current_slot(cru);
+	if (slot < 0)
+		return IRQ_HANDLED;
 
-		/* Capture frame */
-		if (cru->queue_buf[slot]) {
-			struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
-
-			buf->field = cru->format.field;
-			buf->sequence = cru->sequence;
-			buf->vb2_buf.timestamp = ktime_get_ns();
-			vb2_buffer_done(&buf->vb2_buf, VB2_BUF_STATE_DONE);
-			cru->queue_buf[slot] = NULL;
-		} else {
-			/* Scratch buffer was used, dropping frame. */
-			dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
+	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
+	cru->buf_addr[slot] = 0;
+
+	/*
+	 * To hand buffers back in a known order to userspace start
+	 * to capture first from slot 0.
+	 */
+	if (cru->state == RZG2L_CRU_DMA_STARTING) {
+		if (slot != 0) {
+			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
+			return IRQ_HANDLED;
 		}
+		dev_dbg(cru->dev, "Capture start synced!\n");
+		cru->state = RZG2L_CRU_DMA_RUNNING;
+	}
 
-		cru->sequence++;
+	/* Capture frame */
+	if (cru->queue_buf[slot]) {
+		struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
 
-		/* Prepare for next frame */
-		rzg2l_cru_fill_hw_slot(cru, slot);
+		buf->field = cru->format.field;
+		buf->sequence = cru->sequence;
+		buf->vb2_buf.timestamp = ktime_get_ns();
+		vb2_buffer_done(&buf->vb2_buf, VB2_BUF_STATE_DONE);
+		cru->queue_buf[slot] = NULL;
+	} else {
+		/* Scratch buffer was used, dropping frame. */
+		dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
 	}
 
+	cru->sequence++;
+
+	/* Prepare for next frame */
+	rzg2l_cru_fill_hw_slot(cru, slot);
+
 	return IRQ_HANDLED;
 }
 

-- 
2.53.0

[PATCH v2 05/15] media: rzg2l-cru: Remove locking from start/stop routines

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The start/stop streaming routines do not need to lock the whole function
body against possible concurrent accesses to the CRU buffers or hardware
registers.

The stop function starts by disabling interrupts, and only this portion
needs to be protected not to race against a possible IRQ.

Once interrupts are disabled, nothing in the video device driver can race
and once the peripheral has been disabled we can release all pending
buffers.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 75928b0f48be..96c71f1357f8 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -340,23 +340,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
 void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 {
 	unsigned int retries = 0;
-	unsigned long flags;
 	u32 icnms;
 
-	spin_lock_irqsave(&cru->qlock, flags);
-
-	/* Disable and clear the interrupt */
-	cru->info->disable_interrupts(cru);
+	scoped_guard(spinlock_irq, &cru->qlock) {
+		/* Disable and clear the interrupt */
+		cru->info->disable_interrupts(cru);
+	}
 
 	/* Stop the operation of image conversion */
 	rzg2l_cru_write(cru, ICnEN, 0);
 
 	/* Wait for streaming to stop */
-	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
-		spin_unlock_irqrestore(&cru->qlock, flags);
+	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
 		msleep(RZG2L_TIMEOUT_MS);
-		spin_lock_irqsave(&cru->qlock, flags);
-	}
 
 	icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
 	if (icnms)
@@ -400,8 +396,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 
 	/* Resets the image processing module */
 	rzg2l_cru_write(cru, CRUnRST, 0);
-
-	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
 static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
@@ -469,8 +463,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	csi_vc = ret;
 	cru->svc_channel = csi_vc;
 
-	guard(spinlock_irqsave)(&cru->qlock);
-
 	/* Select a video input */
 	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
 

-- 
2.53.0

[PATCH v2 06/15] media: rzg2l-cru: Do not use irqsave when not needed

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The return_unused_buffers() and rzg2l_cru_buffer_queue() functions
are never called from an interrupt context, hence they do not need to
use the irqsave version of the spinlock primitives.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 96c71f1357f8..b2d1b6a4aaaa 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -112,7 +112,7 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 	struct rzg2l_cru_buffer *buf, *node;
 	unsigned int i;
 
-	guard(spinlock_irqsave)(&cru->qlock);
+	guard(spinlock_irq)(&cru->qlock);
 
 	for (i = 0; i < cru->num_buf; i++) {
 		if (cru->queue_buf[i]) {
@@ -165,7 +165,7 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vb->vb2_queue);
 
-	guard(spinlock_irqsave)(&cru->qlock);
+	guard(spinlock_irq)(&cru->qlock);
 	list_add_tail(to_buf_list(vbuf), &cru->buf_list);
 }
 

-- 
2.53.0

[PATCH v2 07/15] media: rzg2l-cru: Remove wrong locking comment

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

A function documented as "need to hold qlock before calling" actually
takes the lock itself.

Drop the comment and prepare to replace it with proper annotations where
appropriate.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index b2d1b6a4aaaa..c49131587679 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -105,7 +105,6 @@ __rzg2l_cru_read_constant(struct rzg2l_cru_dev *cru, u32 offset)
 	 __rzg2l_cru_read_constant(cru, offset) : \
 	 __rzg2l_cru_read(cru, offset))
 
-/* Need to hold qlock before calling */
 static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 				  enum vb2_buffer_state state)
 {

-- 
2.53.0

[PATCH v2 08/15] media: rz2gl-cru: Introduce a spinlock for hw operations

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The CRU driver uses a single spinlock to protect the buffers queue and
the hardware operations.

This single spinlock is held for the whole duration of the interrupt
handler, causing all other driver's operations to freeze.

Under heavy system stress conditions with userspace not providing
buffers fast enough, this causes loss of frames.

Prepare to re-work the driver locking by introducing (but not using yet)
a new spinlock to protect the hardware registers programming.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h   | 10 +++++++---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index be4a9a4953d1..12d574182eb8 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -111,7 +111,6 @@ struct rzg2l_cru_info {
  * @v4l2_dev:		V4L2 device
  * @num_buf:		Holds the current number of buffers enabled
  * @svc_channel:	SVC0/1/2/3 to use for RZ/G3E
- * @buf_addr:		Memory addresses where current video data is written.
  * @notifier:		V4L2 asynchronous subdevs notifier
  *
  * @ip:			Image processing subdev info
@@ -120,6 +119,10 @@ struct rzg2l_cru_info {
  * @mdev_lock:		protects the count, notifier and csi members
  * @pad:		media pad for the video device entity
  *
+ * @hw_lock:		protects the slot counter, hardware programming of
+ *			slot addresses and the @buf_addr[] list
+ * @buf_addr:		Memory addresses where current video data is written
+ *
  * @lock:		protects @queue
  * @queue:		vb2 buffers queue
  * @scratch:		cpu address for scratch buffer
@@ -149,8 +152,6 @@ struct rzg2l_cru_dev {
 	u8 num_buf;
 
 	u8 svc_channel;
-	dma_addr_t buf_addr[RZG2L_CRU_HW_BUFFER_DEFAULT];
-
 	struct v4l2_async_notifier notifier;
 
 	struct rzg2l_cru_ip ip;
@@ -159,6 +160,9 @@ struct rzg2l_cru_dev {
 	struct mutex mdev_lock;
 	struct media_pad pad;
 
+	spinlock_t hw_lock;
+	dma_addr_t buf_addr[RZG2L_CRU_HW_BUFFER_DEFAULT];
+
 	struct mutex lock;
 	struct vb2_queue queue;
 	void *scratch;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index c49131587679..b6616d54f8a3 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -843,6 +843,7 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
 	mutex_init(&cru->lock);
 	INIT_LIST_HEAD(&cru->buf_list);
 
+	spin_lock_init(&cru->hw_lock);
 	spin_lock_init(&cru->qlock);
 
 	cru->state = RZG2L_CRU_DMA_STOPPED;

-- 
2.53.0

[PATCH v2 09/15] media: rzg2l-cru: Split hw locking from buffers

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

Split the locking between a spinlock dedicated to protect the hardware
slots programming (hw_lock) and one lock (qlock) to protect the queue of
buffers submitted by userspace.

Do not rework the locking strategy yet but start simply by splitting the
locking in two.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 30 ++++++++++++----------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index b6616d54f8a3..5769dbcbd084 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -111,19 +111,21 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 	struct rzg2l_cru_buffer *buf, *node;
 	unsigned int i;
 
-	guard(spinlock_irq)(&cru->qlock);
-
-	for (i = 0; i < cru->num_buf; i++) {
-		if (cru->queue_buf[i]) {
-			vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
-					state);
-			cru->queue_buf[i] = NULL;
+	scoped_guard(spinlock_irq, &cru->hw_lock) {
+		for (i = 0; i < cru->num_buf; i++) {
+			if (cru->queue_buf[i]) {
+				vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
+						state);
+				cru->queue_buf[i] = NULL;
+			}
 		}
 	}
 
-	list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
-		vb2_buffer_done(&buf->vb.vb2_buf, state);
-		list_del(&buf->list);
+	scoped_guard(spinlock_irq, &cru->qlock) {
+		list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
+			vb2_buffer_done(&buf->vb.vb2_buf, state);
+			list_del(&buf->list);
+		}
 	}
 }
 
@@ -203,6 +205,8 @@ static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
 
 	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
 
+	guard(spinlock)(&cru->qlock);
+
 	if (list_empty(&cru->buf_list)) {
 		cru->queue_buf[slot] = NULL;
 		phys_addr = cru->scratch_phys;
@@ -341,7 +345,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 	unsigned int retries = 0;
 	u32 icnms;
 
-	scoped_guard(spinlock_irq, &cru->qlock) {
+	scoped_guard(spinlock_irq, &cru->hw_lock) {
 		/* Disable and clear the interrupt */
 		cru->info->disable_interrupts(cru);
 	}
@@ -559,7 +563,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	u32 amnmbs;
 	int slot;
 
-	guard(spinlock_irqsave)(&cru->qlock);
+	guard(spinlock_irqsave)(&cru->hw_lock);
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS);
 	if (!irq_status)
@@ -661,7 +665,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	u32 irq_status;
 	int slot;
 
-	guard(spinlock)(&cru->qlock);
+	guard(spinlock)(&cru->hw_lock);
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
 	if (!irq_status)

-- 
2.53.0

[PATCH v2 10/15] media: rzg2l-cru: Manually track active slot number

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The CRU cycles over the hardware slots where the destination address for
the next frame has to be programmed.

The RZ/G2L version of the IP has a register that tells which is the
last used slot by the hardware but, unfortunately, such register is not
available on RZ/G3E and RZ/V2H(P).

The driver currently compares the value of the AMnMADRSL/H register
which report "the memory address which the current video data was
written to" and compares it with the address programmed in the slots.

This heuristic requires a bit of book keeping and proper locking. As the
driver handles the FrameEnd interrupt, it's way easier to keep track
of the slot that has been used by ourselves with a driver variable.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   |  6 +++--
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 31 +++-------------------
 2 files changed, 7 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 12d574182eb8..25f17069585c 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -119,9 +119,10 @@ struct rzg2l_cru_info {
  * @mdev_lock:		protects the count, notifier and csi members
  * @pad:		media pad for the video device entity
  *
- * @hw_lock:		protects the slot counter, hardware programming of
- *			slot addresses and the @buf_addr[] list
+ * @hw_lock:		protects the @active_slot counter, hardware programming
+ *			of slot addresses and the @buf_addr[] list
  * @buf_addr:		Memory addresses where current video data is written
+ * @active_slot:	The slot in use
  *
  * @lock:		protects @queue
  * @queue:		vb2 buffers queue
@@ -162,6 +163,7 @@ struct rzg2l_cru_dev {
 
 	spinlock_t hw_lock;
 	dma_addr_t buf_addr[RZG2L_CRU_HW_BUFFER_DEFAULT];
+	unsigned int active_slot;
 
 	struct mutex lock;
 	struct vb2_queue queue;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 5769dbcbd084..b02940369a18 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -634,31 +634,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
-static int rzg3e_cru_get_current_slot(struct rzg2l_cru_dev *cru)
-{
-	u64 amnmadrs;
-	int slot;
-
-	/*
-	 * When AMnMADRSL is read, AMnMADRSH of the higher-order
-	 * address also latches the address.
-	 *
-	 * AMnMADRSH must be read after AMnMADRSL has been read.
-	 */
-	amnmadrs = rzg2l_cru_read(cru, AMnMADRSL);
-	amnmadrs |= (u64)rzg2l_cru_read(cru, AMnMADRSH) << 32;
-
-	/* Ensure amnmadrs is within this buffer range */
-	for (slot = 0; slot < cru->num_buf; slot++) {
-		if (amnmadrs >= cru->buf_addr[slot] &&
-		    amnmadrs < cru->buf_addr[slot] + cru->format.sizeimage)
-			return slot;
-	}
-
-	dev_err(cru->dev, "Invalid MB address 0x%llx (out of range)\n", amnmadrs);
-	return -EINVAL;
-}
-
 irqreturn_t rzg3e_cru_irq(int irq, void *data)
 {
 	struct rzg2l_cru_dev *cru = data;
@@ -690,9 +665,8 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	slot = rzg3e_cru_get_current_slot(cru);
-	if (slot < 0)
-		return IRQ_HANDLED;
+	slot = cru->active_slot;
+	cru->active_slot = (cru->active_slot + 1) % cru->num_buf;
 
 	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
 	cru->buf_addr[slot] = 0;
@@ -769,6 +743,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 		goto assert_presetn;
 	}
 
+	cru->active_slot = 0;
 	cru->sequence = 0;
 
 	ret = rzg2l_cru_set_stream(cru, 1);

-- 
2.53.0

[PATCH v2 11/15] media: rz2gl-cru: Return pending buffers in order

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

Buffers are programmed into slots in queueing order.

When returning pending buffers we can't simply start from the first slot
but we should actually iterate slots starting from the one is use. The
rzg3e_cru_irq() handler already uses 'active_slot', make rzg2l_cru_irq()
use it as well to know where to start iterating from.

As the pattern of iterating over slots in order will be used for slots
programming in the next patches, provide an helper macro to do that.

While at it, rename return_unused_buffers() to rzg2l_cru_return_buffers().

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 68 +++++++++++++++-------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index b02940369a18..a5197196a408 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -42,6 +42,24 @@ struct rzg2l_cru_buffer {
 #define to_buf_list(vb2_buffer) \
 	(&container_of(vb2_buffer, struct rzg2l_cru_buffer, vb)->list)
 
+/*
+ * The CRU hardware cycles over its slots when transferring frames. All drivers
+ * structure that contains programming data for the slots, such as the memory
+ * destination addresses have to be iterated as they were circular buffers.
+ *
+ * Provide here utilities to iterate over slots and the associated data.
+ */
+static inline unsigned int rzg2l_cru_slot_next(struct rzg2l_cru_dev *cru,
+					       unsigned int slot)
+{
+	return (slot + 1) % cru->num_buf;
+}
+
+/* Start cycling on cru slots from the one after 'start'. */
+#define for_each_cru_slot_from(cru, slot, start)			\
+	for (slot = rzg2l_cru_slot_next(cru, start);			\
+	     slot != start; slot = rzg2l_cru_slot_next(cru, slot))
+
 /* -----------------------------------------------------------------------------
  * DMA operations
  */
@@ -105,28 +123,36 @@ __rzg2l_cru_read_constant(struct rzg2l_cru_dev *cru, u32 offset)
 	 __rzg2l_cru_read_constant(cru, offset) : \
 	 __rzg2l_cru_read(cru, offset))
 
-static void return_unused_buffers(struct rzg2l_cru_dev *cru,
-				  enum vb2_buffer_state state)
+static void rzg2l_cru_return_buffers(struct rzg2l_cru_dev *cru,
+				     enum vb2_buffer_state state)
 {
 	struct rzg2l_cru_buffer *buf, *node;
-	unsigned int i;
 
 	scoped_guard(spinlock_irq, &cru->hw_lock) {
-		for (i = 0; i < cru->num_buf; i++) {
-			if (cru->queue_buf[i]) {
-				vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
-						state);
-				cru->queue_buf[i] = NULL;
-			}
+		/* Return the buffer in progress first, if not completed yet. */
+		unsigned int slot = cru->active_slot;
+
+		if (cru->queue_buf[slot]) {
+			vb2_buffer_done(&cru->queue_buf[slot]->vb2_buf, state);
+			cru->queue_buf[slot] = NULL;
 		}
-	}
 
-	scoped_guard(spinlock_irq, &cru->qlock) {
-		list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
-			vb2_buffer_done(&buf->vb.vb2_buf, state);
-			list_del(&buf->list);
+		/* Return all the pending buffers after the active one. */
+		for_each_cru_slot_from(cru, slot, cru->active_slot) {
+			if (!cru->queue_buf[slot])
+				continue;
+
+			vb2_buffer_done(&cru->queue_buf[slot]->vb2_buf, state);
+			cru->queue_buf[slot] = NULL;
 		}
 	}
+
+	guard(spinlock_irq)(&cru->qlock);
+
+	list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
+		list_del(&buf->list);
+	}
 }
 
 static int rzg2l_cru_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
@@ -588,16 +614,16 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 
 	/* Prepare for capture and update state */
 	amnmbs = rzg2l_cru_read(cru, AMnMBS);
-	slot = amnmbs & AMnMBS_MBSTS;
+	cru->active_slot = amnmbs & AMnMBS_MBSTS;
 
 	/*
 	 * AMnMBS.MBSTS indicates the destination of Memory Bank (MB).
 	 * Recalculate to get the current transfer complete MB.
 	 */
-	if (slot == 0)
+	if (cru->active_slot == 0)
 		slot = cru->num_buf - 1;
 	else
-		slot--;
+		slot = cru->active_slot - 1;
 
 	/*
 	 * To hand buffers back in a known order to userspace start
@@ -666,7 +692,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	}
 
 	slot = cru->active_slot;
-	cru->active_slot = (cru->active_slot + 1) % cru->num_buf;
+	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
 
 	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
 	cru->buf_addr[slot] = 0;
@@ -737,7 +763,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 	cru->scratch = dma_alloc_coherent(cru->dev, cru->format.sizeimage,
 					  &cru->scratch_phys, GFP_KERNEL);
 	if (!cru->scratch) {
-		return_unused_buffers(cru, VB2_BUF_STATE_QUEUED);
+		rzg2l_cru_return_buffers(cru, VB2_BUF_STATE_QUEUED);
 		dev_err(cru->dev, "Failed to allocate scratch buffer\n");
 		ret = -ENOMEM;
 		goto assert_presetn;
@@ -748,7 +774,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 
 	ret = rzg2l_cru_set_stream(cru, 1);
 	if (ret) {
-		return_unused_buffers(cru, VB2_BUF_STATE_QUEUED);
+		rzg2l_cru_return_buffers(cru, VB2_BUF_STATE_QUEUED);
 		goto out;
 	}
 
@@ -785,7 +811,7 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
 	dma_free_coherent(cru->dev, cru->format.sizeimage,
 			  cru->scratch, cru->scratch_phys);
 
-	return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
+	rzg2l_cru_return_buffers(cru, VB2_BUF_STATE_ERROR);
 
 	reset_control_assert(cru->presetn);
 	clk_disable_unprepare(cru->vclk);

-- 
2.53.0

[PATCH v2 12/15] media: rzg2l-cru: Rework rzg2l_cru_fill_hw_slot()

[Jacopo Mondi]

From: Daniel Scally <dan.scally+renesas@ideasonboard.com>

The current implementation of rzg2l_cru_fill_hw_slot() results in the
artificial loss of frames. At present whenever a frame-complete IRQ
is received the driver fills the hardware slot that was just written
to with the address of the next buffer in the driver's queue. If the
queue is empty, that hardware slot's address is set to the address of
the scratch buffer to enable the capture loop to keep running. There
is a minimum of a two-frame delay before that slot will be written to
however, and in the intervening period userspace may queue more
buffers which could be used.

To resolve the issue rework rzg2l_cru_fill_hw_slot() so that it
iteratively fills all slots from the queue which currently do not
have a buffer assigned, until the queue is empty. The scratch
buffer is only resorted to in the event that the queue is empty and
the next slot that will be written to does not already have a buffer
assigned.

Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 64 +++++++++++++---------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index a5197196a408..f061bee51ea8 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -214,47 +214,52 @@ static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
 }
 
 /*
- * Moves a buffer from the queue to the HW slot. If no buffer is
- * available use the scratch buffer. The scratch buffer is never
- * returned to userspace, its only function is to enable the capture
- * loop to keep running.
+ * Move as many buffers as possible from the queue to HW slots If no buffer is
+ * available use the scratch buffer. The scratch buffer is never returned to
+ * userspace, its only function is to enable the capture loop to keep running.
+ *
+ * @cru: the CRU device
+ * @slot: the slot that has just completed
  */
 static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
 {
-	struct vb2_v4l2_buffer *vbuf;
 	struct rzg2l_cru_buffer *buf;
+	struct vb2_v4l2_buffer *vbuf;
+	unsigned int next_slot;
 	dma_addr_t phys_addr;
 
-	/* A already populated slot shall never be overwritten. */
-	if (WARN_ON(cru->queue_buf[slot]))
-		return;
+	lockdep_assert_held(&cru->hw_lock);
 
-	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
+	/* Find the next slot which hasn't a valid address programmed. */
+	for_each_cru_slot_from(cru, next_slot, slot) {
+		if (cru->queue_buf[next_slot])
+			continue;
 
-	guard(spinlock)(&cru->qlock);
+		scoped_guard(spinlock_irqsave, &cru->qlock) {
+			buf = list_first_entry_or_null(&cru->buf_list,
+						       struct rzg2l_cru_buffer, list);
+			if (buf)
+				list_del_init(&buf->list);
+		}
 
-	if (list_empty(&cru->buf_list)) {
-		cru->queue_buf[slot] = NULL;
-		phys_addr = cru->scratch_phys;
-	} else {
-		/* Keep track of buffer we give to HW */
-		buf = list_entry(cru->buf_list.next,
-				 struct rzg2l_cru_buffer, list);
-		vbuf = &buf->vb;
-		list_del_init(to_buf_list(vbuf));
-		cru->queue_buf[slot] = vbuf;
+		if (!buf) {
+			/* Direct frames to the scratch buffer. */
+			phys_addr = cru->scratch_phys;
+			cru->queue_buf[next_slot] = NULL;
+			rzg2l_cru_set_slot_addr(cru, next_slot, phys_addr);
+			return;
+		}
 
-		/* Setup DMA */
+		vbuf = &buf->vb;
+		cru->queue_buf[next_slot] = vbuf;
 		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
+		rzg2l_cru_set_slot_addr(cru, next_slot, phys_addr);
 	}
-
-	rzg2l_cru_set_slot_addr(cru, slot, phys_addr);
 }
 
 static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
 {
 	const struct rzg2l_cru_info *info = cru->info;
-	unsigned int slot;
 	u32 amnaxiattr;
 
 	/*
@@ -263,8 +268,14 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
 	 */
 	rzg2l_cru_write(cru, AMnMBVALID, AMnMBVALID_MBVALID(cru->num_buf - 1));
 
-	for (slot = 0; slot < cru->num_buf; slot++)
-		rzg2l_cru_fill_hw_slot(cru, slot);
+	/*
+	 * Program slot#0 with the first available buffer, if any. Pass to the
+	 * function 'num_buf - 1' as rzg2l_cru_fill_hw_slot() calculates which
+	 * is the next slot to program.
+	 */
+	scoped_guard(spinlock_irq, &cru->hw_lock) {
+		rzg2l_cru_fill_hw_slot(cru, cru->num_buf - 1);
+	}
 
 	if (info->has_stride) {
 		u32 stride = cru->format.bytesperline;
@@ -695,7 +706,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
 
 	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
-	cru->buf_addr[slot] = 0;
 
 	/*
 	 * To hand buffers back in a known order to userspace start

-- 
2.53.0

Re: [PATCH v2 12/15] media: rzg2l-cru: Rework rzg2l_cru_fill_hw_slot()

[Tommaso Merciai]

Hi Jacopo,
Thanks for your patch.

On Tue, Mar 31, 2026 at 12:27:42PM +0200, Jacopo Mondi wrote:
> From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> 
> The current implementation of rzg2l_cru_fill_hw_slot() results in the
> artificial loss of frames. At present whenever a frame-complete IRQ
> is received the driver fills the hardware slot that was just written
> to with the address of the next buffer in the driver's queue. If the
> queue is empty, that hardware slot's address is set to the address of
> the scratch buffer to enable the capture loop to keep running. There
> is a minimum of a two-frame delay before that slot will be written to
> however, and in the intervening period userspace may queue more
> buffers which could be used.
> 
> To resolve the issue rework rzg2l_cru_fill_hw_slot() so that it
> iteratively fills all slots from the queue which currently do not
> have a buffer assigned, until the queue is empty. The scratch
> buffer is only resorted to in the event that the queue is empty and
> the next slot that will be written to does not already have a buffer
> assigned.
> 

Tested on RZ/G3E + OV5645 image sensor.
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

Kind Regards,
Tommaso

> Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 64 +++++++++++++---------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index a5197196a408..f061bee51ea8 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -214,47 +214,52 @@ static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
>  }
>  
>  /*
> - * Moves a buffer from the queue to the HW slot. If no buffer is
> - * available use the scratch buffer. The scratch buffer is never
> - * returned to userspace, its only function is to enable the capture
> - * loop to keep running.
> + * Move as many buffers as possible from the queue to HW slots If no buffer is
> + * available use the scratch buffer. The scratch buffer is never returned to
> + * userspace, its only function is to enable the capture loop to keep running.
> + *
> + * @cru: the CRU device
> + * @slot: the slot that has just completed
>   */
>  static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
>  {
> -	struct vb2_v4l2_buffer *vbuf;
>  	struct rzg2l_cru_buffer *buf;
> +	struct vb2_v4l2_buffer *vbuf;
> +	unsigned int next_slot;
>  	dma_addr_t phys_addr;
>  
> -	/* A already populated slot shall never be overwritten. */
> -	if (WARN_ON(cru->queue_buf[slot]))
> -		return;
> +	lockdep_assert_held(&cru->hw_lock);
>  
> -	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
> +	/* Find the next slot which hasn't a valid address programmed. */
> +	for_each_cru_slot_from(cru, next_slot, slot) {
> +		if (cru->queue_buf[next_slot])
> +			continue;
>  
> -	guard(spinlock)(&cru->qlock);
> +		scoped_guard(spinlock_irqsave, &cru->qlock) {
> +			buf = list_first_entry_or_null(&cru->buf_list,
> +						       struct rzg2l_cru_buffer, list);
> +			if (buf)
> +				list_del_init(&buf->list);
> +		}
>  
> -	if (list_empty(&cru->buf_list)) {
> -		cru->queue_buf[slot] = NULL;
> -		phys_addr = cru->scratch_phys;
> -	} else {
> -		/* Keep track of buffer we give to HW */
> -		buf = list_entry(cru->buf_list.next,
> -				 struct rzg2l_cru_buffer, list);
> -		vbuf = &buf->vb;
> -		list_del_init(to_buf_list(vbuf));
> -		cru->queue_buf[slot] = vbuf;
> +		if (!buf) {
> +			/* Direct frames to the scratch buffer. */
> +			phys_addr = cru->scratch_phys;
> +			cru->queue_buf[next_slot] = NULL;
> +			rzg2l_cru_set_slot_addr(cru, next_slot, phys_addr);
> +			return;
> +		}
>  
> -		/* Setup DMA */
> +		vbuf = &buf->vb;
> +		cru->queue_buf[next_slot] = vbuf;
>  		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> +		rzg2l_cru_set_slot_addr(cru, next_slot, phys_addr);
>  	}
> -
> -	rzg2l_cru_set_slot_addr(cru, slot, phys_addr);
>  }
>  
>  static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  {
>  	const struct rzg2l_cru_info *info = cru->info;
> -	unsigned int slot;
>  	u32 amnaxiattr;
>  
>  	/*
> @@ -263,8 +268,14 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  	 */
>  	rzg2l_cru_write(cru, AMnMBVALID, AMnMBVALID_MBVALID(cru->num_buf - 1));
>  
> -	for (slot = 0; slot < cru->num_buf; slot++)
> -		rzg2l_cru_fill_hw_slot(cru, slot);
> +	/*
> +	 * Program slot#0 with the first available buffer, if any. Pass to the
> +	 * function 'num_buf - 1' as rzg2l_cru_fill_hw_slot() calculates which
> +	 * is the next slot to program.
> +	 */
> +	scoped_guard(spinlock_irq, &cru->hw_lock) {
> +		rzg2l_cru_fill_hw_slot(cru, cru->num_buf - 1);
> +	}
>  
>  	if (info->has_stride) {
>  		u32 stride = cru->format.bytesperline;
> @@ -695,7 +706,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>  	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
>  
>  	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
> -	cru->buf_addr[slot] = 0;
>  
>  	/*
>  	 * To hand buffers back in a known order to userspace start
> 
> -- 
> 2.53.0
> 

[PATCH v2 13/15] media: rzg2l-cru: Remove the 'state' variable

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The cru driver uses a 'state' variable for debugging purpose in the
interrupt handler. The state is used to detect invalid usage conditions
that are not meant to happen unless the driver has a bug in handling the
stop and start conditions.

Remove the state variable which seems to be a debugging leftover.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   | 15 -----
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 71 +---------------------
 2 files changed, 3 insertions(+), 83 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 25f17069585c..5bf334e173d2 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -38,20 +38,6 @@ enum rzg2l_csi2_pads {
 
 struct rzg2l_cru_dev;
 
-/**
- * enum rzg2l_cru_dma_state - DMA states
- * @RZG2L_CRU_DMA_STOPPED:   No operation in progress
- * @RZG2L_CRU_DMA_STARTING:  Capture starting up
- * @RZG2L_CRU_DMA_RUNNING:   Operation in progress have buffers
- * @RZG2L_CRU_DMA_STOPPING:  Stopping operation
- */
-enum rzg2l_cru_dma_state {
-	RZG2L_CRU_DMA_STOPPED = 0,
-	RZG2L_CRU_DMA_STARTING,
-	RZG2L_CRU_DMA_RUNNING,
-	RZG2L_CRU_DMA_STOPPING,
-};
-
 struct rzg2l_cru_csi {
 	struct v4l2_async_connection *asd;
 	struct v4l2_subdev *subdev;
@@ -174,7 +160,6 @@ struct rzg2l_cru_dev {
 	struct vb2_v4l2_buffer *queue_buf[RZG2L_CRU_HW_BUFFER_MAX];
 	struct list_head buf_list;
 	unsigned int sequence;
-	enum rzg2l_cru_dma_state state;
 
 	struct v4l2_pix_format format;
 };
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index f061bee51ea8..721057edca6b 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -398,8 +398,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 	if (icnms)
 		dev_err(cru->dev, "Failed stop HW, something is seriously broken\n");
 
-	cru->state = RZG2L_CRU_DMA_STOPPED;
-
 	/* Wait until the FIFO becomes empty */
 	for (retries = 5; retries > 0; retries--) {
 		if (cru->info->fifo_empty(cru))
@@ -587,8 +585,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 
 static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru)
 {
-	cru->state = RZG2L_CRU_DMA_STOPPING;
-
 	rzg2l_cru_set_stream(cru, 0);
 }
 
@@ -600,8 +596,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	u32 amnmbs;
 	int slot;
 
-	guard(spinlock_irqsave)(&cru->hw_lock);
-
 	irq_status = rzg2l_cru_read(cru, CRUnINTS);
 	if (!irq_status)
 		return IRQ_RETVAL(handled);
@@ -610,20 +604,9 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 
 	rzg2l_cru_write(cru, CRUnINTS, rzg2l_cru_read(cru, CRUnINTS));
 
-	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
-	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
-		dev_dbg(cru->dev, "IRQ while state stopped\n");
-		return IRQ_RETVAL(handled);
-	}
-
-	/* Increase stop retries if capture status is 'RZG2L_CRU_DMA_STOPPING' */
-	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
-		if (irq_status & CRUnINTS_SFS)
-			dev_dbg(cru->dev, "IRQ while state stopping\n");
-		return IRQ_RETVAL(handled);
-	}
+	/* Calculate slot and prepare for new capture. */
+	guard(spinlock_irqsave)(&cru->hw_lock);
 
-	/* Prepare for capture and update state */
 	amnmbs = rzg2l_cru_read(cru, AMnMBS);
 	cru->active_slot = amnmbs & AMnMBS_MBSTS;
 
@@ -636,20 +619,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	else
 		slot = cru->active_slot - 1;
 
-	/*
-	 * To hand buffers back in a known order to userspace start
-	 * to capture first from slot 0.
-	 */
-	if (cru->state == RZG2L_CRU_DMA_STARTING) {
-		if (slot != 0) {
-			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
-			return IRQ_RETVAL(handled);
-		}
-
-		dev_dbg(cru->dev, "Capture start synced!\n");
-		cru->state = RZG2L_CRU_DMA_RUNNING;
-	}
-
 	/* Capture frame */
 	if (cru->queue_buf[slot]) {
 		cru->queue_buf[slot]->field = cru->format.field;
@@ -677,49 +646,18 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	u32 irq_status;
 	int slot;
 
-	guard(spinlock)(&cru->hw_lock);
-
 	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
 	if (!irq_status)
 		return IRQ_NONE;
 
-	dev_dbg(cru->dev, "CRUnINTS2 0x%x\n", irq_status);
-
 	rzg2l_cru_write(cru, CRUnINTS2, rzg2l_cru_read(cru, CRUnINTS2));
 
-	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
-	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
-		dev_dbg(cru->dev, "IRQ while state stopped\n");
-		return IRQ_HANDLED;
-	}
-
-	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
-		if (irq_status & CRUnINTS2_FExS(0) ||
-		    irq_status & CRUnINTS2_FExS(1) ||
-		    irq_status & CRUnINTS2_FExS(2) ||
-		    irq_status & CRUnINTS2_FExS(3))
-			dev_dbg(cru->dev, "IRQ while state stopping\n");
-		return IRQ_HANDLED;
-	}
-
+	guard(spinlock)(&cru->hw_lock);
 	slot = cru->active_slot;
 	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
 
 	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
 
-	/*
-	 * To hand buffers back in a known order to userspace start
-	 * to capture first from slot 0.
-	 */
-	if (cru->state == RZG2L_CRU_DMA_STARTING) {
-		if (slot != 0) {
-			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
-			return IRQ_HANDLED;
-		}
-		dev_dbg(cru->dev, "Capture start synced!\n");
-		cru->state = RZG2L_CRU_DMA_RUNNING;
-	}
-
 	/* Capture frame */
 	if (cru->queue_buf[slot]) {
 		struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
@@ -788,7 +726,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 		goto out;
 	}
 
-	cru->state = RZG2L_CRU_DMA_STARTING;
 	dev_dbg(cru->dev, "Starting to capture\n");
 	return 0;
 
@@ -861,8 +798,6 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
 	spin_lock_init(&cru->hw_lock);
 	spin_lock_init(&cru->qlock);
 
-	cru->state = RZG2L_CRU_DMA_STOPPED;
-
 	for (i = 0; i < RZG2L_CRU_HW_BUFFER_MAX; i++)
 		cru->queue_buf[i] = NULL;
 

-- 
2.53.0

Re: [PATCH v2 13/15] media: rzg2l-cru: Remove the 'state' variable

[Tommaso Merciai]

Hi Jacopo,

On Tue, Mar 31, 2026 at 12:27:43PM +0200, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> The cru driver uses a 'state' variable for debugging purpose in the
> interrupt handler. The state is used to detect invalid usage conditions
> that are not meant to happen unless the driver has a bug in handling the
> stop and start conditions.
> 
> Remove the state variable which seems to be a debugging leftover.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

Kind Regards,
Tommaso

> ---
>  .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   | 15 -----
>  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 71 +---------------------
>  2 files changed, 3 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 25f17069585c..5bf334e173d2 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -38,20 +38,6 @@ enum rzg2l_csi2_pads {
>  
>  struct rzg2l_cru_dev;
>  
> -/**
> - * enum rzg2l_cru_dma_state - DMA states
> - * @RZG2L_CRU_DMA_STOPPED:   No operation in progress
> - * @RZG2L_CRU_DMA_STARTING:  Capture starting up
> - * @RZG2L_CRU_DMA_RUNNING:   Operation in progress have buffers
> - * @RZG2L_CRU_DMA_STOPPING:  Stopping operation
> - */
> -enum rzg2l_cru_dma_state {
> -	RZG2L_CRU_DMA_STOPPED = 0,
> -	RZG2L_CRU_DMA_STARTING,
> -	RZG2L_CRU_DMA_RUNNING,
> -	RZG2L_CRU_DMA_STOPPING,
> -};
> -
>  struct rzg2l_cru_csi {
>  	struct v4l2_async_connection *asd;
>  	struct v4l2_subdev *subdev;
> @@ -174,7 +160,6 @@ struct rzg2l_cru_dev {
>  	struct vb2_v4l2_buffer *queue_buf[RZG2L_CRU_HW_BUFFER_MAX];
>  	struct list_head buf_list;
>  	unsigned int sequence;
> -	enum rzg2l_cru_dma_state state;
>  
>  	struct v4l2_pix_format format;
>  };
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index f061bee51ea8..721057edca6b 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -398,8 +398,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  	if (icnms)
>  		dev_err(cru->dev, "Failed stop HW, something is seriously broken\n");
>  
> -	cru->state = RZG2L_CRU_DMA_STOPPED;
> -
>  	/* Wait until the FIFO becomes empty */
>  	for (retries = 5; retries > 0; retries--) {
>  		if (cru->info->fifo_empty(cru))
> @@ -587,8 +585,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  
>  static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru)
>  {
> -	cru->state = RZG2L_CRU_DMA_STOPPING;
> -
>  	rzg2l_cru_set_stream(cru, 0);
>  }
>  
> @@ -600,8 +596,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>  	u32 amnmbs;
>  	int slot;
>  
> -	guard(spinlock_irqsave)(&cru->hw_lock);
> -
>  	irq_status = rzg2l_cru_read(cru, CRUnINTS);
>  	if (!irq_status)
>  		return IRQ_RETVAL(handled);
> @@ -610,20 +604,9 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>  
>  	rzg2l_cru_write(cru, CRUnINTS, rzg2l_cru_read(cru, CRUnINTS));
>  
> -	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
> -	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
> -		dev_dbg(cru->dev, "IRQ while state stopped\n");
> -		return IRQ_RETVAL(handled);
> -	}
> -
> -	/* Increase stop retries if capture status is 'RZG2L_CRU_DMA_STOPPING' */
> -	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
> -		if (irq_status & CRUnINTS_SFS)
> -			dev_dbg(cru->dev, "IRQ while state stopping\n");
> -		return IRQ_RETVAL(handled);
> -	}
> +	/* Calculate slot and prepare for new capture. */
> +	guard(spinlock_irqsave)(&cru->hw_lock);
>  
> -	/* Prepare for capture and update state */
>  	amnmbs = rzg2l_cru_read(cru, AMnMBS);
>  	cru->active_slot = amnmbs & AMnMBS_MBSTS;
>  
> @@ -636,20 +619,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>  	else
>  		slot = cru->active_slot - 1;
>  
> -	/*
> -	 * To hand buffers back in a known order to userspace start
> -	 * to capture first from slot 0.
> -	 */
> -	if (cru->state == RZG2L_CRU_DMA_STARTING) {
> -		if (slot != 0) {
> -			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
> -			return IRQ_RETVAL(handled);
> -		}
> -
> -		dev_dbg(cru->dev, "Capture start synced!\n");
> -		cru->state = RZG2L_CRU_DMA_RUNNING;
> -	}
> -
>  	/* Capture frame */
>  	if (cru->queue_buf[slot]) {
>  		cru->queue_buf[slot]->field = cru->format.field;
> @@ -677,49 +646,18 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>  	u32 irq_status;
>  	int slot;
>  
> -	guard(spinlock)(&cru->hw_lock);
> -
>  	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
>  	if (!irq_status)
>  		return IRQ_NONE;
>  
> -	dev_dbg(cru->dev, "CRUnINTS2 0x%x\n", irq_status);
> -
>  	rzg2l_cru_write(cru, CRUnINTS2, rzg2l_cru_read(cru, CRUnINTS2));
>  
> -	/* Nothing to do if capture status is 'RZG2L_CRU_DMA_STOPPED' */
> -	if (cru->state == RZG2L_CRU_DMA_STOPPED) {
> -		dev_dbg(cru->dev, "IRQ while state stopped\n");
> -		return IRQ_HANDLED;
> -	}
> -
> -	if (cru->state == RZG2L_CRU_DMA_STOPPING) {
> -		if (irq_status & CRUnINTS2_FExS(0) ||
> -		    irq_status & CRUnINTS2_FExS(1) ||
> -		    irq_status & CRUnINTS2_FExS(2) ||
> -		    irq_status & CRUnINTS2_FExS(3))
> -			dev_dbg(cru->dev, "IRQ while state stopping\n");
> -		return IRQ_HANDLED;
> -	}
> -
> +	guard(spinlock)(&cru->hw_lock);
>  	slot = cru->active_slot;
>  	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
>  
>  	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
>  
> -	/*
> -	 * To hand buffers back in a known order to userspace start
> -	 * to capture first from slot 0.
> -	 */
> -	if (cru->state == RZG2L_CRU_DMA_STARTING) {
> -		if (slot != 0) {
> -			dev_dbg(cru->dev, "Starting sync slot: %d\n", slot);
> -			return IRQ_HANDLED;
> -		}
> -		dev_dbg(cru->dev, "Capture start synced!\n");
> -		cru->state = RZG2L_CRU_DMA_RUNNING;
> -	}
> -
>  	/* Capture frame */
>  	if (cru->queue_buf[slot]) {
>  		struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
> @@ -788,7 +726,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
>  		goto out;
>  	}
>  
> -	cru->state = RZG2L_CRU_DMA_STARTING;
>  	dev_dbg(cru->dev, "Starting to capture\n");
>  	return 0;
>  
> @@ -861,8 +798,6 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
>  	spin_lock_init(&cru->hw_lock);
>  	spin_lock_init(&cru->qlock);
>  
> -	cru->state = RZG2L_CRU_DMA_STOPPED;
> -
>  	for (i = 0; i < RZG2L_CRU_HW_BUFFER_MAX; i++)
>  		cru->queue_buf[i] = NULL;
>  
> 
> -- 
> 2.53.0
> 

[PATCH v2 14/15] media: rzg2l-cru: Remove debug printouts from irq

[Jacopo Mondi]

Using dev_dbg() in irq handlers to debug per-frame events is marginally
useful and possibly not the best idea, as using printk-based helpers
introduce latencies that impact the drivers operations.

If any tracing/debugging has to be performed around frame events
in interrupt handlers, the tracing subsystem offers better alternatives.

Drop dev_dgb() calls from the CRU interrupt handlers.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 721057edca6b..e2073cc1afc6 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -627,9 +627,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 		vb2_buffer_done(&cru->queue_buf[slot]->vb2_buf,
 				VB2_BUF_STATE_DONE);
 		cru->queue_buf[slot] = NULL;
-	} else {
-		/* Scratch buffer was used, dropping frame. */
-		dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
 	}
 
 	cru->sequence++;
@@ -656,8 +653,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	slot = cru->active_slot;
 	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
 
-	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
-
 	/* Capture frame */
 	if (cru->queue_buf[slot]) {
 		struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
@@ -667,9 +662,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 		buf->vb2_buf.timestamp = ktime_get_ns();
 		vb2_buffer_done(&buf->vb2_buf, VB2_BUF_STATE_DONE);
 		cru->queue_buf[slot] = NULL;
-	} else {
-		/* Scratch buffer was used, dropping frame. */
-		dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
 	}
 
 	cru->sequence++;

-- 
2.53.0

Re: [PATCH v2 14/15] media: rzg2l-cru: Remove debug printouts from irq

[Dan Scally]

Hi Jacopo

On 31/03/2026 11:27, Jacopo Mondi wrote:
> Using dev_dbg() in irq handlers to debug per-frame events is marginally
> useful and possibly not the best idea, as using printk-based helpers
> introduce latencies that impact the drivers operations.
> 
> If any tracing/debugging has to be performed around frame events
> in interrupt handlers, the tracing subsystem offers better alternatives.
> 
> Drop dev_dgb() calls from the CRU interrupt handlers.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Yeah good idea:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 721057edca6b..e2073cc1afc6 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -627,9 +627,6 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>   		vb2_buffer_done(&cru->queue_buf[slot]->vb2_buf,
>   				VB2_BUF_STATE_DONE);
>   		cru->queue_buf[slot] = NULL;
> -	} else {
> -		/* Scratch buffer was used, dropping frame. */
> -		dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
>   	}
>   
>   	cru->sequence++;
> @@ -656,8 +653,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>   	slot = cru->active_slot;
>   	cru->active_slot = rzg2l_cru_slot_next(cru, cru->active_slot);
>   
> -	dev_dbg(cru->dev, "Current written slot: %d\n", slot);
> -
>   	/* Capture frame */
>   	if (cru->queue_buf[slot]) {
>   		struct vb2_v4l2_buffer *buf = cru->queue_buf[slot];
> @@ -667,9 +662,6 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>   		buf->vb2_buf.timestamp = ktime_get_ns();
>   		vb2_buffer_done(&buf->vb2_buf, VB2_BUF_STATE_DONE);
>   		cru->queue_buf[slot] = NULL;
> -	} else {
> -		/* Scratch buffer was used, dropping frame. */
> -		dev_dbg(cru->dev, "Dropping frame %u\n", cru->sequence);
>   	}
>   
>   	cru->sequence++;
> 

[PATCH v2 15/15] media: rzg2l-cru: Simplify irq return value handling

[Jacopo Mondi]

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The rzg2l_cru_irq() irq handler uses a local variable to store the
handler return value.

Simplify it by using IRQ_NONE and IRQ_HANDLED.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index e2073cc1afc6..3390e08cd868 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -591,16 +591,13 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru)
 irqreturn_t rzg2l_cru_irq(int irq, void *data)
 {
 	struct rzg2l_cru_dev *cru = data;
-	unsigned int handled = 0;
 	u32 irq_status;
 	u32 amnmbs;
 	int slot;
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS);
 	if (!irq_status)
-		return IRQ_RETVAL(handled);
-
-	handled = 1;
+		return IRQ_NONE;
 
 	rzg2l_cru_write(cru, CRUnINTS, rzg2l_cru_read(cru, CRUnINTS));
 
@@ -634,7 +631,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	/* Prepare for next frame */
 	rzg2l_cru_fill_hw_slot(cru, slot);
 
-	return IRQ_RETVAL(handled);
+	return IRQ_HANDLED;
 }
 
 irqreturn_t rzg3e_cru_irq(int irq, void *data)

-- 
2.53.0