转发: 回复: [PATCH] drm/komeda: drop all currently held locks if deadlock happens

["Liu Lucas/刘保柱" <lucas.liu@siengine.com>]

-----邮件原件-----
发件人: Liu Lucas/刘保柱 
发送时间: 2023年8月1日 16:59
收件人: 'Liviu Dudau' <liviu.dudau@arm.com>
抄送: airlied@gmail.com; daniel@ffwll.ch; Huang Menghui/黄梦辉 <menghui.huang@siengine.com>
主题: 回复: 回复: [PATCH] drm/komeda: drop all currently held locks if deadlock happens

Ok , later, I will send a new patch.

-----邮件原件-----
发件人: Liviu Dudau <liviu.dudau@arm.com>
发送时间: 2023年8月1日 16:38
收件人: Liu Lucas/刘保柱 <lucas.liu@siengine.com>
抄送: airlied@gmail.com; daniel@ffwll.ch; Huang Menghui/黄梦辉 <menghui.huang@siengine.com>
主题: Re: 回复: [PATCH] drm/komeda: drop all currently held locks if deadlock happens

On Tue, Aug 01, 2023 at 02:16:46AM +0000, Liu Lucas/刘保柱 wrote:
> Hello,

Hi Liu,

> 
> I'm sorry that the previous modification was not explained in detail.
> (1). We are doing a deadlock detection on the kernel, so turn on the debug option for locks:
> CONFIG_LOCKDEP=y
> CONFIG_PROVE_LOCKING=y
> CONFIG_LOCK_STAT=y
> CONFIG_DEBUG_RT_MUTEXES=y
> CONFIG_DEBUG_SPINLOCK=y
> CONFIG_DEBUG_MUTEXES=y
> CONFIG_DEBUG_RWSEMS=y
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y
> CONFIG_DEBUG_LOCK_ALLOC=y
> CONFIG_DEBUG_ATOMIC_SLEEP=y
> CONFIG_PROVE_RAW_LOCK_NESTING=y
> 
> Problems with the CONFIG_DEBUG_WW_MUTEX_SLOWPATH option were found:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 345 at
> drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1248
> komeda_release_unclaimed_resources+0x13c/0x170
> Modules linked in:
> CPU: 2 PID: 345 Comm: composer@2.1-se Kdump: loaded Tainted: G        W         5.10.110-SE-SDK1.8-dirty #16
> Hardware name: Siengine Se1000 Evaluation board (DT)
> pstate: 20400009 (nzCv daif +PAN -UAO -TCO BTYPE=--) pc : 
> komeda_release_unclaimed_resources+0x13c/0x170
> lr : komeda_release_unclaimed_resources+0xbc/0x170
> sp : ffff800017b8b8d0
> pmr_save: 000000e0
> x29: ffff800017b8b8d0 x28: ffff000cf2f96200
> x27: ffff000c8f5a8800 x26: 0000000000000000
> x25: 0000000000000038 x24: ffff8000116a0140
> x23: 0000000000000038 x22: ffff000cf2f96200
> x21: ffff000cfc300300 x20: ffff000c8ab77080
> x19: 0000000000000003 x18: 0000000000000000
> x17: 0000000000000000 x16: 0000000000000000
> x15: b400e638f738ba38 x14: 0000000000000000
> x13: 0000000106400a00 x12: 0000000000000000
> x11: 0000000000000000 x10: 0000000000000000
> x9 : ffff800012f80000 x8 : ffff000ca3308000
> x7 : 0000000ff3000000 x6 : ffff80001084034c
> x5 : ffff800017b8bc40 x4 : 000000000000000f
> x3 : ffff000ca3308000 x2 : 0000000000000000
> x1 : 0000000000000000 x0 : ffffffffffffffdd Call trace:
> komeda_release_unclaimed_resources+0x13c/0x170
> komeda_crtc_atomic_check+0x68/0xf0
> drm_atomic_helper_check_planes+0x138/0x1f4
> komeda_kms_check+0x284/0x36c
> drm_atomic_check_only+0x40c/0x714
> drm_atomic_nonblocking_commit+0x1c/0x60
> drm_mode_atomic_ioctl+0xa3c/0xb8c
> drm_ioctl_kernel+0xc4/0x120
> drm_ioctl+0x268/0x534
> __arm64_sys_ioctl+0xa8/0xf0
> el0_svc_common.constprop.0+0x80/0x240
> do_el0_svc+0x24/0x90
> el0_svc+0x20/0x30
> el0_sync_handler+0xe8/0xf0
> el0_sync+0x1a4/0x1c0
> irq event stamp: 0
> hardirqs last  enabled at (0): [<0000000000000000>] 0x0 hardirqs last 
> disabled at (0): [<ffff800010056d34>] copy_process+0x5d0/0x183c 
> softirqs last  enabled at (0): [<ffff800010056d34>] 
> copy_process+0x5d0/0x183c softirqs last disabled at (0):
> [<0000000000000000>] 0x0 ---[ end trace 20ae984fa860184a ]--- 
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 345 at drivers/gpu/drm/drm_modeset_lock.c:228
> drm_modeset_drop_locks+0x84/0x90 Modules linked in:
> CPU: 3 PID: 345 Comm: composer@2.1-se Kdump: loaded Tainted: G        W         5.10.110-SE-SDK1.8-dirty #16
> Hardware name: Siengine Se1000 Evaluation board (DT)
> pstate: 20400009 (nzCv daif +PAN -UAO -TCO BTYPE=--) pc : 
> drm_modeset_drop_locks+0x84/0x90 lr : 
> drm_mode_atomic_ioctl+0x860/0xb8c sp : ffff800017b8bb10
> pmr_save: 000000e0
> x29: ffff800017b8bb10 x28: 0000000000000001
> x27: 0000000000000038 x26: 0000000000000002
> x25: ffff000cecbefa00 x24: ffff000cf2f96200
> x23: 0000000000000001 x22: 0000000000000018
> x21: 0000000000000001 x20: ffff800017b8bc10
> x19: 0000000000000000 x18: 0000000000000000
> x17: 0000000002e8bf2c x16: 0000000002e94c6b
> x15: 0000000002ea48b9 x14: ffff8000121f0300
> x13: 0000000002ee2ca8 x12: ffff80001129cae0
> x11: ffff800012435000 x10: ffff000ed46b5e88
> x9 : ffff000c9935e600 x8 : 0000000000000000
> x7 : 000000008020001e x6 : 000000008020001f
> x5 : ffff80001085fbe0 x4 : fffffe0033a59f20
> x3 : 000000008020001e x2 : 0000000000000000
> x1 : 0000000000000000 x0 : ffff000c8f596090 Call trace:
> drm_modeset_drop_locks+0x84/0x90
> drm_mode_atomic_ioctl+0x860/0xb8c
> drm_ioctl_kernel+0xc4/0x120
> drm_ioctl+0x268/0x534
> __arm64_sys_ioctl+0xa8/0xf0
> el0_svc_common.constprop.0+0x80/0x240
> do_el0_svc+0x24/0x90
> el0_svc+0x20/0x30
> el0_sync_handler+0xe8/0xf0
> el0_sync+0x1a4/0x1c0
> irq event stamp: 0
> hardirqs last  enabled at (0): [<0000000000000000>] 0x0 hardirqs last 
> disabled at (0): [<ffff800010056d34>] copy_process+0x5d0/0x183c 
> softirqs last  enabled at (0): [<ffff800010056d34>] 
> copy_process+0x5d0/0x183c softirqs last disabled at (0):
> [<0000000000000000>] 0x0 ---[ end trace 20ae984fa860184b ]---
> 
> (2). According to the call trace information, it can be located to be WARN_ON(IS_ERR(c_st)) in the komeda_pipeline_unbound_components function; Then follow the function.
> komeda_pipeline_unbound_components
> -> komeda_component_get_state_and_set_user
>   -> komeda_pipeline_get_state_and_set_crtc
>     -> komeda_pipeline_get_state
>       ->drm_atomic_get_private_obj_state
>         -> drm_atomic_get_private_obj_state
>           -> drm_modeset_lock
> 
> 
> komeda_pipeline_unbound_components
> -> komeda_component_get_state_and_set_user
>   -> komeda_component_get_state
>     -> drm_atomic_get_private_obj_state
>      -> drm_modeset_lock
> 
> ret = drm_modeset_lock(&obj->lock, state->acquire_ctx); if (ret)
> 	return ERR_PTR(ret);
> Here it return -EDEADLK.
> 
> (3). Therefore, deal with the deadlock as suggested by [1], using the
>     function drm_modeset_backoff().
>     [1]
> https://docs.kernel.org/gpu/drm-kms.html?highlight=kms#kms-locking
> 
> According to the call trace information:
> Call trace:
> komeda_release_unclaimed_resources+0x13c/0x170
> komeda_crtc_atomic_check+0x68/0xf0
> drm_atomic_helper_check_planes+0x138/0x1f4
> komeda_kms_check+0x284/0x36c
> drm_atomic_check_only+0x40c/0x714
> drm_atomic_nonblocking_commit+0x1c/0x60
> drm_mode_atomic_ioctl+0xa3c/0xb8c
> drm_ioctl_kernel+0xc4/0x120
> drm_ioctl+0x268/0x534

This is a much better description of the problem that contains the relevant information for me. Can you please send a new revision of the patch where the commit message is the text above? With that, you can have my Reviewed-by tag.

Best regards,
Liviu


> 
> Add a determination of the return value to the function komeda_pipeline_unbound_components, Finally, komeda_release_unclaimed_resources returns to the drm_mode_atomic_ioctl function to call drm_modeset_backoff based on the value of ret.
> 
> (4). WARN_ON(IS_ERR(c_st)); This code can be retained, the modification was not carefully considered at that time, I will modify this patch again later and submit a new one.
>                 c_st = komeda_component_get_state_and_set_user(c,
>                                 drm_st, NULL, new->crtc);
> -               WARN_ON(IS_ERR(c_st));
> +               if (PTR_ERR(c_st) == -EDEADLK)
> +                       return -EDEADLK;
> +               else
> +                       WARN_ON(IS_ERR(c_st));
> 
> -----邮件原件-----
> 发件人: Liviu Dudau <liviu.dudau@arm.com>
> 发送时间: 2023年7月31日 22:11
> 收件人: Huang Menghui/黄梦辉 <menghui.huang@siengine.com>
> 抄送: airlied@gmail.com; daniel@ffwll.ch; Liu Lucas/刘保柱
> <lucas.liu@siengine.com>
> 主题: Re: [PATCH] drm/komeda: drop all currently held locks if deadlock 
> happens
> 
> Hello,
> 
> On Mon, Jul 31, 2023 at 04:08:43PM +0800, menghui.huang wrote:
> > From: "baozhu.liu" <lucas.liu@siengine.com>
> > 
> > If komeda_pipeline_unbound_components() returns -EDEADLK, it means 
> > that a deadlock happened in the locking context.
> > Currently, komeda is not dealing with the deadlock properly, 
> > producing the following output when CONFIG_DEBUG_WW_MUTEX_SLOWPATH is enabled:
> > 
> > ------------[ cut here ]------------
> > WARNING: CPU: 2 PID: 345 at drivers/gpu/drm/drm_modeset_lock.c:228
> > drm_modeset_drop_locks+0x84/0x90 Modules linked in:
> > CPU: 2 PID: 345 Comm: composer@2.1-se Kdump: loaded Tainted: G        W
> > Hardware name: Siengine Se1000 Evaluation board (DT)
> > pstate: 20400009 (nzCv daif +PAN -UAO -TCO BTYPE=--) pc : 
> > drm_modeset_drop_locks+0x84/0x90 lr : 
> > drm_mode_atomic_ioctl+0x860/0xb8c sp : ffff800017b8bb10
> > pmr_save: 000000e0
> > x29: ffff800017b8bb10 x28: 0000000000000001
> > x27: 0000000000000038 x26: 0000000000000002
> > x25: ffff000d03a6cc00 x24: ffff000cf2fe9000
> > x23: 0000000000000001 x22: 0000000000000018
> > x21: 0000000000000001 x20: ffff800017b8bc10
> > x19: 0000000000000000 x18: 0000000000000000
> > x17: 000000000000a2bb x16: 0000000000f77f9a
> > x15: 00000000025a0830 x14: ffff8000121f0300
> > x13: 0000000005740083 x12: ffff80001129cae0
> > x11: ffff800012435000 x10: ffff000ed46b5e88
> > x9 : ffff000c9935e600 x8 : 0000000000000000
> > x7 : 0000000000000001 x6 : 00000000000ba57a
> > x5 : ffff000cf458e400 x4 : ffff000ed4c528a0
> > x3 : 00000000000ba582 x2 : 0000000000000000
> > x1 : 0000000000000000 x0 : ffff000c8f597290 Call trace:
> > drm_modeset_drop_locks+0x84/0x90
> > drm_mode_atomic_ioctl+0x860/0xb8c
> > drm_ioctl_kernel+0xc4/0x120
> > drm_ioctl+0x268/0x534
> > __arm64_sys_ioctl+0xa8/0xf0
> > el0_svc_common.constprop.0+0x80/0x240
> > do_el0_svc+0x24/0x90
> > el0_svc+0x20/0x30
> > el0_sync_handler+0xe8/0xf0
> > el0_sync+0x1a4/0x1c0
> > irq event stamp: 0
> > hardirqs last  enabled at (0): [<0000000000000000>] 0x0 hardirqs 
> > last disabled at (0): [<ffff800010056d34>] copy_process+0x5d0/0x183c 
> > softirqs last  enabled at (0): [<ffff800010056d34>] 
> > copy_process+0x5d0/0x183c softirqs last disabled at (0):
> > [<0000000000000000>] 0x0 ---[ end trace 20ae984fa8601849 ]---
> > 
> > Therefore, handling this deadlock can be solved by adding return 
> > -EDEADLK back to the drm_modeset_backoff processing flow in the 
> > drm_mode_atomic_ioctl function.
> > 
> > Signed-off-by: baozhu.liu <lucas.liu@siengine.com>
> > ---
> >  .../gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 10
> > ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > index 3276a3e82c62..8ad021259a37 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > @@ -1223,7 +1223,7 @@ int komeda_build_display_data_flow(struct komeda_crtc *kcrtc,
> >  	return 0;
> >  }
> >  
> > -static void
> > +static int
> >  komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
> >  				   struct komeda_pipeline_state *new)  { @@ -1243,8 +1243,11 @@ 
> > komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
> >  		c = komeda_pipeline_get_component(pipe, id);
> >  		c_st = komeda_component_get_state_and_set_user(c,
> >  				drm_st, NULL, new->crtc);
> > -		WARN_ON(IS_ERR(c_st));
> > +		if (PTR_ERR(c_st) == -EDEADLK)
> > +			return -EDEADLK;
> >  	}
> > +
> > +	return 0;
> >  }
> >  
> >  /* release unclaimed pipeline resource */ @@ -1266,9 +1269,8 @@ int 
> > komeda_release_unclaimed_resources(struct komeda_pipeline *pipe,
> >  	if (WARN_ON(IS_ERR_OR_NULL(st)))
> >  		return -EINVAL;
> >  
> > -	komeda_pipeline_unbound_components(pipe, st);
> > +	return komeda_pipeline_unbound_components(pipe, st);
> >  
> > -	return 0;
> >  }
> >  
> >  /* Since standalone disabled components must be disabled separately 
> > and in the
> > --
> > 2.17.1
> > 
> 
> Thank you for sending this patch, but commit message need a bit more clarity about what you're trying to do and why you think this patch solves it.
> 
> First of all, the commit title talks about dropping locks which is obviously not what the patch is doing here. Second, the patch replaces a WARN_ON() which I would expect to be printed in the log fragment that you have provided, but instead I see the WARNING() from drm_modeset_lock.c:228. I cannot see from the call trace where you've hit a komeda function call, so something is missing.
> 
> Can you also provide us with information on the kernel version you're using?
> For me, line 228 is on an empty line inside
> drm_warn_on_modeset_not_all_locked()
> and drm_modeset_drop_locks() only starts at line 274.
> 
> Best regards,
> Liviu
> 
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

--
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