| Message ID | 20230822200626.1931129-5-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: qcom: camss: Bugfix series | expand |
Hi Bryan, Thank you for the patch. On Tue, Aug 22, 2023 at 09:06:21PM +0100, Bryan O'Donoghue wrote: > There are two problems with the current vfe_disable_output() routine. > > Firstly we rightly use a spinlock to protect output->gen2.active_num > everywhere except for in the IDLE timeout path of vfe_disable_output(). > Even if that is not racy "in practice" somehow it is by happenstance not > by design. > > Secondly we do not get consistent behaviour from this routine. On > sc8280xp 50% of the time I get "VFE idle timeout - resetting". In this > case the subsequent capture will succeed. The other 50% of the time, we > don't hit the idle timeout, never do the VFE reset and subsequent > captures stall indefinitely. > > Rewrite the vfe_disable_output() routine to > > - Quiesce write masters with vfe_wm_stop() > - Set active_num = 0 > > remembering to hold the spinlock when we do so followed by > > - Reset the VFE > > Testing on sc8280xp and sdm845 shows this to be a valid fix. > > Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170") > Cc: stable@vger.kernel.org > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> I can't comment on the validity of the fix, but nothing shocks me in the patch, so I'm fine with it. > --- > .../media/platform/qcom/camss/camss-vfe-170.c | 19 +++---------------- > 1 file changed, 3 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c > index 02494c89da91c..ae9137633c301 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c > @@ -500,28 +500,15 @@ static int vfe_disable_output(struct vfe_line *line) > struct vfe_output *output = &line->output; > unsigned long flags; > unsigned int i; > - bool done; > - int timeout = 0; > - > - do { > - spin_lock_irqsave(&vfe->output_lock, flags); > - done = !output->gen2.active_num; > - spin_unlock_irqrestore(&vfe->output_lock, flags); > - usleep_range(10000, 20000); Now that you don't sleep anymore, I think you can drop the inclusion of linux/delay.h. > - > - if (timeout++ == 100) { > - dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n"); > - vfe_reset(vfe); > - output->gen2.active_num = 0; > - return 0; > - } > - } while (!done); > > spin_lock_irqsave(&vfe->output_lock, flags); > for (i = 0; i < output->wm_num; i++) > vfe_wm_stop(vfe, output->wm_idx[i]); > + output->gen2.active_num = 0; > spin_unlock_irqrestore(&vfe->output_lock, flags); > > + vfe_reset(vfe); > + > return 0; This function could become void, especially given that its only caller doesn't check the return value. > } > > -- > 2.41.0 >
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c index 02494c89da91c..ae9137633c301 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c @@ -500,28 +500,15 @@ static int vfe_disable_output(struct vfe_line *line) struct vfe_output *output = &line->output; unsigned long flags; unsigned int i; - bool done; - int timeout = 0; - - do { - spin_lock_irqsave(&vfe->output_lock, flags); - done = !output->gen2.active_num; - spin_unlock_irqrestore(&vfe->output_lock, flags); - usleep_range(10000, 20000); - - if (timeout++ == 100) { - dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n"); - vfe_reset(vfe); - output->gen2.active_num = 0; - return 0; - } - } while (!done); spin_lock_irqsave(&vfe->output_lock, flags); for (i = 0; i < output->wm_num; i++) vfe_wm_stop(vfe, output->wm_idx[i]); + output->gen2.active_num = 0; spin_unlock_irqrestore(&vfe->output_lock, flags); + vfe_reset(vfe); + return 0; }
There are two problems with the current vfe_disable_output() routine. Firstly we rightly use a spinlock to protect output->gen2.active_num everywhere except for in the IDLE timeout path of vfe_disable_output(). Even if that is not racy "in practice" somehow it is by happenstance not by design. Secondly we do not get consistent behaviour from this routine. On sc8280xp 50% of the time I get "VFE idle timeout - resetting". In this case the subsequent capture will succeed. The other 50% of the time, we don't hit the idle timeout, never do the VFE reset and subsequent captures stall indefinitely. Rewrite the vfe_disable_output() routine to - Quiesce write masters with vfe_wm_stop() - Set active_num = 0 remembering to hold the spinlock when we do so followed by - Reset the VFE Testing on sc8280xp and sdm845 shows this to be a valid fix. Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170") Cc: stable@vger.kernel.org Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../media/platform/qcom/camss/camss-vfe-170.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)