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:

quoted

Quoting Niklas Söderlund (2021-11-23 15:54:43)

quoted

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.

quoted

--
Kieran

quoted

+               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