[PATCH v2 00/14] media: staging: rkisp1: various bug fixes

[PATCH v2 00/14] media: staging: rkisp1: various bug fixes

[Dafna Hirschfeld]

Fix various bugs mainly in params and stats. The patchset fixes
buffers synchronization issues discussed
https://patchwork.kernel.org/patch/11066513/#23544763

As discussed with Tomasz, we decide that in order to keep the code
simple, we assume that the v-start signal (RKISP1_CIF_ISP_V_START)
and the frame-out signal RKISP1_CIF_ISP_FRAME don't arrive together.
So when v-starts arrives the frame sequence is incremented and
when frame-out arrives, the stats and params isr are called.
Also the frame sequence of the params buffer should be the frame to
which the params are applied.

The params node needs to apply the first params buffer when
the streaming from main/selfpath starts before the first frame arrive
so that the first buffer will configure the first frame.
The way it is implemented is that the first buffer queued
to the params node is not added to the list but instead a local copy
is saved and the buffer returns to userspace immediately.
Then the local copy is applied when main/selfpath stream starts.
This implementation is buggy for the following reasons:

- The first params buffer is applied and returned to userspace even if
userspace never calls to streamon on the params node.
- If the first params buffer is queued after the stream started on the
params node then it will return to userspace but will never be used.
- The frame_sequence of the first buffer is set to -1 if the main/selfpath
did not start streaming.

To fix this, the buffers are added to the buffers list on qbuf and
then when a stream start from selfpath/mainpath the first buffer is removed
from the list, applied and returned to userspace. This is done only
if the params node is also streaming.

Testing:
I added a code to libcamera that sets the red, blue, and green gain
interchangeably. The frames are then saved to files.
To run the code:

git clone --single-branch --branch rkisp1-tests-for-params-fixes-patchset https://gitlab.collabora.com/dafna/libcamera.git
cd libcamera
ninja  -C build install
meson test rkisp1-ramzor -C build

Then adding debug prints in the params node in the kernel
that prints the gain values and the current frame
http://ix.io/2ue3

Then playing frames with

ffplay -f rawvideo -pixel_format nv12 -video_size 640x480 build/frame-bla-<FRAME-NUM>.bin

and looking that the frame color matches the debug prints.

changes from v1:
- patch 1 from v1 is removed
- patch 3 from v1 (now patch 5) which refactor the stop_streaming params cb
is changed according to discussion with Tomasz
- 12 new patches added


Dafna Hirschfeld (14):
  media: staging: rkisp1: call params isr only upon frame out
  media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg
    in isr
  media: staging: rkisp1: params: use the new effect value in cproc
    config
  media: staging: rkisp1: params: don't release lock in isr before
    buffer is done
  media: staging: rkisp1: params: upon stream stop, iterate a local list
    to return the buffers
  media: staging: rkisp1: params: in the isr, return if buffer list is
    empty
  media: staging: rkisp1: params: avoid using buffer if params is not
    streaming
  media: staging: rkisp1: params: set vb.sequence to be the isp's
    frame_sequence + 1
  media: staging: rkisp1: remove atomic operations for frame sequence
  media: staging: rkisp1: isp: add a warning and debugfs var for irq
    delay
  media: staging: rkisp1: isp: don't enable signal
    RKISP1_CIF_ISP_FRAME_IN
  media: staging: rkisp1: stats: protect write to 'is_streaming' in
    start_streaming cb
  media: staging: rkisp1: call media_pipeline_start/stop from stats and
    params
  media: staging: rkisp1: params: no need to lock default config

 drivers/staging/media/rkisp1/rkisp1-capture.c |   2 +-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  10 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c     |   2 +
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  39 +++---
 drivers/staging/media/rkisp1/rkisp1-params.c  | 124 ++++++++----------
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  14 +-
 6 files changed, 96 insertions(+), 95 deletions(-)

-- 
2.17.1

[PATCH v2 00/14] media: staging: rkisp1: various bug fixes

[Dafna Hirschfeld]

Fix various bugs mainly in params and stats. The patchset fixes
buffers synchronization issues discussed
https://patchwork.kernel.org/patch/11066513/#23544763

As discussed with Tomasz, we decide that in order to keep the code
simple, we assume that the v-start signal (RKISP1_CIF_ISP_V_START)
and the frame-out signal RKISP1_CIF_ISP_FRAME don't arrive together.
So when v-starts arrives the frame sequence is incremented and
when frame-out arrives, the stats and params isr are called.
Also the frame sequence of the params buffer should be the frame to
which the params are applied.

The params node needs to apply the first params buffer when
the streaming from main/selfpath starts before the first frame arrive
so that the first buffer will configure the first frame.
The way it is implemented is that the first buffer queued
to the params node is not added to the list but instead a local copy
is saved and the buffer returns to userspace immediately.
Then the local copy is applied when main/selfpath stream starts.
This implementation is buggy for the following reasons:

- The first params buffer is applied and returned to userspace even if
userspace never calls to streamon on the params node.
- If the first params buffer is queued after the stream started on the
params node then it will return to userspace but will never be used.
- The frame_sequence of the first buffer is set to -1 if the main/selfpath
did not start streaming.

To fix this, the buffers are added to the buffers list on qbuf and
then when a stream start from selfpath/mainpath the first buffer is removed
from the list, applied and returned to userspace. This is done only
if the params node is also streaming.

Testing:
I added a code to libcamera that sets the red, blue, and green gain
interchangeably. The frames are then saved to files.
To run the code:

git clone --single-branch --branch rkisp1-tests-for-params-fixes-patchset https://gitlab.collabora.com/dafna/libcamera.git
cd libcamera
ninja  -C build install
meson test rkisp1-ramzor -C build

Then adding debug prints in the params node in the kernel
that prints the gain values and the current frame
http://ix.io/2ue3

Then playing frames with

ffplay -f rawvideo -pixel_format nv12 -video_size 640x480 build/frame-bla-<FRAME-NUM>.bin

and looking that the frame color matches the debug prints.

changes from v1:
- patch 1 from v1 is removed
- patch 3 from v1 (now patch 5) which refactor the stop_streaming params cb
is changed according to discussion with Tomasz
- 12 new patches added


Dafna Hirschfeld (14):
  media: staging: rkisp1: call params isr only upon frame out
  media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg
    in isr
  media: staging: rkisp1: params: use the new effect value in cproc
    config
  media: staging: rkisp1: params: don't release lock in isr before
    buffer is done
  media: staging: rkisp1: params: upon stream stop, iterate a local list
    to return the buffers
  media: staging: rkisp1: params: in the isr, return if buffer list is
    empty
  media: staging: rkisp1: params: avoid using buffer if params is not
    streaming
  media: staging: rkisp1: params: set vb.sequence to be the isp's
    frame_sequence + 1
  media: staging: rkisp1: remove atomic operations for frame sequence
  media: staging: rkisp1: isp: add a warning and debugfs var for irq
    delay
  media: staging: rkisp1: isp: don't enable signal
    RKISP1_CIF_ISP_FRAME_IN
  media: staging: rkisp1: stats: protect write to 'is_streaming' in
    start_streaming cb
  media: staging: rkisp1: call media_pipeline_start/stop from stats and
    params
  media: staging: rkisp1: params: no need to lock default config

 drivers/staging/media/rkisp1/rkisp1-capture.c |   2 +-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  10 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c     |   2 +
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  39 +++---
 drivers/staging/media/rkisp1/rkisp1-params.c  | 124 ++++++++----------
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  14 +-
 6 files changed, 96 insertions(+), 95 deletions(-)

-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 01/14] media: staging: rkisp1: call params isr only upon frame out

[Dafna Hirschfeld]

Currently the params isr is called and then returned when
isp-frame interrupt is not set. This condition is already
tested in the isp's isr so move the call under the condition
in the isp's isr.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  2 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 ++++----
 drivers/staging/media/rkisp1/rkisp1-params.c | 29 +++++++++-----------
 3 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 3dc51d703f73..29eaadc58489 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -313,7 +313,7 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
 void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
 void 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);
+void rkisp1_params_isr(struct rkisp1_device *rkisp1);
 
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 6ec1e9816e9f..ad2ece78abbf 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1141,12 +1141,12 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
 		if (isp_ris & RKISP1_STATS_MEAS_MASK)
 			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
+		/*
+		 * Then update changed configs. Some of them involve
+		 * lot of register writes. Do those only one per frame.
+		 * Do the updates in the order of the processing flow.
+		 */
+		rkisp1_params_isr(rkisp1);
 	}
 
-	/*
-	 * Then update changed configs. Some of them involve
-	 * lot of register writes. Do those only one per frame.
-	 * Do the updates in the order of the processing flow.
-	 */
-	rkisp1_params_isr(rkisp1, status);
 }
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 797e79de659c..6d69df36c495 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1193,12 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 	}
 }
 
-void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
+void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 {
 	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
 	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
+	u32 isp_ctrl;
 
 	spin_lock(&params->config_lock);
 	if (!params->is_streaming) {
@@ -1217,24 +1218,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
 
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
-	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
-		u32 isp_ctrl;
+	rkisp1_isp_isr_other_config(params, new_params);
+	rkisp1_isp_isr_meas_config(params, new_params);
 
-		rkisp1_isp_isr_other_config(params, new_params);
-		rkisp1_isp_isr_meas_config(params, new_params);
+	/* update shadow register immediately */
+	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
+	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
+	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
 
-		/* update shadow register immediately */
-		isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
-		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
-		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
-
-		spin_lock(&params->config_lock);
-		list_del(&cur_buf->queue);
-		spin_unlock(&params->config_lock);
+	spin_lock(&params->config_lock);
+	list_del(&cur_buf->queue);
+	spin_unlock(&params->config_lock);
 
-		cur_buf->vb.sequence = frame_sequence;
-		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
-	}
+	cur_buf->vb.sequence = frame_sequence;
+	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 }
 
 static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
-- 
2.17.1

[PATCH v2 01/14] media: staging: rkisp1: call params isr only upon frame out

[Dafna Hirschfeld]

Currently the params isr is called and then returned when
isp-frame interrupt is not set. This condition is already
tested in the isp's isr so move the call under the condition
in the isp's isr.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  2 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 ++++----
 drivers/staging/media/rkisp1/rkisp1-params.c | 29 +++++++++-----------
 3 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 3dc51d703f73..29eaadc58489 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -313,7 +313,7 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
 void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
 void 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);
+void rkisp1_params_isr(struct rkisp1_device *rkisp1);
 
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 6ec1e9816e9f..ad2ece78abbf 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1141,12 +1141,12 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
 		if (isp_ris & RKISP1_STATS_MEAS_MASK)
 			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
+		/*
+		 * Then update changed configs. Some of them involve
+		 * lot of register writes. Do those only one per frame.
+		 * Do the updates in the order of the processing flow.
+		 */
+		rkisp1_params_isr(rkisp1);
 	}
 
-	/*
-	 * Then update changed configs. Some of them involve
-	 * lot of register writes. Do those only one per frame.
-	 * Do the updates in the order of the processing flow.
-	 */
-	rkisp1_params_isr(rkisp1, status);
 }
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 797e79de659c..6d69df36c495 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1193,12 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 	}
 }
 
-void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
+void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 {
 	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
 	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
+	u32 isp_ctrl;
 
 	spin_lock(&params->config_lock);
 	if (!params->is_streaming) {
@@ -1217,24 +1218,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
 
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
-	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
-		u32 isp_ctrl;
+	rkisp1_isp_isr_other_config(params, new_params);
+	rkisp1_isp_isr_meas_config(params, new_params);
 
-		rkisp1_isp_isr_other_config(params, new_params);
-		rkisp1_isp_isr_meas_config(params, new_params);
+	/* update shadow register immediately */
+	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
+	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
+	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
 
-		/* update shadow register immediately */
-		isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
-		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
-		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
-
-		spin_lock(&params->config_lock);
-		list_del(&cur_buf->queue);
-		spin_unlock(&params->config_lock);
+	spin_lock(&params->config_lock);
+	list_del(&cur_buf->queue);
+	spin_unlock(&params->config_lock);
 
-		cur_buf->vb.sequence = frame_sequence;
-		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
-	}
+	cur_buf->vb.sequence = frame_sequence;
+	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 }
 
 static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 02/14] media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr

[Dafna Hirschfeld]

replace a read and write of RKISP1_CIF_ISP_CTRL register
in the isr with the 'rkisp1_param_set_bits' function
that does that.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 6d69df36c495..8d881f1df1e6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1199,7 +1199,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
-	u32 isp_ctrl;
 
 	spin_lock(&params->config_lock);
 	if (!params->is_streaming) {
@@ -1222,9 +1221,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	rkisp1_isp_isr_meas_config(params, new_params);
 
 	/* update shadow register immediately */
-	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
-	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
-	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
+	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
 
 	spin_lock(&params->config_lock);
 	list_del(&cur_buf->queue);
-- 
2.17.1

[PATCH v2 02/14] media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr

[Dafna Hirschfeld]

replace a read and write of RKISP1_CIF_ISP_CTRL register
in the isr with the 'rkisp1_param_set_bits' function
that does that.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 6d69df36c495..8d881f1df1e6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1199,7 +1199,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
-	u32 isp_ctrl;
 
 	spin_lock(&params->config_lock);
 	if (!params->is_streaming) {
@@ -1222,9 +1221,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	rkisp1_isp_isr_meas_config(params, new_params);
 
 	/* update shadow register immediately */
-	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
-	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
-	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
+	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
 
 	spin_lock(&params->config_lock);
 	list_del(&cur_buf->queue);
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config

[Dafna Hirschfeld]

The cproc (color processing) configuration needs to know if
an image effect is configured. The code uses the image effect
in 'cur_params' which is the first params buffer queued in the stream.
This is the wrong place to read the value. The value should be
taken from the current params buffer.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 8d881f1df1e6..eb77b4ed8655 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -560,7 +560,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
 				const struct rkisp1_cif_isp_cproc_config *arg)
 {
 	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =
-						&params->cur_params.others;
+		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
 	struct rkisp1_cif_isp_ie_config *cur_ie_config =
 						&cur_other_cfg->ie_config;
 	u32 effect = cur_ie_config->effect;
-- 
2.17.1

[PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config

[Dafna Hirschfeld]

The cproc (color processing) configuration needs to know if
an image effect is configured. The code uses the image effect
in 'cur_params' which is the first params buffer queued in the stream.
This is the wrong place to read the value. The value should be
taken from the current params buffer.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 8d881f1df1e6..eb77b4ed8655 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -560,7 +560,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
 				const struct rkisp1_cif_isp_cproc_config *arg)
 {
 	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =
-						&params->cur_params.others;
+		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
 	struct rkisp1_cif_isp_ie_config *cur_ie_config =
 						&cur_other_cfg->ie_config;
 	u32 effect = cur_ie_config->effect;
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 04/14] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Dafna Hirschfeld]

In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
should be held as long as the current buffer is used. Otherwise the
stop_streaming calback might remove it from the list and
pass it to userspace while it is referenced in the irq handler.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index eb77b4ed8655..0c2bb2eefb22 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	if (!list_empty(&params->params))
 		cur_buf = list_first_entry(&params->params,
 					   struct rkisp1_buffer, queue);
-	spin_unlock(&params->config_lock);
 
-	if (!cur_buf)
+	if (!cur_buf) {
+		spin_unlock(&params->config_lock);
 		return;
+	}
 
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
@@ -1223,12 +1224,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	/* update shadow register immediately */
 	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
 
-	spin_lock(&params->config_lock);
 	list_del(&cur_buf->queue);
-	spin_unlock(&params->config_lock);
 
 	cur_buf->vb.sequence = frame_sequence;
 	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+	spin_unlock(&params->config_lock);
 }
 
 static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
-- 
2.17.1

[PATCH v2 04/14] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Dafna Hirschfeld]

In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
should be held as long as the current buffer is used. Otherwise the
stop_streaming calback might remove it from the list and
pass it to userspace while it is referenced in the irq handler.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index eb77b4ed8655..0c2bb2eefb22 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	if (!list_empty(&params->params))
 		cur_buf = list_first_entry(&params->params,
 					   struct rkisp1_buffer, queue);
-	spin_unlock(&params->config_lock);
 
-	if (!cur_buf)
+	if (!cur_buf) {
+		spin_unlock(&params->config_lock);
 		return;
+	}
 
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
@@ -1223,12 +1224,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	/* update shadow register immediately */
 	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
 
-	spin_lock(&params->config_lock);
 	list_del(&cur_buf->queue);
-	spin_unlock(&params->config_lock);
 
 	cur_buf->vb.sequence = frame_sequence;
 	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+	spin_unlock(&params->config_lock);
 }
 
 static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Dafna Hirschfeld]

The code in '.stop_streaming' callback releases and acquire the lock
at each iteration when returning the buffers.
Holding the lock disables interrupts so it should be minimized.
To make the code cleaner and still minimize holding the lock,
the buffer list is first moved to a local list and
then can be iterated without the lock.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 0c2bb2eefb22..6a76c586e916 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 {
 	struct rkisp1_params *params = vq->drv_priv;
 	struct rkisp1_buffer *buf;
+	struct list_head tmp_list;
 	unsigned long flags;
-	unsigned int i;
 
-	/* stop params input firstly */
+	INIT_LIST_HEAD(&tmp_list);
+
+	/*
+	 * we first move the buffers into a local list 'tmp_list'
+	 * and then we can iterate it and call vb2_buffer_done
+	 * without holding the lock
+	 */
 	spin_lock_irqsave(&params->config_lock, flags);
 	params->is_streaming = false;
+	list_cut_position(&tmp_list, &params->params, params->params.prev);
 	spin_unlock_irqrestore(&params->config_lock, flags);
 
-	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
-		spin_lock_irqsave(&params->config_lock, flags);
-		if (!list_empty(&params->params)) {
-			buf = list_first_entry(&params->params,
-					       struct rkisp1_buffer, queue);
-			list_del(&buf->queue);
-			spin_unlock_irqrestore(&params->config_lock,
-					       flags);
-		} else {
-			spin_unlock_irqrestore(&params->config_lock,
-					       flags);
-			break;
-		}
-
-		if (buf)
-			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
-		buf = NULL;
-	}
+	list_for_each_entry(buf, &tmp_list, queue)
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 }
 
 static int
-- 
2.17.1

[PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Dafna Hirschfeld]

The code in '.stop_streaming' callback releases and acquire the lock
at each iteration when returning the buffers.
Holding the lock disables interrupts so it should be minimized.
To make the code cleaner and still minimize holding the lock,
the buffer list is first moved to a local list and
then can be iterated without the lock.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 0c2bb2eefb22..6a76c586e916 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 {
 	struct rkisp1_params *params = vq->drv_priv;
 	struct rkisp1_buffer *buf;
+	struct list_head tmp_list;
 	unsigned long flags;
-	unsigned int i;
 
-	/* stop params input firstly */
+	INIT_LIST_HEAD(&tmp_list);
+
+	/*
+	 * we first move the buffers into a local list 'tmp_list'
+	 * and then we can iterate it and call vb2_buffer_done
+	 * without holding the lock
+	 */
 	spin_lock_irqsave(&params->config_lock, flags);
 	params->is_streaming = false;
+	list_cut_position(&tmp_list, &params->params, params->params.prev);
 	spin_unlock_irqrestore(&params->config_lock, flags);
 
-	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
-		spin_lock_irqsave(&params->config_lock, flags);
-		if (!list_empty(&params->params)) {
-			buf = list_first_entry(&params->params,
-					       struct rkisp1_buffer, queue);
-			list_del(&buf->queue);
-			spin_unlock_irqrestore(&params->config_lock,
-					       flags);
-		} else {
-			spin_unlock_irqrestore(&params->config_lock,
-					       flags);
-			break;
-		}
-
-		if (buf)
-			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
-		buf = NULL;
-	}
+	list_for_each_entry(buf, &tmp_list, queue)
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 }
 
 static int
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 06/14] media: staging: rkisp1: params: in the isr, return if buffer list is empty

[Dafna Hirschfeld]

Currently the code in the isr checks if the buffer list is not
empty before referencing a buffer and then check again if the
buffer is not NULL. Instead we can save one 'if' statement by
returning if the buffers list is empty.
Also remove non-helpful inline doc 'get one empty buffer'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 6a76c586e916..86bbd01e18c7 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1206,16 +1206,14 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 		return;
 	}
 
-	/* get one empty buffer */
-	if (!list_empty(&params->params))
-		cur_buf = list_first_entry(&params->params,
-					   struct rkisp1_buffer, queue);
-
-	if (!cur_buf) {
+	if (list_empty(&params->params)) {
 		spin_unlock(&params->config_lock);
 		return;
 	}
 
+	cur_buf = list_first_entry(&params->params,
+				   struct rkisp1_buffer, queue);
+
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
 	rkisp1_isp_isr_other_config(params, new_params);
-- 
2.17.1

[PATCH v2 06/14] media: staging: rkisp1: params: in the isr, return if buffer list is empty

[Dafna Hirschfeld]

Currently the code in the isr checks if the buffer list is not
empty before referencing a buffer and then check again if the
buffer is not NULL. Instead we can save one 'if' statement by
returning if the buffers list is empty.
Also remove non-helpful inline doc 'get one empty buffer'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 6a76c586e916..86bbd01e18c7 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1206,16 +1206,14 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 		return;
 	}
 
-	/* get one empty buffer */
-	if (!list_empty(&params->params))
-		cur_buf = list_first_entry(&params->params,
-					   struct rkisp1_buffer, queue);
-
-	if (!cur_buf) {
+	if (list_empty(&params->params)) {
 		spin_unlock(&params->config_lock);
 		return;
 	}
 
+	cur_buf = list_first_entry(&params->params,
+				   struct rkisp1_buffer, queue);
+
 	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
 
 	rkisp1_isp_isr_other_config(params, new_params);
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[Dafna Hirschfeld]

Currently, the first buffer queued in the params node is returned
immediately to userspace and a copy of it is saved in the field
'cur_params'. The copy is later used for the first configuration
when the stream is initiated by one of selfpath/mainpath capture nodes.

There are 3 problems with this implementation:
- The first params buffer is applied and returned to userspace even if
userspace never calls to streamon on the params node.
- If the first params buffer is queued after the stream started on the
params node then it will return to userspace but will never be used.
- The frame_sequence of the first buffer is set to -1 if the main/selfpath
did not start streaming.

A correct implementation is to apply the first params buffer when stream
is started from mainpath/selfpath and only if params is also streaming.

The patch adds a new function 'rkisp1_params_apply_params_cfg' which takes
a buffer from the buffers queue, apply it and returns it to userspace.
The function is called from the irq handler and when main/selfpath stream
starts - in the function 'rkisp1_params_config_parameter'

Also remove the fields 'cur_params', 'is_first_params' which are no
more needed.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 29eaadc58489..9b41935c6597 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -197,9 +197,6 @@ struct rkisp1_stats {
 
 /*
  * struct rkisp1_params - ISP input parameters device
- *
- * @cur_params: Current ISP parameters
- * @is_first_params: the first params should take effect immediately
  */
 struct rkisp1_params {
 	struct rkisp1_vdev_node vnode;
@@ -207,10 +204,8 @@ struct rkisp1_params {
 
 	spinlock_t config_lock;
 	struct list_head params;
-	struct rkisp1_params_cfg cur_params;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
-	bool is_first_params;
 
 	enum v4l2_quantization quantization;
 	enum rkisp1_fmt_raw_pat_type raw_type;
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 86bbd01e18c7..134b5c9a94c1 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1193,23 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 	}
 }
 
-void rkisp1_params_isr(struct rkisp1_device *rkisp1)
+void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
 {
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
-	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
 
-	spin_lock(&params->config_lock);
-	if (!params->is_streaming) {
-		spin_unlock(&params->config_lock);
-		return;
-	}
-
-	if (list_empty(&params->params)) {
-		spin_unlock(&params->config_lock);
+	if (list_empty(&params->params))
 		return;
-	}
 
 	cur_buf = list_first_entry(&params->params,
 				   struct rkisp1_buffer, queue);
@@ -1226,6 +1216,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 
 	cur_buf->vb.sequence = frame_sequence;
 	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+void rkisp1_params_isr(struct rkisp1_device *rkisp1)
+{
+	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
+	struct rkisp1_params *params = &rkisp1->params;
+
+	spin_lock(&params->config_lock);
+	if (!params->is_streaming) {
+		spin_unlock(&params->config_lock);
+		return;
+	}
+	rkisp1_params_apply_params_cfg(params, frame_sequence);
+
 	spin_unlock(&params->config_lock);
 }
 
@@ -1298,9 +1302,9 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	else
 		rkisp1_csm_config(params, false);
 
-	/* override the default things */
-	rkisp1_isp_isr_other_config(params, &params->cur_params);
-	rkisp1_isp_isr_meas_config(params, &params->cur_params);
+	/* apply the first buffer if there is one already */
+	if (params->is_streaming)
+		rkisp1_params_apply_params_cfg(params, 0);
 
 	spin_unlock(&params->config_lock);
 }
@@ -1428,8 +1432,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
 	sizes[0] = sizeof(struct rkisp1_params_cfg);
 
 	INIT_LIST_HEAD(&params->params);
-	params->is_first_params = true;
-
 	return 0;
 }
 
@@ -1440,20 +1442,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
 		container_of(vbuf, struct rkisp1_buffer, vb);
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_params *params = vq->drv_priv;
-	struct rkisp1_params_cfg *new_params;
 	unsigned long flags;
-	unsigned int frame_sequence =
-		atomic_read(&params->rkisp1->isp.frame_sequence);
-
-	if (params->is_first_params) {
-		new_params = (struct rkisp1_params_cfg *)
-			(vb2_plane_vaddr(vb, 0));
-		vbuf->sequence = frame_sequence;
-		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
-		params->is_first_params = false;
-		params->cur_params = *new_params;
-		return;
-	}
 
 	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
 	spin_lock_irqsave(&params->config_lock, flags);
-- 
2.17.1

[PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[Dafna Hirschfeld]

Currently, the first buffer queued in the params node is returned
immediately to userspace and a copy of it is saved in the field
'cur_params'. The copy is later used for the first configuration
when the stream is initiated by one of selfpath/mainpath capture nodes.

There are 3 problems with this implementation:
- The first params buffer is applied and returned to userspace even if
userspace never calls to streamon on the params node.
- If the first params buffer is queued after the stream started on the
params node then it will return to userspace but will never be used.
- The frame_sequence of the first buffer is set to -1 if the main/selfpath
did not start streaming.

A correct implementation is to apply the first params buffer when stream
is started from mainpath/selfpath and only if params is also streaming.

The patch adds a new function 'rkisp1_params_apply_params_cfg' which takes
a buffer from the buffers queue, apply it and returns it to userspace.
The function is called from the irq handler and when main/selfpath stream
starts - in the function 'rkisp1_params_config_parameter'

Also remove the fields 'cur_params', 'is_first_params' which are no
more needed.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 29eaadc58489..9b41935c6597 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -197,9 +197,6 @@ struct rkisp1_stats {
 
 /*
  * struct rkisp1_params - ISP input parameters device
- *
- * @cur_params: Current ISP parameters
- * @is_first_params: the first params should take effect immediately
  */
 struct rkisp1_params {
 	struct rkisp1_vdev_node vnode;
@@ -207,10 +204,8 @@ struct rkisp1_params {
 
 	spinlock_t config_lock;
 	struct list_head params;
-	struct rkisp1_params_cfg cur_params;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
-	bool is_first_params;
 
 	enum v4l2_quantization quantization;
 	enum rkisp1_fmt_raw_pat_type raw_type;
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 86bbd01e18c7..134b5c9a94c1 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1193,23 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 	}
 }
 
-void rkisp1_params_isr(struct rkisp1_device *rkisp1)
+void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
 {
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
-	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
 
-	spin_lock(&params->config_lock);
-	if (!params->is_streaming) {
-		spin_unlock(&params->config_lock);
-		return;
-	}
-
-	if (list_empty(&params->params)) {
-		spin_unlock(&params->config_lock);
+	if (list_empty(&params->params))
 		return;
-	}
 
 	cur_buf = list_first_entry(&params->params,
 				   struct rkisp1_buffer, queue);
@@ -1226,6 +1216,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 
 	cur_buf->vb.sequence = frame_sequence;
 	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+void rkisp1_params_isr(struct rkisp1_device *rkisp1)
+{
+	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
+	struct rkisp1_params *params = &rkisp1->params;
+
+	spin_lock(&params->config_lock);
+	if (!params->is_streaming) {
+		spin_unlock(&params->config_lock);
+		return;
+	}
+	rkisp1_params_apply_params_cfg(params, frame_sequence);
+
 	spin_unlock(&params->config_lock);
 }
 
@@ -1298,9 +1302,9 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	else
 		rkisp1_csm_config(params, false);
 
-	/* override the default things */
-	rkisp1_isp_isr_other_config(params, &params->cur_params);
-	rkisp1_isp_isr_meas_config(params, &params->cur_params);
+	/* apply the first buffer if there is one already */
+	if (params->is_streaming)
+		rkisp1_params_apply_params_cfg(params, 0);
 
 	spin_unlock(&params->config_lock);
 }
@@ -1428,8 +1432,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
 	sizes[0] = sizeof(struct rkisp1_params_cfg);
 
 	INIT_LIST_HEAD(&params->params);
-	params->is_first_params = true;
-
 	return 0;
 }
 
@@ -1440,20 +1442,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
 		container_of(vbuf, struct rkisp1_buffer, vb);
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_params *params = vq->drv_priv;
-	struct rkisp1_params_cfg *new_params;
 	unsigned long flags;
-	unsigned int frame_sequence =
-		atomic_read(&params->rkisp1->isp.frame_sequence);
-
-	if (params->is_first_params) {
-		new_params = (struct rkisp1_params_cfg *)
-			(vb2_plane_vaddr(vb, 0));
-		vbuf->sequence = frame_sequence;
-		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
-		params->is_first_params = false;
-		params->cur_params = *new_params;
-		return;
-	}
 
 	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
 	spin_lock_irqsave(&params->config_lock, flags);
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Dafna Hirschfeld]

The params isr is called when a frame is out of the isp. The parameters
are applied immediately since the isr updates the shadow registers.
Therefore the params are first applied on the next frame.
We want the vb.sequence to be the frame that the params are applied to.
So we set vb.sequence to be the isp's frame_sequence + 1

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 134b5c9a94c1..4b4391c0a2a0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1220,7 +1220,14 @@ void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int f
 
 void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 {
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
+	/*
+	 * The params isr is called when a frame is out of the isp. The parameters
+	 * are applied immediately since the isr updates the shadow registers.
+	 * Therefore the params are first applied on the next frame.
+	 * We want the vb.sequence to be the frame that the params are applied to.
+	 * So we set vb.sequence to be the isp's frame_sequence + 1
+	 */
+	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
 	struct rkisp1_params *params = &rkisp1->params;
 
 	spin_lock(&params->config_lock);
-- 
2.17.1

[PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Dafna Hirschfeld]

The params isr is called when a frame is out of the isp. The parameters
are applied immediately since the isr updates the shadow registers.
Therefore the params are first applied on the next frame.
We want the vb.sequence to be the frame that the params are applied to.
So we set vb.sequence to be the isp's frame_sequence + 1

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 134b5c9a94c1..4b4391c0a2a0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1220,7 +1220,14 @@ void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int f
 
 void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 {
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
+	/*
+	 * The params isr is called when a frame is out of the isp. The parameters
+	 * are applied immediately since the isr updates the shadow registers.
+	 * Therefore the params are first applied on the next frame.
+	 * We want the vb.sequence to be the frame that the params are applied to.
+	 * So we set vb.sequence to be the isp's frame_sequence + 1
+	 */
+	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
 	struct rkisp1_params *params = &rkisp1->params;
 
 	spin_lock(&params->config_lock);
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

The isp.frame_sequence is now read only from the irq handlers
that are all fired from the same interrupt so there is not need
for atomic operation.
In addition the frame seq incrementation is moved from
rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
clearer.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index c05280950ea0..d7017afc5405 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 	curr_buf = cap->buf.curr;
 
 	if (curr_buf) {
-		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
+		curr_buf->vb.sequence = isp->frame_sequence;
 		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
 		curr_buf->vb.field = V4L2_FIELD_NONE;
 		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 9b41935c6597..79edece6ee77 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -112,7 +112,7 @@ struct rkisp1_isp {
 	const struct rkisp1_isp_mbus_info *src_fmt;
 	struct mutex ops_lock;
 	bool is_dphy_errctrl_disabled;
-	atomic_t frame_sequence;
+	__u32 frame_sequence;
 };
 
 struct rkisp1_vdev_node {
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index ad2ece78abbf..1ffe7cc7bb12 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
 		return -EINVAL;
 
-	atomic_set(&rkisp1->isp.frame_sequence, -1);
+	rkisp1->isp.frame_sequence = -1;
 	mutex_lock(&isp->ops_lock);
 	ret = rkisp1_config_cif(rkisp1);
 	if (ret)
@@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
 	struct v4l2_event event = {
 		.type = V4L2_EVENT_FRAME_SYNC,
 	};
+	event.u.frame_sync.frame_sequence = isp->frame_sequence;
 
-	/*
-	 * Increment the frame sequence on the vsync signal.
-	 * This will allow applications to detect dropped.
-	 * Note that there is a debugfs counter for dropped
-	 * frames, but using this event is more accurate.
-	 */
-	event.u.frame_sync.frame_sequence =
-		atomic_inc_return(&isp->frame_sequence);
 	v4l2_event_queue(isp->sd.devnode, &event);
 }
 
@@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
 
 	/* Vertical sync signal, starting generating new frame */
-	if (status & RKISP1_CIF_ISP_V_START)
+	if (status & RKISP1_CIF_ISP_V_START) {
+		/*
+		 * Increment the frame sequence on the vsync signal.
+		 * This will allow applications to detect dropped.
+		 * Note that there is a debugfs counter for dropped
+		 * frames, but using this event is more accurate.
+		 */
+		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
 
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 4b4391c0a2a0..cc242ad5106e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	 * We want the vb.sequence to be the frame that the params are applied to.
 	 * So we set vb.sequence to be the isp's frame_sequence + 1
 	 */
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
+	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
 	struct rkisp1_params *params = &rkisp1->params;
 
 	spin_lock(&params->config_lock);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 87e4104d20dd..52417b388854 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 {
 	struct rkisp1_stat_buffer *cur_stat_buf;
 	struct rkisp1_buffer *cur_buf = NULL;
-	unsigned int frame_sequence =
-		atomic_read(&stats->rkisp1->isp.frame_sequence);
+	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
 	u64 timestamp = ktime_get_ns();
 
 	/* get one empty buffer */
-- 
2.17.1

[PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

The isp.frame_sequence is now read only from the irq handlers
that are all fired from the same interrupt so there is not need
for atomic operation.
In addition the frame seq incrementation is moved from
rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
clearer.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index c05280950ea0..d7017afc5405 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 	curr_buf = cap->buf.curr;
 
 	if (curr_buf) {
-		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
+		curr_buf->vb.sequence = isp->frame_sequence;
 		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
 		curr_buf->vb.field = V4L2_FIELD_NONE;
 		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 9b41935c6597..79edece6ee77 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -112,7 +112,7 @@ struct rkisp1_isp {
 	const struct rkisp1_isp_mbus_info *src_fmt;
 	struct mutex ops_lock;
 	bool is_dphy_errctrl_disabled;
-	atomic_t frame_sequence;
+	__u32 frame_sequence;
 };
 
 struct rkisp1_vdev_node {
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index ad2ece78abbf..1ffe7cc7bb12 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
 		return -EINVAL;
 
-	atomic_set(&rkisp1->isp.frame_sequence, -1);
+	rkisp1->isp.frame_sequence = -1;
 	mutex_lock(&isp->ops_lock);
 	ret = rkisp1_config_cif(rkisp1);
 	if (ret)
@@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
 	struct v4l2_event event = {
 		.type = V4L2_EVENT_FRAME_SYNC,
 	};
+	event.u.frame_sync.frame_sequence = isp->frame_sequence;
 
-	/*
-	 * Increment the frame sequence on the vsync signal.
-	 * This will allow applications to detect dropped.
-	 * Note that there is a debugfs counter for dropped
-	 * frames, but using this event is more accurate.
-	 */
-	event.u.frame_sync.frame_sequence =
-		atomic_inc_return(&isp->frame_sequence);
 	v4l2_event_queue(isp->sd.devnode, &event);
 }
 
@@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
 
 	/* Vertical sync signal, starting generating new frame */
-	if (status & RKISP1_CIF_ISP_V_START)
+	if (status & RKISP1_CIF_ISP_V_START) {
+		/*
+		 * Increment the frame sequence on the vsync signal.
+		 * This will allow applications to detect dropped.
+		 * Note that there is a debugfs counter for dropped
+		 * frames, but using this event is more accurate.
+		 */
+		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
 
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 4b4391c0a2a0..cc242ad5106e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	 * We want the vb.sequence to be the frame that the params are applied to.
 	 * So we set vb.sequence to be the isp's frame_sequence + 1
 	 */
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
+	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
 	struct rkisp1_params *params = &rkisp1->params;
 
 	spin_lock(&params->config_lock);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 87e4104d20dd..52417b388854 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 {
 	struct rkisp1_stat_buffer *cur_stat_buf;
 	struct rkisp1_buffer *cur_buf = NULL;
-	unsigned int frame_sequence =
-		atomic_read(&stats->rkisp1->isp.frame_sequence);
+	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
 	u64 timestamp = ktime_get_ns();
 
 	/* get one empty buffer */
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Dafna Hirschfeld]

The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
outputting the frame to the next block in the pipeline.
In order to keep buffer synchronization we assume that the
RKISP1_CIF_ISP_V_START signal never arrives together with the
RKISP1_CIF_ISP_FRAME signal.
In case those signals arrive together then the code is not able to
tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
the current v-start or the previous. This patch adds a WARN_ONCE
and a debugfs var to catch it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
 drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
 3 files changed, 8 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 79edece6ee77..5aed0e37850a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -228,6 +228,7 @@ struct rkisp1_debug {
 	unsigned long outform_size_error;
 	unsigned long img_stabilization_size_error;
 	unsigned long inform_size_error;
+	unsigned long irq_delay;
 	unsigned long mipi_error;
 	unsigned long stats_error;
 	unsigned long stop_timeout[2];
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index a0eb8f08708b..557de940286b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 			     &debug->img_stabilization_size_error);
 	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
 			     &debug->inform_size_error);
+	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
+			     &debug->irq_delay);
 	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
 			     &debug->mipi_error);
 	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 1ffe7cc7bb12..33cfad19dde2 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 		 */
 		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
+		if (status & RKISP1_CIF_ISP_FRAME) {
+			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
+			rkisp1->debug.irq_delay++;
+		}
+	}
 
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
 		/* Clear pic_size_error */
-- 
2.17.1

[PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Dafna Hirschfeld]

The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
outputting the frame to the next block in the pipeline.
In order to keep buffer synchronization we assume that the
RKISP1_CIF_ISP_V_START signal never arrives together with the
RKISP1_CIF_ISP_FRAME signal.
In case those signals arrive together then the code is not able to
tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
the current v-start or the previous. This patch adds a WARN_ONCE
and a debugfs var to catch it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
 drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
 3 files changed, 8 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 79edece6ee77..5aed0e37850a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -228,6 +228,7 @@ struct rkisp1_debug {
 	unsigned long outform_size_error;
 	unsigned long img_stabilization_size_error;
 	unsigned long inform_size_error;
+	unsigned long irq_delay;
 	unsigned long mipi_error;
 	unsigned long stats_error;
 	unsigned long stop_timeout[2];
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index a0eb8f08708b..557de940286b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 			     &debug->img_stabilization_size_error);
 	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
 			     &debug->inform_size_error);
+	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
+			     &debug->irq_delay);
 	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
 			     &debug->mipi_error);
 	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 1ffe7cc7bb12..33cfad19dde2 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 		 */
 		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
+		if (status & RKISP1_CIF_ISP_FRAME) {
+			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
+			rkisp1->debug.irq_delay++;
+		}
+	}
 
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
 		/* Clear pic_size_error */
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Dafna Hirschfeld]

The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
there is no need to enable it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 33cfad19dde2..912eb6ad4e0a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
 
 	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
-		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
+		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
 	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
 
 	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
-- 
2.17.1

[PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Dafna Hirschfeld]

The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
there is no need to enable it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 33cfad19dde2..912eb6ad4e0a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
 
 	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
-		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
+		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
 	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
 
 	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 12/14] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb

[Dafna Hirschfeld]

The field stats->is_streaming is written in 'start_streaming' callback
without the stats->lock protection.
The isr might run together with the callback so 'spin_lock_irq'
should be used.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 52417b388854..c86beba709db 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -157,7 +157,9 @@ rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_stats *stats = queue->drv_priv;
 
+	spin_lock_irq(&stats->lock);
 	stats->is_streaming = true;
+	spin_unlock_irq(&stats->lock);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 12/14] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb

[Dafna Hirschfeld]

The field stats->is_streaming is written in 'start_streaming' callback
without the stats->lock protection.
The isr might run together with the callback so 'spin_lock_irq'
should be used.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 52417b388854..c86beba709db 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -157,7 +157,9 @@ rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_stats *stats = queue->drv_priv;
 
+	spin_lock_irq(&stats->lock);
 	stats->is_streaming = true;
+	spin_unlock_irq(&stats->lock);
 
 	return 0;
 }
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 13/14] media: staging: rkisp1: call media_pipeline_start/stop from stats and params

[Dafna Hirschfeld]

The stats and params video nodes register a media entity
and therefore they need to call media_pipeline_start/stop.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 9 +++++++++
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index cc242ad5106e..7d5c21fa630e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1488,14 +1488,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 
 	list_for_each_entry(buf, &tmp_list, queue)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+
+	media_pipeline_stop(&params->vnode.vdev.entity);
 }
 
 static int
 rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_params *params = queue->drv_priv;
+	struct media_entity *entity = &params->vnode.vdev.entity;
 	unsigned long flags;
+	int ret;
 
+	ret = media_pipeline_start(entity, &params->rkisp1->pipe);
+	if (ret) {
+		dev_err(params->rkisp1->dev, "params: start pipeline failed %d\n", ret);
+		return ret;
+	}
 	spin_lock_irqsave(&params->config_lock, flags);
 	params->is_streaming = true;
 	spin_unlock_irqrestore(&params->config_lock, flags);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index c86beba709db..b314fb6a4562 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -150,12 +150,21 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 	}
 	spin_unlock_irq(&stats->lock);
+	media_pipeline_stop(&stats->vnode.vdev.entity);
 }
 
 static int
 rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_stats *stats = queue->drv_priv;
+	struct media_entity *entity = &stats->vnode.vdev.entity;
+	int ret;
+
+	ret = media_pipeline_start(entity, &stats->rkisp1->pipe);
+	if (ret) {
+		dev_err(stats->rkisp1->dev, "stats: start pipeline failed %d\n", ret);
+		return ret;
+	}
 
 	spin_lock_irq(&stats->lock);
 	stats->is_streaming = true;
-- 
2.17.1

[PATCH v2 13/14] media: staging: rkisp1: call media_pipeline_start/stop from stats and params

[Dafna Hirschfeld]

The stats and params video nodes register a media entity
and therefore they need to call media_pipeline_start/stop.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c | 9 +++++++++
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index cc242ad5106e..7d5c21fa630e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1488,14 +1488,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 
 	list_for_each_entry(buf, &tmp_list, queue)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+
+	media_pipeline_stop(&params->vnode.vdev.entity);
 }
 
 static int
 rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_params *params = queue->drv_priv;
+	struct media_entity *entity = &params->vnode.vdev.entity;
 	unsigned long flags;
+	int ret;
 
+	ret = media_pipeline_start(entity, &params->rkisp1->pipe);
+	if (ret) {
+		dev_err(params->rkisp1->dev, "params: start pipeline failed %d\n", ret);
+		return ret;
+	}
 	spin_lock_irqsave(&params->config_lock, flags);
 	params->is_streaming = true;
 	spin_unlock_irqrestore(&params->config_lock, flags);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index c86beba709db..b314fb6a4562 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -150,12 +150,21 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 	}
 	spin_unlock_irq(&stats->lock);
+	media_pipeline_stop(&stats->vnode.vdev.entity);
 }
 
 static int
 rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_stats *stats = queue->drv_priv;
+	struct media_entity *entity = &stats->vnode.vdev.entity;
+	int ret;
+
+	ret = media_pipeline_start(entity, &stats->rkisp1->pipe);
+	if (ret) {
+		dev_err(stats->rkisp1->dev, "stats: start pipeline failed %d\n", ret);
+		return ret;
+	}
 
 	spin_lock_irq(&stats->lock);
 	stats->is_streaming = true;
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 14/14] media: staging: rkisp1: params: no need to lock default config

[Dafna Hirschfeld]

In the function 'rkisp1_params_config_parameter' the lock
is taken while applying the default config. But the lock
only needs to protect the buffers list and the 'is_streaming'
field, so move the locking to lock only what is needed.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 7d5c21fa630e..6c71da311294 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1283,8 +1283,6 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 {
 	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
 
-	spin_lock(&params->config_lock);
-
 	rkisp1_awb_meas_config(params, &rkisp1_awb_params_default_config);
 	rkisp1_awb_meas_enable(params, &rkisp1_awb_params_default_config,
 			       true);
@@ -1309,6 +1307,8 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	else
 		rkisp1_csm_config(params, false);
 
+	spin_lock(&params->config_lock);
+
 	/* apply the first buffer if there is one already */
 	if (params->is_streaming)
 		rkisp1_params_apply_params_cfg(params, 0);
-- 
2.17.1

[PATCH v2 14/14] media: staging: rkisp1: params: no need to lock default config

[Dafna Hirschfeld]

In the function 'rkisp1_params_config_parameter' the lock
is taken while applying the default config. But the lock
only needs to protect the buffers list and the 'is_streaming'
field, so move the locking to lock only what is needed.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 7d5c21fa630e..6c71da311294 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1283,8 +1283,6 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 {
 	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
 
-	spin_lock(&params->config_lock);
-
 	rkisp1_awb_meas_config(params, &rkisp1_awb_params_default_config);
 	rkisp1_awb_meas_enable(params, &rkisp1_awb_params_default_config,
 			       true);
@@ -1309,6 +1307,8 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	else
 		rkisp1_csm_config(params, false);
 
+	spin_lock(&params->config_lock);
+
 	/* apply the first buffer if there is one already */
 	if (params->is_streaming)
 		rkisp1_params_apply_params_cfg(params, 0);
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[kernel test robot]

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

Hi Dafna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.8 next-20200814]
[cannot apply to staging/staging-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/media-staging-rkisp1-various-bug-fixes/20200816-090416
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        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 COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

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

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   arch/xtensa/include/asm/page.h:201:32: note: in expansion of macro 'pfn_valid'
     201 | #define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
         |                                ^~~~~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from ./arch/xtensa/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/xtensa/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/staging/media/rkisp1/rkisp1-params.c: At top level:
>> drivers/staging/media/rkisp1/rkisp1-params.c:1196:6: warning: no previous prototype for 'rkisp1_params_apply_params_cfg' [-Wmissing-prototypes]
    1196 | void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/rkisp1_params_apply_params_cfg +1196 drivers/staging/media/rkisp1/rkisp1-params.c

  1195	
> 1196	void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
  1197	{
  1198		struct rkisp1_params_cfg *new_params;
  1199		struct rkisp1_buffer *cur_buf = NULL;
  1200	
  1201		if (list_empty(&params->params))
  1202			return;
  1203	
  1204		cur_buf = list_first_entry(&params->params,
  1205					   struct rkisp1_buffer, queue);
  1206	
  1207		new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
  1208	
  1209		rkisp1_isp_isr_other_config(params, new_params);
  1210		rkisp1_isp_isr_meas_config(params, new_params);
  1211	
  1212		/* update shadow register immediately */
  1213		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
  1214	
  1215		list_del(&cur_buf->queue);
  1216	
  1217		cur_buf->vb.sequence = frame_sequence;
  1218		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
  1219	}
  1220	

---
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: 64469 bytes --]

Re: [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[kernel test robot]

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

Hi Dafna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.8 next-20200814]
[cannot apply to staging/staging-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/media-staging-rkisp1-various-bug-fixes/20200816-090416
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        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 COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

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

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   arch/xtensa/include/asm/page.h:201:32: note: in expansion of macro 'pfn_valid'
     201 | #define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
         |                                ^~~~~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from ./arch/xtensa/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/xtensa/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/staging/media/rkisp1/rkisp1-params.c: At top level:
>> drivers/staging/media/rkisp1/rkisp1-params.c:1196:6: warning: no previous prototype for 'rkisp1_params_apply_params_cfg' [-Wmissing-prototypes]
    1196 | void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/rkisp1_params_apply_params_cfg +1196 drivers/staging/media/rkisp1/rkisp1-params.c

  1195	
> 1196	void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
  1197	{
  1198		struct rkisp1_params_cfg *new_params;
  1199		struct rkisp1_buffer *cur_buf = NULL;
  1200	
  1201		if (list_empty(&params->params))
  1202			return;
  1203	
  1204		cur_buf = list_first_entry(&params->params,
  1205					   struct rkisp1_buffer, queue);
  1206	
  1207		new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
  1208	
  1209		rkisp1_isp_isr_other_config(params, new_params);
  1210		rkisp1_isp_isr_meas_config(params, new_params);
  1211	
  1212		/* update shadow register immediately */
  1213		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
  1214	
  1215		list_del(&cur_buf->queue);
  1216	
  1217		cur_buf->vb.sequence = frame_sequence;
  1218		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
  1219	}
  1220	

---
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: 64469 bytes --]

[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[kernel test robot]

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

Hi Dafna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.8 next-20200814]
[cannot apply to staging/staging-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/media-staging-rkisp1-various-bug-fixes/20200816-090416
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        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 COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

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

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   arch/xtensa/include/asm/page.h:201:32: note: in expansion of macro 'pfn_valid'
     201 | #define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
         |                                ^~~~~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from ./arch/xtensa/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/xtensa/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/media/v4l2-common.h:17,
                    from drivers/staging/media/rkisp1/rkisp1-params.c:8:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/staging/media/rkisp1/rkisp1-params.c: At top level:
>> drivers/staging/media/rkisp1/rkisp1-params.c:1196:6: warning: no previous prototype for 'rkisp1_params_apply_params_cfg' [-Wmissing-prototypes]
    1196 | void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/rkisp1_params_apply_params_cfg +1196 drivers/staging/media/rkisp1/rkisp1-params.c

  1195	
> 1196	void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
  1197	{
  1198		struct rkisp1_params_cfg *new_params;
  1199		struct rkisp1_buffer *cur_buf = NULL;
  1200	
  1201		if (list_empty(&params->params))
  1202			return;
  1203	
  1204		cur_buf = list_first_entry(&params->params,
  1205					   struct rkisp1_buffer, queue);
  1206	
  1207		new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
  1208	
  1209		rkisp1_isp_isr_other_config(params, new_params);
  1210		rkisp1_isp_isr_meas_config(params, new_params);
  1211	
  1212		/* update shadow register immediately */
  1213		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
  1214	
  1215		list_del(&cur_buf->queue);
  1216	
  1217		cur_buf->vb.sequence = frame_sequence;
  1218		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
  1219	}
  1220	

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

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

Re: [PATCH v2 01/14] media: staging: rkisp1: call params isr only upon frame out

[Helen Koike]

Hi Dafna,

Thanks for the patch, nice cleanup.

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently the params isr is called and then returned when
> isp-frame interrupt is not set. This condition is already
> tested in the isp's isr so move the call under the condition
> in the isp's isr.
> 
> 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 |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 ++++----
>  drivers/staging/media/rkisp1/rkisp1-params.c | 29 +++++++++-----------
>  3 files changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 3dc51d703f73..29eaadc58489 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -313,7 +313,7 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>  void 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);
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1);
>  
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 6ec1e9816e9f..ad2ece78abbf 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1141,12 +1141,12 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
>  		if (isp_ris & RKISP1_STATS_MEAS_MASK)
>  			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
> +		/*
> +		 * Then update changed configs. Some of them involve
> +		 * lot of register writes. Do those only one per frame.
> +		 * Do the updates in the order of the processing flow.
> +		 */
> +		rkisp1_params_isr(rkisp1);
>  	}
>  
> -	/*
> -	 * Then update changed configs. Some of them involve
> -	 * lot of register writes. Do those only one per frame.
> -	 * Do the updates in the order of the processing flow.
> -	 */
> -	rkisp1_params_isr(rkisp1, status);
>  }
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 797e79de659c..6d69df36c495 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1193,12 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> -void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  {
>  	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
>  	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
> +	u32 isp_ctrl;
>  
>  	spin_lock(&params->config_lock);
>  	if (!params->is_streaming) {
> @@ -1217,24 +1218,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
>  
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
> -	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
> -		u32 isp_ctrl;
> +	rkisp1_isp_isr_other_config(params, new_params);
> +	rkisp1_isp_isr_meas_config(params, new_params);
>  
> -		rkisp1_isp_isr_other_config(params, new_params);
> -		rkisp1_isp_isr_meas_config(params, new_params);
> +	/* update shadow register immediately */
> +	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> +	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> +	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
>  
> -		/* update shadow register immediately */
> -		isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> -		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> -		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
> -
> -		spin_lock(&params->config_lock);
> -		list_del(&cur_buf->queue);
> -		spin_unlock(&params->config_lock);
> +	spin_lock(&params->config_lock);
> +	list_del(&cur_buf->queue);
> +	spin_unlock(&params->config_lock);
>  
> -		cur_buf->vb.sequence = frame_sequence;
> -		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> -	}
> +	cur_buf->vb.sequence = frame_sequence;
> +	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  }
>  
>  static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
> 

Re: [PATCH v2 01/14] media: staging: rkisp1: call params isr only upon frame out

[Helen Koike]

Hi Dafna,

Thanks for the patch, nice cleanup.

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently the params isr is called and then returned when
> isp-frame interrupt is not set. This condition is already
> tested in the isp's isr so move the call under the condition
> in the isp's isr.
> 
> 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 |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 ++++----
>  drivers/staging/media/rkisp1/rkisp1-params.c | 29 +++++++++-----------
>  3 files changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 3dc51d703f73..29eaadc58489 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -313,7 +313,7 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>  void 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);
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1);
>  
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 6ec1e9816e9f..ad2ece78abbf 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1141,12 +1141,12 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
>  		if (isp_ris & RKISP1_STATS_MEAS_MASK)
>  			rkisp1_stats_isr(&rkisp1->stats, isp_ris);
> +		/*
> +		 * Then update changed configs. Some of them involve
> +		 * lot of register writes. Do those only one per frame.
> +		 * Do the updates in the order of the processing flow.
> +		 */
> +		rkisp1_params_isr(rkisp1);
>  	}
>  
> -	/*
> -	 * Then update changed configs. Some of them involve
> -	 * lot of register writes. Do those only one per frame.
> -	 * Do the updates in the order of the processing flow.
> -	 */
> -	rkisp1_params_isr(rkisp1, status);
>  }
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 797e79de659c..6d69df36c495 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1193,12 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> -void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  {
>  	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
>  	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
> +	u32 isp_ctrl;
>  
>  	spin_lock(&params->config_lock);
>  	if (!params->is_streaming) {
> @@ -1217,24 +1218,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
>  
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
> -	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
> -		u32 isp_ctrl;
> +	rkisp1_isp_isr_other_config(params, new_params);
> +	rkisp1_isp_isr_meas_config(params, new_params);
>  
> -		rkisp1_isp_isr_other_config(params, new_params);
> -		rkisp1_isp_isr_meas_config(params, new_params);
> +	/* update shadow register immediately */
> +	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> +	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> +	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
>  
> -		/* update shadow register immediately */
> -		isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> -		isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> -		rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
> -
> -		spin_lock(&params->config_lock);
> -		list_del(&cur_buf->queue);
> -		spin_unlock(&params->config_lock);
> +	spin_lock(&params->config_lock);
> +	list_del(&cur_buf->queue);
> +	spin_unlock(&params->config_lock);
>  
> -		cur_buf->vb.sequence = frame_sequence;
> -		vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> -	}
> +	cur_buf->vb.sequence = frame_sequence;
> +	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  }
>  
>  static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 02/14] media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> replace a read and write of RKISP1_CIF_ISP_CTRL register
> in the isr with the 'rkisp1_param_set_bits' function
> that does that.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 6d69df36c495..8d881f1df1e6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1199,7 +1199,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
> -	u32 isp_ctrl;
>  
>  	spin_lock(&params->config_lock);
>  	if (!params->is_streaming) {
> @@ -1222,9 +1221,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	rkisp1_isp_isr_meas_config(params, new_params);
>  
>  	/* update shadow register immediately */
> -	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> -	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> -	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
> +	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
>  
>  	spin_lock(&params->config_lock);
>  	list_del(&cur_buf->queue);
> 

Re: [PATCH v2 02/14] media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> replace a read and write of RKISP1_CIF_ISP_CTRL register
> in the isr with the 'rkisp1_param_set_bits' function
> that does that.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 6d69df36c495..8d881f1df1e6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1199,7 +1199,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
> -	u32 isp_ctrl;
>  
>  	spin_lock(&params->config_lock);
>  	if (!params->is_streaming) {
> @@ -1222,9 +1221,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	rkisp1_isp_isr_meas_config(params, new_params);
>  
>  	/* update shadow register immediately */
> -	isp_ctrl = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_CTRL);
> -	isp_ctrl |= RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD;
> -	rkisp1_write(params->rkisp1, isp_ctrl, RKISP1_CIF_ISP_CTRL);
> +	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
>  
>  	spin_lock(&params->config_lock);
>  	list_del(&cur_buf->queue);
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The cproc (color processing) configuration needs to know if
> an image effect is configured. The code uses the image effect
> in 'cur_params' which is the first params buffer queued in the stream.
> This is the wrong place to read the value. The value should be
> taken from the current params buffer.

This is a bit confusing, since params->cur_params is not the current
parameter as the name suggest, but it is only the first one.

I was thinking we could rename this variable to
`first_params`, and this descriptions would be less confusing,
but I saw you removed it on patch 07/14, so never mind.

It would be easier to read if this patch was just before patch
07/14.

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 8d881f1df1e6..eb77b4ed8655 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -560,7 +560,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
>  				const struct rkisp1_cif_isp_cproc_config *arg)
>  {
>  	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =

Side note: I was also thinking that the prefix "cur_other" is very confusing.
What "other" mean here?

Regards,
Helen

> -						&params->cur_params.others;
> +		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
>  	struct rkisp1_cif_isp_ie_config *cur_ie_config =
>  						&cur_other_cfg->ie_config;
>  	u32 effect = cur_ie_config->effect;
> 

Re: [PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The cproc (color processing) configuration needs to know if
> an image effect is configured. The code uses the image effect
> in 'cur_params' which is the first params buffer queued in the stream.
> This is the wrong place to read the value. The value should be
> taken from the current params buffer.

This is a bit confusing, since params->cur_params is not the current
parameter as the name suggest, but it is only the first one.

I was thinking we could rename this variable to
`first_params`, and this descriptions would be less confusing,
but I saw you removed it on patch 07/14, so never mind.

It would be easier to read if this patch was just before patch
07/14.

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 8d881f1df1e6..eb77b4ed8655 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -560,7 +560,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
>  				const struct rkisp1_cif_isp_cproc_config *arg)
>  {
>  	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =

Side note: I was also thinking that the prefix "cur_other" is very confusing.
What "other" mean here?

Regards,
Helen

> -						&params->cur_params.others;
> +		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
>  	struct rkisp1_cif_isp_ie_config *cur_ie_config =
>  						&cur_other_cfg->ie_config;
>  	u32 effect = cur_ie_config->effect;
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 04/14] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
> should be held as long as the current buffer is used. Otherwise the
> stop_streaming calback might remove it from the list and
> pass it to userspace while it is referenced in the irq handler.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index eb77b4ed8655..0c2bb2eefb22 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	if (!list_empty(&params->params))
>  		cur_buf = list_first_entry(&params->params,
>  					   struct rkisp1_buffer, queue);
> -	spin_unlock(&params->config_lock);
>  
> -	if (!cur_buf)
> +	if (!cur_buf) {
> +		spin_unlock(&params->config_lock);
>  		return;
> +	}
>  
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
> @@ -1223,12 +1224,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	/* update shadow register immediately */
>  	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
>  
> -	spin_lock(&params->config_lock);
>  	list_del(&cur_buf->queue);
> -	spin_unlock(&params->config_lock);
>  
>  	cur_buf->vb.sequence = frame_sequence;
>  	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	spin_unlock(&params->config_lock);
>  }
>  
>  static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
> 

Re: [PATCH v2 04/14] media: staging: rkisp1: params: don't release lock in isr before buffer is done

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> In the irq handler 'rkisp1_params_isr', the lock 'config_lock'
> should be held as long as the current buffer is used. Otherwise the
> stop_streaming calback might remove it from the list and
> pass it to userspace while it is referenced in the irq handler.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index eb77b4ed8655..0c2bb2eefb22 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1210,10 +1210,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	if (!list_empty(&params->params))
>  		cur_buf = list_first_entry(&params->params,
>  					   struct rkisp1_buffer, queue);
> -	spin_unlock(&params->config_lock);
>  
> -	if (!cur_buf)
> +	if (!cur_buf) {
> +		spin_unlock(&params->config_lock);
>  		return;
> +	}
>  
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
> @@ -1223,12 +1224,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	/* update shadow register immediately */
>  	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
>  
> -	spin_lock(&params->config_lock);
>  	list_del(&cur_buf->queue);
> -	spin_unlock(&params->config_lock);
>  
>  	cur_buf->vb.sequence = frame_sequence;
>  	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	spin_unlock(&params->config_lock);
>  }
>  
>  static const struct rkisp1_cif_isp_awb_meas_config rkisp1_awb_params_default_config = {
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The code in '.stop_streaming' callback releases and acquire the lock
> at each iteration when returning the buffers.
> Holding the lock disables interrupts so it should be minimized.
> To make the code cleaner and still minimize holding the lock,
> the buffer list is first moved to a local list and
> then can be iterated without the lock.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

lgtm

Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 0c2bb2eefb22..6a76c586e916 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rkisp1_params *params = vq->drv_priv;
>  	struct rkisp1_buffer *buf;
> +	struct list_head tmp_list;
>  	unsigned long flags;
> -	unsigned int i;
>  
> -	/* stop params input firstly */
> +	INIT_LIST_HEAD(&tmp_list);
> +
> +	/*
> +	 * we first move the buffers into a local list 'tmp_list'
> +	 * and then we can iterate it and call vb2_buffer_done
> +	 * without holding the lock
> +	 */
>  	spin_lock_irqsave(&params->config_lock, flags);
>  	params->is_streaming = false;
> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>  	spin_unlock_irqrestore(&params->config_lock, flags);
>  
> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> -		spin_lock_irqsave(&params->config_lock, flags);
> -		if (!list_empty(&params->params)) {
> -			buf = list_first_entry(&params->params,
> -					       struct rkisp1_buffer, queue);
> -			list_del(&buf->queue);
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
> -		} else {
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
> -			break;
> -		}
> -
> -		if (buf)
> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> -		buf = NULL;
> -	}
> +	list_for_each_entry(buf, &tmp_list, queue)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  }
>  
>  static int
> 

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The code in '.stop_streaming' callback releases and acquire the lock
> at each iteration when returning the buffers.
> Holding the lock disables interrupts so it should be minimized.
> To make the code cleaner and still minimize holding the lock,
> the buffer list is first moved to a local list and
> then can be iterated without the lock.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

lgtm

Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 0c2bb2eefb22..6a76c586e916 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rkisp1_params *params = vq->drv_priv;
>  	struct rkisp1_buffer *buf;
> +	struct list_head tmp_list;
>  	unsigned long flags;
> -	unsigned int i;
>  
> -	/* stop params input firstly */
> +	INIT_LIST_HEAD(&tmp_list);
> +
> +	/*
> +	 * we first move the buffers into a local list 'tmp_list'
> +	 * and then we can iterate it and call vb2_buffer_done
> +	 * without holding the lock
> +	 */
>  	spin_lock_irqsave(&params->config_lock, flags);
>  	params->is_streaming = false;
> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>  	spin_unlock_irqrestore(&params->config_lock, flags);
>  
> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> -		spin_lock_irqsave(&params->config_lock, flags);
> -		if (!list_empty(&params->params)) {
> -			buf = list_first_entry(&params->params,
> -					       struct rkisp1_buffer, queue);
> -			list_del(&buf->queue);
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
> -		} else {
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
> -			break;
> -		}
> -
> -		if (buf)
> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> -		buf = NULL;
> -	}
> +	list_for_each_entry(buf, &tmp_list, queue)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  }
>  
>  static int
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 06/14] media: staging: rkisp1: params: in the isr, return if buffer list is empty

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently the code in the isr checks if the buffer list is not
> empty before referencing a buffer and then check again if the
> buffer is not NULL. Instead we can save one 'if' statement by
> returning if the buffers list is empty.
> Also remove non-helpful inline doc 'get one empty buffer'
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 6a76c586e916..86bbd01e18c7 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1206,16 +1206,14 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  		return;
>  	}
>  
> -	/* get one empty buffer */
> -	if (!list_empty(&params->params))
> -		cur_buf = list_first_entry(&params->params,
> -					   struct rkisp1_buffer, queue);
> -
> -	if (!cur_buf) {
> +	if (list_empty(&params->params)) {
>  		spin_unlock(&params->config_lock);
>  		return;
>  	}
>  
> +	cur_buf = list_first_entry(&params->params,
> +				   struct rkisp1_buffer, queue);
> +
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
>  	rkisp1_isp_isr_other_config(params, new_params);
> 

Re: [PATCH v2 06/14] media: staging: rkisp1: params: in the isr, return if buffer list is empty

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently the code in the isr checks if the buffer list is not
> empty before referencing a buffer and then check again if the
> buffer is not NULL. Instead we can save one 'if' statement by
> returning if the buffers list is empty.
> Also remove non-helpful inline doc 'get one empty buffer'
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 6a76c586e916..86bbd01e18c7 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1206,16 +1206,14 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  		return;
>  	}
>  
> -	/* get one empty buffer */
> -	if (!list_empty(&params->params))
> -		cur_buf = list_first_entry(&params->params,
> -					   struct rkisp1_buffer, queue);
> -
> -	if (!cur_buf) {
> +	if (list_empty(&params->params)) {
>  		spin_unlock(&params->config_lock);
>  		return;
>  	}
>  
> +	cur_buf = list_first_entry(&params->params,
> +				   struct rkisp1_buffer, queue);
> +
>  	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
>  
>  	rkisp1_isp_isr_other_config(params, new_params);
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[Helen Koike]

Hi Dafna,

Thanks for the patch

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently, the first buffer queued in the params node is returned
> immediately to userspace and a copy of it is saved in the field
> 'cur_params'. The copy is later used for the first configuration
> when the stream is initiated by one of selfpath/mainpath capture nodes.
> 
> There are 3 problems with this implementation:
> - The first params buffer is applied and returned to userspace even if
> userspace never calls to streamon on the params node.
> - If the first params buffer is queued after the stream started on the
> params node then it will return to userspace but will never be used.
> - The frame_sequence of the first buffer is set to -1 if the main/selfpath
> did not start streaming.
> 
> A correct implementation is to apply the first params buffer when stream
> is started from mainpath/selfpath and only if params is also streaming.
> 
> The patch adds a new function 'rkisp1_params_apply_params_cfg' which takes
> a buffer from the buffers queue, apply it and returns it to userspace.
> The function is called from the irq handler and when main/selfpath stream
> starts - in the function 'rkisp1_params_config_parameter'
> 
> Also remove the fields 'cur_params', 'is_first_params' which are no
> more needed.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

lgtm

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

Regards,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  5 --
>  drivers/staging/media/rkisp1/rkisp1-params.c | 49 ++++++++------------
>  2 files changed, 19 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 29eaadc58489..9b41935c6597 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -197,9 +197,6 @@ struct rkisp1_stats {
>  
>  /*
>   * struct rkisp1_params - ISP input parameters device
> - *
> - * @cur_params: Current ISP parameters
> - * @is_first_params: the first params should take effect immediately
>   */
>  struct rkisp1_params {
>  	struct rkisp1_vdev_node vnode;
> @@ -207,10 +204,8 @@ struct rkisp1_params {
>  
>  	spinlock_t config_lock;
>  	struct list_head params;
> -	struct rkisp1_params_cfg cur_params;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
> -	bool is_first_params;
>  
>  	enum v4l2_quantization quantization;
>  	enum rkisp1_fmt_raw_pat_type raw_type;
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 86bbd01e18c7..134b5c9a94c1 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1193,23 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> -void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> +void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
>  {
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> -	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
>  
> -	spin_lock(&params->config_lock);
> -	if (!params->is_streaming) {
> -		spin_unlock(&params->config_lock);
> -		return;
> -	}
> -
> -	if (list_empty(&params->params)) {
> -		spin_unlock(&params->config_lock);
> +	if (list_empty(&params->params))
>  		return;
> -	}
>  
>  	cur_buf = list_first_entry(&params->params,
>  				   struct rkisp1_buffer, queue);
> @@ -1226,6 +1216,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  
>  	cur_buf->vb.sequence = frame_sequence;
>  	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> +{
> +	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> +	struct rkisp1_params *params = &rkisp1->params;
> +
> +	spin_lock(&params->config_lock);
> +	if (!params->is_streaming) {
> +		spin_unlock(&params->config_lock);
> +		return;
> +	}
> +	rkisp1_params_apply_params_cfg(params, frame_sequence);
> +
>  	spin_unlock(&params->config_lock);
>  }
>  
> @@ -1298,9 +1302,9 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  	else
>  		rkisp1_csm_config(params, false);
>  
> -	/* override the default things */
> -	rkisp1_isp_isr_other_config(params, &params->cur_params);
> -	rkisp1_isp_isr_meas_config(params, &params->cur_params);
> +	/* apply the first buffer if there is one already */
> +	if (params->is_streaming)
> +		rkisp1_params_apply_params_cfg(params, 0);
>  
>  	spin_unlock(&params->config_lock);
>  }
> @@ -1428,8 +1432,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
>  	sizes[0] = sizeof(struct rkisp1_params_cfg);
>  
>  	INIT_LIST_HEAD(&params->params);
> -	params->is_first_params = true;
> -
>  	return 0;
>  }
>  
> @@ -1440,20 +1442,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
>  		container_of(vbuf, struct rkisp1_buffer, vb);
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct rkisp1_params *params = vq->drv_priv;
> -	struct rkisp1_params_cfg *new_params;
>  	unsigned long flags;
> -	unsigned int frame_sequence =
> -		atomic_read(&params->rkisp1->isp.frame_sequence);
> -
> -	if (params->is_first_params) {
> -		new_params = (struct rkisp1_params_cfg *)
> -			(vb2_plane_vaddr(vb, 0));
> -		vbuf->sequence = frame_sequence;
> -		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> -		params->is_first_params = false;
> -		params->cur_params = *new_params;
> -		return;
> -	}
>  
>  	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>  	spin_lock_irqsave(&params->config_lock, flags);
> 

Re: [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[Helen Koike]

Hi Dafna,

Thanks for the patch

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> Currently, the first buffer queued in the params node is returned
> immediately to userspace and a copy of it is saved in the field
> 'cur_params'. The copy is later used for the first configuration
> when the stream is initiated by one of selfpath/mainpath capture nodes.
> 
> There are 3 problems with this implementation:
> - The first params buffer is applied and returned to userspace even if
> userspace never calls to streamon on the params node.
> - If the first params buffer is queued after the stream started on the
> params node then it will return to userspace but will never be used.
> - The frame_sequence of the first buffer is set to -1 if the main/selfpath
> did not start streaming.
> 
> A correct implementation is to apply the first params buffer when stream
> is started from mainpath/selfpath and only if params is also streaming.
> 
> The patch adds a new function 'rkisp1_params_apply_params_cfg' which takes
> a buffer from the buffers queue, apply it and returns it to userspace.
> The function is called from the irq handler and when main/selfpath stream
> starts - in the function 'rkisp1_params_config_parameter'
> 
> Also remove the fields 'cur_params', 'is_first_params' which are no
> more needed.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

lgtm

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

Regards,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  5 --
>  drivers/staging/media/rkisp1/rkisp1-params.c | 49 ++++++++------------
>  2 files changed, 19 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 29eaadc58489..9b41935c6597 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -197,9 +197,6 @@ struct rkisp1_stats {
>  
>  /*
>   * struct rkisp1_params - ISP input parameters device
> - *
> - * @cur_params: Current ISP parameters
> - * @is_first_params: the first params should take effect immediately
>   */
>  struct rkisp1_params {
>  	struct rkisp1_vdev_node vnode;
> @@ -207,10 +204,8 @@ struct rkisp1_params {
>  
>  	spinlock_t config_lock;
>  	struct list_head params;
> -	struct rkisp1_params_cfg cur_params;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
> -	bool is_first_params;
>  
>  	enum v4l2_quantization quantization;
>  	enum rkisp1_fmt_raw_pat_type raw_type;
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 86bbd01e18c7..134b5c9a94c1 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1193,23 +1193,13 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> -void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> +void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int frame_sequence)
>  {
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> -	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
>  
> -	spin_lock(&params->config_lock);
> -	if (!params->is_streaming) {
> -		spin_unlock(&params->config_lock);
> -		return;
> -	}
> -
> -	if (list_empty(&params->params)) {
> -		spin_unlock(&params->config_lock);
> +	if (list_empty(&params->params))
>  		return;
> -	}
>  
>  	cur_buf = list_first_entry(&params->params,
>  				   struct rkisp1_buffer, queue);
> @@ -1226,6 +1216,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  
>  	cur_buf->vb.sequence = frame_sequence;
>  	vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> +{
> +	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> +	struct rkisp1_params *params = &rkisp1->params;
> +
> +	spin_lock(&params->config_lock);
> +	if (!params->is_streaming) {
> +		spin_unlock(&params->config_lock);
> +		return;
> +	}
> +	rkisp1_params_apply_params_cfg(params, frame_sequence);
> +
>  	spin_unlock(&params->config_lock);
>  }
>  
> @@ -1298,9 +1302,9 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  	else
>  		rkisp1_csm_config(params, false);
>  
> -	/* override the default things */
> -	rkisp1_isp_isr_other_config(params, &params->cur_params);
> -	rkisp1_isp_isr_meas_config(params, &params->cur_params);
> +	/* apply the first buffer if there is one already */
> +	if (params->is_streaming)
> +		rkisp1_params_apply_params_cfg(params, 0);
>  
>  	spin_unlock(&params->config_lock);
>  }
> @@ -1428,8 +1432,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
>  	sizes[0] = sizeof(struct rkisp1_params_cfg);
>  
>  	INIT_LIST_HEAD(&params->params);
> -	params->is_first_params = true;
> -
>  	return 0;
>  }
>  
> @@ -1440,20 +1442,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
>  		container_of(vbuf, struct rkisp1_buffer, vb);
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct rkisp1_params *params = vq->drv_priv;
> -	struct rkisp1_params_cfg *new_params;
>  	unsigned long flags;
> -	unsigned int frame_sequence =
> -		atomic_read(&params->rkisp1->isp.frame_sequence);
> -
> -	if (params->is_first_params) {
> -		new_params = (struct rkisp1_params_cfg *)
> -			(vb2_plane_vaddr(vb, 0));
> -		vbuf->sequence = frame_sequence;
> -		vb2_buffer_done(&params_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> -		params->is_first_params = false;
> -		params->cur_params = *new_params;
> -		return;
> -	}
>  
>  	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>  	spin_lock_irqsave(&params->config_lock, flags);
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The params isr is called when a frame is out of the isp. The parameters
> are applied immediately since the isr updates the shadow registers.
> Therefore the params are first applied on the next frame.
> We want the vb.sequence to be the frame that the params are applied to.
> So we set vb.sequence to be the isp's frame_sequence + 1
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 134b5c9a94c1..4b4391c0a2a0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1220,7 +1220,14 @@ void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int f
>  
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  {
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> +	/*
> +	 * The params isr is called when a frame is out of the isp. The parameters
> +	 * are applied immediately since the isr updates the shadow registers.
> +	 * Therefore the params are first applied on the next frame.
> +	 * We want the vb.sequence to be the frame that the params are applied to.
> +	 * So we set vb.sequence to be the isp's frame_sequence + 1
> +	 */

I would just re-phrase this a bit, how about:

	This isr is called when the ISP finishes processing a frame.
	To configure the parameters, we update the shadow registers, which means
	that the next frame will already take these new configuration into consideration.
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.


Or maybe smaller:

	This isr is called when the ISP finishes processing a frame.
	Configurations performed here will be applied to the next frame.	
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.

What do you think?

With an improvement in the text (and also commit message):

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

Regards,
Helen

> +	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
>  	struct rkisp1_params *params = &rkisp1->params;
>  
>  	spin_lock(&params->config_lock);
> 

Re: [PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The params isr is called when a frame is out of the isp. The parameters
> are applied immediately since the isr updates the shadow registers.
> Therefore the params are first applied on the next frame.
> We want the vb.sequence to be the frame that the params are applied to.
> So we set vb.sequence to be the isp's frame_sequence + 1
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 134b5c9a94c1..4b4391c0a2a0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1220,7 +1220,14 @@ void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int f
>  
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  {
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> +	/*
> +	 * The params isr is called when a frame is out of the isp. The parameters
> +	 * are applied immediately since the isr updates the shadow registers.
> +	 * Therefore the params are first applied on the next frame.
> +	 * We want the vb.sequence to be the frame that the params are applied to.
> +	 * So we set vb.sequence to be the isp's frame_sequence + 1
> +	 */

I would just re-phrase this a bit, how about:

	This isr is called when the ISP finishes processing a frame.
	To configure the parameters, we update the shadow registers, which means
	that the next frame will already take these new configuration into consideration.
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.


Or maybe smaller:

	This isr is called when the ISP finishes processing a frame.
	Configurations performed here will be applied to the next frame.	
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.

What do you think?

With an improvement in the text (and also commit message):

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

Regards,
Helen

> +	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
>  	struct rkisp1_params *params = &rkisp1->params;
>  
>  	spin_lock(&params->config_lock);
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The isp.frame_sequence is now read only from the irq handlers
> that are all fired from the same interrupt so there is not need
> for atomic operation.
> In addition the frame seq incrementation is moved from
> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
> clearer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 20 +++++++++----------
>  drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>  5 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index c05280950ea0..d7017afc5405 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  	curr_buf = cap->buf.curr;
>  
>  	if (curr_buf) {
> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
> +		curr_buf->vb.sequence = isp->frame_sequence;
>  		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>  		curr_buf->vb.field = V4L2_FIELD_NONE;
>  		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 9b41935c6597..79edece6ee77 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -112,7 +112,7 @@ struct rkisp1_isp {
>  	const struct rkisp1_isp_mbus_info *src_fmt;
>  	struct mutex ops_lock;
>  	bool is_dphy_errctrl_disabled;
> -	atomic_t frame_sequence;
> +	__u32 frame_sequence;
>  };
>  
>  struct rkisp1_vdev_node {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index ad2ece78abbf..1ffe7cc7bb12 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
>  		return -EINVAL;
>  
> -	atomic_set(&rkisp1->isp.frame_sequence, -1);
> +	rkisp1->isp.frame_sequence = -1;
>  	mutex_lock(&isp->ops_lock);
>  	ret = rkisp1_config_cif(rkisp1);
>  	if (ret)
> @@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>  	struct v4l2_event event = {
>  		.type = V4L2_EVENT_FRAME_SYNC,
>  	};
> +	event.u.frame_sync.frame_sequence = isp->frame_sequence;
>  
> -	/*
> -	 * Increment the frame sequence on the vsync signal.
> -	 * This will allow applications to detect dropped.
> -	 * Note that there is a debugfs counter for dropped
> -	 * frames, but using this event is more accurate.
> -	 */
> -	event.u.frame_sync.frame_sequence =
> -		atomic_inc_return(&isp->frame_sequence);
>  	v4l2_event_queue(isp->sd.devnode, &event);
>  }
>  
> @@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>  
>  	/* Vertical sync signal, starting generating new frame */
> -	if (status & RKISP1_CIF_ISP_V_START)
> +	if (status & RKISP1_CIF_ISP_V_START) {
> +		/*
> +		 * Increment the frame sequence on the vsync signal.
> +		 * This will allow applications to detect dropped.

I know just moved the comment, but I would like to take this opportunity
to fix this text:

s/to detect dropped/to detect dropped frames.

> +		 * Note that there is a debugfs counter for dropped
> +		 * frames, but using this event is more accurate.

I'm not sure if this phrase is accurate :P
Since the debugfs counter should be reliable to verify we dropped frames.

I mean, usage is different.

maybe just:

s/, but using this event is more accurate././


With or without these changes:

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

Regards,
Helen

> +		 */
> +		rkisp1->isp.frame_sequence++;
>  		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>  
>  	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 4b4391c0a2a0..cc242ad5106e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	 * We want the vb.sequence to be the frame that the params are applied to.
>  	 * So we set vb.sequence to be the isp's frame_sequence + 1
>  	 */
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
> +	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
>  	struct rkisp1_params *params = &rkisp1->params;
>  
>  	spin_lock(&params->config_lock);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 87e4104d20dd..52417b388854 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>  {
>  	struct rkisp1_stat_buffer *cur_stat_buf;
>  	struct rkisp1_buffer *cur_buf = NULL;
> -	unsigned int frame_sequence =
> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
> +	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
>  	u64 timestamp = ktime_get_ns();
>  
>  	/* get one empty buffer */
> 

Re: [PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The isp.frame_sequence is now read only from the irq handlers
> that are all fired from the same interrupt so there is not need
> for atomic operation.
> In addition the frame seq incrementation is moved from
> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
> clearer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 20 +++++++++----------
>  drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>  5 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index c05280950ea0..d7017afc5405 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  	curr_buf = cap->buf.curr;
>  
>  	if (curr_buf) {
> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
> +		curr_buf->vb.sequence = isp->frame_sequence;
>  		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>  		curr_buf->vb.field = V4L2_FIELD_NONE;
>  		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 9b41935c6597..79edece6ee77 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -112,7 +112,7 @@ struct rkisp1_isp {
>  	const struct rkisp1_isp_mbus_info *src_fmt;
>  	struct mutex ops_lock;
>  	bool is_dphy_errctrl_disabled;
> -	atomic_t frame_sequence;
> +	__u32 frame_sequence;
>  };
>  
>  struct rkisp1_vdev_node {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index ad2ece78abbf..1ffe7cc7bb12 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
>  		return -EINVAL;
>  
> -	atomic_set(&rkisp1->isp.frame_sequence, -1);
> +	rkisp1->isp.frame_sequence = -1;
>  	mutex_lock(&isp->ops_lock);
>  	ret = rkisp1_config_cif(rkisp1);
>  	if (ret)
> @@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>  	struct v4l2_event event = {
>  		.type = V4L2_EVENT_FRAME_SYNC,
>  	};
> +	event.u.frame_sync.frame_sequence = isp->frame_sequence;
>  
> -	/*
> -	 * Increment the frame sequence on the vsync signal.
> -	 * This will allow applications to detect dropped.
> -	 * Note that there is a debugfs counter for dropped
> -	 * frames, but using this event is more accurate.
> -	 */
> -	event.u.frame_sync.frame_sequence =
> -		atomic_inc_return(&isp->frame_sequence);
>  	v4l2_event_queue(isp->sd.devnode, &event);
>  }
>  
> @@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>  
>  	/* Vertical sync signal, starting generating new frame */
> -	if (status & RKISP1_CIF_ISP_V_START)
> +	if (status & RKISP1_CIF_ISP_V_START) {
> +		/*
> +		 * Increment the frame sequence on the vsync signal.
> +		 * This will allow applications to detect dropped.

I know just moved the comment, but I would like to take this opportunity
to fix this text:

s/to detect dropped/to detect dropped frames.

> +		 * Note that there is a debugfs counter for dropped
> +		 * frames, but using this event is more accurate.

I'm not sure if this phrase is accurate :P
Since the debugfs counter should be reliable to verify we dropped frames.

I mean, usage is different.

maybe just:

s/, but using this event is more accurate././


With or without these changes:

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

Regards,
Helen

> +		 */
> +		rkisp1->isp.frame_sequence++;
>  		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>  
>  	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 4b4391c0a2a0..cc242ad5106e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	 * We want the vb.sequence to be the frame that the params are applied to.
>  	 * So we set vb.sequence to be the isp's frame_sequence + 1
>  	 */
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
> +	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
>  	struct rkisp1_params *params = &rkisp1->params;
>  
>  	spin_lock(&params->config_lock);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 87e4104d20dd..52417b388854 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>  {
>  	struct rkisp1_stat_buffer *cur_stat_buf;
>  	struct rkisp1_buffer *cur_buf = NULL;
> -	unsigned int frame_sequence =
> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
> +	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
>  	u64 timestamp = ktime_get_ns();
>  
>  	/* get one empty buffer */
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
> outputting the frame to the next block in the pipeline.
> In order to keep buffer synchronization we assume that the
> RKISP1_CIF_ISP_V_START signal never arrives together with the
> RKISP1_CIF_ISP_FRAME signal.
> In case those signals arrive together then the code is not able to
> tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
> the current v-start or the previous. This patch adds a WARN_ONCE
> and a debugfs var to catch it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
>  drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 79edece6ee77..5aed0e37850a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -228,6 +228,7 @@ struct rkisp1_debug {
>  	unsigned long outform_size_error;
>  	unsigned long img_stabilization_size_error;
>  	unsigned long inform_size_error;
> +	unsigned long irq_delay;
>  	unsigned long mipi_error;
>  	unsigned long stats_error;
>  	unsigned long stop_timeout[2];
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index a0eb8f08708b..557de940286b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  			     &debug->img_stabilization_size_error);
>  	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
>  			     &debug->inform_size_error);
> +	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
> +			     &debug->irq_delay);
>  	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
>  			     &debug->mipi_error);
>  	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 1ffe7cc7bb12..33cfad19dde2 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  		 */
>  		rkisp1->isp.frame_sequence++;
>  		rkisp1_isp_queue_event_sof(&rkisp1->isp);
> +		if (status & RKISP1_CIF_ISP_FRAME) {
> +			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
> +			rkisp1->debug.irq_delay++;
> +		}
> +	}

I was wondering, if both interrupts arrive at the same time, shouldn't we handle the
frame before incrementing the frame_sequence?

I mean, move the code block

	if (status & RKISP1_CIF_ISP_V_START) {...}

after this other code block

	if (status & RKISP1_CIF_ISP_FRAME) {...}


Thanks,
Helen

>  
>  	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>  		/* Clear pic_size_error */
> 

Re: [PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
> outputting the frame to the next block in the pipeline.
> In order to keep buffer synchronization we assume that the
> RKISP1_CIF_ISP_V_START signal never arrives together with the
> RKISP1_CIF_ISP_FRAME signal.
> In case those signals arrive together then the code is not able to
> tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
> the current v-start or the previous. This patch adds a WARN_ONCE
> and a debugfs var to catch it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
>  drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 79edece6ee77..5aed0e37850a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -228,6 +228,7 @@ struct rkisp1_debug {
>  	unsigned long outform_size_error;
>  	unsigned long img_stabilization_size_error;
>  	unsigned long inform_size_error;
> +	unsigned long irq_delay;
>  	unsigned long mipi_error;
>  	unsigned long stats_error;
>  	unsigned long stop_timeout[2];
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index a0eb8f08708b..557de940286b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  			     &debug->img_stabilization_size_error);
>  	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
>  			     &debug->inform_size_error);
> +	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
> +			     &debug->irq_delay);
>  	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
>  			     &debug->mipi_error);
>  	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 1ffe7cc7bb12..33cfad19dde2 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>  		 */
>  		rkisp1->isp.frame_sequence++;
>  		rkisp1_isp_queue_event_sof(&rkisp1->isp);
> +		if (status & RKISP1_CIF_ISP_FRAME) {
> +			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
> +			rkisp1->debug.irq_delay++;
> +		}
> +	}

I was wondering, if both interrupts arrive at the same time, shouldn't we handle the
frame before incrementing the frame_sequence?

I mean, move the code block

	if (status & RKISP1_CIF_ISP_V_START) {...}

after this other code block

	if (status & RKISP1_CIF_ISP_FRAME) {...}


Thanks,
Helen

>  
>  	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>  		/* Clear pic_size_error */
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
> there is no need to enable it.

I saw in the docs this is for when sampled input frame is complete.
(I was just wondering for curiosity what is a sampled input frame
here, but never mind).

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 33cfad19dde2..912eb6ad4e0a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
>  	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
>  
>  	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
> -		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
> +		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
>  	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
>  
>  	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> 

Re: [PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
> there is no need to enable it.

I saw in the docs this is for when sampled input frame is complete.
(I was just wondering for curiosity what is a sampled input frame
here, but never mind).

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 33cfad19dde2..912eb6ad4e0a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
>  	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
>  
>  	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
> -		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
> +		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
>  	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
>  
>  	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 12/14] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The field stats->is_streaming is written in 'start_streaming' callback
> without the stats->lock protection.
> The isr might run together with the callback so 'spin_lock_irq'
> should be used.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 52417b388854..c86beba709db 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -157,7 +157,9 @@ rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_stats *stats = queue->drv_priv;
>  
> +	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = true;
> +	spin_unlock_irq(&stats->lock);
>  
>  	return 0;
>  }
> 

Re: [PATCH v2 12/14] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The field stats->is_streaming is written in 'start_streaming' callback
> without the stats->lock protection.
> The isr might run together with the callback so 'spin_lock_irq'
> should be used.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 52417b388854..c86beba709db 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -157,7 +157,9 @@ rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_stats *stats = queue->drv_priv;
>  
> +	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = true;
> +	spin_unlock_irq(&stats->lock);
>  
>  	return 0;
>  }
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 13/14] media: staging: rkisp1: call media_pipeline_start/stop from stats and params

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The stats and params video nodes register a media entity
> and therefore they need to call media_pipeline_start/stop.

I'm not sure this is really required, iirc, these functions
basically locks the topology, which would cause EBUSY errors
when trying to modify the topology.
I can be wrong, but I don't think params
and stats requires locking the topology configuration.

Regards,
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 +++++++++
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 9 +++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index cc242ad5106e..7d5c21fa630e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1488,14 +1488,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  
>  	list_for_each_entry(buf, &tmp_list, queue)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +
> +	media_pipeline_stop(&params->vnode.vdev.entity);
>  }
>  
>  static int
>  rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_params *params = queue->drv_priv;
> +	struct media_entity *entity = &params->vnode.vdev.entity;
>  	unsigned long flags;
> +	int ret;
>  
> +	ret = media_pipeline_start(entity, &params->rkisp1->pipe);
> +	if (ret) {
> +		dev_err(params->rkisp1->dev, "params: start pipeline failed %d\n", ret);
> +		return ret;
> +	}
>  	spin_lock_irqsave(&params->config_lock, flags);
>  	params->is_streaming = true;
>  	spin_unlock_irqrestore(&params->config_lock, flags);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index c86beba709db..b314fb6a4562 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -150,12 +150,21 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  	}
>  	spin_unlock_irq(&stats->lock);
> +	media_pipeline_stop(&stats->vnode.vdev.entity);
>  }
>  
>  static int
>  rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_stats *stats = queue->drv_priv;
> +	struct media_entity *entity = &stats->vnode.vdev.entity;
> +	int ret;
> +
> +	ret = media_pipeline_start(entity, &stats->rkisp1->pipe);
> +	if (ret) {
> +		dev_err(stats->rkisp1->dev, "stats: start pipeline failed %d\n", ret);
> +		return ret;
> +	}
>  
>  	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = true;
> 

Re: [PATCH v2 13/14] media: staging: rkisp1: call media_pipeline_start/stop from stats and params

[Helen Koike]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The stats and params video nodes register a media entity
> and therefore they need to call media_pipeline_start/stop.

I'm not sure this is really required, iirc, these functions
basically locks the topology, which would cause EBUSY errors
when trying to modify the topology.
I can be wrong, but I don't think params
and stats requires locking the topology configuration.

Regards,
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 +++++++++
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 9 +++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index cc242ad5106e..7d5c21fa630e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1488,14 +1488,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  
>  	list_for_each_entry(buf, &tmp_list, queue)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +
> +	media_pipeline_stop(&params->vnode.vdev.entity);
>  }
>  
>  static int
>  rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_params *params = queue->drv_priv;
> +	struct media_entity *entity = &params->vnode.vdev.entity;
>  	unsigned long flags;
> +	int ret;
>  
> +	ret = media_pipeline_start(entity, &params->rkisp1->pipe);
> +	if (ret) {
> +		dev_err(params->rkisp1->dev, "params: start pipeline failed %d\n", ret);
> +		return ret;
> +	}
>  	spin_lock_irqsave(&params->config_lock, flags);
>  	params->is_streaming = true;
>  	spin_unlock_irqrestore(&params->config_lock, flags);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index c86beba709db..b314fb6a4562 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -150,12 +150,21 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  	}
>  	spin_unlock_irq(&stats->lock);
> +	media_pipeline_stop(&stats->vnode.vdev.entity);
>  }
>  
>  static int
>  rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_stats *stats = queue->drv_priv;
> +	struct media_entity *entity = &stats->vnode.vdev.entity;
> +	int ret;
> +
> +	ret = media_pipeline_start(entity, &stats->rkisp1->pipe);
> +	if (ret) {
> +		dev_err(stats->rkisp1->dev, "stats: start pipeline failed %d\n", ret);
> +		return ret;
> +	}
>  
>  	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = true;
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 14/14] media: staging: rkisp1: params: no need to lock default config

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> In the function 'rkisp1_params_config_parameter' the lock
> is taken while applying the default config. But the lock
> only needs to protect the buffers list and the 'is_streaming'
> field, so move the locking to lock only what is needed.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 7d5c21fa630e..6c71da311294 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1283,8 +1283,6 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  {
>  	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
>  
> -	spin_lock(&params->config_lock);
> -
>  	rkisp1_awb_meas_config(params, &rkisp1_awb_params_default_config);
>  	rkisp1_awb_meas_enable(params, &rkisp1_awb_params_default_config,
>  			       true);
> @@ -1309,6 +1307,8 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  	else
>  		rkisp1_csm_config(params, false);
>  
> +	spin_lock(&params->config_lock);
> +
>  	/* apply the first buffer if there is one already */
>  	if (params->is_streaming)
>  		rkisp1_params_apply_params_cfg(params, 0);
> 

Re: [PATCH v2 14/14] media: staging: rkisp1: params: no need to lock default config

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> In the function 'rkisp1_params_config_parameter' the lock
> is taken while applying the default config. But the lock
> only needs to protect the buffers list and the 'is_streaming'
> field, so move the locking to lock only what is needed.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 7d5c21fa630e..6c71da311294 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1283,8 +1283,6 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  {
>  	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
>  
> -	spin_lock(&params->config_lock);
> -
>  	rkisp1_awb_meas_config(params, &rkisp1_awb_params_default_config);
>  	rkisp1_awb_meas_enable(params, &rkisp1_awb_params_default_config,
>  			       true);
> @@ -1309,6 +1307,8 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  	else
>  		rkisp1_csm_config(params, false);
>  
> +	spin_lock(&params->config_lock);
> +
>  	/* apply the first buffer if there is one already */
>  	if (params->is_streaming)
>  		rkisp1_params_apply_params_cfg(params, 0);
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Dafna Hirschfeld]

Am 17.08.20 um 23:48 schrieb Helen Koike:
> 
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
>> there is no need to enable it.
> 
> I saw in the docs this is for when sampled input frame is complete.
> (I was just wondering for curiosity what is a sampled input frame
> here, but never mind).

Hi,
I discussed the meaning of the interrupts with Tomasz, https://patchwork.kernel.org/patch/11066513/#23505185

Thanks,
Dafna

> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Thanks,
> Helen
> 
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index 33cfad19dde2..912eb6ad4e0a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
>>   	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
>>   
>>   	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
>> -		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
>> +		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
>>   	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
>>   
>>   	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
>>
> 

Re: [PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN

[Dafna Hirschfeld]

Am 17.08.20 um 23:48 schrieb Helen Koike:
> 
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The signal RKISP1_CIF_ISP_FRAME_IN is not used in the isr so
>> there is no need to enable it.
> 
> I saw in the docs this is for when sampled input frame is complete.
> (I was just wondering for curiosity what is a sampled input frame
> here, but never mind).

Hi,
I discussed the meaning of the interrupts with Tomasz, https://patchwork.kernel.org/patch/11066513/#23505185

Thanks,
Dafna

> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Thanks,
> Helen
> 
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-isp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index 33cfad19dde2..912eb6ad4e0a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -348,7 +348,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1)
>>   	rkisp1_write(rkisp1, sink_crop->height, RKISP1_CIF_ISP_OUT_V_SIZE);
>>   
>>   	irq_mask |= RKISP1_CIF_ISP_FRAME | RKISP1_CIF_ISP_V_START |
>> -		    RKISP1_CIF_ISP_PIC_SIZE_ERROR | RKISP1_CIF_ISP_FRAME_IN;
>> +		    RKISP1_CIF_ISP_PIC_SIZE_ERROR;
>>   	rkisp1_write(rkisp1, irq_mask, RKISP1_CIF_ISP_IMSC);
>>   
>>   	if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
>>
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Dafna Hirschfeld]

Am 17.08.20 um 23:48 schrieb Helen Koike:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
>> outputting the frame to the next block in the pipeline.
>> In order to keep buffer synchronization we assume that the
>> RKISP1_CIF_ISP_V_START signal never arrives together with the
>> RKISP1_CIF_ISP_FRAME signal.
>> In case those signals arrive together then the code is not able to
>> tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
>> the current v-start or the previous. This patch adds a WARN_ONCE
>> and a debugfs var to catch it.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
>>   drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
>>   drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
>>   3 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 79edece6ee77..5aed0e37850a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -228,6 +228,7 @@ struct rkisp1_debug {
>>   	unsigned long outform_size_error;
>>   	unsigned long img_stabilization_size_error;
>>   	unsigned long inform_size_error;
>> +	unsigned long irq_delay;
>>   	unsigned long mipi_error;
>>   	unsigned long stats_error;
>>   	unsigned long stop_timeout[2];
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
>> index a0eb8f08708b..557de940286b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
>> @@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>>   			     &debug->img_stabilization_size_error);
>>   	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
>>   			     &debug->inform_size_error);
>> +	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
>> +			     &debug->irq_delay);
>>   	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
>>   			     &debug->mipi_error);
>>   	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index 1ffe7cc7bb12..33cfad19dde2 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>   		 */
>>   		rkisp1->isp.frame_sequence++;
>>   		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>> +		if (status & RKISP1_CIF_ISP_FRAME) {
>> +			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
>> +			rkisp1->debug.irq_delay++;
>> +		}
>> +	}
> 
> I was wondering, if both interrupts arrive at the same time, shouldn't we handle the
> frame before incrementing the frame_sequence?
> 
> I mean, move the code block
> 
> 	if (status & RKISP1_CIF_ISP_V_START) {...}
> 
> after this other code block
> 
> 	if (status & RKISP1_CIF_ISP_FRAME) {...}

When the two interrupts arrive together it is not clear if the frame interrupt belongs
to the current v-start or the previous one, this is why I added this debugfs var.
So it is a bit of guessing what should be done first. Maybe changing the order is generally
more correct. I discussed that with Tomasz https://patchwork.kernel.org/patch/11066513/#23544763

Thanks,
Dafna

> 
> 
> Thanks,
> Helen
> 
>>   
>>   	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>>   		/* Clear pic_size_error */
>>

Re: [PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay

[Dafna Hirschfeld]

Am 17.08.20 um 23:48 schrieb Helen Koike:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The signal RKISP1_CIF_ISP_FRAME is set when the ISP completes
>> outputting the frame to the next block in the pipeline.
>> In order to keep buffer synchronization we assume that the
>> RKISP1_CIF_ISP_V_START signal never arrives together with the
>> RKISP1_CIF_ISP_FRAME signal.
>> In case those signals arrive together then the code is not able to
>> tell if the RKISP1_CIF_ISP_FRAME signal relates to the frame of
>> the current v-start or the previous. This patch adds a WARN_ONCE
>> and a debugfs var to catch it.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
>>   drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
>>   drivers/staging/media/rkisp1/rkisp1-isp.c    | 5 +++++
>>   3 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 79edece6ee77..5aed0e37850a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -228,6 +228,7 @@ struct rkisp1_debug {
>>   	unsigned long outform_size_error;
>>   	unsigned long img_stabilization_size_error;
>>   	unsigned long inform_size_error;
>> +	unsigned long irq_delay;
>>   	unsigned long mipi_error;
>>   	unsigned long stats_error;
>>   	unsigned long stop_timeout[2];
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
>> index a0eb8f08708b..557de940286b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
>> @@ -445,6 +445,8 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>>   			     &debug->img_stabilization_size_error);
>>   	debugfs_create_ulong("inform_size_error", 0444,  debug->debugfs_dir,
>>   			     &debug->inform_size_error);
>> +	debugfs_create_ulong("irq_delay", 0444,  debug->debugfs_dir,
>> +			     &debug->irq_delay);
>>   	debugfs_create_ulong("mipi_error", 0444, debug->debugfs_dir,
>>   			     &debug->mipi_error);
>>   	debugfs_create_ulong("stats_error", 0444, debug->debugfs_dir,
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index 1ffe7cc7bb12..33cfad19dde2 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -1118,6 +1118,11 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>   		 */
>>   		rkisp1->isp.frame_sequence++;
>>   		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>> +		if (status & RKISP1_CIF_ISP_FRAME) {
>> +			WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
>> +			rkisp1->debug.irq_delay++;
>> +		}
>> +	}
> 
> I was wondering, if both interrupts arrive at the same time, shouldn't we handle the
> frame before incrementing the frame_sequence?
> 
> I mean, move the code block
> 
> 	if (status & RKISP1_CIF_ISP_V_START) {...}
> 
> after this other code block
> 
> 	if (status & RKISP1_CIF_ISP_FRAME) {...}

When the two interrupts arrive together it is not clear if the frame interrupt belongs
to the current v-start or the previous one, this is why I added this debugfs var.
So it is a bit of guessing what should be done first. Maybe changing the order is generally
more correct. I discussed that with Tomasz https://patchwork.kernel.org/patch/11066513/#23544763

Thanks,
Dafna

> 
> 
> Thanks,
> Helen
> 
>>   
>>   	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>>   		/* Clear pic_size_error */
>>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Hans Verkuil]

On 17/08/2020 23:47, Helen Koike wrote:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The code in '.stop_streaming' callback releases and acquire the lock
>> at each iteration when returning the buffers.
>> Holding the lock disables interrupts so it should be minimized.
>> To make the code cleaner and still minimize holding the lock,
>> the buffer list is first moved to a local list and
>> then can be iterated without the lock.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> lgtm
> 
> Helen Koike <helen.koike@collabora.com>

Missing 'Acked-by:' tag?

Hans

> 
> Thanks
> Helen
> 
>> ---
>>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>> index 0c2bb2eefb22..6a76c586e916 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>  {
>>  	struct rkisp1_params *params = vq->drv_priv;
>>  	struct rkisp1_buffer *buf;
>> +	struct list_head tmp_list;
>>  	unsigned long flags;
>> -	unsigned int i;
>>  
>> -	/* stop params input firstly */
>> +	INIT_LIST_HEAD(&tmp_list);
>> +
>> +	/*
>> +	 * we first move the buffers into a local list 'tmp_list'
>> +	 * and then we can iterate it and call vb2_buffer_done
>> +	 * without holding the lock
>> +	 */
>>  	spin_lock_irqsave(&params->config_lock, flags);
>>  	params->is_streaming = false;
>> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>>  	spin_unlock_irqrestore(&params->config_lock, flags);
>>  
>> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>> -		spin_lock_irqsave(&params->config_lock, flags);
>> -		if (!list_empty(&params->params)) {
>> -			buf = list_first_entry(&params->params,
>> -					       struct rkisp1_buffer, queue);
>> -			list_del(&buf->queue);
>> -			spin_unlock_irqrestore(&params->config_lock,
>> -					       flags);
>> -		} else {
>> -			spin_unlock_irqrestore(&params->config_lock,
>> -					       flags);
>> -			break;
>> -		}
>> -
>> -		if (buf)
>> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> -		buf = NULL;
>> -	}
>> +	list_for_each_entry(buf, &tmp_list, queue)
>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>  }
>>  
>>  static int
>>

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Hans Verkuil]

On 17/08/2020 23:47, Helen Koike wrote:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The code in '.stop_streaming' callback releases and acquire the lock
>> at each iteration when returning the buffers.
>> Holding the lock disables interrupts so it should be minimized.
>> To make the code cleaner and still minimize holding the lock,
>> the buffer list is first moved to a local list and
>> then can be iterated without the lock.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> lgtm
> 
> Helen Koike <helen.koike@collabora.com>

Missing 'Acked-by:' tag?

Hans

> 
> Thanks
> Helen
> 
>> ---
>>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>> index 0c2bb2eefb22..6a76c586e916 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>  {
>>  	struct rkisp1_params *params = vq->drv_priv;
>>  	struct rkisp1_buffer *buf;
>> +	struct list_head tmp_list;
>>  	unsigned long flags;
>> -	unsigned int i;
>>  
>> -	/* stop params input firstly */
>> +	INIT_LIST_HEAD(&tmp_list);
>> +
>> +	/*
>> +	 * we first move the buffers into a local list 'tmp_list'
>> +	 * and then we can iterate it and call vb2_buffer_done
>> +	 * without holding the lock
>> +	 */
>>  	spin_lock_irqsave(&params->config_lock, flags);
>>  	params->is_streaming = false;
>> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>>  	spin_unlock_irqrestore(&params->config_lock, flags);
>>  
>> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>> -		spin_lock_irqsave(&params->config_lock, flags);
>> -		if (!list_empty(&params->params)) {
>> -			buf = list_first_entry(&params->params,
>> -					       struct rkisp1_buffer, queue);
>> -			list_del(&buf->queue);
>> -			spin_unlock_irqrestore(&params->config_lock,
>> -					       flags);
>> -		} else {
>> -			spin_unlock_irqrestore(&params->config_lock,
>> -					       flags);
>> -			break;
>> -		}
>> -
>> -		if (buf)
>> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> -		buf = NULL;
>> -	}
>> +	list_for_each_entry(buf, &tmp_list, queue)
>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>  }
>>  
>>  static int
>>


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Helen Koike]

On 8/20/20 6:27 AM, Hans Verkuil wrote:
> On 17/08/2020 23:47, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>>> The code in '.stop_streaming' callback releases and acquire the lock
>>> at each iteration when returning the buffers.
>>> Holding the lock disables interrupts so it should be minimized.
>>> To make the code cleaner and still minimize holding the lock,
>>> the buffer list is first moved to a local list and
>>> then can be iterated without the lock.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>
>> lgtm
>>
>> Helen Koike <helen.koike@collabora.com>
> 
> Missing 'Acked-by:' tag?

Ops, yes

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

> 
> Hans
> 
>>
>> Thanks
>> Helen
>>
>>> ---
>>>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> index 0c2bb2eefb22..6a76c586e916 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>>  {
>>>  	struct rkisp1_params *params = vq->drv_priv;
>>>  	struct rkisp1_buffer *buf;
>>> +	struct list_head tmp_list;
>>>  	unsigned long flags;
>>> -	unsigned int i;
>>>  
>>> -	/* stop params input firstly */
>>> +	INIT_LIST_HEAD(&tmp_list);
>>> +
>>> +	/*
>>> +	 * we first move the buffers into a local list 'tmp_list'
>>> +	 * and then we can iterate it and call vb2_buffer_done
>>> +	 * without holding the lock
>>> +	 */
>>>  	spin_lock_irqsave(&params->config_lock, flags);
>>>  	params->is_streaming = false;
>>> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>>>  	spin_unlock_irqrestore(&params->config_lock, flags);
>>>  
>>> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> -		spin_lock_irqsave(&params->config_lock, flags);
>>> -		if (!list_empty(&params->params)) {
>>> -			buf = list_first_entry(&params->params,
>>> -					       struct rkisp1_buffer, queue);
>>> -			list_del(&buf->queue);
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -		} else {
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -			break;
>>> -		}
>>> -
>>> -		if (buf)
>>> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>> -		buf = NULL;
>>> -	}
>>> +	list_for_each_entry(buf, &tmp_list, queue)
>>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>  }
>>>  
>>>  static int
>>>
> 

Re: [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Helen Koike]

On 8/20/20 6:27 AM, Hans Verkuil wrote:
> On 17/08/2020 23:47, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>>> The code in '.stop_streaming' callback releases and acquire the lock
>>> at each iteration when returning the buffers.
>>> Holding the lock disables interrupts so it should be minimized.
>>> To make the code cleaner and still minimize holding the lock,
>>> the buffer list is first moved to a local list and
>>> then can be iterated without the lock.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>
>> lgtm
>>
>> Helen Koike <helen.koike@collabora.com>
> 
> Missing 'Acked-by:' tag?

Ops, yes

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

> 
> Hans
> 
>>
>> Thanks
>> Helen
>>
>>> ---
>>>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> index 0c2bb2eefb22..6a76c586e916 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>>> @@ -1477,32 +1477,23 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>>>  {
>>>  	struct rkisp1_params *params = vq->drv_priv;
>>>  	struct rkisp1_buffer *buf;
>>> +	struct list_head tmp_list;
>>>  	unsigned long flags;
>>> -	unsigned int i;
>>>  
>>> -	/* stop params input firstly */
>>> +	INIT_LIST_HEAD(&tmp_list);
>>> +
>>> +	/*
>>> +	 * we first move the buffers into a local list 'tmp_list'
>>> +	 * and then we can iterate it and call vb2_buffer_done
>>> +	 * without holding the lock
>>> +	 */
>>>  	spin_lock_irqsave(&params->config_lock, flags);
>>>  	params->is_streaming = false;
>>> +	list_cut_position(&tmp_list, &params->params, params->params.prev);
>>>  	spin_unlock_irqrestore(&params->config_lock, flags);
>>>  
>>> -	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
>>> -		spin_lock_irqsave(&params->config_lock, flags);
>>> -		if (!list_empty(&params->params)) {
>>> -			buf = list_first_entry(&params->params,
>>> -					       struct rkisp1_buffer, queue);
>>> -			list_del(&buf->queue);
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -		} else {
>>> -			spin_unlock_irqrestore(&params->config_lock,
>>> -					       flags);
>>> -			break;
>>> -		}
>>> -
>>> -		if (buf)
>>> -			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>> -		buf = NULL;
>>> -	}
>>> +	list_for_each_entry(buf, &tmp_list, queue)
>>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>>  }
>>>  
>>>  static int
>>>
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

Hi

Am 17.08.20 um 23:48 schrieb Helen Koike:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The isp.frame_sequence is now read only from the irq handlers
>> that are all fired from the same interrupt so there is not need
>> for atomic operation.
>> In addition the frame seq incrementation is moved from
>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
>> clearer.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 20 +++++++++----------
>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>   5 files changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index c05280950ea0..d7017afc5405 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>>   	curr_buf = cap->buf.curr;
>>   
>>   	if (curr_buf) {
>> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
>> +		curr_buf->vb.sequence = isp->frame_sequence;
>>   		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>>   		curr_buf->vb.field = V4L2_FIELD_NONE;
>>   		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 9b41935c6597..79edece6ee77 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -112,7 +112,7 @@ struct rkisp1_isp {
>>   	const struct rkisp1_isp_mbus_info *src_fmt;
>>   	struct mutex ops_lock;
>>   	bool is_dphy_errctrl_disabled;
>> -	atomic_t frame_sequence;
>> +	__u32 frame_sequence;
>>   };
>>   
>>   struct rkisp1_vdev_node {
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index ad2ece78abbf..1ffe7cc7bb12 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>>   	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
>>   		return -EINVAL;
>>   
>> -	atomic_set(&rkisp1->isp.frame_sequence, -1);
>> +	rkisp1->isp.frame_sequence = -1;
>>   	mutex_lock(&isp->ops_lock);
>>   	ret = rkisp1_config_cif(rkisp1);
>>   	if (ret)
>> @@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>>   	struct v4l2_event event = {
>>   		.type = V4L2_EVENT_FRAME_SYNC,
>>   	};
>> +	event.u.frame_sync.frame_sequence = isp->frame_sequence;
>>   
>> -	/*
>> -	 * Increment the frame sequence on the vsync signal.
>> -	 * This will allow applications to detect dropped.
>> -	 * Note that there is a debugfs counter for dropped
>> -	 * frames, but using this event is more accurate.
>> -	 */
>> -	event.u.frame_sync.frame_sequence =
>> -		atomic_inc_return(&isp->frame_sequence);
>>   	v4l2_event_queue(isp->sd.devnode, &event);
>>   }
>>   
>> @@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>   	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>>   
>>   	/* Vertical sync signal, starting generating new frame */
>> -	if (status & RKISP1_CIF_ISP_V_START)
>> +	if (status & RKISP1_CIF_ISP_V_START) {
>> +		/*
>> +		 * Increment the frame sequence on the vsync signal.
>> +		 * This will allow applications to detect dropped.
> 
> I know just moved the comment, but I would like to take this opportunity
> to fix this text:
> 
> s/to detect dropped/to detect dropped frames.
> 
>> +		 * Note that there is a debugfs counter for dropped
>> +		 * frames, but using this event is more accurate.
> 
> I'm not sure if this phrase is accurate :P
> Since the debugfs counter should be reliable to verify we dropped frames.
> 
> I mean, usage is different.
> 
> maybe just:
> 
> s/, but using this event is more accurate././

Actually the debugfs counter for dropped frames is incremented
when there is a frame ready on the capture but there are no buffers in the v4l2 queue.
It is quit unrelated to the v-start event, so I think the whole comment can be dropped.

Thanks,
Dafna

> 
> 
> With or without these changes:
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Regards,
> Helen
> 
>> +		 */
>> +		rkisp1->isp.frame_sequence++;
>>   		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>>   
>>   	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>> index 4b4391c0a2a0..cc242ad5106e 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>> @@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>>   	 * We want the vb.sequence to be the frame that the params are applied to.
>>   	 * So we set vb.sequence to be the isp's frame_sequence + 1
>>   	 */
>> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
>> +	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
>>   	struct rkisp1_params *params = &rkisp1->params;
>>   
>>   	spin_lock(&params->config_lock);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> index 87e4104d20dd..52417b388854 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> @@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>>   {
>>   	struct rkisp1_stat_buffer *cur_stat_buf;
>>   	struct rkisp1_buffer *cur_buf = NULL;
>> -	unsigned int frame_sequence =
>> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
>> +	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
>>   	u64 timestamp = ktime_get_ns();
>>   
>>   	/* get one empty buffer */
>>

Re: [PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

Hi

Am 17.08.20 um 23:48 schrieb Helen Koike:
> Hi Dafna,
> 
> On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
>> The isp.frame_sequence is now read only from the irq handlers
>> that are all fired from the same interrupt so there is not need
>> for atomic operation.
>> In addition the frame seq incrementation is moved from
>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
>> clearer.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 20 +++++++++----------
>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>   5 files changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index c05280950ea0..d7017afc5405 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>>   	curr_buf = cap->buf.curr;
>>   
>>   	if (curr_buf) {
>> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
>> +		curr_buf->vb.sequence = isp->frame_sequence;
>>   		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>>   		curr_buf->vb.field = V4L2_FIELD_NONE;
>>   		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 9b41935c6597..79edece6ee77 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -112,7 +112,7 @@ struct rkisp1_isp {
>>   	const struct rkisp1_isp_mbus_info *src_fmt;
>>   	struct mutex ops_lock;
>>   	bool is_dphy_errctrl_disabled;
>> -	atomic_t frame_sequence;
>> +	__u32 frame_sequence;
>>   };
>>   
>>   struct rkisp1_vdev_node {
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index ad2ece78abbf..1ffe7cc7bb12 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -940,7 +940,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>>   	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
>>   		return -EINVAL;
>>   
>> -	atomic_set(&rkisp1->isp.frame_sequence, -1);
>> +	rkisp1->isp.frame_sequence = -1;
>>   	mutex_lock(&isp->ops_lock);
>>   	ret = rkisp1_config_cif(rkisp1);
>>   	if (ret)
>> @@ -1093,15 +1093,8 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
>>   	struct v4l2_event event = {
>>   		.type = V4L2_EVENT_FRAME_SYNC,
>>   	};
>> +	event.u.frame_sync.frame_sequence = isp->frame_sequence;
>>   
>> -	/*
>> -	 * Increment the frame sequence on the vsync signal.
>> -	 * This will allow applications to detect dropped.
>> -	 * Note that there is a debugfs counter for dropped
>> -	 * frames, but using this event is more accurate.
>> -	 */
>> -	event.u.frame_sync.frame_sequence =
>> -		atomic_inc_return(&isp->frame_sequence);
>>   	v4l2_event_queue(isp->sd.devnode, &event);
>>   }
>>   
>> @@ -1116,7 +1109,14 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
>>   	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>>   
>>   	/* Vertical sync signal, starting generating new frame */
>> -	if (status & RKISP1_CIF_ISP_V_START)
>> +	if (status & RKISP1_CIF_ISP_V_START) {
>> +		/*
>> +		 * Increment the frame sequence on the vsync signal.
>> +		 * This will allow applications to detect dropped.
> 
> I know just moved the comment, but I would like to take this opportunity
> to fix this text:
> 
> s/to detect dropped/to detect dropped frames.
> 
>> +		 * Note that there is a debugfs counter for dropped
>> +		 * frames, but using this event is more accurate.
> 
> I'm not sure if this phrase is accurate :P
> Since the debugfs counter should be reliable to verify we dropped frames.
> 
> I mean, usage is different.
> 
> maybe just:
> 
> s/, but using this event is more accurate././

Actually the debugfs counter for dropped frames is incremented
when there is a frame ready on the capture but there are no buffers in the v4l2 queue.
It is quit unrelated to the v-start event, so I think the whole comment can be dropped.

Thanks,
Dafna

> 
> 
> With or without these changes:
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Regards,
> Helen
> 
>> +		 */
>> +		rkisp1->isp.frame_sequence++;
>>   		rkisp1_isp_queue_event_sof(&rkisp1->isp);
>>   
>>   	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
>> index 4b4391c0a2a0..cc242ad5106e 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
>> @@ -1227,7 +1227,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>>   	 * We want the vb.sequence to be the frame that the params are applied to.
>>   	 * So we set vb.sequence to be the isp's frame_sequence + 1
>>   	 */
>> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
>> +	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
>>   	struct rkisp1_params *params = &rkisp1->params;
>>   
>>   	spin_lock(&params->config_lock);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> index 87e4104d20dd..52417b388854 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> @@ -307,8 +307,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>>   {
>>   	struct rkisp1_stat_buffer *cur_stat_buf;
>>   	struct rkisp1_buffer *cur_buf = NULL;
>> -	unsigned int frame_sequence =
>> -		atomic_read(&stats->rkisp1->isp.frame_sequence);
>> +	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
>>   	u64 timestamp = ktime_get_ns();
>>   
>>   	/* get one empty buffer */
>>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip