[PATCH v2 0/4] media: rkisp1: Misc fixes and debug

[PATCH v2 0/4] media: rkisp1: Misc fixes and debug

[Paul Elder]

This series adds a small cleanup of the register definitions, as well as
some additions to the debug for the rkisp1 driver.

Paul Elder (4):
  media: rkisp1: regs: Consolidate MI interrupt wrap fields
  media: rkisp1: debug: Add register dump for IS
  media: rkisp1: debug: Count completed frame interrupts
  media: rkisp1: debug: Consolidate counter debugfs files

 .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  5 ++
 .../platform/rockchip/rkisp1/rkisp1-debug.c   | 71 +++++++++++++------
 .../platform/rockchip/rkisp1/rkisp1-isp.c     |  2 +
 .../platform/rockchip/rkisp1/rkisp1-regs.h    |  9 +--
 5 files changed, 60 insertions(+), 29 deletions(-)

-- 
2.39.2


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

[PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields

[Paul Elder]

Consolidate the wraparound fields in the memory interface interrupt
status registers, so that it can be more succinctly expressed by taking
the stream ID (main or self) as a parameter.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
index 350f452e676f..bea69a0d766a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
@@ -172,12 +172,9 @@
 #define RKISP1_CIF_MI_FRAME(stream)			BIT((stream)->id)
 #define RKISP1_CIF_MI_MBLK_LINE				BIT(2)
 #define RKISP1_CIF_MI_FILL_MP_Y				BIT(3)
-#define RKISP1_CIF_MI_WRAP_MP_Y				BIT(4)
-#define RKISP1_CIF_MI_WRAP_MP_CB			BIT(5)
-#define RKISP1_CIF_MI_WRAP_MP_CR			BIT(6)
-#define RKISP1_CIF_MI_WRAP_SP_Y				BIT(7)
-#define RKISP1_CIF_MI_WRAP_SP_CB			BIT(8)
-#define RKISP1_CIF_MI_WRAP_SP_CR			BIT(9)
+#define RKISP1_CIF_MI_WRAP_Y(stream)			BIT(4 + (stream)->id * 3)
+#define RKISP1_CIF_MI_WRAP_CB(stream)			BIT(5 + (stream)->id * 3)
+#define RKISP1_CIF_MI_WRAP_CR(stream)			BIT(6 + (stream)->id * 3)
 #define RKISP1_CIF_MI_DMA_READY				BIT(11)
 
 /* MI_STATUS */
-- 
2.39.2


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

[PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS

[Paul Elder]

Add register dump for the ISP image stabilizer module to debugfs. This
helps debugging issues related to digital zoom.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v2:
- Move from IS debugfs file into the ISP debugfs file

 drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
index 71df3dc95e6f..d2fbed42164e 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
@@ -92,6 +92,10 @@ static int rkisp1_debug_dump_isp_regs_show(struct seq_file *m, void *p)
 		RKISP1_DEBUG_REG(ISP_FLAGS_SHD),
 		RKISP1_DEBUG_REG(ISP_RIS),
 		RKISP1_DEBUG_REG(ISP_ERR),
+		RKISP1_DEBUG_SHD_REG(ISP_IS_H_OFFS),
+		RKISP1_DEBUG_SHD_REG(ISP_IS_V_OFFS),
+		RKISP1_DEBUG_SHD_REG(ISP_IS_H_SIZE),
+		RKISP1_DEBUG_SHD_REG(ISP_IS_V_SIZE),
 		{ /* Sentinel */ },
 	};
 	struct rkisp1_device *rkisp1 = m->private;
-- 
2.39.2


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

[PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts

[Paul Elder]

Add a counter to debugfs to count the number of frame-end interrupts.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 1 +
 drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c  | 2 ++
 drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c    | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 1e7cea1bea5e..be69173958a4 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -417,6 +417,7 @@ struct rkisp1_debug {
 	unsigned long stats_error;
 	unsigned long stop_timeout[2];
 	unsigned long frame_drop[2];
+	unsigned long complete_frames;
 };
 
 /*
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
index d2fbed42164e..79cda589d935 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
@@ -221,6 +221,8 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 			     &debug->frame_drop[RKISP1_MAINPATH]);
 	debugfs_create_ulong("sp_frame_drop", 0444, debug->debugfs_dir,
 			     &debug->frame_drop[RKISP1_SELFPATH]);
+	debugfs_create_ulong("complete_frames", 0444, debug->debugfs_dir,
+			     &debug->complete_frames);
 	debugfs_create_file("input_status", 0444, debug->debugfs_dir, rkisp1,
 			    &rkisp1_debug_input_status_fops);
 
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 88ca8b2283b7..bd3acd926410 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -989,6 +989,8 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
 	if (status & RKISP1_CIF_ISP_FRAME) {
 		u32 isp_ris;
 
+		rkisp1->debug.complete_frames++;
+
 		/* New frame from the sensor received */
 		isp_ris = rkisp1_read(rkisp1, RKISP1_CIF_ISP_RIS);
 		if (isp_ris & RKISP1_STATS_MEAS_MASK)
-- 
2.39.2


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

[PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files

[Paul Elder]

Consolidate all the debugfs files that were each a single counter into a
single "counters" file.

While at it, reset the counters at stream on time to make it easier for
to interpret the values in userspace.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v2

 .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 ++
 .../platform/rockchip/rkisp1/rkisp1-debug.c   | 69 ++++++++++++-------
 3 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index c6d7e01c8949..67b2e94dfd67 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -1030,6 +1030,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	struct media_entity *entity = &cap->vnode.vdev.entity;
 	int ret;
 
+	rkisp1_debug_reset_counters(cap->rkisp1);
+
 	mutex_lock(&cap->rkisp1->stream_lock);
 
 	ret = video_device_pipeline_start(&cap->vnode.vdev, &cap->rkisp1->pipe);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index be69173958a4..789259fb304a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -599,9 +599,13 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1);
 void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
+void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1);
 void rkisp1_debug_init(struct rkisp1_device *rkisp1);
 void rkisp1_debug_cleanup(struct rkisp1_device *rkisp1);
 #else
+static inline void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
+{
+}
 static inline void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 {
 }
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
index 79cda589d935..4358ed1367ed 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
@@ -25,6 +25,11 @@ struct rkisp1_debug_register {
 	const char * const name;
 };
 
+struct rkisp1_debug_counter {
+	const char * const name;
+	unsigned long *value;
+};
+
 #define RKISP1_DEBUG_REG(name)		{ RKISP1_CIF_##name, 0, #name }
 #define RKISP1_DEBUG_SHD_REG(name) { \
 	RKISP1_CIF_##name, RKISP1_CIF_##name##_SHD, #name \
@@ -191,6 +196,43 @@ static int rkisp1_debug_input_status_show(struct seq_file *m, void *p)
 }
 DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_input_status);
 
+static int rkisp1_debug_counters_show(struct seq_file *m, void *p)
+{
+	struct rkisp1_device *rkisp1 = m->private;
+	struct rkisp1_debug *debug = &rkisp1->debug;
+
+	const struct rkisp1_debug_counter counters[] = {
+		{ "data_loss", &debug->data_loss },
+		{ "outform_size_err", &debug->outform_size_error },
+		{ "img_stabilization_size_error", &debug->img_stabilization_size_error },
+		{ "inform_size_error", &debug->inform_size_error },
+		{ "irq_delay", &debug->irq_delay },
+		{ "mipi_error", &debug->mipi_error },
+		{ "stats_error", &debug->stats_error },
+		{ "mp_stop_timeout", &debug->stop_timeout[RKISP1_MAINPATH] },
+		{ "sp_stop_timeout", &debug->stop_timeout[RKISP1_SELFPATH] },
+		{ "mp_frame_drop", &debug->frame_drop[RKISP1_MAINPATH] },
+		{ "sp_frame_drop", &debug->frame_drop[RKISP1_SELFPATH] },
+		{ "complete_frames", &debug->complete_frames },
+		{ /* Sentinel */ },
+	};
+
+	const struct rkisp1_debug_counter *counter = counters;
+
+	for (; counter->name; ++counter)
+		seq_printf(m, "%s: %lu\n", counter->name, *counter->value);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_counters);
+
+void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
+{
+	struct dentry *debugfs_dir = rkisp1->debug.debugfs_dir;
+	memset(&rkisp1->debug, 0, sizeof(rkisp1->debug));
+	rkisp1->debug.debugfs_dir = debugfs_dir;
+}
+
 void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 {
 	struct rkisp1_debug *debug = &rkisp1->debug;
@@ -198,31 +240,8 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 
 	debug->debugfs_dir = debugfs_create_dir(dev_name(rkisp1->dev), NULL);
 
-	debugfs_create_ulong("data_loss", 0444, debug->debugfs_dir,
-			     &debug->data_loss);
-	debugfs_create_ulong("outform_size_err", 0444,  debug->debugfs_dir,
-			     &debug->outform_size_error);
-	debugfs_create_ulong("img_stabilization_size_error", 0444,
-			     debug->debugfs_dir,
-			     &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,
-			     &debug->stats_error);
-	debugfs_create_ulong("mp_stop_timeout", 0444, debug->debugfs_dir,
-			     &debug->stop_timeout[RKISP1_MAINPATH]);
-	debugfs_create_ulong("sp_stop_timeout", 0444, debug->debugfs_dir,
-			     &debug->stop_timeout[RKISP1_SELFPATH]);
-	debugfs_create_ulong("mp_frame_drop", 0444, debug->debugfs_dir,
-			     &debug->frame_drop[RKISP1_MAINPATH]);
-	debugfs_create_ulong("sp_frame_drop", 0444, debug->debugfs_dir,
-			     &debug->frame_drop[RKISP1_SELFPATH]);
-	debugfs_create_ulong("complete_frames", 0444, debug->debugfs_dir,
-			     &debug->complete_frames);
+	debugfs_create_file("counters", 0444, debug->debugfs_dir, rkisp1,
+			    &rkisp1_debug_counters_fops);
 	debugfs_create_file("input_status", 0444, debug->debugfs_dir, rkisp1,
 			    &rkisp1_debug_input_status_fops);
 
-- 
2.39.2


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

Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files

[Kieran Bingham]

Quoting Paul Elder (2023-12-01 14:04:33)
> Consolidate all the debugfs files that were each a single counter into a
> single "counters" file.
> 
> While at it, reset the counters at stream on time to make it easier for
> to interpret the values in userspace.

That gives a better atomicity here I think so that's good. I guess the
debug struct could have a lock around it sometime - but I don't think
that matters in this context.

Resetting at stream on looks to make sense so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> New in v2
> 
>  .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 ++
>  .../platform/rockchip/rkisp1/rkisp1-debug.c   | 69 ++++++++++++-------
>  3 files changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index c6d7e01c8949..67b2e94dfd67 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -1030,6 +1030,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>         struct media_entity *entity = &cap->vnode.vdev.entity;
>         int ret;
>  
> +       rkisp1_debug_reset_counters(cap->rkisp1);
> +
>         mutex_lock(&cap->rkisp1->stream_lock);
>  
>         ret = video_device_pipeline_start(&cap->vnode.vdev, &cap->rkisp1->pipe);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index be69173958a4..789259fb304a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -599,9 +599,13 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1);
>  void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1);
>  void rkisp1_debug_init(struct rkisp1_device *rkisp1);
>  void rkisp1_debug_cleanup(struct rkisp1_device *rkisp1);
>  #else
> +static inline void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> +{
> +}
>  static inline void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  {
>  }
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> index 79cda589d935..4358ed1367ed 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> @@ -25,6 +25,11 @@ struct rkisp1_debug_register {
>         const char * const name;
>  };
>  
> +struct rkisp1_debug_counter {
> +       const char * const name;
> +       unsigned long *value;
> +};
> +
>  #define RKISP1_DEBUG_REG(name)         { RKISP1_CIF_##name, 0, #name }
>  #define RKISP1_DEBUG_SHD_REG(name) { \
>         RKISP1_CIF_##name, RKISP1_CIF_##name##_SHD, #name \
> @@ -191,6 +196,43 @@ static int rkisp1_debug_input_status_show(struct seq_file *m, void *p)
>  }
>  DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_input_status);
>  
> +static int rkisp1_debug_counters_show(struct seq_file *m, void *p)
> +{
> +       struct rkisp1_device *rkisp1 = m->private;
> +       struct rkisp1_debug *debug = &rkisp1->debug;
> +
> +       const struct rkisp1_debug_counter counters[] = {
> +               { "data_loss", &debug->data_loss },
> +               { "outform_size_err", &debug->outform_size_error },
> +               { "img_stabilization_size_error", &debug->img_stabilization_size_error },
> +               { "inform_size_error", &debug->inform_size_error },
> +               { "irq_delay", &debug->irq_delay },
> +               { "mipi_error", &debug->mipi_error },
> +               { "stats_error", &debug->stats_error },
> +               { "mp_stop_timeout", &debug->stop_timeout[RKISP1_MAINPATH] },
> +               { "sp_stop_timeout", &debug->stop_timeout[RKISP1_SELFPATH] },
> +               { "mp_frame_drop", &debug->frame_drop[RKISP1_MAINPATH] },
> +               { "sp_frame_drop", &debug->frame_drop[RKISP1_SELFPATH] },
> +               { "complete_frames", &debug->complete_frames },
> +               { /* Sentinel */ },
> +       };
> +
> +       const struct rkisp1_debug_counter *counter = counters;
> +
> +       for (; counter->name; ++counter)
> +               seq_printf(m, "%s: %lu\n", counter->name, *counter->value);
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_counters);
> +
> +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> +{
> +       struct dentry *debugfs_dir = rkisp1->debug.debugfs_dir;
> +       memset(&rkisp1->debug, 0, sizeof(rkisp1->debug));
> +       rkisp1->debug.debugfs_dir = debugfs_dir;
> +}
> +
>  void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  {
>         struct rkisp1_debug *debug = &rkisp1->debug;
> @@ -198,31 +240,8 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  
>         debug->debugfs_dir = debugfs_create_dir(dev_name(rkisp1->dev), NULL);
>  
> -       debugfs_create_ulong("data_loss", 0444, debug->debugfs_dir,
> -                            &debug->data_loss);
> -       debugfs_create_ulong("outform_size_err", 0444,  debug->debugfs_dir,
> -                            &debug->outform_size_error);
> -       debugfs_create_ulong("img_stabilization_size_error", 0444,
> -                            debug->debugfs_dir,
> -                            &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,
> -                            &debug->stats_error);
> -       debugfs_create_ulong("mp_stop_timeout", 0444, debug->debugfs_dir,
> -                            &debug->stop_timeout[RKISP1_MAINPATH]);
> -       debugfs_create_ulong("sp_stop_timeout", 0444, debug->debugfs_dir,
> -                            &debug->stop_timeout[RKISP1_SELFPATH]);
> -       debugfs_create_ulong("mp_frame_drop", 0444, debug->debugfs_dir,
> -                            &debug->frame_drop[RKISP1_MAINPATH]);
> -       debugfs_create_ulong("sp_frame_drop", 0444, debug->debugfs_dir,
> -                            &debug->frame_drop[RKISP1_SELFPATH]);
> -       debugfs_create_ulong("complete_frames", 0444, debug->debugfs_dir,
> -                            &debug->complete_frames);
> +       debugfs_create_file("counters", 0444, debug->debugfs_dir, rkisp1,
> +                           &rkisp1_debug_counters_fops);
>         debugfs_create_file("input_status", 0444, debug->debugfs_dir, rkisp1,
>                             &rkisp1_debug_input_status_fops);
>  
> -- 
> 2.39.2
>

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

Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files

[Tomi Valkeinen]

On 01/12/2023 16:04, Paul Elder wrote:
> Consolidate all the debugfs files that were each a single counter into a
> single "counters" file.
> 
> While at it, reset the counters at stream on time to make it easier for
> to interpret the values in userspace.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> New in v2
> 
>   .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
>   .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 ++
>   .../platform/rockchip/rkisp1/rkisp1-debug.c   | 69 ++++++++++++-------
>   3 files changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index c6d7e01c8949..67b2e94dfd67 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -1030,6 +1030,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>   	struct media_entity *entity = &cap->vnode.vdev.entity;
>   	int ret;
>   
> +	rkisp1_debug_reset_counters(cap->rkisp1);
> +
>   	mutex_lock(&cap->rkisp1->stream_lock);
>   
>   	ret = video_device_pipeline_start(&cap->vnode.vdev, &cap->rkisp1->pipe);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index be69173958a4..789259fb304a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -599,9 +599,13 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1);
>   void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
>   
>   #if IS_ENABLED(CONFIG_DEBUG_FS)
> +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1);
>   void rkisp1_debug_init(struct rkisp1_device *rkisp1);
>   void rkisp1_debug_cleanup(struct rkisp1_device *rkisp1);
>   #else
> +static inline void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> +{
> +}
>   static inline void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>   {
>   }
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> index 79cda589d935..4358ed1367ed 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> @@ -25,6 +25,11 @@ struct rkisp1_debug_register {
>   	const char * const name;
>   };
>   
> +struct rkisp1_debug_counter {
> +	const char * const name;
> +	unsigned long *value;
> +};
> +
>   #define RKISP1_DEBUG_REG(name)		{ RKISP1_CIF_##name, 0, #name }
>   #define RKISP1_DEBUG_SHD_REG(name) { \
>   	RKISP1_CIF_##name, RKISP1_CIF_##name##_SHD, #name \
> @@ -191,6 +196,43 @@ static int rkisp1_debug_input_status_show(struct seq_file *m, void *p)
>   }
>   DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_input_status);
>   
> +static int rkisp1_debug_counters_show(struct seq_file *m, void *p)
> +{
> +	struct rkisp1_device *rkisp1 = m->private;
> +	struct rkisp1_debug *debug = &rkisp1->debug;
> +
> +	const struct rkisp1_debug_counter counters[] = {
> +		{ "data_loss", &debug->data_loss },
> +		{ "outform_size_err", &debug->outform_size_error },
> +		{ "img_stabilization_size_error", &debug->img_stabilization_size_error },
> +		{ "inform_size_error", &debug->inform_size_error },
> +		{ "irq_delay", &debug->irq_delay },
> +		{ "mipi_error", &debug->mipi_error },
> +		{ "stats_error", &debug->stats_error },
> +		{ "mp_stop_timeout", &debug->stop_timeout[RKISP1_MAINPATH] },
> +		{ "sp_stop_timeout", &debug->stop_timeout[RKISP1_SELFPATH] },
> +		{ "mp_frame_drop", &debug->frame_drop[RKISP1_MAINPATH] },
> +		{ "sp_frame_drop", &debug->frame_drop[RKISP1_SELFPATH] },
> +		{ "complete_frames", &debug->complete_frames },
> +		{ /* Sentinel */ },
> +	};
> +
> +	const struct rkisp1_debug_counter *counter = counters;
> +
> +	for (; counter->name; ++counter)
> +		seq_printf(m, "%s: %lu\n", counter->name, *counter->value);
> +

You could also do:

	const struct {
		const char *name;
		unsigned long value;
	} counters[] = {
		{ "data_loss", debug->data_loss },
		{ "outform_size_err", debug->outform_size_error },
		{ "img_stabilization_size_error", debug->img_stabilization_size_error },
		{ "inform_size_error", debug->inform_size_error },
		{ "irq_delay", debug->irq_delay },
		{ "mipi_error", debug->mipi_error },
		{ "stats_error", debug->stats_error },
		{ "mp_stop_timeout", debug->stop_timeout[RKISP1_MAINPATH] },
		{ "sp_stop_timeout", debug->stop_timeout[RKISP1_SELFPATH] },
		{ "mp_frame_drop", debug->frame_drop[RKISP1_MAINPATH] },
		{ "sp_frame_drop", debug->frame_drop[RKISP1_SELFPATH] },
		{ "complete_frames", debug->complete_frames },
	};

	for (unsigned int i = 0; i < ARRAY_SIZE(counters); ++i)
		seq_printf(m, "%s: %lu\n", counters[i].name, counters[i].value);

Not a big difference, but this doesn't "leak" the struct used only inside the
function, doesn't need the sentinel, and I don't see a reason to store pointers
to values, instead of just storing the values.

  Tomi

> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_counters);
> +
> +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> +{
> +	struct dentry *debugfs_dir = rkisp1->debug.debugfs_dir;
> +	memset(&rkisp1->debug, 0, sizeof(rkisp1->debug));
> +	rkisp1->debug.debugfs_dir = debugfs_dir;
> +}
> +
>   void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>   {
>   	struct rkisp1_debug *debug = &rkisp1->debug;
> @@ -198,31 +240,8 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>   
>   	debug->debugfs_dir = debugfs_create_dir(dev_name(rkisp1->dev), NULL);
>   
> -	debugfs_create_ulong("data_loss", 0444, debug->debugfs_dir,
> -			     &debug->data_loss);
> -	debugfs_create_ulong("outform_size_err", 0444,  debug->debugfs_dir,
> -			     &debug->outform_size_error);
> -	debugfs_create_ulong("img_stabilization_size_error", 0444,
> -			     debug->debugfs_dir,
> -			     &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,
> -			     &debug->stats_error);
> -	debugfs_create_ulong("mp_stop_timeout", 0444, debug->debugfs_dir,
> -			     &debug->stop_timeout[RKISP1_MAINPATH]);
> -	debugfs_create_ulong("sp_stop_timeout", 0444, debug->debugfs_dir,
> -			     &debug->stop_timeout[RKISP1_SELFPATH]);
> -	debugfs_create_ulong("mp_frame_drop", 0444, debug->debugfs_dir,
> -			     &debug->frame_drop[RKISP1_MAINPATH]);
> -	debugfs_create_ulong("sp_frame_drop", 0444, debug->debugfs_dir,
> -			     &debug->frame_drop[RKISP1_SELFPATH]);
> -	debugfs_create_ulong("complete_frames", 0444, debug->debugfs_dir,
> -			     &debug->complete_frames);
> +	debugfs_create_file("counters", 0444, debug->debugfs_dir, rkisp1,
> +			    &rkisp1_debug_counters_fops);
>   	debugfs_create_file("input_status", 0444, debug->debugfs_dir, rkisp1,
>   			    &rkisp1_debug_input_status_fops);
>   


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

Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files

[Laurent Pinchart]

On Tue, Dec 05, 2023 at 10:22:47AM +0200, Tomi Valkeinen wrote:
> On 01/12/2023 16:04, Paul Elder wrote:
> > Consolidate all the debugfs files that were each a single counter into a
> > single "counters" file.
> > 
> > While at it, reset the counters at stream on time to make it easier for
> > to interpret the values in userspace.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > New in v2
> > 
> >   .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
> >   .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 ++
> >   .../platform/rockchip/rkisp1/rkisp1-debug.c   | 69 ++++++++++++-------
> >   3 files changed, 50 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index c6d7e01c8949..67b2e94dfd67 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -1030,6 +1030,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
> >   	struct media_entity *entity = &cap->vnode.vdev.entity;
> >   	int ret;
> >   
> > +	rkisp1_debug_reset_counters(cap->rkisp1);
> > +
> >   	mutex_lock(&cap->rkisp1->stream_lock);
> >   
> >   	ret = video_device_pipeline_start(&cap->vnode.vdev, &cap->rkisp1->pipe);
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index be69173958a4..789259fb304a 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -599,9 +599,13 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1);
> >   void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
> >   
> >   #if IS_ENABLED(CONFIG_DEBUG_FS)
> > +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1);
> >   void rkisp1_debug_init(struct rkisp1_device *rkisp1);
> >   void rkisp1_debug_cleanup(struct rkisp1_device *rkisp1);
> >   #else
> > +static inline void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> > +{
> > +}
> >   static inline void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >   {
> >   }
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> > index 79cda589d935..4358ed1367ed 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c
> > @@ -25,6 +25,11 @@ struct rkisp1_debug_register {
> >   	const char * const name;
> >   };
> >   
> > +struct rkisp1_debug_counter {
> > +	const char * const name;
> > +	unsigned long *value;
> > +};
> > +
> >   #define RKISP1_DEBUG_REG(name)		{ RKISP1_CIF_##name, 0, #name }
> >   #define RKISP1_DEBUG_SHD_REG(name) { \
> >   	RKISP1_CIF_##name, RKISP1_CIF_##name##_SHD, #name \
> > @@ -191,6 +196,43 @@ static int rkisp1_debug_input_status_show(struct seq_file *m, void *p)
> >   }
> >   DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_input_status);
> >   
> > +static int rkisp1_debug_counters_show(struct seq_file *m, void *p)
> > +{
> > +	struct rkisp1_device *rkisp1 = m->private;
> > +	struct rkisp1_debug *debug = &rkisp1->debug;
> > +
> > +	const struct rkisp1_debug_counter counters[] = {
> > +		{ "data_loss", &debug->data_loss },
> > +		{ "outform_size_err", &debug->outform_size_error },
> > +		{ "img_stabilization_size_error", &debug->img_stabilization_size_error },
> > +		{ "inform_size_error", &debug->inform_size_error },
> > +		{ "irq_delay", &debug->irq_delay },
> > +		{ "mipi_error", &debug->mipi_error },
> > +		{ "stats_error", &debug->stats_error },
> > +		{ "mp_stop_timeout", &debug->stop_timeout[RKISP1_MAINPATH] },
> > +		{ "sp_stop_timeout", &debug->stop_timeout[RKISP1_SELFPATH] },
> > +		{ "mp_frame_drop", &debug->frame_drop[RKISP1_MAINPATH] },
> > +		{ "sp_frame_drop", &debug->frame_drop[RKISP1_SELFPATH] },
> > +		{ "complete_frames", &debug->complete_frames },
> > +		{ /* Sentinel */ },
> > +	};
> > +
> > +	const struct rkisp1_debug_counter *counter = counters;
> > +
> > +	for (; counter->name; ++counter)
> > +		seq_printf(m, "%s: %lu\n", counter->name, *counter->value);
> > +
> 
> You could also do:
> 
> 	const struct {
> 		const char *name;
> 		unsigned long value;
> 	} counters[] = {
> 		{ "data_loss", debug->data_loss },
> 		{ "outform_size_err", debug->outform_size_error },
> 		{ "img_stabilization_size_error", debug->img_stabilization_size_error },
> 		{ "inform_size_error", debug->inform_size_error },
> 		{ "irq_delay", debug->irq_delay },
> 		{ "mipi_error", debug->mipi_error },
> 		{ "stats_error", debug->stats_error },
> 		{ "mp_stop_timeout", debug->stop_timeout[RKISP1_MAINPATH] },
> 		{ "sp_stop_timeout", debug->stop_timeout[RKISP1_SELFPATH] },
> 		{ "mp_frame_drop", debug->frame_drop[RKISP1_MAINPATH] },
> 		{ "sp_frame_drop", debug->frame_drop[RKISP1_SELFPATH] },
> 		{ "complete_frames", debug->complete_frames },
> 	};
> 
> 	for (unsigned int i = 0; i < ARRAY_SIZE(counters); ++i)
> 		seq_printf(m, "%s: %lu\n", counters[i].name, counters[i].value);
> 
> Not a big difference, but this doesn't "leak" the struct used only inside the
> function, doesn't need the sentinel, and I don't see a reason to store pointers
> to values, instead of just storing the values.

Wouldn't it be nicer to avoid constructing this on the stack for every
call ? We could store the counters in an array:

enum rkisp1_debug_counter {
	RKISP1_DBG_COUNT_DATA_LOSS,
	...
	RKISP1_DBG_COUNT_FRAME_DROP_MP,
	RKISP1_DBG_COUNT_FRAME_DROP_SP,
	RKISP1_DBG_COUNT_MAX,
};

struct rksisp1_debug {
	struct dentry *debugfs_dir;
	unsigned long counters[RKISP1_DBG_COUNT_MAX];
};

The counters[] array above would become a

static const char * const counter_names[] = {
	[RKISP1_DBG_COUNT_DATA_LOSS] = "data_loss",
	...
};

> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_counters);
> > +
> > +void rkisp1_debug_reset_counters(struct rkisp1_device *rkisp1)
> > +{
> > +	struct dentry *debugfs_dir = rkisp1->debug.debugfs_dir;
> > +	memset(&rkisp1->debug, 0, sizeof(rkisp1->debug));
> > +	rkisp1->debug.debugfs_dir = debugfs_dir;
> > +}
> > +
> >   void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >   {
> >   	struct rkisp1_debug *debug = &rkisp1->debug;
> > @@ -198,31 +240,8 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >   
> >   	debug->debugfs_dir = debugfs_create_dir(dev_name(rkisp1->dev), NULL);
> >   
> > -	debugfs_create_ulong("data_loss", 0444, debug->debugfs_dir,
> > -			     &debug->data_loss);
> > -	debugfs_create_ulong("outform_size_err", 0444,  debug->debugfs_dir,
> > -			     &debug->outform_size_error);
> > -	debugfs_create_ulong("img_stabilization_size_error", 0444,
> > -			     debug->debugfs_dir,
> > -			     &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,
> > -			     &debug->stats_error);
> > -	debugfs_create_ulong("mp_stop_timeout", 0444, debug->debugfs_dir,
> > -			     &debug->stop_timeout[RKISP1_MAINPATH]);
> > -	debugfs_create_ulong("sp_stop_timeout", 0444, debug->debugfs_dir,
> > -			     &debug->stop_timeout[RKISP1_SELFPATH]);
> > -	debugfs_create_ulong("mp_frame_drop", 0444, debug->debugfs_dir,
> > -			     &debug->frame_drop[RKISP1_MAINPATH]);
> > -	debugfs_create_ulong("sp_frame_drop", 0444, debug->debugfs_dir,
> > -			     &debug->frame_drop[RKISP1_SELFPATH]);
> > -	debugfs_create_ulong("complete_frames", 0444, debug->debugfs_dir,
> > -			     &debug->complete_frames);
> > +	debugfs_create_file("counters", 0444, debug->debugfs_dir, rkisp1,
> > +			    &rkisp1_debug_counters_fops);
> >   	debugfs_create_file("input_status", 0444, debug->debugfs_dir, rkisp1,
> >   			    &rkisp1_debug_input_status_fops);
> >   

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 0/4] media: rkisp1: Misc fixes and debug

[Tomi Valkeinen]

On 01/12/2023 16:04, Paul Elder wrote:
> This series adds a small cleanup of the register definitions, as well as
> some additions to the debug for the rkisp1 driver.
> 
> Paul Elder (4):
>    media: rkisp1: regs: Consolidate MI interrupt wrap fields
>    media: rkisp1: debug: Add register dump for IS
>    media: rkisp1: debug: Count completed frame interrupts
>    media: rkisp1: debug: Consolidate counter debugfs files
> 
>   .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
>   .../platform/rockchip/rkisp1/rkisp1-common.h  |  5 ++
>   .../platform/rockchip/rkisp1/rkisp1-debug.c   | 71 +++++++++++++------
>   .../platform/rockchip/rkisp1/rkisp1-isp.c     |  2 +
>   .../platform/rockchip/rkisp1/rkisp1-regs.h    |  9 +--
>   5 files changed, 60 insertions(+), 29 deletions(-)
> 

For the series, with or without the change I proposed for patch 4:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi


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

Re: [PATCH v2 0/4] media: rkisp1: Misc fixes and debug

[Laurent Pinchart]

Hi Paul,

Thank you for the patches.

On Fri, Dec 01, 2023 at 11:04:29PM +0900, Paul Elder wrote:
> This series adds a small cleanup of the register definitions, as well as
> some additions to the debug for the rkisp1 driver.
> 
> Paul Elder (4):
>   media: rkisp1: regs: Consolidate MI interrupt wrap fields
>   media: rkisp1: debug: Add register dump for IS
>   media: rkisp1: debug: Count completed frame interrupts
>   media: rkisp1: debug: Consolidate counter debugfs files

I'll take patch 1/4 to 3/4 in my tree already, while the discussion on
4/4 continues. Please let me know if you would rather have me wait.

>  .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  5 ++
>  .../platform/rockchip/rkisp1/rkisp1-debug.c   | 71 +++++++++++++------
>  .../platform/rockchip/rkisp1/rkisp1-isp.c     |  2 +
>  .../platform/rockchip/rkisp1/rkisp1-regs.h    |  9 +--
>  5 files changed, 60 insertions(+), 29 deletions(-)

-- 
Regards,

Laurent Pinchart

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