Debug issues related to display can be a challenge due to the complexity around this topic and different source of information might help in this process. We already have support for tracepoints inside the display component, i.e., we have the basic functionalities available and we just need to expand it in order to make it more valuable for debugging. For this reason, this patchset reworks part of the current tracepoint options and add different sets of tracing inside amdgpu_dm, display core, and DCN10. The first patch of this series just rework part of the current tracepoints and the last set of patches introduces new tracepoints.
This first patchset version is functional. Please, let me know what I can improve in the current version but also let me know what kind of tracepoint I can add for the next version.
Finally, I want to highlight that this work is based on a set of patches originally made by Nicholas Kazlauskas.
Change in V2: - I added another patch for capturing the clock state for different display architecture.
Rodrigo Siqueira (4): drm/amd/display: Rework registers tracepoint drm/amd/display: Add tracepoint for amdgpu_dm drm/amd/display: Add pipe_state tracepoint drm/amd/display: Add tracepoint for capturing clocks state
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 + .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 712 +++++++++++++++++- .../dc/clk_mgr/dce112/dce112_clk_mgr.c | 5 + .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 4 + .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 4 + drivers/gpu/drm/amd/display/dc/core/dc.c | 11 + .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c | 5 + .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 10 files changed, 747 insertions(+), 36 deletions(-)
amdgpu_dc_rreg and amdgpu_dc_wreg are very similar, for this reason, this commits abstract these two events by using DECLARE_EVENT_CLASS and create an instance of it for each one of these events.
Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com --- .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 55 ++++++++----------- 1 file changed, 24 insertions(+), 31 deletions(-)
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 index d898981684d5..dd34e11b1079 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -31,40 +31,33 @@
#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) -); +DECLARE_EVENT_CLASS(amdgpu_dc_reg_template, + TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value), + TP_ARGS(count, reg, 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) + TP_STRUCT__entry( + __field(uint32_t, reg) + __field(uint32_t, value) + ), + + TP_fast_assign( + __entry->reg = reg; + __entry->value = value; + *count = *count + 1; + ), + + TP_printk("reg=0x%08lx, value=0x%08lx", + (unsigned long)__entry->reg, + (unsigned long)__entry->value) );
+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_rreg, + TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value), + TP_ARGS(count, reg, value)); + +DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_wreg, + TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value), + TP_ARGS(count, reg, value));
TRACE_EVENT(amdgpu_dc_performance, TP_PROTO(unsigned long read_count, unsigned long write_count,
On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:
amdgpu_dc_rreg and amdgpu_dc_wreg are very similar, for this reason, this commits abstract these two events by using DECLARE_EVENT_CLASS and create an instance of it for each one of these events.
Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com
This looks reasonable to me. Does this still show up as amdpgu_dc_rrreg/amdgpu_dc_wreg in the captured trace log?
As long as we can still tell this apart you can consider this patch:
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Regards, Nicholas Kazlauskas
.../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 55 ++++++++----------- 1 file changed, 24 insertions(+), 31 deletions(-)
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 index d898981684d5..dd34e11b1079 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -31,40 +31,33 @@
#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)-); +DECLARE_EVENT_CLASS(amdgpu_dc_reg_template,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, 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)
TP_STRUCT__entry(__field(uint32_t, reg)__field(uint32_t, value)),TP_fast_assign(__entry->reg = reg;__entry->value = value;*count = *count + 1;),TP_printk("reg=0x%08lx, value=0x%08lx",(unsigned long)__entry->reg,(unsigned long)__entry->value)+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_rreg,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, value));+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_wreg,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, value));TRACE_EVENT(amdgpu_dc_performance, TP_PROTO(unsigned long read_count, unsigned long write_count,
On 09/11, Kazlauskas, Nicholas wrote:
On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:
amdgpu_dc_rreg and amdgpu_dc_wreg are very similar, for this reason, this commits abstract these two events by using DECLARE_EVENT_CLASS and create an instance of it for each one of these events.
Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com
This looks reasonable to me. Does this still show up as amdpgu_dc_rrreg/amdgpu_dc_wreg in the captured trace log?
As long as we can still tell this apart you can consider this patch:
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Yes, this change does not change anything from the user perspective.
Thanks
Regards, Nicholas Kazlauskas
.../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 55 ++++++++----------- 1 file changed, 24 insertions(+), 31 deletions(-)
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 index d898981684d5..dd34e11b1079 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -31,40 +31,33 @@ #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)-); +DECLARE_EVENT_CLASS(amdgpu_dc_reg_template,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, 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)
TP_STRUCT__entry(__field(uint32_t, reg)__field(uint32_t, value)),TP_fast_assign(__entry->reg = reg;__entry->value = value;*count = *count + 1;),TP_printk("reg=0x%08lx, value=0x%08lx",(unsigned long)__entry->reg,(unsigned long)__entry->value)+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_rreg,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, value));+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_wreg,
TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),TP_ARGS(count, reg, value));
Debug amdgpu_dm could be a complicated task, therefore, this commit adds tracepoints in some convenient functions such as plane and connector check inside amdgpu_dm.
Co-developed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 ++ .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 287 ++++++++++++++++++ 2 files changed, 304 insertions(+)
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 cb624ee70545..552ca67c2a71 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5424,6 +5424,8 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn, struct drm_crtc_state *new_crtc_state; int ret;
+ trace_amdgpu_dm_connector_atomic_check(new_con_state); + if (!crtc) return 0;
@@ -5542,6 +5544,8 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state); int ret = -EINVAL;
+ trace_amdgpu_dm_crtc_atomic_check(state); + dm_update_crtc_active_planes(crtc, state);
if (unlikely(!dm_crtc_state->stream && @@ -5916,6 +5920,8 @@ static int dm_plane_atomic_check(struct drm_plane *plane, struct drm_crtc_state *new_crtc_state; int ret;
+ trace_amdgpu_dm_plane_atomic_check(state); + dm_plane_state = to_dm_plane_state(state);
if (!dm_plane_state->dc_state) @@ -5956,6 +5962,8 @@ static void dm_plane_atomic_async_update(struct drm_plane *plane, struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(new_state->state, plane);
+ trace_amdgpu_dm_atomic_update_cursor(new_state); + swap(plane->state->fb, new_state->fb);
plane->state->src_x = new_state->src_x; @@ -7546,6 +7554,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) int crtc_disable_count = 0; bool mode_set_reset_required = false;
+ trace_amdgpu_dm_atomic_commit_tail_begin(state); + drm_atomic_helper_update_legacy_modeset_state(dev, state);
dm_state = dm_atomic_get_new_state(state); @@ -8616,6 +8626,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, int ret, i; bool lock_and_validation_needed = false;
+ trace_amdgpu_dm_atomic_check_begin(state); + ret = drm_atomic_helper_check_modeset(dev, state); if (ret) goto fail; @@ -8912,6 +8924,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
/* Must be success */ WARN_ON(ret); + + trace_amdgpu_dm_atomic_check_finish(state, ret); + return ret;
fail: @@ -8922,6 +8937,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, else DRM_DEBUG_DRIVER("Atomic check failed with err: %d \n", ret);
+ trace_amdgpu_dm_atomic_check_finish(state, ret); + return ret; }
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 index dd34e11b1079..5fb4c4a5c349 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -30,6 +30,12 @@ #define _AMDGPU_DM_TRACE_H_
#include <linux/tracepoint.h> +#include <drm/drm_connector.h> +#include <drm/drm_crtc.h> +#include <drm/drm_plane.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_encoder.h> +#include <drm/drm_atomic.h>
DECLARE_EVENT_CLASS(amdgpu_dc_reg_template, TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value), @@ -89,6 +95,287 @@ TRACE_EVENT(amdgpu_dc_performance, (unsigned long)__entry->write_delta, (unsigned long)__entry->writes) ); + +TRACE_EVENT(amdgpu_dm_connector_atomic_check, + TP_PROTO(const struct drm_connector_state *state), + TP_ARGS(state), + + TP_STRUCT__entry( + __field(uint32_t, conn_id) + __field(const struct drm_connector_state *, conn_state) + __field(const struct drm_atomic_state *, state) + __field(const struct drm_crtc_commit *, commit) + __field(uint32_t, crtc_id) + __field(uint32_t, best_encoder_id) + __field(enum drm_link_status, link_status) + __field(bool, self_refresh_aware) + __field(enum hdmi_picture_aspect, picture_aspect_ratio) + __field(unsigned int, content_type) + __field(unsigned int, hdcp_content_type) + __field(unsigned int, content_protection) + __field(unsigned int, scaling_mode) + __field(u32, colorspace) + __field(u8, max_requested_bpc) + __field(u8, max_bpc) + ), + + TP_fast_assign( + __entry->conn_id = state->connector->base.id; + __entry->conn_state = state; + __entry->state = state->state; + __entry->commit = state->commit; + __entry->crtc_id = state->crtc ? state->crtc->base.id : 0; + __entry->best_encoder_id = state->best_encoder ? + state->best_encoder->base.id : 0; + __entry->link_status = state->link_status; + __entry->self_refresh_aware = state->self_refresh_aware; + __entry->picture_aspect_ratio = state->picture_aspect_ratio; + __entry->content_type = state->content_type; + __entry->hdcp_content_type = state->hdcp_content_type; + __entry->content_protection = state->content_protection; + __entry->scaling_mode = state->scaling_mode; + __entry->colorspace = state->colorspace; + __entry->max_requested_bpc = state->max_requested_bpc; + __entry->max_bpc = state->max_bpc; + ), + + TP_printk("conn_id=%u conn_state=%p state=%p commit=%p crtc_id=%u " + "best_encoder_id=%u link_status=%d self_refresh_aware=%d " + "picture_aspect_ratio=%d content_type=%u " + "hdcp_content_type=%u content_protection=%u scaling_mode=%u " + "colorspace=%u max_requested_bpc=%u max_bpc=%u", + __entry->conn_id, __entry->conn_state, __entry->state, + __entry->commit, __entry->crtc_id, __entry->best_encoder_id, + __entry->link_status, __entry->self_refresh_aware, + __entry->picture_aspect_ratio, __entry->content_type, + __entry->hdcp_content_type, __entry->content_protection, + __entry->scaling_mode, __entry->colorspace, + __entry->max_requested_bpc, __entry->max_bpc) +); + +TRACE_EVENT(amdgpu_dm_crtc_atomic_check, + TP_PROTO(const struct drm_crtc_state *state), + TP_ARGS(state), + + TP_STRUCT__entry( + __field(const struct drm_atomic_state *, state) + __field(const struct drm_crtc_state *, crtc_state) + __field(const struct drm_crtc_commit *, commit) + __field(uint32_t, crtc_id) + __field(bool, enable) + __field(bool, active) + __field(bool, planes_changed) + __field(bool, mode_changed) + __field(bool, active_changed) + __field(bool, connectors_changed) + __field(bool, zpos_changed) + __field(bool, color_mgmt_changed) + __field(bool, no_vblank) + __field(bool, async_flip) + __field(bool, vrr_enabled) + __field(bool, self_refresh_active) + __field(u32, plane_mask) + __field(u32, connector_mask) + __field(u32, encoder_mask) + ), + + TP_fast_assign( + __entry->state = state->state; + __entry->crtc_state = state; + __entry->crtc_id = state->crtc->base.id; + __entry->commit = state->commit; + __entry->enable = state->enable; + __entry->active = state->active; + __entry->planes_changed = state->planes_changed; + __entry->mode_changed = state->mode_changed; + __entry->active_changed = state->active_changed; + __entry->connectors_changed = state->connectors_changed; + __entry->zpos_changed = state->zpos_changed; + __entry->color_mgmt_changed = state->color_mgmt_changed; + __entry->no_vblank = state->no_vblank; + __entry->async_flip = state->async_flip; + __entry->vrr_enabled = state->vrr_enabled; + __entry->self_refresh_active = state->self_refresh_active; + __entry->plane_mask = state->plane_mask; + __entry->connector_mask = state->connector_mask; + __entry->encoder_mask = state->encoder_mask; + ), + + TP_printk("crtc_id=%u crtc_state=%p state=%p commit=%p changed(" + "planes=%d mode=%d active=%d conn=%d zpos=%d color_mgmt=%d) " + "state(enable=%d active=%d async_flip=%d vrr_enabled=%d " + "self_refresh_active=%d no_vblank=%d) mask(plane=%x conn=%x " + "enc=%x)", + __entry->crtc_id, __entry->crtc_state, __entry->state, + __entry->commit, __entry->planes_changed, + __entry->mode_changed, __entry->active_changed, + __entry->connectors_changed, __entry->zpos_changed, + __entry->color_mgmt_changed, __entry->enable, __entry->active, + __entry->async_flip, __entry->vrr_enabled, + __entry->self_refresh_active, __entry->no_vblank, + __entry->plane_mask, __entry->connector_mask, + __entry->encoder_mask) +); + +DECLARE_EVENT_CLASS(amdgpu_dm_plane_state_template, + TP_PROTO(const struct drm_plane_state *state), + TP_ARGS(state), + TP_STRUCT__entry( + __field(uint32_t, plane_id) + __field(enum drm_plane_type, plane_type) + __field(const struct drm_plane_state *, plane_state) + __field(const struct drm_atomic_state *, state) + __field(uint32_t, crtc_id) + __field(uint32_t, fb_id) + __field(uint32_t, fb_format) + __field(uint8_t, fb_planes) + __field(uint64_t, fb_modifier) + __field(const struct dma_fence *, fence) + __field(int32_t, crtc_x) + __field(int32_t, crtc_y) + __field(uint32_t, crtc_w) + __field(uint32_t, crtc_h) + __field(uint32_t, src_x) + __field(uint32_t, src_y) + __field(uint32_t, src_w) + __field(uint32_t, src_h) + __field(u32, alpha) + __field(uint32_t, pixel_blend_mode) + __field(unsigned int, rotation) + __field(unsigned int, zpos) + __field(unsigned int, normalized_zpos) + __field(enum drm_color_encoding, color_encoding) + __field(enum drm_color_range, color_range) + __field(bool, visible) + ), + + TP_fast_assign( + __entry->plane_id = state->plane->base.id; + __entry->plane_type = state->plane->type; + __entry->plane_state = state; + __entry->state = state->state; + __entry->crtc_id = state->crtc ? state->crtc->base.id : 0; + __entry->fb_id = state->fb ? state->fb->base.id : 0; + __entry->fb_format = state->fb ? state->fb->format->format : 0; + __entry->fb_planes = state->fb ? state->fb->format->num_planes : 0; + __entry->fb_modifier = state->fb ? state->fb->modifier : 0; + __entry->fence = state->fence; + __entry->crtc_x = state->crtc_x; + __entry->crtc_y = state->crtc_y; + __entry->crtc_w = state->crtc_w; + __entry->crtc_h = state->crtc_h; + __entry->src_x = state->src_x >> 16; + __entry->src_y = state->src_y >> 16; + __entry->src_w = state->src_w >> 16; + __entry->src_h = state->src_h >> 16; + __entry->alpha = state->alpha; + __entry->pixel_blend_mode = state->pixel_blend_mode; + __entry->rotation = state->rotation; + __entry->zpos = state->zpos; + __entry->normalized_zpos = state->normalized_zpos; + __entry->color_encoding = state->color_encoding; + __entry->color_range = state->color_range; + __entry->visible = state->visible; + ), + + TP_printk("plane_id=%u plane_type=%d plane_state=%p state=%p " + "crtc_id=%u fb(id=%u fmt=%c%c%c%c planes=%u mod=%llu) " + "fence=%p crtc_x=%d crtc_y=%d crtc_w=%u crtc_h=%u " + "src_x=%u src_y=%u src_w=%u src_h=%u alpha=%u " + "pixel_blend_mode=%u rotation=%u zpos=%u " + "normalized_zpos=%u color_encoding=%d color_range=%d " + "visible=%d", + __entry->plane_id, __entry->plane_type, __entry->plane_state, + __entry->state, __entry->crtc_id, __entry->fb_id, + (__entry->fb_format & 0xff) ? (__entry->fb_format & 0xff) : 'N', + ((__entry->fb_format >> 8) & 0xff) ? ((__entry->fb_format >> 8) & 0xff) : 'O', + ((__entry->fb_format >> 16) & 0xff) ? ((__entry->fb_format >> 16) & 0xff) : 'N', + ((__entry->fb_format >> 24) & 0x7f) ? ((__entry->fb_format >> 24) & 0x7f) : 'E', + __entry->fb_planes, + __entry->fb_modifier, __entry->fence, __entry->crtc_x, + __entry->crtc_y, __entry->crtc_w, __entry->crtc_h, + __entry->src_x, __entry->src_y, __entry->src_w, __entry->src_h, + __entry->alpha, __entry->pixel_blend_mode, __entry->rotation, + __entry->zpos, __entry->normalized_zpos, + __entry->color_encoding, __entry->color_range, + __entry->visible) +); + +DEFINE_EVENT(amdgpu_dm_plane_state_template, amdgpu_dm_plane_atomic_check, + TP_PROTO(const struct drm_plane_state *state), + TP_ARGS(state)); + +DEFINE_EVENT(amdgpu_dm_plane_state_template, amdgpu_dm_atomic_update_cursor, + TP_PROTO(const struct drm_plane_state *state), + TP_ARGS(state)); + +TRACE_EVENT(amdgpu_dm_atomic_state_template, + TP_PROTO(const struct drm_atomic_state *state), + TP_ARGS(state), + + TP_STRUCT__entry( + __field(const struct drm_atomic_state *, state) + __field(bool, allow_modeset) + __field(bool, legacy_cursor_update) + __field(bool, async_update) + __field(bool, duplicated) + __field(int, num_connector) + __field(int, num_private_objs) + ), + + TP_fast_assign( + __entry->state = state; + __entry->allow_modeset = state->allow_modeset; + __entry->legacy_cursor_update = state->legacy_cursor_update; + __entry->async_update = state->async_update; + __entry->duplicated = state->duplicated; + __entry->num_connector = state->num_connector; + __entry->num_private_objs = state->num_private_objs; + ), + + TP_printk("state=%p allow_modeset=%d legacy_cursor_update=%d " + "async_update=%d duplicated=%d num_connector=%d " + "num_private_objs=%d", + __entry->state, __entry->allow_modeset, __entry->legacy_cursor_update, + __entry->async_update, __entry->duplicated, __entry->num_connector, + __entry->num_private_objs) +); + +DEFINE_EVENT(amdgpu_dm_atomic_state_template, amdgpu_dm_atomic_commit_tail_begin, + TP_PROTO(const struct drm_atomic_state *state), + TP_ARGS(state)); + +DEFINE_EVENT(amdgpu_dm_atomic_state_template, amdgpu_dm_atomic_commit_tail_finish, + TP_PROTO(const struct drm_atomic_state *state), + TP_ARGS(state)); + +DEFINE_EVENT(amdgpu_dm_atomic_state_template, amdgpu_dm_atomic_check_begin, + TP_PROTO(const struct drm_atomic_state *state), + TP_ARGS(state)); + +TRACE_EVENT(amdgpu_dm_atomic_check_finish, + TP_PROTO(const struct drm_atomic_state *state, int res), + TP_ARGS(state, res), + + TP_STRUCT__entry( + __field(const struct drm_atomic_state *, state) + __field(int, res) + __field(bool, async_update) + __field(bool, allow_modeset) + ), + + TP_fast_assign( + __entry->state = state; + __entry->res = res; + __entry->async_update = state->async_update; + __entry->allow_modeset = state->allow_modeset; + ), + + TP_printk("state=%p res=%d async_update=%d allow_modeset=%d", + __entry->state, __entry->res, + __entry->async_update, __entry->allow_modeset) +); + #endif /* _AMDGPU_DM_TRACE_H_ */
#undef TRACE_INCLUDE_PATH
This commit introduces a trace mechanism for struct pipe_ctx by adding a middle layer struct in the amdgpu_dm_trace.h for capturing the most important data from struct pipe_ctx and showing its data via tracepoint. This tracepoint was added to dc.c and dcn10_hw_sequencer, however, it can be added to other DCN architecture.
Co-developed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com --- .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 172 ++++++++++++++++++ drivers/gpu/drm/amd/display/dc/core/dc.c | 11 ++ .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 3 files changed, 195 insertions(+), 5 deletions(-)
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 index 5fb4c4a5c349..53f62506e17c 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -376,6 +376,178 @@ TRACE_EVENT(amdgpu_dm_atomic_check_finish, __entry->async_update, __entry->allow_modeset) );
+#ifndef _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ +#define _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ + +struct amdgpu_dm_trace_pipe_state { + int pipe_idx; + const void *stream; + int stream_w; + int stream_h; + int dst_x; + int dst_y; + int dst_w; + int dst_h; + int src_x; + int src_y; + int src_w; + int src_h; + int clip_x; + int clip_y; + int clip_w; + int clip_h; + int recout_x; + int recout_y; + int recout_w; + int recout_h; + int viewport_x; + int viewport_y; + int viewport_w; + int viewport_h; + int flip_immediate; + int surface_pitch; + int format; + int swizzle; + unsigned int update_flags; +}; + +#define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \ + do { \ + trace_pipe_state.pipe_idx = (pipe_ctx)->pipe_idx; \ + trace_pipe_state.stream = (pipe_ctx)->stream; \ + trace_pipe_state.stream_w = (pipe_ctx)->stream->timing.h_addressable; \ + trace_pipe_state.stream_h = (pipe_ctx)->stream->timing.v_addressable; \ + trace_pipe_state.dst_x = (pipe_ctx)->plane_state->dst_rect.x; \ + trace_pipe_state.dst_y = (pipe_ctx)->plane_state->dst_rect.y; \ + trace_pipe_state.dst_w = (pipe_ctx)->plane_state->dst_rect.width; \ + trace_pipe_state.dst_h = (pipe_ctx)->plane_state->dst_rect.height; \ + trace_pipe_state.src_x = (pipe_ctx)->plane_state->src_rect.x; \ + trace_pipe_state.src_y = (pipe_ctx)->plane_state->src_rect.y; \ + trace_pipe_state.src_w = (pipe_ctx)->plane_state->src_rect.width; \ + trace_pipe_state.src_h = (pipe_ctx)->plane_state->src_rect.height; \ + trace_pipe_state.clip_x = (pipe_ctx)->plane_state->clip_rect.x; \ + trace_pipe_state.clip_y = (pipe_ctx)->plane_state->clip_rect.y; \ + trace_pipe_state.clip_w = (pipe_ctx)->plane_state->clip_rect.width; \ + trace_pipe_state.clip_h = (pipe_ctx)->plane_state->clip_rect.height; \ + trace_pipe_state.recout_x = (pipe_ctx)->plane_res.scl_data.recout.x; \ + trace_pipe_state.recout_y = (pipe_ctx)->plane_res.scl_data.recout.y; \ + trace_pipe_state.recout_w = (pipe_ctx)->plane_res.scl_data.recout.width; \ + trace_pipe_state.recout_h = (pipe_ctx)->plane_res.scl_data.recout.height; \ + trace_pipe_state.viewport_x = (pipe_ctx)->plane_res.scl_data.viewport.x; \ + trace_pipe_state.viewport_y = (pipe_ctx)->plane_res.scl_data.viewport.y; \ + trace_pipe_state.viewport_w = (pipe_ctx)->plane_res.scl_data.viewport.width; \ + trace_pipe_state.viewport_h = (pipe_ctx)->plane_res.scl_data.viewport.height; \ + trace_pipe_state.flip_immediate = (pipe_ctx)->plane_state->flip_immediate; \ + trace_pipe_state.surface_pitch = (pipe_ctx)->plane_state->plane_size.surface_pitch; \ + trace_pipe_state.format = (pipe_ctx)->plane_state->format; \ + trace_pipe_state.swizzle = (pipe_ctx)->plane_state->tiling_info.gfx9.swizzle; \ + trace_pipe_state.update_flags = (pipe_ctx)->update_flags.raw; \ + } while (0) + +#endif /* _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ */ + +TRACE_EVENT(amdgpu_dm_dc_pipe_state, + TP_PROTO(const struct amdgpu_dm_trace_pipe_state *pipe_state), + TP_ARGS(pipe_state), + TP_STRUCT__entry( + __field(int, pipe_idx) + __field(const void *, stream) + __field(int, stream_w) + __field(int, stream_h) + __field(int, dst_x) + __field(int, dst_y) + __field(int, dst_w) + __field(int, dst_h) + __field(int, src_x) + __field(int, src_y) + __field(int, src_w) + __field(int, src_h) + __field(int, clip_x) + __field(int, clip_y) + __field(int, clip_w) + __field(int, clip_h) + __field(int, recout_x) + __field(int, recout_y) + __field(int, recout_w) + __field(int, recout_h) + __field(int, viewport_x) + __field(int, viewport_y) + __field(int, viewport_w) + __field(int, viewport_h) + __field(int, flip_immediate) + __field(int, surface_pitch) + __field(int, format) + __field(int, swizzle) + __field(unsigned int, update_flags) + ), + + TP_fast_assign( + __entry->pipe_idx = pipe_state->pipe_idx; + __entry->stream = pipe_state->stream; + __entry->stream_w = pipe_state->stream_w; + __entry->stream_h = pipe_state->stream_h; + __entry->dst_x = pipe_state->dst_x; + __entry->dst_y = pipe_state->dst_y; + __entry->dst_w = pipe_state->dst_w; + __entry->dst_h = pipe_state->dst_h; + __entry->src_x = pipe_state->src_x; + __entry->src_y = pipe_state->src_y; + __entry->src_w = pipe_state->src_w; + __entry->src_h = pipe_state->src_h; + __entry->clip_x = pipe_state->clip_x; + __entry->clip_y = pipe_state->clip_y; + __entry->clip_w = pipe_state->clip_w; + __entry->clip_h = pipe_state->clip_h; + __entry->recout_x = pipe_state->recout_x; + __entry->recout_y = pipe_state->recout_y; + __entry->recout_w = pipe_state->recout_w; + __entry->recout_h = pipe_state->recout_h; + __entry->viewport_x = pipe_state->viewport_x; + __entry->viewport_y = pipe_state->viewport_y; + __entry->viewport_w = pipe_state->viewport_w; + __entry->viewport_h = pipe_state->viewport_h; + __entry->flip_immediate = pipe_state->flip_immediate; + __entry->surface_pitch = pipe_state->surface_pitch; + __entry->format = pipe_state->format; + __entry->swizzle = pipe_state->swizzle; + __entry->update_flags = pipe_state->update_flags; + ), + TP_printk("pipe_idx=%d stream=%p rct(%d,%d) dst=(%d,%d,%d,%d) " + "src=(%d,%d,%d,%d) clip=(%d,%d,%d,%d) recout=(%d,%d,%d,%d) " + "viewport=(%d,%d,%d,%d) flip_immediate=%d pitch=%d " + "format=%d swizzle=%d update_flags=%x", + __entry->pipe_idx, + __entry->stream, + __entry->stream_w, + __entry->stream_h, + __entry->dst_x, + __entry->dst_y, + __entry->dst_w, + __entry->dst_h, + __entry->src_x, + __entry->src_y, + __entry->src_w, + __entry->src_h, + __entry->clip_x, + __entry->clip_y, + __entry->clip_w, + __entry->clip_h, + __entry->recout_x, + __entry->recout_y, + __entry->recout_w, + __entry->recout_h, + __entry->viewport_x, + __entry->viewport_y, + __entry->viewport_w, + __entry->viewport_h, + __entry->flip_immediate, + __entry->surface_pitch, + __entry->format, + __entry->swizzle, + __entry->update_flags + ) +); + #endif /* _AMDGPU_DM_TRACE_H_ */
#undef TRACE_INCLUDE_PATH diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index dc463d99ef50..0c9f177e5827 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2644,6 +2644,17 @@ void dc_commit_updates_for_stream(struct dc *dc, } }
+ for (i = 0; i < MAX_PIPES; ++i) { + struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i]; + + if (pipe_ctx->plane_state) { + struct amdgpu_dm_trace_pipe_state pipe_state_trace; + + fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx); + trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace); + } + } + commit_planes_for_stream( dc, srf_updates, diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c index 8ca94f506195..464d0ad093b9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c @@ -1020,15 +1020,22 @@ static bool dcn10_hw_wa_force_recovery(struct dc *dc)
}
- void dcn10_verify_allow_pstate_change_high(struct dc *dc) { - static bool should_log_hw_state; /* prevent hw state log by default */ - if (!hubbub1_verify_allow_pstate_change_high(dc->res_pool->hubbub)) { - if (should_log_hw_state) { - dcn10_log_hw_state(dc, NULL); + int i; + + for (i = 0; i < MAX_PIPES; ++i) { + struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i]; + + if (pipe_ctx->plane_state) { + struct amdgpu_dm_trace_pipe_state pipe_state_trace; + + fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx); + trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace); + } } + BREAK_TO_DEBUGGER(); if (dcn10_hw_wa_force_recovery(dc)) { /*check again*/
On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:
This commit introduces a trace mechanism for struct pipe_ctx by adding a middle layer struct in the amdgpu_dm_trace.h for capturing the most important data from struct pipe_ctx and showing its data via tracepoint. This tracepoint was added to dc.c and dcn10_hw_sequencer, however, it can be added to other DCN architecture.
Co-developed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com
This patch, while very useful, unfortunately pulls in a lot of DM code into DC so I would prefer to hold off on this one for now.
It would be better if this had a proper DC interface for tracing/logging these states. If the API was more like how we handle tracing register reads/writes this would be cleaner.
Regards, Nicholas Kazlauskas
.../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 172 ++++++++++++++++++ drivers/gpu/drm/amd/display/dc/core/dc.c | 11 ++ .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 3 files changed, 195 insertions(+), 5 deletions(-)
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 index 5fb4c4a5c349..53f62506e17c 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -376,6 +376,178 @@ TRACE_EVENT(amdgpu_dm_atomic_check_finish, __entry->async_update, __entry->allow_modeset) );
+#ifndef _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ +#define _AMDGPU_DM_TRACE_STRUCTS_DEFINED_
+struct amdgpu_dm_trace_pipe_state {
- int pipe_idx;
- const void *stream;
- int stream_w;
- int stream_h;
- int dst_x;
- int dst_y;
- int dst_w;
- int dst_h;
- int src_x;
- int src_y;
- int src_w;
- int src_h;
- int clip_x;
- int clip_y;
- int clip_w;
- int clip_h;
- int recout_x;
- int recout_y;
- int recout_w;
- int recout_h;
- int viewport_x;
- int viewport_y;
- int viewport_w;
- int viewport_h;
- int flip_immediate;
- int surface_pitch;
- int format;
- int swizzle;
- unsigned int update_flags;
+};
+#define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \
- do { \
trace_pipe_state.pipe_idx = (pipe_ctx)->pipe_idx; \trace_pipe_state.stream = (pipe_ctx)->stream; \trace_pipe_state.stream_w = (pipe_ctx)->stream->timing.h_addressable; \trace_pipe_state.stream_h = (pipe_ctx)->stream->timing.v_addressable; \trace_pipe_state.dst_x = (pipe_ctx)->plane_state->dst_rect.x; \trace_pipe_state.dst_y = (pipe_ctx)->plane_state->dst_rect.y; \trace_pipe_state.dst_w = (pipe_ctx)->plane_state->dst_rect.width; \trace_pipe_state.dst_h = (pipe_ctx)->plane_state->dst_rect.height; \trace_pipe_state.src_x = (pipe_ctx)->plane_state->src_rect.x; \trace_pipe_state.src_y = (pipe_ctx)->plane_state->src_rect.y; \trace_pipe_state.src_w = (pipe_ctx)->plane_state->src_rect.width; \trace_pipe_state.src_h = (pipe_ctx)->plane_state->src_rect.height; \trace_pipe_state.clip_x = (pipe_ctx)->plane_state->clip_rect.x; \trace_pipe_state.clip_y = (pipe_ctx)->plane_state->clip_rect.y; \trace_pipe_state.clip_w = (pipe_ctx)->plane_state->clip_rect.width; \trace_pipe_state.clip_h = (pipe_ctx)->plane_state->clip_rect.height; \trace_pipe_state.recout_x = (pipe_ctx)->plane_res.scl_data.recout.x; \trace_pipe_state.recout_y = (pipe_ctx)->plane_res.scl_data.recout.y; \trace_pipe_state.recout_w = (pipe_ctx)->plane_res.scl_data.recout.width; \trace_pipe_state.recout_h = (pipe_ctx)->plane_res.scl_data.recout.height; \trace_pipe_state.viewport_x = (pipe_ctx)->plane_res.scl_data.viewport.x; \trace_pipe_state.viewport_y = (pipe_ctx)->plane_res.scl_data.viewport.y; \trace_pipe_state.viewport_w = (pipe_ctx)->plane_res.scl_data.viewport.width; \trace_pipe_state.viewport_h = (pipe_ctx)->plane_res.scl_data.viewport.height; \trace_pipe_state.flip_immediate = (pipe_ctx)->plane_state->flip_immediate; \trace_pipe_state.surface_pitch = (pipe_ctx)->plane_state->plane_size.surface_pitch; \trace_pipe_state.format = (pipe_ctx)->plane_state->format; \trace_pipe_state.swizzle = (pipe_ctx)->plane_state->tiling_info.gfx9.swizzle; \trace_pipe_state.update_flags = (pipe_ctx)->update_flags.raw; \- } while (0)
+#endif /* _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ */
+TRACE_EVENT(amdgpu_dm_dc_pipe_state,
TP_PROTO(const struct amdgpu_dm_trace_pipe_state *pipe_state),TP_ARGS(pipe_state),TP_STRUCT__entry(__field(int, pipe_idx)__field(const void *, stream)__field(int, stream_w)__field(int, stream_h)__field(int, dst_x)__field(int, dst_y)__field(int, dst_w)__field(int, dst_h)__field(int, src_x)__field(int, src_y)__field(int, src_w)__field(int, src_h)__field(int, clip_x)__field(int, clip_y)__field(int, clip_w)__field(int, clip_h)__field(int, recout_x)__field(int, recout_y)__field(int, recout_w)__field(int, recout_h)__field(int, viewport_x)__field(int, viewport_y)__field(int, viewport_w)__field(int, viewport_h)__field(int, flip_immediate)__field(int, surface_pitch)__field(int, format)__field(int, swizzle)__field(unsigned int, update_flags)- ),
- TP_fast_assign(
__entry->pipe_idx = pipe_state->pipe_idx;__entry->stream = pipe_state->stream;__entry->stream_w = pipe_state->stream_w;__entry->stream_h = pipe_state->stream_h;__entry->dst_x = pipe_state->dst_x;__entry->dst_y = pipe_state->dst_y;__entry->dst_w = pipe_state->dst_w;__entry->dst_h = pipe_state->dst_h;__entry->src_x = pipe_state->src_x;__entry->src_y = pipe_state->src_y;__entry->src_w = pipe_state->src_w;__entry->src_h = pipe_state->src_h;__entry->clip_x = pipe_state->clip_x;__entry->clip_y = pipe_state->clip_y;__entry->clip_w = pipe_state->clip_w;__entry->clip_h = pipe_state->clip_h;__entry->recout_x = pipe_state->recout_x;__entry->recout_y = pipe_state->recout_y;__entry->recout_w = pipe_state->recout_w;__entry->recout_h = pipe_state->recout_h;__entry->viewport_x = pipe_state->viewport_x;__entry->viewport_y = pipe_state->viewport_y;__entry->viewport_w = pipe_state->viewport_w;__entry->viewport_h = pipe_state->viewport_h;__entry->flip_immediate = pipe_state->flip_immediate;__entry->surface_pitch = pipe_state->surface_pitch;__entry->format = pipe_state->format;__entry->swizzle = pipe_state->swizzle;__entry->update_flags = pipe_state->update_flags;- ),
- TP_printk("pipe_idx=%d stream=%p rct(%d,%d) dst=(%d,%d,%d,%d) "
"src=(%d,%d,%d,%d) clip=(%d,%d,%d,%d) recout=(%d,%d,%d,%d) ""viewport=(%d,%d,%d,%d) flip_immediate=%d pitch=%d ""format=%d swizzle=%d update_flags=%x",__entry->pipe_idx,__entry->stream,__entry->stream_w,__entry->stream_h,__entry->dst_x,__entry->dst_y,__entry->dst_w,__entry->dst_h,__entry->src_x,__entry->src_y,__entry->src_w,__entry->src_h,__entry->clip_x,__entry->clip_y,__entry->clip_w,__entry->clip_h,__entry->recout_x,__entry->recout_y,__entry->recout_w,__entry->recout_h,__entry->viewport_x,__entry->viewport_y,__entry->viewport_w,__entry->viewport_h,__entry->flip_immediate,__entry->surface_pitch,__entry->format,__entry->swizzle,__entry->update_flags- )
+);
#endif /* _AMDGPU_DM_TRACE_H_ */
#undef TRACE_INCLUDE_PATH
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index dc463d99ef50..0c9f177e5827 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2644,6 +2644,17 @@ void dc_commit_updates_for_stream(struct dc *dc, } }
- for (i = 0; i < MAX_PIPES; ++i) {
struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];if (pipe_ctx->plane_state) {struct amdgpu_dm_trace_pipe_state pipe_state_trace;fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);}- }
- commit_planes_for_stream( dc, srf_updates,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c index 8ca94f506195..464d0ad093b9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c @@ -1020,15 +1020,22 @@ static bool dcn10_hw_wa_force_recovery(struct dc *dc)
}
- void dcn10_verify_allow_pstate_change_high(struct dc *dc) {
- static bool should_log_hw_state; /* prevent hw state log by default */
- if (!hubbub1_verify_allow_pstate_change_high(dc->res_pool->hubbub)) {
if (should_log_hw_state) {dcn10_log_hw_state(dc, NULL);
int i;for (i = 0; i < MAX_PIPES; ++i) {struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];if (pipe_ctx->plane_state) {struct amdgpu_dm_trace_pipe_state pipe_state_trace;fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);}- BREAK_TO_DEBUGGER(); if (dcn10_hw_wa_force_recovery(dc)) { /*check again*/
On 09/11, Kazlauskas, Nicholas wrote:
On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:
This commit introduces a trace mechanism for struct pipe_ctx by adding a middle layer struct in the amdgpu_dm_trace.h for capturing the most important data from struct pipe_ctx and showing its data via tracepoint. This tracepoint was added to dc.c and dcn10_hw_sequencer, however, it can be added to other DCN architecture.
Co-developed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com
This patch, while very useful, unfortunately pulls in a lot of DM code into DC so I would prefer to hold off on this one for now.
Hi Nicholas, first of all, thanks for your feedback.
By "pulls in a lot of DM code into DC" do you mean all references to plane_state and plane_res? Or the data inserted in the struct?
It would be better if this had a proper DC interface for tracing/logging these states. If the API was more like how we handle tracing register reads/writes this would be cleaner.
Could you elaborate a little bit more about "a proper DC interface"? What is your view of this sort of interface?
Also, how about Patch 04? Same problems?
Best Regards Rodrigo Siqueira
Regards, Nicholas Kazlauskas
.../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 172 ++++++++++++++++++ drivers/gpu/drm/amd/display/dc/core/dc.c | 11 ++ .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 3 files changed, 195 insertions(+), 5 deletions(-)
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 index 5fb4c4a5c349..53f62506e17c 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -376,6 +376,178 @@ TRACE_EVENT(amdgpu_dm_atomic_check_finish, __entry->async_update, __entry->allow_modeset) ); +#ifndef _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ +#define _AMDGPU_DM_TRACE_STRUCTS_DEFINED_
+struct amdgpu_dm_trace_pipe_state {
- int pipe_idx;
- const void *stream;
- int stream_w;
- int stream_h;
- int dst_x;
- int dst_y;
- int dst_w;
- int dst_h;
- int src_x;
- int src_y;
- int src_w;
- int src_h;
- int clip_x;
- int clip_y;
- int clip_w;
- int clip_h;
- int recout_x;
- int recout_y;
- int recout_w;
- int recout_h;
- int viewport_x;
- int viewport_y;
- int viewport_w;
- int viewport_h;
- int flip_immediate;
- int surface_pitch;
- int format;
- int swizzle;
- unsigned int update_flags;
+};
+#define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \
- do { \
trace_pipe_state.pipe_idx = (pipe_ctx)->pipe_idx; \trace_pipe_state.stream = (pipe_ctx)->stream; \trace_pipe_state.stream_w = (pipe_ctx)->stream->timing.h_addressable; \trace_pipe_state.stream_h = (pipe_ctx)->stream->timing.v_addressable; \trace_pipe_state.dst_x = (pipe_ctx)->plane_state->dst_rect.x; \trace_pipe_state.dst_y = (pipe_ctx)->plane_state->dst_rect.y; \trace_pipe_state.dst_w = (pipe_ctx)->plane_state->dst_rect.width; \trace_pipe_state.dst_h = (pipe_ctx)->plane_state->dst_rect.height; \trace_pipe_state.src_x = (pipe_ctx)->plane_state->src_rect.x; \trace_pipe_state.src_y = (pipe_ctx)->plane_state->src_rect.y; \trace_pipe_state.src_w = (pipe_ctx)->plane_state->src_rect.width; \trace_pipe_state.src_h = (pipe_ctx)->plane_state->src_rect.height; \trace_pipe_state.clip_x = (pipe_ctx)->plane_state->clip_rect.x; \trace_pipe_state.clip_y = (pipe_ctx)->plane_state->clip_rect.y; \trace_pipe_state.clip_w = (pipe_ctx)->plane_state->clip_rect.width; \trace_pipe_state.clip_h = (pipe_ctx)->plane_state->clip_rect.height; \trace_pipe_state.recout_x = (pipe_ctx)->plane_res.scl_data.recout.x; \trace_pipe_state.recout_y = (pipe_ctx)->plane_res.scl_data.recout.y; \trace_pipe_state.recout_w = (pipe_ctx)->plane_res.scl_data.recout.width; \trace_pipe_state.recout_h = (pipe_ctx)->plane_res.scl_data.recout.height; \trace_pipe_state.viewport_x = (pipe_ctx)->plane_res.scl_data.viewport.x; \trace_pipe_state.viewport_y = (pipe_ctx)->plane_res.scl_data.viewport.y; \trace_pipe_state.viewport_w = (pipe_ctx)->plane_res.scl_data.viewport.width; \trace_pipe_state.viewport_h = (pipe_ctx)->plane_res.scl_data.viewport.height; \trace_pipe_state.flip_immediate = (pipe_ctx)->plane_state->flip_immediate; \trace_pipe_state.surface_pitch = (pipe_ctx)->plane_state->plane_size.surface_pitch; \trace_pipe_state.format = (pipe_ctx)->plane_state->format; \trace_pipe_state.swizzle = (pipe_ctx)->plane_state->tiling_info.gfx9.swizzle; \trace_pipe_state.update_flags = (pipe_ctx)->update_flags.raw; \- } while (0)
+#endif /* _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ */
+TRACE_EVENT(amdgpu_dm_dc_pipe_state,
TP_PROTO(const struct amdgpu_dm_trace_pipe_state *pipe_state),TP_ARGS(pipe_state),TP_STRUCT__entry(__field(int, pipe_idx)__field(const void *, stream)__field(int, stream_w)__field(int, stream_h)__field(int, dst_x)__field(int, dst_y)__field(int, dst_w)__field(int, dst_h)__field(int, src_x)__field(int, src_y)__field(int, src_w)__field(int, src_h)__field(int, clip_x)__field(int, clip_y)__field(int, clip_w)__field(int, clip_h)__field(int, recout_x)__field(int, recout_y)__field(int, recout_w)__field(int, recout_h)__field(int, viewport_x)__field(int, viewport_y)__field(int, viewport_w)__field(int, viewport_h)__field(int, flip_immediate)__field(int, surface_pitch)__field(int, format)__field(int, swizzle)__field(unsigned int, update_flags)- ),
- TP_fast_assign(
__entry->pipe_idx = pipe_state->pipe_idx;__entry->stream = pipe_state->stream;__entry->stream_w = pipe_state->stream_w;__entry->stream_h = pipe_state->stream_h;__entry->dst_x = pipe_state->dst_x;__entry->dst_y = pipe_state->dst_y;__entry->dst_w = pipe_state->dst_w;__entry->dst_h = pipe_state->dst_h;__entry->src_x = pipe_state->src_x;__entry->src_y = pipe_state->src_y;__entry->src_w = pipe_state->src_w;__entry->src_h = pipe_state->src_h;__entry->clip_x = pipe_state->clip_x;__entry->clip_y = pipe_state->clip_y;__entry->clip_w = pipe_state->clip_w;__entry->clip_h = pipe_state->clip_h;__entry->recout_x = pipe_state->recout_x;__entry->recout_y = pipe_state->recout_y;__entry->recout_w = pipe_state->recout_w;__entry->recout_h = pipe_state->recout_h;__entry->viewport_x = pipe_state->viewport_x;__entry->viewport_y = pipe_state->viewport_y;__entry->viewport_w = pipe_state->viewport_w;__entry->viewport_h = pipe_state->viewport_h;__entry->flip_immediate = pipe_state->flip_immediate;__entry->surface_pitch = pipe_state->surface_pitch;__entry->format = pipe_state->format;__entry->swizzle = pipe_state->swizzle;__entry->update_flags = pipe_state->update_flags;- ),
- TP_printk("pipe_idx=%d stream=%p rct(%d,%d) dst=(%d,%d,%d,%d) "
"src=(%d,%d,%d,%d) clip=(%d,%d,%d,%d) recout=(%d,%d,%d,%d) ""viewport=(%d,%d,%d,%d) flip_immediate=%d pitch=%d ""format=%d swizzle=%d update_flags=%x",__entry->pipe_idx,__entry->stream,__entry->stream_w,__entry->stream_h,__entry->dst_x,__entry->dst_y,__entry->dst_w,__entry->dst_h,__entry->src_x,__entry->src_y,__entry->src_w,__entry->src_h,__entry->clip_x,__entry->clip_y,__entry->clip_w,__entry->clip_h,__entry->recout_x,__entry->recout_y,__entry->recout_w,__entry->recout_h,__entry->viewport_x,__entry->viewport_y,__entry->viewport_w,__entry->viewport_h,__entry->flip_immediate,__entry->surface_pitch,__entry->format,__entry->swizzle,__entry->update_flags- )
+);
- #endif /* _AMDGPU_DM_TRACE_H_ */ #undef TRACE_INCLUDE_PATH
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index dc463d99ef50..0c9f177e5827 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2644,6 +2644,17 @@ void dc_commit_updates_for_stream(struct dc *dc, } }
- for (i = 0; i < MAX_PIPES; ++i) {
struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];if (pipe_ctx->plane_state) {struct amdgpu_dm_trace_pipe_state pipe_state_trace;fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);}- }
- commit_planes_for_stream( dc, srf_updates,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c index 8ca94f506195..464d0ad093b9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c @@ -1020,15 +1020,22 @@ static bool dcn10_hw_wa_force_recovery(struct dc *dc) }
- void dcn10_verify_allow_pstate_change_high(struct dc *dc) {
- static bool should_log_hw_state; /* prevent hw state log by default */
- if (!hubbub1_verify_allow_pstate_change_high(dc->res_pool->hubbub)) {
if (should_log_hw_state) {dcn10_log_hw_state(dc, NULL);
int i;for (i = 0; i < MAX_PIPES; ++i) {struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];if (pipe_ctx->plane_state) {struct amdgpu_dm_trace_pipe_state pipe_state_trace;fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);}- BREAK_TO_DEBUGGER(); if (dcn10_hw_wa_force_recovery(dc)) { /*check again*/
The clock state update is the source of many problems, and capturing this sort of information helps debug. This commit introduces tracepoints for capturing clock values and also add traces in DCE, DCN1, DCN2x, and DCN3.
Co-developed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com --- .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 198 ++++++++++++++++++ .../dc/clk_mgr/dce112/dce112_clk_mgr.c | 5 + .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 4 + .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 4 + .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c | 5 + 7 files changed, 224 insertions(+)
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 index 53f62506e17c..fb22b233224a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h @@ -411,6 +411,42 @@ struct amdgpu_dm_trace_pipe_state { unsigned int update_flags; };
+struct amdgpu_dm_trace_dc_clocks_state { + int dispclk_khz; + int dppclk_khz; + int disp_dpp_voltage_level_khz; + int dcfclk_khz; + int socclk_khz; + int dcfclk_deep_sleep_khz; + int fclk_khz; + int phyclk_khz; + int dramclk_khz; + bool p_state_change_support; + int pwr_state; + bool prev_p_state_change_support; + int dtm_level; + int max_supported_dppclk_khz; + int max_supported_dispclk_khz; + int bw_dppclk_khz; + int bw_dispclk_khz; + int safe_to_lower; +}; + +struct amdgpu_dm_trace_dce_clocks_state { + bool cpuc_state_change_enable; + bool cpup_state_change_enable; + bool stutter_mode_enable; + bool nbp_state_change_enable; + bool all_displays_in_sync; + int sclk_khz; + int sclk_deep_sleep_khz; + int yclk_khz; + int dispclk_khz; + int blackout_recovery_time_us; + int patched_disp_clk; + int safe_to_lower; +}; + #define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \ do { \ trace_pipe_state.pipe_idx = (pipe_ctx)->pipe_idx; \ @@ -444,6 +480,44 @@ struct amdgpu_dm_trace_pipe_state { trace_pipe_state.update_flags = (pipe_ctx)->update_flags.raw; \ } while (0)
+#define fill_out_trace_clock_state(trace_clock_state, clocks, safe_to_lower) \ + do { \ + trace_clock_state.dispclk_khz = (clocks)->dispclk_khz; \ + trace_clock_state.dppclk_khz = (clocks)->dppclk_khz; \ + trace_clock_state.disp_dpp_voltage_level_khz = (clocks)->disp_dpp_voltage_level_khz; \ + trace_clock_state.dcfclk_khz = (clocks)->dcfclk_khz; \ + trace_clock_state.socclk_khz = (clocks)->socclk_khz; \ + trace_clock_state.dcfclk_deep_sleep_khz = (clocks)->dcfclk_deep_sleep_khz; \ + trace_clock_state.fclk_khz = (clocks)->fclk_khz; \ + trace_clock_state.phyclk_khz = (clocks)->phyclk_khz; \ + trace_clock_state.dramclk_khz = (clocks)->dramclk_khz; \ + trace_clock_state.p_state_change_support = (clocks)->p_state_change_support; \ + trace_clock_state.pwr_state = (clocks)->pwr_state; \ + trace_clock_state.prev_p_state_change_support = (clocks)->prev_p_state_change_support; \ + trace_clock_state.dtm_level = (clocks)->dtm_level; \ + trace_clock_state.max_supported_dppclk_khz = (clocks)->max_supported_dppclk_khz; \ + trace_clock_state.max_supported_dispclk_khz = (clocks)->max_supported_dispclk_khz; \ + trace_clock_state.bw_dppclk_khz = (clocks)->bw_dppclk_khz; \ + trace_clock_state.bw_dispclk_khz = (clocks)->bw_dispclk_khz; \ + trace_clock_state.safe_to_lower = safe_to_lower; \ + } while (0) + +#define fill_out_trace_dce_clock_state(trace_clock_state, clocks, patched_disp_clk, safe_to_lower) \ + do { \ + trace_clock_state.cpuc_state_change_enable = (clocks)->cpuc_state_change_enable; \ + trace_clock_state.cpup_state_change_enable = (clocks)->cpup_state_change_enable; \ + trace_clock_state.stutter_mode_enable = (clocks)->stutter_mode_enable; \ + trace_clock_state.nbp_state_change_enable = (clocks)->nbp_state_change_enable; \ + trace_clock_state.all_displays_in_sync = (clocks)->all_displays_in_sync; \ + trace_clock_state.sclk_khz = (clocks)->sclk_khz; \ + trace_clock_state.sclk_deep_sleep_khz = (clocks)->sclk_deep_sleep_khz; \ + trace_clock_state.yclk_khz = (clocks)->yclk_khz; \ + trace_clock_state.dispclk_khz = (clocks)->dispclk_khz; \ + trace_clock_state.blackout_recovery_time_us = (clocks)->blackout_recovery_time_us; \ + trace_clock_state.patched_disp_clk = patched_disp_clk; \ + trace_clock_state.safe_to_lower = safe_to_lower; \ + } while (0) + #endif /* _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ */
TRACE_EVENT(amdgpu_dm_dc_pipe_state, @@ -548,6 +622,130 @@ TRACE_EVENT(amdgpu_dm_dc_pipe_state, ) );
+TRACE_EVENT(amdgpu_dm_dc_clocks_state, + TP_PROTO(const struct amdgpu_dm_trace_dc_clocks_state *clk), + TP_ARGS(clk), + + TP_STRUCT__entry( + __field(int, dispclk_khz) + __field(int, dppclk_khz) + __field(int, disp_dpp_voltage_level_khz) + __field(int, dcfclk_khz) + __field(int, socclk_khz) + __field(int, dcfclk_deep_sleep_khz) + __field(int, fclk_khz) + __field(int, phyclk_khz) + __field(int, dramclk_khz) + __field(int, p_state_change_support) + __field(int, prev_p_state_change_support) + __field(int, pwr_state) + __field(int, dtm_level) + __field(int, max_supported_dppclk_khz) + __field(int, max_supported_dispclk_khz) + __field(int, bw_dppclk_khz) + __field(int, bw_dispclk_khz) + __field(bool, safe_to_lower) + ), + TP_fast_assign( + __entry->dispclk_khz = clk->dispclk_khz; + __entry->dppclk_khz = clk->dppclk_khz; + __entry->dcfclk_khz = clk->dcfclk_khz; + __entry->socclk_khz = clk->socclk_khz; + __entry->dcfclk_deep_sleep_khz = clk->dcfclk_deep_sleep_khz; + __entry->fclk_khz = clk->fclk_khz; + __entry->phyclk_khz = clk->phyclk_khz; + __entry->dramclk_khz = clk->dramclk_khz; + __entry->p_state_change_support = clk->p_state_change_support; + __entry->prev_p_state_change_support = clk->prev_p_state_change_support; + __entry->pwr_state = clk->pwr_state; + __entry->prev_p_state_change_support = clk->prev_p_state_change_support; + __entry->dtm_level = clk->dtm_level; + __entry->max_supported_dppclk_khz = clk->max_supported_dppclk_khz; + __entry->max_supported_dispclk_khz = clk->max_supported_dispclk_khz; + __entry->bw_dppclk_khz = clk->bw_dppclk_khz; + __entry->bw_dispclk_khz = clk->bw_dispclk_khz; + __entry->safe_to_lower = clk->safe_to_lower; + ), + TP_printk("dispclk_khz=%d dppclk_khz=%d disp_dpp_voltage_level_khz=%d dcfclk_khz=%d socclk_khz=%d " + "dcfclk_deep_sleep_khz=%d fclk_khz=%d phyclk_khz=%d " + "dramclk_khz=%d p_state_change_support=%d " + "prev_p_state_change_support=%d pwr_state=%d prev_p_state_change_support=%d " + "dtm_level=%d max_supported_dppclk_khz=%d max_supported_dispclk_khz=%d " + "bw_dppclk_khz=%d bw_dispclk_khz=%d " + "safe_to_lower=%d ", + __entry->dispclk_khz, + __entry->dppclk_khz, + __entry->disp_dpp_voltage_level_khz, + __entry->dcfclk_khz, + __entry->socclk_khz, + __entry->dcfclk_deep_sleep_khz, + __entry->fclk_khz, + __entry->phyclk_khz, + __entry->dramclk_khz, + __entry->p_state_change_support, + __entry->prev_p_state_change_support, + __entry->pwr_state, + __entry->prev_p_state_change_support, + __entry->dtm_level, + __entry->max_supported_dppclk_khz, + __entry->max_supported_dispclk_khz, + __entry->bw_dppclk_khz, + __entry->bw_dispclk_khz, + __entry->safe_to_lower + ) +); + +TRACE_EVENT(amdgpu_dm_dce_clocks_state, + TP_PROTO(const struct amdgpu_dm_trace_dce_clocks_state *clk), + TP_ARGS(clk), + + TP_STRUCT__entry( + __field(bool, cpuc_state_change_enable) + __field(bool, cpup_state_change_enable) + __field(bool, stutter_mode_enable) + __field(bool, nbp_state_change_enable) + __field(bool, all_displays_in_sync) + __field(int, sclk_khz) + __field(int, sclk_deep_sleep_khz) + __field(int, yclk_khz) + __field(int, dispclk_khz) + __field(int, blackout_recovery_time_us) + __field(int, patched_disp_clk) + __field(int, safe_to_lower) + ), + TP_fast_assign( + __entry->cpuc_state_change_enable = clk->cpuc_state_change_enable; + __entry->cpup_state_change_enable = clk->cpup_state_change_enable; + __entry->stutter_mode_enable = clk->stutter_mode_enable; + __entry->nbp_state_change_enable = clk->nbp_state_change_enable; + __entry->all_displays_in_sync = clk->all_displays_in_sync; + __entry->sclk_khz = clk->sclk_khz; + __entry->sclk_deep_sleep_khz = clk->sclk_deep_sleep_khz; + __entry->yclk_khz = clk->yclk_khz; + __entry->dispclk_khz = clk->dispclk_khz; + __entry->blackout_recovery_time_us = clk->blackout_recovery_time_us; + __entry->patched_disp_clk = clk->patched_disp_clk; + __entry->safe_to_lower = clk->safe_to_lower; + ), + TP_printk("cpuc_state_change_enable=%d cpup_state_change_enable=%d stutter_mode_enable=%d " + "nbp_state_change_enable=%d all_displays_in_sync=%d sclk_khz=%d sclk_deep_sleep_khz=%d " + "yclk_khz=%d dispclk_khz=%d blackout_recovery_time_us=%d patched_disp_clk=%d " + "safe_to_lower=%d", + __entry->cpuc_state_change_enable, + __entry->cpup_state_change_enable, + __entry->stutter_mode_enable, + __entry->nbp_state_change_enable, + __entry->all_displays_in_sync, + __entry->sclk_khz, + __entry->sclk_deep_sleep_khz, + __entry->yclk_khz, + __entry->dispclk_khz, + __entry->blackout_recovery_time_us, + __entry->patched_disp_clk, + __entry->safe_to_lower + ) +); + #endif /* _AMDGPU_DM_TRACE_H_ */
#undef TRACE_INCLUDE_PATH diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce112/dce112_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce112/dce112_clk_mgr.c index d031bd3d3072..913070efbbf4 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce112/dce112_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce112/dce112_clk_mgr.c @@ -195,8 +195,10 @@ static void dce112_update_clocks(struct clk_mgr *clk_mgr_base, bool safe_to_lower) { struct clk_mgr_internal *clk_mgr_dce = TO_CLK_MGR_INTERNAL(clk_mgr_base); + struct dce_bw_output *dce_clocks = &context->bw_ctx.bw.dce; struct dm_pp_power_level_change_request level_change_req; int patched_disp_clk = context->bw_ctx.bw.dce.dispclk_khz; + struct amdgpu_dm_trace_dce_clocks_state trace;
/*TODO: W/A for dal3 linux, investigate why this works */ if (!clk_mgr_dce->dfs_bypass_active) @@ -210,6 +212,9 @@ static void dce112_update_clocks(struct clk_mgr *clk_mgr_base, clk_mgr_dce->cur_min_clks_state = level_change_req.power_level; }
+ fill_out_trace_dce_clock_state(trace, dce_clocks, patched_disp_clk, safe_to_lower); + trace_amdgpu_dm_dce_clocks_state(&trace); + if (should_set_clock(safe_to_lower, patched_disp_clk, clk_mgr_base->clks.dispclk_khz)) { patched_disp_clk = dce112_set_clock(clk_mgr_base, patched_disp_clk); clk_mgr_base->clks.dispclk_khz = patched_disp_clk; diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c index e133edc587d3..6348c52dbb03 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c @@ -196,6 +196,7 @@ static void rv1_update_clocks(struct clk_mgr *clk_mgr_base, struct dc_debug_options *debug = &dc->debug; struct dc_clocks *new_clocks = &context->bw_ctx.bw.dcn.clk; struct pp_smu_funcs_rv *pp_smu = NULL; + struct amdgpu_dm_trace_dc_clocks_state trace; bool send_request_to_increase = false; bool send_request_to_lower = false; int display_count; @@ -211,6 +212,9 @@ static void rv1_update_clocks(struct clk_mgr *clk_mgr_base,
display_count = clk_mgr_helper_get_active_display_cnt(dc, context);
+ fill_out_trace_clock_state(trace, new_clocks, safe_to_lower); + trace_amdgpu_dm_dc_clocks_state(&trace); + if (display_count == 0) enter_display_off = true;
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c index f2114bc910bf..2d7a8250778a 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c @@ -148,6 +148,7 @@ void dcn2_update_clocks(struct clk_mgr *clk_mgr_base, { struct clk_mgr_internal *clk_mgr = TO_CLK_MGR_INTERNAL(clk_mgr_base); struct dc_clocks *new_clocks = &context->bw_ctx.bw.dcn.clk; + struct amdgpu_dm_trace_dc_clocks_state trace; struct dc *dc = clk_mgr_base->ctx->dc; struct pp_smu_funcs_nv *pp_smu = NULL; int display_count; @@ -247,6 +248,9 @@ void dcn2_update_clocks(struct clk_mgr *clk_mgr_base, pp_smu->set_voltage_by_freq(&pp_smu->pp_smu, PP_SMU_NV_DISPCLK, clk_mgr_base->clks.disp_dpp_voltage_level_khz / 1000); }
+ fill_out_trace_clock_state(trace, new_clocks, safe_to_lower); + trace_amdgpu_dm_dc_clocks_state(&trace); + if (dc->config.forced_clocks == false || (force_reset && safe_to_lower)) { if (dpp_clock_lowered) { // if clock is being lowered, increase DTO before lowering refclk diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c index 543afa34d87a..da8a4b06cc09 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c @@ -109,6 +109,7 @@ void rn_update_clocks(struct clk_mgr *clk_mgr_base, { struct clk_mgr_internal *clk_mgr = TO_CLK_MGR_INTERNAL(clk_mgr_base); struct dc_clocks *new_clocks = &context->bw_ctx.bw.dcn.clk; + struct amdgpu_dm_trace_dc_clocks_state trace; struct dc *dc = clk_mgr_base->ctx->dc; int display_count; bool update_dppclk = false; @@ -120,6 +121,9 @@ void rn_update_clocks(struct clk_mgr *clk_mgr_base, if (dc->work_arounds.skip_clock_update) return;
+ fill_out_trace_clock_state(trace, new_clocks, safe_to_lower); + trace_amdgpu_dm_dc_clocks_state(&trace); + /* * if it is safe to lower, but we are already in the lower state, we don't have to do anything * also if safe to lower is false, we just go in the higher state diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c index b0e9b0509568..5d71ddeb3c2b 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c @@ -230,6 +230,7 @@ static void dcn3_update_clocks(struct clk_mgr *clk_mgr_base, { struct clk_mgr_internal *clk_mgr = TO_CLK_MGR_INTERNAL(clk_mgr_base); struct dc_clocks *new_clocks = &context->bw_ctx.bw.dcn.clk; + struct amdgpu_dm_trace_dc_clocks_state trace; struct dc *dc = clk_mgr_base->ctx->dc; int display_count; bool update_dppclk = false; @@ -264,6 +265,9 @@ static void dcn3_update_clocks(struct clk_mgr *clk_mgr_base, new_clocks->dcfclk_khz = (new_clocks->dcfclk_khz > (dc->debug.force_min_dcfclk_mhz * 1000)) ? new_clocks->dcfclk_khz : (dc->debug.force_min_dcfclk_mhz * 1000);
+ fill_out_trace_clock_state(trace, new_clocks, safe_to_lower); + trace_amdgpu_dm_dc_clocks_state(&trace); + if (should_set_clock(safe_to_lower, new_clocks->dcfclk_khz, clk_mgr_base->clks.dcfclk_khz)) { clk_mgr_base->clks.dcfclk_khz = new_clocks->dcfclk_khz; dcn30_smu_set_hard_min_by_freq(clk_mgr, PPCLK_DCEFCLK, clk_mgr_base->clks.dcfclk_khz / 1000); diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clk_mgr.c index 29d69dfc9848..b29afee54645 100644 --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clk_mgr.c @@ -669,9 +669,11 @@ static void dce_update_clocks(struct clk_mgr *clk_mgr, struct dc_state *context, bool safe_to_lower) { + struct dce_bw_output *dce_clocks = &context->bw_ctx.bw.dce; struct dce_clk_mgr *clk_mgr_dce = TO_DCE_CLK_MGR(clk_mgr); struct dm_pp_power_level_change_request level_change_req; int patched_disp_clk = context->bw_ctx.bw.dce.dispclk_khz; + struct amdgpu_dm_trace_dce_clocks_state trace;
/*TODO: W/A for dal3 linux, investigate why this works */ if (!clk_mgr_dce->dfs_bypass_active) @@ -685,6 +687,9 @@ static void dce_update_clocks(struct clk_mgr *clk_mgr, clk_mgr_dce->cur_min_clks_state = level_change_req.power_level; }
+ fill_out_trace_dce_clock_state(trace, dce_clocks, patched_disp_clk, safe_to_lower); + trace_amdgpu_dm_dce_clocks_state(&trace); + if (should_set_clock(safe_to_lower, patched_disp_clk, clk_mgr->clks.dispclk_khz)) { patched_disp_clk = dce_set_clock(clk_mgr, patched_disp_clk); clk_mgr->clks.dispclk_khz = patched_disp_clk;
On Fri, Sep 11, 2020 at 10:59:23AM -0400, Rodrigo Siqueira wrote:
Debug issues related to display can be a challenge due to the complexity around this topic and different source of information might help in this process. We already have support for tracepoints inside the display component, i.e., we have the basic functionalities available and we just need to expand it in order to make it more valuable for debugging. For this reason, this patchset reworks part of the current tracepoint options and add different sets of tracing inside amdgpu_dm, display core, and DCN10. The first patch of this series just rework part of the current tracepoints and the last set of patches introduces new tracepoints.
This first patchset version is functional. Please, let me know what I can improve in the current version but also let me know what kind of tracepoint I can add for the next version.
Finally, I want to highlight that this work is based on a set of patches originally made by Nicholas Kazlauskas.
Change in V2:
- I added another patch for capturing the clock state for different display architecture.
Hm I'm not super sure tracepoints for state dumping are the right thing here. We kinda have the atomic state dumping code with all the various callbacks, and you can extend that pretty easily. Gives you full state dump in debugfs, plus a few function to dump into dmesg.
Maybe what we need is a function to dump this also into printk tracepoint (otoh with Sean Paul's tracepoint work we'd get that through the dmesg stuff already), and then you could do it there?
Upside is that for customers they'd get a much more consistent way to debug display issues across different drivers.
For low-level hw debug what we do is give the hw guys an mmio trace, and they replay it on the fancy boxes :-) So for that I think this here is again too high level, but maybe what you have is a bit different. -Daniel
Rodrigo Siqueira (4): drm/amd/display: Rework registers tracepoint drm/amd/display: Add tracepoint for amdgpu_dm drm/amd/display: Add pipe_state tracepoint drm/amd/display: Add tracepoint for capturing clocks state
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 + .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 712 +++++++++++++++++- .../dc/clk_mgr/dce112/dce112_clk_mgr.c | 5 + .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 4 + .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 4 + drivers/gpu/drm/amd/display/dc/core/dc.c | 11 + .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c | 5 + .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 10 files changed, 747 insertions(+), 36 deletions(-)
-- 2.28.0
On 2020-09-16 5:12 a.m., Daniel Vetter wrote:
On Fri, Sep 11, 2020 at 10:59:23AM -0400, Rodrigo Siqueira wrote:
Debug issues related to display can be a challenge due to the complexity around this topic and different source of information might help in this process. We already have support for tracepoints inside the display component, i.e., we have the basic functionalities available and we just need to expand it in order to make it more valuable for debugging. For this reason, this patchset reworks part of the current tracepoint options and add different sets of tracing inside amdgpu_dm, display core, and DCN10. The first patch of this series just rework part of the current tracepoints and the last set of patches introduces new tracepoints.
This first patchset version is functional. Please, let me know what I can improve in the current version but also let me know what kind of tracepoint I can add for the next version.
Finally, I want to highlight that this work is based on a set of patches originally made by Nicholas Kazlauskas.
Change in V2:
- I added another patch for capturing the clock state for different display architecture.
Hm I'm not super sure tracepoints for state dumping are the right thing here. We kinda have the atomic state dumping code with all the various callbacks, and you can extend that pretty easily. Gives you full state dump in debugfs, plus a few function to dump into dmesg.
Maybe what we need is a function to dump this also into printk tracepoint (otoh with Sean Paul's tracepoint work we'd get that through the dmesg stuff already), and then you could do it there?
Upside is that for customers they'd get a much more consistent way to debug display issues across different drivers.
For low-level hw debug what we do is give the hw guys an mmio trace, and they replay it on the fancy boxes :-) So for that I think this here is again too high level, but maybe what you have is a bit different. -Daniel
We have raw register traces, but what I find most useful is to be able to see are the incoming DRM IOCTLs, objects and properties per commit.
Many of the bugs we see in display code is in the conversion from DRM -> DM -> DC state. The current HW state is kind of useless in most cases, but the sequence helps track down intermittent problems and understand state transitions.
Tracepoints provide everything I really need to be able to track down these problems without falling back to a full debugger. The existing DRM prints (even at high logging levels) aren't enough to understand what's going on in most cases in our driver so funneling those into tracepoints to improve perf doesn't really help that much.
I think this kind of idea was rejected for DRM core last year with Sean's patch series but if we can't get them into core then I'd like to get them into our driver at least. These are a cleaned up version of Sean's work + my work that I end up applying locally whenever I debug something.
Regards, Nicholas Kazlauskas
Rodrigo Siqueira (4): drm/amd/display: Rework registers tracepoint drm/amd/display: Add tracepoint for amdgpu_dm drm/amd/display: Add pipe_state tracepoint drm/amd/display: Add tracepoint for capturing clocks state
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 + .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 712 +++++++++++++++++- .../dc/clk_mgr/dce112/dce112_clk_mgr.c | 5 + .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 4 + .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 4 + drivers/gpu/drm/amd/display/dc/core/dc.c | 11 + .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c | 5 + .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 10 files changed, 747 insertions(+), 36 deletions(-)
-- 2.28.0
On Wed, Sep 16, 2020 at 11:27:27AM -0400, Kazlauskas, Nicholas wrote:
On 2020-09-16 5:12 a.m., Daniel Vetter wrote:
On Fri, Sep 11, 2020 at 10:59:23AM -0400, Rodrigo Siqueira wrote:
Debug issues related to display can be a challenge due to the complexity around this topic and different source of information might help in this process. We already have support for tracepoints inside the display component, i.e., we have the basic functionalities available and we just need to expand it in order to make it more valuable for debugging. For this reason, this patchset reworks part of the current tracepoint options and add different sets of tracing inside amdgpu_dm, display core, and DCN10. The first patch of this series just rework part of the current tracepoints and the last set of patches introduces new tracepoints.
This first patchset version is functional. Please, let me know what I can improve in the current version but also let me know what kind of tracepoint I can add for the next version.
Finally, I want to highlight that this work is based on a set of patches originally made by Nicholas Kazlauskas.
Change in V2:
- I added another patch for capturing the clock state for different display architecture.
Hm I'm not super sure tracepoints for state dumping are the right thing here. We kinda have the atomic state dumping code with all the various callbacks, and you can extend that pretty easily. Gives you full state dump in debugfs, plus a few function to dump into dmesg.
Maybe what we need is a function to dump this also into printk tracepoint (otoh with Sean Paul's tracepoint work we'd get that through the dmesg stuff already), and then you could do it there?
Upside is that for customers they'd get a much more consistent way to debug display issues across different drivers.
For low-level hw debug what we do is give the hw guys an mmio trace, and they replay it on the fancy boxes :-) So for that I think this here is again too high level, but maybe what you have is a bit different. -Daniel
We have raw register traces, but what I find most useful is to be able to see are the incoming DRM IOCTLs, objects and properties per commit.
Many of the bugs we see in display code is in the conversion from DRM -> DM -> DC state. The current HW state is kind of useless in most cases, but the sequence helps track down intermittent problems and understand state transitions.
Tracepoints provide everything I really need to be able to track down these problems without falling back to a full debugger. The existing DRM prints (even at high logging levels) aren't enough to understand what's going on in most cases in our driver so funneling those into tracepoints to improve perf doesn't really help that much.
I think this kind of idea was rejected for DRM core last year with Sean's patch series but if we can't get them into core then I'd like to get them into our driver at least. These are a cleaned up version of Sean's work + my work that I end up applying locally whenever I debug something.
Nah, Sean's series wasn't rejected. It's simply stuck waiting for review. So if your goal is to get better dumping going on, I think combining this with Sean's work (and getting that reviewed), plus then tapping into the atomic state dumping code. Then you know what was requested, plus what your atomic_check code computed should be the hw state, and you can compare that with the register dumps you already grab.
Feels at least like a more complete and flexible solution than ad-hoc tracepoints for debuggin in each driver. The idea behind Sean's work is also that we'd have a blackbox recorder for any drm issues which distros in the field could use. So driver doing their own debug output doesn't sound super great.
I think Siqueira already chatted a bit with Sean. -Daniel
Regards, Nicholas Kazlauskas
Rodrigo Siqueira (4): drm/amd/display: Rework registers tracepoint drm/amd/display: Add tracepoint for amdgpu_dm drm/amd/display: Add pipe_state tracepoint drm/amd/display: Add tracepoint for capturing clocks state
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 + .../amd/display/amdgpu_dm/amdgpu_dm_trace.h | 712 +++++++++++++++++- .../dc/clk_mgr/dce112/dce112_clk_mgr.c | 5 + .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 4 + .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 4 + .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 4 + drivers/gpu/drm/amd/display/dc/core/dc.c | 11 + .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c | 5 + .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 17 +- 10 files changed, 747 insertions(+), 36 deletions(-)
-- 2.28.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Kazlauskas, Nicholas -
Rodrigo Siqueira