Re: [Intel-xe] [PATCH] drm/xe: Introduce xe_ASSERT macros

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Aug 08, 2023 at 10:28:01AM -0600, Lucas De Marchi wrote:
> On Tue, Aug 08, 2023 at 06:04:20PM +0300, Jani Nikula wrote:
> > On Tue, 08 Aug 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > > On Mon, Aug 07, 2023 at 07:34:46PM +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
> > > > 
> > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > Cc: Oded Gabbay <ogabbay@kernel.org>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_assert.h | 160 +++++++++++++++++++++++++++++++++
> > > > 1 file changed, 160 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..7ea295b7091c
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/xe/xe_assert.h
> > > > @@ -0,0 +1,160 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +/*
> > > > + * Copyright © 2023 Intel Corporation
> > > > + */
> > > > +
> > > > +#ifndef __XE_ASSERT_H__
> > > > +#define __XE_ASSERT_H__
> > > > +
> > > > +#include <linux/string_helpers.h>
> > > > +#include <drm/drm_print.h>
> > > > +#include "xe_device_types.h"
> > > > +#include "xe_step.h"
> > > > +
> > > > +/**
> > > > + * DOC: Xe ASSERTs
> > > > + *
> > > > + * While Xe driver aims to be simpler than legacy i915 driver it is still
> > > > + * complex enough that some changes introduced while adding new functionality
> > > > + * could break the existing code.
> > > > + *
> > > > + * Adding &drm_WARN or &drm_err to catch unwanted programming usage could lead
> > > > + * to undesired increased driver footprint and may impact production driver
> > > > + * performance as this additional code will be always present.
> > > > + *
> > > > + * To allow annotate functions with additional detailed debug checks to assert
> > > > + * that all prerequisites are satisfied, without worrying about footprint or
> > > > + * performance penalty on production builds where all potential misuses
> > > > + * introduced during code integration were already fixed, we introduce family
> > > > + * of ASSERT macros that try to follow classic assert() utility and can be
> > > > + * compiled out on non-debug builds:
> > > > + *
> > > > + *  * &xe_ASSERT
> > > 
> > > pass by comment, not really checking anything else here... Why are we
> > > mixing upper/lower case? It's perfectly fine to use XE_ as the namespace
> > > like is done for other macros.
> > 
> > I think it comes from drm_WARN, but yeah, could be XE_ASSERT.
> 
> yeah... I think copying that would be like perpetuating a mistake.
>

Agree with Lucas.

Matt
 
> Lucas De Marchi
> 
> > 
> > > 
> > > Lucas De Marchi
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center