Re: [PATCH v2 06/17] media: atomisp: Also track buffers in a list when submitted to the ISP

[Andy Shevchenko <andy@kernel.org>]

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@linux.intel.com

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