Re: [PATCH] drm/amd/display: Don't leak dc_stream_state.

From: Harry Wentland <harry.wentland-5C7GfCeVMHo@public.gmane.org>
To: Darren Salt
	<devspam-kUQ7/2e57v/LS5A4iEQ01Vpr/1R2p/CL@public.gmane.org>,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: "Grodzovsky, Andrey" <Andrey.Grodzovsky-5C7GfCeVMHo@public.gmane.org>
Subject: Re: [PATCH] drm/amd/display: Don't leak dc_stream_state.
Date: Tue, 12 Sep 2017 10:37:46 -0400	[thread overview]
Message-ID: <2fa7074c-eacd-7691-4170-08bddaed22ba@amd.com> (raw)
In-Reply-To: <20170908154134.8657-1-devspam-kUQ7/2e57v/LS5A4iEQ01Vpr/1R2p/CL@public.gmane.org>

On 2017-09-08 11:41 AM, 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

Please leave a Signed-off-by tag (see part 10 of
https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html).

Otherwise the patch looks good. Please resend with the Signed-off-by.

Looks like we'd have the same problem with the break statements. This
wouldn't be as noticeable since they are error conditions.

Thanks,
Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 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..394fc3c03362 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4525,7 +4525,7 @@ static int dm_update_crtcs_state(
>  
>  
>  		if (!drm_atomic_crtc_needs_modeset(crtc_state))
> -				continue;
> +			goto free_new_stream_continue;
>  
>  		DRM_DEBUG_KMS(
>  			"amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
> @@ -4543,7 +4543,7 @@ static int dm_update_crtcs_state(
>  		if (!enable) {
>  
>  			if (!old_acrtc_state->stream)
> -				continue;
> +				goto free_new_stream_continue;
>  
>  			DRM_DEBUG_KMS("Disabling DRM crtc: %d\n",
>  					crtc->base.id);
> @@ -4565,7 +4565,7 @@ static int dm_update_crtcs_state(
>  		} else {/* Add stream for any updated/enabled CRTC */
>  
>  			if (modereset_required(crtc_state))
> -				continue;
> +				goto free_new_stream_continue;
>  
>  			if (modeset_required(crtc_state, new_stream,
>  					     old_acrtc_state->stream)) {
> @@ -4590,6 +4590,7 @@ static int dm_update_crtcs_state(
>  			}
>  		}
>  
> +free_new_stream_continue:
>  		/* Release extra reference */
>  		if (new_stream)
>  			 dc_stream_release(new_stream);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx