[Intel-gfx] [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.

[Ville Syrjälä]

On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> This introduces a slight behavioral change to rmfb. Instead of
> disabling a crtc when the primary plane is disabled, we try to
> preserve it.
> 
> Apart from old versions of the vmwgfx xorg driver, there is
> nothing depending on rmfb disabling a crtc.
> 
> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> enabled without plane, so we can do this safely.
> 
> If the atomic commit is rejected by the driver then we will still
> fall back to the old behavior and turn off the crtc.
> 
> Changes since v1:
> - Restart completely when rmfb with crtc on fails (Sean Paul).
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Sean Paul <seanpaul at chromium.org>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 2affe53f3fda..f0679468f421 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>  	struct drm_plane *plane;
>  	struct drm_connector *conn;
>  	struct drm_connector_state *conn_state;
> -	int i, ret = 0;
> +	int i, ret;
>  	unsigned plane_mask;
> +	bool disable_crtcs = false;
>  
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> +retry_disable:
>  	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  	state->acquire_ctx = &ctx;
>  
>  retry:
> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>  			goto unlock;
>  		}
>  
> -		if (plane_state->crtc->primary == plane) {
> +		if (disable_crtcs && plane_state->crtc->primary == plane) {
>  			struct drm_crtc_state *crtc_state;
>  
>  			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>  		plane->old_fb = plane->fb;
>  	}
>  
> +	/* This list is only filled when disable_crtcs is set. */
>  	for_each_new_connector_in_state(state, conn, conn_state, i) {

WARN_ON(!disable_crtcs) maybe?

>  		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>  
> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>  
>  	drm_atomic_state_put(state);
>  
> +out:
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  
> +	if (ret == -EINVAL && !disable_crtcs) {

Hmm. -EINVAL seems rather specific. Not sure if we could just check for
any error?

Or... I'm not sure if we have any central place where we do the
"can I disable the primary plane w/o disabling the crtc?" check. If we
do then we could also add a comment there informing people that the
-EINVAL is important.

> +		disable_crtcs = true;
> +		goto retry_disable;
> +	}
> +
>  	return ret;
>  }
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC