[Intel-xe] [PATCH v2] drm/xe: Introduce Xe assert macros

[Lucas De Marchi]

On Wed, Aug 09, 2023 at 08:05:15PM +0200, Michal Wajdeczko wrote:
>As we are moving away from the controversial XE_BUG_ON macro,
>relying just on WARN_ON or drm_err does not cover the cases
>where we want to annotate functions with additional detailed
>debug checks to assert that all prerequisites are satisfied,
>without paying footprint or performance penalty on non-debug
>builds, where all misuses introduced during code integration
>were already fixed.
>
>Introduce family of Xe assert macros that try to follow classic
>assert() utility and can be compiled out on non-debug builds.
>
>Macros are based on drm_WARN, but unlikely to origin, disallow
>use in expressions since we will compile that code out.
>
>As we are operating on the xe pointers, we can print additional
>information about the device, like tile or GT identifier, that
>is not available from generic WARN report:
>
>[ ] xe 0000:00:02.0: [drm] Assertion `true == false` failed!
>    platform: 1 subplatform: 1
>    graphics: Xe_LP 12.0 step B0
>    media: Xe_M 12.0 step B0
>    display: enabled step D0
>    tile: 0 VRAM 0 B
>    GT: 0 type 1
>
>[ ] xe 0000:b3:00.0: [drm] Assertion `true == false` failed!
>    platform: 7 subplatform: 3
>    graphics: Xe_HPG 12.55 step A1
>    media: Xe_HPM 12.55 step A1
>    display: disabled step **
>    tile: 0 VRAM 14.0 GiB
>    GT: 0 type 1
>
>[ ] WARNING: CPU: 0 PID: 2687 at drivers/gpu/drm/xe/xe_device.c:281 xe_device_probe+0x374/0x520 [xe]
>[ ] RIP: 0010:xe_device_probe+0x374/0x520 [xe]
>[ ] Call Trace:
>[ ]  ? __warn+0x7b/0x160
>[ ]  ? xe_device_probe+0x374/0x520 [xe]
>[ ]  ? report_bug+0x1c3/0x1d0
>[ ]  ? handle_bug+0x42/0x70
>[ ]  ? exc_invalid_op+0x14/0x70
>[ ]  ? asm_exc_invalid_op+0x16/0x20
>[ ]  ? xe_device_probe+0x374/0x520 [xe]
>[ ]  ? xe_device_probe+0x374/0x520 [xe]
>[ ]  xe_pci_probe+0x6e3/0x950 [xe]
>[ ]  ? lockdep_hardirqs_on+0xc7/0x140
>[ ]  pci_device_probe+0x9e/0x160
>[ ]  really_probe+0x19d/0x400
>
>v2: use lowercase names
>
>Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>Cc: Oded Gabbay <ogabbay at kernel.org>
>Cc: Jani Nikula <jani.nikula at intel.com>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Matthew Brost <matthew.brost at intel.com>


Just a few comments below, but motivation, doc and intended use all look
good to me. With the changes below, this is

	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

As maybe a controversial thing, let's wait for at least one more ack
before applying.

>---
> drivers/gpu/drm/xe/xe_assert.h | 175 +++++++++++++++++++++++++++++++++
> 1 file changed, 175 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_assert.h
>
>diff --git a/drivers/gpu/drm/xe/xe_assert.h b/drivers/gpu/drm/xe/xe_assert.h
>new file mode 100644
>index 000000000000..4977d9fd3dab
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_assert.h
>@@ -0,0 +1,175 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2023 Intel Corporation
>+ */
>+
>+#ifndef __XE_ASSERT_H__
>+#define __XE_ASSERT_H__

in xe we use just one underscore for header guards. Example:

#ifndef _XE_REGS_H_
#define _XE_REGS_H_


>+
>+#include <linux/string_helpers.h>
>+#include <drm/drm_print.h>

missing newline here to separate the 2 blocks and each block should be
alphabetically sorted.


>+#include "xe_device_types.h"
>+#include "xe_step.h"
>+


< snip >

>+
>+#endif /* __XE_ASSERT_H__ */

and no comment at the end of the endif


thanks
Lucas De Marchi

>-- 
>2.25.1
>