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.
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?
-- 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
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
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.
On Mon, Jul 2, 2018 at 5:41 PM, Russell King - ARM Linux linux@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:
- switch planes to use the plane transitional helpers
- migrate crtc to mode_set_nofb()
- migrate page_flip to use a transitional implementation
- clean up to use the plane state for all properties
- switch to atomic modeset by adding .atomic_check / .atomic_commit and DRIVER_ATOMIC flag
- 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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Russell King - ARM Linux