Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

["Christian König" <christian.koenig@amd.com>]

Am 21.07.21 um 13:44 schrieb Rodrigo Siqueira:
> On 07/20, Christian König wrote:
>>
>> Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
>>> We don't have any mechanism for tracing FPU operations inside the
>>> display core, making the debug work a little bit tricky. This commit
>>> introduces a trace mechanism inside our DC_FP_START/END macros for
>>> trying to alleviate this problem.
>>>
>>> Changes since V2:
>>> - Make sure that we compile FPU operation only in the context that DCN
>>>     is enabled.
>>>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Anson Jacob <Anson.Jacob@amd.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Hersen Wu <hersenxs.wu@amd.com>
>>> Cc: Aric Cyr <aric.cyr@amd.com>
>>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>> ---
>>>    .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  4 ++
>>>    .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
>>>    drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
>>>    drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
>>>    6 files changed, 128 insertions(+), 3 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> index 91fb72c96545..718e123a3230 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> @@ -27,6 +27,10 @@
>>>    AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>>> +ifdef CONFIG_DRM_AMD_DC_DCN
>>> +AMDGPUDM += dc_fpu.o
>>> +endif
>>> +
>>>    ifneq ($(CONFIG_DRM_AMD_DC),)
>>>    AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
>>>    endif
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> index 46a33f64cf8e..230bb12c405e 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>>>    		  __entry->refresh_rate_ns)
>>>    );
>>> +TRACE_EVENT(dcn_fpu,
>>> +	    TP_PROTO(bool begin, const char *function, const int line),
>>> +	    TP_ARGS(begin, function, line),
>>> +
>>> +	    TP_STRUCT__entry(
>>> +			     __field(bool, begin)
>>> +			     __field(const char *, function)
>>> +			     __field(int, line)
>>> +	    ),
>>> +	    TP_fast_assign(
>>> +			   __entry->begin = begin;
>>> +			   __entry->function = function;
>>> +			   __entry->line = line;
>>> +	    ),
>>> +	    TP_printk("%s()+%d: %s",
>>> +		      __entry->function,
>>> +		      __entry->line,
>>> +		      __entry->begin ? "begin" : "end"
>>> +	    )
>>> +);
>>> +
>>>    #endif /* _AMDGPU_DM_TRACE_H_ */
>>>    #undef TRACE_INCLUDE_PATH
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> new file mode 100644
>>> index 000000000000..d5d156a4517e
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#include "dc_trace.h"
>>> +
>>> +#include <asm/fpu/api.h>
>>> +
>>> +/**
>>> + * dc_fpu_begin - Enables FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + *   (usually __func__)
>>> + *
>>> + * @line: A line number where DC_FP_START was invoked for debug purpose
>>> + *   (usually __LINE__)
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_begin() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_START().
>>> + */
>>> +void dc_fpu_begin(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(true, function_name, line);
>>> +	kernel_fpu_begin();
>>> +}
>>> +
>>> +/**
>>> + * dc_fpu_end - Disable FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + * @line: A-line number where DC_FP_END was invoked for debug purpose
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_end() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_END().
>>> + */
>>> +void dc_fpu_end(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(false, function_name, line);
>>> +	kernel_fpu_end();
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>> new file mode 100644
>>> index 000000000000..fb54983c5c60
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#ifndef __DC_FPU_H__
>>> +#define __DC_FPU_H__
>>> +
>>> +void dc_fpu_begin(const char *function_name, const int line);
>>> +void dc_fpu_end(const char *function_name, const int line);
>>> +
>>> +#endif /* __DC_FPU_H__ */
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> index d2615357269b..d598ba697e45 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> @@ -37,3 +37,6 @@
>>>    #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>>>    	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
>>> +
>>> +#define TRACE_DCN_FPU(begin, function, line) \
>>> +	trace_dcn_fpu(begin, function, line)
>>> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> index 126c2f3a4dd3..2ba49aef370d 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> @@ -52,9 +52,9 @@
>>>    #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>    #if defined(CONFIG_X86)
>>> -#include <asm/fpu/api.h>
>>> -#define DC_FP_START() kernel_fpu_begin()
>>> -#define DC_FP_END() kernel_fpu_end()
>>> +#include "amdgpu_dm/dc_fpu.h"
>>> +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
>>> +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
>> This still has the issue that you don't correctly handle other architectures
>> than x86.
>>
>> I'm pretty sure the kernel bots will complain about that.
> Hi Christian,
>
> Kernel bots will not break because we are using CONFIG_DRM_AMD_DC_DCN to
> isolate DCN architecture. Currently, we support DCN on x86 and PPC (idk
> the details for ppc); in this series, I'm trying to enable us to start
> to isolate FPU code to a single place. When we start to move the FPU
> functions to a single place, we can take the opportunity to refactor the
> FPU code. After we isolate the FPU source, we can safely start to enable
> other architectures such as ARM.
>
> This is why I did not try to enable it for other architecture yet. I
> think this is the second step.

That sounds like a plan to me, but I have to tell you that this will 
still break the build bots for PPC.

>
> Thanks
> Siqueira
>   
>> Regards,
>> Christian.
>>
>>>    #elif defined(CONFIG_PPC64)

Because of this switch here.

You need the unification of the floating point handling from the other 
mail thread as prerequisite for this series if you want to avoid this.

Regards,
Christian.

>>>    #include <asm/switch_to.h>
>>>    #include <asm/cputable.h>