On Mon, Jul 02, 2018 at 10:47:03AM +0200, Daniel Vetter wrote: > On Sun, Jul 01, 2018 at 04:34:02PM +0100, Russell King - ARM Linux wrote: > > > drivers/gpu/drm/arm/hdlcd_crtc.c: drm_plane_helper_disable(plane); > > > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c: drm_plane_helper_disable(plane); > > > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c: drm_plane_helper_disable(plane); > > > drivers/gpu/drm/arc/arcpgu_crtc.c: drm_plane_helper_disable(plane); > > > drivers/gpu/drm/sti/sti_hqvdp.c: drm_plane_helper_disable(drm_plane); > > > drivers/gpu/drm/sti/sti_gdp.c: drm_plane_helper_disable(drm_plane); > > > drivers/gpu/drm/sti/sti_cursor.c: drm_plane_helper_disable(drm_plane); > > > drivers/gpu/drm/vc4/vc4_plane.c: drm_plane_helper_disable(plane); > > > drivers/gpu/drm/zte/zx_plane.c: drm_plane_helper_disable(plane); > > > > > > Are these drivers buggy using the transitional helper, which won't do > > > anything but change the state of that single plane, leaving the CRTC, > > > encoders, bridges, etc all active? > > Yes that's all buggy usage. I've started sprinkling > WARN_ON(drm_drv_uses_adomit_modeset) on such legacy functions, but > drm_plane_helper_disable() is used a bit too much really. However, isn't there a problem in doing that, as going through the transition process means you: 1. switch planes to use the plane transitional helpers 2. migrate crtc to mode_set_nofb() 3. migrate page_flip to use a transitional implementation 4. clean up to use the plane state for all properties 5. switch to atomic modeset by adding .atomic_check / .atomic_commit and DRIVER_ATOMIC flag 6. migrate away from transitional helpers as separate stages - which means there's a brief period where you have the .atomic_commit method populated (and hence drm_drv_uses_atomic_modeset() returns true) but the transitional helpers are still being used. I've found if you do (5) and (6) in one go, it becomes quite a large set of changes: drivers/gpu/drm/armada/armada_crtc.c | 308 +------------------------------- drivers/gpu/drm/armada/armada_crtc.h | 20 --- drivers/gpu/drm/armada/armada_overlay.c | 175 ++++-------------- drivers/gpu/drm/armada/armada_plane.c | 4 +- 4 files changed, 36 insertions(+), 471 deletions(-) > It's all the same cargo-cult though in the plane->destroy hook. What > drivers should do instead is: > - Shut down the hw before cleaning up their modeset objects using > something like drm_atomic_helper_shutdown(). Hmm, that's something else I've missed in my conversion... thanks for pointing it out. ;) > - Remove the drm_plane_helper_disable(). > > But for your patch to add the ctx argument everywhere I think just passing > NULL is ok too. It's not going to make things worse :-) Since that patch > needs to touch a bunch of drivers probably best if we get it landed > through drm-misc-next. Ok, I've a commit for that - I just need to merge it with the one I already have adding the ctx argument, grab drm-misc-next and make sure it applies there... I'll try to get it out in shortly. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
