[PATCH 0/3] media: rcar-vin: Generate FRAME_SYNC events

[PATCH 0/3] media: rcar-vin: Generate FRAME_SYNC events

[Niklas Söderlund]

Hi,

This series extend the VIN interrupt handling to be able to generate 
FRAME_SYNC events. Having these events in user-space is a great help to 
know when a sensor starts to expose a new frame.

Patch 1/3 and 2/3 prepays the existing interrupt infrastructure to 
support more then "frame captured" interrupts. While patch 3/3 enables 
and checks for VSYNC detection and generates the new event.

The feature is tested on Gen2, Gen3 and Gen4 and all devices correctly 
generate FRAME_SYNC events.

Niklas Söderlund (3):
  media: rcar-vin: Fold interrupt helpers into only callers
  media: rcar-vin: Check for correct capture interrupt event
  media: rcar-vin: Generate FRAME_SYNC events

 .../platform/renesas/rcar-vin/rcar-dma.c      | 54 ++++++++++---------
 .../platform/renesas/rcar-vin/rcar-v4l2.c     |  2 +
 2 files changed, 32 insertions(+), 24 deletions(-)

-- 
2.49.0

[PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Niklas Söderlund]

The call sites using the interrupt helper functions have all been
reworked to only one for each. Fold echo of them into the only call
sites left.

While at it rename the variable holding the current interrupt status to
make the code easier to read.

There is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index 5c08ee2c9807..585b8b3dcfd8 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
 	return 0;
 }
 
-static void rvin_disable_interrupts(struct rvin_dev *vin)
-{
-	rvin_write(vin, 0, VNIE_REG);
-}
-
-static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
-{
-	return rvin_read(vin, VNINTS_REG);
-}
-
-static void rvin_ack_interrupt(struct rvin_dev *vin)
-{
-	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
-}
-
 static bool rvin_capture_active(struct rvin_dev *vin)
 {
 	return rvin_read(vin, VNMS_REG) & VNMS_CA;
@@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
 static irqreturn_t rvin_irq(int irq, void *data)
 {
 	struct rvin_dev *vin = data;
-	u32 int_status, vnms;
+	u32 status, vnms;
 	int slot;
 	unsigned int handled = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
-	int_status = rvin_get_interrupt_status(vin);
-	if (!int_status)
+	status = rvin_read(vin, VNINTS_REG);
+	if (!status)
 		goto done;
 
-	rvin_ack_interrupt(vin);
+	rvin_write(vin, status, VNINTS_REG);
 	handled = 1;
 
 	/* Nothing to do if nothing was captured. */
-	if (!(int_status & VNINTS_FIS))
+	if (!(status & VNINTS_FIS))
 		goto done;
 
 	/* Nothing to do if not running. */
@@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
 	rvin_set_stream(vin, 0);
 
 	/* disable interrupts */
-	rvin_disable_interrupts(vin);
+	rvin_write(vin, 0, VNIE_REG);
 
 	/* Return unprocessed buffers from hardware. */
 	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {
-- 
2.49.0

[PATCH 2/3] media: rcar-vin: Check for correct capture interrupt event

[Niklas Söderlund]

Depending on if the capture session deals with fields or whole frames
interrupts can be generated at an end of field, or end of frame event.
The interrupt mask is setup to generate an interrupt on one of the two
events depending on what is needed when the VIN is started. The end of
field bit is set in both cases so controlling the mask that generates an
interrupt have been enough to control the two use-cases.

Before extending the interrupt handler to deal with other types of
interrupt events it is needs to extended to "capture complete" check for
correct the use-case in operation. Without this the simplification in
the handler can result in corrupted frames when the mask on what type of
events can generate an interrupt generated can no longer be assumed to
only be an "capture complete" event.

Which bit is checked matches which bit is enabled at configuration time
as which event can generate an interrupt for "capture complete". There
is no functional change.

While at it switch to use the BIT() macro to describe the bit positions
for the interrupt functions.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index 585b8b3dcfd8..85e44a00e0fc 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -115,11 +115,12 @@
 #define VNFC_S_FRAME		(1 << 0)
 
 /* Video n Interrupt Enable Register bits */
-#define VNIE_FIE		(1 << 4)
-#define VNIE_EFE		(1 << 1)
+#define VNIE_FIE		BIT(4)
+#define VNIE_EFE		BIT(1)
 
 /* Video n Interrupt Status Register bits */
-#define VNINTS_FIS		(1 << 4)
+#define VNINTS_FIS		BIT(4)
+#define VNINTS_EFS		BIT(1)
 
 /* Video n Data Mode Register bits */
 #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
@@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
 static irqreturn_t rvin_irq(int irq, void *data)
 {
 	struct rvin_dev *vin = data;
-	u32 status, vnms;
+	u32 capture, status, vnms;
 	int slot;
 	unsigned int handled = 0;
 	unsigned long flags;
@@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	handled = 1;
 
 	/* Nothing to do if nothing was captured. */
-	if (!(status & VNINTS_FIS))
+	capture = vin->format.field == V4L2_FIELD_NONE ||
+		vin->format.field == V4L2_FIELD_ALTERNATE ?
+		VNINTS_FIS : VNINTS_EFS;
+	if (!(status & capture))
 		goto done;
 
 	/* Nothing to do if not running. */
-- 
2.49.0

[PATCH 3/3] media: rcar-vin: Generate FRAME_SYNC events

[Niklas Söderlund]

Enable the VSYNC Rising Edge Detection interrupt and generate a
FRAME_SYNC event form it. The interrupt is available on all supported
models of the VIN (Gen2, Gen3 and Gen4).

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../media/platform/renesas/rcar-vin/rcar-dma.c  | 17 +++++++++++++++++
 .../media/platform/renesas/rcar-vin/rcar-v4l2.c |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index 85e44a00e0fc..a1ae9c9bccc7 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
 
+#include <media/v4l2-event.h>
 #include <media/videobuf2-dma-contig.h>
 
 #include "rcar-vin.h"
@@ -115,10 +116,14 @@
 #define VNFC_S_FRAME		(1 << 0)
 
 /* Video n Interrupt Enable Register bits */
+#define VNIE_VFE		BIT(17)
+#define VNIE_VRE		BIT(16)
 #define VNIE_FIE		BIT(4)
 #define VNIE_EFE		BIT(1)
 
 /* Video n Interrupt Status Register bits */
+#define VNINTS_VFS		BIT(17)
+#define VNINTS_VRS		BIT(16)
 #define VNINTS_FIS		BIT(4)
 #define VNINTS_EFS		BIT(1)
 
@@ -898,6 +903,8 @@ static int rvin_setup(struct rvin_dev *vin)
 
 	/* Progressive or interlaced mode */
 	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
+	/* Enable VSYNC Rising Edge Detection. */
+	interrupts |= VNIE_VRE;
 
 	/* Ack interrupts */
 	rvin_write(vin, interrupts, VNINTS_REG);
@@ -1049,6 +1056,16 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	rvin_write(vin, status, VNINTS_REG);
 	handled = 1;
 
+	/* Signal Start of Frame. */
+	if (status & VNINTS_VRS) {
+		struct v4l2_event event = {
+			.type = V4L2_EVENT_FRAME_SYNC,
+			.u.frame_sync.frame_sequence = vin->sequence,
+		};
+
+		v4l2_event_queue(&vin->vdev, &event);
+	}
+
 	/* Nothing to do if nothing was captured. */
 	capture = vin->format.field == V4L2_FIELD_NONE ||
 		vin->format.field == V4L2_FIELD_ALTERNATE ?
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
index db091af57c19..6339de54b02b 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
@@ -734,6 +734,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
 				const struct v4l2_event_subscription *sub)
 {
 	switch (sub->type) {
+	case V4L2_EVENT_FRAME_SYNC:
+		return v4l2_event_subscribe(fh, sub, 2, NULL);
 	case V4L2_EVENT_SOURCE_CHANGE:
 		return v4l2_event_subscribe(fh, sub, 4, NULL);
 	}
-- 
2.49.0

Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 14, 2025 at 04:15:43PM +0200, Niklas Söderlund wrote:
> The call sites using the interrupt helper functions have all been
> reworked to only one for each. Fold echo of them into the only call
> sites left.
> 
> While at it rename the variable holding the current interrupt status to
> make the code easier to read.
> 
> There is no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 5c08ee2c9807..585b8b3dcfd8 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
>  	return 0;
>  }
>  
> -static void rvin_disable_interrupts(struct rvin_dev *vin)
> -{
> -	rvin_write(vin, 0, VNIE_REG);
> -}
> -
> -static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
> -{
> -	return rvin_read(vin, VNINTS_REG);
> -}
> -
> -static void rvin_ack_interrupt(struct rvin_dev *vin)
> -{
> -	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
> -}
> -
>  static bool rvin_capture_active(struct rvin_dev *vin)
>  {
>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> @@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> -	u32 int_status, vnms;
> +	u32 status, vnms;
>  	int slot;
>  	unsigned int handled = 0;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&vin->qlock, flags);
>  
> -	int_status = rvin_get_interrupt_status(vin);
> -	if (!int_status)
> +	status = rvin_read(vin, VNINTS_REG);
> +	if (!status)
>  		goto done;
>  
> -	rvin_ack_interrupt(vin);
> +	rvin_write(vin, status, VNINTS_REG);

Actually there is a functional change here. Before this change, if an
interrupt occured between reading VNINTS_REG in
rvin_get_interrupt_status() and reading it again in
rvin_ack_interrupt(), it would get lost. This patch fixes a possible
bug.

With an updated commit message,

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

>  	handled = 1;
>  
>  	/* Nothing to do if nothing was captured. */
> -	if (!(int_status & VNINTS_FIS))
> +	if (!(status & VNINTS_FIS))
>  		goto done;
>  
>  	/* Nothing to do if not running. */
> @@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
>  	rvin_set_stream(vin, 0);
>  
>  	/* disable interrupts */
> -	rvin_disable_interrupts(vin);
> +	rvin_write(vin, 0, VNIE_REG);
>  
>  	/* Return unprocessed buffers from hardware. */
>  	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: rcar-vin: Check for correct capture interrupt event

[Laurent Pinchart]

Hi Niklas,

On Sat, Jun 14, 2025 at 04:15:44PM +0200, Niklas Söderlund wrote:
> Depending on if the capture session deals with fields or whole frames
> interrupts can be generated at an end of field, or end of frame event.
> The interrupt mask is setup to generate an interrupt on one of the two
> events depending on what is needed when the VIN is started. The end of
> field bit is set in both cases so controlling the mask that generates an
> interrupt have been enough to control the two use-cases.
> 
> Before extending the interrupt handler to deal with other types of
> interrupt events it is needs to extended to "capture complete" check for
> correct the use-case in operation. Without this the simplification in
> the handler can result in corrupted frames when the mask on what type of
> events can generate an interrupt generated can no longer be assumed to
> only be an "capture complete" event.
> 
> Which bit is checked matches which bit is enabled at configuration time
> as which event can generate an interrupt for "capture complete". There
> is no functional change.
> 
> While at it switch to use the BIT() macro to describe the bit positions
> for the interrupt functions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 585b8b3dcfd8..85e44a00e0fc 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -115,11 +115,12 @@
>  #define VNFC_S_FRAME		(1 << 0)
>  
>  /* Video n Interrupt Enable Register bits */
> -#define VNIE_FIE		(1 << 4)
> -#define VNIE_EFE		(1 << 1)
> +#define VNIE_FIE		BIT(4)
> +#define VNIE_EFE		BIT(1)
>  
>  /* Video n Interrupt Status Register bits */
> -#define VNINTS_FIS		(1 << 4)
> +#define VNINTS_FIS		BIT(4)
> +#define VNINTS_EFS		BIT(1)
>  
>  /* Video n Data Mode Register bits */
>  #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
> @@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> -	u32 status, vnms;
> +	u32 capture, status, vnms;
>  	int slot;
>  	unsigned int handled = 0;
>  	unsigned long flags;
> @@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	handled = 1;
>  
>  	/* Nothing to do if nothing was captured. */
> -	if (!(status & VNINTS_FIS))
> +	capture = vin->format.field == V4L2_FIELD_NONE ||
> +		vin->format.field == V4L2_FIELD_ALTERNATE ?
> +		VNINTS_FIS : VNINTS_EFS;

I would find

	capture = vin->format.field == V4L2_FIELD_NONE ||
		  vin->format.field == V4L2_FIELD_ALTERNATE
		? VNINTS_FIS : VNINTS_EFS;

easier to read, but it could be just me.

Do I read it correctly that you use the "Field Interrupt" with
V4L2_FIELD_NONE ? That seems a bit weird to me, but maybe I don't get
how the hardware operates.

> +	if (!(status & capture))
>  		goto done;
>  
>  	/* Nothing to do if not running. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: rcar-vin: Generate FRAME_SYNC events

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 14, 2025 at 04:15:45PM +0200, Niklas Söderlund wrote:
> Enable the VSYNC Rising Edge Detection interrupt and generate a
> FRAME_SYNC event form it. The interrupt is available on all supported
> models of the VIN (Gen2, Gen3 and Gen4).
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> ---
>  .../media/platform/renesas/rcar-vin/rcar-dma.c  | 17 +++++++++++++++++
>  .../media/platform/renesas/rcar-vin/rcar-v4l2.c |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 85e44a00e0fc..a1ae9c9bccc7 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -14,6 +14,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/pm_runtime.h>
>  
> +#include <media/v4l2-event.h>
>  #include <media/videobuf2-dma-contig.h>
>  
>  #include "rcar-vin.h"
> @@ -115,10 +116,14 @@
>  #define VNFC_S_FRAME		(1 << 0)
>  
>  /* Video n Interrupt Enable Register bits */
> +#define VNIE_VFE		BIT(17)
> +#define VNIE_VRE		BIT(16)
>  #define VNIE_FIE		BIT(4)
>  #define VNIE_EFE		BIT(1)
>  
>  /* Video n Interrupt Status Register bits */
> +#define VNINTS_VFS		BIT(17)
> +#define VNINTS_VRS		BIT(16)
>  #define VNINTS_FIS		BIT(4)
>  #define VNINTS_EFS		BIT(1)
>  
> @@ -898,6 +903,8 @@ static int rvin_setup(struct rvin_dev *vin)
>  
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
> +	/* Enable VSYNC Rising Edge Detection. */
> +	interrupts |= VNIE_VRE;
>  
>  	/* Ack interrupts */
>  	rvin_write(vin, interrupts, VNINTS_REG);
> @@ -1049,6 +1056,16 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	rvin_write(vin, status, VNINTS_REG);
>  	handled = 1;
>  
> +	/* Signal Start of Frame. */
> +	if (status & VNINTS_VRS) {
> +		struct v4l2_event event = {
> +			.type = V4L2_EVENT_FRAME_SYNC,
> +			.u.frame_sync.frame_sequence = vin->sequence,
> +		};
> +
> +		v4l2_event_queue(&vin->vdev, &event);
> +	}
> +
>  	/* Nothing to do if nothing was captured. */
>  	capture = vin->format.field == V4L2_FIELD_NONE ||
>  		vin->format.field == V4L2_FIELD_ALTERNATE ?
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> index db091af57c19..6339de54b02b 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> @@ -734,6 +734,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
>  				const struct v4l2_event_subscription *sub)
>  {
>  	switch (sub->type) {
> +	case V4L2_EVENT_FRAME_SYNC:
> +		return v4l2_event_subscribe(fh, sub, 2, NULL);
>  	case V4L2_EVENT_SOURCE_CHANGE:
>  		return v4l2_event_subscribe(fh, sub, 4, NULL);
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: rcar-vin: Check for correct capture interrupt event

[Niklas Söderlund]

Hi Laurent,

Thanks for your review!

On 2025-06-15 00:32:13 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Sat, Jun 14, 2025 at 04:15:44PM +0200, Niklas Söderlund wrote:
> > Depending on if the capture session deals with fields or whole frames
> > interrupts can be generated at an end of field, or end of frame event.
> > The interrupt mask is setup to generate an interrupt on one of the two
> > events depending on what is needed when the VIN is started. The end of
> > field bit is set in both cases so controlling the mask that generates an
> > interrupt have been enough to control the two use-cases.
> > 
> > Before extending the interrupt handler to deal with other types of
> > interrupt events it is needs to extended to "capture complete" check for
> > correct the use-case in operation. Without this the simplification in
> > the handler can result in corrupted frames when the mask on what type of
> > events can generate an interrupt generated can no longer be assumed to
> > only be an "capture complete" event.
> > 
> > Which bit is checked matches which bit is enabled at configuration time
> > as which event can generate an interrupt for "capture complete". There
> > is no functional change.
> > 
> > While at it switch to use the BIT() macro to describe the bit positions
> > for the interrupt functions.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index 585b8b3dcfd8..85e44a00e0fc 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -115,11 +115,12 @@
> >  #define VNFC_S_FRAME		(1 << 0)
> >  
> >  /* Video n Interrupt Enable Register bits */
> > -#define VNIE_FIE		(1 << 4)
> > -#define VNIE_EFE		(1 << 1)
> > +#define VNIE_FIE		BIT(4)
> > +#define VNIE_EFE		BIT(1)
> >  
> >  /* Video n Interrupt Status Register bits */
> > -#define VNINTS_FIS		(1 << 4)
> > +#define VNINTS_FIS		BIT(4)
> > +#define VNINTS_EFS		BIT(1)
> >  
> >  /* Video n Data Mode Register bits */
> >  #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
> > @@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> >  static irqreturn_t rvin_irq(int irq, void *data)
> >  {
> >  	struct rvin_dev *vin = data;
> > -	u32 status, vnms;
> > +	u32 capture, status, vnms;
> >  	int slot;
> >  	unsigned int handled = 0;
> >  	unsigned long flags;
> > @@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	handled = 1;
> >  
> >  	/* Nothing to do if nothing was captured. */
> > -	if (!(status & VNINTS_FIS))
> > +	capture = vin->format.field == V4L2_FIELD_NONE ||
> > +		vin->format.field == V4L2_FIELD_ALTERNATE ?
> > +		VNINTS_FIS : VNINTS_EFS;
> 
> I would find
> 
> 	capture = vin->format.field == V4L2_FIELD_NONE ||
> 		  vin->format.field == V4L2_FIELD_ALTERNATE
> 		? VNINTS_FIS : VNINTS_EFS;
> 
> easier to read, but it could be just me.

I agree it's easier to read, but my auto style checker for the file will 
not allow it. And if I make one exception from my format rules I fear 
the file will soon turn wild. The whole construct is ugly and I hope to 
be able to remove it all together in follow up work.

> 
> Do I read it correctly that you use the "Field Interrupt" with
> V4L2_FIELD_NONE ? That seems a bit weird to me, but maybe I don't get
> how the hardware operates.

I agree it's odd, and likely can be improved upon. But it mirrors what 
is done in the capture setup routine. The "Field Interrupt" is enabled 
for NONE and ALTERNATE, and the "End Frame Interrupt" is used for the 
other cases. See the 'progressive' variable in rvin_setup().

The historical reason for this oddity is that VIN placed to much logic 
in the kernel driver that should have been up to user-space, specially 
for Gen2. In this case if the video source was providing ALTERNATE then 
VIN would automatically default to INTERLACED the output. Some of this 
have been reworked and fixed over the years, and a lot of it is finally 
fixed in the Gen2 MC series on the list.

That work is however not a dependency of this work, and I feel like a 
broken record, but I have started work to refactor this and make it 
right. That work is however based on the Gen2 MC series as this allows 
me to add proper tests for all of this that covers all the different 
generations.

I did not want this series to depend on the cleanups and potentially 
grew to yet another large series that fix lots of unrelated things. I 
hope it's OK we live with this oddity a little while longer so we can 
address it properly as a separate thing.

> 
> > +	if (!(status & capture))
> >  		goto done;
> >  
> >  	/* Nothing to do if not running. */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Niklas Söderlund]

Hi Laurent,

Thanks for your review time.

On 2025-06-15 00:23:14 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, Jun 14, 2025 at 04:15:43PM +0200, Niklas Söderlund wrote:
> > The call sites using the interrupt helper functions have all been
> > reworked to only one for each. Fold echo of them into the only call
> > sites left.
> > 
> > While at it rename the variable holding the current interrupt status to
> > make the code easier to read.
> > 
> > There is no functional change.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
> >  1 file changed, 6 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index 5c08ee2c9807..585b8b3dcfd8 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
> >  	return 0;
> >  }
> >  
> > -static void rvin_disable_interrupts(struct rvin_dev *vin)
> > -{
> > -	rvin_write(vin, 0, VNIE_REG);
> > -}
> > -
> > -static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
> > -{
> > -	return rvin_read(vin, VNINTS_REG);
> > -}
> > -
> > -static void rvin_ack_interrupt(struct rvin_dev *vin)
> > -{
> > -	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
> > -}
> > -
> >  static bool rvin_capture_active(struct rvin_dev *vin)
> >  {
> >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> > @@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> >  static irqreturn_t rvin_irq(int irq, void *data)
> >  {
> >  	struct rvin_dev *vin = data;
> > -	u32 int_status, vnms;
> > +	u32 status, vnms;
> >  	int slot;
> >  	unsigned int handled = 0;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&vin->qlock, flags);
> >  
> > -	int_status = rvin_get_interrupt_status(vin);
> > -	if (!int_status)
> > +	status = rvin_read(vin, VNINTS_REG);
> > +	if (!status)
> >  		goto done;
> >  
> > -	rvin_ack_interrupt(vin);
> > +	rvin_write(vin, status, VNINTS_REG);
> 
> Actually there is a functional change here. Before this change, if an
> interrupt occured between reading VNINTS_REG in
> rvin_get_interrupt_status() and reading it again in
> rvin_ack_interrupt(), it would get lost. This patch fixes a possible
> bug.

Indeed, thanks for pointing it out. I will update the commit message to 
mention this.

> 
> With an updated commit message,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> >  	handled = 1;
> >  
> >  	/* Nothing to do if nothing was captured. */
> > -	if (!(int_status & VNINTS_FIS))
> > +	if (!(status & VNINTS_FIS))
> >  		goto done;
> >  
> >  	/* Nothing to do if not running. */
> > @@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
> >  	rvin_set_stream(vin, 0);
> >  
> >  	/* disable interrupts */
> > -	rvin_disable_interrupts(vin);
> > +	rvin_write(vin, 0, VNIE_REG);
> >  
> >  	/* Return unprocessed buffers from hardware. */
> >  	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Geert Uytterhoeven]

Hi Niklas,

Thanks for your patch!

On Sat, 14 Jun 2025 at 16:16, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The call sites using the interrupt helper functions have all been
> reworked to only one for each. Fold echo of them into the only call

s/echo/each/?

> sites left.
>
> While at it rename the variable holding the current interrupt status to
> make the code easier to read.
>
> There is no functional change.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] media: rcar-vin: Check for correct capture interrupt event

[Laurent Pinchart]

Hi Niklas,

On Sun, Jun 15, 2025 at 02:47:52PM +0200, Niklas Söderlund wrote:
> On 2025-06-15 00:32:13 +0300, Laurent Pinchart wrote:
> > On Sat, Jun 14, 2025 at 04:15:44PM +0200, Niklas Söderlund wrote:
> > > Depending on if the capture session deals with fields or whole frames
> > > interrupts can be generated at an end of field, or end of frame event.
> > > The interrupt mask is setup to generate an interrupt on one of the two
> > > events depending on what is needed when the VIN is started. The end of
> > > field bit is set in both cases so controlling the mask that generates an
> > > interrupt have been enough to control the two use-cases.
> > > 
> > > Before extending the interrupt handler to deal with other types of
> > > interrupt events it is needs to extended to "capture complete" check for
> > > correct the use-case in operation. Without this the simplification in
> > > the handler can result in corrupted frames when the mask on what type of
> > > events can generate an interrupt generated can no longer be assumed to
> > > only be an "capture complete" event.
> > > 
> > > Which bit is checked matches which bit is enabled at configuration time
> > > as which event can generate an interrupt for "capture complete". There
> > > is no functional change.
> > > 
> > > While at it switch to use the BIT() macro to describe the bit positions
> > > for the interrupt functions.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > index 585b8b3dcfd8..85e44a00e0fc 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > @@ -115,11 +115,12 @@
> > >  #define VNFC_S_FRAME		(1 << 0)
> > >  
> > >  /* Video n Interrupt Enable Register bits */
> > > -#define VNIE_FIE		(1 << 4)
> > > -#define VNIE_EFE		(1 << 1)
> > > +#define VNIE_FIE		BIT(4)
> > > +#define VNIE_EFE		BIT(1)
> > >  
> > >  /* Video n Interrupt Status Register bits */
> > > -#define VNINTS_FIS		(1 << 4)
> > > +#define VNINTS_FIS		BIT(4)
> > > +#define VNINTS_EFS		BIT(1)
> > >  
> > >  /* Video n Data Mode Register bits */
> > >  #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
> > > @@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > >  static irqreturn_t rvin_irq(int irq, void *data)
> > >  {
> > >  	struct rvin_dev *vin = data;
> > > -	u32 status, vnms;
> > > +	u32 capture, status, vnms;
> > >  	int slot;
> > >  	unsigned int handled = 0;
> > >  	unsigned long flags;
> > > @@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > >  	handled = 1;
> > >  
> > >  	/* Nothing to do if nothing was captured. */
> > > -	if (!(status & VNINTS_FIS))
> > > +	capture = vin->format.field == V4L2_FIELD_NONE ||
> > > +		vin->format.field == V4L2_FIELD_ALTERNATE ?
> > > +		VNINTS_FIS : VNINTS_EFS;
> > 
> > I would find
> > 
> > 	capture = vin->format.field == V4L2_FIELD_NONE ||
> > 		  vin->format.field == V4L2_FIELD_ALTERNATE
> > 		? VNINTS_FIS : VNINTS_EFS;
> > 
> > easier to read, but it could be just me.
> 
> I agree it's easier to read, but my auto style checker for the file will 
> not allow it. And if I make one exception from my format rules I fear 
> the file will soon turn wild. The whole construct is ugly and I hope to 
> be able to remove it all together in follow up work.

Up to you. Fixing the style checker is a big rabbit hole.

> > Do I read it correctly that you use the "Field Interrupt" with
> > V4L2_FIELD_NONE ? That seems a bit weird to me, but maybe I don't get
> > how the hardware operates.
> 
> I agree it's odd, and likely can be improved upon. But it mirrors what 
> is done in the capture setup routine. The "Field Interrupt" is enabled 
> for NONE and ALTERNATE, and the "End Frame Interrupt" is used for the 
> other cases. See the 'progressive' variable in rvin_setup().
> 
> The historical reason for this oddity is that VIN placed to much logic 
> in the kernel driver that should have been up to user-space, specially 
> for Gen2. In this case if the video source was providing ALTERNATE then 
> VIN would automatically default to INTERLACED the output. Some of this 
> have been reworked and fixed over the years, and a lot of it is finally 
> fixed in the Gen2 MC series on the list.
> 
> That work is however not a dependency of this work, and I feel like a 
> broken record, but I have started work to refactor this and make it 
> right. That work is however based on the Gen2 MC series as this allows 
> me to add proper tests for all of this that covers all the different 
> generations.
> 
> I did not want this series to depend on the cleanups and potentially 
> grew to yet another large series that fix lots of unrelated things. I 
> hope it's OK we live with this oddity a little while longer so we can 
> address it properly as a separate thing.

Fine with me.

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

> > > +	if (!(status & capture))
> > >  		goto done;
> > >  
> > >  	/* Nothing to do if not running. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Laurent Pinchart]

On Sun, Jun 15, 2025 at 02:48:39PM +0200, Niklas Söderlund wrote:
> On 2025-06-15 00:23:14 +0300, Laurent Pinchart wrote:
> > On Sat, Jun 14, 2025 at 04:15:43PM +0200, Niklas Söderlund wrote:
> > > The call sites using the interrupt helper functions have all been
> > > reworked to only one for each. Fold echo of them into the only call
> > > sites left.
> > > 
> > > While at it rename the variable holding the current interrupt status to
> > > make the code easier to read.
> > > 
> > > There is no functional change.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
> > >  1 file changed, 6 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > index 5c08ee2c9807..585b8b3dcfd8 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > @@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
> > >  	return 0;
> > >  }
> > >  
> > > -static void rvin_disable_interrupts(struct rvin_dev *vin)
> > > -{
> > > -	rvin_write(vin, 0, VNIE_REG);
> > > -}
> > > -
> > > -static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
> > > -{
> > > -	return rvin_read(vin, VNINTS_REG);
> > > -}
> > > -
> > > -static void rvin_ack_interrupt(struct rvin_dev *vin)
> > > -{
> > > -	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
> > > -}
> > > -
> > >  static bool rvin_capture_active(struct rvin_dev *vin)
> > >  {
> > >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> > > @@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > >  static irqreturn_t rvin_irq(int irq, void *data)
> > >  {
> > >  	struct rvin_dev *vin = data;
> > > -	u32 int_status, vnms;
> > > +	u32 status, vnms;
> > >  	int slot;
> > >  	unsigned int handled = 0;
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&vin->qlock, flags);
> > >  
> > > -	int_status = rvin_get_interrupt_status(vin);
> > > -	if (!int_status)
> > > +	status = rvin_read(vin, VNINTS_REG);
> > > +	if (!status)
> > >  		goto done;
> > >  
> > > -	rvin_ack_interrupt(vin);
> > > +	rvin_write(vin, status, VNINTS_REG);
> > 
> > Actually there is a functional change here. Before this change, if an
> > interrupt occured between reading VNINTS_REG in
> > rvin_get_interrupt_status() and reading it again in
> > rvin_ack_interrupt(), it would get lost. This patch fixes a possible
> > bug.
> 
> Indeed, thanks for pointing it out. I will update the commit message to 
> mention this.

Let me make a proposal.

---
The call sites using the interrupt helper functions have all been
reworked to only one for each. Fold echo of them into the only call
sites left.

This fixes a possible interrupt loss in case an interrupt occurs between
reading VNINTS_REG in rvin_get_interrupt_status() and reading it again
in rvin_ack_interrupt().

While at it rename the variable holding the current interrupt status to
make the code easier to read.
---

Feel free to add a Fixes: tag if applicable (and a Cc:
stable@vger.kernel.org in that case).

> > With an updated commit message,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > >  	handled = 1;
> > >  
> > >  	/* Nothing to do if nothing was captured. */
> > > -	if (!(int_status & VNINTS_FIS))
> > > +	if (!(status & VNINTS_FIS))
> > >  		goto done;
> > >  
> > >  	/* Nothing to do if not running. */
> > > @@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
> > >  	rvin_set_stream(vin, 0);
> > >  
> > >  	/* disable interrupts */
> > > -	rvin_disable_interrupts(vin);
> > > +	rvin_write(vin, 0, VNIE_REG);
> > >  
> > >  	/* Return unprocessed buffers from hardware. */
> > >  	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {

-- 
Regards,

Laurent Pinchart