Re: [PATCH 2/4] drm/fb-helper: atomic restore_fbdev_mode()..

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 25, 2015 at 03:35:58PM -0400, Rob Clark wrote:
> Add support for using atomic code-paths for restore_fbdev_mode().
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 131 +++++++++++++++++++++---------------
>  drivers/gpu/drm/drm_fb_helper.c     |  74 ++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |   6 ++
>  include/drm/drm_fb_helper.h         |   8 +++
>  4 files changed, 166 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d432348..dbce582 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1502,21 +1502,9 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +	ret = __drm_atomic_helper_disable_plane(plane, plane_state);
>  	if (ret != 0)
>  		goto fail;
> -	drm_atomic_set_fb_for_plane(plane_state, NULL);
> -	plane_state->crtc_x = 0;
> -	plane_state->crtc_y = 0;
> -	plane_state->crtc_h = 0;
> -	plane_state->crtc_w = 0;
> -	plane_state->src_x = 0;
> -	plane_state->src_y = 0;
> -	plane_state->src_h = 0;
> -	plane_state->src_w = 0;
> -
> -	if (plane == plane->crtc->cursor)
> -		state->legacy_cursor_update = true;
>  
>  	ret = drm_atomic_commit(state);
>  	if (ret != 0)
> @@ -1546,6 +1534,32 @@ backoff:
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
>  
> +/* just used from fb-helper and atomic-helper: */
> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> +		struct drm_plane_state *plane_state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +	if (ret != 0)
> +		return ret;
> +
> +	drm_atomic_set_fb_for_plane(plane_state, NULL);
> +	plane_state->crtc_x = 0;
> +	plane_state->crtc_y = 0;
> +	plane_state->crtc_h = 0;
> +	plane_state->crtc_w = 0;
> +	plane_state->src_x = 0;
> +	plane_state->src_y = 0;
> +	plane_state->src_h = 0;
> +	plane_state->src_w = 0;
> +
> +	if (plane->crtc && (plane == plane->crtc->cursor))
> +		plane_state->state->legacy_cursor_update = true;
> +
> +	return 0;
> +}
> +
>  static int update_output_state(struct drm_atomic_state *state,
>  			       struct drm_mode_set *set)
>  {
> @@ -1629,8 +1643,6 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_crtc *crtc = set->crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_plane_state *primary_state;
>  	int ret = 0;
>  
>  	state = drm_atomic_state_alloc(crtc->dev);
> @@ -1639,17 +1651,54 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
>  
>  	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
>  retry:
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state)) {
> -		ret = PTR_ERR(crtc_state);
> +	ret = __drm_atomic_helper_set_config(set, state);
> +	if (ret != 0)
>  		goto fail;
> -	}
>  
> -	primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> -	if (IS_ERR(primary_state)) {
> -		ret = PTR_ERR(primary_state);
> +	ret = drm_atomic_commit(state);
> +	if (ret != 0)
>  		goto fail;
> -	}
> +
> +	/* Driver takes ownership of state on successful commit. */
> +	return 0;
> +fail:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_free(state);
> +
> +	return ret;
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_atomic_legacy_backoff(state);
> +
> +	/*
> +	 * Someone might have exchanged the framebuffer while we dropped locks
> +	 * in the backoff code. We need to fix up the fb refcount tracking the
> +	 * core does for us.
> +	 */
> +	crtc->primary->old_fb = crtc->primary->fb;
> +
> +	goto retry;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_set_config);
> +
> +/* just used from fb-helper and atomic-helper: */
> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> +		struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane_state *primary_state;
> +	struct drm_crtc *crtc = set->crtc;
> +	int ret;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> +	if (IS_ERR(primary_state))
> +		return PTR_ERR(primary_state);
>  
>  	if (!set->mode) {
>  		WARN_ON(set->fb);
> @@ -1657,13 +1706,13 @@ retry:
>  
>  		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
>  		if (ret != 0)
> -			goto fail;
> +			return ret;
>  
>  		crtc_state->active = false;
>  
>  		ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
>  		if (ret != 0)
> -			goto fail;
> +			return ret;
>  
>  		drm_atomic_set_fb_for_plane(primary_state, NULL);
>  
> @@ -1675,13 +1724,14 @@ retry:
>  
>  	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
>  	if (ret != 0)
> -		goto fail;
> +		return ret;
>  
>  	crtc_state->active = true;
>  
>  	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
>  	if (ret != 0)
> -		goto fail;
> +		return ret;
> +
>  	drm_atomic_set_fb_for_plane(primary_state, set->fb);
>  	primary_state->crtc_x = 0;
>  	primary_state->crtc_y = 0;
> @@ -1695,35 +1745,10 @@ retry:
>  commit:
>  	ret = update_output_state(state, set);
>  	if (ret)
> -		goto fail;
> -
> -	ret = drm_atomic_commit(state);
> -	if (ret != 0)
> -		goto fail;
> +		return ret;
>  
> -	/* Driver takes ownership of state on successful commit. */
>  	return 0;
> -fail:
> -	if (ret == -EDEADLK)
> -		goto backoff;
> -
> -	drm_atomic_state_free(state);
> -
> -	return ret;
> -backoff:
> -	drm_atomic_state_clear(state);
> -	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	crtc->primary->old_fb = crtc->primary->fb;
> -
> -	goto retry;
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_set_config);
>  
>  /**
>   * drm_atomic_helper_crtc_set_property - helper for crtc properties
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ba12f51..9741d79 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -38,6 +38,8 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  static bool drm_fbdev_emulation = true;
>  module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> @@ -334,6 +336,73 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>  
> +static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct drm_plane *plane;
> +	struct drm_atomic_state *state;
> +	int i, ret;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +retry:
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto fail;
> +		}
> +
> +		/* reset rotation: */

Comment is superflous imo. And we need to check for rotation_property here
like for the legacy path otherwise we'll blow up if that's not set up.

> +		ret = drm_atomic_plane_set_property(plane, plane_state,
> +				dev->mode_config.rotation_property,
> +				BIT(DRM_ROTATE_0));
> +		if (ret != 0)
> +			goto fail;
> +
> +		/* disable non-primary: */
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +			continue;
> +
> +		ret = __drm_atomic_helper_disable_plane(plane, plane_state);
> +		if (ret != 0)
> +			goto fail;
> +	}
> +
> +	for(i = 0; i < fb_helper->crtc_count; i++) {
> +		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> +
> +		ret = __drm_atomic_helper_set_config(mode_set, state);
> +		if (ret != 0)
> +			goto fail;
> +	}
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret != 0)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_free(state);
> +
> +	return ret;
> +
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_atomic_legacy_backoff(state);
> +
> +	goto retry;
> +}
> +
>  static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
> @@ -342,6 +411,9 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  
>  	drm_warn_on_modeset_not_all_locked(dev);
>  
> +	if (fb_helper->atomic)
> +		return restore_fbdev_mode_atomic(fb_helper);
> +
>  	drm_for_each_plane(plane, dev) {
>  		if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>  			drm_plane_force_disable(plane);
> @@ -644,6 +716,8 @@ int drm_fb_helper_init(struct drm_device *dev,
>  		i++;
>  	}
>  
> +	fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);
> +
>  	return 0;
>  out_free:
>  	drm_fb_helper_crtc_free(fb_helper);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 11266d14..88c0ef3 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -30,6 +30,8 @@
>  
>  #include <drm/drm_crtc.h>
>  
> +struct drm_atomic_state;
> +
>  int drm_atomic_helper_check_modeset(struct drm_device *dev,
>  				struct drm_atomic_state *state);
>  int drm_atomic_helper_check_planes(struct drm_device *dev,
> @@ -72,7 +74,11 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  				   uint32_t src_x, uint32_t src_y,
>  				   uint32_t src_w, uint32_t src_h);
>  int drm_atomic_helper_disable_plane(struct drm_plane *plane);
> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> +		struct drm_plane_state *plane_state);
>  int drm_atomic_helper_set_config(struct drm_mode_set *set);
> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> +		struct drm_atomic_state *state);
>  
>  int drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  					struct drm_property *property,
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6254136..cc3d820 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -117,6 +117,12 @@ struct drm_fb_helper_connector {
>   * @pseudo_palette: fake palette of 16 colors
>   * @kernel_fb_list: list_head in kernel_fb_helper_list
>   * @delayed_hotplug: was there a hotplug while kms master active?
> + * @atomic: use atomic updates for restore_fbdev_mode(), etc.  This
> + *          defaults to true if driver has DRIVER_ATOMIC feature
> + *          flag, but drivers can override it to true after
> + *          drm_fb_helper_init() if they support atomic modeset
> + *          but do not yet advertise DRIVER_ATOMIC (note that
> + *          fb-helper does not require ASYNC commits).

New kerneldoc in 4.3 (already in linux-next and drm-intel-nightly) allows
you to push that right to the struct member, which is imo better for
longer explanations like this:

>   */
>  struct drm_fb_helper {
>  	struct drm_framebuffer *fb;
> @@ -134,6 +140,8 @@ struct drm_fb_helper {
>  	/* we got a hotplug but fbdev wasn't running the console
>  	   delay until next set_par */
>  	bool delayed_hotplug;

	/**
	 * @atomic:
	 *
	 * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
	 * true if driver has DRIVER_ATOMIC feature flag, but drivers can override
	 * it to true after drm_fb_helper_init() if they support atomic modeset
	 * but do not yet advertise DRIVER_ATOMIC (note that fb-helper does not
	 * require ASYNC commits).
	 */
	bool atomic;

Imo that looks prettier.
-Daniel

>  };
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> -- 
> 2.4.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel