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

[Laurent Pinchart]

Hello,

On Thu, Aug 13, 2020 at 12:44:35PM +0200, Dafna Hirschfeld wrote:
> Am 26.06.20 um 18:58 schrieb Robin Murphy:
> > On 2020-06-26 16:59, Tomasz Figa wrote:
> >> On Fri, Jun 26, 2020 at 5:48 PM Dafna Hirschfeld wrote:
> >>> On 26.06.20 16:03, Tomasz Figa wrote:
> >>>> On Fri, Jun 26, 2020 at 3:32 PM Robin Murphy wrote:
> >>>>> On 2020-06-25 18:42, Dafna Hirschfeld wrote:
> >>>>>> In the stop_streaming callback 'rkisp1_params_vb2_stop_streaming'
> >>>>>> there is no need to release the lock 'config_lock' and then acquire
> >>>>>> it again at each iteration when returning all buffers.
> >>>>>> This is because the stream is about to end and there is no need
> >>>>>> to let the isr access a buffer.
> >>>>>>
> >>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> >>>>>> ---
> >>>>>>     drivers/staging/media/rkisp1/rkisp1-params.c | 7 +------
> >>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> >>>>>> index bf006dbeee2d..5169b02731f1 100644
> >>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> >>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> >>>>>> @@ -1488,19 +1488,13 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
> >>>>>>         /* stop params input firstly */
> >>>>>>         spin_lock_irqsave(&params->config_lock, flags);
> >>>>>>         params->is_streaming = false;
> >>>>>> -     spin_unlock_irqrestore(&params->config_lock, flags);
> >>>>>>
> >>>>>>         for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> >>>>>> -             spin_lock_irqsave(&params->config_lock, flags);
> >>>>>>                 if (!list_empty(&params->params)) {
> >>>>>>                         buf = list_first_entry(&params->params,
> >>>>>>                                                struct rkisp1_buffer, queue);
> >>>>>>                         list_del(&buf->queue);
> >>>>>> -                     spin_unlock_irqrestore(&params->config_lock,
> >>>>>> -                                            flags);
> >>>>>>                 } else {
> >>>>>> -                     spin_unlock_irqrestore(&params->config_lock,
> >>>>>> -                                            flags);
> >>>>>>                         break;
> >>>>>>                 }
> >>>>>
> >>>>> Just skimming through out of idle curiosity I was going to comment that
> >>>>> if you end up with this pattern:
> >>>>>
> >>>>>           if (!x) {
> >>>>>                   //do stuff
> >>>>>           } else {
> >>>>>                   break;
> >>>>>           }
> >>>>>
> >>>>> it would be better as:
> >>>>>
> >>>>>           if (x)
> >>>>>                   break;
> >>>>>           //do stuff
> >>>>>
> >>>>> However I then went and looked at the whole function and frankly it's a
> >>>>> bit of a WTF. As best I could decipher, this whole crazy loop appears to
> >>>>> be a baroque reinvention of:
> >>>>>
> >>>>>           list_for_each_entry_safe(&params->params, ..., buf) {
> >>>>>                   list_del(&buf->queue);
> >>>>>                   vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >>>>>           }
> >>> Hi, indeed this is a much simpler implementation, greping 'return_all_buffers'
> >>> I see that many drivers implement it this way.
> >>> thanks!
> >>>
> >>>>>
> >>>>> (assuming from context that the list should never contain more than
> >>>>> RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)
> >>>>
> >>>> Or if we want to avoid disabling the interrupts for the whole
> >>>> iteration, we could use list_splice() to move all the entries of
> >>>
> >>> But this code runs when userspace asks to stop the streaming so I don't
> >>> think it is important at that stage to allow the interrupts.
> >>
> >> It's generally a good practice to reduce the time spent with
> >> interrupts disabled. Disabling the interrupts prevents the system from
> >> handling external events, including timer interrupts, and scheduling
> >> higher priority tasks, including real time ones. How much the system
> >> runs with interrupts disabled is one of the factors determining the
> >> general system latency.
> > 
> > Right, with the way we handle interrupt affinity on Arm an IRQ can't
> > target multiple CPUs in hardware, so any time spent with IRQs
> > disabled might be preventing other devices' interrupts from being
> > taken even if they're not explicitly affine to the current CPU.
> > 
> > Now that I've looked, it appears that vb2_buffer_done() might end up
> > performing a DMA sync on the buffers, which, if it has to do
> > order-of-megabytes worth of cache maintenance for large frames, is
> > the kind of relatively slow operation that really doesn't want to be
> > done with IRQs disabled (or under a lock at all, ideally) unless it
> > absolutely *has* to be. If the lock is only needed here to protect
> > modifications to the params list itself, then moving the whole list
> > at once to do the cleanup "offline" sounds like a great idea to me.

Ouch.

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

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

-- 
Regards,

Laurent Pinchart