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

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