Hi Eugen
On Fri, Oct 22, 2021 at 10:52:34AM +0300, 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>
I cannot really comment on the issue itself, but seeing that isc->stop
is mostly used in negated form (!isc->stop) I suggest to make it
isc->streaming ?
Thanks
j
> ---
> 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 f3e89a892373..6f14cc549543 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -442,12 +442,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);
> }
>
> @@ -1015,7 +1017,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)
> return -EBUSY;
>
> return isc_set_fmt(isc, f);
> @@ -1537,7 +1539,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) {
> /*
> * If we are streaming, we can update profile to
> * have the new settings in place.
> @@ -1553,8 +1555,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);
>
> @@ -1830,6 +1831,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> struct vb2_queue *q = &isc->vb2_vidq;
> int ret = 0;
>
> + isc->stop = true;
> +
> INIT_WORK(&isc->awb_work, isc_awb_work);
>
> ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
> --
> 2.25.1
>
