Re: [PATCH] drm/rockchip: Allow driver to be shutdown on reboot/kexec

[Heiko Stübner]

Hi Brian,

Am Mittwoch, 5. Dezember 2018, 04:01:34 CET schrieb Brian Norris:
> + others
> 
> Hi,
> 
> On Sun, Aug 05, 2018 at 01:48:07PM +0100, Marc Zyngier wrote:
> > Leaving the DRM driver enabled on reboot or kexec has the annoying
> > effect of leaving the display generating transactions whilst the
> > IOMMU has been shut down.
> > 
> > In turn, the IOMMU driver (which shares its interrupt line with
> > the VOP) starts warning either on shutdown or when entering the
> > secondary kernel in the kexec case (nothing is expected on that
> > front).
> > 
> > A cheap way of ensuring that things are nicely shut down is to
> > register a shutdown callback in the platform driver.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > ---
> 
> This patch made it into 4.20-rc1 as well as -stable, and it has caused
> regressions for me, on the Kevin and Scarlet [1] RK3399 platforms.
>
> On
> shutdown/reboot, I see this:
> 
> [   94.742559] WARNING: CPU: 4 PID: 2035 at
> drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x1c4/0x294
> ...
> [   94.775904] CPU: 4 PID: 2035 Comm: reboot Tainted: G        W        
> 4.20.0-rc5+ #83 [   94.784651] Hardware name: Google Scarlet (DT)
> [   94.789611] pstate: 20000005 (nzCv daif -PAN -UAO)
> [   94.794959] pc : drm_mode_config_cleanup+0x1c4/0x294
> [   94.800500] lr : drm_mode_config_cleanup+0x108/0x294
> ...
> [   94.898683] Call trace:
> [   94.901410]  drm_mode_config_cleanup+0x1c4/0x294
> [   94.906565]  rockchip_drm_unbind+0x4c/0x8c
> [   94.911138]  component_master_del+0x88/0xb8
> [   94.915807]  rockchip_drm_platform_remove+0x2c/0x44
> [   94.921243]  rockchip_drm_platform_shutdown+0x20/0x2c
> [   94.926881]  platform_drv_shutdown+0x2c/0x38
> [   94.931647]  device_shutdown+0x164/0x1b8
> [   94.936016]  kernel_restart_prepare+0x40/0x48
> [   94.940878]  kernel_restart+0x20/0x68
> [   94.944964]  __se_sys_reboot+0x1ac/0x204
> [   94.949331]  __arm64_sys_reboot+0x2c/0x38
> [   94.953806]  el0_svc_common+0xa4/0xec
> [   94.957891]  el0_svc_compat_handler+0x30/0x3c
> [   94.962753]  el0_svc_compat+0x8/0x18
> [   94.966740] ---[ end trace b9ba2e701f4fb233 ]---
> [   95.255169] Memory manager not clean during takedown.
> [   95.260824] WARNING: CPU: 4 PID: 2035 at drivers/gpu/drm/drm_mm.c:950
> drm_mm_takedown+0x34/0x44 ...
> [   95.292314] CPU: 4 PID: 2035 Comm: reboot Tainted: G        W        
> 4.20.0-rc5+ #83 [   95.301061] Hardware name: Google Scarlet (DT)
> [   95.306020] pstate: 60000005 (nZCv daif -PAN -UAO)
> [   95.311369] pc : drm_mm_takedown+0x34/0x44
> [   95.315940] lr : drm_mm_takedown+0x34/0x44
> ...
> [   95.415857]  drm_mm_takedown+0x34/0x44
> [   95.420042]  rockchip_drm_unbind+0x64/0x8c
> [   95.424613]  component_master_del+0x88/0xb8
> [   95.429283]  rockchip_drm_platform_remove+0x2c/0x44
> [   95.434728]  rockchip_drm_platform_shutdown+0x20/0x2c
> [   95.440360]  platform_drv_shutdown+0x2c/0x38
> [   95.445127]  device_shutdown+0x164/0x1b8
> [   95.449504]  kernel_restart_prepare+0x40/0x48
> [   95.454358]  kernel_restart+0x20/0x68
> [   95.458436]  __se_sys_reboot+0x1ac/0x204
> [   95.462812]  __arm64_sys_reboot+0x2c/0x38
> [   95.467287]  el0_svc_common+0xa4/0xec
> [   95.471373]  el0_svc_compat_handler+0x30/0x3c
> [   95.476235]  el0_svc_compat+0x8/0x18
> [   95.480215] ---[ end trace b9ba2e701f4fb234 ]---
> 
> It's especially bad on -stable kernels, where I believe the remove()
> paths were even worse. This triggers a variety of OOPSes, and it's not
> clear if those are simply because of backports (e.g., RK3399 did not
> have support in 4.4.y, but our downstream has merged all sorts of
> backports to make it work).
> 
> Anyway, the above warnings occur on v4.20-rc, which I think is
> justification enough for a revert.

That's strange. I remember testing quite a number of shutdown/reboot
cycles before applying that patch. And for good measure did the same
again right now.

- Kevin, with netboot firmware, booting into Debian+console only
- Bob, with stock firmware, booting into Debian+KDE Plasma
- Scarlet, with stock firmware, booting into Debian+KDE Plasma

With some random number of reboot and shutdowns on each I didn't
see any warnings at all.

> I plan to submit a revert which I hope can go to 4.20 as well as
> -stable. I'd hope the remove()/shutdown() paths should be fixed before
> this gets applied again, and that it does not get shipped to -stable
> kernels.

But judging by the fact that the warning indicates that somthing is still
holding onto a framebuffer and a rmmod rockchipdrm is not possible
at runrtime for likely the same reason, I guess we really might be creating
a problem with that shutdown.

Can you maybe give "drm/rockchip: shutdown drm subsystem on shutdown" [2]
a try? When the underlying issue of rebooting surfaced we had 2 competing 
solutions, so we at least don't reopen the issue, that people have problems
rebooting?

drm-drivers seem to be short on shutdown handlers to peek at but this
second variant mimics what tinydrm does in its shutdown
(calling drm_atomic_helper_shutdown), so at least shouldn't be completely
wrong.


[2] https://patchwork.kernel.org/patch/10556151/


Heiko


> [1] Technically Scarlet needed a few patches from -next to work at all,
>     but Kevin is a similar platform that has been working for several
>     releases.
> 
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index
> > f814d37b1db2..05368fa4f956 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > @@ -442,6 +442,11 @@ static int rockchip_drm_platform_remove(struct
> > platform_device *pdev)> 
> >  	return 0;
> >  
> >  }
> > 
> > +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> > +{
> > +	rockchip_drm_platform_remove(pdev);
> > +}
> > +
> > 
> >  static const struct of_device_id rockchip_drm_dt_ids[] = {
> >  
> >  	{ .compatible = "rockchip,display-subsystem", },
> >  	{ /* sentinel */ },
> > 
> > @@ -451,6 +456,7 @@ MODULE_DEVICE_TABLE(of, rockchip_drm_dt_ids);
> > 
> >  static struct platform_driver rockchip_drm_platform_driver = {
> >  
> >  	.probe = rockchip_drm_platform_probe,
> >  	.remove = rockchip_drm_platform_remove,
> > 
> > +	.shutdown = rockchip_drm_platform_shutdown,
> > 
> >  	.driver = {
> >  	
> >  		.name = "rockchip-drm",
> >  		.of_match_table = rockchip_drm_dt_ids,