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 chattiness >>> >>> >>> >>> On 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 = 1 >>>> Which 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? >
I agree, but one of the issues we've faced is that there's a whole lot of log-spam that's unrelated to DC which often prevents people seeing DC problems. Does DRM have log levels that can be used more fine-grained than DEBUG_KMS/DRIVER/etc? Harry > Christian. > >> >> Alex >> >>> Harry >>> >>>> Christian. >>>> >>>>> 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 @@ void >>> amdgpu_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 New >>> sink=%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 @@ void >>> amdgpu_dm_register_backlight_device(struct >>>>> amdgpu_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(struct >>> drm_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 to >>> deadlock.\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
