Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state

From: "Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Harry Wentland <harry.wentland-5C7GfCeVMHo@public.gmane.org>,
	Shirish S <shirish.s-5C7GfCeVMHo@public.gmane.org>,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state
Date: Thu, 31 May 2018 20:05:42 +0200	[thread overview]
Message-ID: <56590c3a-6f96-17b1-a3c3-c3159bf5b313@gmail.com> (raw)
In-Reply-To: <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org>

Am 30.05.2018 um 18:03 schrieb Harry Wentland:
> On 2018-05-30 06:17 AM, Shirish S wrote:
>> This patch fixes the warning messages that are caused due to calling
>> sleep in atomic context as below:
>>
>> BUG: sleeping function called from invalid context at mm/slab.h:419
>> in_atomic(): 1, irqs_disabled(): 1, pid: 5, name: kworker/u4:0
>> CPU: 1 PID: 5 Comm: kworker/u4:0 Tainted: G        W       4.14.35 #941
>> Workqueue: events_unbound commit_work
>> Call Trace:
>>   dump_stack+0x4d/0x63
>>   ___might_sleep+0x11f/0x12e
>>   kmem_cache_alloc_trace+0x41/0xea
>>   dc_create_state+0x1f/0x30
>>   dc_commit_updates_for_stream+0x73/0x4cf
>>   ? amdgpu_get_crtc_scanoutpos+0x82/0x16b
>>   amdgpu_dm_do_flip+0x239/0x298
>>   amdgpu_dm_commit_planes.isra.23+0x379/0x54b
>>   ? dc_commit_state+0x3da/0x404
>>   amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
>>   ? wait_for_common+0x5b/0x69
>>   commit_tail+0x42/0x64
>>   process_one_work+0x1b0/0x314
>>   worker_thread+0x1cb/0x2c1
>>   ? create_worker+0x1da/0x1da
>>   kthread+0x156/0x15e
>>   ? kthread_flush_work+0xea/0xea
>>   ret_from_fork+0x22/0x40
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/core/dc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
>> index 33149ed..d62206f 100644
>> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
>> @@ -588,7 +588,7 @@ static void disable_dangling_plane(struct dc *dc, struct dc_state *context)
>>   
>>   struct dc *dc_create(const struct dc_init_data *init_params)
>>    {
>> -	struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
>> +	struct dc *dc = kzalloc(sizeof(*dc), GFP_ATOMIC);
> Are you sure this one can be called in atomic_context?
>
> If so then everything in consstruct() would also need GFP_ATOMIC.

Well the backtrace is quite obvious, but I agree that change still looks 
fishy to me as well.

Using GFP_ATOMIC should only be a last resort when nothing else helps, 
but here it looks more like we misuse a spinlock where a mutex or 
semaphore would be more appropriate.

Where exactly becomes the context atomic in the call trace?

Christian.

>
> Harry
>
>>   	unsigned int full_pipe_count;
>>   
>>   	if (NULL == dc)
>> @@ -937,7 +937,7 @@ bool dc_post_update_surfaces_to_stream(struct dc *dc)
>>   struct dc_state *dc_create_state(void)
>>   {
>>   	struct dc_state *context = kzalloc(sizeof(struct dc_state),
>> -					   GFP_KERNEL);
>> +					   GFP_ATOMIC);
>>   
>>   	if (!context)
>>   		return NULL;
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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