public inbox for linux-rockchip@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] media: rkisp1: Misc fixes and debug
@ 2023-12-01 14:04 Paul Elder
  2023-12-01 14:04 ` [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields Paul Elder
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Paul Elder @ 2023-12-01 14:04 UTC (permalink / raw)
  To: linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, tomi.valkeinen, umang.jain,
	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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
@ 2023-12-01 14:04 ` Paul Elder
  2023-12-01 14:04 ` [PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS Paul Elder
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Paul Elder @ 2023-12-01 14:04 UTC (permalink / raw)
  To: linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, tomi.valkeinen, umang.jain,
	Paul Elder, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, moderated list:ARM/Rockchip SoC support,
	open list

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
  2023-12-01 14:04 ` [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields Paul Elder
@ 2023-12-01 14:04 ` Paul Elder
  2023-12-01 14:04 ` [PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts Paul Elder
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Paul Elder @ 2023-12-01 14:04 UTC (permalink / raw)
  To: linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, tomi.valkeinen, umang.jain,
	Paul Elder, Alexander Stein, Dafna Hirschfeld,
	Mauro Carvalho Chehab, Heiko Stuebner,
	moderated list:ARM/Rockchip SoC support, open list

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
  2023-12-01 14:04 ` [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields Paul Elder
  2023-12-01 14:04 ` [PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS Paul Elder
@ 2023-12-01 14:04 ` Paul Elder
  2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Paul Elder @ 2023-12-01 14:04 UTC (permalink / raw)
  To: linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, tomi.valkeinen, umang.jain,
	Paul Elder, Alexander Stein, Dafna Hirschfeld,
	Mauro Carvalho Chehab, Heiko Stuebner,
	moderated list:ARM/Rockchip SoC support, open list

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
                   ` (2 preceding siblings ...)
  2023-12-01 14:04 ` [PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts Paul Elder
@ 2023-12-01 14:04 ` Paul Elder
  2023-12-01 15:30   ` Kieran Bingham
  2023-12-05  8:22   ` Tomi Valkeinen
  2023-12-05  8:23 ` [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Tomi Valkeinen
  2023-12-07 14:51 ` Laurent Pinchart
  5 siblings, 2 replies; 10+ messages in thread
From: Paul Elder @ 2023-12-01 14:04 UTC (permalink / raw)
  To: linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, tomi.valkeinen, umang.jain,
	Paul Elder, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, moderated list:ARM/Rockchip SoC support,
	open list

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files
  2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder
@ 2023-12-01 15:30   ` Kieran Bingham
  2023-12-05  8:22   ` Tomi Valkeinen
  1 sibling, 0 replies; 10+ messages in thread
From: Kieran Bingham @ 2023-12-01 15:30 UTC (permalink / raw)
  To: Paul Elder, linux-media, linux-rockchip
  Cc: laurent.pinchart, tomi.valkeinen, umang.jain, Paul Elder,
	Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner,
	linux-arm-kernel, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files
  2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder
  2023-12-01 15:30   ` Kieran Bingham
@ 2023-12-05  8:22   ` Tomi Valkeinen
  2023-12-07 14:50     ` Laurent Pinchart
  1 sibling, 1 reply; 10+ messages in thread
From: Tomi Valkeinen @ 2023-12-05  8:22 UTC (permalink / raw)
  To: Paul Elder, linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, umang.jain, Dafna Hirschfeld,
	Mauro Carvalho Chehab, Heiko Stuebner,
	moderated list:ARM/Rockchip SoC support, open list

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/4] media: rkisp1: Misc fixes and debug
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
                   ` (3 preceding siblings ...)
  2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder
@ 2023-12-05  8:23 ` Tomi Valkeinen
  2023-12-07 14:51 ` Laurent Pinchart
  5 siblings, 0 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2023-12-05  8:23 UTC (permalink / raw)
  To: Paul Elder, linux-media, linux-rockchip
  Cc: laurent.pinchart, kieran.bingham, umang.jain

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files
  2023-12-05  8:22   ` Tomi Valkeinen
@ 2023-12-07 14:50     ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-12-07 14:50 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Paul Elder, linux-media, linux-rockchip, kieran.bingham,
	umang.jain, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, moderated list:ARM/Rockchip SoC support,
	open list

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/4] media: rkisp1: Misc fixes and debug
  2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
                   ` (4 preceding siblings ...)
  2023-12-05  8:23 ` [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Tomi Valkeinen
@ 2023-12-07 14:51 ` Laurent Pinchart
  5 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-12-07 14:51 UTC (permalink / raw)
  To: Paul Elder
  Cc: linux-media, linux-rockchip, kieran.bingham, tomi.valkeinen,
	umang.jain

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-12-07 14:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-01 14:04 [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Paul Elder
2023-12-01 14:04 ` [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields Paul Elder
2023-12-01 14:04 ` [PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS Paul Elder
2023-12-01 14:04 ` [PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts Paul Elder
2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder
2023-12-01 15:30   ` Kieran Bingham
2023-12-05  8:22   ` Tomi Valkeinen
2023-12-07 14:50     ` Laurent Pinchart
2023-12-05  8:23 ` [PATCH v2 0/4] media: rkisp1: Misc fixes and debug Tomi Valkeinen
2023-12-07 14:51 ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox