All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eugen Hristev <eugen.hristev@microchip.com>
To: <robh+dt@kernel.org>, <linux-media@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<jacopo+renesas@jmondi.org>, <hverkuil-cisco@xs4all.nl>
Cc: <nicolas.ferre@microchip.com>, <sakari.ailus@iki.fi>,
	<laurent.pinchart@ideasonboard.com>,
	Eugen Hristev <eugen.hristev@microchip.com>
Subject: [PATCH v4 05/11] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming
Date: Fri, 21 Jan 2022 15:14:10 +0200	[thread overview]
Message-ID: <20220121131416.603972-6-eugen.hristev@microchip.com> (raw)
In-Reply-To: <20220121131416.603972-1-eugen.hristev@microchip.com>

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.

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 6b482270eb93..f7a88399bd54 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -400,6 +400,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);
 
 	/* Wait until the end of the current frame */
@@ -407,6 +408,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);
 
@@ -1395,10 +1398,6 @@ static void isc_awb_work(struct work_struct *w)
 	u32 min, max;
 	int ret;
 
-	/* streaming is not active anymore */
-	if (!vb2_start_streaming_called(&isc->vb2_vidq))
-		return;
-
 	if (ctrls->hist_stat != HIST_ENABLED)
 		return;
 
@@ -1453,7 +1452,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);
+
+	/*
+	 * 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 (!vb2_start_streaming_called(&isc->vb2_vidq)) {
+		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);
@@ -1532,6 +1548,8 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
 
 		isc_update_awb_ctrls(isc);
 
+		mutex_lock(&isc->awb_mutex);
+
 		if (vb2_start_streaming_called(&isc->vb2_vidq)) {
 			/*
 			 * If we are streaming, we can update profile to
@@ -1546,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);
 
 		/* if we have autowhitebalance on, start histogram procedure */
 		if (ctrls->awb == ISC_WB_AUTO &&
@@ -1738,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);
@@ -1848,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 */
@@ -1927,6 +1949,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 c9234c90ae58..04ea5e340808 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -305,6 +305,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: Eugen Hristev <eugen.hristev@microchip.com>
To: <robh+dt@kernel.org>, <linux-media@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<jacopo+renesas@jmondi.org>, <hverkuil-cisco@xs4all.nl>
Cc: Eugen Hristev <eugen.hristev@microchip.com>,
	sakari.ailus@iki.fi, laurent.pinchart@ideasonboard.com
Subject: [PATCH v4 05/11] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming
Date: Fri, 21 Jan 2022 15:14:10 +0200	[thread overview]
Message-ID: <20220121131416.603972-6-eugen.hristev@microchip.com> (raw)
In-Reply-To: <20220121131416.603972-1-eugen.hristev@microchip.com>

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.

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 6b482270eb93..f7a88399bd54 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -400,6 +400,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);
 
 	/* Wait until the end of the current frame */
@@ -407,6 +408,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);
 
@@ -1395,10 +1398,6 @@ static void isc_awb_work(struct work_struct *w)
 	u32 min, max;
 	int ret;
 
-	/* streaming is not active anymore */
-	if (!vb2_start_streaming_called(&isc->vb2_vidq))
-		return;
-
 	if (ctrls->hist_stat != HIST_ENABLED)
 		return;
 
@@ -1453,7 +1452,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);
+
+	/*
+	 * 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 (!vb2_start_streaming_called(&isc->vb2_vidq)) {
+		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);
@@ -1532,6 +1548,8 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
 
 		isc_update_awb_ctrls(isc);
 
+		mutex_lock(&isc->awb_mutex);
+
 		if (vb2_start_streaming_called(&isc->vb2_vidq)) {
 			/*
 			 * If we are streaming, we can update profile to
@@ -1546,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);
 
 		/* if we have autowhitebalance on, start histogram procedure */
 		if (ctrls->awb == ISC_WB_AUTO &&
@@ -1738,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);
@@ -1848,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 */
@@ -1927,6 +1949,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 c9234c90ae58..04ea5e340808 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -305,6 +305,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

  parent reply	other threads:[~2022-01-21 13:16 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-21 13:14 [PATCH v4 00/11] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-01-21 13:14 ` Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 01/11] media: atmel: atmel-isc: replace 'stop' variable with vb2 calls Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-02-07 10:46   ` Jacopo Mondi
2022-02-07 10:46     ` Jacopo Mondi
2022-02-08 12:50     ` Eugen.Hristev
2022-02-08 12:50       ` Eugen.Hristev
2022-01-21 13:14 ` [PATCH v4 02/11] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 03/11] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-02-07 12:44   ` Jacopo Mondi
2022-02-07 12:44     ` Jacopo Mondi
2022-02-07 13:40     ` Eugen.Hristev
2022-02-07 13:40       ` Eugen.Hristev
2022-02-08  8:59       ` Jacopo Mondi
2022-02-08  8:59         ` Jacopo Mondi
2022-02-08 10:33         ` Eugen.Hristev
2022-02-08 10:33           ` Eugen.Hristev
2022-02-08 19:30           ` Jacopo Mondi
2022-02-08 19:30             ` Jacopo Mondi
2022-02-09  6:59             ` Eugen.Hristev
2022-02-09  6:59               ` Eugen.Hristev
2022-02-10 17:49               ` Jacopo Mondi
2022-02-10 17:49                 ` Jacopo Mondi
2022-02-11 10:32                 ` Eugen.Hristev
2022-02-11 10:32                   ` Eugen.Hristev
2022-02-11 11:55                   ` Jacopo Mondi
2022-02-11 11:55                     ` Jacopo Mondi
2022-02-11 12:32                     ` Eugen.Hristev
2022-02-11 12:32                       ` Eugen.Hristev
2022-02-11 13:23                       ` Jacopo Mondi
2022-02-11 13:23                         ` Jacopo Mondi
2022-01-21 13:14 ` [PATCH v4 04/11] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-01-21 13:14 ` Eugen Hristev [this message]
2022-01-21 13:14   ` [PATCH v4 05/11] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 06/11] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 07/11] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-02-10 18:43   ` Jacopo Mondi
2022-02-10 18:43     ` Jacopo Mondi
2022-02-11 10:38     ` Eugen.Hristev
2022-02-11 10:38       ` Eugen.Hristev
2022-02-11 11:59       ` Jacopo Mondi
2022-02-11 11:59         ` Jacopo Mondi
2022-01-21 13:14 ` [PATCH v4 08/11] dt-bindings: media: microchip,xisc: add bus-width of 14 Eugen Hristev
2022-01-21 13:14   ` [PATCH v4 08/11] dt-bindings: media: microchip, xisc: " Eugen Hristev
2022-02-04 22:59   ` [PATCH v4 08/11] dt-bindings: media: microchip,xisc: " Rob Herring
2022-02-04 22:59     ` Rob Herring
2022-01-21 13:14 ` [PATCH v4 09/11] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 10/11] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2022-01-21 13:14   ` Eugen Hristev
2022-01-21 13:14 ` [PATCH v4 11/11] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
2022-01-21 13:14   ` 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=20220121131416.603972-6-eugen.hristev@microchip.com \
    --to=eugen.hristev@microchip.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jacopo+renesas@jmondi.org \
    --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: link
Be 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.