On 29/04/2022 10:08, Eugen.Hristev@xxxxxxxxxxxxx wrote: > 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 ? I have more comments (just finished the full review), so a v10 is definitely needed. Regards, Hans > > 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]; >> >
