[PATCH 0/5] media: staging: rkisp1: change workqueue to threaded irq in stats

[PATCH 0/5] media: staging: rkisp1: change workqueue to threaded irq in stats

[Dafna Hirschfeld]

Reading the statistics registers might take too long
to run inside the irq handler. Currently it is deferred
to bottom half using workqueues. This patch replaces the
workqueue with threaded interrupts.
This fixes an item in the TODO file to move the driver from staging.

Patches Summary:
1. Since the irq is shared, the isr should return either IRQ_NONE or IRQ_HANDELD.
This patch fixes it. In later patch, IRQ_WAKE_THREAD will be added.
2. Replace a long bitwise-or of the statistics flags with a macro to improve readability
3. Fixes a bug of using spin_lock instead of spin_lock_irqsave inside an irq handler
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. Replaces the workqueue with threaded irq in the statistics.

The code is tested using the 'cam' command from libcamera:

cam -c 1 -C  -s width=1280,height=960 --file="/tmp/libcamframe#.data"

Dafna Hirschfeld (4):
  media: staging: rkisp1: use a macro for the statistics flags mask
  media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock
  media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with
    one lock
  media: staging: rkisp1: replace workqueue with threaded irq for
    reading statistics registers

Helen Koike (1):
  media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP

 drivers/staging/media/rkisp1/TODO             |   1 -
 drivers/staging/media/rkisp1/rkisp1-capture.c |   7 +-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  20 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c     |  22 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  20 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c   | 197 +++++++-----------
 6 files changed, 121 insertions(+), 146 deletions(-)

-- 
2.17.1

[PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP

[Dafna Hirschfeld]

From: Helen Koike <helen.koike@collabora.com>

rkisp1 shares the interrupt line, then it shouldn't always return
IRQ_HANDLED, otherwise it can flag as handled an interrupt that wans't
meant for ISP.

return IRQ_NONE when the interrupt wans't meant for ISP

Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")

Signed-off-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c |  7 ++++++-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  6 +++---
 drivers/staging/media/rkisp1/rkisp1-dev.c     | 14 ++++++++++----
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 12 ++++++++----
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index f69235f82c45..19021875e8a9 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -649,12 +649,15 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 	rkisp1_set_next_buf(cap);
 }
 
-void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
+irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1)
 {
 	unsigned int i;
 	u32 status;
 
 	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
+	if (!status)
+		return IRQ_NONE;
+
 	rkisp1_write(rkisp1, status, RKISP1_CIF_MI_ICR);
 
 	for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); ++i) {
@@ -682,6 +685,8 @@ void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
 		cap->is_streaming = false;
 		wake_up(&cap->done);
 	}
+
+	return IRQ_HANDLED;
 }
 
 /* ----------------------------------------------------------------------------
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 0c4fe503adc9..33dffe21c769 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -305,9 +305,9 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
 
 const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
 
-void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
-void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
-void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
+irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
+irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
+irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
 void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
 void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
 
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index 9ac38bafb839..b7f43dab71c8 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -387,10 +387,13 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
 	return ret;
 }
 
-static irqreturn_t rkisp1_isr(int irq, void *ctx)
+irqreturn_t rkisp1_isr(int irq, void *ctx)
 {
 	struct device *dev = ctx;
 	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
+	irqreturn_t isp_ret;
+	irqreturn_t cap_ret;
+	irqreturn_t mipi_ret;
 
 	/*
 	 * Call rkisp1_capture_isr() first to handle the frame that
@@ -398,9 +401,12 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
 	 * it is potentially incremented by rkisp1_isp_isr() in the vertical
 	 * sync.
 	 */
-	rkisp1_capture_isr(rkisp1);
-	rkisp1_isp_isr(rkisp1);
-	rkisp1_mipi_isr(rkisp1);
+	cap_ret = rkisp1_capture_isr(rkisp1);
+	isp_ret = rkisp1_isp_isr(rkisp1);
+	mipi_ret = rkisp1_mipi_isr(rkisp1);
+
+	if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
+		return IRQ_NONE;
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index dc2b59a0160a..19ab0ed323aa 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1046,13 +1046,13 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
  * Interrupt handlers
  */
 
-void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
+irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
 {
 	u32 val, status;
 
 	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
 	if (!status)
-		return;
+		return IRQ_NONE;
 
 	rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);
 
@@ -1087,6 +1087,8 @@ void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
 	} else {
 		rkisp1->debug.mipi_error++;
 	}
+
+	return IRQ_HANDLED;
 }
 
 static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
@@ -1106,13 +1108,13 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
 	v4l2_event_queue(isp->sd.devnode, &event);
 }
 
-void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
+irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 {
 	u32 status, isp_err;
 
 	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
 	if (!status)
-		return;
+		return IRQ_NONE;
 
 	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
 
@@ -1148,4 +1150,6 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	 * Do the updates in the order of the processing flow.
 	 */
 	rkisp1_params_isr(rkisp1, status);
+
+	return IRQ_HANDLED;
 }
-- 
2.17.1

Re: [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6054 bytes --]

Hi Dafna,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.7-rc5 next-20200512]
[cannot apply to staging/staging-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/media-staging-rkisp1-change-workqueue-to-threaded-irq-in-stats/20200512-200942
base:   git://linuxtv.org/media_tree.git master
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

In file included from drivers/staging/media/rkisp1/rkisp1-capture.c:21:
>> drivers/staging/media/rkisp1/rkisp1-common.h:308:1: error: unknown type name 'irqreturn_t'
308 | irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:309:1: error: unknown type name 'irqreturn_t'
309 | irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:310:1: error: unknown type name 'irqreturn_t'
310 | irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
>> drivers/staging/media/rkisp1/rkisp1-capture.c:652:1: error: unknown type name 'irqreturn_t'
652 | irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1)
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-capture.c: In function 'rkisp1_capture_isr':
>> drivers/staging/media/rkisp1/rkisp1-capture.c:659:10: error: 'IRQ_NONE' undeclared (first use in this function)
659 |   return IRQ_NONE;
|          ^~~~~~~~
drivers/staging/media/rkisp1/rkisp1-capture.c:659:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/staging/media/rkisp1/rkisp1-capture.c:689:9: error: 'IRQ_HANDLED' undeclared (first use in this function)
689 |  return IRQ_HANDLED;
|         ^~~~~~~~~~~
>> drivers/staging/media/rkisp1/rkisp1-capture.c:690:1: warning: control reaches end of non-void function [-Wreturn-type]
690 | }
| ^
--
In file included from drivers/staging/media/rkisp1/rkisp1-common.c:10:
>> drivers/staging/media/rkisp1/rkisp1-common.h:308:1: error: unknown type name 'irqreturn_t'
308 | irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:309:1: error: unknown type name 'irqreturn_t'
309 | irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:310:1: error: unknown type name 'irqreturn_t'
310 | irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
--
In file included from drivers/staging/media/rkisp1/rkisp1-isp.c:19:
>> drivers/staging/media/rkisp1/rkisp1-common.h:308:1: error: unknown type name 'irqreturn_t'
308 | irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:309:1: error: unknown type name 'irqreturn_t'
309 | irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:310:1: error: unknown type name 'irqreturn_t'
310 | irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
>> drivers/staging/media/rkisp1/rkisp1-isp.c:1049:1: error: unknown type name 'irqreturn_t'
1049 | irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_mipi_isr':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:1055:10: error: 'IRQ_NONE' undeclared (first use in this function)
1055 |   return IRQ_NONE;
|          ^~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c:1055:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/staging/media/rkisp1/rkisp1-isp.c:1091:9: error: 'IRQ_HANDLED' undeclared (first use in this function)
1091 |  return IRQ_HANDLED;
|         ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c: At top level:
drivers/staging/media/rkisp1/rkisp1-isp.c:1111:1: error: unknown type name 'irqreturn_t'
1111 | irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_isr':
drivers/staging/media/rkisp1/rkisp1-isp.c:1117:10: error: 'IRQ_NONE' undeclared (first use in this function)
1117 |   return IRQ_NONE;
|          ^~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c:1154:9: error: 'IRQ_HANDLED' undeclared (first use in this function)
1154 |  return IRQ_HANDLED;
|         ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_mipi_isr':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:1092:1: warning: control reaches end of non-void function [-Wreturn-type]
1092 | }
| ^
drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_isr':
drivers/staging/media/rkisp1/rkisp1-isp.c:1155:1: warning: control reaches end of non-void function [-Wreturn-type]
1155 | }
| ^

vim +/irqreturn_t +308 drivers/staging/media/rkisp1/rkisp1-common.h

   307	
 > 308	irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
   309	irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
   310	irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
   311	void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
   312	void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
   313	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55567 bytes --]

Re: [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP

[Helen Koike]

Hi Dafna,

Thanks for the patch.

On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> From: Helen Koike <helen.koike@collabora.com>
> 
> rkisp1 shares the interrupt line, then it shouldn't always return
> IRQ_HANDLED, otherwise it can flag as handled an interrupt that wans't
> meant for ISP.
> 
> return IRQ_NONE when the interrupt wans't meant for ISP
> 
> Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  7 ++++++-
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  6 +++---
>  drivers/staging/media/rkisp1/rkisp1-dev.c     | 14 ++++++++++----
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 12 ++++++++----
>  4 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index f69235f82c45..19021875e8a9 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -649,12 +649,15 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  	rkisp1_set_next_buf(cap);
>  }
>  
> -void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1)
>  {
>  	unsigned int i;
>  	u32 status;
>  
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
> +	if (!status)
> +		return IRQ_NONE;
> +
>  	rkisp1_write(rkisp1, status, RKISP1_CIF_MI_ICR);
>  
>  	for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); ++i) {
> @@ -682,6 +685,8 @@ void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
>  		cap->is_streaming = false;
>  		wake_up(&cap->done);
>  	}
> +
> +	return IRQ_HANDLED;
>  }
>  
>  /* ----------------------------------------------------------------------------
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 0c4fe503adc9..33dffe21c769 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -305,9 +305,9 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
>  
>  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>  
> -void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> -void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>  
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index 9ac38bafb839..b7f43dab71c8 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -387,10 +387,13 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
>  	return ret;
>  }
>  
> -static irqreturn_t rkisp1_isr(int irq, void *ctx)
> +irqreturn_t rkisp1_isr(int irq, void *ctx)
>  {
>  	struct device *dev = ctx;
>  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +	irqreturn_t isp_ret;
> +	irqreturn_t cap_ret;
> +	irqreturn_t mipi_ret;

Just cosmetics, you could declare them in a single line

	irqreturn_t cap_ret, isp_ret, mipi_ret;

With or without this change:

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

Thanks
Helen

>  
>  	/*
>  	 * Call rkisp1_capture_isr() first to handle the frame that
> @@ -398,9 +401,12 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
>  	 * it is potentially incremented by rkisp1_isp_isr() in the vertical
>  	 * sync.
>  	 */
> -	rkisp1_capture_isr(rkisp1);
> -	rkisp1_isp_isr(rkisp1);
> -	rkisp1_mipi_isr(rkisp1);
> +	cap_ret = rkisp1_capture_isr(rkisp1);
> +	isp_ret = rkisp1_isp_isr(rkisp1);
> +	mipi_ret = rkisp1_mipi_isr(rkisp1);
> +
> +	if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
> +		return IRQ_NONE;
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index dc2b59a0160a..19ab0ed323aa 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1046,13 +1046,13 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
>   * Interrupt handlers
>   */
>  
> -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
>  {
>  	u32 val, status;
>  
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
>  	if (!status)
> -		return;
> +		return IRQ_NONE;
>  
>  	rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);
>  
> @@ -1087,6 +1087,8 @@ void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
>  	} else {
>  		rkisp1->debug.mipi_error++;
>  	}
> +
> +	return IRQ_HANDLED;
>  }
>  
>  static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> @@ -1106,13 +1108,13 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>  	v4l2_event_queue(isp->sd.devnode, &event);
>  }
>  
> -void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  {
>  	u32 status, isp_err;
>  
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>  	if (!status)
> -		return;
> +		return IRQ_NONE;
>  
>  	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>  
> @@ -1148,4 +1150,6 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  	 * Do the updates in the order of the processing flow.
>  	 */
>  	rkisp1_params_isr(rkisp1, status);
> +
> +	return IRQ_HANDLED;
>  }
> 

Re: [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Wed, May 20, 2020 at 07:58:41AM -0300, Helen Koike wrote:
> On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> > From: Helen Koike <helen.koike@collabora.com>
> > 
> > rkisp1 shares the interrupt line, then it shouldn't always return
> > IRQ_HANDLED, otherwise it can flag as handled an interrupt that wans't
> > meant for ISP.
> > 
> > return IRQ_NONE when the interrupt wans't meant for ISP
> > 
> > Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")
> > 
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/rkisp1-capture.c |  7 ++++++-
> >  drivers/staging/media/rkisp1/rkisp1-common.h  |  6 +++---
> >  drivers/staging/media/rkisp1/rkisp1-dev.c     | 14 ++++++++++----
> >  drivers/staging/media/rkisp1/rkisp1-isp.c     | 12 ++++++++----
> >  4 files changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index f69235f82c45..19021875e8a9 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -649,12 +649,15 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
> >  	rkisp1_set_next_buf(cap);
> >  }
> >  
> > -void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> > +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> >  {
> >  	unsigned int i;
> >  	u32 status;
> >  
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> >  	rkisp1_write(rkisp1, status, RKISP1_CIF_MI_ICR);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); ++i) {
> > @@ -682,6 +685,8 @@ void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> >  		cap->is_streaming = false;
> >  		wake_up(&cap->done);
> >  	}
> > +
> > +	return IRQ_HANDLED;
> >  }
> >  
> >  /* ----------------------------------------------------------------------------
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> > index 0c4fe503adc9..33dffe21c769 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> > +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> > @@ -305,9 +305,9 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
> >  
> >  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
> >  
> > -void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> > -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> > -void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> > +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> > +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> > +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> >  void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
> >  void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
> >  
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> > index 9ac38bafb839..b7f43dab71c8 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> > @@ -387,10 +387,13 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
> >  	return ret;
> >  }
> >  
> > -static irqreturn_t rkisp1_isr(int irq, void *ctx)
> > +irqreturn_t rkisp1_isr(int irq, void *ctx)
> >  {
> >  	struct device *dev = ctx;
> >  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > +	irqreturn_t isp_ret;
> > +	irqreturn_t cap_ret;
> > +	irqreturn_t mipi_ret;
> 
> Just cosmetics, you could declare them in a single line
> 
> 	irqreturn_t cap_ret, isp_ret, mipi_ret;
> 
> With or without this change:
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> >  
> >  	/*
> >  	 * Call rkisp1_capture_isr() first to handle the frame that
> > @@ -398,9 +401,12 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
> >  	 * it is potentially incremented by rkisp1_isp_isr() in the vertical
> >  	 * sync.
> >  	 */
> > -	rkisp1_capture_isr(rkisp1);
> > -	rkisp1_isp_isr(rkisp1);
> > -	rkisp1_mipi_isr(rkisp1);
> > +	cap_ret = rkisp1_capture_isr(rkisp1);
> > +	isp_ret = rkisp1_isp_isr(rkisp1);
> > +	mipi_ret = rkisp1_mipi_isr(rkisp1);
> > +
> > +	if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
> > +		return IRQ_NONE;

Another cosmetic change proposal:

	irqreturn_t ret = IRQ_NONE;
	...

	if (rkisp1_capture_isr(rkisp1) == IRQ_HANDLED)
		ret = IRQ_HANDLED;

	if (rkisp1_isp_isr(rkisp1) == IRQ_HANDLED)
		ret = IRQ_HANDLED;

	if (rkisp1_mipi_isr(rkisp1) == IRQ_HANDLED)
		ret = IRQ_HANDLED;

	return ret;

With or without it,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  
> >  	return IRQ_HANDLED;
> >  }
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > index dc2b59a0160a..19ab0ed323aa 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > @@ -1046,13 +1046,13 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
> >   * Interrupt handlers
> >   */
> >  
> > -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> > +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> >  {
> >  	u32 val, status;
> >  
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
> >  	if (!status)
> > -		return;
> > +		return IRQ_NONE;
> >  
> >  	rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);
> >  
> > @@ -1087,6 +1087,8 @@ void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> >  	} else {
> >  		rkisp1->debug.mipi_error++;
> >  	}
> > +
> > +	return IRQ_HANDLED;
> >  }
> >  
> >  static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> > @@ -1106,13 +1108,13 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> >  	v4l2_event_queue(isp->sd.devnode, &event);
> >  }
> >  
> > -void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> > +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> >  {
> >  	u32 status, isp_err;
> >  
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
> >  	if (!status)
> > -		return;
> > +		return IRQ_NONE;
> >  
> >  	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
> >  
> > @@ -1148,4 +1150,6 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> >  	 * Do the updates in the order of the processing flow.
> >  	 */
> >  	rkisp1_params_isr(rkisp1, status);
> > +
> > +	return IRQ_HANDLED;
> >  }
> > 

-- 
Regards,

Laurent Pinchart

[PATCH 2/5] 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>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  4 ++++
 drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 +----
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 12 +++---------
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 33dffe21c769..c0ab16c6b3db 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 19ab0ed323aa..49b47e1734b0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1137,10 +1137,7 @@ irqreturn_t 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 6dfcbdc3deb8..12998db955e6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -407,22 +407,16 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 
 	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;
+	val = RKISP1_STATS_MEAS_MASK;
 	rkisp1_write(rkisp1, val, 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

Re: [PATCH 2/5] media: staging: rkisp1: use a macro for the statistics flags mask

[Helen Koike]

Hi Dafna,

On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> 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>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  4 ++++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 +----
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 12 +++---------
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 33dffe21c769..c0ab16c6b3db 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 19ab0ed323aa..49b47e1734b0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1137,10 +1137,7 @@ irqreturn_t 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 6dfcbdc3deb8..12998db955e6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -407,22 +407,16 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  
>  	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;
> +	val = RKISP1_STATS_MEAS_MASK;
>  	rkisp1_write(rkisp1, val, 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,
> 

Re: [PATCH 2/5] media: staging: rkisp1: use a macro for the statistics flags mask

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Tue, May 12, 2020 at 02:05:19PM +0200, Dafna Hirschfeld wrote:
> 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>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  4 ++++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 +----
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 12 +++---------
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 33dffe21c769..c0ab16c6b3db 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 19ab0ed323aa..49b47e1734b0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1137,10 +1137,7 @@ irqreturn_t 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 6dfcbdc3deb8..12998db955e6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -407,22 +407,16 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  
>  	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;
> +	val = RKISP1_STATS_MEAS_MASK;
>  	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);

This could become

 	rkisp1_write(rkisp1, RKISP1_STATS_MEAS_MASK, RKISP1_CIF_ISP_ICR);

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	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,

-- 
Regards,

Laurent Pinchart

[PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock

[Dafna Hirschfeld]

Currently 'spin_lock' is used in order to lock the 'irq_lock'.
This should be replaced with 'spin_lock_irqsave' since it is
used in the irq handler.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 12998db955e6..5578fdeb8a18 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -403,9 +403,10 @@ 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;
+	unsigned long flags;
 	u32 val;
 
-	spin_lock(&stats->irq_lock);
+	spin_lock_irqsave(&stats->irq_lock, flags);
 
 	val = RKISP1_STATS_MEAS_MASK;
 	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
@@ -435,7 +436,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	}
 
 unlock:
-	spin_unlock(&stats->irq_lock);
+	spin_unlock_irqrestore(&stats->irq_lock, flags);
 }
 
 static void rkisp1_init_stats(struct rkisp1_stats *stats)
-- 
2.17.1

Re: [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock

[Helen Koike]

Hi Dafna,

On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> Currently 'spin_lock' is used in order to lock the 'irq_lock'.
> This should be replaced with 'spin_lock_irqsave' since it is
> used in the irq handler.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 12998db955e6..5578fdeb8a18 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -403,9 +403,10 @@ 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;
> +	unsigned long flags;
>  	u32 val;
>  
> -	spin_lock(&stats->irq_lock);
> +	spin_lock_irqsave(&stats->irq_lock, flags);

Since you are moving this function to a threaded irq handler, you won't be in interrupt context.

The spin_lock_irqsave() function disable interrupts for the critical section, are you sure this is
required?

Regards,
Helen

>  
>  	val = RKISP1_STATS_MEAS_MASK;
>  	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
> @@ -435,7 +436,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	}
>  
>  unlock:
> -	spin_unlock(&stats->irq_lock);
> +	spin_unlock_irqrestore(&stats->irq_lock, flags);
>  }
>  
>  static void rkisp1_init_stats(struct rkisp1_stats *stats)
> 

Re: [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock

[Dafna Hirschfeld]

On 20.05.20 13:11, Helen Koike wrote:
> Hi Dafna,
> 
> On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
>> Currently 'spin_lock' is used in order to lock the 'irq_lock'.
>> This should be replaced with 'spin_lock_irqsave' since it is
>> used in the irq handler.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-stats.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> index 12998db955e6..5578fdeb8a18 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> @@ -403,9 +403,10 @@ 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;
>> +	unsigned long flags;
>>   	u32 val;
>>   
>> -	spin_lock(&stats->irq_lock);
>> +	spin_lock_irqsave(&stats->irq_lock, flags);
> 
> Since you are moving this function to a threaded irq handler, you won't be in interrupt context.
> 
> The spin_lock_irqsave() function disable interrupts for the critical section, are you sure this is
> required?
Hi,
The lock is also used in the hard irq handler in the patch that moves the statistics to threaded interrupt.
The code in the hard irq iterates the buffers queue to find the next buffer available and set the flags of
the ready statistics on it.

Thanks,
Dafna

> 
> Regards,
> Helen
> 
>>   
>>   	val = RKISP1_STATS_MEAS_MASK;
>>   	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
>> @@ -435,7 +436,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>>   	}
>>   
>>   unlock:
>> -	spin_unlock(&stats->irq_lock);
>> +	spin_unlock_irqrestore(&stats->irq_lock, flags);
>>   }
>>   
>>   static void rkisp1_init_stats(struct rkisp1_stats *stats)
>>

