Hi Am 07.08.20 um 10:50 schrieb daniel@ffwll.ch: > On Wed, Aug 05, 2020 at 12:54:28PM +0200, Thomas Zimmermann wrote: >> The ast HW cursor requires the primary plane and CRTC to display at >> a valid mode and format. This is not the case while switching >> display modes, which can lead to the screen turing permanently dark. >> >> As a workaround, the ast driver now disables active planes while the >> mode or format switch takes place. It also synchronizes with the vertical >> refresh to give CRTC and planes some time to catch up on each other. >> The active planes planes (primary or cursor) will be re-enabled by >> each plane's atomic_update() function. >> >> v2: >> * move the logic into the commit-tail function >> >> Signed-off-by: Thomas Zimmermann >> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting") >> Cc: Thomas Zimmermann >> Cc: Gerd Hoffmann >> Cc: Dave Airlie >> Cc: Daniel Vetter >> Cc: Sam Ravnborg >> Cc: Emil Velikov >> Cc: "Y.C. Chen" >> Cc: # v5.6+ >> --- >> drivers/gpu/drm/ast/ast_drv.h | 2 + >> drivers/gpu/drm/ast/ast_mode.c | 68 ++++++++++++++++++++++++++++++++-- >> 2 files changed, 66 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h >> index c1af6b725933..467049ca8430 100644 >> --- a/drivers/gpu/drm/ast/ast_drv.h >> +++ b/drivers/gpu/drm/ast/ast_drv.h >> @@ -177,6 +177,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv, >> >> #define AST_IO_MM_OFFSET (0x380) >> >> +#define AST_IO_VGAIR1_VREFRESH BIT(3) >> + >> #define __ast_read(x) \ >> static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \ >> u##x val = 0;\ >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index ae5cb0a333f7..a379d51f3543 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -514,6 +514,17 @@ static void ast_set_start_address_crt1(struct ast_private *ast, >> >> } >> >> +static void ast_wait_for_vretrace(struct ast_private *ast) >> +{ >> + unsigned long timeout = jiffies + HZ; >> + u8 vgair1; >> + >> + do { >> + vgair1 = ast_io_read8(ast, AST_IO_INPUT_STATUS1_READ); >> + } while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) && >> + time_before(jiffies, timeout)); >> +} >> + >> /* >> * Primary plane >> */ >> @@ -1043,23 +1054,72 @@ static int ast_connector_init(struct drm_device *dev) >> * Mode config >> */ >> >> +static bool >> +ast_crtc_needs_planes_disabled(struct drm_crtc_state *old_crtc_state, >> + struct drm_crtc_state *new_crtc_state) >> +{ >> + struct ast_crtc_state *old_ast_crtc_state, *new_ast_crtc_state; >> + >> + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) >> + return true; >> + >> + old_ast_crtc_state = to_ast_crtc_state(old_crtc_state); >> + new_ast_crtc_state = to_ast_crtc_state(new_crtc_state); >> + >> + if (old_ast_crtc_state->format != new_ast_crtc_state->format) >> + return true; >> + >> + return false; >> +} >> + >> static void >> ast_mode_config_helper_commit_tail(struct drm_atomic_state *old_state) >> { >> struct drm_device *dev = old_state->dev; >> + struct ast_private *ast = to_ast_private(dev); >> + struct drm_crtc_state *old_crtc_state, *new_crtc_state; >> + struct drm_crtc *crtc; >> + int i; >> + bool wait_for_vretrace = false; >> >> drm_atomic_helper_commit_modeset_disables(dev, old_state); >> >> - drm_atomic_helper_commit_planes(dev, old_state, 0); >> + /* >> + * HW cursors require the underlying primary plane and CRTC to >> + * display a valid mode and image. This is not the case during >> + * full modeset operations. So we temporarily disable any active >> + * plane, including the HW cursor. Each plane's atomic_update() >> + * helper will re-enable it if necessary. >> + * >> + * We only do this during *full* modesets. It does not affect >> + * simple pageflips on the planes. >> + */ >> + for_each_oldnew_crtc_in_state(old_state, crtc, >> + old_crtc_state, >> + new_crtc_state, i) { >> + if (!ast_crtc_needs_planes_disabled(old_crtc_state, >> + new_crtc_state)) >> + continue; >> + drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, >> + false); >> + wait_for_vretrace = true; >> + } > > Hm this still feels like you're fighting the framework more than using it. > Comment here, but it's kinda review comments on the entire series. > > - ast_crtc_needs_planes_disabled feels a bit strange, the usual way to > handle this kind of stuff is to set crtc_state->needs_modeset from your > plane's atomic_check function. You might need your own atomic_check > implementation for that, so that after the plane checks you run the > modeset checks again. > > - with that you can put your call here to disable all planes into the crtc > ->atomic_disable callback. You can then also put the > ast_wait_for_retrace in there, at the end. The CRTC's atomic_disable/enable only run if needs_modeset() is true. I brought back support for fast format changes of the primary plane. Moving that code into atomic_disable/enable would require to set needs_modeset in atomic_check() for format changes. And later figure out in atomic_disable/enable if it's really a modeset or just a change of the format. That's not good either. Best regards Thomas > >> + >> + /* >> + * Ensure that no scanout takes place before reprogramming mode >> + * and format registers. >> + */ >> + if (wait_for_vretrace) >> + ast_wait_for_vretrace(ast); >> + >> + drm_atomic_helper_commit_planes(dev, old_state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > > This order also feels a bit strange, especially with the first 2 patches > where you put the crtc modeset code into atomic_begin. It feels a bit like > if you do the plane commit _after_ modeset enables, then you could move > the crtc code into the crtc ->atomic_enable hook, and then let the plane > update stuff roll through all in commit_planes. Moving the modset code > into atomic_begin at least suggests you want modeset enables before plane > commit, and lots of drivers have that sequence in their commit_tail. It's > even a default implementation with drm_atomic_helper_commit_tail_rpm. > > Sorry this is all dragging around so much, figuring out the best atomic > flow is occasionally a bit an endeavour :-/ > > Cheers, Daniel > >> >> drm_atomic_helper_commit_modeset_enables(dev, old_state); >> >> drm_atomic_helper_fake_vblank(old_state); >> - >> drm_atomic_helper_commit_hw_done(old_state); >> - >> drm_atomic_helper_wait_for_vblanks(dev, old_state); >> - >> drm_atomic_helper_cleanup_planes(dev, old_state); >> } >> >> -- >> 2.28.0 >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer