Hi,
On 11/14/22 12:53, Andy Shevchenko wrote:
> 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.
That is super useful, thanks. But as you mention it is probably best to
just conver the code here to this alter. I've added this to my atomisp
TODO list.
>
> ...
>
>> + 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).
The problem with that is that the else branch does a "return -EINVAL;"
so then I would need a separate spin_unlock_irqrestore() for the else
branch and I really dislike not having my locks / unlocks cleanly
balanced 1:1.
Regards,
Hans
>
>> + list_move_tail(&vb->queue, &pipe->buffers_in_css);
>> + vb->state = VIDEOBUF_ACTIVE;
>> }
>
>> spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
>>
>> + if (!vb)
>> + return -EINVAL;
>
>
