From: Jacopo Mondi <jacopo@jmondi.org> To: Eugen Hristev <eugen.hristev@microchip.com> Cc: linux-media@vger.kernel.org, robh+dt@kernel.org, laurent.pinchart@ideasonboard.com, sakari.ailus@iki.fi, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, nicolas.ferre@microchip.com Subject: Re: [PATCH v3 17/23] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Date: Wed, 12 Jan 2022 09:58:35 +0100 [thread overview] Message-ID: <20220112085835.twjhxjnigl3uhgqj@uno.localdomain> (raw) In-Reply-To: <20211213134940.324266-18-eugen.hristev@microchip.com> Hi Eugen On Mon, Dec 13, 2021 at 03:49:34PM +0200, 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 isc->stop 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. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > drivers/media/platform/atmel/atmel-isc-base.c | 31 ++++++++++++++++--- > drivers/media/platform/atmel/atmel-isc.h | 1 + > 2 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c > index b0c3ed21f372..53cac1aac0fd 100644 > --- a/drivers/media/platform/atmel/atmel-isc-base.c > +++ b/drivers/media/platform/atmel/atmel-isc-base.c > @@ -401,6 +401,7 @@ static void isc_stop_streaming(struct vb2_queue *vq) > struct isc_buffer *buf; > int ret; > > + mutex_lock(&isc->awb_mutex); > v4l2_ctrl_activate(isc->do_wb_ctrl, false); > > isc->stop = true; > @@ -410,6 +411,8 @@ static void isc_stop_streaming(struct vb2_queue *vq) > v4l2_err(&isc->v4l2_dev, > "Timeout waiting for end of the capture\n"); > > + mutex_unlock(&isc->awb_mutex); > + > /* Disable DMA interrupt */ > regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE); > > @@ -1416,10 +1419,6 @@ static void isc_awb_work(struct work_struct *w) > u32 min, max; > int ret; > > - /* streaming is not active anymore */ > - if (isc->stop) > - return; > - > if (ctrls->hist_stat != HIST_ENABLED) > return; > > @@ -1470,7 +1469,24 @@ static void isc_awb_work(struct work_struct *w) > } > regmap_write(regmap, ISC_HIS_CFG + isc->offsets.his, > hist_id | baysel | ISC_HIS_CFG_RAR); isc_stop_streaming() calls runtime_put and here you access the hw. Feels like it's safer to hold the mutex for the whole duration of the AWB routine ? > + > + /* > + * We have to make sure the streaming has not stopped meanwhile. > + * ISC requires a frame to clock the internal profile update. > + * To avoid issues, lock the sequence with a mutex > + */ > + mutex_lock(&isc->awb_mutex); > + > + /* streaming is not active anymore */ > + if (isc->stop) { > + mutex_unlock(&isc->awb_mutex); > + return; > + }; > + > isc_update_profile(isc); > + > + mutex_unlock(&isc->awb_mutex); > + > /* if awb has been disabled, we don't need to start another histogram */ > if (ctrls->awb) > regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ); > @@ -1549,6 +1565,8 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl) > > isc_update_awb_ctrls(isc); > > + mutex_lock(&isc->awb_mutex); > + > if (!isc->stop) { > /* > * If we are streaming, we can update profile to > @@ -1563,6 +1581,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl) > */ > v4l2_ctrl_activate(isc->do_wb_ctrl, false); > } > + mutex_unlock(&isc->awb_mutex); > > /* if we have autowhitebalance on, start histogram procedure */ > if (ctrls->awb == ISC_WB_AUTO && !isc->stop && > @@ -1754,6 +1773,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); > @@ -1866,6 +1886,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 */ > @@ -1941,6 +1963,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 0b6370d7775f..c2cb805faff3 100644 > --- a/drivers/media/platform/atmel/atmel-isc.h > +++ b/drivers/media/platform/atmel/atmel-isc.h > @@ -307,6 +307,7 @@ struct isc_device { > struct work_struct awb_work; > > struct mutex lock; /* serialize access to file operations */ > + struct mutex awb_mutex; /* serialize access to streaming status from awb work queue */ > spinlock_t awb_lock; /* serialize access to DMA buffers from awb work queue */ > > struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM]; > -- > 2.25.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Jacopo Mondi <jacopo@jmondi.org> To: Eugen Hristev <eugen.hristev@microchip.com> Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, sakari.ailus@iki.fi, laurent.pinchart@ideasonboard.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH v3 17/23] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Date: Wed, 12 Jan 2022 09:58:35 +0100 [thread overview] Message-ID: <20220112085835.twjhxjnigl3uhgqj@uno.localdomain> (raw) In-Reply-To: <20211213134940.324266-18-eugen.hristev@microchip.com> Hi Eugen On Mon, Dec 13, 2021 at 03:49:34PM +0200, 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 isc->stop 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. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > drivers/media/platform/atmel/atmel-isc-base.c | 31 ++++++++++++++++--- > drivers/media/platform/atmel/atmel-isc.h | 1 + > 2 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c > index b0c3ed21f372..53cac1aac0fd 100644 > --- a/drivers/media/platform/atmel/atmel-isc-base.c > +++ b/drivers/media/platform/atmel/atmel-isc-base.c > @@ -401,6 +401,7 @@ static void isc_stop_streaming(struct vb2_queue *vq) > struct isc_buffer *buf; > int ret; > > + mutex_lock(&isc->awb_mutex); > v4l2_ctrl_activate(isc->do_wb_ctrl, false); > > isc->stop = true; > @@ -410,6 +411,8 @@ static void isc_stop_streaming(struct vb2_queue *vq) > v4l2_err(&isc->v4l2_dev, > "Timeout waiting for end of the capture\n"); > > + mutex_unlock(&isc->awb_mutex); > + > /* Disable DMA interrupt */ > regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE); > > @@ -1416,10 +1419,6 @@ static void isc_awb_work(struct work_struct *w) > u32 min, max; > int ret; > > - /* streaming is not active anymore */ > - if (isc->stop) > - return; > - > if (ctrls->hist_stat != HIST_ENABLED) > return; > > @@ -1470,7 +1469,24 @@ static void isc_awb_work(struct work_struct *w) > } > regmap_write(regmap, ISC_HIS_CFG + isc->offsets.his, > hist_id | baysel | ISC_HIS_CFG_RAR); isc_stop_streaming() calls runtime_put and here you access the hw. Feels like it's safer to hold the mutex for the whole duration of the AWB routine ? > + > + /* > + * We have to make sure the streaming has not stopped meanwhile. > + * ISC requires a frame to clock the internal profile update. > + * To avoid issues, lock the sequence with a mutex > + */ > + mutex_lock(&isc->awb_mutex); > + > + /* streaming is not active anymore */ > + if (isc->stop) { > + mutex_unlock(&isc->awb_mutex); > + return; > + }; > + > isc_update_profile(isc); > + > + mutex_unlock(&isc->awb_mutex); > + > /* if awb has been disabled, we don't need to start another histogram */ > if (ctrls->awb) > regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ); > @@ -1549,6 +1565,8 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl) > > isc_update_awb_ctrls(isc); > > + mutex_lock(&isc->awb_mutex); > + > if (!isc->stop) { > /* > * If we are streaming, we can update profile to > @@ -1563,6 +1581,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl) > */ > v4l2_ctrl_activate(isc->do_wb_ctrl, false); > } > + mutex_unlock(&isc->awb_mutex); > > /* if we have autowhitebalance on, start histogram procedure */ > if (ctrls->awb == ISC_WB_AUTO && !isc->stop && > @@ -1754,6 +1773,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); > @@ -1866,6 +1886,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 */ > @@ -1941,6 +1963,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 0b6370d7775f..c2cb805faff3 100644 > --- a/drivers/media/platform/atmel/atmel-isc.h > +++ b/drivers/media/platform/atmel/atmel-isc.h > @@ -307,6 +307,7 @@ struct isc_device { > struct work_struct awb_work; > > struct mutex lock; /* serialize access to file operations */ > + struct mutex awb_mutex; /* serialize access to streaming status from awb work queue */ > spinlock_t awb_lock; /* serialize access to DMA buffers from awb work queue */ > > struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM]; > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-01-12 8:57 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-13 13:49 [PATCH v3 00/23] media: atmel: atmel-isc: implement media controller Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 01/23] MAINTAINERS: add microchip csi2dc Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 02/23] dt-bindings: media: atmel: csi2dc: add bindings for " Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-15 20:16 ` Rob Herring 2021-12-15 20:16 ` Rob Herring 2021-12-13 13:49 ` [PATCH v3 03/23] media: atmel: introduce microchip csi2dc driver Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-11 15:43 ` Jacopo Mondi 2022-01-11 15:43 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 04/23] media: atmel: atmel-isc: split the clock code into separate source file Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-11 15:43 ` Jacopo Mondi 2022-01-11 15:43 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 05/23] media: atmel: atmel-isc: replace video device name with module name Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 06/23] media: atmel: atmel-sama7g5-isc: fix ispck leftover Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 07/23] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-11 15:53 ` Hans Verkuil 2022-01-11 15:53 ` Hans Verkuil 2022-01-11 16:38 ` Eugen.Hristev 2022-01-11 16:38 ` Eugen.Hristev 2022-01-12 9:52 ` Hans Verkuil 2022-01-12 9:52 ` Hans Verkuil 2021-12-13 13:49 ` [PATCH v3 08/23] media: atmel: atmel-isc-base: remove frameintervals VIDIOC Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 09/23] media: atmel: atmel-isc-base: report frame sizes as full supported range Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 10/23] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 11/23] media: atmel: atmel-isc-base: fix bytesperline value for planar formats Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 12/23] media: atmel: atmel-isc: implement media controller Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-11 16:06 ` Jacopo Mondi 2022-01-11 16:06 ` Jacopo Mondi 2022-01-11 16:30 ` Eugen.Hristev 2022-01-11 16:30 ` Eugen.Hristev 2022-01-11 16:51 ` Jacopo Mondi 2022-01-11 16:51 ` Jacopo Mondi 2022-01-11 17:00 ` Eugen.Hristev 2022-01-11 17:00 ` Eugen.Hristev 2021-12-13 13:49 ` [PATCH v3 13/23] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 14/23] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 15/23] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 16/23] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2021-12-13 13:49 ` [PATCH v3 17/23] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 8:58 ` Jacopo Mondi [this message] 2022-01-12 8:58 ` Jacopo Mondi 2022-01-19 15:40 ` Eugen.Hristev 2022-01-19 15:40 ` Eugen.Hristev 2021-12-13 13:49 ` [PATCH v3 18/23] media: atmel: atmel-isc-base: add wb debug messages Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:00 ` Jacopo Mondi 2022-01-12 9:00 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 19/23] media: atmel: atmel-isc-base: clamp wb gain coefficients Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:04 ` Jacopo Mondi 2022-01-12 9:04 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 20/23] media: atmel: atmel-sama7g5-isc: fix UYVY input format mbus_code typo Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:04 ` Jacopo Mondi 2022-01-12 9:04 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 21/23] media: atmel: atmel-isc: add raw Bayer 8bit 10bit output formats Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:05 ` Jacopo Mondi 2022-01-12 9:05 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 22/23] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:06 ` Jacopo Mondi 2022-01-12 9:06 ` Jacopo Mondi 2021-12-13 13:49 ` [PATCH v3 23/23] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev 2021-12-13 13:49 ` Eugen Hristev 2022-01-12 9:21 ` Jacopo Mondi 2022-01-12 9:21 ` Jacopo Mondi 2022-01-20 8:43 ` Eugen.Hristev 2022-01-20 8:43 ` Eugen.Hristev 2022-01-20 8:48 ` Eugen.Hristev 2022-01-20 8:48 ` Eugen.Hristev 2022-01-20 8:58 ` Jacopo Mondi 2022-01-20 8:58 ` Jacopo Mondi 2022-01-12 12:07 ` [PATCH v3 00/23] media: atmel: atmel-isc: implement media controller Hans Verkuil 2022-01-12 12:07 ` Hans Verkuil 2022-01-12 12:46 ` Eugen.Hristev 2022-01-12 12:46 ` Eugen.Hristev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220112085835.twjhxjnigl3uhgqj@uno.localdomain \ --to=jacopo@jmondi.org \ --cc=devicetree@vger.kernel.org \ --cc=eugen.hristev@microchip.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=nicolas.ferre@microchip.com \ --cc=robh+dt@kernel.org \ --cc=sakari.ailus@iki.fi \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.