On 8/1/2023 3:01 AM, Krzysztof Kozlowski wrote: > On 28/07/2023 15:23, Vikash Garodia wrote: >> this implements the debugging framework. > > Your commit msgs are not helping to understand why do you need it and > what is this doing. Based on this commit description I would ask you to > drop most of this code as it looks useless. Extend the commit msg to > provide proper justification and list of features each unit provides. > > Please do not use "This commit/patch", but imperative mood. See longer > explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> >> Signed-off-by: Vikash Garodia <quic_vgarodia@xxxxxxxxxxx> >> --- >> .../platform/qcom/iris/vidc/inc/msm_vidc_debug.h | 186 +++++++ >> .../platform/qcom/iris/vidc/src/msm_vidc_debug.c | 581 +++++++++++++++++++++ >> 2 files changed, 767 insertions(+) >> create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h >> create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_debug.c >> >> diff --git a/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h b/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h >> new file mode 100644 >> index 0000000..ffced01 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_debug.h >> @@ -0,0 +1,186 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#ifndef __MSM_VIDC_DEBUG__ >> +#define __MSM_VIDC_DEBUG__ >> + >> +#include <linux/debugfs.h> >> +#include <linux/delay.h> >> +#include <linux/errno.h> >> +#include <linux/module.h> >> +#include <linux/moduleparam.h> >> +#include <linux/types.h> >> + >> +struct msm_vidc_core; >> +struct msm_vidc_inst; >> + >> +#ifndef VIDC_DBG_LABEL >> +#define VIDC_DBG_LABEL "msm_vidc" >> +#endif > > Drop these three. Don't re-invent Linux kernel API. > >> + >> +/* Allow only 6 prints/sec */ >> +#define VIDC_DBG_SESSION_RATELIMIT_INTERVAL (1 * HZ) >> +#define VIDC_DBG_SESSION_RATELIMIT_BURST 6 >> + >> +#define VIDC_DBG_TAG_INST VIDC_DBG_LABEL ": %4s: %s: " >> +#define VIDC_DBG_TAG_CORE VIDC_DBG_LABEL ": %4s: %08x: %s: " >> +#define FW_DBG_TAG VIDC_DBG_LABEL ": %6s: " >> +#define DEFAULT_SID ((u32)-1) >> + >> +#ifndef MSM_VIDC_EMPTY_BRACE >> +#define MSM_VIDC_EMPTY_BRACE {}, > > That's the funniest code I saw since some time. > >> +#endif >> + >> +extern unsigned int msm_vidc_debug; > > Nope. > >> +extern unsigned int msm_fw_debug; > > Nope. > >> +extern bool msm_vidc_fw_dump; > > Nope. > >> + >> +/* do not modify the log message as it is used in test scripts */ >> +#define FMT_STRING_SET_CTRL \ >> + "%s: state %s, name %s, id 0x%x value %d\n" >> +#define FMT_STRING_STATE_CHANGE \ >> + "%s: state changed to %s from %s\n" >> +#define FMT_STRING_MSG_SFR \ >> + "SFR Message from FW: %s\n" >> +#define FMT_STRING_FAULT_HANDLER \ >> + "%s: faulting address: %lx\n" >> +#define FMT_STRING_SET_CAP \ >> + "set cap: name: %24s, cap value: %#10x, hfi: %#10llx\n" >> + >> +/* To enable messages OR these values and >> + * echo the result to debugfs file. >> + * >> + * To enable all messages set msm_vidc_debug = 0x101F >> + */ >> + >> +enum vidc_msg_prio_drv { >> + VIDC_ERR = 0x00000001, >> + VIDC_HIGH = 0x00000002, >> + VIDC_LOW = 0x00000004, >> + VIDC_PERF = 0x00000008, >> + VIDC_PKT = 0x00000010, >> + VIDC_BUS = 0x00000020, >> + VIDC_STAT = 0x00000040, >> + VIDC_ENCODER = 0x00000100, >> + VIDC_DECODER = 0x00000200, >> + VIDC_PRINTK = 0x10000000, >> + VIDC_FTRACE = 0x20000000, >> +}; >> + >> +enum vidc_msg_prio_fw { >> + FW_LOW = 0x00000001, >> + FW_MED = 0x00000002, >> + FW_HIGH = 0x00000004, >> + FW_ERROR = 0x00000008, >> + FW_FATAL = 0x00000010, >> + FW_PERF = 0x00000020, >> + FW_CACHE_LOW = 0x00000100, >> + FW_CACHE_MED = 0x00000200, >> + FW_CACHE_HIGH = 0x00000400, >> + FW_CACHE_ERROR = 0x00000800, >> + FW_CACHE_FATAL = 0x00001000, >> + FW_CACHE_PERF = 0x00002000, >> + FW_PRINTK = 0x10000000, >> + FW_FTRACE = 0x20000000, >> +}; >> + >> +#define DRV_LOG (VIDC_ERR | VIDC_PRINTK) >> +#define DRV_LOGSHIFT (0) >> +#define DRV_LOGMASK (0x0FFFFFFF) >> + >> +#define FW_LOG (FW_ERROR | FW_FATAL | FW_PRINTK) >> +#define FW_LOGSHIFT (0) >> +#define FW_LOGMASK (0x0FFFFFFF) >> + >> +#define dprintk_inst(__level, __level_str, inst, __fmt, ...) \ >> + do { \ >> + if (inst && (msm_vidc_debug & (__level))) { \ >> + pr_info(VIDC_DBG_TAG_INST __fmt, \ >> + __level_str, \ >> + inst->debug_str, \ >> + ##__VA_ARGS__); \ >> + } \ >> + } while (0) >> + >> +#define i_vpr_e(inst, __fmt, ...) dprintk_inst(VIDC_ERR, "err ", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_i(inst, __fmt, ...) dprintk_inst(VIDC_HIGH, "high", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_h(inst, __fmt, ...) dprintk_inst(VIDC_HIGH, "high", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_l(inst, __fmt, ...) dprintk_inst(VIDC_LOW, "low ", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_p(inst, __fmt, ...) dprintk_inst(VIDC_PERF, "perf", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_t(inst, __fmt, ...) dprintk_inst(VIDC_PKT, "pkt ", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_b(inst, __fmt, ...) dprintk_inst(VIDC_BUS, "bus ", inst, __fmt, ##__VA_ARGS__) > > NAK for entire interface. Please use standard debugging functions, not > pr_info for everything. > > dev_dbg, dev_info, dev_warn, dev_err. Only these. > > >> +#define i_vpr_s(inst, __fmt, ...) dprintk_inst(VIDC_STAT, "stat", inst, __fmt, ##__VA_ARGS__) >> + >> +#define i_vpr_hp(inst, __fmt, ...) \ >> + dprintk_inst(VIDC_HIGH | VIDC_PERF, "high", inst, __fmt, ##__VA_ARGS__) >> +#define i_vpr_hs(inst, __fmt, ...) \ >> + dprintk_inst(VIDC_HIGH | VIDC_STAT, "stat", inst, __fmt, ##__VA_ARGS__) >> +> +#define dprintk_core(__level, __level_str, __fmt, ...) \ > > NAK > >> + do { \ >> + if (msm_vidc_debug & (__level)) { \ >> + pr_info(VIDC_DBG_TAG_CORE __fmt, \ >> + __level_str, \ >> + DEFAULT_SID, \ >> + "codec", \ >> + ##__VA_ARGS__); \ >> + } \ >> + } while (0) >> + >> +#define d_vpr_e(__fmt, ...) dprintk_core(VIDC_ERR, "err ", __fmt, ##__VA_ARGS__) >> +#define d_vpr_h(__fmt, ...) dprintk_core(VIDC_HIGH, "high", __fmt, ##__VA_ARGS__) >> +#define d_vpr_l(__fmt, ...) dprintk_core(VIDC_LOW, "low ", __fmt, ##__VA_ARGS__) >> +#define d_vpr_p(__fmt, ...) dprintk_core(VIDC_PERF, "perf", __fmt, ##__VA_ARGS__) >> +#define d_vpr_t(__fmt, ...) dprintk_core(VIDC_PKT, "pkt ", __fmt, ##__VA_ARGS__) >> +#define d_vpr_b(__fmt, ...) dprintk_core(VIDC_BUS, "bus ", __fmt, ##__VA_ARGS__) >> +#define d_vpr_s(__fmt, ...) dprintk_core(VIDC_STAT, "stat", __fmt, ##__VA_ARGS__) >> +#define d_vpr_hs(__fmt, ...) \ >> + dprintk_core(VIDC_HIGH | VIDC_STAT, "high", __fmt, ##__VA_ARGS__) >> + >> +#define dprintk_ratelimit(__level, __level_str, __fmt, ...) \ >> + do { \ >> + if (msm_vidc_check_ratelimit()) { \ >> + dprintk_core(__level, __level_str, __fmt, ##__VA_ARGS__); \ >> + } \ >> + } while (0) >> + >> +#define dprintk_firmware(__level, __fmt, ...) \ >> + do { \ >> + if ((msm_fw_debug & (__level)) & FW_PRINTK) { \ >> + pr_info(FW_DBG_TAG __fmt, \ >> + "fw", \ >> + ##__VA_ARGS__); \ >> + } \ >> + } while (0) >> + >> +enum msm_vidc_debugfs_event { >> + MSM_VIDC_DEBUGFS_EVENT_ETB, >> + MSM_VIDC_DEBUGFS_EVENT_EBD, >> + MSM_VIDC_DEBUGFS_EVENT_FTB, >> + MSM_VIDC_DEBUGFS_EVENT_FBD, >> +}; >> + >> +enum msm_vidc_bug_on_error { >> + MSM_VIDC_BUG_ON_FATAL = BIT(0), >> + MSM_VIDC_BUG_ON_NOC = BIT(1), >> + MSM_VIDC_BUG_ON_WD_TIMEOUT = BIT(2), >> +}; >> + >> +struct dentry *msm_vidc_debugfs_init_drv(void); >> +struct dentry *msm_vidc_debugfs_init_core(struct msm_vidc_core *core); >> +struct dentry *msm_vidc_debugfs_init_inst(struct msm_vidc_inst *inst, >> + struct dentry *parent); >> +void msm_vidc_debugfs_deinit_inst(struct msm_vidc_inst *inst); >> +void msm_vidc_debugfs_update(struct msm_vidc_inst *inst, >> + enum msm_vidc_debugfs_event e); >> +int msm_vidc_check_ratelimit(void); >> + >> +static inline bool is_stats_enabled(void) >> +{ >> + return !!(msm_vidc_debug & VIDC_STAT); > > ... > >> +struct dentry *msm_vidc_debugfs_init_core(struct msm_vidc_core *core) >> +{ >> + struct dentry *dir = NULL; >> + char debugfs_name[MAX_DEBUGFS_NAME]; >> + struct dentry *parent; >> + >> + if (!core->debugfs_parent) { >> + d_vpr_e("%s: invalid params\n", __func__); >> + goto failed_create_dir; >> + } >> + parent = core->debugfs_parent; >> + >> + snprintf(debugfs_name, MAX_DEBUGFS_NAME, "core"); >> + dir = debugfs_create_dir(debugfs_name, parent); >> + if (IS_ERR_OR_NULL(dir)) { >> + dir = NULL; >> + d_vpr_e("Failed to create debugfs for msm_vidc\n"); >> + goto failed_create_dir; >> + } >> + if (!debugfs_create_file("info", 0444, dir, core, &core_info_fops)) { >> + d_vpr_e("debugfs_create_file: fail\n"); >> + goto failed_create_dir; >> + } >> + >> + if (!debugfs_create_file("stats_delay_ms", 0644, dir, core, &stats_delay_fops)) { >> + d_vpr_e("debugfs_create_file: fail\n"); > > > What is this entire debugfs supposed to provide? will remove this whole file in next version as part of custom debug wrappers removal. Thanks, Dikshita > > > > Best regards, > Krzysztof >
