| Message ID | 20211123155443.3705143-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Add check for completed capture before completing buffer | expand |
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 >
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 > >
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
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");
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(+)