* [PATCH 0/3] media: staging: rkisp1: various bug fixes in params
@ 2020-06-25 17:42 Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done Dafna Hirschfeld
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-06-25 17:42 UTC (permalink / raw)
To: linux-media, laurent.pinchart
Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done
2020-06-25 17:42 [PATCH 0/3] media: staging: rkisp1: various bug fixes in params Dafna Hirschfeld
@ 2020-06-25 17:42 ` Dafna Hirschfeld
2020-06-26 17:02 ` Helen Koike
2020-06-25 17:42 ` [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers Dafna Hirschfeld
2 siblings, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-06-25 17:42 UTC (permalink / raw)
To: linux-media, laurent.pinchart
Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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(¶ms_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
params->is_first_params = false;
params->cur_params = *new_params;
+ vb2_buffer_done(¶ms_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
return;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done
2020-06-25 17:42 [PATCH 0/3] media: staging: rkisp1: various bug fixes in params Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done Dafna Hirschfeld
@ 2020-06-25 17:42 ` Dafna Hirschfeld
2020-06-26 17:20 ` Helen Koike
2020-06-25 17:42 ` [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers Dafna Hirschfeld
2 siblings, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-06-25 17:42 UTC (permalink / raw)
To: linux-media, laurent.pinchart
Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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(¶ms->params))
cur_buf = list_first_entry(¶ms->params,
struct rkisp1_buffer, queue);
- spin_unlock(¶ms->config_lock);
- if (!cur_buf)
+ if (!cur_buf) {
+ spin_unlock(¶ms->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(¶ms->config_lock);
list_del(&cur_buf->queue);
- spin_unlock(¶ms->config_lock);
-
cur_buf->vb.sequence = frame_sequence;
vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
}
+ spin_unlock(¶ms->config_lock);
}
static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-25 17:42 [PATCH 0/3] media: staging: rkisp1: various bug fixes in params Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done Dafna Hirschfeld
@ 2020-06-25 17:42 ` Dafna Hirschfeld
2020-06-26 13:32 ` Robin Murphy
2 siblings, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-06-25 17:42 UTC (permalink / raw)
To: linux-media, laurent.pinchart
Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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(¶ms->config_lock, flags);
params->is_streaming = false;
- spin_unlock_irqrestore(¶ms->config_lock, flags);
for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
- spin_lock_irqsave(¶ms->config_lock, flags);
if (!list_empty(¶ms->params)) {
buf = list_first_entry(¶ms->params,
struct rkisp1_buffer, queue);
list_del(&buf->queue);
- spin_unlock_irqrestore(¶ms->config_lock,
- flags);
} else {
- spin_unlock_irqrestore(¶ms->config_lock,
- flags);
break;
}
@@ -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(¶ms->config_lock, flags);
}
static int
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-25 17:42 ` [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers Dafna Hirschfeld
@ 2020-06-26 13:32 ` Robin Murphy
2020-06-26 14:03 ` Tomasz Figa
0 siblings, 1 reply; 19+ messages in thread
From: Robin Murphy @ 2020-06-26 13:32 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media, laurent.pinchart
Cc: mchehab, dafna3, tfiga, hverkuil, linux-rockchip, helen.koike,
sakari.ailus, kernel, ezequiel
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(¶ms->config_lock, flags);
> params->is_streaming = false;
> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>
> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> - spin_lock_irqsave(¶ms->config_lock, flags);
> if (!list_empty(¶ms->params)) {
> buf = list_first_entry(¶ms->params,
> struct rkisp1_buffer, queue);
> list_del(&buf->queue);
> - spin_unlock_irqrestore(¶ms->config_lock,
> - flags);
> } else {
> - spin_unlock_irqrestore(¶ms->config_lock,
> - flags);
> break;
> }
Just skimming through out of idle curiosity I was going to comment that
if you end up with this pattern:
if (!x) {
//do stuff
} else {
break;
}
it would be better as:
if (x)
break;
//do stuff
However I then went and looked at the whole function and frankly it's a
bit of a WTF. As best I could decipher, this whole crazy loop appears to
be a baroque reinvention of:
list_for_each_entry_safe(¶ms->params, ..., buf) {
list_del(&buf->queue);
vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
}
(assuming from context that the list should never contain more than
RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
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(¶ms->config_lock, flags);
> }
>
> static int
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 13:32 ` Robin Murphy
@ 2020-06-26 14:03 ` Tomasz Figa
2020-06-26 15:48 ` Dafna Hirschfeld
2020-08-14 11:04 ` Dafna Hirschfeld
0 siblings, 2 replies; 19+ messages in thread
From: Tomasz Figa @ 2020-06-26 14:03 UTC (permalink / raw)
To: Robin Murphy
Cc: Dafna Hirschfeld, Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> > params->is_streaming = false;
> > - spin_unlock_irqrestore(¶ms->config_lock, flags);
> >
> > for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> > - spin_lock_irqsave(¶ms->config_lock, flags);
> > if (!list_empty(¶ms->params)) {
> > buf = list_first_entry(¶ms->params,
> > struct rkisp1_buffer, queue);
> > list_del(&buf->queue);
> > - spin_unlock_irqrestore(¶ms->config_lock,
> > - flags);
> > } else {
> > - spin_unlock_irqrestore(¶ms->config_lock,
> > - flags);
> > break;
> > }
>
> Just skimming through out of idle curiosity I was going to comment that
> if you end up with this pattern:
>
> if (!x) {
> //do stuff
> } else {
> break;
> }
>
> it would be better as:
>
> if (x)
> break;
> //do stuff
>
> However I then went and looked at the whole function and frankly it's a
> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> be a baroque reinvention of:
>
> list_for_each_entry_safe(¶ms->params, ..., buf) {
> list_del(&buf->queue);
> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> }
>
> (assuming from context that the list should never contain more than
> RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
Or if we want to avoid disabling the interrupts for the whole
iteration, we could use list_splice() to move all the entries of
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 14:03 ` Tomasz Figa
@ 2020-06-26 15:48 ` Dafna Hirschfeld
2020-06-26 15:59 ` Tomasz Figa
2020-08-14 11:04 ` Dafna Hirschfeld
1 sibling, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-06-26 15:48 UTC (permalink / raw)
To: Tomasz Figa, Robin Murphy
Cc: Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
>>> params->is_streaming = false;
>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>>>
>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> - spin_lock_irqsave(¶ms->config_lock, flags);
>>> if (!list_empty(¶ms->params)) {
>>> buf = list_first_entry(¶ms->params,
>>> struct rkisp1_buffer, queue);
>>> list_del(&buf->queue);
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> } else {
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> break;
>>> }
>>
>> Just skimming through out of idle curiosity I was going to comment that
>> if you end up with this pattern:
>>
>> if (!x) {
>> //do stuff
>> } else {
>> break;
>> }
>>
>> it would be better as:
>>
>> if (x)
>> break;
>> //do stuff
>>
>> However I then went and looked at the whole function and frankly it's a
>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
>> be a baroque reinvention of:
>>
>> list_for_each_entry_safe(¶ms->params, ..., buf) {
>> list_del(&buf->queue);
>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> }
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 15:48 ` Dafna Hirschfeld
@ 2020-06-26 15:59 ` Tomasz Figa
2020-06-26 16:58 ` Robin Murphy
0 siblings, 1 reply; 19+ messages in thread
From: Tomasz Figa @ 2020-06-26 15:59 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Robin Murphy, Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> >>> params->is_streaming = false;
> >>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> >>>
> >>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> >>> - spin_lock_irqsave(¶ms->config_lock, flags);
> >>> if (!list_empty(¶ms->params)) {
> >>> buf = list_first_entry(¶ms->params,
> >>> struct rkisp1_buffer, queue);
> >>> list_del(&buf->queue);
> >>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>> - flags);
> >>> } else {
> >>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>> - flags);
> >>> break;
> >>> }
> >>
> >> Just skimming through out of idle curiosity I was going to comment that
> >> if you end up with this pattern:
> >>
> >> if (!x) {
> >> //do stuff
> >> } else {
> >> break;
> >> }
> >>
> >> it would be better as:
> >>
> >> if (x)
> >> break;
> >> //do stuff
> >>
> >> However I then went and looked at the whole function and frankly it's a
> >> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> >> be a baroque reinvention of:
> >>
> >> list_for_each_entry_safe(¶ms->params, ..., buf) {
> >> list_del(&buf->queue);
> >> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >> }
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 15:59 ` Tomasz Figa
@ 2020-06-26 16:58 ` Robin Murphy
2020-08-13 10:44 ` Dafna Hirschfeld
0 siblings, 1 reply; 19+ messages in thread
From: Robin Murphy @ 2020-06-26 16:58 UTC (permalink / raw)
To: Tomasz Figa, Dafna Hirschfeld
Cc: Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
>>>>> params->is_streaming = false;
>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>>>>>
>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
>>>>> if (!list_empty(¶ms->params)) {
>>>>> buf = list_first_entry(¶ms->params,
>>>>> struct rkisp1_buffer, queue);
>>>>> list_del(&buf->queue);
>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>>>> - flags);
>>>>> } else {
>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>>>> - flags);
>>>>> break;
>>>>> }
>>>>
>>>> Just skimming through out of idle curiosity I was going to comment that
>>>> if you end up with this pattern:
>>>>
>>>> if (!x) {
>>>> //do stuff
>>>> } else {
>>>> break;
>>>> }
>>>>
>>>> it would be better as:
>>>>
>>>> if (x)
>>>> break;
>>>> //do stuff
>>>>
>>>> However I then went and looked at the whole function and frankly it's a
>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
>>>> be a baroque reinvention of:
>>>>
>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
>>>> list_del(&buf->queue);
>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>> }
>> 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done
2020-06-25 17:42 ` [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done Dafna Hirschfeld
@ 2020-06-26 17:02 ` Helen Koike
0 siblings, 0 replies; 19+ messages in thread
From: Helen Koike @ 2020-06-26 17:02 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media, laurent.pinchart
Cc: ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip,
mchehab, tfiga
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(¶ms_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(¶ms_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> return;> }
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done
2020-06-25 17:42 ` [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done Dafna Hirschfeld
@ 2020-06-26 17:20 ` Helen Koike
0 siblings, 0 replies; 19+ messages in thread
From: Helen Koike @ 2020-06-26 17:20 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media, laurent.pinchart
Cc: ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip,
mchehab, tfiga
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(¶ms->params))
> cur_buf = list_first_entry(¶ms->params,
> struct rkisp1_buffer, queue);
> - spin_unlock(¶ms->config_lock);
>
> - if (!cur_buf)
> + if (!cur_buf) {
> + spin_unlock(¶ms->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(¶ms->config_lock);
> list_del(&cur_buf->queue);
> - spin_unlock(¶ms->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(¶ms->config_lock);
> }
>
> static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 16:58 ` Robin Murphy
@ 2020-08-13 10:44 ` Dafna Hirschfeld
2020-08-13 10:53 ` Laurent Pinchart
0 siblings, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-08-13 10:44 UTC (permalink / raw)
To: Robin Murphy, Tomasz Figa
Cc: Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
>>>>>> params->is_streaming = false;
>>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>>>>>>
>>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
>>>>>> if (!list_empty(¶ms->params)) {
>>>>>> buf = list_first_entry(¶ms->params,
>>>>>> struct rkisp1_buffer, queue);
>>>>>> list_del(&buf->queue);
>>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>>>>> - flags);
>>>>>> } else {
>>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>>>>> - flags);
>>>>>> break;
>>>>>> }
>>>>>
>>>>> Just skimming through out of idle curiosity I was going to comment that
>>>>> if you end up with this pattern:
>>>>>
>>>>> if (!x) {
>>>>> //do stuff
>>>>> } else {
>>>>> break;
>>>>> }
>>>>>
>>>>> it would be better as:
>>>>>
>>>>> if (x)
>>>>> break;
>>>>> //do stuff
>>>>>
>>>>> However I then went and looked at the whole function and frankly it's a
>>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
>>>>> be a baroque reinvention of:
>>>>>
>>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
>>>>> list_del(&buf->queue);
>>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>>> }
>>> 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-13 10:44 ` Dafna Hirschfeld
@ 2020-08-13 10:53 ` Laurent Pinchart
2020-08-13 12:50 ` Tomasz Figa
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2020-08-13 10:53 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Robin Murphy, Tomasz Figa, Linux Media Mailing List,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> >>>>>> params->is_streaming = false;
> >>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> >>>>>>
> >>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> >>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
> >>>>>> if (!list_empty(¶ms->params)) {
> >>>>>> buf = list_first_entry(¶ms->params,
> >>>>>> struct rkisp1_buffer, queue);
> >>>>>> list_del(&buf->queue);
> >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>>>>> - flags);
> >>>>>> } else {
> >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>>>>> - flags);
> >>>>>> break;
> >>>>>> }
> >>>>>
> >>>>> Just skimming through out of idle curiosity I was going to comment that
> >>>>> if you end up with this pattern:
> >>>>>
> >>>>> if (!x) {
> >>>>> //do stuff
> >>>>> } else {
> >>>>> break;
> >>>>> }
> >>>>>
> >>>>> it would be better as:
> >>>>>
> >>>>> if (x)
> >>>>> break;
> >>>>> //do stuff
> >>>>>
> >>>>> However I then went and looked at the whole function and frankly it's a
> >>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> >>>>> be a baroque reinvention of:
> >>>>>
> >>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
> >>>>> list_del(&buf->queue);
> >>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >>>>> }
> >>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-13 10:53 ` Laurent Pinchart
@ 2020-08-13 12:50 ` Tomasz Figa
2020-08-13 13:02 ` Laurent Pinchart
0 siblings, 1 reply; 19+ messages in thread
From: Tomasz Figa @ 2020-08-13 12:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Dafna Hirschfeld, Robin Murphy, Linux Media Mailing List,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> > >>>>>> params->is_streaming = false;
> > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> > >>>>>>
> > >>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> > >>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
> > >>>>>> if (!list_empty(¶ms->params)) {
> > >>>>>> buf = list_first_entry(¶ms->params,
> > >>>>>> struct rkisp1_buffer, queue);
> > >>>>>> list_del(&buf->queue);
> > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > >>>>>> - flags);
> > >>>>>> } else {
> > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > >>>>>> - flags);
> > >>>>>> break;
> > >>>>>> }
> > >>>>>
> > >>>>> Just skimming through out of idle curiosity I was going to comment that
> > >>>>> if you end up with this pattern:
> > >>>>>
> > >>>>> if (!x) {
> > >>>>> //do stuff
> > >>>>> } else {
> > >>>>> break;
> > >>>>> }
> > >>>>>
> > >>>>> it would be better as:
> > >>>>>
> > >>>>> if (x)
> > >>>>> break;
> > >>>>> //do stuff
> > >>>>>
> > >>>>> However I then went and looked at the whole function and frankly it's a
> > >>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> > >>>>> be a baroque reinvention of:
> > >>>>>
> > >>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
> > >>>>> list_del(&buf->queue);
> > >>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > >>>>> }
> > >>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-13 12:50 ` Tomasz Figa
@ 2020-08-13 13:02 ` Laurent Pinchart
2020-08-13 13:05 ` Tomasz Figa
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2020-08-13 13:02 UTC (permalink / raw)
To: Tomasz Figa
Cc: Dafna Hirschfeld, Robin Murphy, Linux Media Mailing List,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> > > >>>>>> params->is_streaming = false;
> > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> > > >>>>>>
> > > >>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> > > >>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
> > > >>>>>> if (!list_empty(¶ms->params)) {
> > > >>>>>> buf = list_first_entry(¶ms->params,
> > > >>>>>> struct rkisp1_buffer, queue);
> > > >>>>>> list_del(&buf->queue);
> > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > > >>>>>> - flags);
> > > >>>>>> } else {
> > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > > >>>>>> - flags);
> > > >>>>>> break;
> > > >>>>>> }
> > > >>>>>
> > > >>>>> Just skimming through out of idle curiosity I was going to comment that
> > > >>>>> if you end up with this pattern:
> > > >>>>>
> > > >>>>> if (!x) {
> > > >>>>> //do stuff
> > > >>>>> } else {
> > > >>>>> break;
> > > >>>>> }
> > > >>>>>
> > > >>>>> it would be better as:
> > > >>>>>
> > > >>>>> if (x)
> > > >>>>> break;
> > > >>>>> //do stuff
> > > >>>>>
> > > >>>>> However I then went and looked at the whole function and frankly it's a
> > > >>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> > > >>>>> be a baroque reinvention of:
> > > >>>>>
> > > >>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
> > > >>>>> list_del(&buf->queue);
> > > >>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > > >>>>> }
> > > >>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-13 13:02 ` Laurent Pinchart
@ 2020-08-13 13:05 ` Tomasz Figa
2020-08-13 13:09 ` Laurent Pinchart
0 siblings, 1 reply; 19+ messages in thread
From: Tomasz Figa @ 2020-08-13 13:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Dafna Hirschfeld, Robin Murphy, Linux Media Mailing List,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> > > > >>>>>> params->is_streaming = false;
> > > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> > > > >>>>>>
> > > > >>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> > > > >>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
> > > > >>>>>> if (!list_empty(¶ms->params)) {
> > > > >>>>>> buf = list_first_entry(¶ms->params,
> > > > >>>>>> struct rkisp1_buffer, queue);
> > > > >>>>>> list_del(&buf->queue);
> > > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > > > >>>>>> - flags);
> > > > >>>>>> } else {
> > > > >>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> > > > >>>>>> - flags);
> > > > >>>>>> break;
> > > > >>>>>> }
> > > > >>>>>
> > > > >>>>> Just skimming through out of idle curiosity I was going to comment that
> > > > >>>>> if you end up with this pattern:
> > > > >>>>>
> > > > >>>>> if (!x) {
> > > > >>>>> //do stuff
> > > > >>>>> } else {
> > > > >>>>> break;
> > > > >>>>> }
> > > > >>>>>
> > > > >>>>> it would be better as:
> > > > >>>>>
> > > > >>>>> if (x)
> > > > >>>>> break;
> > > > >>>>> //do stuff
> > > > >>>>>
> > > > >>>>> However I then went and looked at the whole function and frankly it's a
> > > > >>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> > > > >>>>> be a baroque reinvention of:
> > > > >>>>>
> > > > >>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
> > > > >>>>> list_del(&buf->queue);
> > > > >>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > > > >>>>> }
> > > > >>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-13 13:05 ` Tomasz Figa
@ 2020-08-13 13:09 ` Laurent Pinchart
0 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2020-08-13 13:09 UTC (permalink / raw)
To: Tomasz Figa
Cc: Dafna Hirschfeld, Robin Murphy, Linux Media Mailing List,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> >>>>>>>>>> params->is_streaming = false;
> >>>>>>>>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> >>>>>>>>>>
> >>>>>>>>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> >>>>>>>>>> - spin_lock_irqsave(¶ms->config_lock, flags);
> >>>>>>>>>> if (!list_empty(¶ms->params)) {
> >>>>>>>>>> buf = list_first_entry(¶ms->params,
> >>>>>>>>>> struct rkisp1_buffer, queue);
> >>>>>>>>>> list_del(&buf->queue);
> >>>>>>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>>>>>>>>> - flags);
> >>>>>>>>>> } else {
> >>>>>>>>>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>>>>>>>>> - flags);
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> Just skimming through out of idle curiosity I was going to comment that
> >>>>>>>>> if you end up with this pattern:
> >>>>>>>>>
> >>>>>>>>> if (!x) {
> >>>>>>>>> //do stuff
> >>>>>>>>> } else {
> >>>>>>>>> break;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> it would be better as:
> >>>>>>>>>
> >>>>>>>>> if (x)
> >>>>>>>>> break;
> >>>>>>>>> //do stuff
> >>>>>>>>>
> >>>>>>>>> However I then went and looked at the whole function and frankly it's a
> >>>>>>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> >>>>>>>>> be a baroque reinvention of:
> >>>>>>>>>
> >>>>>>>>> list_for_each_entry_safe(¶ms->params, ..., buf) {
> >>>>>>>>> list_del(&buf->queue);
> >>>>>>>>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >>>>>>>>> }
> >>>>>>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-06-26 14:03 ` Tomasz Figa
2020-06-26 15:48 ` Dafna Hirschfeld
@ 2020-08-14 11:04 ` Dafna Hirschfeld
2020-08-14 11:23 ` Tomasz Figa
1 sibling, 1 reply; 19+ messages in thread
From: Dafna Hirschfeld @ 2020-08-14 11:04 UTC (permalink / raw)
To: Tomasz Figa, Robin Murphy
Cc: Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
>>> params->is_streaming = false;
>>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
>>>
>>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> - spin_lock_irqsave(¶ms->config_lock, flags);
>>> if (!list_empty(¶ms->params)) {
>>> buf = list_first_entry(¶ms->params,
>>> struct rkisp1_buffer, queue);
>>> list_del(&buf->queue);
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> } else {
>>> - spin_unlock_irqrestore(¶ms->config_lock,
>>> - flags);
>>> break;
>>> }
>>
>> Just skimming through out of idle curiosity I was going to comment that
>> if you end up with this pattern:
>>
>> if (!x) {
>> //do stuff
>> } else {
>> break;
>> }
>>
>> it would be better as:
>>
>> if (x)
>> break;
>> //do stuff
>>
>> However I then went and looked at the whole function and frankly it's a
>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
>> be a baroque reinvention of:
>>
>> list_for_each_entry_safe(¶ms->params, ..., buf) {
>> list_del(&buf->queue);
>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> }
>>
>> (assuming from context that the list should never contain more than
>> RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
>
> Or if we want to avoid disabling the interrupts for the whole
> iteration, we could use list_splice() to move all the entries of
Hi, list_splice combines two lists together, I guess you meant list_cut_position
which cut a list into two
Thanks,
Dafna
> params->params to a local list_head under the spinlock, release it and
> then loop over the local head. As a side effect, one could even drop
> list_del() and switch to the non-safe variant of
> list_for_each_entry().
>
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers
2020-08-14 11:04 ` Dafna Hirschfeld
@ 2020-08-14 11:23 ` Tomasz Figa
0 siblings, 0 replies; 19+ messages in thread
From: Tomasz Figa @ 2020-08-14 11:23 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Robin Murphy, Linux Media Mailing List, Laurent Pinchart,
Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
open list:ARM/Rockchip SoC...,
Helen Koike, Sakari Ailus, kernel, Ezequiel Garcia
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(¶ms->config_lock, flags);
> >>> params->is_streaming = false;
> >>> - spin_unlock_irqrestore(¶ms->config_lock, flags);
> >>>
> >>> for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> >>> - spin_lock_irqsave(¶ms->config_lock, flags);
> >>> if (!list_empty(¶ms->params)) {
> >>> buf = list_first_entry(¶ms->params,
> >>> struct rkisp1_buffer, queue);
> >>> list_del(&buf->queue);
> >>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>> - flags);
> >>> } else {
> >>> - spin_unlock_irqrestore(¶ms->config_lock,
> >>> - flags);
> >>> break;
> >>> }
> >>
> >> Just skimming through out of idle curiosity I was going to comment that
> >> if you end up with this pattern:
> >>
> >> if (!x) {
> >> //do stuff
> >> } else {
> >> break;
> >> }
> >>
> >> it would be better as:
> >>
> >> if (x)
> >> break;
> >> //do stuff
> >>
> >> However I then went and looked at the whole function and frankly it's a
> >> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> >> be a baroque reinvention of:
> >>
> >> list_for_each_entry_safe(¶ms->params, ..., buf) {
> >> list_del(&buf->queue);
> >> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >> }
> >>
> >> (assuming from context that the list should never contain more than
> >> RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
> >
> > Or if we want to avoid disabling the interrupts for the whole
> > iteration, we could use list_splice() to move all the entries of
>
> Hi, list_splice combines two lists together, I guess you meant list_cut_position
> which cut a list into two
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
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-08-14 11:23 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 17:42 [PATCH 0/3] media: staging: rkisp1: various bug fixes in params Dafna Hirschfeld
2020-06-25 17:42 ` [PATCH 1/3] media: staging: rkisp1: params: don't reference the vb2 buffer after calling vb2_buffer_done Dafna Hirschfeld
2020-06-26 17:02 ` Helen Koike
2020-06-25 17:42 ` [PATCH 2/3] media: staging: rkisp1: params: don't release lock in isr before buffer is done Dafna Hirschfeld
2020-06-26 17:20 ` Helen Koike
2020-06-25 17:42 ` [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers Dafna Hirschfeld
2020-06-26 13:32 ` Robin Murphy
2020-06-26 14:03 ` Tomasz Figa
2020-06-26 15:48 ` Dafna Hirschfeld
2020-06-26 15:59 ` Tomasz Figa
2020-06-26 16:58 ` Robin Murphy
2020-08-13 10:44 ` Dafna Hirschfeld
2020-08-13 10:53 ` Laurent Pinchart
2020-08-13 12:50 ` Tomasz Figa
2020-08-13 13:02 ` Laurent Pinchart
2020-08-13 13:05 ` Tomasz Figa
2020-08-13 13:09 ` Laurent Pinchart
2020-08-14 11:04 ` Dafna Hirschfeld
2020-08-14 11:23 ` Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).