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