[PATCH 0/3] media: staging: rkisp1: various bug fixes in params

[PATCH 0/3] media: staging: rkisp1: various bug fixes in params

[Dafna Hirschfeld]

3 bug fixes found in rkisp1-params.c related to managing the vb2 buffers.

Testing:
--------

A test that streams the selfpath and params entities simultaneously
can be found in this branch of v4l-utils:
https://gitlab.collabora.com/dafna/v4l-utils/-/tree/params-bug-fixes

In the repo:
$ cd contrib/test
$ python3 test-rkisp1.py -t RKISP1_TEST_simple_stream_with_params -o /root -S -v --params-buf params-buf.dat

Dafna Hirschfeld (3):
  media: staging: rkisp1: params: don't reference the vb2 buffer after
    calling vb2_buffer_done
  media: staging: rkisp1: params: don't release lock in isr before
    buffer is done
  media: staging: rkisp1: params: in 'stop_streaming' don't release the
    lock while returning buffers

 drivers/staging/media/rkisp1/rkisp1-params.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done

[Dafna Hirschfeld]

The driver should not reference the buffer pointer of vb2_buffer
after calling 'vb2_buffer_done' on that buffer since the call passes
the buffer to userspace.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 797e79de659c..762c2259b807 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1457,9 +1457,9 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
 		new_params = (struct rkisp1_params_cfg *)
 			(vb2_plane_vaddr(vb, 0));
 		vbuf->sequence = frame_sequence;
-		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 		params->is_first_params = false;
 		params->cur_params = *new_params;
+		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 		return;
 	}
 
-- 
2.17.1

[PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Dafna Hirschfeld]

In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
should be held as long as the current buffer is used. Otherwise the
stop_streaming calback might remove it from the list and
pass it to userspace while it is referenced in the irq handler.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 762c2259b807..bf006dbeee2d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
 	if (!list_empty(&params->params))
 		cur_buf = list_first_entry(&params->params,
 					   struct rkisp1_buffer, queue);
-	spin_unlock(&params->config_lock);
 
-	if (!cur_buf)
+	if (!cur_buf) {
+		spin_unlock(&params->config_lock);
 		return;
+	}
 
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
@@ -1228,13 +1229,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
 		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
 		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
 
-		spin_lock(&params->config_lock);
 		list_del(&cur_buf->queue);
-		spin_unlock(&params->config_lock);
-
 		cur_buf->vb.sequence = frame_sequence;
 		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 	}
+	spin_unlock(&params->config_lock);
 }
 
 static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
-- 
2.17.1

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

[Dafna Hirschfeld]

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;
 		}
 
@@ -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
-- 
2.17.1

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

[Robin Murphy]

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
> 

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

[Tomasz Figa]

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

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

[Dafna Hirschfeld]

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
> 

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

[Tomasz Figa]

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

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

[Robin Murphy]

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.

Re: [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done

[Helen Koike]

Hi Dafna,

Thanks for your patch.

On 6/25/20 2:42 PM, Dafna Hirschfeld wrote:
> The driver should not reference the buffer pointer of vb2_buffer
> after calling 'vb2_buffer_done' on that buffer since the call passes
> the buffer to userspace.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 797e79de659c..762c2259b807 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1457,9 +1457,9 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
>  		new_params = (struct rkisp1_params_cfg *)
>  			(vb2_plane_vaddr(vb, 0));
>  		vbuf->sequence = frame_sequence;
> -		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  		params->is_first_params = false;
>  		params->cur_params = *new_params;

Maybe we can remove this new_params variable entirely, and just copy directly to params->cur_params in the begining of the if statement, what do yo think?

With or without this change

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

Thanks
Helen

> +		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  		return;>  	}
>  
> 

Re: [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Helen Koike]

Hi Dafna,

Thanks for the patch

On 6/25/20 2:42 PM, Dafna Hirschfeld wrote:
> In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
> should be held as long as the current buffer is used. Otherwise the
> stop_streaming calback might remove it from the list and
> pass it to userspace while it is referenced in the irq handler.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

