[PATCH] rcar-vin: Add check for completed capture before completing buffer

[PATCH] rcar-vin: Add check for completed capture before completing buffer

[Niklas Söderlund]

Before reading which slot was captured to by examining the module status
(VnMS) register, make sure something was captured at all by examining
the interrupt status register (VnINTS).

Failing this a buffer maybe completed before it was captured too.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 25ead9333d0046e7..87ccbdc3d11a0f2d 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -111,6 +111,9 @@
 #define VNIE_FIE		(1 << 4)
 #define VNIE_EFE		(1 << 1)
 
+/* Video n Interrupt Status Register bits */
+#define VNINTS_FIS		(1 << 4)
+
 /* Video n Data Mode Register bits */
 #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
 #define VNDMR_A8BIT_MASK	(0xff << 24)
@@ -1005,6 +1008,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	rvin_ack_interrupt(vin);
 	handled = 1;
 
+	/* Nothing to do if nothing was captured. */
+	if (!(int_status & VNINTS_FIS))
+		goto done;
+
 	/* Nothing to do if capture status is 'STOPPED' */
 	if (vin->state == STOPPED) {
 		vin_dbg(vin, "IRQ while state stopped\n");
-- 
2.34.0

Re: [PATCH] rcar-vin: Add check for completed capture before completing buffer

[Kieran Bingham]

Quoting Niklas Söderlund (2021-11-23 15:54:43)
> Before reading which slot was captured to by examining the module status
> (VnMS) register, make sure something was captured at all by examining
> the interrupt status register (VnINTS).
> 
> Failing this a buffer maybe completed before it was captured too.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 25ead9333d0046e7..87ccbdc3d11a0f2d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -111,6 +111,9 @@
>  #define VNIE_FIE               (1 << 4)
>  #define VNIE_EFE               (1 << 1)
>  
> +/* Video n Interrupt Status Register bits */
> +#define VNINTS_FIS             (1 << 4)
> +
>  /* Video n Data Mode Register bits */
>  #define VNDMR_A8BIT(n)         (((n) & 0xff) << 24)
>  #define VNDMR_A8BIT_MASK       (0xff << 24)
> @@ -1005,6 +1008,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
>         rvin_ack_interrupt(vin);
>         handled = 1;
>  
> +       /* Nothing to do if nothing was captured. */
> +       if (!(int_status & VNINTS_FIS))

Does this deserve a warning or debug print? It sounds like it may be
somewhat spurious or unexpected if it occurs?

--
Kieran


> +               goto done;
> +
>         /* Nothing to do if capture status is 'STOPPED' */
>         if (vin->state == STOPPED) {
>                 vin_dbg(vin, "IRQ while state stopped\n");
> -- 
> 2.34.0
>

Re: [PATCH] rcar-vin: Add check for completed capture before completing buffer

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2021-11-24 22:45:17 +0000, Kieran Bingham wrote:
> Quoting Niklas Söderlund (2021-11-23 15:54:43)
> > Before reading which slot was captured to by examining the module status
> > (VnMS) register, make sure something was captured at all by examining
> > the interrupt status register (VnINTS).
> > 
> > Failing this a buffer maybe completed before it was captured too.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 25ead9333d0046e7..87ccbdc3d11a0f2d 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -111,6 +111,9 @@
> >  #define VNIE_FIE               (1 << 4)
> >  #define VNIE_EFE               (1 << 1)
> >  
> > +/* Video n Interrupt Status Register bits */
> > +#define VNINTS_FIS             (1 << 4)
> > +
> >  /* Video n Data Mode Register bits */
> >  #define VNDMR_A8BIT(n)         (((n) & 0xff) << 24)
> >  #define VNDMR_A8BIT_MASK       (0xff << 24)
> > @@ -1005,6 +1008,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >         rvin_ack_interrupt(vin);
> >         handled = 1;
> >  
> > +       /* Nothing to do if nothing was captured. */
> > +       if (!(int_status & VNINTS_FIS))
> 
> Does this deserve a warning or debug print? It sounds like it may be
> somewhat spurious or unexpected if it occurs?

I don't think so. One can enable more interrupts then the ones we do 
today, for example during debugging capture issues. This check just make 
sure we don't try to process a capture if the interrupt is not related 
to capture ;-)

> 
> --
> Kieran
> 
> 
> > +               goto done;
> > +
> >         /* Nothing to do if capture status is 'STOPPED' */
> >         if (vin->state == STOPPED) {
> >                 vin_dbg(vin, "IRQ while state stopped\n");
> > -- 
> > 2.34.0
> >

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] rcar-vin: Add check for completed capture before completing buffer

[Kieran Bingham]

Quoting Niklas Söderlund (2021-11-25 08:41:34)
> Hi Kieran,
> 
> Thanks for your feedback.
> 
> On 2021-11-24 22:45:17 +0000, Kieran Bingham wrote:
> > Quoting Niklas Söderlund (2021-11-23 15:54:43)
> > > Before reading which slot was captured to by examining the module status
> > > (VnMS) register, make sure something was captured at all by examining
> > > the interrupt status register (VnINTS).
> > > 
> > > Failing this a buffer maybe completed before it was captured too.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-dma.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 25ead9333d0046e7..87ccbdc3d11a0f2d 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -111,6 +111,9 @@
> > >  #define VNIE_FIE               (1 << 4)
> > >  #define VNIE_EFE               (1 << 1)
> > >  
> > > +/* Video n Interrupt Status Register bits */
> > > +#define VNINTS_FIS             (1 << 4)
> > > +
> > >  /* Video n Data Mode Register bits */
> > >  #define VNDMR_A8BIT(n)         (((n) & 0xff) << 24)
> > >  #define VNDMR_A8BIT_MASK       (0xff << 24)
> > > @@ -1005,6 +1008,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > >         rvin_ack_interrupt(vin);
> > >         handled = 1;
> > >  
> > > +       /* Nothing to do if nothing was captured. */
> > > +       if (!(int_status & VNINTS_FIS))
> > 
> > Does this deserve a warning or debug print? It sounds like it may be
> > somewhat spurious or unexpected if it occurs?
> 
> I don't think so. One can enable more interrupts then the ones we do 
> today, for example during debugging capture issues. This check just make 
> sure we don't try to process a capture if the interrupt is not related 
> to capture ;-)

Ok, I see. So it shouldn't occur in current code which doesn't enable
other interrupts though.

I think it adds value/protection so is helpful, so

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


If I were doing this though, I'd move the capture specific handling to
it's own function and guard it explicitly for each possible handler:

	if (int_status & VNINTS_FIS)
		rvin_handle_fis(vin);
	
	if (int_status & VNINTS_FOS)
		rvin_handle_fifo_overflow(vin);
	
	if (int_status & VNINTS_EFS)
		rvin_handle_frame_end(vin);

Then each interrupt handler is distinct, and does not get processed
when it's interrupt isn't raised.

> 
> > 
> > --
> > Kieran
> > 
> > 
> > > +               goto done;
> > > +
> > >         /* Nothing to do if capture status is 'STOPPED' */
> > >         if (vin->state == STOPPED) {
> > >                 vin_dbg(vin, "IRQ while state stopped\n");
> > > -- 
> > > 2.34.0
> > >
> 
> -- 
> Kind Regards,
> Niklas Söderlund