[PATCH v2 0/4] media: staging: rkisp1: move stats reading to irq handler

[PATCH v2 0/4] media: staging: rkisp1: move stats reading to irq handler

[Dafna Hirschfeld]

Currently reading the stats is done with workqueue.
We decided to move the reading of the stats to the hard irq handler
since it seems and also tested to be fast enough.

The patchset was tested on a Scarlet device with chromeos
and also with the 'cam' command from libcamera in rockpi4 board.

This fixes the TODO item:
'Use threaded interrupt for rkisp1_stats_isr(), remove work queue.'

Patchset Summary:
1. Replace a long bitwise-or of the statistics flags with a macro to improve readability
2. In the 'stop_streaming' callback, replace the usage of 'spin_lock_irqsave' with 'spin_lock_irq'
3. Replace two locks in the rkisp1_stats object with one lock that
protects the 'is_streaming' variable and the 'stat' list of buffers.
4. Move the reading of the stats to the hard irq handler.

Changes since v1:
- patch 1 from v1 "return IRQ_NONE in isr when irq isn't for ISP"
is not needed anymore for this version, it can be sent as a separate patch.
- patch 3 from v1 "stats: use spin_lock_irqsave for irq_lock" is not needed,
since it is enough to use 'spin_lock' in a isr. Instead, patch 2 in this version
is added that replaces 'spin_lock_irqsave' with 'spin_lock_irq' in user context callback.
- the last patch in this version moves the stats reading to the hard irq while
in v1 it was move to threaded irq.
- removing the item 'Use threaded interrupt for rkisp1_stats_isr(), remove work queue.'
from the TODO file.

Dafna Hirschfeld (4):
  media: staging: rkisp1: use a macro for the statistics flags mask
  media: staging: rkisp1: stats: replace spin_lock_irqsave with
    spin_lock_irq
  media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with
    one lock
  media: staging: rkisp1: stats: read the stats in the isr

 drivers/staging/media/rkisp1/TODO            |   1 -
 drivers/staging/media/rkisp1/rkisp1-common.h |  12 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c    |   5 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 117 +++----------------
 4 files changed, 23 insertions(+), 112 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] media: staging: rkisp1: use a macro for the statistics flags mask

[Dafna Hirschfeld]

The mask of the ready statistics flags is used in
several places in the code using bitwise-or.
Use a macro for that to make the code more readable.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  4 ++++
 drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 +----
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 14 +++-----------
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 0c4fe503adc9..d2c669091fae 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -42,6 +42,10 @@
 
 #define RKISP1_MAX_BUS_CLK	8
 
+#define RKISP1_STATS_MEAS_MASK		(RKISP1_CIF_ISP_AWB_DONE |	\
+					 RKISP1_CIF_ISP_AFM_FIN |	\
+					 RKISP1_CIF_ISP_EXP_END |	\
+					 RKISP1_CIF_ISP_HIST_MEASURE_RDY)
 enum rkisp1_rsz_pad {
 	RKISP1_RSZ_PAD_SINK,
 	RKISP1_RSZ_PAD_SRC,
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 6763379d6a65..47ea7cd999eb 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1138,10 +1138,7 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 
 		/* New frame from the sensor received */
 		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
-		if (isp_ris & (RKISP1_CIF_ISP_AWB_DONE |
-			       RKISP1_CIF_ISP_AFM_FIN |
-			       RKISP1_CIF_ISP_EXP_END |
-			       RKISP1_CIF_ISP_HIST_MEASURE_RDY))
+		if (isp_ris & RKISP1_STATS_MEAS_MASK)
 			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
 	}
 
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 0616793ae395..b19a6d9cdd4d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -396,26 +396,18 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	struct rkisp1_device *rkisp1 = stats->rkisp1;
 	struct rkisp1_isp_readout_work *work;
 	unsigned int isp_mis_tmp = 0;
-	u32 val;
 
 	spin_lock(&stats->irq_lock);
 
-	val = RKISP1_CIF_ISP_AWB_DONE | RKISP1_CIF_ISP_AFM_FIN |
-	      RKISP1_CIF_ISP_EXP_END | RKISP1_CIF_ISP_HIST_MEASURE_RDY;
-	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
+	rkisp1_write(rkisp1, RKISP1_STATS_MEAS_MASK, RKISP1_CIF_ISP_ICR);
 
 	isp_mis_tmp = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
-	if (isp_mis_tmp &
-	    (RKISP1_CIF_ISP_AWB_DONE | RKISP1_CIF_ISP_AFM_FIN |
-	     RKISP1_CIF_ISP_EXP_END | RKISP1_CIF_ISP_HIST_MEASURE_RDY))
+	if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
 		rkisp1->debug.stats_error++;
 
 	if (!stats->is_streaming)
 		goto unlock;
-	if (isp_ris & (RKISP1_CIF_ISP_AWB_DONE |
-		       RKISP1_CIF_ISP_AFM_FIN |
-		       RKISP1_CIF_ISP_EXP_END |
-		       RKISP1_CIF_ISP_HIST_MEASURE_RDY)) {
+	if (isp_ris & RKISP1_STATS_MEAS_MASK) {
 		work = kzalloc(sizeof(*work), GFP_ATOMIC);
 		if (work) {
 			INIT_WORK(&work->work,
-- 
2.17.1

[PATCH v2 2/4] media: staging: rkisp1: stats: replace spin_lock_irqsave with spin_lock_irq

[Dafna Hirschfeld]

The function 'rkisp1_stats_vb2_stop_streaming' runs in user context
therefore it is enough to use spin_lock_irq

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-stats.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index b19a6d9cdd4d..d4f0df4342e0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -152,13 +152,12 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 {
 	struct rkisp1_stats *stats = vq->drv_priv;
 	struct rkisp1_buffer *buf;
-	unsigned long flags;
 	unsigned int i;
 
 	/* Make sure no new work queued in isr before draining wq */
-	spin_lock_irqsave(&stats->irq_lock, flags);
+	spin_lock_irq(&stats->irq_lock);
 	stats->is_streaming = false;
-	spin_unlock_irqrestore(&stats->irq_lock, flags);
+	spin_unlock_irq(&stats->irq_lock);
 
 	drain_workqueue(stats->readout_wq);
 
-- 
2.17.1

[PATCH v2 3/4] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock

[Dafna Hirschfeld]

This patch removes two locks in the rkisp1_stats struct:
- The mutex 'wq_lock' that is used to protect the buffers list 'stat'
- The spin_lock 'irq_lock' that is used to protect the
variable 'is_streaming'

It replaces them with one spin_lock 'lock' that protects
both the buffers list and the 'is_streaming' variable.
In later patch the reading of the statistics will move to
the isr so there will be no need for the mutex 'wq_lock'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  5 ++--
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 25 +++++++++-----------
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index d2c669091fae..c92ae1b7093f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -178,7 +178,7 @@ struct rkisp1_capture {
 /*
  * struct rkisp1_stats - ISP Statistics device
  *
- * @irq_lock: buffer queue lock
+ * @lock: locks the buffer list 'stat' and 'is_streaming'
  * @stat: stats buffer list
  * @readout_wq: workqueue for statistics information read
  */
@@ -186,13 +186,12 @@ struct rkisp1_stats {
 	struct rkisp1_vdev_node vnode;
 	struct rkisp1_device *rkisp1;
 
-	spinlock_t irq_lock;
+	spinlock_t lock; /* locks 'is_streaming', and 'stats' */
 	struct list_head stat;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
 
 	struct workqueue_struct *readout_wq;
-	struct mutex wq_lock;
 };
 
 /*
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index d4f0df4342e0..58329e6b0598 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -133,9 +133,9 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
 
 	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
 
-	mutex_lock(&stats_dev->wq_lock);
+	spin_lock_irq(&stats_dev->lock);
 	list_add_tail(&stats_buf->queue, &stats_dev->stat);
-	mutex_unlock(&stats_dev->wq_lock);
+	spin_unlock_irq(&stats_dev->lock);
 }
 
 static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
@@ -155,13 +155,13 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 	unsigned int i;
 
 	/* Make sure no new work queued in isr before draining wq */
-	spin_lock_irq(&stats->irq_lock);
+	spin_lock_irq(&stats->lock);
 	stats->is_streaming = false;
-	spin_unlock_irq(&stats->irq_lock);
+	spin_unlock_irq(&stats->lock);
 
 	drain_workqueue(stats->readout_wq);
 
-	mutex_lock(&stats->wq_lock);
+	spin_lock_irq(&stats->lock);
 	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
 		if (list_empty(&stats->stat))
 			break;
@@ -170,7 +170,7 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 		list_del(&buf->queue);
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 	}
-	mutex_unlock(&stats->wq_lock);
+	spin_unlock_irq(&stats->lock);
 }
 
 static int
@@ -340,14 +340,14 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 		frame_sequence = meas_work->frame_id;
 	}
 
-	mutex_lock(&stats->wq_lock);
+	spin_lock_irq(&stats->lock);
 	/* get one empty buffer */
 	if (!list_empty(&stats->stat)) {
 		cur_buf = list_first_entry(&stats->stat,
 					   struct rkisp1_buffer, queue);
 		list_del(&cur_buf->queue);
 	}
-	mutex_unlock(&stats->wq_lock);
+	spin_unlock_irq(&stats->lock);
 
 	if (!cur_buf)
 		return;
@@ -396,7 +396,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	struct rkisp1_isp_readout_work *work;
 	unsigned int isp_mis_tmp = 0;
 
-	spin_lock(&stats->irq_lock);
+	spin_lock(&stats->lock);
 
 	rkisp1_write(rkisp1, RKISP1_STATS_MEAS_MASK, RKISP1_CIF_ISP_ICR);
 
@@ -425,7 +425,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	}
 
 unlock:
-	spin_unlock(&stats->irq_lock);
+	spin_unlock(&stats->lock);
 }
 
 static void rkisp1_init_stats(struct rkisp1_stats *stats)
@@ -445,10 +445,9 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 	int ret;
 
 	stats->rkisp1 = rkisp1;
-	mutex_init(&stats->wq_lock);
 	mutex_init(&node->vlock);
 	INIT_LIST_HEAD(&stats->stat);
-	spin_lock_init(&stats->irq_lock);
+	spin_lock_init(&stats->lock);
 
 	strscpy(vdev->name, RKISP1_STATS_DEV_NAME, sizeof(vdev->name));
 
@@ -495,7 +494,6 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 err_release_queue:
 	vb2_queue_release(vdev->queue);
 	mutex_destroy(&node->vlock);
-	mutex_destroy(&stats->wq_lock);
 	return ret;
 }
 
@@ -509,5 +507,4 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
 	media_entity_cleanup(&vdev->entity);
 	vb2_queue_release(vdev->queue);
 	mutex_destroy(&node->vlock);
-	mutex_destroy(&stats->wq_lock);
 }
-- 
2.17.1

[PATCH v2 4/4] media: staging: rkisp1: stats: read the stats in the isr

[Dafna Hirschfeld]

Currently the stats are read in a work queue. Defering the
reading of the stats is not needed and it is fine to read them
inside the irq handler.
This patch fixes and remove the TODO item:
'Use threaded interrupt for rkisp1_stats_isr(), remove work queue.'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/TODO            |  1 -
 drivers/staging/media/rkisp1/rkisp1-common.h |  3 -
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 87 ++------------------
 3 files changed, 7 insertions(+), 84 deletions(-)

diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
index c0cbec0a164d..bdb1b8f73556 100644
--- a/drivers/staging/media/rkisp1/TODO
+++ b/drivers/staging/media/rkisp1/TODO
@@ -1,5 +1,4 @@
 * Fix pad format size for statistics and parameters entities.
-* Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
 * Fix checkpatch errors.
 * Review and comment every lock
 * Handle quantization
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index c92ae1b7093f..45e554169224 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -180,7 +180,6 @@ struct rkisp1_capture {
  *
  * @lock: locks the buffer list 'stat' and 'is_streaming'
  * @stat: stats buffer list
- * @readout_wq: workqueue for statistics information read
  */
 struct rkisp1_stats {
 	struct rkisp1_vdev_node vnode;
@@ -190,8 +189,6 @@ struct rkisp1_stats {
 	struct list_head stat;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
-
-	struct workqueue_struct *readout_wq;
 };
 
 /*
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 58329e6b0598..87e4104d20dd 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -18,21 +18,6 @@
 #define RKISP1_ISP_STATS_REQ_BUFS_MIN 2
 #define RKISP1_ISP_STATS_REQ_BUFS_MAX 8
 
-enum rkisp1_isp_readout_cmd {
-	RKISP1_ISP_READOUT_MEAS,
-	RKISP1_ISP_READOUT_META,
-};
-
-struct rkisp1_isp_readout_work {
-	struct work_struct work;
-	struct rkisp1_stats *stats;
-
-	unsigned int frame_id;
-	unsigned int isp_ris;
-	enum rkisp1_isp_readout_cmd readout;
-	struct vb2_buffer *vb;
-};
-
 static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
 					  struct v4l2_fmtdesc *f)
 {
@@ -154,14 +139,8 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 	struct rkisp1_buffer *buf;
 	unsigned int i;
 
-	/* Make sure no new work queued in isr before draining wq */
 	spin_lock_irq(&stats->lock);
 	stats->is_streaming = false;
-	spin_unlock_irq(&stats->lock);
-
-	drain_workqueue(stats->readout_wq);
-
-	spin_lock_irq(&stats->lock);
 	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
 		if (list_empty(&stats->stat))
 			break;
@@ -324,8 +303,7 @@ static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
 }
 
 static void
-rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
-			      struct rkisp1_isp_readout_work *meas_work)
+rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 {
 	struct rkisp1_stat_buffer *cur_stat_buf;
 	struct rkisp1_buffer *cur_buf = NULL;
@@ -333,21 +311,12 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 		atomic_read(&stats->rkisp1->isp.frame_sequence);
 	u64 timestamp = ktime_get_ns();
 
-	if (frame_sequence != meas_work->frame_id) {
-		dev_warn(stats->rkisp1->dev,
-			 "Measurement late(%d, %d)\n",
-			 frame_sequence, meas_work->frame_id);
-		frame_sequence = meas_work->frame_id;
-	}
-
-	spin_lock_irq(&stats->lock);
 	/* get one empty buffer */
 	if (!list_empty(&stats->stat)) {
 		cur_buf = list_first_entry(&stats->stat,
 					   struct rkisp1_buffer, queue);
 		list_del(&cur_buf->queue);
 	}
-	spin_unlock_irq(&stats->lock);
 
 	if (!cur_buf)
 		return;
@@ -355,18 +324,18 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 	cur_stat_buf =
 		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE)
+	if (isp_ris & RKISP1_CIF_ISP_AWB_DONE)
 		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN)
+	if (isp_ris & RKISP1_CIF_ISP_AFM_FIN)
 		rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
+	if (isp_ris & RKISP1_CIF_ISP_EXP_END) {
 		rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
 		rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
 	}
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY)
+	if (isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY)
 		rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
 
 	vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
@@ -376,24 +345,9 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 }
 
-static void rkisp1_stats_readout_work(struct work_struct *work)
-{
-	struct rkisp1_isp_readout_work *readout_work =
-		container_of(work, struct rkisp1_isp_readout_work, work);
-	struct rkisp1_stats *stats = readout_work->stats;
-
-	if (readout_work->readout == RKISP1_ISP_READOUT_MEAS)
-		rkisp1_stats_send_measurement(stats, readout_work);
-
-	kfree(readout_work);
-}
-
 void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 {
-	unsigned int frame_sequence =
-		atomic_read(&stats->rkisp1->isp.frame_sequence);
 	struct rkisp1_device *rkisp1 = stats->rkisp1;
-	struct rkisp1_isp_readout_work *work;
 	unsigned int isp_mis_tmp = 0;
 
 	spin_lock(&stats->lock);
@@ -406,23 +360,8 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 
 	if (!stats->is_streaming)
 		goto unlock;
-	if (isp_ris & RKISP1_STATS_MEAS_MASK) {
-		work = kzalloc(sizeof(*work), GFP_ATOMIC);
-		if (work) {
-			INIT_WORK(&work->work,
-				  rkisp1_stats_readout_work);
-			work->readout = RKISP1_ISP_READOUT_MEAS;
-			work->stats = stats;
-			work->frame_id = frame_sequence;
-			work->isp_ris = isp_ris;
-			if (!queue_work(stats->readout_wq,
-					&work->work))
-				kfree(work);
-		} else {
-			dev_err(stats->rkisp1->dev,
-				"Could not allocate work\n");
-		}
-	}
+	if (isp_ris & RKISP1_STATS_MEAS_MASK)
+		rkisp1_stats_send_measurement(stats, isp_ris);
 
 unlock:
 	spin_unlock(&stats->lock);
@@ -476,19 +415,8 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 		goto err_cleanup_media_entity;
 	}
 
-	stats->readout_wq = alloc_workqueue("measurement_queue",
-					    WQ_UNBOUND | WQ_MEM_RECLAIM,
-					    1);
-
-	if (!stats->readout_wq) {
-		ret = -ENOMEM;
-		goto err_unreg_vdev;
-	}
-
 	return 0;
 
-err_unreg_vdev:
-	video_unregister_device(vdev);
 err_cleanup_media_entity:
 	media_entity_cleanup(&vdev->entity);
 err_release_queue:
@@ -502,7 +430,6 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
 	struct rkisp1_vdev_node *node = &stats->vnode;
 	struct video_device *vdev = &node->vdev;
 
-	destroy_workqueue(stats->readout_wq);
 	video_unregister_device(vdev);
 	media_entity_cleanup(&vdev->entity);
 	vb2_queue_release(vdev->queue);
-- 
2.17.1

Re: [PATCH v2 2/4] media: staging: rkisp1: stats: replace spin_lock_irqsave with spin_lock_irq

[Helen Koike]

Hi Dafna,

On 6/26/20 5:51 AM, Dafna Hirschfeld wrote:
> The function 'rkisp1_stats_vb2_stop_streaming' runs in user context
> therefore it is enough to use spin_lock_irq
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index b19a6d9cdd4d..d4f0df4342e0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -152,13 +152,12 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rkisp1_stats *stats = vq->drv_priv;
>  	struct rkisp1_buffer *buf;
> -	unsigned long flags;
>  	unsigned int i;
>  
>  	/* Make sure no new work queued in isr before draining wq */
> -	spin_lock_irqsave(&stats->irq_lock, flags);
> +	spin_lock_irq(&stats->irq_lock);
>  	stats->is_streaming = false;
> -	spin_unlock_irqrestore(&stats->irq_lock, flags);
> +	spin_unlock_irq(&stats->irq_lock);
>  
>  	drain_workqueue(stats->readout_wq);
>  
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock

[Helen Koike]

Hi Dafna,

On 6/26/20 5:51 AM, Dafna Hirschfeld wrote:
> This patch removes two locks in the rkisp1_stats struct:
> - The mutex 'wq_lock' that is used to protect the buffers list 'stat'
> - The spin_lock 'irq_lock' that is used to protect the
> variable 'is_streaming'
> 
> It replaces them with one spin_lock 'lock' that protects
> both the buffers list and the 'is_streaming' variable.
> In later patch the reading of the statistics will move to
> the isr so there will be no need for the mutex 'wq_lock'
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  5 ++--
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 25 +++++++++-----------
>  2 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index d2c669091fae..c92ae1b7093f 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -178,7 +178,7 @@ struct rkisp1_capture {
>  /*
>   * struct rkisp1_stats - ISP Statistics device
>   *
> - * @irq_lock: buffer queue lock
> + * @lock: locks the buffer list 'stat' and 'is_streaming'
>   * @stat: stats buffer list
>   * @readout_wq: workqueue for statistics information read
>   */
> @@ -186,13 +186,12 @@ struct rkisp1_stats {
>  	struct rkisp1_vdev_node vnode;
>  	struct rkisp1_device *rkisp1;
>  
> -	spinlock_t irq_lock;
> +	spinlock_t lock; /* locks 'is_streaming', and 'stats' */
>  	struct list_head stat;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
>  
>  	struct workqueue_struct *readout_wq;
> -	struct mutex wq_lock;
>  };
>  
>  /*
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index d4f0df4342e0..58329e6b0598 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -133,9 +133,9 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
>  
>  	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>  
> -	mutex_lock(&stats_dev->wq_lock);
> +	spin_lock_irq(&stats_dev->lock);
>  	list_add_tail(&stats_buf->queue, &stats_dev->stat);
> -	mutex_unlock(&stats_dev->wq_lock);
> +	spin_unlock_irq(&stats_dev->lock);
>  }
>  
>  static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
> @@ -155,13 +155,13 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  	unsigned int i;
>  
>  	/* Make sure no new work queued in isr before draining wq */
> -	spin_lock_irq(&stats->irq_lock);
> +	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = false;
> -	spin_unlock_irq(&stats->irq_lock);
> +	spin_unlock_irq(&stats->lock);
>  
>  	drain_workqueue(stats->readout_wq);
>  
> -	mutex_lock(&stats->wq_lock);
> +	spin_lock_irq(&stats->lock);
>  	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>  		if (list_empty(&stats->stat))
>  			break;
> @@ -170,7 +170,7 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  		list_del(&buf->queue);
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  	}
> -	mutex_unlock(&stats->wq_lock);
> +	spin_unlock_irq(&stats->lock);
>  }
>  
>  static int
> @@ -340,14 +340,14 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  		frame_sequence = meas_work->frame_id;
>  	}
>  
> -	mutex_lock(&stats->wq_lock);
> +	spin_lock_irq(&stats->lock);
>  	/* get one empty buffer */
>  	if (!list_empty(&stats->stat)) {
>  		cur_buf = list_first_entry(&stats->stat,
>  					   struct rkisp1_buffer, queue);
>  		list_del(&cur_buf->queue);
>  	}
> -	mutex_unlock(&stats->wq_lock);
> +	spin_unlock_irq(&stats->lock);
>  
>  	if (!cur_buf)
>  		return;
> @@ -396,7 +396,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	struct rkisp1_isp_readout_work *work;
>  	unsigned int isp_mis_tmp = 0;
>  
> -	spin_lock(&stats->irq_lock);
> +	spin_lock(&stats->lock);
>  
>  	rkisp1_write(rkisp1, RKISP1_STATS_MEAS_MASK, RKISP1_CIF_ISP_ICR);
>  
> @@ -425,7 +425,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	}
>  
>  unlock:
> -	spin_unlock(&stats->irq_lock);
> +	spin_unlock(&stats->lock);
>  }
>  
>  static void rkisp1_init_stats(struct rkisp1_stats *stats)
> @@ -445,10 +445,9 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
>  	int ret;
>  
>  	stats->rkisp1 = rkisp1;
> -	mutex_init(&stats->wq_lock);
>  	mutex_init(&node->vlock);
>  	INIT_LIST_HEAD(&stats->stat);
> -	spin_lock_init(&stats->irq_lock);
> +	spin_lock_init(&stats->lock);
>  
>  	strscpy(vdev->name, RKISP1_STATS_DEV_NAME, sizeof(vdev->name));
>  
> @@ -495,7 +494,6 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
>  err_release_queue:
>  	vb2_queue_release(vdev->queue);
>  	mutex_destroy(&node->vlock);
> -	mutex_destroy(&stats->wq_lock);
>  	return ret;
>  }
>  
> @@ -509,5 +507,4 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
>  	media_entity_cleanup(&vdev->entity);
>  	vb2_queue_release(vdev->queue);
>  	mutex_destroy(&node->vlock);
> -	mutex_destroy(&stats->wq_lock);
>  }
> 

Re: [PATCH v2 4/4] media: staging: rkisp1: stats: read the stats in the isr

[Helen Koike]

Hi Dafna,

On 6/26/20 5:51 AM, Dafna Hirschfeld wrote:
> Currently the stats are read in a work queue. Defering the
> reading of the stats is not needed and it is fine to read them
> inside the irq handler.
> This patch fixes and remove the TODO item:
> 'Use threaded interrupt for rkisp1_stats_isr(), remove work queue.'
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

lgtm

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/TODO            |  1 -
>  drivers/staging/media/rkisp1/rkisp1-common.h |  3 -
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 87 ++------------------
>  3 files changed, 7 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index c0cbec0a164d..bdb1b8f73556 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -1,5 +1,4 @@
>  * Fix pad format size for statistics and parameters entities.
> -* Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
>  * Fix checkpatch errors.
>  * Review and comment every lock
>  * Handle quantization
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index c92ae1b7093f..45e554169224 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -180,7 +180,6 @@ struct rkisp1_capture {
>   *
>   * @lock: locks the buffer list 'stat' and 'is_streaming'
>   * @stat: stats buffer list
> - * @readout_wq: workqueue for statistics information read
>   */
>  struct rkisp1_stats {
>  	struct rkisp1_vdev_node vnode;
> @@ -190,8 +189,6 @@ struct rkisp1_stats {
>  	struct list_head stat;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
> -
> -	struct workqueue_struct *readout_wq;
>  };
>  
>  /*
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 58329e6b0598..87e4104d20dd 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -18,21 +18,6 @@
>  #define RKISP1_ISP_STATS_REQ_BUFS_MIN 2
>  #define RKISP1_ISP_STATS_REQ_BUFS_MAX 8
>  
> -enum rkisp1_isp_readout_cmd {
> -	RKISP1_ISP_READOUT_MEAS,
> -	RKISP1_ISP_READOUT_META,
> -};
> -
> -struct rkisp1_isp_readout_work {
> -	struct work_struct work;
> -	struct rkisp1_stats *stats;
> -
> -	unsigned int frame_id;
> -	unsigned int isp_ris;
> -	enum rkisp1_isp_readout_cmd readout;
> -	struct vb2_buffer *vb;
> -};
> -
>  static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
>  					  struct v4l2_fmtdesc *f)
>  {
> @@ -154,14 +139,8 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  	struct rkisp1_buffer *buf;
>  	unsigned int i;
>  
> -	/* Make sure no new work queued in isr before draining wq */
>  	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = false;
> -	spin_unlock_irq(&stats->lock);
> -
> -	drain_workqueue(stats->readout_wq);
> -
> -	spin_lock_irq(&stats->lock);
>  	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>  		if (list_empty(&stats->stat))
>  			break;
> @@ -324,8 +303,7 @@ static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
>  }
>  
>  static void
> -rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
> -			      struct rkisp1_isp_readout_work *meas_work)
> +rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>  {
>  	struct rkisp1_stat_buffer *cur_stat_buf;
>  	struct rkisp1_buffer *cur_buf = NULL;
> @@ -333,21 +311,12 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  		atomic_read(&stats->rkisp1->isp.frame_sequence);
>  	u64 timestamp = ktime_get_ns();
>  
> -	if (frame_sequence != meas_work->frame_id) {
> -		dev_warn(stats->rkisp1->dev,
> -			 "Measurement late(%d, %d)\n",
> -			 frame_sequence, meas_work->frame_id);
> -		frame_sequence = meas_work->frame_id;
> -	}
> -
> -	spin_lock_irq(&stats->lock);
>  	/* get one empty buffer */
>  	if (!list_empty(&stats->stat)) {
>  		cur_buf = list_first_entry(&stats->stat,
>  					   struct rkisp1_buffer, queue);
>  		list_del(&cur_buf->queue);
>  	}
> -	spin_unlock_irq(&stats->lock);
>  
>  	if (!cur_buf)
>  		return;
> @@ -355,18 +324,18 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  	cur_stat_buf =
>  		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE)
> +	if (isp_ris & RKISP1_CIF_ISP_AWB_DONE)
>  		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN)
> +	if (isp_ris & RKISP1_CIF_ISP_AFM_FIN)
>  		rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
> +	if (isp_ris & RKISP1_CIF_ISP_EXP_END) {
>  		rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
>  		rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
>  	}
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY)
> +	if (isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY)
>  		rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
>  
>  	vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
> @@ -376,24 +345,9 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  }
>  
> -static void rkisp1_stats_readout_work(struct work_struct *work)
> -{
> -	struct rkisp1_isp_readout_work *readout_work =
> -		container_of(work, struct rkisp1_isp_readout_work, work);
> -	struct rkisp1_stats *stats = readout_work->stats;
> -
> -	if (readout_work->readout == RKISP1_ISP_READOUT_MEAS)
> -		rkisp1_stats_send_measurement(stats, readout_work);
> -
> -	kfree(readout_work);
> -}
> -
>  void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  {
> -	unsigned int frame_sequence =
> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
>  	struct rkisp1_device *rkisp1 = stats->rkisp1;
> -	struct rkisp1_isp_readout_work *work;
>  	unsigned int isp_mis_tmp = 0;
>  
>  	spin_lock(&stats->lock);
> @@ -406,23 +360,8 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  
>  	if (!stats->is_streaming)
>  		goto unlock;
> -	if (isp_ris & RKISP1_STATS_MEAS_MASK) {
> -		work = kzalloc(sizeof(*work), GFP_ATOMIC);
> -		if (work) {
> -			INIT_WORK(&work->work,
> -				  rkisp1_stats_readout_work);
> -			work->readout = RKISP1_ISP_READOUT_MEAS;
> -			work->stats = stats;
> -			work->frame_id = frame_sequence;
> -			work->isp_ris = isp_ris;
> -			if (!queue_work(stats->readout_wq,
> -					&work->work))
> -				kfree(work);
> -		} else {
> -			dev_err(stats->rkisp1->dev,
> -				"Could not allocate work\n");
> -		}
> -	}
> +	if (isp_ris & RKISP1_STATS_MEAS_MASK)
> +		rkisp1_stats_send_measurement(stats, isp_ris);
>  
>  unlock:
>  	spin_unlock(&stats->lock);
> @@ -476,19 +415,8 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
>  		goto err_cleanup_media_entity;
>  	}
>  
> -	stats->readout_wq = alloc_workqueue("measurement_queue",
> -					    WQ_UNBOUND | WQ_MEM_RECLAIM,
> -					    1);
> -
> -	if (!stats->readout_wq) {
> -		ret = -ENOMEM;
> -		goto err_unreg_vdev;
> -	}
> -
>  	return 0;
>  
> -err_unreg_vdev:
> -	video_unregister_device(vdev);
>  err_cleanup_media_entity:
>  	media_entity_cleanup(&vdev->entity);
>  err_release_queue:
> @@ -502,7 +430,6 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
>  	struct rkisp1_vdev_node *node = &stats->vnode;
>  	struct video_device *vdev = &node->vdev;
>  
> -	destroy_workqueue(stats->readout_wq);
>  	video_unregister_device(vdev);
>  	media_entity_cleanup(&vdev->entity);
>  	vb2_queue_release(vdev->queue);
>