I think we need to clarify what 'config_lock' protects, it seems it protects
the is_streaming variable and the buffer list.
I see it being used by rkisp1_params_config_parameter(), which doesn't look right to me.

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 762c2259b807..bf006dbeee2d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
>  	if (!list_empty(&params->params))
>  		cur_buf = list_first_entry(&params->params,
>  					   struct rkisp1_buffer, queue);
> -	spin_unlock(&params->config_lock);
>  
> -	if (!cur_buf)
> +	if (!cur_buf) {
> +		spin_unlock(&params->config_lock);
>  		return;
> +	}
>  
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
> @@ -1228,13 +1229,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
>  		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
>  		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
>  
> -		spin_lock(&params->config_lock);
>  		list_del(&cur_buf->queue);
> -		spin_unlock(&params->config_lock);
> -
>  		cur_buf->vb.sequence = frame_sequence;
>  		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  	}

Maybe we could refactor this whole function, as mentioned by Robin in patch 3/3, we could remove
this identation with:

if (!(isp_mis & RKISP1_CIF_ISP_FRAME))
    return;

Keep in mind that params and stats were barely touched from the original driver, so don't be afraid to refactor things :)

Thanks
Helen


> +	spin_unlock(&params->config_lock);
>  }
>  
>  static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
> 

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

[Dafna Hirschfeld]

Am 26.06.20 um 18:58 schrieb Robin Murphy:
> 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.

ok, that might be a problem in v4l2 in general since vb2_buffer_done is actually often used inside an irq handler

> 
> Robin.

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

[Laurent Pinchart]

Hello,

On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> Am 26.06.20 um 18:58 schrieb Robin Murphy:
> > On 2020-06-26 16:59, Tomasz Figa wrote:
> >> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> >>> On 26.06.20 16:03, Tomasz Figa wrote:
> >>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> >>>>> 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.

Ouch.

> ok, that might be a problem in v4l2 in general since vb2_buffer_done
> is actually often used inside an irq handler

Correct. The DMA sync should be moved to DQBUF time, there shouldn't be
any reason to do it in the IRQ handler. I thought this had already been
fixed :-(

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

On Thu, Aug 13, 2020 at 12:53 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> > Am 26.06.20 um 18:58 schrieb Robin Murphy:
> > > On 2020-06-26 16:59, Tomasz Figa wrote:
> > >> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> > >>> On 26.06.20 16:03, Tomasz Figa wrote:
> > >>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> > >>>>> 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.
>
> Ouch.
>
> > ok, that might be a problem in v4l2 in general since vb2_buffer_done
> > is actually often used inside an irq handler
>
> Correct. The DMA sync should be moved to DQBUF time, there shouldn't be
> any reason to do it in the IRQ handler. I thought this had already been
> fixed :-(

For reference, there was a patch [1] proposed, but it moved the
synchronization to a wrong place in the sequence, already after the
.buf_finish queue callback, ending up breaking the drivers which need
to access the buffer contents there.

[1] https://patchwork.linuxtv.org/project/linux-media/patch/1494255810-12672-4-git-send-email-sakari.ailus@linux.intel.com/

Best regards,
Tomasz

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

[Laurent Pinchart]

Hi Tomasz,

On Thu, Aug 13, 2020 at 02:50:17PM +0200, Tomasz Figa wrote:
> On Thu, Aug 13, 2020 at 12:53 PM Laurent Pinchart wrote:
> > On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> > > Am 26.06.20 um 18:58 schrieb Robin Murphy:
> > > > On 2020-06-26 16:59, Tomasz Figa wrote:
> > > >> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> > > >>> On 26.06.20 16:03, Tomasz Figa wrote:
> > > >>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> > > >>>>> 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.
> >
> > Ouch.
> >
> > > ok, that might be a problem in v4l2 in general since vb2_buffer_done
> > > is actually often used inside an irq handler
> >
> > Correct. The DMA sync should be moved to DQBUF time, there shouldn't be
> > any reason to do it in the IRQ handler. I thought this had already been
> > fixed :-(
> 
> For reference, there was a patch [1] proposed, but it moved the
> synchronization to a wrong place in the sequence, already after the
> .buf_finish queue callback, ending up breaking the drivers which need
> to access the buffer contents there.
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/1494255810-12672-4-git-send-email-sakari.ailus@linux.intel.com/

I think we need to fix the drivers. We just can't do cache sync in IRQ
context by default because a few drivers need to access the buffer
contents. Those drivers should instead deffer access to a work queue,
and sync explicitly. We could possibly provide helpers for that, making
it transparent if a queue flag is set.

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

On Thu, Aug 13, 2020 at 3:02 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Thu, Aug 13, 2020 at 02:50:17PM +0200, Tomasz Figa wrote:
> > On Thu, Aug 13, 2020 at 12:53 PM Laurent Pinchart wrote:
> > > On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> > > > Am 26.06.20 um 18:58 schrieb Robin Murphy:
> > > > > On 2020-06-26 16:59, Tomasz Figa wrote:
> > > > >> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> > > > >>> On 26.06.20 16:03, Tomasz Figa wrote:
> > > > >>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> > > > >>>>> 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.
> > >
> > > Ouch.
> > >
> > > > ok, that might be a problem in v4l2 in general since vb2_buffer_done
> > > > is actually often used inside an irq handler
> > >
> > > Correct. The DMA sync should be moved to DQBUF time, there shouldn't be
> > > any reason to do it in the IRQ handler. I thought this had already been
> > > fixed :-(
> >
> > For reference, there was a patch [1] proposed, but it moved the
> > synchronization to a wrong place in the sequence, already after the
> > .buf_finish queue callback, ending up breaking the drivers which need
> > to access the buffer contents there.
> >
> > [1] https://patchwork.linuxtv.org/project/linux-media/patch/1494255810-12672-4-git-send-email-sakari.ailus@linux.intel.com/
>
> I think we need to fix the drivers. We just can't do cache sync in IRQ
> context by default because a few drivers need to access the buffer
> contents. Those drivers should instead deffer access to a work queue,
> and sync explicitly. We could possibly provide helpers for that, making
> it transparent if a queue flag is set.

The drivers don't access the buffers explicitly from the IRQ. The vb2
queue .buf_finish callback is called at DQBUF time. It was just the
patch mentioned that moved it to a part of DQBUF executed too late.

Best regards,
Tomasz

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

[Laurent Pinchart]

Hi Tomasz,

On Thu, Aug 13, 2020 at 03:05:09PM +0200, Tomasz Figa wrote:
> On Thu, Aug 13, 2020 at 3:02 PM Laurent Pinchart wrote:
> > On Thu, Aug 13, 2020 at 02:50:17PM +0200, Tomasz Figa wrote:
> >> On Thu, Aug 13, 2020 at 12:53 PM Laurent Pinchart wrote:
> >>> On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> >>>> Am 26.06.20 um 18:58 schrieb Robin Murphy:
> >>>>> On 2020-06-26 16:59, Tomasz Figa wrote:
> >>>>>> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> >>>>>>> On 26.06.20 16:03, Tomasz Figa wrote:
> >>>>>>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> >>>>>>>>> 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.
> >>>
> >>> Ouch.
> >>>
> >>>> ok, that might be a problem in v4l2 in general since vb2_buffer_done
> >>>> is actually often used inside an irq handler
> >>>
> >>> Correct. The DMA sync should be moved to DQBUF time, there shouldn't be
> >>> any reason to do it in the IRQ handler. I thought this had already been
> >>> fixed :-(
> >>
> >> For reference, there was a patch [1] proposed, but it moved the
> >> synchronization to a wrong place in the sequence, already after the
> >> .buf_finish queue callback, ending up breaking the drivers which need
> >> to access the buffer contents there.
> >>
> >> [1] https://patchwork.linuxtv.org/project/linux-media/patch/1494255810-12672-4-git-send-email-sakari.ailus@linux.intel.com/
> >
> > I think we need to fix the drivers. We just can't do cache sync in IRQ
> > context by default because a few drivers need to access the buffer
> > contents. Those drivers should instead deffer access to a work queue,
> > and sync explicitly. We could possibly provide helpers for that, making
> > it transparent if a queue flag is set.
> 
> The drivers don't access the buffers explicitly from the IRQ. The vb2
> queue .buf_finish callback is called at DQBUF time. It was just the
> patch mentioned that moved it to a part of DQBUF executed too late.

Aahhh it's better than I thought :-) Shouldn't be too hard to fix then.
Thanks for refreshing my memory.

-- 
Regards,

Laurent Pinchart

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

[Dafna Hirschfeld]

Am 26.06.20 um 16:03 schrieb Tomasz Figa:
> 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

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
> 

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

[Tomasz Figa]

On Fri, Aug 14, 2020 at 1:04 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> Am 26.06.20 um 16:03 schrieb Tomasz Figa:
> > 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
>
> Hi, list_splice combines two lists together, I guess you meant list_cut_position
> which cut a list into two

No, I meant list_splice() and it does the expected thing here. It adds
all the elements from one list (the list argument) to another list
(the head argument). Since an element can't be on two lists at the
same time, the first list is rendered invalid.

Best regards,
Tomasz