lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Sep 2021 14:35:04 +0800 From: Jammy Huang <jammy_huang@...eedtech.com> To: Zev Weiss <zweiss@...inix.com> CC: "eajames@...ux.ibm.com" <eajames@...ux.ibm.com>, "mchehab@...nel.org" <mchehab@...nel.org>, "joel@....id.au" <joel@....id.au>, "andrew@...id.au" <andrew@...id.au>, "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, BMC-SW <BMC-SW@...eedtech.com> Subject: Re: [RESEND PATCH] media: aspeed: add debugfs Hi Zev, On 2021/9/29 下午 01:30, Zev Weiss wrote: > On Tue, Sep 28, 2021 at 06:16:53PM PDT, Jammy Huang wrote: >> To show video real-time information as below: >> >> Signal| Resolution| FRC >> | Width Height| >> Lock| 1920 1080| 0 >> >> Frame#| Frame Duration| FPS >> | Now Min Max| >> 496| 26 25 30| 40 >> >> Signed-off-by: Jammy Huang <jammy_huang@...eedtech.com> >> --- >> drivers/media/platform/aspeed-video.c | 100 ++++++++++++++++++++++++++ >> 1 file changed, 100 insertions(+) >> >> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c >> index 8b3939b8052d..5b98dc7b7b15 100644 >> --- a/drivers/media/platform/aspeed-video.c >> +++ b/drivers/media/platform/aspeed-video.c >> @@ -21,6 +21,8 @@ >> #include <linux/videodev2.h> >> #include <linux/wait.h> >> #include <linux/workqueue.h> >> +#include <linux/debugfs.h> >> +#include <linux/ktime.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-dev.h> >> #include <media/v4l2-device.h> >> @@ -203,6 +205,14 @@ struct aspeed_video_buffer { >> struct list_head link; >> }; >> >> +struct aspeed_video_perf { >> + ktime_t last_sample; >> + u32 totaltime; >> + u32 duration; >> + u32 duration_min; >> + u32 duration_max; >> +}; >> + >> #define to_aspeed_video_buffer(x) \ >> container_of((x), struct aspeed_video_buffer, vb) >> >> @@ -241,6 +251,8 @@ struct aspeed_video { >> unsigned int frame_left; >> unsigned int frame_right; >> unsigned int frame_top; >> + >> + struct aspeed_video_perf perf; >> }; >> >> #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev) >> @@ -444,6 +456,18 @@ static void aspeed_video_write(struct aspeed_video *video, u32 reg, u32 val) >> readl(video->base + reg)); >> } >> >> +static void update_perf(struct aspeed_video *v) >> +{ >> + v->perf.duration = >> + ktime_to_ms(ktime_sub(ktime_get(), v->perf.last_sample)); >> + v->perf.totaltime += v->perf.duration; >> + >> + if (!v->perf.duration_max || v->perf.duration > v->perf.duration_max) >> + v->perf.duration_max = v->perf.duration; > How about > > v->perf.duration_max = max(v->perf.duration, v->perf.duration_max); > > instead of manually testing & branching? OK, this will be included in the next patch. >> + if (!v->perf.duration_min || v->perf.duration < v->perf.duration_min) >> + v->perf.duration_min = v->perf.duration; > And likewise with min(...) here. > > As a minor style thing, I might suggest adding a variable declaration > like > > struct aspeed_video_perf *p = &v->perf; > > and using that in the rest of the function to cut down on the > verbosity/repetition a bit. Or actually, since it looks like there > aren't any other members of struct aspeed_video accessed in this > function, maybe just make struct aspeed_video_perf be the parameter > instead? OK, this will be included in the next patch. > >> +} >> + >> static int aspeed_video_start_frame(struct aspeed_video *video) >> { >> dma_addr_t addr; >> @@ -482,6 +506,8 @@ static int aspeed_video_start_frame(struct aspeed_video *video) >> aspeed_video_update(video, VE_INTERRUPT_CTRL, 0, >> VE_INTERRUPT_COMP_COMPLETE); >> >> + video->perf.last_sample = ktime_get(); >> + >> aspeed_video_update(video, VE_SEQ_CTRL, 0, >> VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP); >> >> @@ -600,6 +626,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg) >> u32 frame_size = aspeed_video_read(video, >> VE_JPEG_COMP_SIZE_READ_BACK); >> >> + update_perf(video); >> + >> spin_lock(&video->lock); >> clear_bit(VIDEO_FRAME_INPRG, &video->flags); >> buf = list_first_entry_or_null(&video->buffers, >> @@ -760,6 +788,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video) >> det->width = MIN_WIDTH; >> det->height = MIN_HEIGHT; >> video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL; >> + memset(&video->perf, 0, sizeof(video->perf)); >> >> do { >> if (tries) { >> @@ -1517,6 +1546,71 @@ static const struct vb2_ops aspeed_video_vb2_ops = { >> .buf_queue = aspeed_video_buf_queue, >> }; >> >> +#ifdef CONFIG_DEBUG_FS >> +static int aspeed_video_debugfs_show(struct seq_file *s, void *data) >> +{ >> + struct aspeed_video *v = s->private; >> + >> + seq_printf(s, "%10s|%21s|%10s\n", >> + "Signal", "Resolution", "FRC"); >> + seq_printf(s, "%10s|%10s%11s|%10s\n", >> + "", "Width", "Height", ""); >> + seq_printf(s, "%10s|%10d%11d|%10d\n", >> + v->v4l2_input_status ? "Unlock" : "Lock", >> + v->pix_fmt.width, v->pix_fmt.height, v->frame_rate); >> + >> + seq_puts(s, "\n"); >> + >> + seq_printf(s, "%10s|%21s|%10s\n", >> + "Frame#", "Frame Duration", "FPS"); >> + seq_printf(s, "%10s|%7s%7s%7s|%10s\n", >> + "", "Now", "Min", "Max", ""); >> + seq_printf(s, "%10d|%7d%7d%7d|%10d\n", >> + v->sequence, v->perf.duration, v->perf.duration_min, >> + v->perf.duration_max, 1000/(v->perf.totaltime/v->sequence)); >> + > This looks like a convenient format for eyeballing with 'cat', but also > like it would be kind of awkward to parse if you wanted to do any sort > of automated analysis of the performance data it provides. Would a > key:value type format like > > width: %d > height: %d > frame_rate: %d > frame_number: %d > # etc. > > maybe provide a decent compromise? (Easily parseable, almost as easily > readable.) Your concern is reasonable, I will change the style to show information. This will be included in the next patch. > >> + return 0; >> +} >> + >> +int aspeed_video_proc_open(struct inode *inode, struct file *file) >> +{ >> + return single_open(file, aspeed_video_debugfs_show, inode->i_private); >> +} >> + >> +static const struct file_operations aspeed_video_debugfs_ops = { >> + .owner = THIS_MODULE, >> + .open = aspeed_video_proc_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> +}; >> + >> +static struct dentry *debugfs_entry; > I don't know how realistic the odds are of a system ever having multiple > aspeed-video devices, but structurally would this make more sense as > part of struct aspeed_video instead of being a single global? Since this is the driver for aspeed-video ip of SoC, it couldn't have multiple devices. >> + >> +static void aspeed_video_debugfs_remove(struct aspeed_video *video) >> +{ >> + debugfs_remove_recursive(debugfs_entry); >> + debugfs_entry = NULL; >> +} >> + >> +static int aspeed_video_debugfs_create(struct aspeed_video *video) >> +{ >> + debugfs_entry = debugfs_create_file(DEVICE_NAME, 0444, NULL, >> + video, >> + &aspeed_video_debugfs_ops); >> + if (!debugfs_entry) >> + aspeed_video_debugfs_remove(video); >> + >> + return debugfs_entry == NULL ? -EIO : 0; >> +} >> +#else >> +static void aspeed_video_debugfs_remove(struct aspeed_video *video) { } >> +static int aspeed_video_debugfs_create(struct aspeed_video *video) >> +{ >> + return 0; >> +} >> +#endif /* CONFIG_DEBUG_FS */ >> + >> static int aspeed_video_setup_video(struct aspeed_video *video) >> { >> const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) | >> @@ -1708,6 +1802,10 @@ static int aspeed_video_probe(struct platform_device *pdev) >> return rc; >> } >> >> + rc = aspeed_video_debugfs_create(video); >> + if (rc) >> + dev_err(video->dev, "debugfs create failed\n"); >> + >> return 0; >> } >> >> @@ -1719,6 +1817,8 @@ static int aspeed_video_remove(struct platform_device *pdev) >> >> aspeed_video_off(video); >> >> + aspeed_video_debugfs_remove(video); >> + >> clk_unprepare(video->vclk); >> clk_unprepare(video->eclk); >> >> -- >> 2.25.1 >>
Powered by blists - more mailing lists