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

Tue Aug 8 21:46:34 UTC 2023

On Tue, Aug 08, 2023 at 08:37:52PM +0200, Michal Wajdeczko wrote:
>
>
>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()

these look fine

>
>[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

then I'd say it's a double mistake... how am I supposed to know what to
use wrt drm_warn() vs drm_WARN()?

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

sounds good to me

Lucas De Marchi

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