Am 15.09.2017 um 19:33 schrieb Harry Wentland:
On 2017-09-15 01:26 PM, Christian König wrote:Am 15.09.2017 um 17:35 schrieb Felix Kuehling:On 2017-09-15 11:28 AM, Christian König wrote:Am 15.09.2017 um 17:26 schrieb Deucher, Alexander:-----Original Message----- From: Wentland, Harry Sent: Friday, September 15, 2017 11:21 AM To: Koenig, Christian; [email protected] Cc: Deucher, Alexander; Grodzovsky, Andrey; Cheng, Tony Subject: Re: [PATCH 3/3] drm/amd/display: Reduce DC chattinessOn 2017-09-15 11:08 AM, Christian König wrote:Am 15.09.2017 um 16:32 schrieb Harry Wentland:Log DC init but default log level to 0 (default for amdgpu_dc_log) otherwise. Bug reporters can still make DC more chatty like so amdgpu.dc_log = 1Which is exactly the reason why I think this patch is superfluous. Either have a compile time option or a run time option, but please not both that's just confusing.True. Thanks for the input. Gonna leave out the run time option for now.Another option would be to tie the dc debug levels to drm debug levels.Which actually sounds like the correct solution to me. I mean we have drm debug levels for display debugging stuff for years, why do we need an extra logging for DC now?FWIW, in KFD we rely on dynamic debug messages (CONFIG_DYNAMIC_DEBUG) with pr_debug. This pretty much obsoletes any driver-specific debug messages options. And it's quite versatile because it allows us to turn on and off messages, by module, source file, function, or even by line number through debugfs. So you can be more or less specific about which messages you want to see, and they're all quiet by default. See also Documentation/admin-guide/dynamic-debug-howto.rst.Yeah, that is certainly something valueable as well. But for everything mode setting related we have this standardized DRM_DEBUG_KMS flag. We should really use this one and only add something else after thoughtful consideration. I also only realized that after Alex mentioned it.Any major objection for going with existing patches for now as an intermediate solution? I don't mind keeping only one of the runtime/compile time options.
For the short term that should work.But I agree with Alex that a module parameter is better suited than a compile time option.
And if it's just an exercise for people how to set a module option :) Christian.
This already improves the log-spam. Doing things properly will take some time to review all of our log statements, pick the right log levels, and make sure we have something workable for internal teams that everyone can sign off. That would take a least a week. HarryRegards, Christian.Regards, FelixChristian.AlexHarryChristian.Change-Id: Icdfb849fa678225e2460519fbd8066540feb451a Signed-off-by: Harry Wentland <[email protected]> --- drivers/gpu/drm/amd/display/Kconfig | 10 +++ drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 77 ++++++++++++---------- drivers/gpu/drm/amd/display/include/logger_types.h | 3 + 3 files changed, 56 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig index 1d1a5f807030..baab055dd362 100644 --- a/drivers/gpu/drm/amd/display/Kconfig +++ b/drivers/gpu/drm/amd/display/Kconfig @@ -31,4 +31,14 @@ config DEBUG_KERNEL_DC if you want to hit kdgb_break in assert. +config DRM_AMD_DC_CHATTY + bool "Make DC chatty again" + default n + depends on DRM_AMD_DC + help + Sometimes it's useful to have a chatty DC + without a ton of spam from DRM. This allows + for that and is recommended for anyone + reporting bugs to DC. + endmenu 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 abe89e3fed5b..6ecb420b2a63 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -333,7 +333,6 @@ int amdgpu_dm_init(struct amdgpu_device*adev)adev->dm.ddev = adev->ddev; adev->dm.adev = adev; - DRM_INFO("DAL is enabled\n"); /* Zero all the fields */ memset(&init_data, 0, sizeof(init_data)); @@ -373,7 +372,15 @@ int amdgpu_dm_init(struct amdgpu_device*adev)init_data.dce_environment = DCE_ENV_PRODUCTION_DRV; +#ifdef CONFIG_DRM_AMD_DC_CHATTY + /* always be chatty */ init_data.log_mask = DC_DEFAULT_LOG_MASK; +#else + if (amdgpu_dc_log) + init_data.log_mask = DC_DEFAULT_LOG_MASK; + else + init_data.log_mask = DC_MIN_LOG_MASK; +#endif #ifdef ENABLE_FBC if (adev->family == FAMILY_CZ) @@ -383,7 +390,9 @@ int amdgpu_dm_init(struct amdgpu_device*adev)/* Display Core create. */ adev->dm.dc = dc_create(&init_data); - if (!adev->dm.dc) + if (adev->dm.dc) + DRM_INFO("Display Core initialized!\n"); + else DRM_INFO("Display Core failed to initialize!\n"); INIT_WORK(&adev->dm.mst_hotplug_work,hotplug_notify_work_func);@@ -393,7 +402,7 @@ int amdgpu_dm_init(struct amdgpu_device*adev)DRM_ERROR( "amdgpu: failed to initialize freesync_module.\n"); } else - DRM_INFO("amdgpu: freesync_module init done %p.\n", + DRM_DEBUG_DRIVER("amdgpu: freesync_module init done %p.\n", adev->dm.freesync_module); if (amdgpu_dm_initialize_drm_device(adev)) { @@ -417,7 +426,7 @@ int amdgpu_dm_init(struct amdgpu_device*adev)goto error; } - DRM_INFO("KMS initialized.\n"); + DRM_DEBUG_DRIVER("KMS initialized.\n"); return 0; error: @@ -475,7 +484,7 @@ static int detect_mst_link_for_all_connectors(struct drm_device *dev) list_for_each_entry(connector, &dev->mode_config.connector_list, head) { aconnector = to_amdgpu_dm_connector(connector); if (aconnector->dc_link->type == dc_connection_mst_branch) { - DRM_INFO("DM_MST: starting TM on aconnector: %p [id: %d]\n", + DRM_DEBUG_DRIVER("DM_MST: starting TM on aconnector: %p [id: %d]\n", aconnector, aconnector->base.base.id); ret = drm_dp_mst_topology_mgr_set_mst(&aconnector->mst_mgr, true); @@ -819,12 +828,12 @@ voidamdgpu_dm_update_connector_after_detect(if (aconnector->dc_sink == sink) { /* We got a DP short pulse (Link Loss, DP CTS, etc...). * Do nothing!! */ - DRM_INFO("DCHPD: connector_id=%d: dc_sink didn't change.\n", + DRM_DEBUG_DRIVER("DCHPD: connector_id=%d: dc_sink didn't change.\n", aconnector->connector_id); return; } - DRM_INFO("DCHPD: connector_id=%d: Old sink=%p Newsink=%p\n",+ DRM_DEBUG_DRIVER("DCHPD: connector_id=%d: Old sink=%p New sink=%p\n", aconnector->connector_id, aconnector->dc_sink, sink); mutex_lock(&dev->mode_config.mutex); @@ -926,7 +935,7 @@ static void dm_handle_hpd_rx_irq(struct amdgpu_dm_connector *aconnector) process_count++; - DRM_DEBUG_KMS("ESI %02x %02x %02x\n", esi[0], esi[1], esi[2]); + DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1], esi[2]); /* handle HPD short pulse irq */ if (aconnector->mst_mgr.mst_state) drm_dp_mst_hpd_irq( @@ -964,7 +973,7 @@ static void dm_handle_hpd_rx_irq(struct amdgpu_dm_connector *aconnector) } if (process_count == max_process_count) - DRM_DEBUG_KMS("Loop exceeded max iterations\n"); + DRM_DEBUG_DRIVER("Loop exceeded max iterations\n"); } static void handle_hpd_rx_irq(void *param) @@ -1283,7 +1292,7 @@ voidamdgpu_dm_register_backlight_device(structamdgpu_display_manager *dm) if (NULL == dm->backlight_dev) DRM_ERROR("DM: Backlight registration failed!\n"); else - DRM_INFO("DM: Registered Backlight device: %s\n", bl_name); + DRM_DEBUG_DRIVER("DM: Registered Backlight device: %s\n", bl_name); } #endif @@ -2064,7 +2073,7 @@ static void update_stream_scaling_settings( stream->src = src; stream->dst = dst; - DRM_DEBUG_KMS("Destination Rectangle x:%d y:%d width:%d height:%d\n", + DRM_DEBUG_DRIVER("Destination Rectangle x:%d y:%d width:%d height:%d\n", dst.x, dst.y, dst.width, dst.height); } @@ -2374,7 +2383,7 @@ static struct dc_stream_state *create_stream_for_sink( * case, we call set mode ourselves to restore the previous mode * and the modelist may not be filled in in time. */ - DRM_INFO("No preferred mode found\n"); + DRM_DEBUG_DRIVER("No preferred mode found\n"); } else { decide_crtc_timing_for_drm_display_mode( &mode, preferred_mode, @@ -2749,7 +2758,7 @@ static struct drm_encoder *best_encoder(struct drm_connector *connector) struct drm_mode_object *obj; struct drm_encoder *encoder; - DRM_DEBUG_KMS("Finding the best encoder\n"); + DRM_DEBUG_DRIVER("Finding the best encoder\n"); /* pick the encoder ids */ if (enc_id) { @@ -3019,7 +3028,7 @@ static int dm_plane_helper_prepare_fb( dm_plane_state_new = to_dm_plane_state(new_state); if (!new_state->fb) { - DRM_DEBUG_KMS("No FB bound\n"); + DRM_DEBUG_DRIVER("No FB bound\n"); return 0; } @@ -3594,7 +3603,7 @@ int amdgpu_dm_connector_init( struct amdgpu_i2c_adapter *i2c; ((struct dc_link *)link)->priv = aconnector; - DRM_DEBUG_KMS("%s()\n", __func__); + DRM_DEBUG_DRIVER("%s()\n", __func__); i2c = create_i2c(link->ddc, link->link_index, &res); aconnector->i2c = i2c; @@ -3835,11 +3844,11 @@ static void handle_cursor_update( if (!plane->state->fb && !old_plane_state->fb) return; - DRM_DEBUG_KMS("%s: crtc_id=%d with size %d to %d\n", - __func__, - amdgpu_crtc->crtc_id, - plane->state->crtc_w, - plane->state->crtc_h); + DRM_DEBUG_DRIVER("%s: crtc_id=%d with size %d to %d\n", + __func__, + amdgpu_crtc->crtc_id, + plane->state->crtc_w, + plane->state->crtc_h); ret = get_cursor_position(plane, crtc, &position); if (ret) @@ -4142,7 +4151,7 @@ void amdgpu_dm_atomic_commit_tail( new_acrtc_state = to_dm_crtc_state(new_state); old_acrtc_state = to_dm_crtc_state(old_crtc_state); - DRM_DEBUG_KMS( + DRM_DEBUG_DRIVER( "amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, " "planes_changed:%d, mode_changed:%d,active_changed:%d," "connectors_changed:%d\n", @@ -4160,7 +4169,7 @@ void amdgpu_dm_atomic_commit_tail( if (modeset_required(new_state, new_acrtc_state->stream, old_acrtc_state->stream)) { - DRM_INFO("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc); + DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc); if (!new_acrtc_state->stream) { /* @@ -4178,7 +4187,7 @@ void amdgpu_dm_atomic_commit_tail( * have a sink to keep the pipe running so that * hw state is consistent with the sw state */ - DRM_DEBUG_KMS("%s: Failed to create new stream for crtc %d\n", + DRM_DEBUG_DRIVER("%s: Failed to create new stream for crtc %d\n", __func__, acrtc->base.base.id); continue; } @@ -4205,7 +4214,7 @@ void amdgpu_dm_atomic_commit_tail( acrtc->hw_mode = crtc->state->mode; crtc->hwmode = crtc->state->mode; } else if (modereset_required(new_state)) { - DRM_INFO("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc); + DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc); /* i.e. reset mode */ if (old_acrtc_state->stream) @@ -4230,7 +4239,7 @@ void amdgpu_dm_atomic_commit_tail( &new_crtcs[i]->base, false); if (!aconnector) { - DRM_INFO("Atomic commit: Failed to find connector for acrtc id:%d " + DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d " "skipping freesync init\n", new_crtcs[i]->crtc_id); continue; @@ -4539,7 +4548,7 @@ static int dm_update_crtcs_state( */ if (!new_stream) { - DRM_DEBUG_KMS("%s: Failed to create new stream for crtc %d\n", + DRM_DEBUG_DRIVER("%s: Failed to create new stream for crtc %d\n", __func__, acrtc->base.base.id); break; } @@ -4550,7 +4559,7 @@ static int dm_update_crtcs_state( crtc_state->mode_changed = false; - DRM_DEBUG_KMS("Mode change not required, setting mode_changed to %d", + DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d", crtc_state->mode_changed); } @@ -4558,7 +4567,7 @@ static int dm_update_crtcs_state( if (!drm_atomic_crtc_needs_modeset(crtc_state)) goto next_crtc; - DRM_DEBUG_KMS( + DRM_DEBUG_DRIVER( "amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, " "planes_changed:%d, mode_changed:%d,active_changed:%d," "connectors_changed:%d\n", @@ -4576,7 +4585,7 @@ static int dm_update_crtcs_state( if (!old_acrtc_state->stream) goto next_crtc; - DRM_DEBUG_KMS("Disabling DRM crtc: %d\n", + DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n", crtc->base.id); /* i.e. reset mode */ @@ -4606,7 +4615,7 @@ static int dm_update_crtcs_state( new_acrtc_state->stream = new_stream; dc_stream_retain(new_stream); - DRM_DEBUG_KMS("Enabling DRM crtc: %d\n", + DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n", crtc->base.id); if (!dc_add_stream_to_ctx( @@ -4681,7 +4690,7 @@ static int dm_update_planes_state( if (!old_acrtc_state->stream) continue; - DRM_DEBUG_KMS("Disabling DRM plane: %d on DRM crtc %d\n", + DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc%d\n",plane->base.id, old_plane_crtc->base.id); if (!dc_remove_plane_from_context( @@ -4719,7 +4728,7 @@ static int dm_update_planes_state( new_dm_plane_state->dc_state = dc_create_plane_state(dc); - DRM_DEBUG_KMS("Enabling DRM plane: %d on DRM crtc %d\n", + DRM_DEBUG_DRIVER("Enabling DRM plane: %d on DRM crtc%d\n",plane->base.id, new_plane_crtc->base.id); if (!new_dm_plane_state->dc_state) { @@ -4874,9 +4883,9 @@ int amdgpu_dm_atomic_check(structdrm_device *dev,fail: if (ret == -EDEADLK) - DRM_DEBUG_KMS("Atomic check stopped due to to deadlock.\n"); + DRM_DEBUG_DRIVER("Atomic check stopped due to todeadlock.\n");else if (ret == -EINTR || ret == -EAGAIN || ret == -ERESTARTSYS) - DRM_DEBUG_KMS("Atomic check stopped due to to signal.\n"); + DRM_DEBUG_DRIVER("Atomic check stopped due to to signal.\n"); else DRM_ERROR("Atomic check failed with err: %d \n", ret); diff --git a/drivers/gpu/drm/amd/display/include/logger_types.h b/drivers/gpu/drm/amd/display/include/logger_types.h index 044805ccac25..1f22e84cedb9 100644 --- a/drivers/gpu/drm/amd/display/include/logger_types.h +++ b/drivers/gpu/drm/amd/display/include/logger_types.h @@ -70,6 +70,9 @@ enum dc_log_type { LOG_SECTION_TOTAL_COUNT }; +#define DC_MIN_LOG_MASK ((1 << LOG_ERROR) | \ + (1 << LOG_DETECTION_EDID_PARSER)) + #define DC_DEFAULT_LOG_MASK ((1 << LOG_ERROR) | \ (1 << LOG_WARNING) | \ (1 << LOG_EVENT_MODE_SET) | \_______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx_______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
