[3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
diff mbox series

Message ID 20200625174257.22216-4-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • media: staging: rkisp1: various bug fixes in params
Related show

Commit Message

Dafna Hirschfeld June 25, 2020, 5:42 p.m. UTC
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@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Robin Murphy June 26, 2020, 1:32 p.m. UTC | #1
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@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(&params->config_lock, flags);
>   	params->is_streaming = false;
> -	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;
>   		}

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(&params->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)

Robin.

>   
> @@ -1508,6 +1502,7 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>   			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>   		buf = NULL;
>   	}
> +	spin_unlock_irqrestore(&params->config_lock, flags);
>   }
>   
>   static int
>
Tomasz Figa June 26, 2020, 2:03 p.m. UTC | #2
On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy <robin.murphy@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@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(&params->config_lock, flags);
> >       params->is_streaming = false;
> > -     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;
> >               }
>
> 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(&params->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
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
Dafna Hirschfeld June 26, 2020, 3:48 p.m. UTC | #3
On 26.06.20 16:03, Tomasz Figa wrote:
> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy <robin.murphy@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@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(&params->config_lock, flags);
>>>        params->is_streaming = false;
>>> -     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;
>>>                }
>>
>> 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(&params->params, ..., buf) {
>>                  list_del(&buf->queue);
>>                  vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>          }
Hi, indeed this is a much simpler implementation, greping 'return_all_buffers'
I see that many drivers implement it this way.
thanks!

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

But this code runs when userspace asks to stop the streaming so I don't
think it is important at that stage to allow the interrupts.

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
>
Tomasz Figa June 26, 2020, 3:59 p.m. UTC | #4
On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> On 26.06.20 16:03, Tomasz Figa wrote:
> > On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy <robin.murphy@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@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(&params->config_lock, flags);
> >>>        params->is_streaming = false;
> >>> -     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;
> >>>                }
> >>
> >> 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(&params->params, ..., buf) {
> >>                  list_del(&buf->queue);
> >>                  vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >>          }
> Hi, indeed this is a much simpler implementation, greping 'return_all_buffers'
> I see that many drivers implement it this way.
> thanks!
>
> >>
> >> (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
>
> But this code runs when userspace asks to stop the streaming so I don't
> think it is important at that stage to allow the interrupts.

It's generally a good practice to reduce the time spent with
interrupts disabled. Disabling the interrupts prevents the system from
handling external events, including timer interrupts, and scheduling
higher priority tasks, including real time ones. How much the system
runs with interrupts disabled is one of the factors determining the
general system latency.

Best regards,
Tomasz
Robin Murphy June 26, 2020, 4:58 p.m. UTC | #5
On 2020-06-26 16:59, Tomasz Figa wrote:
> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>>
>>
>> On 26.06.20 16:03, Tomasz Figa wrote:
>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy <robin.murphy@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@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(&params->config_lock, flags);
>>>>>         params->is_streaming = false;
>>>>> -     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;
>>>>>                 }
>>>>
>>>> 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(&params->params, ..., buf) {
>>>>                   list_del(&buf->queue);
>>>>                   vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>>           }
>> Hi, indeed this is a much simpler implementation, greping 'return_all_buffers'
>> I see that many drivers implement it this way.
>> thanks!
>>
>>>>
>>>> (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
>>
>> But this code runs when userspace asks to stop the streaming so I don't
>> think it is important at that stage to allow the interrupts.
> 
> It's generally a good practice to reduce the time spent with
> interrupts disabled. Disabling the interrupts prevents the system from
> handling external events, including timer interrupts, and scheduling
> higher priority tasks, including real time ones. How much the system
> runs with interrupts disabled is one of the factors determining the
> general system latency.

Right, with the way we handle interrupt affinity on Arm an IRQ can't 
target multiple CPUs in hardware, so any time spent with IRQs disabled 
might be preventing other devices' interrupts from being taken even if 
they're not explicitly affine to the current CPU.

Now that I've looked, it appears that vb2_buffer_done() might end up 
performing a DMA sync on the buffers, which, if it has to do 
order-of-megabytes worth of cache maintenance for large frames, is the 
kind of relatively slow operation that really doesn't want to be done 
with IRQs disabled (or under a lock at all, ideally) unless it 
absolutely *has* to be. If the lock is only needed here to protect 
modifications to the params list itself, then moving the whole list at 
once to do the cleanup "offline" sounds like a great idea to me.

Robin.

Patch
diff mbox series

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(&params->config_lock, flags);
 	params->is_streaming = false;
-	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;
 		}
 
@@ -1508,6 +1502,7 @@  static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 		buf = NULL;
 	}
+	spin_unlock_irqrestore(&params->config_lock, flags);
 }
 
 static int