On 2018-12-07 9:41 a.m., Wentland, Harry wrote: > 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. >
Apologies for the spam. Just noticed that amdgpu.h includes only amdgpu_dm.h which is clean. The problem is that including amdgpu.h from amdgpu_dm.c now pulls in your trace events (from tlb.h) which we don't expect and care about. I think we should make sure amdgpu.h won't include anything that defines TRACE_EVENTs. Harry > Harry > >> Thanks, >> Felix >> >> -----Original Message----- >> From: amd-gfx <amd-gfx-boun...@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.fran...@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.fran...@amd.com> >> Reviewed-by: Harry Wentland <harry.wentl...@amd.com> >> Acked-by: Leo Li <sunpeng...@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 > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx