On 11/01/2022 17:38, Eugen.Hristev@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
>>> ---
>>> 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
>>
>
