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.
buf_queue is only called by vb2 if start_streaming has already been called or is
about to be 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).
> 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
