| Message ID | 20200625174257.22216-4-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: various bug fixes in params | expand |
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 >
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
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 >
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
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.
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.
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 :-(
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
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.
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
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.
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 >
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
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
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(-)