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