[PATCH 3/3] drm/amd/display: Reduce DC chattiness

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 


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?
> 

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 <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) | \
>>>>
>
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux