| Message ID | 20200714123832.28011-2-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: fix possible race conditions in capture | expand |
Hi Dafna, On 7/14/20 9:38 AM, Dafna Hirschfeld wrote: > The function 'rkisp1_vb2_buf_queue' sets the next buffer directly > in case the capture is already streaming but no frame yet arrived > from the sensor. This is an optimization that tries to avoid > dropping a frame. > The call atomic_read(&cap->rkisp1->isp.frame_sequence) is used > to check if a frame arrived. Reading the 'frame_sequence' should > be avoided outside irq handlers to avoid race conditions. > > This patch removes this optimization. Dropping of the first > frames can be avoided if userspace queues the buffers before > start streaming. If userspace starts queueing buffers > only after calling 'streamon' he risks frame drops anyway. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/rkisp1-capture.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index 793ec884c894..572b0949c81f 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -743,18 +743,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb) > ispbuf->buff_addr[RKISP1_PLANE_CB]); > > spin_lock_irqsave(&cap->buf.lock, flags); > - > - /* > - * If there's no next buffer assigned, queue this buffer directly > - * as the next buffer, and update the memory interface. > - */ > - if (cap->is_streaming && !cap->buf.next && > - atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) { > - cap->buf.next = ispbuf; > - rkisp1_set_next_buf(cap); Doesn't this mean we'll always drop the first frame? Since the first user buffer will only be configured to the hardware after receiving the first frame? So the first frame will always go to the dummy buffer, no? Thanks Helen > - } else { > - list_add_tail(&ispbuf->queue, &cap->buf.queue); > - } > + list_add_tail(&ispbuf->queue, &cap->buf.queue); > spin_unlock_irqrestore(&cap->buf.lock, flags); > } > >
On 7/14/20 9:48 AM, Helen Koike wrote: > Hi Dafna, > > On 7/14/20 9:38 AM, Dafna Hirschfeld wrote: >> The function 'rkisp1_vb2_buf_queue' sets the next buffer directly >> in case the capture is already streaming but no frame yet arrived >> from the sensor. This is an optimization that tries to avoid >> dropping a frame. >> The call atomic_read(&cap->rkisp1->isp.frame_sequence) is used >> to check if a frame arrived. Reading the 'frame_sequence' should >> be avoided outside irq handlers to avoid race conditions. >> >> This patch removes this optimization. Dropping of the first >> frames can be avoided if userspace queues the buffers before >> start streaming. If userspace starts queueing buffers >> only after calling 'streamon' he risks frame drops anyway. >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/staging/media/rkisp1/rkisp1-capture.c | 13 +------------ >> 1 file changed, 1 insertion(+), 12 deletions(-) >> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >> index 793ec884c894..572b0949c81f 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >> @@ -743,18 +743,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb) >> ispbuf->buff_addr[RKISP1_PLANE_CB]); >> >> spin_lock_irqsave(&cap->buf.lock, flags); >> - >> - /* >> - * If there's no next buffer assigned, queue this buffer directly >> - * as the next buffer, and update the memory interface. >> - */ >> - if (cap->is_streaming && !cap->buf.next && >> - atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) { >> - cap->buf.next = ispbuf; >> - rkisp1_set_next_buf(cap); > > Doesn't this mean we'll always drop the first frame? Since the first user buffer will only be configured to > the hardware after receiving the first frame? So the first frame will always go to the dummy buffer, no? I see this is actually solved in the last patch of this series. Maybe this can be re-order, so this patch is after 4/4. With or without this re-ordering: Acked-by: Helen Koike <helen.koike@collabora.com> Thanks Helen > > Thanks > Helen > >> - } else { >> - list_add_tail(&ispbuf->queue, &cap->buf.queue); >> - } >> + list_add_tail(&ispbuf->queue, &cap->buf.queue); >> spin_unlock_irqrestore(&cap->buf.lock, flags); >> } >> >>
On 7/14/20 12:11 PM, Helen Koike wrote: > > > On 7/14/20 9:48 AM, Helen Koike wrote: >> Hi Dafna, >> >> On 7/14/20 9:38 AM, Dafna Hirschfeld wrote: >>> The function 'rkisp1_vb2_buf_queue' sets the next buffer directly >>> in case the capture is already streaming but no frame yet arrived >>> from the sensor. This is an optimization that tries to avoid >>> dropping a frame. >>> The call atomic_read(&cap->rkisp1->isp.frame_sequence) is used >>> to check if a frame arrived. Reading the 'frame_sequence' should >>> be avoided outside irq handlers to avoid race conditions. >>> >>> This patch removes this optimization. Dropping of the first >>> frames can be avoided if userspace queues the buffers before >>> start streaming. If userspace starts queueing buffers >>> only after calling 'streamon' he risks frame drops anyway. >>> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>> --- >>> drivers/staging/media/rkisp1/rkisp1-capture.c | 13 +------------ >>> 1 file changed, 1 insertion(+), 12 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> index 793ec884c894..572b0949c81f 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> @@ -743,18 +743,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb) >>> ispbuf->buff_addr[RKISP1_PLANE_CB]); >>> >>> spin_lock_irqsave(&cap->buf.lock, flags); >>> - >>> - /* >>> - * If there's no next buffer assigned, queue this buffer directly >>> - * as the next buffer, and update the memory interface. >>> - */ >>> - if (cap->is_streaming && !cap->buf.next && >>> - atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) { >>> - cap->buf.next = ispbuf; >>> - rkisp1_set_next_buf(cap); >> >> Doesn't this mean we'll always drop the first frame? Since the first user buffer will only be configured to >> the hardware after receiving the first frame? So the first frame will always go to the dummy buffer, no? > > I see this is actually solved in the last patch of this series. > > Maybe this can be re-order, so this patch is after 4/4. > > With or without this re-ordering: As discussed throught chat, this is not an issue, since we have a minimum amount of buffers that need to be queued before starting the stream, so we'll never have frame_sequence == -1 (which represents the first frame) with an empty queue, so no patch re-ordering is needed. Regards, Helen > > Acked-by: Helen Koike <helen.koike@collabora.com> > > Thanks > Helen > >> >> Thanks >> Helen >> >>> - } else { >>> - list_add_tail(&ispbuf->queue, &cap->buf.queue); >>> - } >>> + list_add_tail(&ispbuf->queue, &cap->buf.queue); >>> spin_unlock_irqrestore(&cap->buf.lock, flags); >>> } >>> >>>
diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c index 793ec884c894..572b0949c81f 100644 --- a/drivers/staging/media/rkisp1/rkisp1-capture.c +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c @@ -743,18 +743,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb) ispbuf->buff_addr[RKISP1_PLANE_CB]); spin_lock_irqsave(&cap->buf.lock, flags); - - /* - * If there's no next buffer assigned, queue this buffer directly - * as the next buffer, and update the memory interface. - */ - if (cap->is_streaming && !cap->buf.next && - atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) { - cap->buf.next = ispbuf; - rkisp1_set_next_buf(cap); - } else { - list_add_tail(&ispbuf->queue, &cap->buf.queue); - } + list_add_tail(&ispbuf->queue, &cap->buf.queue); spin_unlock_irqrestore(&cap->buf.lock, flags); }
The function 'rkisp1_vb2_buf_queue' sets the next buffer directly in case the capture is already streaming but no frame yet arrived from the sensor. This is an optimization that tries to avoid dropping a frame. The call atomic_read(&cap->rkisp1->isp.frame_sequence) is used to check if a frame arrived. Reading the 'frame_sequence' should be avoided outside irq handlers to avoid race conditions. This patch removes this optimization. Dropping of the first frames can be avoided if userspace queues the buffers before start streaming. If userspace starts queueing buffers only after calling 'streamon' he risks frame drops anyway. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/rkisp1-capture.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)