[PATCH 3/3] drm/amd/display: Reduce DC chattiness
Felix Kuehling
felix.kuehling at amd.com
Fri Sep 15 15:35:28 UTC 2017
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; amd-gfx at lists.freedesktop.org
>>> 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?
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.
Regards,
Felix
>
> Christian.
>
>>
>> Alex
>>
>>> Harry
>>>
>>>> Christian.
>>>>
>>>>> Change-Id: Icdfb849fa678225e2460519fbd8066540feb451a
>>>>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>>>>> ---
>>>>> 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
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list