Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Helen Koike <helen.koike@xxxxxxxxxxxxx>]

On 8/20/20 6:27 AM, Hans Verkuil wrote:
> On 17/08/2020 23:47, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>>> The code in '.stop_streaming' callback releases and acquire the lock
>>> at each iteration when returning the buffers.
>>> Holding the lock disables interrupts so it should be minimized.
>>> To make the code cleaner and still minimize holding the lock,
>>> the buffer list is first moved to a local list and
>>> then can be iterated without the lock.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
>>
>> lgtm
>>
>> Helen Koike <helen.koike@xxxxxxxxxxxxx>
> 
> Missing 'Acked-by:' tag?

Ops, yes

Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>

> 
> Hans
> 
>>
>> Thanks
>> Helen
>>
>>> ---
>>>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> index 0c2bb2eefb22..6a76c586e916 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>>  {
>>>  	struct rkisp1_params *params = vq->drv_priv;
>>>  	struct rkisp1_buffer *buf;
>>> +	struct list_head tmp_list;
>>>  	unsigned long flags;
>>> -	unsigned int i;
>>>  
>>> -	/* stop params input firstly */
>>> +	INIT_LIST_HEAD(&tmp_list);
>>> +
>>> +	/*
>>> +	 * we first move the buffers into a local list 'tmp_list'
>>> +	 * and then we can iterate it and call vb2_buffer_done
>>> +	 * without holding the lock
>>> +	 */
>>>  	spin_lock_irqsave(&params->config_lock, flags);
>>>  	params->is_streaming = false;
>>> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>>>  	spin_unlock_irqrestore(&params->config_lock, flags);
>>>  
>>> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> -		spin_lock_irqsave(&params->config_lock, flags);
>>> -		if (!list_empty(&params->params)) {
>>> -			buf = list_first_entry(&params->params,
>>> -					       struct rkisp1_buffer, queue);
>>> -			list_del(&buf->queue);
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -		} else {
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -			break;
>>> -		}
>>> -
>>> -		if (buf)
>>> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>> -		buf = NULL;
>>> -	}
>>> +	list_for_each_entry(buf, &tmp_list, queue)
>>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>  }
>>>  
>>>  static int
>>>
>