Re: [PATCH v3 07/23] media: atmel: atmel-isc-base: use streaming status when queueing buffers

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/01/2022 17:38, Eugen.Hristev@microchip.com wrote:
> On 1/11/22 5:53 PM, Hans Verkuil wrote:
>> On 13/12/2021 14:49, Eugen Hristev wrote:
>>> During experiments with libcamera, it looks like vb2_is_streaming returns
>>> true before our start streaming is called.
>>> Order of operations is streamon -> queue -> start_streaming
>>> ISC would have started the DMA immediately when a buffer is being added
>>> to the vbqueue if the queue is streaming.
>>> It is more safe to start the DMA after the start streaming of the driver is
>>> called.
>>> Thus, even if vb2queue is streaming, add the buffer to the dma queue of the
>>> driver instead of actually starting the DMA process, if the start streaming
>>> has not been called yet.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>> ---
>>>   drivers/media/platform/atmel/atmel-isc-base.c | 17 ++++++++++-------
>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>> index 26a6090f056c..e6c9071c04f0 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>> @@ -441,12 +441,14 @@ static void isc_buffer_queue(struct vb2_buffer *vb)
>>>        unsigned long flags;
>>>
>>>        spin_lock_irqsave(&isc->dma_queue_lock, flags);
>>> -     if (!isc->cur_frm && list_empty(&isc->dma_queue) &&
>>> -             vb2_is_streaming(vb->vb2_queue)) {
>>> +
>>> +     if (!isc->cur_frm && list_empty(&isc->dma_queue) && !isc->stop) {
>>>                isc->cur_frm = buf;
>>>                isc_start_dma(isc);
>>> -     } else
>>> +     } else {
>>>                list_add_tail(&buf->list, &isc->dma_queue);
>>> +     }
>>> +
>>>        spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
>>>   }
>>
>> Both the old and new code doesn't make a lot of sense.
> 
> Hello Hans,
> 
> I debated this a bit with Laurent when I initially wrote the patch.
> 
> The problem with the current code, or the current way things are 
> working, is the fact that vb2_is_streaming returns true from the moment 
> of streamon.
> The DMA engine of the ISC is being started at the moment of 
> start_streaming, and if buffers are being sent earlier/later, they are 
> just added to the queue and DMA engine is restarted if it finished 
> previously.
> However if frames are added beforehand, like, first streamon, then add 

beforehand -> afterwards?

> some frames, and then later call the start_streaming, ISC will crash. 
> (as I said, DMA engine is prepared at start_streaming).
> To avoid this, I changed the vb2_is_streaming calls to keeping an 
> internal state of the streaming.
> Do you think I should try another different approach to solve this ?
> Perhaps do not prepare the ISC dma engine at start_streaming, but at 
> another place ?
> 
> Laurent do you remember the talk we had about this problem a while back ?
> 
>>
>> buf_queue is only called by vb2 if start_streaming has already been called or is
>> about to be called.
> 
> That's a huge difference for ISC driver as it is today. start_streaming 
> being called afterwards, means the ISC DMA engine is not started (not 
> started streaming), hence a crash.

The start_streaming op is called when streamon is called and if there are
sufficient buffers (>= q->min_buffers_needed). If there are insufficient
buffers at streamon time, then start_streaming is only called when the number
of queued buffers reaches the minimum.

vb2 has three different state checks that you can use:

vb2_is_busy() is true if buffers are allocated. Typically this means that you
can no longer do format changes or anything that might change the buffer
sizes or layout.

vb2_is_streaming() is true if streamon was called. If this is true, then vb2_is_busy()
is also true.

And finally vb2_start_streaming_called() is true if the start_streaming op
was called (i.e. DMA is in progress). If this is true, then vb2_is_streaming is
also true.

Basically vb2_is_streaming() reports the streaming state that userspace sees,
while vb2_start_streaming_called() reports the internal DMA streaming state.

The confusion is probably that you were using vb2_is_streaming instead of
vb2_start_streaming_called.

> 
>>
>> Typically all that the buf_queue op does is to call list_add_tail(&buf->list, &isc->dma_queue);
>> inside the spinlock.
>>
>>>
>>> @@ -1014,7 +1016,7 @@ static int isc_s_fmt_vid_cap(struct file *file, void *priv,
>>>   {
>>>        struct isc_device *isc = video_drvdata(file);
>>>
>>> -     if (vb2_is_streaming(&isc->vb2_vidq))
>>> +     if (!isc->stop)
>>
>> This is weird as well. Normally this calls vb2_is_busy to check if the
>> queue is busy (that really means that buffers are already allocated, so
>> changing the format isn't allowed anymore).
> 
> You think the query to the streaming status makes no sense here hence it 
> should be removed completely ?

Right. And instead check vb2_is_busy().

I actually think that this is something that would fail with v4l2-compliance.

Do you still run v4l2-compliance?

> I can do that with a prequel patch if it's the case.

Probably wise.

Regards,

	Hans

> 
> Thanks for reviewing,
> 
> Eugen
> 
>>
>>>                return -EBUSY;
>>>
>>>        return isc_set_fmt(isc, f);
>>> @@ -1536,7 +1538,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
>>>
>>>                isc_update_awb_ctrls(isc);
>>>
>>> -             if (vb2_is_streaming(&isc->vb2_vidq)) {
>>> +             if (!isc->stop) {
>>
>> Ditto.
>>
>>>                        /*
>>>                         * If we are streaming, we can update profile to
>>>                         * have the new settings in place.
>>> @@ -1552,8 +1554,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
>>>                }
>>>
>>>                /* if we have autowhitebalance on, start histogram procedure */
>>> -             if (ctrls->awb == ISC_WB_AUTO &&
>>> -                 vb2_is_streaming(&isc->vb2_vidq) &&
>>> +             if (ctrls->awb == ISC_WB_AUTO && !isc->stop &&
>>>                    ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
>>>                        isc_set_histogram(isc, true);
>>>
>>> @@ -1829,6 +1830,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>        struct vb2_queue *q = &isc->vb2_vidq;
>>>        int ret = 0;
>>>
>>> +     isc->stop = true;
>>> +
>>
>> I'm really not sure that you need the stop bool at all.
>>
>>>        INIT_WORK(&isc->awb_work, isc_awb_work);
>>>
>>>        ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
>>
>> Regards,
>>
>>          Hans
>>
>