Hi Jacopo,
On 2018-03-14 16:17:33 +0100, Jacopo Mondi wrote:
> Hi Niklas, Kieran,
>
> On Tue, Mar 13, 2018 at 06:56:54PM +0100, Niklas Söderlund wrote:
> > Hi Kieran,
> >
> > Thanks for your feedback.
> >
> > On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> > > Hi Niklas,
> > >
> > > Thanks for the patch series :) - I've been looking forward to seeing this one !
> > >
> > > On 10/03/18 01:09, Niklas Söderlund wrote:
> > > > This is an error from when the driver where converted from soc-camera.
> > > > There is absolutely no gain to check the state variable two times to be
> > > > extra sure if the hardware is stopped.
> > >
> > > I'll not say this isn't a redundant check ... but isn't the check against two
> > > different states, and thus the remaining check doesn't actually catch the case
> > > now where state == STOPPED ?
> >
> > Thanks for noticing this, you are correct. I think I need to refresh my
> > glasses subscription after missing this :-)
> >
> > >
> > > (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> > > the code)
> >
> > I will respin this in a v2 and either use state != RUNNING or at least
> > combine the two checks to prevent future embarrassing mistakes like
> > this.
>
> I am sorry I have missed this comment, but I think your patch has some
> merits. Ofc no need to hold on v2 of this series for this, but still I
> think you can re-propose this later (and I didn't get from
> your commit message you were confusing STOPPED/STOPPING).
>
> In rvin_stop_streaming(), you enter STOPPING state, disable the
> interface cleaning ME bit in VnMC and single/continuous capture mode
> in VnFC, and then poll on CA bit of VnMS until the VIN peripheral has
> not been stopped, at this point you set interface state to STOPPED.
>
> As you loop you can still receive interrupts, as you are releasing the
> spinlock when sleeping before testing the ME bit again, so it's fine
> checking for STOPPING state in irq handler.
> It seems to me though, that once you enter STOPPED state, you are sure the
> peripheral has stopped and you should not receive any more interrupt, spurious
> ones apart or when the peripheral fails to stop at all, but things went
> south already at that point.
>
> Again no need to have this part of this series, but you may want to
> take into consideration this for the future, as with this change you can
> remove the STOPPED state at all from the driver.
You are correct.
This patch was extracted from another series I plan to post after the
VIN Gen3 beast is done. The aim of that series is to add SEQ_TB/BT
support to VIN and to do so another state STARTING is needed to handle
the first few fields. But to avoid growing that series too large I
thought I could get away with adding this cleanup to this series which
cleans up the interrupt handler. So this patch will comeback in some
form when I post that series :-)
But for now I'm happy to drop it as the performance gain with this
patch-set applied are so nice when running into buffer starved
situations.
>
> Thanks
> j
>
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > > ---
> > > > drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> > > > 1 file changed, 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > > > rvin_ack_interrupt(vin);
> > > > handled = 1;
> > > >
> > > > - /* Nothing to do if capture status is 'STOPPED' */
> > > > - if (vin->state == STOPPED) {
> > > > - vin_dbg(vin, "IRQ while state stopped\n");
> > > > - goto done;
> > > > - }
> > > > -
> > > > /* Nothing to do if capture status is 'STOPPING' */
> > > > if (vin->state == STOPPING) {
> > > > vin_dbg(vin, "IRQ while state stopping\n");
> > > >
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
