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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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?

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
>>