From: Thierry Reding treding@nvidia.com

Move the modeset locking from drivers into FB helpers.

v2: Also handle intel_connector_add_to_fbdev.

Tested-by: John Stultz john.stultz@linaro.org Signed-off-by: Thierry Reding treding@nvidia.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_dp_mst.c | 6 ----- drivers/gpu/drm/radeon/radeon_dp_mst.c | 7 ------ 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 574af01d3ce9..5e73cc9f51e3 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -109,8 +109,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \ i__ = 0; i__ < (fbh)->connector_count; i__++)

-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, - struct drm_connector *connector) +static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, + struct drm_connector *connector) { struct drm_fb_helper_connector *fb_conn; struct drm_fb_helper_connector **temp; @@ -141,8 +141,23 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, drm_connector_get(connector); fb_conn->connector = connector; fb_helper->connector_info[fb_helper->connector_count++] = fb_conn; + return 0; } + +int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, + struct drm_connector *connector) +{ + int err; + + mutex_lock(&fb_helper->dev->mode_config.mutex); + + err = __drm_fb_helper_add_one_connector(fb_helper, connector); + + mutex_unlock(&fb_helper->dev->mode_config.mutex); + + return err; +} EXPORT_SYMBOL(drm_fb_helper_add_one_connector);

/** @@ -172,8 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) mutex_lock(&dev->mode_config.mutex); drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { - ret = drm_fb_helper_add_one_connector(fb_helper, connector); - + ret = __drm_fb_helper_add_one_connector(fb_helper, connector); if (ret) goto fail; } @@ -198,8 +212,8 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) } EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);

-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, - struct drm_connector *connector) +static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, + struct drm_connector *connector) { struct drm_fb_helper_connector *fb_helper_connector; int i, j; @@ -227,6 +241,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,

return 0; } + +int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, + struct drm_connector *connector) +{ + int err; + + mutex_lock(&fb_helper->dev->mode_config.mutex); + + err = __drm_fb_helper_remove_one_connector(fb_helper, connector); + + mutex_unlock(&fb_helper->dev->mode_config.mutex); + + return err; +} EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);

static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper) diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 2cf046beae0f..5b78165882ce 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -501,11 +501,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo static void intel_dp_register_mst_connector(struct drm_connector *connector) { struct intel_connector *intel_connector = to_intel_connector(connector); - struct drm_device *dev = connector->dev;

- drm_modeset_lock_all(dev); intel_connector_add_to_fbdev(intel_connector); - drm_modeset_unlock_all(dev);

drm_connector_register(&intel_connector->base); } @@ -514,15 +511,12 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct drm_connector *connector) { struct intel_connector *intel_connector = to_intel_connector(connector); - struct drm_device *dev = connector->dev;

drm_connector_unregister(connector);

/* need to nuke the connector */ - drm_modeset_lock_all(dev); intel_connector_remove_from_fbdev(intel_connector); intel_connector->mst_port = NULL; - drm_modeset_unlock_all(dev);

drm_connector_unreference(&intel_connector->base); DRM_DEBUG_KMS("\n"); diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index 6598306dca9b..ebdf1b859cb6 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -300,9 +300,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct radeon_device *rdev = dev->dev_private;

- drm_modeset_lock_all(dev); radeon_fb_add_connector(rdev, connector); - drm_modeset_unlock_all(dev);

drm_connector_register(connector); } @@ -315,13 +313,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct radeon_device *rdev = dev->dev_private;

drm_connector_unregister(connector); - /* need to nuke the connector */ - drm_modeset_lock_all(dev); - /* dpms off */ radeon_fb_remove_connector(rdev, connector); - drm_connector_cleanup(connector); - drm_modeset_unlock_all(dev);

kfree(connector); DRM_DEBUG_KMS("\n");

From: Thierry Reding treding@nvidia.com

Introduce a new top-level lock for the FB helper code. This will allow better locking granularity and avoid the need to abuse modeset locking for this purpose instead.

This patch just adds the new lock everywhere we currently grab mode_config->mutex (explicitly, or through drm_modeset_lock_all). Follow-up patches will push the kms locking down into only the places that need it.

v2: - use lockdep_assert_held - use drm_fb_helper_for_each_connector where possible - use the new top-level lock consistently, i.e. in all the places we're currently acquiring mode_config.mutex. - small polish to the kerneldoc

Tested-by: John Stultz john.stultz@linaro.org Signed-off-by: Thierry Reding treding@nvidia.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fb_helper.c | 47 +++++++++++++++++++++++++++++++++++++---- include/drm/drm_fb_helper.h | 19 ++++++++++++++++- 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5e73cc9f51e3..a0d4603857d8 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -119,7 +119,8 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, if (!drm_fbdev_emulation) return 0;

- WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex)); + lockdep_assert_held(&fb_helper->lock); + lockdep_assert_held(&fb_helper->dev->mode_config.mutex);

count = fb_helper->connector_count + 1;

@@ -150,11 +151,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, { int err;

+ mutex_lock(&fb_helper->lock); mutex_lock(&fb_helper->dev->mode_config.mutex);

err = __drm_fb_helper_add_one_connector(fb_helper, connector);

mutex_unlock(&fb_helper->dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock);

return err; } @@ -184,6 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) if (!drm_fbdev_emulation) return 0;

+ mutex_lock(&fb_helper->lock); mutex_lock(&dev->mode_config.mutex); drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { @@ -207,6 +211,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) out: drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock);

return ret; } @@ -221,9 +226,9 @@ static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, if (!drm_fbdev_emulation) return 0;

- WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex)); + lockdep_assert_held(&fb_helper->lock);

- for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { if (fb_helper->connector_info[i]->connector == connector) break; } @@ -247,11 +252,13 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, { int err;

+ mutex_lock(&fb_helper->lock); mutex_lock(&fb_helper->dev->mode_config.mutex);

err = __drm_fb_helper_remove_one_connector(fb_helper, connector);

mutex_unlock(&fb_helper->dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock);

return err; } @@ -517,16 +524,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) if (!drm_fbdev_emulation) return -ENODEV;

+ mutex_lock(&fb_helper->lock); drm_modeset_lock_all(dev); + ret = restore_fbdev_mode(fb_helper);

do_delayed = fb_helper->delayed_hotplug; if (do_delayed) fb_helper->delayed_hotplug = false; + drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock);

if (do_delayed) drm_fb_helper_hotplug_event(fb_helper); + return ret; } EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked); @@ -576,11 +588,13 @@ static bool drm_fb_helper_force_kernel_mode(void) if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) continue;

+ mutex_lock(&helper->lock); drm_modeset_lock_all(dev); ret = restore_fbdev_mode(helper); if (ret) error = true; drm_modeset_unlock_all(dev); + mutex_unlock(&helper->lock); } return error; } @@ -620,9 +634,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) /* * For each CRTC in this fb, turn the connectors on/off. */ + mutex_lock(&fb_helper->lock); drm_modeset_lock_all(dev); if (!drm_fb_helper_is_bound(fb_helper)) { drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock); return; }

@@ -641,6 +657,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) } } drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock); }

/** @@ -762,6 +779,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker); INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work); helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0; + mutex_init(&helper->lock); helper->funcs = funcs; helper->dev = dev; } @@ -927,6 +945,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) } mutex_unlock(&kernel_fb_helper_lock);

+ mutex_destroy(&fb_helper->lock); drm_fb_helper_crtc_free(fb_helper);

} @@ -1257,9 +1276,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) if (oops_in_progress) return -EBUSY;

+ mutex_lock(&fb_helper->lock); drm_modeset_lock_all(dev); if (!drm_fb_helper_is_bound(fb_helper)) { drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock); return -EBUSY; }

@@ -1292,6 +1313,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) } out: drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock); return rc; } EXPORT_SYMBOL(drm_fb_helper_setcmap); @@ -1314,6 +1336,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, struct drm_crtc *crtc; int ret = 0;

+ mutex_lock(&fb_helper->lock); mutex_lock(&dev->mode_config.mutex); if (!drm_fb_helper_is_bound(fb_helper)) { ret = -EBUSY; @@ -1360,6 +1383,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,

unlock: mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock); return ret; } EXPORT_SYMBOL(drm_fb_helper_ioctl); @@ -1589,9 +1613,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, if (oops_in_progress) return -EBUSY;

+ mutex_lock(&fb_helper->lock); drm_modeset_lock_all(dev); if (!drm_fb_helper_is_bound(fb_helper)) { drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock); return -EBUSY; }

@@ -1600,6 +1626,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, else ret = pan_display_legacy(var, info); drm_modeset_unlock_all(dev); + mutex_unlock(&fb_helper->lock);

return ret; } @@ -2266,6 +2293,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, DRM_DEBUG_KMS("No connectors reported connected with modes\n");

/* prevent concurrent modification of connector_count by hotplug */ + lockdep_assert_held(&fb_helper->lock); lockdep_assert_held(&fb_helper->dev->mode_config.mutex);

crtcs = kcalloc(fb_helper->connector_count, @@ -2392,12 +2420,14 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) if (!drm_fbdev_emulation) return 0;

+ mutex_lock(&fb_helper->lock); mutex_lock(&dev->mode_config.mutex); drm_setup_crtcs(fb_helper, dev->mode_config.max_width, dev->mode_config.max_height); ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock); if (ret) return ret;

@@ -2445,25 +2475,34 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; + int err = 0;

if (!drm_fbdev_emulation) return 0;

+ mutex_lock(&fb_helper->lock); mutex_lock(&dev->mode_config.mutex); + if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) { fb_helper->delayed_hotplug = true; mutex_unlock(&dev->mode_config.mutex); - return 0; + goto unlock; } + DRM_DEBUG_KMS("\n");

drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);

mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&fb_helper->lock);

drm_fb_helper_set_par(fb_helper->fbdev);

return 0; + +unlock: + mutex_unlock(&fb_helper->lock); + return err; } EXPORT_SYMBOL(drm_fb_helper_hotplug_event);

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 119e5e4609c7..ea170b96e88d 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -169,7 +169,6 @@ struct drm_fb_helper_connector { * @crtc_info: per-CRTC helper state (mode, x/y offset, etc) * @connector_count: number of connected connectors * @connector_info_alloc_count: size of connector_info - * @connector_info: array of per-connector information * @funcs: driver callbacks for fb helper * @fbdev: emulated fbdev device info struct * @pseudo_palette: fake palette of 16 colors @@ -191,6 +190,12 @@ struct drm_fb_helper { struct drm_fb_helper_crtc *crtc_info; int connector_count; int connector_info_alloc_count; + /** + * @connector_info: + * + * Array of per-connector information. Do not iterate directly, but use + * drm_fb_helper_for_each_connector. + */ struct drm_fb_helper_connector **connector_info; const struct drm_fb_helper_funcs *funcs; struct fb_info *fbdev; @@ -201,6 +206,18 @@ struct drm_fb_helper { struct work_struct resume_work;

/** + * @lock: + * + * Top-level FBDEV helper lock. This protects all internal data + * structures and lists, such as @connector_info and @crtc_info. + * + * FIXME: fbdev emulation locking is a mess and long term we want to + * protect all helper internal state with this lock as well as reduce + * core KMS locking as much as possible. + */ + struct mutex lock; + + /** * @kernel_fb_list: * * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.

Like with the drm-native vblank wait ioctl we can entirely rely on the spinlocks in drm_vblank.c, no need at all to take expensive mutexes. The only reason we had to take mode_config.mutex was to protect the fbdev helper's data-structures, but that's now done by fb_helper->lock.

Cc: John Stultz john.stultz@linaro.org Cc: Thierry Reding treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_helper.c | 2 -- 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 14b3f885a01f..13330c22e6bf 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1342,7 +1342,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, goto unlock; }

- mutex_lock(&dev->mode_config.mutex); switch (cmd) { case FBIO_WAITFORVSYNC: /* @@ -1382,7 +1381,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, }

unlock: - mutex_unlock(&dev->mode_config.mutex); mutex_unlock(&fb_helper->lock); return ret; }

Same game as with the panning function, use drm_modeset_lock_all for legacy paths, and a proper acquire ctx w/w mutex dance for atomic.

Cc: John Stultz john.stultz@linaro.org Cc: Thierry Reding treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_helper.c | 48 +++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5d8bf7dfa618..400bbb07eff2 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -396,12 +396,17 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) struct drm_atomic_state *state; int i, ret; unsigned int plane_mask; + struct drm_modeset_acquire_ctx ctx; + + drm_modeset_acquire_init(&ctx, 0);

state = drm_atomic_state_alloc(dev); - if (!state) - return -ENOMEM; + if (!state) { + ret = -ENOMEM; + goto out_ctx; + }

- state->acquire_ctx = dev->mode_config.acquire_ctx; + state->acquire_ctx = &ctx; retry: plane_mask = 0; drm_for_each_plane(plane, dev) { @@ -410,7 +415,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { ret = PTR_ERR(plane_state); - goto fail; + goto out_state; }

plane_state->rotation = DRM_MODE_ROTATE_0; @@ -424,7 +429,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)

ret = __drm_atomic_helper_disable_plane(plane, plane_state); if (ret != 0) - goto fail; + goto out_state; }

for (i = 0; i < fb_helper->crtc_count; i++) { @@ -432,23 +437,27 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)

ret = __drm_atomic_helper_set_config(mode_set, state); if (ret != 0) - goto fail; + goto out_state; }

ret = drm_atomic_commit(state);

-fail: +out_state: drm_atomic_clean_old_fb(dev, plane_mask, ret);

if (ret == -EDEADLK) goto backoff;

drm_atomic_state_put(state); +out_ctx: + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + return ret;

backoff: drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); + drm_modeset_backoff(&ctx);

goto retry; } @@ -457,8 +466,9 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; struct drm_plane *plane; - int i; + int i, ret = 0;

+ drm_modeset_lock_all(fb_helper->dev); drm_for_each_plane(plane, dev) { if (plane->type != DRM_PLANE_TYPE_PRIMARY) drm_plane_force_disable(plane); @@ -472,32 +482,31 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper) for (i = 0; i < fb_helper->crtc_count; i++) { struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set; struct drm_crtc *crtc = mode_set->crtc; - int ret;

if (crtc->funcs->cursor_set2) { ret = crtc->funcs->cursor_set2(crtc, NULL, 0, 0, 0, 0, 0); if (ret) - return ret; + goto out; } else if (crtc->funcs->cursor_set) { ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); if (ret) - return ret; + goto out; }

ret = drm_mode_set_config_internal(mode_set); if (ret) - return ret; + goto out; } +out: + drm_modeset_unlock_all(fb_helper->dev);

- return 0; + return ret; }

static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev;

- drm_warn_on_modeset_not_all_locked(dev); - if (drm_drv_uses_atomic_modeset(dev)) return restore_fbdev_mode_atomic(fb_helper); else @@ -517,7 +526,6 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) */ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) { - struct drm_device *dev = fb_helper->dev; bool do_delayed; int ret;

@@ -525,15 +533,11 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return -ENODEV;

mutex_lock(&fb_helper->lock); - drm_modeset_lock_all(dev); - ret = restore_fbdev_mode(fb_helper);

do_delayed = fb_helper->delayed_hotplug; if (do_delayed) fb_helper->delayed_hotplug = false; - - drm_modeset_unlock_all(dev); mutex_unlock(&fb_helper->lock);

if (do_delayed) @@ -591,11 +595,9 @@ static bool drm_fb_helper_force_kernel_mode(void) continue;

mutex_lock(&helper->lock); - drm_modeset_lock_all(dev); ret = restore_fbdev_mode(helper); if (ret) error = true; - drm_modeset_unlock_all(dev); mutex_unlock(&helper->lock); } return error;

Like with panning and modesetting, and like with those, stick with simple drm_modeset_locking_all for the legacy path, and the full atomic dance for atomic drivers.

This means a bit more boilerplate since setting up the atomic state machinery is rather verbose, but then this is shared code for 30+ drivers or so, so meh.

After this patch there's only the LUT/cmap path which is still using drm_modeset_lock_all for an atomic driver. But Peter is already locking into reworking that, so I'll leave that code as-is for now.

Cc: Peter Rosin peda@axentia.se Cc: John Stultz john.stultz@linaro.org Cc: Thierry Reding treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_helper.c | 88 +++++++++++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 59e2916471b2..bbd4c6d0378d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -616,23 +616,13 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; #endif

-static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode) { - struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; struct drm_connector *connector; int i, j;

- /* - * For each CRTC in this fb, turn the connectors on/off. - */ - mutex_lock(&fb_helper->lock); - if (!drm_fb_helper_is_bound(fb_helper)) { - mutex_unlock(&fb_helper->lock); - return; - } - drm_modeset_lock_all(dev); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; @@ -649,6 +639,81 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) } } drm_modeset_unlock_all(dev); +} + +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode) +{ + struct drm_device *dev = fb_helper->dev; + struct drm_atomic_state *state; + int i, ret; + + struct drm_modeset_acquire_ctx ctx; + + drm_modeset_acquire_init(&ctx, 0); + + state = drm_atomic_state_alloc(dev); + if (!state) { + ret = -ENOMEM; + goto out_ctx; + } + + state->acquire_ctx = &ctx; +retry: + for (i = 0; i < fb_helper->crtc_count; i++) { + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + + if (!fb_helper->crtc_info[i].mode_set.mode) + continue; + + crtc = fb_helper->crtc_info[i].mode_set.crtc; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + goto out_state; + } + + crtc_state->active = dpms_mode == DRM_MODE_DPMS_ON; + } + + ret = drm_atomic_commit(state); +out_state: + if (ret == -EDEADLK) + goto backoff; + + drm_atomic_state_put(state); +out_ctx: + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return; + +backoff: + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + + goto retry; + +} + +static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +{ + struct drm_fb_helper *fb_helper = info->par; + + /* + * For each CRTC in this fb, turn the connectors on/off. + */ + mutex_lock(&fb_helper->lock); + if (!drm_fb_helper_is_bound(fb_helper)) { + mutex_unlock(&fb_helper->lock); + return; + } + + if (drm_drv_uses_atomic_modeset(fb_helper->dev)) + dpms_atomic(fb_helper, dpms_mode); + else + dpms_legacy(fb_helper, dpms_mode); mutex_unlock(&fb_helper->lock); }

@@ -2467,7 +2532,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); */ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { - struct drm_device *dev = fb_helper->dev; int err = 0;

if (!drm_fbdev_emulation)

FB helper code falls back to a 1024x768 mode if no outputs are connected or don't report back any modes upon initialization. This can be annoying because outputs that are added to FB helper later on can't be used with FB helper if they don't support a matching mode.

The fallback is in place because VGA connectors can happen to report an unknown connection status even when they are in fact connected.

Some drivers have custom solutions in place to defer FB helper setup until at least one output is connected. But the logic behind these solutions is always the same and there is nothing driver-specific about it, so a better alterative is to fix the FB helper core and add support for all drivers automatically.

This patch adds support for deferred FB helper setup. It checks all the connectors for their connection status, and if all of them report to be disconnected marks the FB helper as needing deferred setup. Whet setup is deferred, the FB helper core will automatically retry setup after a hotplug event, and it will keep trying until it succeeds.

v2: Rebase onto my entirely reworked fbdev helper locking. One big difference is that this version again drops&reacquires the fbdev lock (which is now fb_helper->lock, but before this patch series it was mode_config->mutex), because register_framebuffer must be able to recurse back into fbdev helper code for the initial screen setup.

v3: __drm_fb_helper_initial_config must hold fb_helper->lock upon return, I've fumbled that in the deferred setup case (Liviu).

v4: I was blind, redo this all. __drm_fb_helper_initial_config shouldn't need to reacquire fb_helper->lock, that just confuses callers. I myself got confused by kernel_fb_helper_lock and somehow thought it's the same as fb_helper->lock. Tsk.

Also simplify the logic a bit (we don't need two functions to probe connectors), we can stick much closer to the existing code. And update some comments I've spotted that are outdated.

Cc: Liviu Dudau liviu@dudau.co.uk Cc: John Stultz john.stultz@linaro.org Cc: Thierry Reding treding@nvidia.com (v1) Tested-by: John Stultz john.stultz@linaro.org Signed-off-by: Thierry Reding treding@nvidia.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fb_helper.c | 100 ++++++++++++++++++++++++++-------------- include/drm/drm_fb_helper.h | 23 +++++++++ 2 files changed, 88 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index bbd4c6d0378d..d833eb2320d1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -521,6 +521,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) if (!drm_fbdev_emulation) return -ENODEV;

+ if (READ_ONCE(fb_helper->deferred_setup)) + return 0; + mutex_lock(&fb_helper->lock); ret = restore_fbdev_mode(fb_helper);

@@ -1695,8 +1698,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);

/* * Allocates the backing storage and sets up the fbdev info structure through - * the ->fb_probe callback and then registers the fbdev and sets up the panic - * notifier. + * the ->fb_probe callback. */ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int preferred_bpp) @@ -1794,13 +1796,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, }

if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) { - /* - * hmm everyone went away - assume VGA cable just fell out - * and will come back later. - */ - DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n"); - sizes.fb_width = sizes.surface_width = 1024; - sizes.fb_height = sizes.surface_height = 768; + DRM_INFO("Cannot find any crtc or sizes\n"); + return -EAGAIN; }

/* Handle our overallocation */ @@ -2429,6 +2426,57 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, kfree(enabled); }

+/* Note: Drops fb_helper->lock before returning. */ +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, + int bpp_sel) +{ + struct drm_device *dev = fb_helper->dev; + struct fb_info *info; + unsigned int width, height; + int ret; + + width = dev->mode_config.max_width; + height = dev->mode_config.max_height; + + drm_setup_crtcs(fb_helper, width, height); + ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); + if (ret < 0) { + if (ret == -EAGAIN) { + fb_helper->preferred_bpp = bpp_sel; + fb_helper->deferred_setup = true; + } + mutex_unlock(&fb_helper->lock); + + return ret; + } + + fb_helper->deferred_setup = false; + + info = fb_helper->fbdev; + info->var.pixclock = 0; + + /* Need to drop locks to avoid recursive deadlock in + * register_framebuffer. This is ok because the only thing left to do is + * register the fbdev emulation instance in kernel_fb_helper_list. */ + mutex_unlock(&fb_helper->lock); + + ret = register_framebuffer(info); + if (ret < 0) + return ret; + + dev_info(dev->dev, "fb%d: %s frame buffer device\n", + info->node, info->fix.id); + + mutex_lock(&kernel_fb_helper_lock); + if (list_empty(&kernel_fb_helper_list)) + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); + + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); + mutex_unlock(&kernel_fb_helper_lock); + + return 0; +} + /** * drm_fb_helper_initial_config - setup a sane initial connector configuration * @fb_helper: fb_helper device struct @@ -2473,39 +2521,15 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, */ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) { - struct drm_device *dev = fb_helper->dev; - struct fb_info *info; int ret;

if (!drm_fbdev_emulation) return 0;

mutex_lock(&fb_helper->lock); - drm_setup_crtcs(fb_helper, - dev->mode_config.max_width, - dev->mode_config.max_height); - ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); - mutex_unlock(&fb_helper->lock); - if (ret) - return ret; + ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);

- info = fb_helper->fbdev; - info->var.pixclock = 0; - ret = register_framebuffer(info); - if (ret < 0) - return ret; - - dev_info(dev->dev, "fb%d: %s frame buffer device\n", - info->node, info->fix.id); - - mutex_lock(&kernel_fb_helper_lock); - if (list_empty(&kernel_fb_helper_list)) - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); - - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); - mutex_unlock(&kernel_fb_helper_lock); - - return 0; + return ret; } EXPORT_SYMBOL(drm_fb_helper_initial_config);

@@ -2538,6 +2562,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) return 0;

mutex_lock(&fb_helper->lock); + if (fb_helper->deferred_setup) { + err = __drm_fb_helper_initial_config(fb_helper, + fb_helper->preferred_bpp); + return err; + } + if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) { fb_helper->delayed_hotplug = true; mutex_unlock(&fb_helper->lock); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index ea170b96e88d..a5ea6ffdfecc 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -232,6 +232,29 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug; + + /** + * @deferred_setup: + * + * If no outputs are connected (disconnected or unknown) the FB helper + * code will defer setup until at least one of the outputs shows up. + * This field keeps track of the status so that setup can be retried + * at every hotplug event until it succeeds eventually. + * + * Protected by @lock. + */ + bool deferred_setup; + + /** + * @preferred_bpp: + * + * Temporary storage for the driver's preferred BPP setting passed to + * FB helper initialization. This needs to be tracked so that deferred + * FB helper setup can pass this on. + * + * See also: @deferred_setup + */ + int preferred_bpp; };

/**

From: Thierry Reding treding@nvidia.com

The FB helper core now supports deferred setup, so the driver's custom implementation can be removed.

Cc: Xinliang Liu z.liuxinliang@hisilicon.com Cc: Rongrong Zou zourongrong@gmail.com Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Chen Feng puck.chen@hisilicon.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c index 8065d6cb1d7f..da51befa8246 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c @@ -54,14 +54,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev) { struct kirin_drm_private *priv = dev->dev_private;

- if (priv->fbdev) { - drm_fbdev_cma_hotplug_event(priv->fbdev); - } else { - priv->fbdev = drm_fbdev_cma_init(dev, 32, - dev->mode_config.num_connector); - if (IS_ERR(priv->fbdev)) - priv->fbdev = NULL; - } + drm_fbdev_cma_hotplug_event(priv->fbdev); } #endif

@@ -128,11 +121,19 @@ static int kirin_drm_kms_init(struct drm_device *dev) /* init kms poll for handling hpd */ drm_kms_helper_poll_init(dev);

- /* force detection after connectors init */ - (void)drm_helper_hpd_irq_event(dev); + priv->fbdev = drm_fbdev_cma_init(dev, 32, + dev->mode_config.num_connector); + if (IS_ERR(priv->fbdev)) { + DRM_ERROR("failed to initialize fbdev.\n"); + ret = PTR_ERR(priv->fbdev); + goto err_cleanup_poll; + }

return 0;

+err_cleanup_poll: + drm_kms_helper_poll_fini(dev); + drm_vblank_cleanup(dev); err_unbind_all: component_unbind_all(dev->dev, dev); err_dc_cleanup:

Too jarring.

Fixes: f869a6ecf254 ("drm/atomic: Add target_vblank support in atomic helpers (v2)") Cc: Andrey Grodzovsky Andrey.Grodzovsky@amd.com Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2f269e4267da..5a80b6960ae1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2964,12 +2964,11 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);

-static int page_flip_common( - struct drm_atomic_state *state, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, - uint32_t flags) +static int page_flip_common(struct drm_atomic_state *state, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_pending_vblank_event *event, + uint32_t flags) { struct drm_plane *plane = crtc->primary; struct drm_plane_state *plane_state; @@ -3063,13 +3062,12 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip); * Returns: * Returns 0 on success, negative errno numbers on failure. */ -int drm_atomic_helper_page_flip_target( - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, - uint32_t flags, - uint32_t target, - struct drm_modeset_acquire_ctx *ctx) +int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_pending_vblank_event *event, + uint32_t flags, + uint32_t target, + struct drm_modeset_acquire_ctx *ctx) { struct drm_plane *plane = crtc->primary; struct drm_atomic_state *state;