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

Tue Aug 8 18:37:52 UTC 2023

On 08.08.2023 18:28, 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 at 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 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>
>>>> ---
>>>> 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.

note that while this functionality is implemented as a macro, due to
necessary inside magic to obtain file/line, from the usage POV it tries
to mimic inline function, so using all upper case, as for simple value
macro, seems incorrect

>>
>> I think it comes from drm_WARN,  

yes, it was following drm_WARN() name pattern (and it uses WARN inside)

>> but yeah, could be XE_ASSERT.

quote from [1] "It's a matter of taste, but the SHOUTING UPPERCASE has
been argued to be less readable than lowercase."

so if you really don't want to mix cases then maybe it should be all
lower case:

 - xe_assert()
 - xe_tile_assert()
 - xe_gt_assert()

[1]
https://cgit.freedesktop.org/drm/drm-tip/commit/include/drm/drm_print.h?id=fb6c7ab8718eb2543695d77ad8302ff81e8e1e32

> 
> yeah... I think copying that would be like perpetuating a mistake.

not sure if that was a mistake, rather nice try to avoid clash with
drm_warn() that was providing different functionality

but since there is no global ASSERT that we follow, we can name it all
lowercase like e.g. lockdep_assert_held()

Michal

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