Re: [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock

[Laurent Pinchart]

Hi Dafna,

On Wed, May 20, 2020 at 09:22:29PM +0200, Dafna Hirschfeld wrote:
> On 20.05.20 13:11, Helen Koike wrote:
> > On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> >> Currently 'spin_lock' is used in order to lock the 'irq_lock'.
> >> This should be replaced with 'spin_lock_irqsave' since it is
> >> used in the irq handler.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-stats.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> >> index 12998db955e6..5578fdeb8a18 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> >> @@ -403,9 +403,10 @@ 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;
> >> +	unsigned long flags;
> >>   	u32 val;
> >>   
> >> -	spin_lock(&stats->irq_lock);
> >> +	spin_lock_irqsave(&stats->irq_lock, flags);
> > 
> > Since you are moving this function to a threaded irq handler, you won't be in interrupt context.
> > 
> > The spin_lock_irqsave() function disable interrupts for the critical section, are you sure this is
> > required?
>
> Hi,
> The lock is also used in the hard irq handler in the patch that moves the statistics to threaded interrupt.
> The code in the hard irq iterates the buffers queue to find the next buffer available and set the flags of
> the ready statistics on it.

The rules about spinlocks are as follows.

If the lock is never used in an interrupt handler context (or similar
context, such as a timer handler for instance), then you can use
spin_lock(). The reason is that, in such cases, a section covered by
spin_lock()/spin_unlock() will not be preempted on the same CPU by a
section that could try to take the same lock. This also applies to locks
that are only used in interrupt contexts where interrupts are guaranteed
to be disabled. To put it differently, if there's no risk that a
spinlock-covered section will be preempted by code that will try to take
the same lock, spin_lock() is enough.

Otherwise, you should use spin_lock() in code that is guaranteed to run
with interrupts disabled (such as hard IRQ handlers, or code that called
after another spin_lock_irq() or spin_lock_irqsave() on another lock),
spin_lock_irq() in code that is guaranteed to run with interrupts
enabled (such as code paths from userspace where you can guarantee
interrupts haven't been disabled by, for instance, a spin_lock_irq() in
the call stack), and spin_lock_irqsave() when you're not sure.

There's a tendency to always use spin_lock_irqsave() just to make sure,
and it can indeed avoid potential issues when code is later refactored
and assumptions that lead to the selection of the propery spin_lock*()
variant change. That's a bit idea in paths where latency is critical,
but should otherwise not be too much of a problem.

In this patch, as rkisp1_stats_isr() is run in a hard IRQ context with
interrupts disabled, there's no need to use spin_lock_irqsave(),
spin_lock() is totally fine.

> >>   	val = RKISP1_STATS_MEAS_MASK;
> >>   	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
> >> @@ -435,7 +436,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> >>   	}
> >>   
> >>   unlock:
> >> -	spin_unlock(&stats->irq_lock);
> >> +	spin_unlock_irqrestore(&stats->irq_lock, flags);
> >>   }
> >>   
> >>   static void rkisp1_init_stats(struct rkisp1_stats *stats)

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] 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 'stats_lock' that protects
both the buffers list and the 'is_streaming' variable.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index c0ab16c6b3db..c635bb0a7727 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
+ * @stats_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 stats_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 5578fdeb8a18..e6fb2c5f3b3e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -130,12 +130,13 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
 		container_of(vbuf, struct rkisp1_buffer, vb);
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_stats *stats_dev = vq->drv_priv;
+	unsigned long flags;
 
 	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
 
-	mutex_lock(&stats_dev->wq_lock);
+	spin_lock_irqsave(&stats_dev->stats_lock, flags);
 	list_add_tail(&stats_buf->queue, &stats_dev->stat);
-	mutex_unlock(&stats_dev->wq_lock);
+	spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
 }
 
 static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
@@ -156,13 +157,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_irqsave(&stats->irq_lock, flags);
+	spin_lock_irqsave(&stats->stats_lock, flags);
 	stats->is_streaming = false;
-	spin_unlock_irqrestore(&stats->irq_lock, flags);
+	spin_unlock_irqrestore(&stats->stats_lock, flags);
 
 	drain_workqueue(stats->readout_wq);
 
-	mutex_lock(&stats->wq_lock);
+	spin_lock_irqsave(&stats->stats_lock, flags);
 	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
 		if (list_empty(&stats->stat))
 			break;
@@ -171,7 +172,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_irqrestore(&stats->stats_lock, flags);
 }
 
 static int
@@ -333,6 +334,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 	unsigned int frame_sequence =
 		atomic_read(&stats->rkisp1->isp.frame_sequence);
 	u64 timestamp = ktime_get_ns();
+	unsigned long flags;
 
 	if (frame_sequence != meas_work->frame_id) {
 		dev_warn(stats->rkisp1->dev,
@@ -341,14 +343,14 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
 		frame_sequence = meas_work->frame_id;
 	}
 
-	mutex_lock(&stats->wq_lock);
+	spin_lock_irqsave(&stats->stats_lock, flags);
 	/* 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_irqrestore(&stats->stats_lock, flags);
 
 	if (!cur_buf)
 		return;
@@ -406,7 +408,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&stats->irq_lock, flags);
+	spin_lock_irqsave(&stats->stats_lock, flags);
 
 	val = RKISP1_STATS_MEAS_MASK;
 	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
@@ -436,7 +438,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	}
 
 unlock:
-	spin_unlock_irqrestore(&stats->irq_lock, flags);
+	spin_unlock_irqrestore(&stats->stats_lock, flags);
 }
 
 static void rkisp1_init_stats(struct rkisp1_stats *stats)
@@ -456,10 +458,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->stats_lock);
 
 	strscpy(vdev->name, RKISP1_STATS_DEV_NAME, sizeof(vdev->name));
 
@@ -506,7 +507,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;
 }
 
@@ -520,5 +520,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

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

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Tue, May 12, 2020 at 02:05:21PM +0200, 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 'stats_lock' that protects
> both the buffers list and the 'is_streaming' variable.

You should explain in the commit message why this is needed.

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  5 ++--
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 27 ++++++++++----------
>  2 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index c0ab16c6b3db..c635bb0a7727 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
> + * @stats_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 stats_lock; /* locks 'is_streaming', and 'stats' */

If there's a single lock, how about simply calling it 'lock' ?

>  	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 5578fdeb8a18..e6fb2c5f3b3e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -130,12 +130,13 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
>  		container_of(vbuf, struct rkisp1_buffer, vb);
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct rkisp1_stats *stats_dev = vq->drv_priv;
> +	unsigned long flags;
>  
>  	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>  
> -	mutex_lock(&stats_dev->wq_lock);
> +	spin_lock_irqsave(&stats_dev->stats_lock, flags);

As I explained in a previous e-mail in this series, you could use
spin_lock_irq() here as there's a guarantee interrupts are not disabled.
Up to you.

>  	list_add_tail(&stats_buf->queue, &stats_dev->stat);
> -	mutex_unlock(&stats_dev->wq_lock);
> +	spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
>  }
>  
>  static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
> @@ -156,13 +157,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_irqsave(&stats->irq_lock, flags);
> +	spin_lock_irqsave(&stats->stats_lock, flags);
>  	stats->is_streaming = false;
> -	spin_unlock_irqrestore(&stats->irq_lock, flags);
> +	spin_unlock_irqrestore(&stats->stats_lock, flags);
>  
>  	drain_workqueue(stats->readout_wq);
>  
> -	mutex_lock(&stats->wq_lock);
> +	spin_lock_irqsave(&stats->stats_lock, flags);
>  	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>  		if (list_empty(&stats->stat))
>  			break;
> @@ -171,7 +172,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_irqrestore(&stats->stats_lock, flags);
>  }
>  
>  static int
> @@ -333,6 +334,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  	unsigned int frame_sequence =
>  		atomic_read(&stats->rkisp1->isp.frame_sequence);
>  	u64 timestamp = ktime_get_ns();
> +	unsigned long flags;
>  
>  	if (frame_sequence != meas_work->frame_id) {
>  		dev_warn(stats->rkisp1->dev,
> @@ -341,14 +343,14 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
>  		frame_sequence = meas_work->frame_id;
>  	}
>  
> -	mutex_lock(&stats->wq_lock);
> +	spin_lock_irqsave(&stats->stats_lock, flags);
>  	/* 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_irqrestore(&stats->stats_lock, flags);
>  
>  	if (!cur_buf)
>  		return;
> @@ -406,7 +408,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	unsigned long flags;
>  	u32 val;
>  
> -	spin_lock_irqsave(&stats->irq_lock, flags);
> +	spin_lock_irqsave(&stats->stats_lock, flags);
>  
>  	val = RKISP1_STATS_MEAS_MASK;
>  	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
> @@ -436,7 +438,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	}
>  
>  unlock:
> -	spin_unlock_irqrestore(&stats->irq_lock, flags);
> +	spin_unlock_irqrestore(&stats->stats_lock, flags);
>  }
>  
>  static void rkisp1_init_stats(struct rkisp1_stats *stats)
> @@ -456,10 +458,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->stats_lock);
>  
>  	strscpy(vdev->name, RKISP1_STATS_DEV_NAME, sizeof(vdev->name));
>  
> @@ -506,7 +507,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;
>  }
>  
> @@ -520,5 +520,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);
>  }

-- 
Regards,

Laurent Pinchart

[PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[Dafna Hirschfeld]

Reading the statistics registers might take too long
to run inside the irq handler. Currently it is deferred
to bottom half using workqueues. This patch replaces the
workqueue with threaded interrupts for reading the
statistics registers.

A new struct type 'rkisp1_kstats_buffer' is used as the statistics
buffers. The struct has a field 'ris' which is the flags of ready
statistics. If new statistics are ready, the irq handler sets
this variable and the frame sequence on the next available buffer
and returns IRQ_WAKE_THREAD.
Then the threaded interrupt reads the registers and calls
vb2_buffer_done.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/TODO            |   1 -
 drivers/staging/media/rkisp1/rkisp1-common.h |   5 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c    |   8 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c    |   5 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 167 ++++++++-----------
 5 files changed, 76 insertions(+), 110 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 c635bb0a7727..c8adcdf661ab 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -190,8 +190,6 @@ struct rkisp1_stats {
 	struct list_head stat;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
-
-	struct workqueue_struct *readout_wq;
 };
 
 /*
@@ -308,10 +306,11 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
 
 const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
 
+irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx);
 irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
 irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
 irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
-void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
+irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
 void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
 
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index b7f43dab71c8..12e2e8559acd 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -405,6 +405,8 @@ irqreturn_t rkisp1_isr(int irq, void *ctx)
 	isp_ret = rkisp1_isp_isr(rkisp1);
 	mipi_ret = rkisp1_mipi_isr(rkisp1);
 
+	if (isp_ret == IRQ_WAKE_THREAD)
+		return IRQ_WAKE_THREAD;
 	if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
 		return IRQ_NONE;
 
@@ -490,8 +492,10 @@ static int rkisp1_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	ret = devm_request_irq(dev, irq, rkisp1_isr, IRQF_SHARED,
-			       dev_driver_string(dev), dev);
+	ret = devm_request_threaded_irq(dev, irq, rkisp1_isr,
+					rkisp1_read_stats_threaded_irq,
+					IRQF_SHARED,
+					dev_driver_string(dev), dev);
 	if (ret) {
 		dev_err(dev, "request irq failed: %d\n", ret);
 		return ret;
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 49b47e1734b0..09893073af00 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1111,6 +1111,7 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
 irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 {
 	u32 status, isp_err;
+	irqreturn_t ret = IRQ_HANDLED;
 
 	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
 	if (!status)
@@ -1138,7 +1139,7 @@ irqreturn_t 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_STATS_MEAS_MASK)
-			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
+			ret = rkisp1_stats_isr(&rkisp1->stats, isp_ris);
 	}
 
 	/*
@@ -1148,5 +1149,5 @@ irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	 */
 	rkisp1_params_isr(rkisp1, status);
 
-	return IRQ_HANDLED;
+	return ret;
 }
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index e6fb2c5f3b3e..f5eaa81362ea 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -18,19 +18,9 @@
 #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;
+struct rkisp1_kstats_buffer {
+	struct rkisp1_buffer buff;
+	u32 ris;
 };
 
 static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
@@ -126,16 +116,17 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
 static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
-	struct rkisp1_buffer *stats_buf =
-		container_of(vbuf, struct rkisp1_buffer, vb);
+	struct rkisp1_kstats_buffer *kstats_buf =
+		container_of(vbuf, struct rkisp1_kstats_buffer, buff.vb);
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_stats *stats_dev = vq->drv_priv;
 	unsigned long flags;
 
-	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
+	kstats_buf->buff.vaddr[0] = vb2_plane_vaddr(vb, 0);
+	kstats_buf->ris = 0;
 
 	spin_lock_irqsave(&stats_dev->stats_lock, flags);
-	list_add_tail(&stats_buf->queue, &stats_dev->stat);
+	list_add_tail(&kstats_buf->buff.queue, &stats_dev->stat);
 	spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
 }
 
@@ -152,25 +143,19 @@ static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
 static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 {
 	struct rkisp1_stats *stats = vq->drv_priv;
-	struct rkisp1_buffer *buf;
+	struct rkisp1_kstats_buffer *buf;
 	unsigned long flags;
 	unsigned int i;
 
-	/* Make sure no new work queued in isr before draining wq */
 	spin_lock_irqsave(&stats->stats_lock, flags);
 	stats->is_streaming = false;
-	spin_unlock_irqrestore(&stats->stats_lock, flags);
-
-	drain_workqueue(stats->readout_wq);
-
-	spin_lock_irqsave(&stats->stats_lock, flags);
 	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
 		if (list_empty(&stats->stat))
 			break;
 		buf = list_first_entry(&stats->stat,
-				       struct rkisp1_buffer, queue);
-		list_del(&buf->queue);
-		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+				       struct rkisp1_kstats_buffer, buff.queue);
+		list_del(&buf->buff.queue);
+		vb2_buffer_done(&buf->buff.vb.vb2_buf, VB2_BUF_STATE_ERROR);
 	}
 	spin_unlock_irqrestore(&stats->stats_lock, flags);
 }
@@ -207,7 +192,7 @@ rkisp1_stats_init_vb2_queue(struct vb2_queue *q, struct rkisp1_stats *stats)
 	q->drv_priv = stats;
 	q->ops = &rkisp1_stats_vb2_ops;
 	q->mem_ops = &vb2_vmalloc_memops;
-	q->buf_struct_size = sizeof(struct rkisp1_buffer);
+	q->buf_struct_size = sizeof(struct rkisp1_kstats_buffer);
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	q->lock = &node->vlock;
 
@@ -325,85 +310,81 @@ 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)
+irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
 {
+	struct device *dev = ctx;
+	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
+	struct rkisp1_stats *stats = &rkisp1->stats;
+	struct rkisp1_kstats_buffer *kstats_buf = NULL;
 	struct rkisp1_stat_buffer *cur_stat_buf;
-	struct rkisp1_buffer *cur_buf = NULL;
-	unsigned int frame_sequence =
-		atomic_read(&stats->rkisp1->isp.frame_sequence);
-	u64 timestamp = ktime_get_ns();
 	unsigned long flags;
-
-	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;
-	}
+	u64 timestamp = ktime_get_ns();
 
 	spin_lock_irqsave(&stats->stats_lock, flags);
-	/* 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);
+	if (!stats->is_streaming) {
+		spin_unlock_irqrestore(&stats->stats_lock, flags);
+		return IRQ_HANDLED;
+	}
+	if (list_empty(&stats->stat)) {
+		spin_unlock_irqrestore(&stats->stats_lock, flags);
+		WARN("%s: threaded irq waked but there are no buffers",
+		     __func__);
+		return IRQ_HANDLED;
+	}
+	kstats_buf = list_first_entry(&stats->stat,
+				      struct rkisp1_kstats_buffer, buff.queue);
+
+	/*
+	 * each waked irq thread reads exactly one ready statistics
+	 * so it is a bug  if no statistics are ready
+	 */
+	if (!kstats_buf->ris) {
+		spin_unlock_irqrestore(&stats->stats_lock, flags);
+		WARN("%s: threaded irq waked but buffer holds no measures",
+		     __func__);
+		return IRQ_HANDLED;
 	}
+	list_del(&kstats_buf->buff.queue);
 	spin_unlock_irqrestore(&stats->stats_lock, flags);
 
-	if (!cur_buf)
-		return;
-
 	cur_stat_buf =
-		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
+		(struct rkisp1_stat_buffer *)(kstats_buf->buff.vaddr[0]);
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE) {
+	if (kstats_buf->ris & RKISP1_CIF_ISP_AWB_DONE) {
 		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
 		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AWB;
 	}
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN) {
+	if (kstats_buf->ris & RKISP1_CIF_ISP_AFM_FIN) {
 		rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
 		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
 	}
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
+	if (kstats_buf->ris & RKISP1_CIF_ISP_EXP_END) {
 		rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
 		rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
 		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
 	}
 
-	if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
+	if (kstats_buf->ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
 		rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
 		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
 	}
 
-	vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
+	vb2_set_plane_payload(&kstats_buf->buff.vb.vb2_buf, 0,
 			      sizeof(struct rkisp1_stat_buffer));
-	cur_buf->vb.sequence = frame_sequence;
-	cur_buf->vb.vb2_buf.timestamp = timestamp;
-	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+	kstats_buf->buff.vb.vb2_buf.timestamp = timestamp;
+	vb2_buffer_done(&kstats_buf->buff.vb.vb2_buf, VB2_BUF_STATE_DONE);
+	return IRQ_HANDLED;
 }
 
-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)
+irqreturn_t 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;
+	struct rkisp1_isp *isp = &rkisp1->isp;
+	struct rkisp1_kstats_buffer *buf = NULL;
+	irqreturn_t ret = IRQ_HANDLED;
 	unsigned int isp_mis_tmp = 0;
 	unsigned long flags;
 	u32 val;
@@ -417,28 +398,22 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
 		rkisp1->debug.stats_error++;
 
-	if (!stats->is_streaming)
+	if (!stats->is_streaming || !(isp_ris & RKISP1_STATS_MEAS_MASK))
 		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");
+
+	list_for_each_entry(buf, &stats->stat, buff.queue) {
+		if (!buf->ris) {
+			buf->buff.vb.sequence =
+				atomic_read(&isp->frame_sequence);
+			buf->ris = isp_ris;
+			ret = IRQ_WAKE_THREAD;
+			break;
 		}
 	}
 
 unlock:
 	spin_unlock_irqrestore(&stats->stats_lock, flags);
+	return ret;
 }
 
 static void rkisp1_init_stats(struct rkisp1_stats *stats)
@@ -489,19 +464,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:
@@ -515,7 +479,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 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7822 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.7-rc5 next-20200512]
[cannot apply to staging/staging-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/media-staging-rkisp1-change-workqueue-to-threaded-irq-in-stats/20200512-200942
base:   git://linuxtv.org/media_tree.git master
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

In file included from drivers/staging/media/rkisp1/rkisp1-stats.c:14:
drivers/staging/media/rkisp1/rkisp1-common.h:309:1: error: unknown type name 'irqreturn_t'
309 | irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:310:1: error: unknown type name 'irqreturn_t'
310 | irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:311:1: error: unknown type name 'irqreturn_t'
311 | irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:312:1: error: unknown type name 'irqreturn_t'
312 | irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-common.h:313:1: error: unknown type name 'irqreturn_t'
313 | irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
| ^~~~~~~~~~~
>> drivers/staging/media/rkisp1/rkisp1-stats.c:313:1: error: unknown type name 'irqreturn_t'
313 | irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-stats.c: In function 'rkisp1_read_stats_threaded_irq':
>> drivers/staging/media/rkisp1/rkisp1-stats.c:326:10: error: 'IRQ_HANDLED' undeclared (first use in this function)
326 |   return IRQ_HANDLED;
|          ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-stats.c:326:10: note: each undeclared identifier is reported only once for each function it appears in
drivers/staging/media/rkisp1/rkisp1-stats.c: At top level:
drivers/staging/media/rkisp1/rkisp1-stats.c:382:1: error: unknown type name 'irqreturn_t'
382 | irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
| ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-stats.c: In function 'rkisp1_stats_isr':
drivers/staging/media/rkisp1/rkisp1-stats.c:387:2: error: unknown type name 'irqreturn_t'
387 |  irqreturn_t ret = IRQ_HANDLED;
|  ^~~~~~~~~~~
drivers/staging/media/rkisp1/rkisp1-stats.c:387:20: error: 'IRQ_HANDLED' undeclared (first use in this function)
387 |  irqreturn_t ret = IRQ_HANDLED;
|                    ^~~~~~~~~~~
>> drivers/staging/media/rkisp1/rkisp1-stats.c:409:10: error: 'IRQ_WAKE_THREAD' undeclared (first use in this function); did you mean 'RUSAGE_THREAD'?
409 |    ret = IRQ_WAKE_THREAD;
|          ^~~~~~~~~~~~~~~
|          RUSAGE_THREAD
drivers/staging/media/rkisp1/rkisp1-stats.c: In function 'rkisp1_read_stats_threaded_irq':
>> drivers/staging/media/rkisp1/rkisp1-stats.c:379:1: warning: control reaches end of non-void function [-Wreturn-type]
379 | }
| ^

vim +/irqreturn_t +313 drivers/staging/media/rkisp1/rkisp1-stats.c

   312	
 > 313	irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
   314	{
   315		struct device *dev = ctx;
   316		struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
   317		struct rkisp1_stats *stats = &rkisp1->stats;
   318		struct rkisp1_kstats_buffer *kstats_buf = NULL;
   319		struct rkisp1_stat_buffer *cur_stat_buf;
   320		unsigned long flags;
   321		u64 timestamp = ktime_get_ns();
   322	
   323		spin_lock_irqsave(&stats->stats_lock, flags);
   324		if (!stats->is_streaming) {
   325			spin_unlock_irqrestore(&stats->stats_lock, flags);
 > 326			return IRQ_HANDLED;
   327		}
   328		if (list_empty(&stats->stat)) {
   329			spin_unlock_irqrestore(&stats->stats_lock, flags);
   330			WARN("%s: threaded irq waked but there are no buffers",
   331			     __func__);
   332			return IRQ_HANDLED;
   333		}
   334		kstats_buf = list_first_entry(&stats->stat,
   335					      struct rkisp1_kstats_buffer, buff.queue);
   336	
   337		/*
   338		 * each waked irq thread reads exactly one ready statistics
   339		 * so it is a bug  if no statistics are ready
   340		 */
   341		if (!kstats_buf->ris) {
   342			spin_unlock_irqrestore(&stats->stats_lock, flags);
   343			WARN("%s: threaded irq waked but buffer holds no measures",
   344			     __func__);
   345			return IRQ_HANDLED;
   346		}
   347		list_del(&kstats_buf->buff.queue);
   348		spin_unlock_irqrestore(&stats->stats_lock, flags);
   349	
   350		cur_stat_buf =
   351			(struct rkisp1_stat_buffer *)(kstats_buf->buff.vaddr[0]);
   352	
   353		if (kstats_buf->ris & RKISP1_CIF_ISP_AWB_DONE) {
   354			rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
   355			cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AWB;
   356		}
   357	
   358		if (kstats_buf->ris & RKISP1_CIF_ISP_AFM_FIN) {
   359			rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
   360			cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
   361		}
   362	
   363		if (kstats_buf->ris & RKISP1_CIF_ISP_EXP_END) {
   364			rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
   365			rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
   366			cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
   367		}
   368	
   369		if (kstats_buf->ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
   370			rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
   371			cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
   372		}
   373	
   374		vb2_set_plane_payload(&kstats_buf->buff.vb.vb2_buf, 0,
   375				      sizeof(struct rkisp1_stat_buffer));
   376		kstats_buf->buff.vb.vb2_buf.timestamp = timestamp;
   377		vb2_buffer_done(&kstats_buf->buff.vb.vb2_buf, VB2_BUF_STATE_DONE);
   378		return IRQ_HANDLED;
 > 379	}
   380	
   381	
   382	irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
   383	{
   384		struct rkisp1_device *rkisp1 = stats->rkisp1;
   385		struct rkisp1_isp *isp = &rkisp1->isp;
   386		struct rkisp1_kstats_buffer *buf = NULL;
 > 387		irqreturn_t ret = IRQ_HANDLED;
   388		unsigned int isp_mis_tmp = 0;
   389		unsigned long flags;
   390		u32 val;
   391	
   392		spin_lock_irqsave(&stats->stats_lock, flags);
   393	
   394		val = RKISP1_STATS_MEAS_MASK;
   395		rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
   396	
   397		isp_mis_tmp = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
   398		if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
   399			rkisp1->debug.stats_error++;
   400	
   401		if (!stats->is_streaming || !(isp_ris & RKISP1_STATS_MEAS_MASK))
   402			goto unlock;
   403	
   404		list_for_each_entry(buf, &stats->stat, buff.queue) {
   405			if (!buf->ris) {
   406				buf->buff.vb.sequence =
   407					atomic_read(&isp->frame_sequence);
   408				buf->ris = isp_ris;
 > 409				ret = IRQ_WAKE_THREAD;
   410				break;
   411			}
   412		}
   413	
   414	unlock:
   415		spin_unlock_irqrestore(&stats->stats_lock, flags);
   416		return ret;
   417	}
   418	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55567 bytes --]

Re: [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Tue, May 12, 2020 at 02:05:22PM +0200, Dafna Hirschfeld wrote:
> Reading the statistics registers might take too long
> to run inside the irq handler. Currently it is deferred
> to bottom half using workqueues. This patch replaces the
> workqueue with threaded interrupts for reading the
> statistics registers.

Could you please explain why this is needed/desired ? Removal of the
kzalloc(GFP_ATOMIC) is very nice, but I'm sure there would have been
ways to avoid it while keeping a workqueue. I'm not claiming the patch
is bad, but I'd like to understand the reason.

> A new struct type 'rkisp1_kstats_buffer' is used as the statistics
> buffers. The struct has a field 'ris' which is the flags of ready
> statistics. If new statistics are ready, the irq handler sets
> this variable and the frame sequence on the next available buffer
> and returns IRQ_WAKE_THREAD.
> Then the threaded interrupt reads the registers and calls
> vb2_buffer_done.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO            |   1 -
>  drivers/staging/media/rkisp1/rkisp1-common.h |   5 +-
>  drivers/staging/media/rkisp1/rkisp1-dev.c    |   8 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c    |   5 +-
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 167 ++++++++-----------
>  5 files changed, 76 insertions(+), 110 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 c635bb0a7727..c8adcdf661ab 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -190,8 +190,6 @@ struct rkisp1_stats {
>  	struct list_head stat;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
> -
> -	struct workqueue_struct *readout_wq;
>  };
>  
>  /*
> @@ -308,10 +306,11 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
>  
>  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>  
> +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx);
>  irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>  irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>  irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
> +irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>  
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index b7f43dab71c8..12e2e8559acd 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -405,6 +405,8 @@ irqreturn_t rkisp1_isr(int irq, void *ctx)
>  	isp_ret = rkisp1_isp_isr(rkisp1);
>  	mipi_ret = rkisp1_mipi_isr(rkisp1);
>  
> +	if (isp_ret == IRQ_WAKE_THREAD)
> +		return IRQ_WAKE_THREAD;
>  	if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
>  		return IRQ_NONE;

Let me slightly modify my proposal from another patch in this series:

	irqreturn_t ret = IRQ_NONE;
	...

	ret |= rkisp1_capture_isr(rkisp1);
	ret |= rkisp1_isp_isr(rkisp1);
	ret |= rkisp1_mipi_isr(rkisp1);

	return ret & IRQ_WAKE_THREAD ? IRQ_WAKE_THREAD : ret;

Up to you.

>  
> @@ -490,8 +492,10 @@ static int rkisp1_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> -	ret = devm_request_irq(dev, irq, rkisp1_isr, IRQF_SHARED,
> -			       dev_driver_string(dev), dev);
> +	ret = devm_request_threaded_irq(dev, irq, rkisp1_isr,
> +					rkisp1_read_stats_threaded_irq,
> +					IRQF_SHARED,
> +					dev_driver_string(dev), dev);
>  	if (ret) {
>  		dev_err(dev, "request irq failed: %d\n", ret);
>  		return ret;
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 49b47e1734b0..09893073af00 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1111,6 +1111,7 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>  irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  {
>  	u32 status, isp_err;
> +	irqreturn_t ret = IRQ_HANDLED;
>  
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>  	if (!status)
> @@ -1138,7 +1139,7 @@ irqreturn_t 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_STATS_MEAS_MASK)
> -			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
> +			ret = rkisp1_stats_isr(&rkisp1->stats, isp_ris);

Overriding the ret variable isn't very nice, even if it should work in
practice here as ret is already equal to IRQ_HANDLED and
rkisp1_stats_isr() returns IRQ_HANDLED or IRQ_WAKE_THREAD, never
IRQ_NONE. You could however write

			ret |= rkisp1_stats_isr(&rkisp1->stats, isp_ris);

as IRQ_HANDLED and IRQ_WAKE_THREAD are independent bit flags to
accumulate them, provided you return IRQ_WAKE_THREAD from rkisp1_isr()
when both IRQ_WAKE_THREAD and IRQ_HANDLED are set (it seems the the core
IRQ code can't deal with both being set).

>  	}
>  
>  	/*
> @@ -1148,5 +1149,5 @@ irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  	 */
>  	rkisp1_params_isr(rkisp1, status);
>  
> -	return IRQ_HANDLED;
> +	return ret;
>  }
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index e6fb2c5f3b3e..f5eaa81362ea 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -18,19 +18,9 @@
>  #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;
> +struct rkisp1_kstats_buffer {
> +	struct rkisp1_buffer buff;
> +	u32 ris;
>  };
>  
>  static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
> @@ -126,16 +116,17 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
>  static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> -	struct rkisp1_buffer *stats_buf =
> -		container_of(vbuf, struct rkisp1_buffer, vb);
> +	struct rkisp1_kstats_buffer *kstats_buf =
> +		container_of(vbuf, struct rkisp1_kstats_buffer, buff.vb);
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct rkisp1_stats *stats_dev = vq->drv_priv;
>  	unsigned long flags;
>  
> -	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
> +	kstats_buf->buff.vaddr[0] = vb2_plane_vaddr(vb, 0);
> +	kstats_buf->ris = 0;
>  
>  	spin_lock_irqsave(&stats_dev->stats_lock, flags);
> -	list_add_tail(&stats_buf->queue, &stats_dev->stat);
> +	list_add_tail(&kstats_buf->buff.queue, &stats_dev->stat);
>  	spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
>  }
>  
> @@ -152,25 +143,19 @@ static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
>  static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rkisp1_stats *stats = vq->drv_priv;
> -	struct rkisp1_buffer *buf;
> +	struct rkisp1_kstats_buffer *buf;
>  	unsigned long flags;
>  	unsigned int i;
>  
> -	/* Make sure no new work queued in isr before draining wq */
>  	spin_lock_irqsave(&stats->stats_lock, flags);
>  	stats->is_streaming = false;
> -	spin_unlock_irqrestore(&stats->stats_lock, flags);
> -
> -	drain_workqueue(stats->readout_wq);
> -
> -	spin_lock_irqsave(&stats->stats_lock, flags);
>  	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>  		if (list_empty(&stats->stat))
>  			break;
>  		buf = list_first_entry(&stats->stat,
> -				       struct rkisp1_buffer, queue);
> -		list_del(&buf->queue);
> -		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +				       struct rkisp1_kstats_buffer, buff.queue);
> +		list_del(&buf->buff.queue);
> +		vb2_buffer_done(&buf->buff.vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  	}
>  	spin_unlock_irqrestore(&stats->stats_lock, flags);
>  }
> @@ -207,7 +192,7 @@ rkisp1_stats_init_vb2_queue(struct vb2_queue *q, struct rkisp1_stats *stats)
>  	q->drv_priv = stats;
>  	q->ops = &rkisp1_stats_vb2_ops;
>  	q->mem_ops = &vb2_vmalloc_memops;
> -	q->buf_struct_size = sizeof(struct rkisp1_buffer);
> +	q->buf_struct_size = sizeof(struct rkisp1_kstats_buffer);
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	q->lock = &node->vlock;
>  
> @@ -325,85 +310,81 @@ 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)
> +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
>  {
> +	struct device *dev = ctx;
> +	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +	struct rkisp1_stats *stats = &rkisp1->stats;
> +	struct rkisp1_kstats_buffer *kstats_buf = NULL;
>  	struct rkisp1_stat_buffer *cur_stat_buf;
> -	struct rkisp1_buffer *cur_buf = NULL;
> -	unsigned int frame_sequence =
> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
> -	u64 timestamp = ktime_get_ns();
>  	unsigned long flags;
> -
> -	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;
> -	}
> +	u64 timestamp = ktime_get_ns();
>  
>  	spin_lock_irqsave(&stats->stats_lock, flags);
> -	/* 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);
> +	if (!stats->is_streaming) {
> +		spin_unlock_irqrestore(&stats->stats_lock, flags);
> +		return IRQ_HANDLED;
> +	}
> +	if (list_empty(&stats->stat)) {
> +		spin_unlock_irqrestore(&stats->stats_lock, flags);
> +		WARN("%s: threaded irq waked but there are no buffers",
> +		     __func__);
> +		return IRQ_HANDLED;
> +	}
> +	kstats_buf = list_first_entry(&stats->stat,
> +				      struct rkisp1_kstats_buffer, buff.queue);
> +
> +	/*
> +	 * each waked irq thread reads exactly one ready statistics
> +	 * so it is a bug  if no statistics are ready
> +	 */

What happens if this function takes too long to run ? With the
workqueue, if the work gets delayed once in a while, the work items will
accumulate, and all of them will be handled eventually. Here it seems
that an IRQ could get lost.

> +	if (!kstats_buf->ris) {
> +		spin_unlock_irqrestore(&stats->stats_lock, flags);
> +		WARN("%s: threaded irq waked but buffer holds no measures",
> +		     __func__);
> +		return IRQ_HANDLED;
>  	}
> +	list_del(&kstats_buf->buff.queue);
>  	spin_unlock_irqrestore(&stats->stats_lock, flags);
>  
> -	if (!cur_buf)
> -		return;
> -
>  	cur_stat_buf =
> -		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
> +		(struct rkisp1_stat_buffer *)(kstats_buf->buff.vaddr[0]);
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE) {
> +	if (kstats_buf->ris & RKISP1_CIF_ISP_AWB_DONE) {
>  		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
>  		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AWB;
>  	}
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN) {
> +	if (kstats_buf->ris & RKISP1_CIF_ISP_AFM_FIN) {
>  		rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
>  		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
>  	}
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
> +	if (kstats_buf->ris & RKISP1_CIF_ISP_EXP_END) {
>  		rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
>  		rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
>  		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
>  	}
>  
> -	if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
> +	if (kstats_buf->ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
>  		rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
>  		cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
>  	}
>  
> -	vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
> +	vb2_set_plane_payload(&kstats_buf->buff.vb.vb2_buf, 0,
>  			      sizeof(struct rkisp1_stat_buffer));
> -	cur_buf->vb.sequence = frame_sequence;
> -	cur_buf->vb.vb2_buf.timestamp = timestamp;
> -	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	kstats_buf->buff.vb.vb2_buf.timestamp = timestamp;
> +	vb2_buffer_done(&kstats_buf->buff.vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	return IRQ_HANDLED;
>  }
>  
> -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);
>  

Is the second blank line intentional ?

> -	kfree(readout_work);
> -}
> -
> -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> +irqreturn_t 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;
> +	struct rkisp1_isp *isp = &rkisp1->isp;
> +	struct rkisp1_kstats_buffer *buf = NULL;
> +	irqreturn_t ret = IRQ_HANDLED;
>  	unsigned int isp_mis_tmp = 0;
>  	unsigned long flags;
>  	u32 val;
> @@ -417,28 +398,22 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
>  		rkisp1->debug.stats_error++;
>  
> -	if (!stats->is_streaming)
> +	if (!stats->is_streaming || !(isp_ris & RKISP1_STATS_MEAS_MASK))
>  		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");
> +
> +	list_for_each_entry(buf, &stats->stat, buff.queue) {
> +		if (!buf->ris) {
> +			buf->buff.vb.sequence =
> +				atomic_read(&isp->frame_sequence);
> +			buf->ris = isp_ris;
> +			ret = IRQ_WAKE_THREAD;
> +			break;
>  		}
>  	}
>  
>  unlock:
>  	spin_unlock_irqrestore(&stats->stats_lock, flags);
> +	return ret;
>  }
>  
>  static void rkisp1_init_stats(struct rkisp1_stats *stats)
> @@ -489,19 +464,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:
> @@ -515,7 +479,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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[Tomasz Figa]

Hi Dafna,

On Thu, May 21, 2020 at 2:09 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dafna,
>
> Thank you for the patch.
>
> On Tue, May 12, 2020 at 02:05:22PM +0200, Dafna Hirschfeld wrote:
> > Reading the statistics registers might take too long
> > to run inside the irq handler. Currently it is deferred
> > to bottom half using workqueues. This patch replaces the
> > workqueue with threaded interrupts for reading the
> > statistics registers.
>
> Could you please explain why this is needed/desired ? Removal of the
> kzalloc(GFP_ATOMIC) is very nice, but I'm sure there would have been
> ways to avoid it while keeping a workqueue. I'm not claiming the patch
> is bad, but I'd like to understand the reason.
>

Thanks a lot for working on this driver!

I'll second what Laurent said. In general, a description of the patch
should explain why a change is needed, e.g. what issues it fixes or
what improvements it brings.

Also, would you mind adding me on CC for any patches for this driver?
Since we use this driver in Chrome OS, I'd like to stay on top of any
updates. Thanks in advance!

Best regards,
Tomasz

> > A new struct type 'rkisp1_kstats_buffer' is used as the statistics
> > buffers. The struct has a field 'ris' which is the flags of ready
> > statistics. If new statistics are ready, the irq handler sets
> > this variable and the frame sequence on the next available buffer
> > and returns IRQ_WAKE_THREAD.
> > Then the threaded interrupt reads the registers and calls
> > vb2_buffer_done.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/TODO            |   1 -
> >  drivers/staging/media/rkisp1/rkisp1-common.h |   5 +-
> >  drivers/staging/media/rkisp1/rkisp1-dev.c    |   8 +-
> >  drivers/staging/media/rkisp1/rkisp1-isp.c    |   5 +-
> >  drivers/staging/media/rkisp1/rkisp1-stats.c  | 167 ++++++++-----------
> >  5 files changed, 76 insertions(+), 110 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 c635bb0a7727..c8adcdf661ab 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> > +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> > @@ -190,8 +190,6 @@ struct rkisp1_stats {
> >       struct list_head stat;
> >       struct v4l2_format vdev_fmt;
> >       bool is_streaming;
> > -
> > -     struct workqueue_struct *readout_wq;
> >  };
> >
> >  /*
> > @@ -308,10 +306,11 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
> >
> >  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
> >
> > +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx);
> >  irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> >  irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> >  irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> > -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
> > +irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
> >  void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
> >
> >  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> > index b7f43dab71c8..12e2e8559acd 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> > @@ -405,6 +405,8 @@ irqreturn_t rkisp1_isr(int irq, void *ctx)
> >       isp_ret = rkisp1_isp_isr(rkisp1);
> >       mipi_ret = rkisp1_mipi_isr(rkisp1);
> >
> > +     if (isp_ret == IRQ_WAKE_THREAD)
> > +             return IRQ_WAKE_THREAD;
> >       if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
> >               return IRQ_NONE;
>
> Let me slightly modify my proposal from another patch in this series:
>
>         irqreturn_t ret = IRQ_NONE;
>         ...
>
>         ret |= rkisp1_capture_isr(rkisp1);
>         ret |= rkisp1_isp_isr(rkisp1);
>         ret |= rkisp1_mipi_isr(rkisp1);
>
>         return ret & IRQ_WAKE_THREAD ? IRQ_WAKE_THREAD : ret;
>
> Up to you.
>
> >
> > @@ -490,8 +492,10 @@ static int rkisp1_probe(struct platform_device *pdev)
> >       if (irq < 0)
> >               return irq;
> >
> > -     ret = devm_request_irq(dev, irq, rkisp1_isr, IRQF_SHARED,
> > -                            dev_driver_string(dev), dev);
> > +     ret = devm_request_threaded_irq(dev, irq, rkisp1_isr,
> > +                                     rkisp1_read_stats_threaded_irq,
> > +                                     IRQF_SHARED,
> > +                                     dev_driver_string(dev), dev);
> >       if (ret) {
> >               dev_err(dev, "request irq failed: %d\n", ret);
> >               return ret;
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > index 49b47e1734b0..09893073af00 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > @@ -1111,6 +1111,7 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> >  irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> >  {
> >       u32 status, isp_err;
> > +     irqreturn_t ret = IRQ_HANDLED;
> >
> >       status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
> >       if (!status)
> > @@ -1138,7 +1139,7 @@ irqreturn_t 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_STATS_MEAS_MASK)
> > -                     rkisp1_stats_isr(&rkisp1->stats, isp_ris);
> > +                     ret = rkisp1_stats_isr(&rkisp1->stats, isp_ris);
>
> Overriding the ret variable isn't very nice, even if it should work in
> practice here as ret is already equal to IRQ_HANDLED and
> rkisp1_stats_isr() returns IRQ_HANDLED or IRQ_WAKE_THREAD, never
> IRQ_NONE. You could however write
>
>                         ret |= rkisp1_stats_isr(&rkisp1->stats, isp_ris);
>
> as IRQ_HANDLED and IRQ_WAKE_THREAD are independent bit flags to
> accumulate them, provided you return IRQ_WAKE_THREAD from rkisp1_isr()
> when both IRQ_WAKE_THREAD and IRQ_HANDLED are set (it seems the the core
> IRQ code can't deal with both being set).
>
> >       }
> >
> >       /*
> > @@ -1148,5 +1149,5 @@ irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> >        */
> >       rkisp1_params_isr(rkisp1, status);
> >
> > -     return IRQ_HANDLED;
> > +     return ret;
> >  }
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> > index e6fb2c5f3b3e..f5eaa81362ea 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> > @@ -18,19 +18,9 @@
> >  #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;
> > +struct rkisp1_kstats_buffer {
> > +     struct rkisp1_buffer buff;
> > +     u32 ris;
> >  };
> >
> >  static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
> > @@ -126,16 +116,17 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
> >  static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
> >  {
> >       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > -     struct rkisp1_buffer *stats_buf =
> > -             container_of(vbuf, struct rkisp1_buffer, vb);
> > +     struct rkisp1_kstats_buffer *kstats_buf =
> > +             container_of(vbuf, struct rkisp1_kstats_buffer, buff.vb);
> >       struct vb2_queue *vq = vb->vb2_queue;
> >       struct rkisp1_stats *stats_dev = vq->drv_priv;
> >       unsigned long flags;
> >
> > -     stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
> > +     kstats_buf->buff.vaddr[0] = vb2_plane_vaddr(vb, 0);
> > +     kstats_buf->ris = 0;
> >
> >       spin_lock_irqsave(&stats_dev->stats_lock, flags);
> > -     list_add_tail(&stats_buf->queue, &stats_dev->stat);
> > +     list_add_tail(&kstats_buf->buff.queue, &stats_dev->stat);
> >       spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
> >  }
> >
> > @@ -152,25 +143,19 @@ static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
> >  static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
> >  {
> >       struct rkisp1_stats *stats = vq->drv_priv;
> > -     struct rkisp1_buffer *buf;
> > +     struct rkisp1_kstats_buffer *buf;
> >       unsigned long flags;
> >       unsigned int i;
> >
> > -     /* Make sure no new work queued in isr before draining wq */
> >       spin_lock_irqsave(&stats->stats_lock, flags);
> >       stats->is_streaming = false;
> > -     spin_unlock_irqrestore(&stats->stats_lock, flags);
> > -
> > -     drain_workqueue(stats->readout_wq);
> > -
> > -     spin_lock_irqsave(&stats->stats_lock, flags);
> >       for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
> >               if (list_empty(&stats->stat))
> >                       break;
> >               buf = list_first_entry(&stats->stat,
> > -                                    struct rkisp1_buffer, queue);
> > -             list_del(&buf->queue);
> > -             vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > +                                    struct rkisp1_kstats_buffer, buff.queue);
> > +             list_del(&buf->buff.queue);
> > +             vb2_buffer_done(&buf->buff.vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >       }
> >       spin_unlock_irqrestore(&stats->stats_lock, flags);
> >  }
> > @@ -207,7 +192,7 @@ rkisp1_stats_init_vb2_queue(struct vb2_queue *q, struct rkisp1_stats *stats)
> >       q->drv_priv = stats;
> >       q->ops = &rkisp1_stats_vb2_ops;
> >       q->mem_ops = &vb2_vmalloc_memops;
> > -     q->buf_struct_size = sizeof(struct rkisp1_buffer);
> > +     q->buf_struct_size = sizeof(struct rkisp1_kstats_buffer);
> >       q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >       q->lock = &node->vlock;
> >
> > @@ -325,85 +310,81 @@ 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)
> > +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
> >  {
> > +     struct device *dev = ctx;
> > +     struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > +     struct rkisp1_stats *stats = &rkisp1->stats;
> > +     struct rkisp1_kstats_buffer *kstats_buf = NULL;
> >       struct rkisp1_stat_buffer *cur_stat_buf;
> > -     struct rkisp1_buffer *cur_buf = NULL;
> > -     unsigned int frame_sequence =
> > -             atomic_read(&stats->rkisp1->isp.frame_sequence);
> > -     u64 timestamp = ktime_get_ns();
> >       unsigned long flags;
> > -
> > -     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;
> > -     }
> > +     u64 timestamp = ktime_get_ns();
> >
> >       spin_lock_irqsave(&stats->stats_lock, flags);
> > -     /* 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);
> > +     if (!stats->is_streaming) {
> > +             spin_unlock_irqrestore(&stats->stats_lock, flags);
> > +             return IRQ_HANDLED;
> > +     }
> > +     if (list_empty(&stats->stat)) {
> > +             spin_unlock_irqrestore(&stats->stats_lock, flags);
> > +             WARN("%s: threaded irq waked but there are no buffers",
> > +                  __func__);
> > +             return IRQ_HANDLED;
> > +     }
> > +     kstats_buf = list_first_entry(&stats->stat,
> > +                                   struct rkisp1_kstats_buffer, buff.queue);
> > +
> > +     /*
> > +      * each waked irq thread reads exactly one ready statistics
> > +      * so it is a bug  if no statistics are ready
> > +      */
>
> What happens if this function takes too long to run ? With the
> workqueue, if the work gets delayed once in a while, the work items will
> accumulate, and all of them will be handled eventually. Here it seems
> that an IRQ could get lost.
>
> > +     if (!kstats_buf->ris) {
> > +             spin_unlock_irqrestore(&stats->stats_lock, flags);
> > +             WARN("%s: threaded irq waked but buffer holds no measures",
> > +                  __func__);
> > +             return IRQ_HANDLED;
> >       }
> > +     list_del(&kstats_buf->buff.queue);
> >       spin_unlock_irqrestore(&stats->stats_lock, flags);
> >
> > -     if (!cur_buf)
> > -             return;
> > -
> >       cur_stat_buf =
> > -             (struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
> > +             (struct rkisp1_stat_buffer *)(kstats_buf->buff.vaddr[0]);
> >
> > -     if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE) {
> > +     if (kstats_buf->ris & RKISP1_CIF_ISP_AWB_DONE) {
> >               rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
> >               cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AWB;
> >       }
> >
> > -     if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN) {
> > +     if (kstats_buf->ris & RKISP1_CIF_ISP_AFM_FIN) {
> >               rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
> >               cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
> >       }
> >
> > -     if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
> > +     if (kstats_buf->ris & RKISP1_CIF_ISP_EXP_END) {
> >               rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
> >               rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
> >               cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
> >       }
> >
> > -     if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
> > +     if (kstats_buf->ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
> >               rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
> >               cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
> >       }
> >
> > -     vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
> > +     vb2_set_plane_payload(&kstats_buf->buff.vb.vb2_buf, 0,
> >                             sizeof(struct rkisp1_stat_buffer));
> > -     cur_buf->vb.sequence = frame_sequence;
> > -     cur_buf->vb.vb2_buf.timestamp = timestamp;
> > -     vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > +     kstats_buf->buff.vb.vb2_buf.timestamp = timestamp;
> > +     vb2_buffer_done(&kstats_buf->buff.vb.vb2_buf, VB2_BUF_STATE_DONE);
> > +     return IRQ_HANDLED;
> >  }
> >
> > -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);
> >
>
> Is the second blank line intentional ?
>
> > -     kfree(readout_work);
> > -}
> > -
> > -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> > +irqreturn_t 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;
> > +     struct rkisp1_isp *isp = &rkisp1->isp;
> > +     struct rkisp1_kstats_buffer *buf = NULL;
> > +     irqreturn_t ret = IRQ_HANDLED;
> >       unsigned int isp_mis_tmp = 0;
> >       unsigned long flags;
> >       u32 val;
> > @@ -417,28 +398,22 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> >       if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
> >               rkisp1->debug.stats_error++;
> >
> > -     if (!stats->is_streaming)
> > +     if (!stats->is_streaming || !(isp_ris & RKISP1_STATS_MEAS_MASK))
> >               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");
> > +
> > +     list_for_each_entry(buf, &stats->stat, buff.queue) {
> > +             if (!buf->ris) {
> > +                     buf->buff.vb.sequence =
> > +                             atomic_read(&isp->frame_sequence);
> > +                     buf->ris = isp_ris;
> > +                     ret = IRQ_WAKE_THREAD;
> > +                     break;
> >               }
> >       }
> >
> >  unlock:
> >       spin_unlock_irqrestore(&stats->stats_lock, flags);
> > +     return ret;
> >  }
> >
> >  static void rkisp1_init_stats(struct rkisp1_stats *stats)
> > @@ -489,19 +464,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:
> > @@ -515,7 +479,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);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[Dafna Hirschfeld]

Hi, Tomasz, Laurent, and everyone

On 21.05.20 12:38, Tomasz Figa wrote:
> Hi Dafna,
> 
> On Thu, May 21, 2020 at 2:09 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Dafna,
>>
>> Thank you for the patch.
>>
>> On Tue, May 12, 2020 at 02:05:22PM +0200, Dafna Hirschfeld wrote:
>>> Reading the statistics registers might take too long
>>> to run inside the irq handler. Currently it is deferred
>>> to bottom half using workqueues. This patch replaces the
>>> workqueue with threaded interrupts for reading the
>>> statistics registers.
>>
>> Could you please explain why this is needed/desired ? Removal of the
>> kzalloc(GFP_ATOMIC) is very nice, but I'm sure there would have been
>> ways to avoid it while keeping a workqueue. I'm not claiming the patch
>> is bad, but I'd like to understand the reason.
>>
> 
> Thanks a lot for working on this driver!
> 
> I'll second what Laurent said. In general, a description of the patch
> should explain why a change is needed, e.g. what issues it fixes or
> what improvements it brings.

So from what I understand, the correct way to handle bottom half is
using threaded interrupts since they run in higher priority.
In case of this statistics reading for example, we want to read the stats
as fast as possible once we recieve the interrupt. If we read the stats too
long after the interrupt was recieved, then the values in the stats registerd
might have changed by then. Does that make sense?

I use the rockpi4 board to test the code and actually I did an experiment
of moving the code that reads the stats into the hard irq and it ran
fine. Maybe you know why the code is currently in a work queue and not
inside the hard irq? Also, the params struct is of size 5337 bytes and
the params subdevice can potentionaly run a lot of registers writing but
for some reasone the code of writing to the params registers still runs
inside the hard irq.

> 
> Also, would you mind adding me on CC for any patches for this driver?
> Since we use this driver in Chrome OS, I'd like to stay on top of any
> updates. Thanks in advance!
sure!

> 
> Best regards,
> Tomasz
> 
>>> A new struct type 'rkisp1_kstats_buffer' is used as the statistics
>>> buffers. The struct has a field 'ris' which is the flags of ready
>>> statistics. If new statistics are ready, the irq handler sets
>>> this variable and the frame sequence on the next available buffer
>>> and returns IRQ_WAKE_THREAD.
>>> Then the threaded interrupt reads the registers and calls
>>> vb2_buffer_done.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/TODO            |   1 -
>>>   drivers/staging/media/rkisp1/rkisp1-common.h |   5 +-
>>>   drivers/staging/media/rkisp1/rkisp1-dev.c    |   8 +-
>>>   drivers/staging/media/rkisp1/rkisp1-isp.c    |   5 +-
>>>   drivers/staging/media/rkisp1/rkisp1-stats.c  | 167 ++++++++-----------
>>>   5 files changed, 76 insertions(+), 110 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 c635bb0a7727..c8adcdf661ab 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>>> @@ -190,8 +190,6 @@ struct rkisp1_stats {
>>>        struct list_head stat;
>>>        struct v4l2_format vdev_fmt;
>>>        bool is_streaming;
>>> -
>>> -     struct workqueue_struct *readout_wq;
>>>   };
>>>
>>>   /*
>>> @@ -308,10 +306,11 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
>>>
>>>   const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>>>
>>> +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx);
>>>   irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>>>   irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>>>   irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
>>> -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
>>> +irqreturn_t rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
>>>   void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>>>
>>>   int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
>>> index b7f43dab71c8..12e2e8559acd 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
>>> @@ -405,6 +405,8 @@ irqreturn_t rkisp1_isr(int irq, void *ctx)
>>>        isp_ret = rkisp1_isp_isr(rkisp1);
>>>        mipi_ret = rkisp1_mipi_isr(rkisp1);
>>>
>>> +     if (isp_ret == IRQ_WAKE_THREAD)
>>> +             return IRQ_WAKE_THREAD;
>>>        if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
>>>                return IRQ_NONE;
>>
>> Let me slightly modify my proposal from another patch in this series:
>>
>>          irqreturn_t ret = IRQ_NONE;
>>          ...
>>
>>          ret |= rkisp1_capture_isr(rkisp1);
>>          ret |= rkisp1_isp_isr(rkisp1);
>>          ret |= rkisp1_mipi_isr(rkisp1);
>>
>>          return ret & IRQ_WAKE_THREAD ? IRQ_WAKE_THREAD : ret;
>>
>> Up to you.
>>
>>>
>>> @@ -490,8 +492,10 @@ static int rkisp1_probe(struct platform_device *pdev)
>>>        if (irq < 0)
>>>                return irq;
>>>
>>> -     ret = devm_request_irq(dev, irq, rkisp1_isr, IRQF_SHARED,
>>> -                            dev_driver_string(dev), dev);
>>> +     ret = devm_request_threaded_irq(dev, irq, rkisp1_isr,
>>> +                                     rkisp1_read_stats_threaded_irq,
>>> +                                     IRQF_SHARED,
>>> +                                     dev_driver_string(dev), dev);
>>>        if (ret) {
>>>                dev_err(dev, "request irq failed: %d\n", ret);
>>>                return ret;
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> index 49b47e1734b0..09893073af00 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> @@ -1111,6 +1111,7 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>>>   irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>>   {
>>>        u32 status, isp_err;
>>> +     irqreturn_t ret = IRQ_HANDLED;
>>>
>>>        status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>>>        if (!status)
>>> @@ -1138,7 +1139,7 @@ irqreturn_t 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_STATS_MEAS_MASK)
>>> -                     rkisp1_stats_isr(&rkisp1->stats, isp_ris);
>>> +                     ret = rkisp1_stats_isr(&rkisp1->stats, isp_ris);
>>
>> Overriding the ret variable isn't very nice, even if it should work in
>> practice here as ret is already equal to IRQ_HANDLED and
>> rkisp1_stats_isr() returns IRQ_HANDLED or IRQ_WAKE_THREAD, never
>> IRQ_NONE. You could however write
>>
>>                          ret |= rkisp1_stats_isr(&rkisp1->stats, isp_ris);
>>
>> as IRQ_HANDLED and IRQ_WAKE_THREAD are independent bit flags to
>> accumulate them, provided you return IRQ_WAKE_THREAD from rkisp1_isr()
>> when both IRQ_WAKE_THREAD and IRQ_HANDLED are set (it seems the the core
>> IRQ code can't deal with both being set).
>>
>>>        }
>>>
>>>        /*
>>> @@ -1148,5 +1149,5 @@ irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>>         */
>>>        rkisp1_params_isr(rkisp1, status);
>>>
>>> -     return IRQ_HANDLED;
>>> +     return ret;
>>>   }
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
>>> index e6fb2c5f3b3e..f5eaa81362ea 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
>>> @@ -18,19 +18,9 @@
>>>   #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;
>>> +struct rkisp1_kstats_buffer {
>>> +     struct rkisp1_buffer buff;
>>> +     u32 ris;
>>>   };
>>>
>>>   static int rkisp1_stats_enum_fmt_meta_cap(struct file *file, void *priv,
>>> @@ -126,16 +116,17 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
>>>   static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
>>>   {
>>>        struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>> -     struct rkisp1_buffer *stats_buf =
>>> -             container_of(vbuf, struct rkisp1_buffer, vb);
>>> +     struct rkisp1_kstats_buffer *kstats_buf =
>>> +             container_of(vbuf, struct rkisp1_kstats_buffer, buff.vb);
>>>        struct vb2_queue *vq = vb->vb2_queue;
>>>        struct rkisp1_stats *stats_dev = vq->drv_priv;
>>>        unsigned long flags;
>>>
>>> -     stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>>> +     kstats_buf->buff.vaddr[0] = vb2_plane_vaddr(vb, 0);
>>> +     kstats_buf->ris = 0;
>>>
>>>        spin_lock_irqsave(&stats_dev->stats_lock, flags);
>>> -     list_add_tail(&stats_buf->queue, &stats_dev->stat);
>>> +     list_add_tail(&kstats_buf->buff.queue, &stats_dev->stat);
>>>        spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
>>>   }
>>>
>>> @@ -152,25 +143,19 @@ static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
>>>   static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>>>   {
>>>        struct rkisp1_stats *stats = vq->drv_priv;
>>> -     struct rkisp1_buffer *buf;
>>> +     struct rkisp1_kstats_buffer *buf;
>>>        unsigned long flags;
>>>        unsigned int i;
>>>
>>> -     /* Make sure no new work queued in isr before draining wq */
>>>        spin_lock_irqsave(&stats->stats_lock, flags);
>>>        stats->is_streaming = false;
>>> -     spin_unlock_irqrestore(&stats->stats_lock, flags);
>>> -
>>> -     drain_workqueue(stats->readout_wq);
>>> -
>>> -     spin_lock_irqsave(&stats->stats_lock, flags);
>>>        for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>>>                if (list_empty(&stats->stat))
>>>                        break;
>>>                buf = list_first_entry(&stats->stat,
>>> -                                    struct rkisp1_buffer, queue);
>>> -             list_del(&buf->queue);
>>> -             vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>> +                                    struct rkisp1_kstats_buffer, buff.queue);
>>> +             list_del(&buf->buff.queue);
>>> +             vb2_buffer_done(&buf->buff.vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>        }
>>>        spin_unlock_irqrestore(&stats->stats_lock, flags);
>>>   }
>>> @@ -207,7 +192,7 @@ rkisp1_stats_init_vb2_queue(struct vb2_queue *q, struct rkisp1_stats *stats)
>>>        q->drv_priv = stats;
>>>        q->ops = &rkisp1_stats_vb2_ops;
>>>        q->mem_ops = &vb2_vmalloc_memops;
>>> -     q->buf_struct_size = sizeof(struct rkisp1_buffer);
>>> +     q->buf_struct_size = sizeof(struct rkisp1_kstats_buffer);
>>>        q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>>        q->lock = &node->vlock;
>>>
>>> @@ -325,85 +310,81 @@ 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)
>>> +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
>>>   {
>>> +     struct device *dev = ctx;
>>> +     struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>>> +     struct rkisp1_stats *stats = &rkisp1->stats;
>>> +     struct rkisp1_kstats_buffer *kstats_buf = NULL;
>>>        struct rkisp1_stat_buffer *cur_stat_buf;
>>> -     struct rkisp1_buffer *cur_buf = NULL;
>>> -     unsigned int frame_sequence =
>>> -             atomic_read(&stats->rkisp1->isp.frame_sequence);
>>> -     u64 timestamp = ktime_get_ns();
>>>        unsigned long flags;
>>> -
>>> -     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;
>>> -     }
>>> +     u64 timestamp = ktime_get_ns();
>>>
>>>        spin_lock_irqsave(&stats->stats_lock, flags);
>>> -     /* 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);
>>> +     if (!stats->is_streaming) {
>>> +             spin_unlock_irqrestore(&stats->stats_lock, flags);
>>> +             return IRQ_HANDLED;
>>> +     }
>>> +     if (list_empty(&stats->stat)) {
>>> +             spin_unlock_irqrestore(&stats->stats_lock, flags);
>>> +             WARN("%s: threaded irq waked but there are no buffers",
>>> +                  __func__);
>>> +             return IRQ_HANDLED;
>>> +     }
>>> +     kstats_buf = list_first_entry(&stats->stat,
>>> +                                   struct rkisp1_kstats_buffer, buff.queue);
>>> +
>>> +     /*
>>> +      * each waked irq thread reads exactly one ready statistics
>>> +      * so it is a bug  if no statistics are ready
>>> +      */
>>
>> What happens if this function takes too long to run ? With the
>> workqueue, if the work gets delayed once in a while, the work items will
>> accumulate, and all of them will be handled eventually. Here it seems
>> that an IRQ could get lost.

