Re: [PATCH 24/33] iris: vidc: add debug files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
> 



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux