Re: [PATCH v1 4/4] drm/ast: Disable planes while switching display modes

[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 <tzimmermann@suse.de>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@vger.kernel.org> # 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.

> +
> +	/*
> +	 * 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch