[PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Fri Aug 14 07:04:27 EDT 2020
Am 26.06.20 um 16:03 schrieb Tomasz Figa:
> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy <robin.murphy at arm.com> wrote:
>>
>> Hi Dafna,
>>
>> On 2020-06-25 18:42, Dafna Hirschfeld wrote:
>>> In the stop_streaming callback 'rkisp1_params_vb2_stop_streaming'
>>> there is no need to release the lock 'config_lock' and then acquire
>>> it again at each iteration when returning all buffers.
>>> This is because the stream is about to end and there is no need
>>> to let the isr access a buffer.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
>>> ---
>>> drivers/staging/media/rkisp1/rkisp1-params.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> index bf006dbeee2d..5169b02731f1 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> @@ -1488,19 +1488,13 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>> /* stop params input firstly */
>>> spin_lock_irqsave(¶ms->config_lock, flags);
>>> params->is_streaming = false;
>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>>>
>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> - spin_lock_irqsave(¶ms->config_lock, flags);
>>> if (!list_empty(¶ms->params)) {
>>> buf = list_first_entry(¶ms->params,
>>> struct rkisp1_buffer, queue);
>>> list_del(&buf->queue);
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> } else {
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> break;
>>> }
>>
>> Just skimming through out of idle curiosity I was going to comment that
>> if you end up with this pattern:
>>
>> if (!x) {
>> //do stuff
>> } else {
>> break;
>> }
>>
>> it would be better as:
>>
>> if (x)
>> break;
>> //do stuff
>>
>> However I then went and looked at the whole function and frankly it's a
>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
>> be a baroque reinvention of:
>>
>> list_for_each_entry_safe(¶ms->params, ..., buf) {
>> list_del(&buf->queue);
>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> }
>>
>> (assuming from context that the list should never contain more than
>> RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
>
> Or if we want to avoid disabling the interrupts for the whole
> iteration, we could use list_splice() to move all the entries of
Hi, list_splice combines two lists together, I guess you meant list_cut_position
which cut a list into two
Thanks,
Dafna
> params->params to a local list_head under the spinlock, release it and
> then loop over the local head. As a side effect, one could even drop
> list_del() and switch to the non-safe variant of
> list_for_each_entry().
>
> Best regards,
> Tomasz
>
More information about the Linux-rockchip
mailing list