Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

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 
> > > > ---
> > > >  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

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[jacopo mondi]

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.

Thanks
   j

>
> >
> > --
> > Kieran
> >
> >
> > >
> > > Signed-off-by: Niklas Söderlund 
> > > ---
> > >  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


signature.asc
Description: PGP signature

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Hans Verkuil]

On 03/09/2018 04:09 PM, Niklas Söderlund wrote:
> This is an error from when the driver where converted from soc-camera.

where -> was

> There is absolutely no gain to check the state variable two times to be
> extra sure if the hardware is stopped.

I'll wait for v2 before applying this.

Regards,

Hans

> 
> Signed-off-by: Niklas Söderlund 
> ---
>  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");
> 

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

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.

> 
> --
> Kieran
> 
> 
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  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

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Kieran Bingham]

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 ?

(Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
the code)

--
Kieran


> 
> Signed-off-by: Niklas Söderlund 
> ---
>  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");
>