On 4/29/22 10:41 AM, Hans Verkuil wrote: > Hi Eugen, > > On 10/03/2022 10:51, Eugen Hristev wrote: >> The AWB workqueue runs in a kernel thread and needs to be synchronized >> w.r.t. the streaming status. >> It is possible that streaming is stopped while the AWB workq is running. >> In this case it is likely that the check for vb2_start_streaming_called is done >> at one point in time, but the AWB computations are done later, including a call >> to isc_update_profile, which requires streaming to be started. >> Thus , isc_update_profile will fail if during this operation sequence the >> streaming was stopped. >> To solve this issue, a mutex is added, that will serialize the awb work and >> streaming stopping, with the mention that either streaming is stopped >> completely including termination of the last frame is done, and after that >> the AWB work can check stream status and stop; either first AWB work is >> completed and after that the streaming can stop correctly. >> The awb spin lock cannot be used since this spinlock is taken in the same >> context and using it in the stop streaming will result in a recursion BUG. > > Please keep the line length in a commit log to no more than 75. > >> >> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx> >> --- >> drivers/media/platform/atmel/atmel-isc-base.c | 29 ++++++++++++++++--- >> drivers/media/platform/atmel/atmel-isc.h | 2 ++ >> 2 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c > > <snip> > >> @@ -1548,6 +1564,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl) >> */ >> v4l2_ctrl_activate(isc->do_wb_ctrl, false); >> } >> + mutex_unlock(&isc->awb_mutex); > > Huh? What is this unlock doing here? Am I missing something? Hello Hans, Sorry. It looks like the corresponding mutex_lock was lost when I rebased the series over the patch to use 'is_streaming' instead of 'stop' variable. In version 3, the mutex_lock was there : https://www.spinics.net/lists/linux-media/msg204059.html Somehow a race problem did not occur in this specific critical area in my tests. I will resend this patch as a v10 , or the whole series if you have other comments on the other patches? How would you prefer ? Thanks, Eugen > > Regards, > > Hans > >> >> /* if we have autowhitebalance on, start histogram procedure */ >> if (ctrls->awb == ISC_WB_AUTO && >> @@ -1740,6 +1757,7 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier, >> { >> struct isc_device *isc = container_of(notifier->v4l2_dev, >> struct isc_device, v4l2_dev); >> + mutex_destroy(&isc->awb_mutex); >> cancel_work_sync(&isc->awb_work); >> video_unregister_device(&isc->video_dev); >> v4l2_ctrl_handler_free(&isc->ctrls.handler); >> @@ -1850,6 +1868,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier) >> isc->current_subdev = container_of(notifier, >> struct isc_subdev_entity, notifier); >> mutex_init(&isc->lock); >> + mutex_init(&isc->awb_mutex); >> + >> init_completion(&isc->comp); >> >> /* Initialize videobuf2 queue */ >> @@ -1930,6 +1950,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier) >> video_unregister_device(vdev); >> >> isc_async_complete_err: >> + mutex_destroy(&isc->awb_mutex); >> mutex_destroy(&isc->lock); >> return ret; >> } >> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h >> index 9cc69c3ae26d..f98f25a55e73 100644 >> --- a/drivers/media/platform/atmel/atmel-isc.h >> +++ b/drivers/media/platform/atmel/atmel-isc.h >> @@ -229,6 +229,7 @@ enum isc_scaler_pads { >> * >> * @lock: lock for serializing userspace file operations >> * with ISC operations >> + * @awb_mutex: serialize access to streaming status from awb work queue >> * @awb_lock: lock for serializing awb work queue operations >> * with DMA/buffer operations >> * >> @@ -307,6 +308,7 @@ struct isc_device { >> struct work_struct awb_work; >> >> struct mutex lock; >> + struct mutex awb_mutex; >> spinlock_t awb_lock; >> >> struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM]; >
