Plane transitional helper prototypes mismatch their methods

[Daniel Vetter]

On Sun, Jul 01, 2018 at 04:34:02PM +0100, Russell King - ARM Linux wrote:
> Ping.
> 
> On Mon, Jun 25, 2018 at 04:52:16PM +0100, Russell King - ARM Linux wrote:
> > Hi,
> > 
> > Currently, the transitional plane helpers have prototypes that
> > do not have the drm_modeset_acquire_ctx argument:
> > 
> > int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> >                             struct drm_framebuffer *fb,
> >                             int crtc_x, int crtc_y,
> >                             unsigned int crtc_w, unsigned int crtc_h,
> >                             uint32_t src_x, uint32_t src_y,
> >                             uint32_t src_w, uint32_t src_h);
> > int drm_plane_helper_disable(struct drm_plane *plane);
> > 
> > However, the helper methods have this as the last argument:
> > 
> >         int (*update_plane)(struct drm_plane *plane,
> >                             struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >                             int crtc_x, int crtc_y,
> >                             unsigned int crtc_w, unsigned int crtc_h,
> >                             uint32_t src_x, uint32_t src_y,
> >                             uint32_t src_w, uint32_t src_h,
> >                             struct drm_modeset_acquire_ctx *ctx);
> >         int (*disable_plane)(struct drm_plane *plane,
> >                              struct drm_modeset_acquire_ctx *ctx);
> > 
> > Are these transitional helpers supposed to be plugged into these
> > methods directly?  Looking back in the history, the following commit
> > to i915 seems to suggest that they were designed to be plugged
> > directly into these methods:
> > 
> > commit ea2c67bb4affa84080c616920f3899f123786e56
> > Author: Matt Roper <matthew.d.roper at intel.com>
> > Date:   Tue Dec 23 10:41:52 2014 -0800
> > 
> >     drm/i915: Move to atomic plane helpers (v9)
> > 
> > It seems easy to add this argument to drm_plane_helper_update() as
> > it has no current users, but the drm_plane_helper_disable()
> > transitional helper seems to be used extensively by what are
> > otherwise fully atomic modeset drivers, despite being described as
> > only a transitional helper.

Yes, I forgotten to add the acquire_ctx to them to make them match up
again. Upfront Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch> on the
patch to re-add that.

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

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().
- 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch