So the issues that Inki and Bjorn ran into with ww_acquire_fini() vs legacy fbdev codepaths (like restore_fbdev_mode() and pan_display()) would be solved by using a single atomic update for these cases. So I hacked up a prototype.
(disclaimer, these are completely untested hacked-up-between-sessions- at-conference patches.. but at least they compile ;-))
I think Daniel wanted something other than DRIVER_ATOMIC check, since i915 currently supports atomic other than async. Other option would be a separate DRIVER_ATOMIC_ASYNC flag (which has some advantage, in that we could start using atomic modeset, but not atomic pageflip, on i915.. and perhaps finally start work on atomic xrandr). Other option would be to, for now, just introduce an fb_helper->use_atomic flag, which driver could override, instead.
For now we should probably revert the ww_acquire_fini patch, and then shoot for re-introducing it on top of these patches (hopefully working by then) for 4.4. And maybe in the process we can fix some i915 multi- monitor VT switch fail.
Rob Clark (2): drm/fb-helper: atomic restore_fbdev_mode().. drm/fb-helper: atomic pan_display()..
drivers/gpu/drm/drm_atomic_helper.c | 129 ++++++++++++++++++++-------------- drivers/gpu/drm/drm_fb_helper.c | 133 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 6 ++ 3 files changed, 215 insertions(+), 53 deletions(-)
Somewhat rough, not sure if some of the code should be moved to different places, etc..
Only compile tested atm..
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/drm_atomic_helper.c | 129 +++++++++++++++++++++--------------- drivers/gpu/drm/drm_fb_helper.c | 74 +++++++++++++++++++++ include/drm/drm_atomic_helper.h | 6 ++ 3 files changed, 156 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 52dbeed..a06068a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1494,21 +1494,9 @@ retry: goto fail; }
- ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + ret = drm_atomic_helper_disable_plane_impl(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) @@ -1538,6 +1526,32 @@ backoff: } EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
+/* just used from fb-helper and atomic-helper: */ +int drm_atomic_helper_disable_plane_impl(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 == 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) { @@ -1621,8 +1635,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); @@ -1631,17 +1643,52 @@ 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_impl(set, 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_impl(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)) { - ret = PTR_ERR(primary_state); - goto fail; - } + if (IS_ERR(primary_state)) + return PTR_ERR(primary_state);
if (!set->mode) { WARN_ON(set->fb); @@ -1649,13 +1696,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);
@@ -1667,13 +1714,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; @@ -1687,35 +1735,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 418d299..3549b1f 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 LIST_HEAD(kernel_fb_helper_list);
@@ -320,6 +322,75 @@ 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; + + // XXX this may be fubar in the opps_in_progress case.. + 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: */ + 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_impl(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_impl(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); + // XXX this may also be a bit fubar in opps_in_progress + drm_atomic_legacy_backoff(state); + + goto retry; +} + static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; @@ -329,6 +400,9 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
drm_warn_on_modeset_not_all_locked(dev);
+ if (drm_core_check_feature(dev, DRIVER_ATOMIC)) + return restore_fbdev_mode_atomic(fb_helper) == 0; + drm_for_each_plane(plane, dev) { if (plane->type != DRM_PLANE_TYPE_PRIMARY) drm_plane_force_disable(plane); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 11266d14..555f9b9 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_impl(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_impl(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,
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/drm_fb_helper.c | 59 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 3549b1f..eef1687 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1192,6 +1192,59 @@ int drm_fb_helper_set_par(struct fb_info *info) } EXPORT_SYMBOL(drm_fb_helper_set_par);
+static int pan_display_atomic(struct fb_var_screeninfo *var, + struct fb_info *info) +{ + struct drm_fb_helper *fb_helper = info->par; + struct drm_device *dev = fb_helper->dev; + 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: + for(i = 0; i < fb_helper->crtc_count; i++) { + struct drm_mode_set *mode_set; + + mode_set = &fb_helper->crtc_info[i].mode_set; + + mode_set->x = var->xoffset; + mode_set->y = var->yoffset; + + ret = drm_atomic_helper_set_config_impl(mode_set, state); + if (ret != 0) + goto fail; + } + + ret = drm_atomic_commit(state); + if (ret != 0) + goto fail; + + // XXX if fail, revert modeset[i]->x,y?? legacy path doesn't seem to do it either.. + + info->var.xoffset = var->xoffset; + info->var.yoffset = var->yoffset; + + 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; +} + /** * drm_fb_helper_pan_display - implementation for ->fb_pan_display * @var: updated screen information @@ -1215,6 +1268,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }
+ if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { + ret = pan_display_atomic(var, info); + goto unlock; + } + for (i = 0; i < fb_helper->crtc_count; i++) { modeset = &fb_helper->crtc_info[i].mode_set;
@@ -1229,6 +1287,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, } } } +unlock: drm_modeset_unlock_all(dev); return ret; }
On Wed, Aug 19, 2015 at 06:45:15PM -0400, Rob Clark wrote:
So the issues that Inki and Bjorn ran into with ww_acquire_fini() vs legacy fbdev codepaths (like restore_fbdev_mode() and pan_display()) would be solved by using a single atomic update for these cases. So I hacked up a prototype.
(disclaimer, these are completely untested hacked-up-between-sessions- at-conference patches.. but at least they compile ;-))
I think Daniel wanted something other than DRIVER_ATOMIC check, since i915 currently supports atomic other than async. Other option would be a separate DRIVER_ATOMIC_ASYNC flag (which has some advantage, in that we could start using atomic modeset, but not atomic pageflip, on i915.. and perhaps finally start work on atomic xrandr). Other option would be to, for now, just introduce an fb_helper->use_atomic flag, which driver could override, instead.
Not a separate driver flag, but simply something to force fbdev helper to use atomic without DRIVER_ATOMIC. That way partially-converted drivers can switch things on their own. With that addressed and testing done this looks ready imo and I'll pull it in.
For now we should probably revert the ww_acquire_fini patch, and then shoot for re-introducing it on top of these patches (hopefully working by then) for 4.4. And maybe in the process we can fix some i915 multi- monitor VT switch fail.
Yeah, Dave already pushed the revert. -Daniel
Rob Clark (2): drm/fb-helper: atomic restore_fbdev_mode().. drm/fb-helper: atomic pan_display()..
drivers/gpu/drm/drm_atomic_helper.c | 129 ++++++++++++++++++++-------------- drivers/gpu/drm/drm_fb_helper.c | 133 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 6 ++ 3 files changed, 215 insertions(+), 53 deletions(-)
-- 2.4.3
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Rob Clark