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@xxxxxxxxxxxxx>
> >>>>>>>>>> ---
> >>>>>>>>>> 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@xxxxxxxxxxxxxxx/
> >
> > 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
