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

[Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxxxx>


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