[PATCH v3 00/12] media: staging: rkisp1: various bug fixes

[PATCH v3 00/12] 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-start arrives, the frame sequence is incremented and
when frame-out arrives, the stats and params isr are called.
Also the vb.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 arrives
so that the first params 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 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, params buffers are added to the buffers list on qbuf and
then when a stream start from selfpath/mainpath then 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 test-various-bug-fixes-v3 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 v2:
* patches 11,12 in v3 are new, they fix the TODO issue "review and comment every lock"
* patches 1,2,4 from v2 are already applied so remove from v3
* patch 13 from v2: "media: staging: rkisp1: call media_pipeline_start/stop from stats and params" - dropped from this version
* patch "media: staging: rkisp1: params: use the new effect value in cproc config"
was reorder to be just before patch "media: staging: rkisp1: params: avoid using buffer if params is not streaming"
* patch 4: declare function 'rkisp1_params_apply_params_cfg' as static to fix a warning reported by 'kernel test robot <lkp@intel.com>'
* patch 5: rephrase commit message and inline doc as suggested by Helen.
* patch 6: add a closing "}" to "if" condition and remove useless inline comment
* patch 7: remove unneeded closing "}" to "if"


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 (12):
  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: use the new effect value in cproc
    config
  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: params: no need to lock default config
  media: staging: rkisp1: use the right variants of spin_lock
  media: staging: rkisp1: cap: protect access to buf with the spin lock

 drivers/staging/media/rkisp1/TODO             |   1 -
 drivers/staging/media/rkisp1/rkisp1-capture.c |  19 ++-
 drivers/staging/media/rkisp1/rkisp1-common.h  |   7 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c     |   2 +
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  22 ++--
 drivers/staging/media/rkisp1/rkisp1-params.c  | 118 ++++++++----------
 drivers/staging/media/rkisp1/rkisp1-stats.c   |   5 +-
 7 files changed, 76 insertions(+), 98 deletions(-)

-- 
2.17.1


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

[PATCH v3 01/12] 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>
Acked-by: Helen Koike <helen.koike@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 3ca2afc51ead..85f3b340c3bf 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1469,32 +1469,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

Re: [PATCH v3 01/12] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:51PM +0200, 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>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 31 +++++++-------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 3ca2afc51ead..85f3b340c3bf 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1469,32 +1469,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);

nit: This could be done at declaration time by using the LIST_HEAD()
macro to declare the list head.

> +
> +	/*
> +	 * 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);

nit: This is equivalent to list_splice_init(&params->params, &tmp_list);
with a simpler interface and without the need to dereference
params->params.prev manually.

Best regards,
Tomasz

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

[PATCH v3 02/12] 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>
Acked-by: Helen Koike <helen.koike@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 85f3b340c3bf..8bd7cc622e4f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1198,16 +1198,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);
 
 	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 v3 03/12] 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>
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 8bd7cc622e4f..ab2deb57b1eb 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -552,7 +552,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 v3 04/12] 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>
Reported-by: kernel test robot <lkp@intel.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
changes since v2:
declare function 'rkisp1_params_apply_params_cfg' as static
to fix a warning reported by 'kernel test robot <lkp@intel.com>'
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  4 --
 drivers/staging/media/rkisp1/rkisp1-params.c | 50 ++++++++------------
 2 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 992d8ec4c448..232bee92d0eb 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -262,10 +262,8 @@ struct rkisp1_stats {
  * @rkisp1:		pointer to the rkisp1 device
  * @config_lock:	locks the buffer list 'params' and 'is_streaming'
  * @params:		queue of rkisp1_buffer
- * @cur_params:		the first params values from userspace
  * @vdev_fmt:		v4l2_format of the metadata format
  * @is_streaming:	device is streaming
- * @is_first_params:	the first params should take effect immediately
  * @quantization:	the quantization configured on the isp's src pad
  * @raw_type:		the bayer pattern on the isp video sink pad
  */
@@ -275,10 +273,8 @@ struct rkisp1_params {
 
 	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
 	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 ab2deb57b1eb..e8049a50575f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1185,23 +1185,14 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 	}
 }
 
-void rkisp1_params_isr(struct rkisp1_device *rkisp1)
+static 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);
@@ -1218,6 +1209,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);
 }
 
@@ -1290,9 +1295,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);
 }
@@ -1420,8 +1425,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;
 }
 
@@ -1432,20 +1435,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 = 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

Re: [PATCH v3 04/12] media: staging: rkisp1: params: avoid using buffer if params is not streaming

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:54PM +0200, 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.

Thank you for the patch. Please see my comments inline.

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

How is this possible? VB2 doesn't call the .buf_queue callback until streaming is started.

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

Why?

> - The frame_sequence of the first buffer is set to -1 if the main/selfpath
> did not start streaming.

