| Message ID | 20230525021219.23638-1-yunfei.dong@mediatek.com |
|---|---|
| Headers | show |
| Series | media: mediatek: vcodec: Add debugfs file for decode and encode | expand |
On 5/30/23 10:27, Yunfei Dong (董云飞) wrote: > Hi Hans, > > Sorry to disturb you. > > Could you please help to review and apply this patch series if it is ok > for you? Or whose review is expected before you can apply? AngeloGioacchino Del Regno reviewed this series before, so I'd like to have his OK for this series. Regards, Hans > > Best Regards, > Yunfei Dong > > On Thu, 2023-05-25 at 10:12 +0800, Yunfei Dong wrote: >> Need to change kernel driver to open decode and encode debug log at >> current period, >> it's very unreasonable. Adding debugfs common interface to support >> decode and encode, >> using echo command to control debug log level and getting useful >> information for each >> instance. >> >> patch 1 add dbgfs common interface. >> patch 2~5 support decode. >> patch 6~7 support encode >> patch 8 add help function >> --- >> changed with v4: >> - rebase to the top of media stage header. >> >> changed with v3: >> - add help function for patch 8 >> - remove append '\0' and enlarge buffer size for patch 4 >> >> changed with v2: >> - using pr_debug and dev_dbg instead of pr_info for patch 2. >> - fix word fail: informatiaoin -> information for patch 3. >> - used to print each instance format information for patch 5. >> >> changed with v1: >> - add new patch 4 and 5. >> - using cmd 'cat vdec' to show debug information instead of pr_info >> directly. >> --- >> Yunfei Dong (8): >> media: mediatek: vcodec: Add debugfs interface to get debug >> information >> media: mediatek: vcodec: Add debug params to control different log >> level >> media: mediatek: vcodec: Add a debugfs file to get different useful >> information >> media: mediatek: vcodec: Get each context resolution information >> media: mediatek: vcodec: Get each instance format type >> media: mediatek: vcodec: Change dbgfs interface to support encode >> media: mediatek: vcodec: Add encode to support dbgfs >> media: mediatek: vcodec: Add dbgfs help function >> >> .../media/platform/mediatek/vcodec/Makefile | 6 + >> .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 216 >> ++++++++++++++++++ >> .../mediatek/vcodec/mtk_vcodec_dbgfs.h | 72 ++++++ >> .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 4 + >> .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 4 + >> .../mediatek/vcodec/mtk_vcodec_enc_drv.c | 2 + >> .../mediatek/vcodec/mtk_vcodec_util.c | 8 + >> .../mediatek/vcodec/mtk_vcodec_util.h | 26 ++- >> 8 files changed, 335 insertions(+), 3 deletions(-) >> create mode 100644 >> drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c >> create mode 100644 >> drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.h >>
On Tue, May 30, 2023 at 6:06 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 25/05/23 04:12, Yunfei Dong ha scritto: > > Getting dbgfs help information with command "echo -help > vdec". > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 24 ++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > > index 237d0dc8a1fc..2372fc449b45 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > > @@ -52,6 +52,23 @@ static void mtk_vdec_dbgfs_get_format_type(struct mtk_vcodec_ctx *ctx, char *buf > > *used += curr_len; > > } > > > > +static void mtk_vdec_dbgfs_get_help(char *buf, int *used, int total) > > +{ > > + int curr_len; > > + > > + curr_len = snprintf(buf + *used, total - *used, > > + "help: (1: echo -'info' > vdec 2: cat vdec)\n"); > > + *used += curr_len; > > + > > + curr_len = snprintf(buf + *used, total - *used, > > + "\t-picinfo: get resolution\n"); > > + *used += curr_len; > > + > > + curr_len = snprintf(buf + *used, total - *used, > > + "\t-format: get output & capture queue format\n"); > > + *used += curr_len; > > +} > > + > > static ssize_t mtk_vdec_dbgfs_write(struct file *filp, const char __user *ubuf, > > size_t count, loff_t *ppos) > > { > > @@ -84,6 +101,11 @@ static ssize_t mtk_vdec_dbgfs_read(struct file *filp, char __user *ubuf, > > if (!buf) > > return -ENOMEM; > > > > + if (strstr(dbgfs->dbgfs_buf, "-help")) { > > I would print the help strings in two conditions: > 1. -help > 2. (nothing) > > ...so that if you don't echo anything to vdec (no params), you get the help text. > Otherwise, you would have to know that "-help" is a parameter that gives you help > text in the first place. > > As for this commit "as is", it works as intended and it is useful to retrieve > the help text; you can either send a followup commit that extends the help to > the corner case that I've explained, or send a v6 adding that to this same commit. > > I would prefer to see a v6 -- BUT -- since this series was sent a long time ago, > you will get my R-b and I will leave the final choice to Hans. > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> The help file could just exist as a separate file. You can then use cleaner interfaces for it. ChenYu
On 5/30/23 12:06, AngeloGioacchino Del Regno wrote: > Il 25/05/23 04:12, Yunfei Dong ha scritto: >> Getting dbgfs help information with command "echo -help > vdec". >> >> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> >> --- >> .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 24 ++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c >> index 237d0dc8a1fc..2372fc449b45 100644 >> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c >> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c >> @@ -52,6 +52,23 @@ static void mtk_vdec_dbgfs_get_format_type(struct mtk_vcodec_ctx *ctx, char *buf >> *used += curr_len; >> } >> >> +static void mtk_vdec_dbgfs_get_help(char *buf, int *used, int total) >> +{ >> + int curr_len; >> + >> + curr_len = snprintf(buf + *used, total - *used, >> + "help: (1: echo -'info' > vdec 2: cat vdec)\n"); >> + *used += curr_len; >> + >> + curr_len = snprintf(buf + *used, total - *used, >> + "\t-picinfo: get resolution\n"); >> + *used += curr_len; >> + >> + curr_len = snprintf(buf + *used, total - *used, >> + "\t-format: get output & capture queue format\n"); >> + *used += curr_len; >> +} >> + >> static ssize_t mtk_vdec_dbgfs_write(struct file *filp, const char __user *ubuf, >> size_t count, loff_t *ppos) >> { >> @@ -84,6 +101,11 @@ static ssize_t mtk_vdec_dbgfs_read(struct file *filp, char __user *ubuf, >> if (!buf) >> return -ENOMEM; >> >> + if (strstr(dbgfs->dbgfs_buf, "-help")) { > > I would print the help strings in two conditions: > 1. -help > 2. (nothing) > > ...so that if you don't echo anything to vdec (no params), you get the help text. > Otherwise, you would have to know that "-help" is a parameter that gives you help > text in the first place. > > As for this commit "as is", it works as intended and it is useful to retrieve > the help text; you can either send a followup commit that extends the help to > the corner case that I've explained, or send a v6 adding that to this same commit. > > I would prefer to see a v6 -- BUT -- since this series was sent a long time ago, > you will get my R-b and I will leave the final choice to Hans. > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > I prefer a v6, rebased on top of the media_stage tree. Regards, Hans
Hi Hans, On Tue, 2023-05-30 at 12:33 +0200, Hans Verkuil wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 5/30/23 12:06, AngeloGioacchino Del Regno wrote: > > Il 25/05/23 04:12, Yunfei Dong ha scritto: > >> Getting dbgfs help information with command "echo -help > vdec". > >> > >> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > >> --- > >> .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 24 > ++++++++++++++++++- > >> 1 file changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > >> index 237d0dc8a1fc..2372fc449b45 100644 > >> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > >> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c > >> @@ -52,6 +52,23 @@ static void > mtk_vdec_dbgfs_get_format_type(struct mtk_vcodec_ctx *ctx, char *buf > >> *used += curr_len; > >> } > >> > >> +static void mtk_vdec_dbgfs_get_help(char *buf, int *used, int > total) > >> +{ > >> +int curr_len; > >> + > >> +curr_len = snprintf(buf + *used, total - *used, > >> + "help: (1: echo -'info' > vdec 2: cat vdec)\n"); > >> +*used += curr_len; > >> + > >> +curr_len = snprintf(buf + *used, total - *used, > >> + "\t-picinfo: get resolution\n"); > >> +*used += curr_len; > >> + > >> +curr_len = snprintf(buf + *used, total - *used, > >> + "\t-format: get output & capture queue format\n"); > >> +*used += curr_len; > >> +} > >> + > >> static ssize_t mtk_vdec_dbgfs_write(struct file *filp, const > char __user *ubuf, > >> size_t count, loff_t *ppos) > >> { > >> @@ -84,6 +101,11 @@ static ssize_t mtk_vdec_dbgfs_read(struct file > *filp, char __user *ubuf, > >> if (!buf) > >> return -ENOMEM; > >> > >> +if (strstr(dbgfs->dbgfs_buf, "-help")) { > > > > I would print the help strings in two conditions: > > 1. -help > > 2. (nothing) > > > > ...so that if you don't echo anything to vdec (no params), you get > the help text. > > Otherwise, you would have to know that "-help" is a parameter that > gives you help > > text in the first place. > > > > As for this commit "as is", it works as intended and it is useful > to retrieve > > the help text; you can either send a followup commit that extends > the help to > > the corner case that I've explained, or send a v6 adding that to > this same commit. > > > > I would prefer to see a v6 -- BUT -- since this series was sent a > long time ago, > > you will get my R-b and I will leave the final choice to Hans. > > > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > > > > > > I prefer a v6, rebased on top of the media_stage tree. > > Regards, > > Hans I already have sent patch v6 to review. Best Regards, Yunfei Dong
Need to change kernel driver to open decode and encode debug log at current period, it's very unreasonable. Adding debugfs common interface to support decode and encode, using echo command to control debug log level and getting useful information for each instance. patch 1 add dbgfs common interface. patch 2~5 support decode. patch 6~7 support encode patch 8 add help function --- changed with v4: - rebase to the top of media stage header. changed with v3: - add help function for patch 8 - remove append '\0' and enlarge buffer size for patch 4 changed with v2: - using pr_debug and dev_dbg instead of pr_info for patch 2. - fix word fail: informatiaoin -> information for patch 3. - used to print each instance format information for patch 5. changed with v1: - add new patch 4 and 5. - using cmd 'cat vdec' to show debug information instead of pr_info directly. --- Yunfei Dong (8): media: mediatek: vcodec: Add debugfs interface to get debug information media: mediatek: vcodec: Add debug params to control different log level media: mediatek: vcodec: Add a debugfs file to get different useful information media: mediatek: vcodec: Get each context resolution information media: mediatek: vcodec: Get each instance format type media: mediatek: vcodec: Change dbgfs interface to support encode media: mediatek: vcodec: Add encode to support dbgfs media: mediatek: vcodec: Add dbgfs help function .../media/platform/mediatek/vcodec/Makefile | 6 + .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 216 ++++++++++++++++++ .../mediatek/vcodec/mtk_vcodec_dbgfs.h | 72 ++++++ .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 4 + .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 4 + .../mediatek/vcodec/mtk_vcodec_enc_drv.c | 2 + .../mediatek/vcodec/mtk_vcodec_util.c | 8 + .../mediatek/vcodec/mtk_vcodec_util.h | 26 ++- 8 files changed, 335 insertions(+), 3 deletions(-) create mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.c create mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dbgfs.h