Messages in this thread | | | Date | Tue, 11 Jan 2022 16:53:40 +0100 | Subject | Re: [PATCH v3 07/23] media: atmel: atmel-isc-base: use streaming status when queueing buffers | From | Hans Verkuil <> |
| |
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.
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
| |