Re: [Intel-xe] [PATCH 1/2] drm/xe: Introduce GT oriented log messages

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 13 Mar 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> +dri-devel
>
> On Mon, Mar 13, 2023 at 09:03:56AM +0100, Michal Wajdeczko wrote:
>>While debugging GT related problems, it's good to know which GT was
>>reporting problems. Introduce helper macros to allow prefix GT logs
>>with GT idetifier. We will use them in upcoming patches.
>>
>>Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>---
>> drivers/gpu/drm/xe/xe_gt_printk.h | 45 +++++++++++++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_printk.h
>>
>>diff --git a/drivers/gpu/drm/xe/xe_gt_printk.h b/drivers/gpu/drm/xe/xe_gt_printk.h
>>new file mode 100644
>>index 000000000000..b12a92024126
>>--- /dev/null
>>+++ b/drivers/gpu/drm/xe/xe_gt_printk.h
>>@@ -0,0 +1,45 @@
>>+/* SPDX-License-Identifier: MIT */
>>+/*
>>+ * Copyright © 2023 Intel Corporation
>>+ */
>>+
>>+#ifndef _XE_GT_PRINTK_H_
>>+#define _XE_GT_PRINTK_H_
>>+
>>+#include <drm/drm_print.h>
>
> missing blank line
>
>>+#include "xe_gt_types.h"
>>+
>>+#define gt_printk(_gt, _level, _fmt, ...) \
>
> any exposed interface in headers should have xe_
> prefix.
>
> I myself find it odd to have these macros that are wrappers over
> wrappers and create a silo with xe-speficic debugging macros.
> The DRM ones at least are shared across drivers.  The pr_dbg(), just
> reusing a local define is nice as it's still the same call used
> everywhere... but it wouldn't work very well here as we need the extra
> parm.

My biggest problem with this in i915 always was where to draw the line
for adding new macros. Add them for something, and you've set the
example that's okay to do.

And now we have gt_dbg, huc_dbg, guc_dbg, and all the variants. Many of
them single-use or unused, but added for completeness.

I display we could have drm subsystem wide variants for drm_crtc,
drm_connector, drm_encoder, but we've all just opted not to. It's more
verbose at the call sites, but it's obvious everywhere, to everyone.

I agree with Lucas, I think the wrappers on wrappers on wrappers on
wrappers is counter productive. Yes, that's where we are in i915:

	guc/huc -> gt -> drm -> dev -> printk

> I won't oppose them though since a lot of people seem to like the
> approach to help their debug and i915 went through similar discussion.

Yeah, well, that's what I said in i915 as well, and it wasn't in my
domain, really. And ditto here. But I'd totally block this direction in
i915 display code.


BR,
Jani.

>
> Lucas De Marchi
>
>>+	drm_##_level(&(_gt)->xe->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>+
>>+#define gt_err(_gt, _fmt, ...) \
>>+	gt_printk((_gt), err, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_warn(_gt, _fmt, ...) \
>>+	gt_printk((_gt), warn, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_notice(_gt, _fmt, ...) \
>>+	gt_printk((_gt), notice, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_info(_gt, _fmt, ...) \
>>+	gt_printk((_gt), info, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_dbg(_gt, _fmt, ...) \
>>+	gt_printk((_gt), dbg, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_err_ratelimited(_gt, _fmt, ...) \
>>+	gt_printk((_gt), err_ratelimited, _fmt, ##__VA_ARGS__)
>>+
>>+#define gt_WARN(_gt, _condition, _fmt, ...) \
>>+	drm_WARN(&(_gt)->xe->drm, _condition, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>+
>>+#define gt_WARN_ONCE(_gt, _condition, _fmt, ...) \
>>+	drm_WARN_ONCE(&(_gt)->xe->drm, _condition, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>+
>>+#define gt_WARN_ON(_gt, _condition) \
>>+	gt_WARN((_gt), _condition, "%s(%s)", "gt_WARN_ON", __stringify(_condition))
>>+
>>+#define gt_WARN_ON_ONCE(_gt, _condition) \
>>+	gt_WARN_ONCE((_gt), _condition, "%s(%s)", "gt_WARN_ON_ONCE", __stringify(_condition))
>>+
>>+#endif
>>-- 
>>2.25.1
>>

-- 
Jani Nikula, Intel Open Source Graphics Center