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