Indeed, this is invalid. The sequence number should correspond to the
sequence number of the frame that the parameters were applied to, i.e. the
parameter buffer A and the video buffer B dequeued from the CAPTURE node
which contains the first frame processed with the parameters from buffer A
should have the same sequence number.

> 
> 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>
> Reported-by: kernel test robot <lkp@intel.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
> changes since v2:
> declare function 'rkisp1_params_apply_params_cfg' as static
> to fix a warning reported by 'kernel test robot <lkp@intel.com>'
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  4 --
>  drivers/staging/media/rkisp1/rkisp1-params.c | 50 ++++++++------------
>  2 files changed, 20 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 992d8ec4c448..232bee92d0eb 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -262,10 +262,8 @@ struct rkisp1_stats {
>   * @rkisp1:		pointer to the rkisp1 device
>   * @config_lock:	locks the buffer list 'params' and 'is_streaming'
>   * @params:		queue of rkisp1_buffer
> - * @cur_params:		the first params values from userspace
>   * @vdev_fmt:		v4l2_format of the metadata format
>   * @is_streaming:	device is streaming
> - * @is_first_params:	the first params should take effect immediately
>   * @quantization:	the quantization configured on the isp's src pad
>   * @raw_type:		the bayer pattern on the isp video sink pad
>   */
> @@ -275,10 +273,8 @@ struct rkisp1_params {
>  
>  	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
>  	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 ab2deb57b1eb..e8049a50575f 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1185,23 +1185,14 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> -void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> +static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
> +					   unsigned int frame_sequence)

Should we call this _locked() since it is expected that the config_lock is
held when called?

To signify such condition, the __must_hold sparse annotation can be used.

>  {
> -	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);
> @@ -1218,6 +1209,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) {

Do we need this check? Wouldn't the queue be empty if params is not
streaming?

Also, if we decide that we need the check, we could replace the private
is_streaming flag with vb2_is_streaming().

Best regards,
Tomasz

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

[PATCH v3 05/12] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Dafna Hirschfeld]

The params isr is called when the ISP finishes processing a frame
(RKISP1_CIF_ISP_FRAME). Configurations performed in the params isr
will be applied on the next frame. Since frame_sequence is updated
on the vertical sync signal, we should use frame_sequence + 1 as
the vb.sequence of the params buffer to indicate to userspace on
which frame these parameters are being applied.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
changes since v2:
rephrase commit message and inline doc as suggested by Helen.
---
 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 e8049a50575f..24a49368df22 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1213,7 +1213,14 @@ static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
 
 void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 {
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
+	/*
+	 * This isr is called when the ISR finishes processing a frame (RKISP1_CIF_ISP_FRAME).
+	 * Configurations performed here will be applied on the next frame.
+	 * Since frame_sequence is updated on the vertical sync signal, we should use
+	 * frame_sequence + 1 here to indicate to userspace on which frame these parameters
+	 * are being applied.
+	 */
+	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 v3 06/12] 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 and the incorrect inline comment is removed.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>

---
changes since v2:
add a closing "}" to if condition
remove usless inline comment
---
 drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
 drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
 5 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 0632582a95b4..1c762a369b63 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 232bee92d0eb..51c92a251ea5 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -131,7 +131,7 @@ struct rkisp1_isp {
 	const struct rkisp1_isp_mbus_info *src_fmt;
 	struct mutex ops_lock; /* serialize the subdevice ops */
 	bool is_dphy_errctrl_disabled;
-	atomic_t frame_sequence;
+	__u32 frame_sequence;
 };
 
 /*
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 02eafea92863..0e71d600dd2b 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)
@@ -1092,15 +1092,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);
 }
 
@@ -1115,9 +1108,10 @@ 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) {
+		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
-
+	}
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
 		/* Clear pic_size_error */
 		isp_err = rkisp1_read(rkisp1, RKISP1_CIF_ISP_ERR);
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 24a49368df22..7aa76f032728 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1220,7 +1220,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	 * frame_sequence + 1 here to indicate to userspace on which frame these parameters
 	 * are being applied.
 	 */
-	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 1daab7a318dc..6aa18d970f2b 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

Re: [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:56PM +0200, 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 and the incorrect inline comment is removed.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> changes since v2:
> add a closing "}" to if condition
> remove usless inline comment
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
>  drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>  5 files changed, 9 insertions(+), 16 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 0632582a95b4..1c762a369b63 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;

I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
memory-intensive system load, like video encoding and graphics rendering,
the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
the V_START for the next frame.

I recall you did some testing back in time [1], showing that the two are
interleaved. Do you remember what CAPTURE resolution was it?

>  		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 232bee92d0eb..51c92a251ea5 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -131,7 +131,7 @@ struct rkisp1_isp {
>  	const struct rkisp1_isp_mbus_info *src_fmt;
>  	struct mutex ops_lock; /* serialize the subdevice ops */
>  	bool is_dphy_errctrl_disabled;
> -	atomic_t frame_sequence;
> +	__u32 frame_sequence;

nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
types. For kernel types please just use the plain u32.

Best regards,
Tomasz

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

Re: [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

Hi,

Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Tue, Sep 22, 2020 at 01:33:56PM +0200, 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 and the incorrect inline comment is removed.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> changes since v2:
>> add a closing "}" to if condition
>> remove usless inline comment
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>   5 files changed, 9 insertions(+), 16 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 0632582a95b4..1c762a369b63 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;
> 
> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> memory-intensive system load, like video encoding and graphics rendering,
> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> the V_START for the next frame.
> 
> I recall you did some testing back in time [1], showing that the two are
> interleaved. Do you remember what CAPTURE resolution was it?

I ran the testing again, I added a patch to allow streaming simultanously from
both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
Then I ran two tests:
stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR

the pixelformat for both is 422P.
I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
The functionality can probably only be tested on the scarlet.

Thanks,
Dafna



> 
>>   		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 232bee92d0eb..51c92a251ea5 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -131,7 +131,7 @@ struct rkisp1_isp {
>>   	const struct rkisp1_isp_mbus_info *src_fmt;
>>   	struct mutex ops_lock; /* serialize the subdevice ops */
>>   	bool is_dphy_errctrl_disabled;
>> -	atomic_t frame_sequence;
>> +	__u32 frame_sequence;
> 
> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> types. For kernel types please just use the plain u32.
> 
> Best regards,
> Tomasz
> 

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

Re: [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Tomasz Figa]

On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi,
>
> Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> > Hi Dafna,
> >
> > On Tue, Sep 22, 2020 at 01:33:56PM +0200, 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 and the incorrect inline comment is removed.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> changes since v2:
> >> add a closing "}" to if condition
> >> remove usless inline comment
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
> >>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
> >>   5 files changed, 9 insertions(+), 16 deletions(-)
> >>
> >
> > Thank you for the patch. Please see my comments inline.
> >
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >> index 0632582a95b4..1c762a369b63 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;
> >
> > I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> > memory-intensive system load, like video encoding and graphics rendering,
> > the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> > the V_START for the next frame.
> >
> > I recall you did some testing back in time [1], showing that the two are
> > interleaved. Do you remember what CAPTURE resolution was it?
>
> I ran the testing again, I added a patch to allow streaming simultanously from
> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
> Then I ran two tests:
> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR
>
> the pixelformat for both is 422P.
> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
> The functionality can probably only be tested on the scarlet.

Okay, thanks. It looks like there is always plenty of time margin on
the hardware side and if the interrupt handling is delayed for a short
time and both are handled by the same handler call, it's also going to
be handled fine because of rkisp1_capture_isr() being called before
rkisp1_isp_isr().

By the way, do we need the MIPI interrupts every frame? Perhaps we
could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,
when we get an error, we disable it and enable
RKISP1_CIF_MIPI_FRAME_END, which would then re-enable
RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?

Best regards,
Tomasz

>
> Thanks,
> Dafna
>
>
>
> >
> >>              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 232bee92d0eb..51c92a251ea5 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> >> @@ -131,7 +131,7 @@ struct rkisp1_isp {
> >>      const struct rkisp1_isp_mbus_info *src_fmt;
> >>      struct mutex ops_lock; /* serialize the subdevice ops */
> >>      bool is_dphy_errctrl_disabled;
> >> -    atomic_t frame_sequence;
> >> +    __u32 frame_sequence;
> >
> > nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> > types. For kernel types please just use the plain u32.
> >
> > Best regards,
> > Tomasz
> >

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

Re: [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Helen Koike]

Hi Tomasz,

(sorry for not had replied this earlier)

On 10/2/20 12:30 PM, Tomasz Figa wrote:
> On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> Hi,
>>
>> Am 25.09.20 um 22:42 schrieb Tomasz Figa:
>>> Hi Dafna,
>>>
>>> On Tue, Sep 22, 2020 at 01:33:56PM +0200, 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 and the incorrect inline comment is removed.
>>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> ---
>>>> changes since v2:
>>>> add a closing "}" to if condition
>>>> remove usless inline comment
>>>> ---
>>>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
>>>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>>>   5 files changed, 9 insertions(+), 16 deletions(-)
>>>>
>>>
>>> Thank you for the patch. Please see my comments inline.
>>>
>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> index 0632582a95b4..1c762a369b63 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;
>>>
>>> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
>>> memory-intensive system load, like video encoding and graphics rendering,
>>> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
>>> the V_START for the next frame.
>>>
>>> I recall you did some testing back in time [1], showing that the two are
>>> interleaved. Do you remember what CAPTURE resolution was it?
>>
>> I ran the testing again, I added a patch to allow streaming simultanously from
>> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
>> Then I ran two tests:
>> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
>> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR
>>
>> the pixelformat for both is 422P.
>> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
>> The functionality can probably only be tested on the scarlet.
> 
> Okay, thanks. It looks like there is always plenty of time margin on
> the hardware side and if the interrupt handling is delayed for a short
> time and both are handled by the same handler call, it's also going to
> be handled fine because of rkisp1_capture_isr() being called before
> rkisp1_isp_isr().
> 
> By the way, do we need the MIPI interrupts every frame? Perhaps we
> could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,
> when we get an error, we disable it and enable
> RKISP1_CIF_MIPI_FRAME_END, which would then re-enable
> RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?

The driver already do this in a sense, it disables these interrupts on
the first MIPI error and re-enable them on RKISP1_CIF_MIPI_FRAME_END.

Please check:

https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/rkisp1/rkisp1-isp.c#n1069

For convenience:

void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
{
	u32 val, status;

	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
	if (!status)
		return;

	rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);

	/*
	 * Disable DPHY errctrl interrupt, because this dphy
	 * erctrl signal is asserted until the next changes
	 * of line state. This time is may be too long and cpu
	 * is hold in this interrupt.
	 */
	if (status & RKISP1_CIF_MIPI_ERR_CTRL(0x0f)) {
		val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
		rkisp1_write(rkisp1, val & ~RKISP1_CIF_MIPI_ERR_CTRL(0x0f),
			     RKISP1_CIF_MIPI_IMSC);
		rkisp1->isp.is_dphy_errctrl_disabled = true;
	}

	/*
	 * Enable DPHY errctrl interrupt again, if mipi have receive
	 * the whole frame without any error.
	 */
	if (status == RKISP1_CIF_MIPI_FRAME_END) {
		/*
		 * Enable DPHY errctrl interrupt again, if mipi have receive
		 * the whole frame without any error.
		 */
		if (rkisp1->isp.is_dphy_errctrl_disabled) {
			val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
			val |= RKISP1_CIF_MIPI_ERR_CTRL(0x0f);
			rkisp1_write(rkisp1, val, RKISP1_CIF_MIPI_IMSC);
			rkisp1->isp.is_dphy_errctrl_disabled = false;
		}
	} else {
		rkisp1->debug.mipi_error++;
	}
}

Regards,
Helen

> 
> Best regards,
> Tomasz
> 
>>
>> Thanks,
>> Dafna
>>
>>
>>
>>>
>>>>              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 232bee92d0eb..51c92a251ea5 100644
>>>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>>>> @@ -131,7 +131,7 @@ struct rkisp1_isp {
>>>>      const struct rkisp1_isp_mbus_info *src_fmt;
>>>>      struct mutex ops_lock; /* serialize the subdevice ops */
>>>>      bool is_dphy_errctrl_disabled;
>>>> -    atomic_t frame_sequence;
>>>> +    __u32 frame_sequence;
>>>
>>> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
>>> types. For kernel types please just use the plain u32.
>>>
>>> Best regards,
>>> Tomasz
>>>
> 

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

Re: [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Tomasz Figa]

Hi Helen,

On Fri, Nov 6, 2020 at 11:43 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Tomasz,
>
> (sorry for not had replied this earlier)
>
> On 10/2/20 12:30 PM, Tomasz Figa wrote:
> > On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> wrote:
> >>
> >> Hi,
> >>
> >> Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> >>> Hi Dafna,
> >>>
> >>> On Tue, Sep 22, 2020 at 01:33:56PM +0200, 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 and the incorrect inline comment is removed.
> >>>>
> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>>> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>>>
> >>>> ---
> >>>> changes since v2:
> >>>> add a closing "}" to if condition
> >>>> remove usless inline comment
> >>>> ---
> >>>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
> >>>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
> >>>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
> >>>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
> >>>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
> >>>>   5 files changed, 9 insertions(+), 16 deletions(-)
> >>>>
> >>>
> >>> Thank you for the patch. Please see my comments inline.
> >>>
> >>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>>> index 0632582a95b4..1c762a369b63 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;
> >>>
> >>> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> >>> memory-intensive system load, like video encoding and graphics rendering,
> >>> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> >>> the V_START for the next frame.
> >>>
> >>> I recall you did some testing back in time [1], showing that the two are
> >>> interleaved. Do you remember what CAPTURE resolution was it?
> >>
> >> I ran the testing again, I added a patch to allow streaming simultanously from
> >> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
> >> Then I ran two tests:
> >> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
> >> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR
> >>
> >> the pixelformat for both is 422P.
> >> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
> >> The functionality can probably only be tested on the scarlet.
> >
> > Okay, thanks. It looks like there is always plenty of time margin on
> > the hardware side and if the interrupt handling is delayed for a short
> > time and both are handled by the same handler call, it's also going to
> > be handled fine because of rkisp1_capture_isr() being called before
> > rkisp1_isp_isr().
> >
> > By the way, do we need the MIPI interrupts every frame? Perhaps we
> > could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,
> > when we get an error, we disable it and enable
> > RKISP1_CIF_MIPI_FRAME_END, which would then re-enable
> > RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?
>
> The driver already do this in a sense, it disables these interrupts on
> the first MIPI error and re-enable them on RKISP1_CIF_MIPI_FRAME_END.

Yes, it disables the ERR interrupts, but doesn't it keep the FRAME_END
interrupt enabled all the time? (At least that seems to be the case in
your traces.) Is it necessary?

Best regards,
Tomasz

>
> Please check:
>
> https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/rkisp1/rkisp1-isp.c#n1069
>
> For convenience:
>
> void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> {
>         u32 val, status;
>
>         status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
>         if (!status)
>                 return;
>
>         rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);
>
>         /*
>          * Disable DPHY errctrl interrupt, because this dphy
>          * erctrl signal is asserted until the next changes
>          * of line state. This time is may be too long and cpu
>          * is hold in this interrupt.
>          */
>         if (status & RKISP1_CIF_MIPI_ERR_CTRL(0x0f)) {
>                 val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
>                 rkisp1_write(rkisp1, val & ~RKISP1_CIF_MIPI_ERR_CTRL(0x0f),
>                              RKISP1_CIF_MIPI_IMSC);
>                 rkisp1->isp.is_dphy_errctrl_disabled = true;
>         }
>
>         /*
>          * Enable DPHY errctrl interrupt again, if mipi have receive
>          * the whole frame without any error.
>          */
>         if (status == RKISP1_CIF_MIPI_FRAME_END) {
>                 /*
>                  * Enable DPHY errctrl interrupt again, if mipi have receive
>                  * the whole frame without any error.
>                  */
>                 if (rkisp1->isp.is_dphy_errctrl_disabled) {
>                         val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
>                         val |= RKISP1_CIF_MIPI_ERR_CTRL(0x0f);
>                         rkisp1_write(rkisp1, val, RKISP1_CIF_MIPI_IMSC);
>                         rkisp1->isp.is_dphy_errctrl_disabled = false;
>                 }
>         } else {
>                 rkisp1->debug.mipi_error++;
>         }
> }
>
> Regards,
> Helen
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> Thanks,
> >> Dafna
> >>
> >>
> >>
> >>>
> >>>>              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 232bee92d0eb..51c92a251ea5 100644
> >>>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> >>>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> >>>> @@ -131,7 +131,7 @@ struct rkisp1_isp {
> >>>>      const struct rkisp1_isp_mbus_info *src_fmt;
> >>>>      struct mutex ops_lock; /* serialize the subdevice ops */
> >>>>      bool is_dphy_errctrl_disabled;
> >>>> -    atomic_t frame_sequence;
> >>>> +    __u32 frame_sequence;
> >>>
> >>> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> >>> types. For kernel types please just use the plain u32.
> >>>
> >>> Best regards,
> >>> Tomasz
> >>>
> >

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

[PATCH v3 07/12] 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>
---
changes since v2:
remove unneeded closing "}" due to fixing
"media: staging: rkisp1: remove atomic operations for frame sequence"
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 1 +
 drivers/staging/media/rkisp1/rkisp1-dev.c    | 2 ++
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 4 ++++
 3 files changed, 7 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 51c92a251ea5..8c79679e67b8 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -326,6 +326,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 d85ac10e5494..91584695804b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -444,6 +444,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 0e71d600dd2b..9020633c1b3f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -1111,6 +1111,10 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	if (status & RKISP1_CIF_ISP_V_START) {
 		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 v3 08/12] 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>
Acked-by: Helen Koike <helen.koike@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 9020633c1b3f..cb6a94136612 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 v3 09/12] 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>
Acked-by: Helen Koike <helen.koike@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 6aa18d970f2b..51c64f75fe29 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

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

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:59PM +0200, 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>
> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 6aa18d970f2b..51c64f75fe29 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;

Do we need this separate stream? This is a similar note to the
parameters patch - it should be possible to handle this based on the
presence of the buffers in the list, without a custom flag.

Best regards,
Tomasz

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

[PATCH v3 10/12] 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>
Acked-by: Helen Koike <helen.koike@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 7aa76f032728..f0258b4258ed 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1276,8 +1276,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);
@@ -1302,6 +1300,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

[PATCH v3 11/12] media: staging: rkisp1: use the right variants of spin_lock

[Dafna Hirschfeld]

When locking, use either 'spin_lock' or 'spin_lock_irq'
according to the context. There is nowhere need to
use 'spin_lock_irqsave'.
Outside of irq context, always use 'spin_lock_irq'
to be on the safe side.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 1c762a369b63..012c0825a386 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -626,9 +626,8 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 {
 	struct rkisp1_isp *isp = &cap->rkisp1->isp;
 	struct rkisp1_buffer *curr_buf;
-	unsigned long flags;
 
-	spin_lock_irqsave(&cap->buf.lock, flags);
+	spin_lock(&cap->buf.lock);
 	curr_buf = cap->buf.curr;
 
 	if (curr_buf) {
@@ -641,7 +640,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 	}
 
 	rkisp1_set_next_buf(cap);
-	spin_unlock_irqrestore(&cap->buf.lock, flags);
+	spin_unlock(&cap->buf.lock);
 }
 
 void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
@@ -716,7 +715,6 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 		container_of(vbuf, struct rkisp1_buffer, vb);
 	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
 	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
-	unsigned long flags;
 	unsigned int i;
 
 	memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
@@ -741,9 +739,9 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
 		     ispbuf->buff_addr[RKISP1_PLANE_CB]);
 
-	spin_lock_irqsave(&cap->buf.lock, flags);
+	spin_lock_irq(&cap->buf.lock);
 	list_add_tail(&ispbuf->queue, &cap->buf.queue);
-	spin_unlock_irqrestore(&cap->buf.lock, flags);
+	spin_unlock_irq(&cap->buf.lock);
 }
 
 static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
@@ -769,10 +767,9 @@ static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
 static void rkisp1_return_all_buffers(struct rkisp1_capture *cap,
 				      enum vb2_buffer_state state)
 {
-	unsigned long flags;
 	struct rkisp1_buffer *buf;
 
-	spin_lock_irqsave(&cap->buf.lock, flags);
+	spin_lock_irq(&cap->buf.lock);
 	if (cap->buf.curr) {
 		vb2_buffer_done(&cap->buf.curr->vb.vb2_buf, state);
 		cap->buf.curr = NULL;
@@ -787,7 +784,7 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap,
 		list_del(&buf->queue);
 		vb2_buffer_done(&buf->vb.vb2_buf, state);
 	}
-	spin_unlock_irqrestore(&cap->buf.lock, flags);
+	spin_unlock_irq(&cap->buf.lock);
 }
 
 /*
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index f0258b4258ed..986d293201e6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1300,16 +1300,15 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	else
 		rkisp1_csm_config(params, false);
 
-	spin_lock(&params->config_lock);
+	spin_lock_irq(&params->config_lock);
 
 	/* 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);
+	spin_unlock_irq(&params->config_lock);
 }
 
-/* Not called when the camera active, thus not isr protection. */
 void rkisp1_params_configure(struct rkisp1_params *params,
 			     enum rkisp1_fmt_raw_pat_type bayer_pat,
 			     enum v4l2_quantization quantization)
@@ -1442,12 +1441,11 @@ 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;
-	unsigned long flags;
 
 	params_buf->vaddr = vb2_plane_vaddr(vb, 0);
-	spin_lock_irqsave(&params->config_lock, flags);
+	spin_lock_irq(&params->config_lock);
 	list_add_tail(&params_buf->queue, &params->params);
-	spin_unlock_irqrestore(&params->config_lock, flags);
+	spin_unlock_irq(&params->config_lock);
 }
 
 static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
@@ -1465,7 +1463,6 @@ 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;
 
 	INIT_LIST_HEAD(&tmp_list);
 
@@ -1474,10 +1471,10 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 	 * and then we can iterate it and call vb2_buffer_done
 	 * without holding the lock
 	 */
-	spin_lock_irqsave(&params->config_lock, flags);
+	spin_lock_irq(&params->config_lock);
 	params->is_streaming = false;
 	list_cut_position(&tmp_list, &params->params, params->params.prev);
-	spin_unlock_irqrestore(&params->config_lock, flags);
+	spin_unlock_irq(&params->config_lock);
 
 	list_for_each_entry(buf, &tmp_list, queue)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
@@ -1487,11 +1484,10 @@ static int
 rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_params *params = queue->drv_priv;
-	unsigned long flags;
 
-	spin_lock_irqsave(&params->config_lock, flags);
+	spin_lock_irq(&params->config_lock);
 	params->is_streaming = true;
-	spin_unlock_irqrestore(&params->config_lock, flags);
+	spin_unlock_irq(&params->config_lock);
 
 	return 0;
 }
-- 
2.17.1


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

Re: [PATCH v3 11/12] media: staging: rkisp1: use the right variants of spin_lock

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:34:01PM +0200, Dafna Hirschfeld wrote:
> When locking, use either 'spin_lock' or 'spin_lock_irq'
> according to the context. There is nowhere need to
> use 'spin_lock_irqsave'.
> Outside of irq context, always use 'spin_lock_irq'
> to be on the safe side.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 15 ++++++--------
>  drivers/staging/media/rkisp1/rkisp1-params.c  | 20 ++++++++-----------
>  2 files changed, 14 insertions(+), 21 deletions(-)
>

I'd prefer to keep the irqsave variant, as it doesn't hurt, is more
consistent (all the calls on the same spinlock use the same variant) and
makes it possible to refactor the code easier in the future.

Best regards,
Tomasz

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

[PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Dafna Hirschfeld]

The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
which access the buffers, so the call should be protected by
taking the cap->buf.lock.
After this patch, all locks are reviewed and commented so remove
the TODO item "review and comment every lock"

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

diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
index f0c90d1c86a8..9662e9b51c7f 100644
--- a/drivers/staging/media/rkisp1/TODO
+++ b/drivers/staging/media/rkisp1/TODO
@@ -1,6 +1,5 @@
 * Fix pad format size for statistics and parameters entities.
 * Fix checkpatch errors.
-* Review and comment every lock
 * Handle quantization
 * streaming paths (mainpath and selfpath) check if the other path is streaming
 in several places of the code, review this, specially that it doesn't seem it
diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 012c0825a386..b9e56dab77af 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -913,6 +913,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
 	cap->ops->config(cap);
 
 	/* Setup a buffer for the next frame */
+	spin_lock_irq(&cap->buf.lock);
 	rkisp1_set_next_buf(cap);
 	cap->ops->enable(cap);
 	/* It's safe to config ACTIVE and SHADOW regs for the
@@ -930,6 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
 			     RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
 		rkisp1_set_next_buf(cap);
 	}
+	spin_unlock_irq(&cap->buf.lock);
 	cap->is_streaming = true;
 }
 
-- 
2.17.1


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

Re: [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Tomasz Figa]

Hi Dafna,

On Tue, Sep 22, 2020 at 01:34:02PM +0200, Dafna Hirschfeld wrote:
> The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
> which access the buffers, so the call should be protected by
> taking the cap->buf.lock.
> After this patch, all locks are reviewed and commented so remove
> the TODO item "review and comment every lock"
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO             | 1 -
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index f0c90d1c86a8..9662e9b51c7f 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -1,6 +1,5 @@
>  * Fix pad format size for statistics and parameters entities.
>  * Fix checkpatch errors.
> -* Review and comment every lock
>  * Handle quantization
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 012c0825a386..b9e56dab77af 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -913,6 +913,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>  	cap->ops->config(cap);
>  
>  	/* Setup a buffer for the next frame */
> +	spin_lock_irq(&cap->buf.lock);
>  	rkisp1_set_next_buf(cap);
>  	cap->ops->enable(cap);
>  	/* It's safe to config ACTIVE and SHADOW regs for the
> @@ -930,6 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>  			     RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
>  		rkisp1_set_next_buf(cap);
>  	}
> +	spin_unlock_irq(&cap->buf.lock);
>  	cap->is_streaming = true;

Should the is_streaming flag be protected in some way?

Also, similarly to the other nodes, is the flag actually needed?

Best regards,
Tomasz

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

Re: [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Mauro Carvalho Chehab]

Em Tue, 22 Sep 2020 13:34:02 +0200
Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:

> The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
> which access the buffers, so the call should be protected by
> taking the cap->buf.lock.
> After this patch, all locks are reviewed and commented so remove
> the TODO item "review and comment every lock"
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO             | 1 -
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index f0c90d1c86a8..9662e9b51c7f 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -1,6 +1,5 @@
>  * Fix pad format size for statistics and parameters entities.
>  * Fix checkpatch errors.
> -* Review and comment every lock
>  * Handle quantization
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it


FYI,

There was a trivial context conflict here. Basically, the upstream
version has this:


	 * Add uapi docs. Remember to add documentation of how quantization is handled.

I solved the conflict, but as some patches for rkisp1 got merged
on a different pull request, and there were some uapi docs at the
other PR, maybe this would need to be revisited.

Thanks,
Mauro

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

Re: [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Dafna Hirschfeld]

Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab:
> Em Tue, 22 Sep 2020 13:34:02 +0200
> Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:
> 
>> The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
>> which access the buffers, so the call should be protected by
>> taking the cap->buf.lock.
>> After this patch, all locks are reviewed and commented so remove
>> the TODO item "review and comment every lock"
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/TODO             | 1 -
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
>> index f0c90d1c86a8..9662e9b51c7f 100644
>> --- a/drivers/staging/media/rkisp1/TODO
>> +++ b/drivers/staging/media/rkisp1/TODO
>> @@ -1,6 +1,5 @@
>>   * Fix pad format size for statistics and parameters entities.
>>   * Fix checkpatch errors.
>> -* Review and comment every lock
>>   * Handle quantization
>>   * streaming paths (mainpath and selfpath) check if the other path is streaming
>>   in several places of the code, review this, specially that it doesn't seem it
> 
> 
> FYI,
> 
> There was a trivial context conflict here. Basically, the upstream
> version has this:
> 
> 
> 	 * Add uapi docs. Remember to add documentation of how quantization is handled.
> 
> I solved the conflict, but as some patches for rkisp1 got merged
> on a different pull request, and there were some uapi docs at the
> other PR, maybe this would need to be revisited.

I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst
so the TODO item
"* Add uapi docs. Remember to add documentation of how quantization is handled."
can be removed.

Thanks,
Dafna

> 
> Thanks,
> Mauro
> 

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

Re: [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Mauro Carvalho Chehab]

Em Sun, 27 Sep 2020 11:54:10 +0200
Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:

> Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab:
> > Em Tue, 22 Sep 2020 13:34:02 +0200
> > Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:
> >   
> >> The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
> >> which access the buffers, so the call should be protected by
> >> taking the cap->buf.lock.
> >> After this patch, all locks are reviewed and commented so remove
> >> the TODO item "review and comment every lock"
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   drivers/staging/media/rkisp1/TODO             | 1 -
> >>   drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++
> >>   2 files changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> >> index f0c90d1c86a8..9662e9b51c7f 100644
> >> --- a/drivers/staging/media/rkisp1/TODO
> >> +++ b/drivers/staging/media/rkisp1/TODO
> >> @@ -1,6 +1,5 @@
> >>   * Fix pad format size for statistics and parameters entities.
> >>   * Fix checkpatch errors.
> >> -* Review and comment every lock
> >>   * Handle quantization
> >>   * streaming paths (mainpath and selfpath) check if the other path is streaming
> >>   in several places of the code, review this, specially that it doesn't seem it  
> > 
> > 
> > FYI,
> > 
> > There was a trivial context conflict here. Basically, the upstream
> > version has this:
> > 
> > 
> > 	 * Add uapi docs. Remember to add documentation of how quantization is handled.
> > 
> > I solved the conflict, but as some patches for rkisp1 got merged
> > on a different pull request, and there were some uapi docs at the
> > other PR, maybe this would need to be revisited.  
> 
> I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst
> so the TODO item
> "* Add uapi docs. Remember to add documentation of how quantization is handled."
> can be removed.

Ok. Please send a followup patch with a "fixes: " tag mentioned the
commit that added it.

Thanks,
Mauro

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

Re: [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock

[Dafna Hirschfeld]

Am 27.09.20 um 13:05 schrieb Mauro Carvalho Chehab:
> Em Sun, 27 Sep 2020 11:54:10 +0200
> Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:
> 
>> Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab:
>>> Em Tue, 22 Sep 2020 13:34:02 +0200
>>> Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu:
>>>    
>>>> The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf'
>>>> which access the buffers, so the call should be protected by
>>>> taking the cap->buf.lock.
>>>> After this patch, all locks are reviewed and commented so remove
>>>> the TODO item "review and comment every lock"
>>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> ---
>>>>    drivers/staging/media/rkisp1/TODO             | 1 -
>>>>    drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++
>>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
>>>> index f0c90d1c86a8..9662e9b51c7f 100644
>>>> --- a/drivers/staging/media/rkisp1/TODO
>>>> +++ b/drivers/staging/media/rkisp1/TODO
>>>> @@ -1,6 +1,5 @@
>>>>    * Fix pad format size for statistics and parameters entities.
>>>>    * Fix checkpatch errors.
>>>> -* Review and comment every lock
>>>>    * Handle quantization
>>>>    * streaming paths (mainpath and selfpath) check if the other path is streaming
>>>>    in several places of the code, review this, specially that it doesn't seem it
>>>
>>>
>>> FYI,
>>>
>>> There was a trivial context conflict here. Basically, the upstream
>>> version has this:
>>>
>>>
>>> 	 * Add uapi docs. Remember to add documentation of how quantization is handled.
>>>
>>> I solved the conflict, but as some patches for rkisp1 got merged
>>> on a different pull request, and there were some uapi docs at the
>>> other PR, maybe this would need to be revisited.
>>
>> I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst
>> so the TODO item
>> "* Add uapi docs. Remember to add documentation of how quantization is handled."
>> can be removed.
> 
> Ok. Please send a followup patch with a "fixes: " tag mentioned the
> commit that added it.

sent just now,

Thanks,
Dafna

> 
> Thanks,
> Mauro
> 

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