Plane transitional helper prototypes mismatch their methods

[Daniel Vetter]

On Mon, Jul 2, 2018 at 5:41 PM, Russell King - ARM Linux
<linux at armlinux.org.uk> wrote:
> 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(-)

Hm yeah that doesn't work. I'd say that's perhaps cleanup work for
someone bored after armada atomic has landed.

I also think that after armada it probably makes sense to drop the
transitional helpers. Anyone else trying to do a legacy2atomic
conversion can dig them out of the git history and just carry them
around in their driver for the transition. That also avoids the
constant headaches of them breaking accidentally all the time.
-Daniel

>> 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch