Re: [PATCH 04/16 v2] drm/amd/display: Add tracing to dc

["Wentland, Harry" <Harry.Wentland-5C7GfCeVMHo@public.gmane.org>]

On 2018-12-07 12:40 a.m., Kuehling, Felix wrote:
> This change seems to be breaking the build for me. I'm getting errors like this:
> 
>    CC [M]  drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> In file included from ../include/trace/events/tlb.h:9:0,
>                   from ../arch/x86/include/asm/mmu_context.h:10,
>                   from ../include/linux/mmu_context.h:5,
>                   from ../drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h:30,
>                   from ../drivers/gpu/drm/amd/amdgpu/amdgpu.h:85,
>                   from ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:34:
> ../include/linux/tracepoint.h:285:20: error: redefinition of â__tpstrtab_amdgpu_dc_rregâ
>    static const char __tpstrtab_##name[]     \
>                      ^
> ../include/linux/tracepoint.h:293:2: note: in expansion of macro âDEFINE_TRACE_FNâ
>    DEFINE_TRACE_FN(name, NULL, NULL);
>    ^
> ../include/trace/define_trace.h:28:2: note: in expansion of macro âDEFINE_TRACEâ
>    DEFINE_TRACE(name)
>    ^
> ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/./amdgpu_dm_trace.h:34:1: note: in expansion of macro âTRACE_EVENTâ
>   TRACE_EVENT(amdgpu_dc_rreg,
>   ^
> 
> I have a local change that adds #include <amdgpu_amdkfd.h> to amdgpu.h, which pulls in include/trace/events/tlb.h. That seems to create some kind of conflict with trace definitions. Any ideas how to fix this? It seems a bit fragile if adding some random include can break the build like this.
> 

That's the trace subsystem for you. David and I are trying to understand what's going on. I think the problem is that both tlb.h and amdgpu_dm_trace.h define trace events and we now include both here.

I think we'd want to include neither trace events from amdgpu.h to avoid this problem in the future, so we'll probably have to (a) clean up the dc.h include to only contain what amdgpu.h needs and (b) clean up amdgpu_amdkfd.h to only contain what amdgpu.h needs. At the end amdgpu.h doesn't care about the tracer. The problem seems that dc.h and amdgpu_amdkfd.h are the main includes for our respective drivers but are also wholesale included in amdgpu.h.

Harry

> Thanks,
>    Felix
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of David Francis
> Sent: Friday, November 30, 2018 9:57 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Francis, David <David.Francis@amd.com>
> Subject: [PATCH 04/16 v2] drm/amd/display: Add tracing to dc
> 
> [Why]
> Tracing is a useful and cheap debug functionality
> 
> [How]
> This creates a new trace system amdgpu_dm, currently with three trace events
> 
> amdgpu_dc_rreg and amdgpu_dc_wreg report the address and value of any dc register reads and writes
> 
> amdgpu_dc_performance requires at least one of those two to be enabled.  It counts the register reads and writes since the last entry
> 
> v2: Don't check for NULL before kfree
> 
> Signed-off-by: David Francis <David.Francis@amd.com>
> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
> Acked-by: Leo Li <sunpeng.li@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   3 +
>   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 104 ++++++++++++++++++
>   drivers/gpu/drm/amd/display/dc/core/dc.c      |  19 ++++
>   drivers/gpu/drm/amd/display/dc/dc_types.h     |   8 ++
>   .../amd/display/dc/dcn10/dcn10_cm_common.c    |   4 +-
>   drivers/gpu/drm/amd/display/dc/dm_services.h  |  12 +-
>   6 files changed, 146 insertions(+), 4 deletions(-)  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 76b1aebdca0c..376927c8bcc6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -23,6 +23,9 @@
>    *
>    */
>   
> +/* The caprices of the preprocessor require that this be declared right
> +here */ #define CREATE_TRACE_POINTS
> +
>   #include "dm_services_types.h"
>   #include "dc.h"
>   #include "dc/inc/core_types.h"
> 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
> new file mode 100644
> index 000000000000..d898981684d5
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright 2018 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
> + *
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM amdgpu_dm
> +
> +#if !defined(_AMDGPU_DM_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _AMDGPU_DM_TRACE_H_
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(amdgpu_dc_rreg,
> +	TP_PROTO(unsigned long *read_count, uint32_t reg, uint32_t value),
> +	TP_ARGS(read_count, reg, value),
> +	TP_STRUCT__entry(
> +			__field(uint32_t, reg)
> +			__field(uint32_t, value)
> +		),
> +	TP_fast_assign(
> +			__entry->reg = reg;
> +			__entry->value = value;
> +			*read_count = *read_count + 1;
> +		),
> +	TP_printk("reg=0x%08lx, value=0x%08lx",
> +			(unsigned long)__entry->reg,
> +			(unsigned long)__entry->value)
> +);
> +
> +TRACE_EVENT(amdgpu_dc_wreg,
> +	TP_PROTO(unsigned long *write_count, uint32_t reg, uint32_t value),
> +	TP_ARGS(write_count, reg, value),
> +	TP_STRUCT__entry(
> +			__field(uint32_t, reg)
> +			__field(uint32_t, value)
> +		),
> +	TP_fast_assign(
> +			__entry->reg = reg;
> +			__entry->value = value;
> +			*write_count = *write_count + 1;
> +		),
> +	TP_printk("reg=0x%08lx, value=0x%08lx",
> +			(unsigned long)__entry->reg,
> +			(unsigned long)__entry->value)
> +);
> +
> +
> +TRACE_EVENT(amdgpu_dc_performance,
> +	TP_PROTO(unsigned long read_count, unsigned long write_count,
> +		unsigned long *last_read, unsigned long *last_write,
> +		const char *func, unsigned int line),
> +	TP_ARGS(read_count, write_count, last_read, last_write, func, line),
> +	TP_STRUCT__entry(
> +			__field(uint32_t, reads)
> +			__field(uint32_t, writes)
> +			__field(uint32_t, read_delta)
> +			__field(uint32_t, write_delta)
> +			__string(func, func)
> +			__field(uint32_t, line)
> +			),
> +	TP_fast_assign(
> +			__entry->reads = read_count;
> +			__entry->writes = write_count;
> +			__entry->read_delta = read_count - *last_read;
> +			__entry->write_delta = write_count - *last_write;
> +			__assign_str(func, func);
> +			__entry->line = line;
> +			*last_read = read_count;
> +			*last_write = write_count;
> +			),
> +	TP_printk("%s:%d reads=%08ld (%08ld total), writes=%08ld (%08ld total)",
> +			__get_str(func), __entry->line,
> +			(unsigned long)__entry->read_delta,
> +			(unsigned long)__entry->reads,
> +			(unsigned long)__entry->write_delta,
> +			(unsigned long)__entry->writes)
> +);
> +#endif /* _AMDGPU_DM_TRACE_H_ */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +#define TRACE_INCLUDE_FILE amdgpu_dm_trace #include
> +<trace/define_trace.h>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index dba6b57830c7..3903b2c0a6f1 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -175,6 +175,17 @@ static bool create_links(
>   	return false;
>   }
>   
> +static struct dc_perf_trace *dc_perf_trace_create(void) {
> +	return kzalloc(sizeof(struct dc_perf_trace), GFP_KERNEL); }
> +
> +static void dc_perf_trace_destroy(struct dc_perf_trace **perf_trace) {
> +	kfree(*perf_trace);
> +	*perf_trace = NULL;
> +}
> +
>   /**
>    *****************************************************************************
>    *  Function: dc_stream_adjust_vmin_vmax @@ -536,6 +547,8 @@ static void destruct(struct dc *dc)
>   	if (dc->ctx->created_bios)
>   		dal_bios_parser_destroy(&dc->ctx->dc_bios);
>   
> +	dc_perf_trace_destroy(&dc->ctx->perf_trace);
> +
>   	kfree(dc->ctx);
>   	dc->ctx = NULL;
>   
> @@ -659,6 +672,12 @@ static bool construct(struct dc *dc,
>   		goto fail;
>   	}
>   
> +	dc_ctx->perf_trace = dc_perf_trace_create();
> +	if (!dc_ctx->perf_trace) {
> +		ASSERT_CRITICAL(false);
> +		goto fail;
> +	}
> +
>   	/* Create GPIO service */
>   	dc_ctx->gpio_service = dal_gpio_service_create(
>   			dc_version,
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
> index 6e12d640d020..8390baedaf71 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
> @@ -73,10 +73,18 @@ struct hw_asic_id {
>   	void *atombios_base_address;
>   };
>   
> +struct dc_perf_trace {
> +	unsigned long read_count;
> +	unsigned long write_count;
> +	unsigned long last_entry_read;
> +	unsigned long last_entry_write;
> +};
> +
>   struct dc_context {
>   	struct dc *dc;
>   
>   	void *driver_context; /* e.g. amdgpu_device */
> +	struct dc_perf_trace *perf_trace;
>   	void *cgs_device;
>   
>   	enum dce_environment dce_environment;
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_cm_common.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_cm_common.c
> index 3eea44092a04..7469333a2c8a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_cm_common.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_cm_common.c
> @@ -324,7 +324,7 @@ bool cm_helper_translate_curve_to_hw_format(
>   	if (output_tf == NULL || lut_params == NULL || output_tf->type == TF_TYPE_BYPASS)
>   		return false;
>   
> -	PERF_TRACE();
> +	PERF_TRACE_CTX(output_tf->ctx);
>   
>   	corner_points = lut_params->corner_points;
>   	rgb_resulted = lut_params->rgb_resulted; @@ -513,7 +513,7 @@ bool cm_helper_translate_curve_to_degamma_hw_format(
>   	if (output_tf == NULL || lut_params == NULL || output_tf->type == TF_TYPE_BYPASS)
>   		return false;
>   
> -	PERF_TRACE();
> +	PERF_TRACE_CTX(output_tf->ctx);
>   
>   	corner_points = lut_params->corner_points;
>   	rgb_resulted = lut_params->rgb_resulted; diff --git a/drivers/gpu/drm/amd/display/dc/dm_services.h b/drivers/gpu/drm/amd/display/dc/dm_services.h
> index 28128c02de00..1961cc6d9143 100644
> --- a/drivers/gpu/drm/amd/display/dc/dm_services.h
> +++ b/drivers/gpu/drm/amd/display/dc/dm_services.h
> @@ -31,6 +31,8 @@
>   
>   #define __DM_SERVICES_H__
>   
> +#include "amdgpu_dm_trace.h"
> +
>   /* TODO: remove when DC is complete. */  #include "dm_services_types.h"
>   #include "logger_interface.h"
> @@ -70,6 +72,7 @@ static inline uint32_t dm_read_reg_func(
>   	}
>   #endif
>   	value = cgs_read_register(ctx->cgs_device, address);
> +	trace_amdgpu_dc_rreg(&ctx->perf_trace->read_count, address, value);
>   
>   	return value;
>   }
> @@ -90,6 +93,7 @@ static inline void dm_write_reg_func(
>   	}
>   #endif
>   	cgs_write_register(ctx->cgs_device, address, value);
> +	trace_amdgpu_dc_wreg(&ctx->perf_trace->write_count, address, value);
>   }
>   
>   static inline uint32_t dm_read_index_reg( @@ -351,8 +355,12 @@ unsigned long long dm_get_elapse_time_in_ns(struct dc_context *ctx,
>   /*
>    * performance tracing
>    */
> -void dm_perf_trace_timestamp(const char *func_name, unsigned int line);
> -#define PERF_TRACE()	dm_perf_trace_timestamp(__func__, __LINE__)
> +#define PERF_TRACE()	trace_amdgpu_dc_performance(CTX->perf_trace->read_count,\
> +		CTX->perf_trace->write_count, &CTX->perf_trace->last_entry_read,\
> +		&CTX->perf_trace->last_entry_write, __func__, __LINE__)
> +#define PERF_TRACE_CTX(__CTX)	trace_amdgpu_dc_performance(__CTX->perf_trace->read_count,\
> +		__CTX->perf_trace->write_count, &__CTX->perf_trace->last_entry_read,\
> +		&__CTX->perf_trace->last_entry_write, __func__, __LINE__)
>   
>   
>   /*
> --
> 2.17.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx