On 2017-09-12 12:10 PM, Darren Salt wrote:
> Noticed while playing â??Valleyâ??, which was causing some 8MB of leakage per
> second. kmemleak listed many entries looking like this:
>
> unreferenced object 0xffff8802c2951800 (size 1024):
> comm "Xorg", pid 2982, jiffies 4297410155 (age 392.787s)
> hex dump (first 32 bytes):
> 00 50 f9 0c 04 88 ff ff 98 08 00 00 00 00 00 00 .P..............
> 80 07 00 00 00 00 00 00 58 00 00 00 2c 00 00 00 ........X...,...
> backtrace:
> [<ffffffff810cd4c3>] create_object+0x13c/0x261
> [<ffffffff815abdc2>] kmemleak_alloc+0x20/0x3c
> [<ffffffff810cad1d>] slab_post_alloc_hook+0x42/0x52
> [<ffffffff810cb8e0>] kmem_cache_alloc+0x67/0x76
> [<ffffffff813bbb54>] dc_create_stream_for_sink+0x24/0x1cf
> [<ffffffff81373aaa>] create_stream_for_sink+0x6f/0x295
> [<ffffffff81373dc2>] dm_update_crtcs_state+0xa6/0x268
> [<ffffffff8137401e>] amdgpu_dm_atomic_check+0x9a/0x314
> [<ffffffff812ac3dd>] drm_atomic_check_only+0x17a/0x42d
> [<ffffffff812ac6a3>] drm_atomic_commit+0x13/0x4b
> [<ffffffff812ad1a5>] drm_atomic_connector_commit_dpms+0xcb/0xe8
> [<ffffffff812b1238>] drm_mode_obj_set_property_ioctl+0xe6/0x1e3
> [<ffffffff812b027b>] drm_mode_connector_property_set_ioctl+0x2b/0x2d
> [<ffffffff8129f427>] drm_ioctl_kernel+0x64/0x9d
> [<ffffffff8129f6a2>] drm_ioctl+0x230/0x316
> [<ffffffff812ca4d3>] amdgpu_drm_ioctl+0x4b/0x7d
>
> v2: also handle break statements.
>
Thanks for the patch. We really should've caught that earlier.
Reviewed-by: Harry Wentland <harry.wentland at amd.com>
Harry
> Signed-off-by: Darren Salt <devspam at moreofthesa.me.uk>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> 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 32c75867eaa7..14f1a4bcf2e9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4466,6 +4466,7 @@ static int dm_update_crtcs_state(
> int i;
> struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
> struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> + struct dc_stream_state *new_stream;
> int ret = 0;
>
> /*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
> @@ -4473,10 +4474,10 @@ static int dm_update_crtcs_state(
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> struct amdgpu_crtc *acrtc = NULL;
> struct amdgpu_connector *aconnector = NULL;
> - struct dc_stream_state *new_stream = NULL;
> struct drm_connector_state *conn_state = NULL;
> struct dm_connector_state *dm_conn_state = NULL;
>
> + new_stream = NULL;
>
> old_acrtc_state = to_dm_crtc_state(crtc->state);
> new_acrtc_state = to_dm_crtc_state(crtc_state);
> @@ -4525,7 +4526,7 @@ static int dm_update_crtcs_state(
>
>
> if (!drm_atomic_crtc_needs_modeset(crtc_state))
> - continue;
> + goto next_crtc;
>
> DRM_DEBUG_KMS(
> "amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
> @@ -4543,7 +4544,7 @@ static int dm_update_crtcs_state(
> if (!enable) {
>
> if (!old_acrtc_state->stream)
> - continue;
> + goto next_crtc;
>
> DRM_DEBUG_KMS("Disabling DRM crtc: %d\n",
> crtc->base.id);
> @@ -4554,7 +4555,7 @@ static int dm_update_crtcs_state(
> dm_state->context,
> old_acrtc_state->stream)) {
> ret = -EINVAL;
> - break;
> + goto fail;
> }
>
> dc_stream_release(old_acrtc_state->stream);
> @@ -4565,7 +4566,7 @@ static int dm_update_crtcs_state(
> } else {/* Add stream for any updated/enabled CRTC */
>
> if (modereset_required(crtc_state))
> - continue;
> + goto next_crtc;
>
> if (modeset_required(crtc_state, new_stream,
> old_acrtc_state->stream)) {
> @@ -4583,19 +4584,25 @@ static int dm_update_crtcs_state(
> dm_state->context,
> new_acrtc_state->stream)) {
> ret = -EINVAL;
> - break;
> + goto fail;
> }
>
> *lock_and_validation_needed = true;
> }
> }
>
> +next_crtc:
> /* Release extra reference */
> if (new_stream)
> dc_stream_release(new_stream);
> }
>
> return ret;
> +
> +fail:
> + if (new_stream)
> + dc_stream_release(new_stream);
> + return ret;
> }
>
> static int dm_update_planes_state(
>
