On Thu, Oct 20, 2022 at 09:55:22PM +0200, Hans de Goede wrote:
> Instead of using an integer to keep count of how many buffers have
> been handed over to the ISP (buffers_in_css) move buffers handed
> over to the ISP to a new buffers_in_css list_head so that we can
> easily loop over them.
>
> This removes the need for atomisp_flush_video_pipe() to loop over
> all buffers and then (ab)use the state to figure out if they
> were handed over to the ISP.
>
> Since the buffers are now always on a list when owned by the driver
> this also allows the buffer_done path on flush vs normal completion
> to be unified (both now need a list_del()) and this common code can
> now be factored out into a new atomisp_buffer_done() helper.
>
> This is a preparation patch for moving the driver over to
> the videobuf2 framework.
...
> +int atomisp_buffers_in_css(struct atomisp_video_pipe *pipe)
> {
> unsigned long irqflags;
> + struct list_head *pos;
> + int buffers_in_css = 0;
>
> + spin_lock_irqsave(&pipe->irq_lock, irqflags);
>
> + list_for_each(pos, &pipe->buffers_in_css)
> + buffers_in_css++;
> +
> + spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
> +
> + return buffers_in_css;
> +}
Looking at this I come up with the
https://lore.kernel.org/r/20221114112842.38565-1-andriy.shevchenko@xxxxxxxxxxxxxxx
But I think your stuff will be earlier in upstream, so feel free to create
a followup later on.
...
> + vb = list_first_entry_or_null(&pipe->activeq, struct videobuf_buffer, queue);
> + if (vb) {
Wouldn't simply list_empty() work here? (Yes, you would need to have else
branch under spin lock, but codewise seems better to me).
> + list_move_tail(&vb->queue, &pipe->buffers_in_css);
> + vb->state = VIDEOBUF_ACTIVE;
> }
> spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
>
> + if (!vb)
> + return -EINVAL;
--
With Best Regards,
Andy Shevchenko
