Plane transitional helper prototypes mismatch their methods

[Russell King - ARM Linux]

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