* [PATCH v2 1/4] media: rkisp1: regs: Consolidate MI interrupt wrap fields [not found] <20231201140433.2126011-1-paul.elder@ideasonboard.com> @ 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 ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/4] media: rkisp1: debug: Add register dump for IS [not found] <20231201140433.2126011-1-paul.elder@ideasonboard.com> 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 2023-12-01 14:04 ` [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files Paul Elder 3 siblings, 0 replies; 7+ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/4] media: rkisp1: debug: Count completed frame interrupts [not found] <20231201140433.2126011-1-paul.elder@ideasonboard.com> 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 3 siblings, 0 replies; 7+ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 4/4] media: rkisp1: debug: Consolidate counter debugfs files [not found] <20231201140433.2126011-1-paul.elder@ideasonboard.com> ` (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 3 siblings, 2 replies; 7+ 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 ^ permalink raw reply [flat|nested] 7+ 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; 7+ 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 > ^ permalink raw reply [flat|nested] 7+ 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; 7+ 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); > ^ permalink raw reply [flat|nested] 7+ 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; 7+ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-12-07 14:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20231201140433.2126011-1-paul.elder@ideasonboard.com>
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®