| Message ID | 20230822200626.1931129-6-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Bugfix series | expand |
Hi Bryan, Thank you for the patch. On Tue, Aug 22, 2023 at 09:06:22PM +0100, Bryan O'Donoghue wrote: > vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in > 17x. > > Fix the vfe_disable_output() logic to no longer be racy and to conform > to the 17x way of quiescing and then resetting the VFE. How about dropping the duplicate function and share a single implementation for the two files ? > Fixes: 4edc8eae715c ("media: camss: Add initial support for VFE hardware version Titan 480") > Cc: stable@vger.kernel.org > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../media/platform/qcom/camss/camss-vfe-480.c | 19 +++---------------- > 1 file changed, 3 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c > index f70aad2e8c237..a64d660abc538 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c > @@ -334,28 +334,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; > } >
On 28/08/2023 18:17, Laurent Pinchart wrote: >> vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in >> 17x. >> >> Fix the vfe_disable_output() logic to no longer be racy and to conform >> to the 17x way of quiescing and then resetting the VFE. > How about dropping the duplicate function and share a single > implementation for the two files ? > Hmm, so I looked at this. In principle I like it. Right now vfe-170 only deals with a single write-master = 0, whereas vfe-480 deals with multiple write-masters. Collapsing down into one place is the right thing to do however, it feels like a larger update to vfe-170 that merits its own series along the lines of "Support multiple write-masters for vfe-17x" or better still "Support virtual channels for vfe-17x" which is what is implied by this. Yet another thing to add to the TODO list here. --- bod
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c index f70aad2e8c237..a64d660abc538 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c @@ -334,28 +334,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; }
vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in 17x. Fix the vfe_disable_output() logic to no longer be racy and to conform to the 17x way of quiescing and then resetting the VFE. Fixes: 4edc8eae715c ("media: camss: Add initial support for VFE hardware version Titan 480") Cc: stable@vger.kernel.org Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../media/platform/qcom/camss/camss-vfe-480.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)