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@xxxxxxxxx> > > 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@xxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
