| Message ID | 20170524145212.27837-20-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-05-24 07:51, Daniel Vetter wrote: > Again cleanup before irq disabling doesn't really stop the races, > so just drop it. Proper fix would be to put drm_atomic_helper_shutdown > before everything gets cleaned up. Hm, I already use the non-atomic drm_crtc_force_disable_all before, I think that fixed the races I saw. But I guess what you are saying instead of using drm_crtc_force_disable_all I should use drm_atomic_helper_shutdown...? Will try that. FWIW, Acked-by: Stefan Agner <stefan@agner.ch> -- Stefan > > Cc: Stefan Agner <stefan@agner.ch> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > index 6e00f4b267f1..b34d09b59eee 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > @@ -109,7 +109,6 @@ static int fsl_dcu_load(struct drm_device *dev, > unsigned long flags) > drm_fbdev_cma_fini(fsl_dev->fbdev); > > drm_mode_config_cleanup(dev); > - drm_vblank_cleanup(dev); > drm_irq_uninstall(dev); > dev->dev_private = NULL; > > @@ -127,7 +126,6 @@ static void fsl_dcu_unload(struct drm_device *dev) > drm_fbdev_cma_fini(fsl_dev->fbdev); > > drm_mode_config_cleanup(dev); > - drm_vblank_cleanup(dev); > drm_irq_uninstall(dev); > > dev->dev_private = NULL;
On Thu, May 25, 2017 at 10:18 AM, Stefan Agner <stefan@agner.ch> wrote: > On 2017-05-24 07:51, Daniel Vetter wrote: >> Again cleanup before irq disabling doesn't really stop the races, >> so just drop it. Proper fix would be to put drm_atomic_helper_shutdown >> before everything gets cleaned up. > > Hm, I already use the non-atomic drm_crtc_force_disable_all before, I > think that fixed the races I saw. > > But I guess what you are saying instead of using > drm_crtc_force_disable_all I should use drm_atomic_helper_shutdown...? Yes. I thought I audited all existing users of the legacy force_disable (it won't work correctly for atomic drivers), but somehow I missed the one in fsl. See commit 18dddadc78c91a91b546acc48506c24f5f840c4f Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Tue Mar 21 17:41:49 2017 +0100 drm/atomic: Introduce drm_atomic_helper_shutdown Cheers, Daniel
On 2017-05-26 00:00, Daniel Vetter wrote: > On Thu, May 25, 2017 at 10:18 AM, Stefan Agner <stefan@agner.ch> wrote: >> On 2017-05-24 07:51, Daniel Vetter wrote: >>> Again cleanup before irq disabling doesn't really stop the races, >>> so just drop it. Proper fix would be to put drm_atomic_helper_shutdown >>> before everything gets cleaned up. >> >> Hm, I already use the non-atomic drm_crtc_force_disable_all before, I >> think that fixed the races I saw. >> >> But I guess what you are saying instead of using >> drm_crtc_force_disable_all I should use drm_atomic_helper_shutdown...? > > Yes. I thought I audited all existing users of the legacy > force_disable (it won't work correctly for atomic drivers), but > somehow I missed the one in fsl. See > > commit 18dddadc78c91a91b546acc48506c24f5f840c4f > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Tue Mar 21 17:41:49 2017 +0100 > > drm/atomic: Introduce drm_atomic_helper_shutdown > Ok, I see. Since this leads to a change just a few lines above this patch, it has potential for merge conflict. Can I take this change through my tree? -- Stefan
On Tue, May 30, 2017 at 02:17:04PM -0700, Stefan Agner wrote: > On 2017-05-26 00:00, Daniel Vetter wrote: > > On Thu, May 25, 2017 at 10:18 AM, Stefan Agner <stefan@agner.ch> wrote: > >> On 2017-05-24 07:51, Daniel Vetter wrote: > >>> Again cleanup before irq disabling doesn't really stop the races, > >>> so just drop it. Proper fix would be to put drm_atomic_helper_shutdown > >>> before everything gets cleaned up. > >> > >> Hm, I already use the non-atomic drm_crtc_force_disable_all before, I > >> think that fixed the races I saw. > >> > >> But I guess what you are saying instead of using > >> drm_crtc_force_disable_all I should use drm_atomic_helper_shutdown...? > > > > Yes. I thought I audited all existing users of the legacy > > force_disable (it won't work correctly for atomic drivers), but > > somehow I missed the one in fsl. See > > > > commit 18dddadc78c91a91b546acc48506c24f5f840c4f > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Tue Mar 21 17:41:49 2017 +0100 > > > > drm/atomic: Introduce drm_atomic_helper_shutdown > > > > Ok, I see. > > Since this leads to a change just a few lines above this patch, it has > potential for merge conflict. Can I take this change through my tree? Yeah, it'll probably take until 4.14 anyway until I get them all in and finally can remove drm_vblank_cleanup from the driver-exported set of functions. Just make sure it does get into 4.13 pls. -Daniel
On 2017-05-31 01:52, Daniel Vetter wrote: > On Tue, May 30, 2017 at 02:17:04PM -0700, Stefan Agner wrote: >> On 2017-05-26 00:00, Daniel Vetter wrote: >> > On Thu, May 25, 2017 at 10:18 AM, Stefan Agner <stefan@agner.ch> wrote: >> >> On 2017-05-24 07:51, Daniel Vetter wrote: >> >>> Again cleanup before irq disabling doesn't really stop the races, >> >>> so just drop it. Proper fix would be to put drm_atomic_helper_shutdown >> >>> before everything gets cleaned up. >> >> >> >> Hm, I already use the non-atomic drm_crtc_force_disable_all before, I >> >> think that fixed the races I saw. >> >> >> >> But I guess what you are saying instead of using >> >> drm_crtc_force_disable_all I should use drm_atomic_helper_shutdown...? >> > >> > Yes. I thought I audited all existing users of the legacy >> > force_disable (it won't work correctly for atomic drivers), but >> > somehow I missed the one in fsl. See >> > >> > commit 18dddadc78c91a91b546acc48506c24f5f840c4f >> > Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> > Date: Tue Mar 21 17:41:49 2017 +0100 >> > >> > drm/atomic: Introduce drm_atomic_helper_shutdown >> > >> >> Ok, I see. >> >> Since this leads to a change just a few lines above this patch, it has >> potential for merge conflict. Can I take this change through my tree? > > Yeah, it'll probably take until 4.14 anyway until I get them all in and > finally can remove drm_vblank_cleanup from the driver-exported set of > functions. Just make sure it does get into 4.13 pls. Ok, applied to my fsl-dcu tree for 4.13. -- Stefan
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index 6e00f4b267f1..b34d09b59eee 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c @@ -109,7 +109,6 @@ static int fsl_dcu_load(struct drm_device *dev, unsigned long flags) drm_fbdev_cma_fini(fsl_dev->fbdev); drm_mode_config_cleanup(dev); - drm_vblank_cleanup(dev); drm_irq_uninstall(dev); dev->dev_private = NULL; @@ -127,7 +126,6 @@ static void fsl_dcu_unload(struct drm_device *dev) drm_fbdev_cma_fini(fsl_dev->fbdev); drm_mode_config_cleanup(dev); - drm_vblank_cleanup(dev); drm_irq_uninstall(dev); dev->dev_private = NULL;
Again cleanup before irq disabling doesn't really stop the races, so just drop it. Proper fix would be to put drm_atomic_helper_shutdown before everything gets cleaned up. Cc: Stefan Agner <stefan@agner.ch> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 -- 1 file changed, 2 deletions(-)