* [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state @ 2018-05-30 10:17 Shirish S [not found] ` <1527675436-24022-1-git-send-email-shirish.s-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Shirish S @ 2018-05-30 10:17 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Harry.Wentland-5C7GfCeVMHo Cc: Shirish S 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); 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; -- 2.7.4 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1527675436-24022-1-git-send-email-shirish.s-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <1527675436-24022-1-git-send-email-shirish.s-5C7GfCeVMHo@public.gmane.org> @ 2018-05-30 16:03 ` Harry Wentland [not found] ` <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Harry Wentland @ 2018-05-30 16:03 UTC (permalink / raw) To: Shirish S, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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. 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org> @ 2018-05-31 10:01 ` S, Shirish 2018-05-31 18:05 ` Christian König 1 sibling, 0 replies; 8+ messages in thread From: S, Shirish @ 2018-05-31 10:01 UTC (permalink / raw) To: Harry Wentland, Shirish S, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 5/30/2018 9:33 PM, Harry Wentland wrote: > 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? My bad, you are right, this is not required. I have re-spun the patch as V2 with the GFP_ATOMIC applied only to dc_create_state. Thanks & Regards, Shirish S > If so then everything in consstruct() would also need GFP_ATOMIC. > > 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; >> -- Regards, Shirish S _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org> 2018-05-31 10:01 ` S, Shirish @ 2018-05-31 18:05 ` Christian König [not found] ` <56590c3a-6f96-17b1-a3c3-c3159bf5b313-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 1 sibling, 1 reply; 8+ messages in thread From: Christian König @ 2018-05-31 18:05 UTC (permalink / raw) To: Harry Wentland, Shirish S, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <56590c3a-6f96-17b1-a3c3-c3159bf5b313-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <56590c3a-6f96-17b1-a3c3-c3159bf5b313-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2018-06-01 9:56 ` S, Shirish [not found] ` <67617d8b-65ee-1bfc-7937-14f534cc98b8-5C7GfCeVMHo@public.gmane.org> 2018-06-07 14:44 ` Michel Dänzer 1 sibling, 1 reply; 8+ messages in thread From: S, Shirish @ 2018-06-01 9:56 UTC (permalink / raw) To: christian.koenig-5C7GfCeVMHo, Harry Wentland, Shirish S, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW The V2 of this patch is already reviewed by Harry. The change i have made in dc_create() is no more applicable. Regards, Shirish S On 5/31/2018 11:35 PM, Christian König wrote: > 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 > -- Regards, Shirish S _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <67617d8b-65ee-1bfc-7937-14f534cc98b8-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <67617d8b-65ee-1bfc-7937-14f534cc98b8-5C7GfCeVMHo@public.gmane.org> @ 2018-06-01 10:10 ` Christian König 0 siblings, 0 replies; 8+ messages in thread From: Christian König @ 2018-06-01 10:10 UTC (permalink / raw) To: S, Shirish, christian.koenig-5C7GfCeVMHo, Harry Wentland, Shirish S, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Ok, that works for me as well. Please always check if it is really necessary before adding any GFP_ATOMIC allocation, cause that is rather invasive and should be avoided. Christian. Am 01.06.2018 um 11:56 schrieb S, Shirish: > > The V2 of this patch is already reviewed by Harry. > The change i have made in dc_create() is no more applicable. > > Regards, > Shirish S > On 5/31/2018 11:35 PM, Christian König wrote: >> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <56590c3a-6f96-17b1-a3c3-c3159bf5b313-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2018-06-01 9:56 ` S, Shirish @ 2018-06-07 14:44 ` Michel Dänzer [not found] ` <3ed3bc96-4218-de1f-ba0a-1e4102883438-otUistvHUpPR7s880joybQ@public.gmane.org> 1 sibling, 1 reply; 8+ messages in thread From: Michel Dänzer @ 2018-06-07 14:44 UTC (permalink / raw) To: christian.koenig-5C7GfCeVMHo, Harry Wentland, Shirish S Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2018-05-31 08:05 PM, Christian König wrote: > 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. I want to raise this concern again, for all of these patches. I'm now seeing similar BUG messages triggered from VCE functions, see an example below. I've never seen these before today, so I assume they're caused by the "drm/amdgpu/pp: replace mutex with spin_lock (V3)" change. Now if we just mechanically convert the mutex to a spinlock whenever this happens, it could be a rat's tail, and we might end up converting a lot of mutexes to spinlocks. This could be bad if any of those locks can be held for significant amounts of time. Instead, we should look into why we end up in atomic context. All of the messages in these patches have been triggered from either a worker thread or an ioctl, neither of which run in atomic context per se. Most likely we end up in atomic context because a spinlock is held, so maybe it can be solved by converting those spinlocks to mutexes instead? [ 6232.096387] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:747 [ 6232.099544] in_atomic(): 1, irqs_disabled(): 0, pid: 14785, name: kworker/3:14 [ 6232.102183] INFO: lockdep is turned off. [ 6232.104965] CPU: 3 PID: 14785 Comm: kworker/3:14 Tainted: G B D W OE 4.16.0-rc7+ #104 [ 6232.107835] Hardware name: Micro-Star International Co., Ltd. MS-7A34/B350 TOMAHAWK (MS-7A34), BIOS 1.80 09/13/2017 [ 6232.110878] Workqueue: events amdgpu_vce_idle_work_handler [amdgpu] [ 6232.113457] Call Trace: [ 6232.116065] dump_stack+0x85/0xc1 [ 6232.117880] ___might_sleep+0x28a/0x3c0 [ 6232.119770] __mutex_lock+0xc4/0x1520 [ 6232.121756] ? vce_v3_0_stop+0x3a/0x170 [amdgpu] [ 6232.123603] ? mutex_lock_io_nested+0x13e0/0x13e0 [ 6232.125447] ? debug_check_no_locks_freed+0x2c0/0x2c0 [ 6232.127333] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] [ 6232.129253] ? __mutex_lock+0xf9/0x1520 [ 6232.131251] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] [ 6232.133324] ? pick_next_task_fair+0x550/0x1720 [ 6232.135178] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] [ 6232.137078] ? vce_v3_0_stop+0x3a/0x170 [amdgpu] [ 6232.138948] vce_v3_0_stop+0x3a/0x170 [amdgpu] [ 6232.140687] amdgpu_device_ip_set_powergating_state+0x150/0x2f0 [amdgpu] [ 6232.142694] smu7_powergate_vce+0x11d/0x1c0 [amdgpu] [ 6232.144336] pp_dpm_powergate_vce+0xf4/0x160 [amdgpu] [ 6232.146283] ? pp_set_clockgating_by_smu+0xe0/0xe0 [amdgpu] [ 6232.148007] amdgpu_dpm_enable_vce+0x298/0x330 [amdgpu] [ 6232.149815] process_one_work+0x715/0x1570 [ 6232.151547] ? pwq_dec_nr_in_flight+0x2b0/0x2b0 [ 6232.153193] ? lock_acquire+0x10b/0x350 [ 6232.155001] worker_thread+0xd4/0xef0 [ 6232.156758] ? process_one_work+0x1570/0x1570 [ 6232.158595] kthread+0x311/0x3d0 [ 6232.160049] ? kthread_create_worker_on_cpu+0xc0/0xc0 [ 6232.161677] ret_from_fork+0x27/0x50 -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <3ed3bc96-4218-de1f-ba0a-1e4102883438-otUistvHUpPR7s880joybQ@public.gmane.org>]
* Re: [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state [not found] ` <3ed3bc96-4218-de1f-ba0a-1e4102883438-otUistvHUpPR7s880joybQ@public.gmane.org> @ 2018-06-07 14:46 ` Harry Wentland 0 siblings, 0 replies; 8+ messages in thread From: Harry Wentland @ 2018-06-07 14:46 UTC (permalink / raw) To: Michel Dänzer, christian.koenig-5C7GfCeVMHo, Shirish S Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2018-06-07 10:44 AM, Michel Dänzer wrote: > On 2018-05-31 08:05 PM, Christian König wrote: >> 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. > > I want to raise this concern again, for all of these patches. > > I'm now seeing similar BUG messages triggered from VCE functions, see an > example below. I've never seen these before today, so I assume they're > caused by the "drm/amdgpu/pp: replace mutex with spin_lock (V3)" change. > > Now if we just mechanically convert the mutex to a spinlock whenever > this happens, it could be a rat's tail, and we might end up converting a > lot of mutexes to spinlocks. This could be bad if any of those locks can > be held for significant amounts of time. > > Instead, we should look into why we end up in atomic context. All of the > messages in these patches have been triggered from either a worker > thread or an ioctl, neither of which run in atomic context per se. Most > likely we end up in atomic context because a spinlock is held, so maybe > it can be solved by converting those spinlocks to mutexes instead? > I fully agree with Michel. Shirish, please follow up on this. Harry > > [ 6232.096387] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:747 > [ 6232.099544] in_atomic(): 1, irqs_disabled(): 0, pid: 14785, name: kworker/3:14 > [ 6232.102183] INFO: lockdep is turned off. > [ 6232.104965] CPU: 3 PID: 14785 Comm: kworker/3:14 Tainted: G B D W OE 4.16.0-rc7+ #104 > [ 6232.107835] Hardware name: Micro-Star International Co., Ltd. MS-7A34/B350 TOMAHAWK (MS-7A34), BIOS 1.80 09/13/2017 > [ 6232.110878] Workqueue: events amdgpu_vce_idle_work_handler [amdgpu] > [ 6232.113457] Call Trace: > [ 6232.116065] dump_stack+0x85/0xc1 > [ 6232.117880] ___might_sleep+0x28a/0x3c0 > [ 6232.119770] __mutex_lock+0xc4/0x1520 > [ 6232.121756] ? vce_v3_0_stop+0x3a/0x170 [amdgpu] > [ 6232.123603] ? mutex_lock_io_nested+0x13e0/0x13e0 > [ 6232.125447] ? debug_check_no_locks_freed+0x2c0/0x2c0 > [ 6232.127333] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] > [ 6232.129253] ? __mutex_lock+0xf9/0x1520 > [ 6232.131251] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] > [ 6232.133324] ? pick_next_task_fair+0x550/0x1720 > [ 6232.135178] ? amdgpu_dpm_enable_vce+0x21a/0x330 [amdgpu] > [ 6232.137078] ? vce_v3_0_stop+0x3a/0x170 [amdgpu] > [ 6232.138948] vce_v3_0_stop+0x3a/0x170 [amdgpu] > [ 6232.140687] amdgpu_device_ip_set_powergating_state+0x150/0x2f0 [amdgpu] > [ 6232.142694] smu7_powergate_vce+0x11d/0x1c0 [amdgpu] > [ 6232.144336] pp_dpm_powergate_vce+0xf4/0x160 [amdgpu] > [ 6232.146283] ? pp_set_clockgating_by_smu+0xe0/0xe0 [amdgpu] > [ 6232.148007] amdgpu_dpm_enable_vce+0x298/0x330 [amdgpu] > [ 6232.149815] process_one_work+0x715/0x1570 > [ 6232.151547] ? pwq_dec_nr_in_flight+0x2b0/0x2b0 > [ 6232.153193] ? lock_acquire+0x10b/0x350 > [ 6232.155001] worker_thread+0xd4/0xef0 > [ 6232.156758] ? process_one_work+0x1570/0x1570 > [ 6232.158595] kthread+0x311/0x3d0 > [ 6232.160049] ? kthread_create_worker_on_cpu+0xc0/0xc0 > [ 6232.161677] ret_from_fork+0x27/0x50 > > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-07 14:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-30 10:17 [PATCH] drm/amd/display: avoid sleeping in atomic context while creating new context or state Shirish S [not found] ` <1527675436-24022-1-git-send-email-shirish.s-5C7GfCeVMHo@public.gmane.org> 2018-05-30 16:03 ` Harry Wentland [not found] ` <1b831a0f-3d54-2b07-12f8-b1ddda9007f6-5C7GfCeVMHo@public.gmane.org> 2018-05-31 10:01 ` S, Shirish 2018-05-31 18:05 ` Christian König [not found] ` <56590c3a-6f96-17b1-a3c3-c3159bf5b313-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2018-06-01 9:56 ` S, Shirish [not found] ` <67617d8b-65ee-1bfc-7937-14f534cc98b8-5C7GfCeVMHo@public.gmane.org> 2018-06-01 10:10 ` Christian König 2018-06-07 14:44 ` Michel Dänzer [not found] ` <3ed3bc96-4218-de1f-ba0a-1e4102883438-otUistvHUpPR7s880joybQ@public.gmane.org> 2018-06-07 14:46 ` Harry Wentland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.