If the irs runs and there are no available buffers then indeed we lost the interrupt.
You think it might be a problem if userspace expect an associated stats buffer
for every recieved frame?
Also with workqueue we will lose the stats if we don't have an available buffer
when the workqueue runs.

Thanks,
Dafna
>>
>>> +     if (!kstats_buf->ris) {
>>> +             spin_unlock_irqrestore(&stats->stats_lock, flags);
>>> +             WARN("%s: threaded irq waked but buffer holds no measures",
>>> +                  __func__);
>>> +             return IRQ_HANDLED;
>>>        }
>>> +     list_del(&kstats_buf->buff.queue);
>>>        spin_unlock_irqrestore(&stats->stats_lock, flags);
>>>
>>> -     if (!cur_buf)
>>> -             return;
>>> -
>>>        cur_stat_buf =
>>> -             (struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
>>> +             (struct rkisp1_stat_buffer *)(kstats_buf->buff.vaddr[0]);
>>>
>>> -     if (meas_work->isp_ris & RKISP1_CIF_ISP_AWB_DONE) {
>>> +     if (kstats_buf->ris & RKISP1_CIF_ISP_AWB_DONE) {
>>>                rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
>>>                cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AWB;
>>>        }
>>>
>>> -     if (meas_work->isp_ris & RKISP1_CIF_ISP_AFM_FIN) {
>>> +     if (kstats_buf->ris & RKISP1_CIF_ISP_AFM_FIN) {
>>>                rkisp1_stats_get_afc_meas(stats, cur_stat_buf);
>>>                cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
>>>        }
>>>
>>> -     if (meas_work->isp_ris & RKISP1_CIF_ISP_EXP_END) {
>>> +     if (kstats_buf->ris & RKISP1_CIF_ISP_EXP_END) {
>>>                rkisp1_stats_get_aec_meas(stats, cur_stat_buf);
>>>                rkisp1_stats_get_bls_meas(stats, cur_stat_buf);
>>>                cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
>>>        }
>>>
>>> -     if (meas_work->isp_ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
>>> +     if (kstats_buf->ris & RKISP1_CIF_ISP_HIST_MEASURE_RDY) {
>>>                rkisp1_stats_get_hst_meas(stats, cur_stat_buf);
>>>                cur_stat_buf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
>>>        }
>>>
>>> -     vb2_set_plane_payload(&cur_buf->vb.vb2_buf, 0,
>>> +     vb2_set_plane_payload(&kstats_buf->buff.vb.vb2_buf, 0,
>>>                              sizeof(struct rkisp1_stat_buffer));
>>> -     cur_buf->vb.sequence = frame_sequence;
>>> -     cur_buf->vb.vb2_buf.timestamp = timestamp;
>>> -     vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>>> +     kstats_buf->buff.vb.vb2_buf.timestamp = timestamp;
>>> +     vb2_buffer_done(&kstats_buf->buff.vb.vb2_buf, VB2_BUF_STATE_DONE);
>>> +     return IRQ_HANDLED;
>>>   }
>>>
>>> -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);
>>>
>>
>> Is the second blank line intentional ?
>>
>>> -     kfree(readout_work);
>>> -}
>>> -
>>> -void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>>> +irqreturn_t 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;
>>> +     struct rkisp1_isp *isp = &rkisp1->isp;
>>> +     struct rkisp1_kstats_buffer *buf = NULL;
>>> +     irqreturn_t ret = IRQ_HANDLED;
>>>        unsigned int isp_mis_tmp = 0;
>>>        unsigned long flags;
>>>        u32 val;
>>> @@ -417,28 +398,22 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>>>        if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
>>>                rkisp1->debug.stats_error++;
>>>
>>> -     if (!stats->is_streaming)
>>> +     if (!stats->is_streaming || !(isp_ris & RKISP1_STATS_MEAS_MASK))
>>>                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");
>>> +
>>> +     list_for_each_entry(buf, &stats->stat, buff.queue) {
>>> +             if (!buf->ris) {
>>> +                     buf->buff.vb.sequence =
>>> +                             atomic_read(&isp->frame_sequence);
>>> +                     buf->ris = isp_ris;
>>> +                     ret = IRQ_WAKE_THREAD;
>>> +                     break;
>>>                }
>>>        }
>>>
>>>   unlock:
>>>        spin_unlock_irqrestore(&stats->stats_lock, flags);
>>> +     return ret;
>>>   }
>>>
>>>   static void rkisp1_init_stats(struct rkisp1_stats *stats)
>>> @@ -489,19 +464,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:
>>> @@ -515,7 +479,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);
>>
>> --
>> Regards,
>>
>> Laurent Pinchart

Re: [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers

[Tomasz Figa]

On Thu, May 28, 2020 at 9:19 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi, Tomasz, Laurent, and everyone
>
> On 21.05.20 12:38, Tomasz Figa wrote:
> > Hi Dafna,
> >
> > On Thu, May 21, 2020 at 2:09 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> Hi Dafna,
> >>
> >> Thank you for the patch.
> >>
> >> On Tue, May 12, 2020 at 02:05:22PM +0200, Dafna Hirschfeld wrote:
> >>> Reading the statistics registers might take too long
> >>> to run inside the irq handler. Currently it is deferred
> >>> to bottom half using workqueues. This patch replaces the
> >>> workqueue with threaded interrupts for reading the
> >>> statistics registers.
> >>
> >> Could you please explain why this is needed/desired ? Removal of the
> >> kzalloc(GFP_ATOMIC) is very nice, but I'm sure there would have been
> >> ways to avoid it while keeping a workqueue. I'm not claiming the patch
> >> is bad, but I'd like to understand the reason.
> >>
> >
> > Thanks a lot for working on this driver!
> >
> > I'll second what Laurent said. In general, a description of the patch
> > should explain why a change is needed, e.g. what issues it fixes or
> > what improvements it brings.
>
> So from what I understand, the correct way to handle bottom half is
> using threaded interrupts since they run in higher priority.
> In case of this statistics reading for example, we want to read the stats
> as fast as possible once we recieve the interrupt. If we read the stats too
> long after the interrupt was recieved, then the values in the stats registerd
> might have changed by then. Does that make sense?
>
> I use the rockpi4 board to test the code and actually I did an experiment
> of moving the code that reads the stats into the hard irq and it ran
> fine. Maybe you know why the code is currently in a work queue and not
> inside the hard irq? Also, the params struct is of size 5337 bytes and
> the params subdevice can potentionaly run a lot of registers writing but
> for some reasone the code of writing to the params registers still runs
> inside the hard irq.
>

Actually I now recall that we already discussed this in the past with
Ezequiel and Helen and I even measured the time needed to readout the
statistics registers and it was consistently taking between 23 to 40
microseconds. We concluded that it could be just done in the hardirq
handler.

I've added you to that email thread.

> >
> > Also, would you mind adding me on CC for any patches for this driver?
> > Since we use this driver in Chrome OS, I'd like to stay on top of any
> > updates. Thanks in advance!
> sure!
>

Thanks!

[snip]
> >>> -static void
> >>> -rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
> >>> -                           struct rkisp1_isp_readout_work *meas_work)
> >>> +irqreturn_t rkisp1_read_stats_threaded_irq(int irq, void *ctx)
> >>>   {
> >>> +     struct device *dev = ctx;
> >>> +     struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> >>> +     struct rkisp1_stats *stats = &rkisp1->stats;
> >>> +     struct rkisp1_kstats_buffer *kstats_buf = NULL;
> >>>        struct rkisp1_stat_buffer *cur_stat_buf;
> >>> -     struct rkisp1_buffer *cur_buf = NULL;
> >>> -     unsigned int frame_sequence =
> >>> -             atomic_read(&stats->rkisp1->isp.frame_sequence);
> >>> -     u64 timestamp = ktime_get_ns();
> >>>        unsigned long flags;
> >>> -
> >>> -     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;
> >>> -     }
> >>> +     u64 timestamp = ktime_get_ns();
> >>>
> >>>        spin_lock_irqsave(&stats->stats_lock, flags);
> >>> -     /* 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);
> >>> +     if (!stats->is_streaming) {
> >>> +             spin_unlock_irqrestore(&stats->stats_lock, flags);
> >>> +             return IRQ_HANDLED;
> >>> +     }
> >>> +     if (list_empty(&stats->stat)) {
> >>> +             spin_unlock_irqrestore(&stats->stats_lock, flags);
> >>> +             WARN("%s: threaded irq waked but there are no buffers",
> >>> +                  __func__);
> >>> +             return IRQ_HANDLED;
> >>> +     }
> >>> +     kstats_buf = list_first_entry(&stats->stat,
> >>> +                                   struct rkisp1_kstats_buffer, buff.queue);
> >>> +
> >>> +     /*
> >>> +      * each waked irq thread reads exactly one ready statistics
> >>> +      * so it is a bug  if no statistics are ready
> >>> +      */
> >>
> >> What happens if this function takes too long to run ? With the
> >> workqueue, if the work gets delayed once in a while, the work items will
> >> accumulate, and all of them will be handled eventually. Here it seems
> >> that an IRQ could get lost.
>
> If the irs runs and there are no available buffers then indeed we lost the interrupt.
> You think it might be a problem if userspace expect an associated stats buffer
> for every recieved frame?
> Also with workqueue we will lose the stats if we don't have an available buffer
> when the workqueue runs.

I believe this hardware doesn't do any buffering internally, so if one
doesn't read the statistics in time, the values are lost, because they
are overridden with the ones coming from the next frame. So actually
hardirq might be a better context to do the read-out indeed.

Best regards,
Tomasz