On 2018-12-14 2:31 p.m., Wentland, Harry wrote:
> 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?
Yes, that's what I ended up doing. I already fixed it in this commit:
commit 2a90860cfb895ff55d961a1b727d58bb68e213ba
Author: Felix Kuehling <felix.kuehl...@amd.com>
Date: Fri Dec 7 17:01:27 2018 -0500
drm/amdgpu: Workaround build failure due to trace conflict
Avoid including mmu_context.h in amdgpu_amdkfd.h since that may be
included in other header files that define traces. This leads to
conflicts due to traces defined in other headers included via
mmu_context.h.
Signed-off-by: Felix Kuehling <felix.kuehl...@amd.com>
Acked-by: Alex Deucher <alexander.deuc...@amd.com>
Regards,
Felix
>
> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
