[PATCH 05/20] drm/amd/display: Call into DC once per multiplane flip

[Kazlauskas, Nicholas]

On 1/28/19 6:59 AM, Michel Dänzer wrote:
> On 2019-01-22 7:28 p.m., sunpeng.li at amd.com wrote:
>> From: David Francis <David.Francis at amd.com>
>>
>> [Why]
>> amdgpu_dm_commit_planes was performing multi-plane
>> flips incorrectly:
>>
>> It waited for vblank once per flipped plane
>>
>> It prepared flip ISR and acquired the corresponding vblank ref
>> once per plane, although it closed ISR and put the ref once
>> per crtc
>>
>> It called into dc once per flipped plane, duplicating some work
>>
>> [How]
>> Wait for vblank, get vblank ref, prepare flip ISR, and call into
>> DC only once, and only if there is a pageflip
>>
>> Make freesync continue to update planes even if vrr information
>> has already been changed
>>
>> Signed-off-by: David Francis <David.Francis at amd.com>
>> Reviewed-by: Harry Wentland <Harry.Wentland at amd.com>
>> Acked-by: Leo Li <sunpeng.li at amd.com>
> 
> This commit introduced a memory leak, see the attached report from kmemleak.
> 
> 
>> 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 db060da..818a2a1 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4987,8 +4837,23 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>>   	struct dm_crtc_state *dm_old_crtc_state =
>>   			to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
>> -	int planes_count = 0;
>> +	int flip_count = 0, planes_count = 0, vpos, hpos;
>>   	unsigned long flags;
>> +	struct amdgpu_bo *abo;
>> +	uint64_t tiling_flags, dcc_address;
>> +	struct dc_stream_status *stream_status;
>> +	uint32_t target, target_vblank;
>> +
>> +	struct {
>> +		struct dc_surface_update surface_updates[MAX_SURFACES];
>> +		struct dc_flip_addrs flip_addrs[MAX_SURFACES];
>> +		struct dc_stream_update stream_update;
>> +	} *flip;
>> +
>> +	flip = kzalloc(sizeof(*flip), GFP_KERNEL);
>> +
>> +	if (!flip)
>> +		dm_error("Failed to allocate update bundles\n");
> 
> I can't see where this memory is freed, maybe this is the leak?

Yeah, I don't think this or the other allocation in a later patch is 
ever freed. Thanks for the heads up, I'll make a quick patch addressing 
these.

Nicholas Kazlauskas

> 
> 
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>