[PATCH 0/6] media: staging: rkisp1: improvements

[PATCH 0/6] media: staging: rkisp1: improvements

[Dafna Hirschfeld]

Some improvements to the rkisp1 code, mainly
to the params and stats.
The first patch was already part of previous patchset

I tested using wrappers for 'v4l2-ctl', 'media-ctl'
and for 'cam' command from libcamera:

git clone --single-branch --branch rkisp1-oct-2 https://gitlab.collabora.com/dafna/v4l-utils.git
cd v4l-utils/contrib/test/
./rkisp1-unit-tests.sh

Dafna Hirschfeld (6):
  media: staging: rkisp1: validate links before powering and streaming
  media: staging: rkisp1: params: in stop_streaming, use
    list_splice_init to move the buffers
  media: staging: rkisp1: initialize buffer lists only on probe
  media: staging: rkisp1: remove the 'is_streaming' field from stats and
    params
  media: staging: rkisp1: params: add '__must_hold' to
    rkisp1_params_apply_params_cfg
  media: staging: rkisp1: cap: add warning when link validation fails

 drivers/staging/media/rkisp1/rkisp1-capture.c | 31 +++++++++++-------
 drivers/staging/media/rkisp1/rkisp1-common.h  | 12 +++----
 drivers/staging/media/rkisp1/rkisp1-params.c  | 32 +++----------------
 drivers/staging/media/rkisp1/rkisp1-stats.c   | 21 ------------
 4 files changed, 29 insertions(+), 67 deletions(-)

-- 
2.17.1


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

[PATCH 1/6] media: staging: rkisp1: validate links before powering and streaming

[Dafna Hirschfeld]

In function rkisp1_vb2_start_streaming, the call to
media_pipeline_start should be the first thing in order
to validate the links and prevents their state from being modified
before power up and streaming.

Adjust stop streaming to the same logic, call media_pipeline_stop
after we disable streaming on the entities in the topology.

Signed-off-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index b6f497ce3e95..9b4a12e13f13 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -921,7 +921,6 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue)
 	mutex_lock(&cap->rkisp1->stream_lock);
 
 	rkisp1_stream_stop(cap);
-	media_pipeline_stop(&node->vdev.entity);
 	ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL,
 					rkisp1_pipeline_disable_cb);
 	if (ret)
@@ -937,6 +936,8 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue)
 
 	rkisp1_dummy_buf_destroy(cap);
 
+	media_pipeline_stop(&node->vdev.entity);
+
 	mutex_unlock(&cap->rkisp1->stream_lock);
 }
 
@@ -986,9 +987,15 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 
 	mutex_lock(&cap->rkisp1->stream_lock);
 
+	ret = media_pipeline_start(entity, &cap->rkisp1->pipe);
+	if (ret) {
+		dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret);
+		goto err_ret_buffers;
+	}
+
 	ret = rkisp1_dummy_buf_create(cap);
 	if (ret)
-		goto err_ret_buffers;
+		goto err_pipeline_stop;
 
 	ret = pm_runtime_get_sync(cap->rkisp1->dev);
 	if (ret < 0) {
@@ -1009,18 +1016,10 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	if (ret)
 		goto err_stop_stream;
 
-	ret = media_pipeline_start(entity, &cap->rkisp1->pipe);
-	if (ret) {
-		dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret);
-		goto err_pipe_disable;
-	}
-
 	mutex_unlock(&cap->rkisp1->stream_lock);
 
 	return 0;
 
-err_pipe_disable:
-	rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb);
 err_stop_stream:
 	rkisp1_stream_stop(cap);
 	v4l2_pipeline_pm_put(entity);
@@ -1028,6 +1027,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	pm_runtime_put(cap->rkisp1->dev);
 err_destroy_dummy:
 	rkisp1_dummy_buf_destroy(cap);
+err_pipeline_stop:
+	media_pipeline_stop(entity);
 err_ret_buffers:
 	rkisp1_return_all_buffers(cap, VB2_BUF_STATE_QUEUED);
 	mutex_unlock(&cap->rkisp1->stream_lock);
-- 
2.17.1


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

[PATCH 2/6] media: staging: rkisp1: params: in stop_streaming, use list_splice_init to move the buffers

[Dafna Hirschfeld]

Currently the code uses 'list_cut_position' to move the
buffers to a temporary list. Replace it with
'list_splice_init'. This is nicer since we don't need
to access params.prev. Also, replace INIT_LIST_HEAD
with the simpler LIST_HEAD.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 986d293201e6..cb9f3332a9a0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1462,9 +1462,7 @@ 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;
-
-	INIT_LIST_HEAD(&tmp_list);
+	LIST_HEAD(tmp_list);
 
 	/*
 	 * we first move the buffers into a local list 'tmp_list'
@@ -1473,7 +1471,7 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 	 */
 	spin_lock_irq(&params->config_lock);
 	params->is_streaming = false;
-	list_cut_position(&tmp_list, &params->params, params->params.prev);
+	list_splice_init(&params->params, &tmp_list);
 	spin_unlock_irq(&params->config_lock);
 
 	list_for_each_entry(buf, &tmp_list, queue)
-- 
2.17.1


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

[PATCH 3/6] media: staging: rkisp1: initialize buffer lists only on probe

[Dafna Hirschfeld]

The buffer lists of stats and params entities
are initialized in queue_setup callback with
'INIT_LIST_HEAD'. It is enough to initialize
the lists only upon registration.
For the stats entity the list is already
initialize upon registration.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index cb9f3332a9a0..aa758f8c2264 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1420,8 +1420,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
 					 unsigned int sizes[],
 					 struct device *alloc_devs[])
 {
-	struct rkisp1_params *params = vq->drv_priv;
-
 	*num_buffers = clamp_t(u32, *num_buffers,
 			       RKISP1_ISP_PARAMS_REQ_BUFS_MIN,
 			       RKISP1_ISP_PARAMS_REQ_BUFS_MAX);
@@ -1430,7 +1428,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = sizeof(struct rkisp1_params_cfg);
 
-	INIT_LIST_HEAD(&params->params);
 	return 0;
 }
 
@@ -1545,6 +1542,7 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
 
 	params->rkisp1 = rkisp1;
 	mutex_init(&node->vlock);
+	INIT_LIST_HEAD(&params->params);
 	spin_lock_init(&params->config_lock);
 
 	strscpy(vdev->name, RKISP1_PARAMS_DEV_NAME, sizeof(vdev->name));
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 51c64f75fe29..c58ae52b8a98 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -94,8 +94,6 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
 					unsigned int sizes[],
 					struct device *alloc_devs[])
 {
-	struct rkisp1_stats *stats = vq->drv_priv;
-
 	*num_planes = 1;
 
 	*num_buffers = clamp_t(u32, *num_buffers, RKISP1_ISP_STATS_REQ_BUFS_MIN,
@@ -103,8 +101,6 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = sizeof(struct rkisp1_stat_buffer);
 
-	INIT_LIST_HEAD(&stats->stat);
-
 	return 0;
 }
 
-- 
2.17.1


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

[PATCH 4/6] media: staging: rkisp1: remove the 'is_streaming' field from stats and params

[Dafna Hirschfeld]

The params and stats entities have a field 'is_streaming'.
This field is not needed since the entities can have available
buffers only if they stream and therefore it is enough to
check if there are buffers available.
As a result, their start_stream callbacks can be removed since
they only set the 'is_streaming' field.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 45abacdbb664..692333c66f9d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -240,19 +240,17 @@ struct rkisp1_capture {
  *
  * @vnode:	  video node
  * @rkisp1:	  pointer to the rkisp1 device
- * @lock:	  locks the buffer list 'stat' and 'is_streaming'
+ * @lock:	  locks the buffer list 'stat'
  * @stat:	  queue of rkisp1_buffer
  * @vdev_fmt:	  v4l2_format of the metadata format
- * @is_streaming: device is streaming
  */
 struct rkisp1_stats {
 	struct rkisp1_vdev_node vnode;
 	struct rkisp1_device *rkisp1;
 
-	spinlock_t lock; /* locks the buffers list 'stats' and 'is_streaming' */
+	spinlock_t lock; /* locks the buffers list 'stats' */
 	struct list_head stat;
 	struct v4l2_format vdev_fmt;
-	bool is_streaming;
 };
 
 /*
@@ -260,10 +258,9 @@ struct rkisp1_stats {
  *
  * @vnode:		video node
  * @rkisp1:		pointer to the rkisp1 device
- * @config_lock:	locks the buffer list 'params' and 'is_streaming'
+ * @config_lock:	locks the buffer list 'params'
  * @params:		queue of rkisp1_buffer
  * @vdev_fmt:		v4l2_format of the metadata format
- * @is_streaming:	device is streaming
  * @quantization:	the quantization configured on the isp's src pad
  * @raw_type:		the bayer pattern on the isp video sink pad
  */
@@ -271,10 +268,9 @@ struct rkisp1_params {
 	struct rkisp1_vdev_node vnode;
 	struct rkisp1_device *rkisp1;
 
-	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
+	spinlock_t config_lock; /* locks the buffers list 'params' */
 	struct list_head params;
 	struct v4l2_format vdev_fmt;
-	bool is_streaming;
 
 	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 aa758f8c2264..3afbc24ca05e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1224,10 +1224,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	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);
@@ -1303,8 +1299,7 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	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);
+	rkisp1_params_apply_params_cfg(params, 0);
 
 	spin_unlock_irq(&params->config_lock);
 }
@@ -1467,7 +1462,6 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 	 * without holding the lock
 	 */
 	spin_lock_irq(&params->config_lock);
-	params->is_streaming = false;
 	list_splice_init(&params->params, &tmp_list);
 	spin_unlock_irq(&params->config_lock);
 
@@ -1475,25 +1469,12 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 }
 
-static int
-rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
-{
-	struct rkisp1_params *params = queue->drv_priv;
-
-	spin_lock_irq(&params->config_lock);
-	params->is_streaming = true;
-	spin_unlock_irq(&params->config_lock);
-
-	return 0;
-}
-
 static struct vb2_ops rkisp1_params_vb2_ops = {
 	.queue_setup = rkisp1_params_vb2_queue_setup,
 	.wait_prepare = vb2_ops_wait_prepare,
 	.wait_finish = vb2_ops_wait_finish,
 	.buf_queue = rkisp1_params_vb2_buf_queue,
 	.buf_prepare = rkisp1_params_vb2_buf_prepare,
-	.start_streaming = rkisp1_params_vb2_start_streaming,
 	.stop_streaming = rkisp1_params_vb2_stop_streaming,
 
 };
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index c58ae52b8a98..3ddab8fa8f2d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -136,7 +136,6 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 	unsigned int i;
 
 	spin_lock_irq(&stats->lock);
-	stats->is_streaming = false;
 	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
 		if (list_empty(&stats->stat))
 			break;
@@ -148,18 +147,6 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
 	spin_unlock_irq(&stats->lock);
 }
 
-static int
-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;
-}
-
 static const struct vb2_ops rkisp1_stats_vb2_ops = {
 	.queue_setup = rkisp1_stats_vb2_queue_setup,
 	.buf_queue = rkisp1_stats_vb2_buf_queue,
@@ -167,7 +154,6 @@ static const struct vb2_ops rkisp1_stats_vb2_ops = {
 	.wait_prepare = vb2_ops_wait_prepare,
 	.wait_finish = vb2_ops_wait_finish,
 	.stop_streaming = rkisp1_stats_vb2_stop_streaming,
-	.start_streaming = rkisp1_stats_vb2_start_streaming,
 };
 
 static int
@@ -355,12 +341,9 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
 	if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
 		rkisp1->debug.stats_error++;
 
-	if (!stats->is_streaming)
-		goto unlock;
 	if (isp_ris & RKISP1_STATS_MEAS_MASK)
 		rkisp1_stats_send_measurement(stats, isp_ris);
 
-unlock:
 	spin_unlock(&stats->lock);
 }
 
-- 
2.17.1


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

[PATCH 5/6] media: staging: rkisp1: params: add '__must_hold' to rkisp1_params_apply_params_cfg

[Dafna Hirschfeld]

The function rkisp1_params_apply_params_cfg must be under the
the lock of params->config_lock. Add the __must_hold annotation
to indicate it.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 3afbc24ca05e..aa18a113245b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1187,6 +1187,7 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
 
 static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
 					   unsigned int frame_sequence)
+	__must_hold(&params->config_lock)
 {
 	struct rkisp1_params_cfg *new_params;
 	struct rkisp1_buffer *cur_buf = NULL;
-- 
2.17.1


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

[PATCH 6/6] media: staging: rkisp1: cap: add warning when link validation fails

[Dafna Hirschfeld]

The link validation between the capture and resizer might
fail because of various reasons. Add an informative warning
to make it easier to debug.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 9b4a12e13f13..75321280ebf0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1269,8 +1269,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 
 	if (sd_fmt.format.height != cap->pix.fmt.height ||
 	    sd_fmt.format.width != cap->pix.fmt.width ||
-	    sd_fmt.format.code != fmt->mbus)
+	    sd_fmt.format.code != fmt->mbus) {
+		dev_warn(cap->rkisp1->dev,
+			 "%s: failed for '%s' sd:cap: height %u:%u, width %u:%u mbus: 0x%x:0x%x\n",
+			 __func__,
+			 cap->vnode.vdev.name,
+			 sd_fmt.format.height, cap->pix.fmt.height,
+			 sd_fmt.format.width, cap->pix.fmt.width,
+			 sd_fmt.format.code, fmt->mbus);
 		return -EPIPE;
+	}
 
 	return 0;
 }
-- 
2.17.1


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

Re: [PATCH 6/6] media: staging: rkisp1: cap: add warning when link validation fails

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Fri, Oct 02, 2020 at 08:42:22PM +0200, Dafna Hirschfeld wrote:
> The link validation between the capture and resizer might
> fail because of various reasons. Add an informative warning
> to make it easier to debug.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 9b4a12e13f13..75321280ebf0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1269,8 +1269,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  
>  	if (sd_fmt.format.height != cap->pix.fmt.height ||
>  	    sd_fmt.format.width != cap->pix.fmt.width ||
> -	    sd_fmt.format.code != fmt->mbus)
> +	    sd_fmt.format.code != fmt->mbus) {
> +		dev_warn(cap->rkisp1->dev,

I'd make this dev_dbg().

> +			 "%s: failed for '%s' sd:cap: height %u:%u, width %u:%u mbus: 0x%x:0x%x\n",
> +			 __func__,
> +			 cap->vnode.vdev.name,
> +			 sd_fmt.format.height, cap->pix.fmt.height,
> +			 sd_fmt.format.width, cap->pix.fmt.width,
> +			 sd_fmt.format.code, fmt->mbus);
>  		return -EPIPE;
> +	}
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 2/6] media: staging: rkisp1: params: in stop_streaming, use list_splice_init to move the buffers

[Helen Koike]

On 10/2/20 3:42 PM, Dafna Hirschfeld wrote:
> Currently the code uses 'list_cut_position' to move the
> buffers to a temporary list. Replace it with
> 'list_splice_init'. This is nicer since we don't need
> to access params.prev. Also, replace INIT_LIST_HEAD
> with the simpler LIST_HEAD.
> 
> 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 | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 986d293201e6..cb9f3332a9a0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1462,9 +1462,7 @@ 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;
> -
> -	INIT_LIST_HEAD(&tmp_list);
> +	LIST_HEAD(tmp_list);
>  
>  	/*
>  	 * we first move the buffers into a local list 'tmp_list'
> @@ -1473,7 +1471,7 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  	 */
>  	spin_lock_irq(&params->config_lock);
>  	params->is_streaming = false;
> -	list_cut_position(&tmp_list, &params->params, params->params.prev);
> +	list_splice_init(&params->params, &tmp_list);
>  	spin_unlock_irq(&params->config_lock);
>  
>  	list_for_each_entry(buf, &tmp_list, queue)
> 

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

Re: [PATCH 3/6] media: staging: rkisp1: initialize buffer lists only on probe

[Helen Koike]

On 10/2/20 3:42 PM, Dafna Hirschfeld wrote:
> The buffer lists of stats and params entities
> are initialized in queue_setup callback with
> 'INIT_LIST_HEAD'. It is enough to initialize
> the lists only upon registration.
> For the stats entity the list is already
> initialize upon registration.
> 
> 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 +---
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 4 ----
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index cb9f3332a9a0..aa758f8c2264 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1420,8 +1420,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
>  					 unsigned int sizes[],
>  					 struct device *alloc_devs[])
>  {
> -	struct rkisp1_params *params = vq->drv_priv;
> -
>  	*num_buffers = clamp_t(u32, *num_buffers,
>  			       RKISP1_ISP_PARAMS_REQ_BUFS_MIN,
>  			       RKISP1_ISP_PARAMS_REQ_BUFS_MAX);
> @@ -1430,7 +1428,6 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
>  
>  	sizes[0] = sizeof(struct rkisp1_params_cfg);
>  
> -	INIT_LIST_HEAD(&params->params);
>  	return 0;
>  }
>  
> @@ -1545,6 +1542,7 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  
>  	params->rkisp1 = rkisp1;
>  	mutex_init(&node->vlock);
> +	INIT_LIST_HEAD(&params->params);
>  	spin_lock_init(&params->config_lock);
>  
>  	strscpy(vdev->name, RKISP1_PARAMS_DEV_NAME, sizeof(vdev->name));
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 51c64f75fe29..c58ae52b8a98 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -94,8 +94,6 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
>  					unsigned int sizes[],
>  					struct device *alloc_devs[])
>  {
> -	struct rkisp1_stats *stats = vq->drv_priv;
> -
>  	*num_planes = 1;
>  
>  	*num_buffers = clamp_t(u32, *num_buffers, RKISP1_ISP_STATS_REQ_BUFS_MIN,
> @@ -103,8 +101,6 @@ static int rkisp1_stats_vb2_queue_setup(struct vb2_queue *vq,
>  
>  	sizes[0] = sizeof(struct rkisp1_stat_buffer);
>  
> -	INIT_LIST_HEAD(&stats->stat);
> -
>  	return 0;
>  }
>  
> 

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

Re: [PATCH 4/6] media: staging: rkisp1: remove the 'is_streaming' field from stats and params

[Helen Koike]

Hi Dafna,

On 10/2/20 3:42 PM, Dafna Hirschfeld wrote:
> The params and stats entities have a field 'is_streaming'.
> This field is not needed since the entities can have available
> buffers only if they stream and therefore it is enough to
> check if there are buffers available.
> As a result, their start_stream callbacks can be removed since
> they only set the 'is_streaming' field.

I would just clarify this a bit, since it is possible to have
is_streaming = true, but without any buffers queued, but
if we have any buffers queued, then we are sure is_streaming = true,
and this is what functions, executed after the testins this bool, check.

> 
> 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 | 12 ++++-------
>  drivers/staging/media/rkisp1/rkisp1-params.c | 21 +-------------------
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 17 ----------------
>  3 files changed, 5 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 45abacdbb664..692333c66f9d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -240,19 +240,17 @@ struct rkisp1_capture {
>   *
>   * @vnode:	  video node
>   * @rkisp1:	  pointer to the rkisp1 device
> - * @lock:	  locks the buffer list 'stat' and 'is_streaming'
> + * @lock:	  locks the buffer list 'stat'
>   * @stat:	  queue of rkisp1_buffer
>   * @vdev_fmt:	  v4l2_format of the metadata format
> - * @is_streaming: device is streaming
>   */
>  struct rkisp1_stats {
>  	struct rkisp1_vdev_node vnode;
>  	struct rkisp1_device *rkisp1;
>  
> -	spinlock_t lock; /* locks the buffers list 'stats' and 'is_streaming' */
> +	spinlock_t lock; /* locks the buffers list 'stats' */
>  	struct list_head stat;
>  	struct v4l2_format vdev_fmt;
> -	bool is_streaming;
>  };
>  
>  /*
> @@ -260,10 +258,9 @@ struct rkisp1_stats {
>   *
>   * @vnode:		video node
>   * @rkisp1:		pointer to the rkisp1 device
> - * @config_lock:	locks the buffer list 'params' and 'is_streaming'
> + * @config_lock:	locks the buffer list 'params'
>   * @params:		queue of rkisp1_buffer
>   * @vdev_fmt:		v4l2_format of the metadata format
> - * @is_streaming:	device is streaming
>   * @quantization:	the quantization configured on the isp's src pad
>   * @raw_type:		the bayer pattern on the isp video sink pad
>   */
> @@ -271,10 +268,9 @@ struct rkisp1_params {
>  	struct rkisp1_vdev_node vnode;
>  	struct rkisp1_device *rkisp1;
>  
> -	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
> +	spinlock_t config_lock; /* locks the buffers list 'params' */
>  	struct list_head params;
>  	struct v4l2_format vdev_fmt;
> -	bool is_streaming;
>  
>  	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 aa758f8c2264..3afbc24ca05e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1224,10 +1224,6 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	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);
> @@ -1303,8 +1299,7 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
>  	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);
> +	rkisp1_params_apply_params_cfg(params, 0);
>  
>  	spin_unlock_irq(&params->config_lock);
>  }
> @@ -1467,7 +1462,6 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  	 * without holding the lock
>  	 */
>  	spin_lock_irq(&params->config_lock);
> -	params->is_streaming = false;
>  	list_splice_init(&params->params, &tmp_list);
>  	spin_unlock_irq(&params->config_lock);
>  
> @@ -1475,25 +1469,12 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>  }
>  
> -static int
> -rkisp1_params_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
> -{
> -	struct rkisp1_params *params = queue->drv_priv;
> -
> -	spin_lock_irq(&params->config_lock);
> -	params->is_streaming = true;
> -	spin_unlock_irq(&params->config_lock);
> -
> -	return 0;
> -}
> -
>  static struct vb2_ops rkisp1_params_vb2_ops = {
>  	.queue_setup = rkisp1_params_vb2_queue_setup,
>  	.wait_prepare = vb2_ops_wait_prepare,
>  	.wait_finish = vb2_ops_wait_finish,
>  	.buf_queue = rkisp1_params_vb2_buf_queue,
>  	.buf_prepare = rkisp1_params_vb2_buf_prepare,
> -	.start_streaming = rkisp1_params_vb2_start_streaming,
>  	.stop_streaming = rkisp1_params_vb2_stop_streaming,
>  
>  };
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index c58ae52b8a98..3ddab8fa8f2d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -136,7 +136,6 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  	unsigned int i;
>  
>  	spin_lock_irq(&stats->lock);
> -	stats->is_streaming = false;
>  	for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
>  		if (list_empty(&stats->stat))
>  			break;
> @@ -148,18 +147,6 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
>  	spin_unlock_irq(&stats->lock);
>  }
>  
> -static int
> -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;
> -}
> -
>  static const struct vb2_ops rkisp1_stats_vb2_ops = {
>  	.queue_setup = rkisp1_stats_vb2_queue_setup,
>  	.buf_queue = rkisp1_stats_vb2_buf_queue,
> @@ -167,7 +154,6 @@ static const struct vb2_ops rkisp1_stats_vb2_ops = {
>  	.wait_prepare = vb2_ops_wait_prepare,
>  	.wait_finish = vb2_ops_wait_finish,
>  	.stop_streaming = rkisp1_stats_vb2_stop_streaming,
> -	.start_streaming = rkisp1_stats_vb2_start_streaming,
>  };
>  
>  static int
> @@ -355,12 +341,9 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	if (isp_mis_tmp & RKISP1_STATS_MEAS_MASK)
>  		rkisp1->debug.stats_error++;
>  
> -	if (!stats->is_streaming)
> -		goto unlock;
>  	if (isp_ris & RKISP1_STATS_MEAS_MASK)
>  		rkisp1_stats_send_measurement(stats, isp_ris);
>  
> -unlock:
>  	spin_unlock(&stats->lock);
>  }
>  
> 

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

Re: [PATCH 5/6] media: staging: rkisp1: params: add '__must_hold' to rkisp1_params_apply_params_cfg

[Helen Koike]

Hi Dafna,

On 10/2/20 3:42 PM, Dafna Hirschfeld wrote:
> The function rkisp1_params_apply_params_cfg must be under the
> the lock of params->config_lock. Add the __must_hold annotation
> to indicate it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 3afbc24ca05e..aa18a113245b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1187,6 +1187,7 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  
>  static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
>  					   unsigned int frame_sequence)
> +	__must_hold(&params->config_lock)
>  {
>  	struct rkisp1_params_cfg *new_params;
>  	struct rkisp1_buffer *cur_buf = NULL;
> 

Instead of adding __must_hold, why no to lock inside this function?
It seems there are only two places that call it, and they don't do anything else
in the critical section.

Regards,
Helen

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