On 2018-12-07 3:32 p.m., Kuehling, Felix wrote: > On 2018-12-07 9:46 a.m., Wentland, Harry wrote: >> 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. > > amdgpu_amdkfd.h defines a macro that uses use_mm and unuse_mm. Therefore > it needs to include mmu_context.h, which pulls in the conflicting trace > events. Maybe we can move that into a different header file that doesn't > get included in amdgpu.h. Or find another way to avoid including > amdgpu_amdkfd.h in amdgpu.h. >
It's defined but not used in amdgpu_amdkfd.h (at least in the amd-stg tree). Couldn't you include mmu_context.h in the files that use it (amdgpu_amdkfd_gfx_v7/8/9.c) instead and avoid the problem that way? Harry > Thanks, > Felix > > > >> >> 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