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

[<Eugen.Hristev@microchip.com>]

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 
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.

> 
> 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 ?
I can do that with a prequel patch if it's the case.

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
>