[PATCH 00/16] [RFC] drm fb helper cleanups

[PATCH 00/16] [RFC] drm fb helper cleanups

[Daniel Vetter]

Hi all,

This series is mostly just a bit of fallout from my modeset locking rework which
just landed, and some other things I've noticed while rearchitecting the modeset
infrastructure for i915.k in 3.7.

First two patches are for omapdrm, but included here since they depend upon the
locking rework in drm-next. Dunno what to do with these patches, so I'll just
drop them here. Rob, you really need to put the finishing touches on
omapdrm and move it out of staging!

The remainder just mostly cleans up the fb helper interfaces and how the helper
code calls down into drivers. Icing on the cake is a good doc update. I'm rather
happy with what the fbdev helper code now looks like after this. A few areas
with potential for improvement remain though:

- Locking around setup/teardown smells a bit fishy, but that ties in with the
  fbdev setup locking (which is insane) and the drm core setup locking (which is
  riddled with legacy stuff and also ripe for some overhaul). I think most of
  pertaining the fbdev emulation is safe though.

- Much worse is the situation around the panic, sysrq and kdbg handlers - those
  simply grab no locks at all. Safe for the ->blank callback, which may not
  sleep since it can be called while the kernel panics in interrupt context. Bug
  spotted by Konstantin Khlebnikov, I've only simplified his originally proposed
  patch a bit.

- The fb helpers have their own special interface for handling gamma/luts. I've
  looked a bit at the code, and I'm pretty sure this can be reworked to only use
  the real kms interfaces for updating the gamma table. That would leave the
  fb helpers with ->best_encoder and ->fb_probe as the two only special
  interfaces. But reworking the gamma code is a bit of work, so I've postponed
  it for now.

Comments, flames, reviews and testing highly welcome.

Cheers, Daniel

Daniel Vetter (16):
  omapdrm: only take crtc->mutex in crtc callbacks
  omapdrm: simply locking in the fb debugfs file
  drm: review locking for drm_fb_helper_restore_fbdev_mode
  drm/fb-helper: kill drm_fb_helper_restore
  drm/fb-helper: unexport drm_fb_helper_panic
  drm/fb-helper: inline drm_fb_helper_single_add_all_connectors
  drm/fb-helper: unexport drm_fb_helper_single_fb_probe
  drm/tegra: don't set up initial fbcon config twice
  drm/fb-helper: don't disable everything in initial_config
  drm/i915: rip out helper->disable noop functions
  drm/fb-helper: fixup up set_config semantics
  drm/fb-helper: directly call set_par from the hotplug handler
  drm/fb-helper: streamline drm_fb_helper_single_fb_probe
  drm/<drivers>: simplify ->fb_probe callback
  drm/fb-helper: improve kerneldoc
  drm/fb-helper: don't sleep for screen unblank when an oopps is in
    progress

 drivers/gpu/drm/ast/ast_fb.c              |   26 +---
 drivers/gpu/drm/cirrus/cirrus_fbdev.c     |   26 +---
 drivers/gpu/drm/drm_fb_cma_helper.c       |   27 +---
 drivers/gpu/drm/drm_fb_helper.c           |  216 +++++++++++++++++++++--------
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   40 +-----
 drivers/gpu/drm/gma500/framebuffer.c      |   14 +-
 drivers/gpu/drm/i915/intel_crt.c          |    1 -
 drivers/gpu/drm/i915/intel_ddi.c          |    1 -
 drivers/gpu/drm/i915/intel_display.c      |   20 +--
 drivers/gpu/drm/i915/intel_dp.c           |    1 -
 drivers/gpu/drm/i915/intel_drv.h          |    1 -
 drivers/gpu/drm/i915/intel_dvo.c          |    1 -
 drivers/gpu/drm/i915/intel_fb.c           |   23 +--
 drivers/gpu/drm/i915/intel_hdmi.c         |    1 -
 drivers/gpu/drm/i915/intel_lvds.c         |    1 -
 drivers/gpu/drm/i915/intel_sdvo.c         |    1 -
 drivers/gpu/drm/i915/intel_tv.c           |    1 -
 drivers/gpu/drm/mgag200/mgag200_fb.c      |   26 +---
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   27 +---
 drivers/gpu/drm/radeon/radeon_fb.c        |   25 +---
 drivers/gpu/drm/tegra/fb.c                |    4 -
 drivers/gpu/drm/udl/udl_fb.c              |   26 +---
 drivers/staging/omapdrm/omap_crtc.c       |   12 +-
 drivers/staging/omapdrm/omap_debugfs.c    |   14 --
 drivers/staging/omapdrm/omap_fbdev.c      |   21 +--
 include/drm/drm_fb_helper.h               |    5 -
 26 files changed, 227 insertions(+), 334 deletions(-)

-- 
1.7.10.4

[PATCH 01/16] omapdrm: only take crtc->mutex in crtc callbacks

[Daniel Vetter]

Omapdrm doesn't do anything nefarious with crtc load detection or has
any shared resources, so this is enough. We also need to adjust the
WARN_ON.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/staging/omapdrm/omap_crtc.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
index 510942e..fb5d722 100644
--- a/drivers/staging/omapdrm/omap_crtc.c
+++ b/drivers/staging/omapdrm/omap_crtc.c
@@ -274,17 +274,16 @@ static void page_flip_worker(struct work_struct *work)
 	struct omap_crtc *omap_crtc =
 			container_of(work, struct omap_crtc, page_flip_work);
 	struct drm_crtc *crtc = &omap_crtc->base;
-	struct drm_device *dev = crtc->dev;
 	struct drm_display_mode *mode = &crtc->mode;
 	struct drm_gem_object *bo;
 
-	drm_modeset_lock_all(dev);
+	mutex_lock(&crtc->mutex);
 	omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb,
 			0, 0, mode->hdisplay, mode->vdisplay,
 			crtc->x << 16, crtc->y << 16,
 			mode->hdisplay << 16, mode->vdisplay << 16,
 			vblank_cb, crtc);
-	drm_modeset_unlock_all(dev);
+	mutex_unlock(&crtc->mutex);
 
 	bo = omap_framebuffer_bo(crtc->fb, 0);
 	drm_gem_object_unreference_unlocked(bo);
@@ -417,7 +416,7 @@ static void apply_worker(struct work_struct *work)
 	 * the callbacks and list modification all serialized
 	 * with respect to modesetting ioctls from userspace.
 	 */
-	drm_modeset_lock_all(dev);
+	mutex_lock(&crtc->mutex);
 	dispc_runtime_get();
 
 	/*
@@ -462,16 +461,15 @@ static void apply_worker(struct work_struct *work)
 
 out:
 	dispc_runtime_put();
-	drm_modeset_unlock_all(dev);
+	mutex_unlock(&crtc->mutex);
 }
 
 int omap_crtc_apply(struct drm_crtc *crtc,
 		struct omap_drm_apply *apply)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
-	struct drm_device *dev = crtc->dev;
 
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	WARN_ON(!mutex_is_locked(&crtc->mutex));
 
 	/* no need to queue it again if it is already queued: */
 	if (apply->queued)
-- 
1.7.10.4

[PATCH 02/16] omapdrm: simply locking in the fb debugfs file

[Daniel Vetter]

We don't need to hold onto mode_config.mutex any more to keep the fb
objects around. And locking dev->struct_mutex is also not required,
since omap_gem_describe only reads data anyway. And for a debug
interface it's better to grab fewer locks in case the driver is
deadlocked already ...

The only thing we need is to hold onto mode_config.fb_lock to ensure
the user-created fbs don't disappear. The fbcon fb doesn't need any
protection, since it lives as long as the driver (and so the debugfs
files) itself. And if the teardown/setup isn't following the right
sequence grabbing locks won't prevent a NULL deref on priv->fbdev if
the fb is not yet (or no longer) there.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/staging/omapdrm/omap_debugfs.c |   14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/staging/omapdrm/omap_debugfs.c b/drivers/staging/omapdrm/omap_debugfs.c
index e95540b..2bf0664 100644
--- a/drivers/staging/omapdrm/omap_debugfs.c
+++ b/drivers/staging/omapdrm/omap_debugfs.c
@@ -57,17 +57,6 @@ static int fb_show(struct seq_file *m, void *arg)
 	struct drm_device *dev = node->minor->dev;
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_framebuffer *fb;
-	int ret;
-
-	ret = mutex_lock_interruptible(&dev->mode_config.mutex);
-	if (ret)
-		return ret;
-
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret) {
-		mutex_unlock(&dev->mode_config.mutex);
-		return ret;
-	}
 
 	seq_printf(m, "fbcon ");
 	omap_framebuffer_describe(priv->fbdev->fb, m);
@@ -82,9 +71,6 @@ static int fb_show(struct seq_file *m, void *arg)
 	}
 	mutex_unlock(&dev->mode_config.fb_lock);
 
-	mutex_unlock(&dev->struct_mutex);
-	mutex_unlock(&dev->mode_config.mutex);
-
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 03/16] drm: review locking for drm_fb_helper_restore_fbdev_mode

[Daniel Vetter]

... it's required. Fix up exynos and the cma helper, and add a
corresponding WARN_ON to drm_fb_helper_restore_fbdev_mode.

Note that tegra calls the fbdev cma helper restore function also from
it's driver-load callback. Which is a bit against current practice,
since usually the call is only from ->lastclose, and initial setup is
done by drm_fb_helper_initial_config.

Also add the relevant drm DocBook entry.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_cma_helper.c       |    2 ++
 drivers/gpu/drm/drm_fb_helper.c           |    8 ++++++++
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    2 ++
 3 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 3742bc9..1b6ba2d 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -389,8 +389,10 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
  */
 void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
 {
+	drm_modeset_lock_all(dev);
 	if (fbdev_cma)
 		drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
+	drm_modeset_unlock_all(dev);
 }
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0c6e25e..0439cb0 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -239,6 +239,14 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
+/**
+ * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
+ * @fb_helper: fbcon to restore
+ *
+ * This should be called from driver's drm->lastclose callback when implementing
+ * an fbcon on top of kms using this helper. This ensures that the user isn't
+ * greeted with a black screen when e.g. X dies.
+ */
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
 	bool error = false;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 90d335c..086d0f7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -376,5 +376,7 @@ void exynos_drm_fbdev_restore_mode(struct drm_device *dev)
 	if (!private || !private->fb_helper)
 		return;
 
+	drm_modeset_lock_all(dev);
 	drm_fb_helper_restore_fbdev_mode(private->fb_helper);
+	drm_modeset_unlock_all(dev);
 }
-- 
1.7.10.4

[PATCH 04/16] drm/fb-helper: kill drm_fb_helper_restore

[Daniel Vetter]

It's only used internally for the sysrq and panic handlers provided by
the drm fb helper implementation. Hence just inline it, kill the
export and remove the confusing kerneldoc. Driver's are supposed to
call drm_fb_helper_restore_fbdev_mode on lastclose.

Note that locking is totally fubar - the sysrq case doesn't take any
locks at all. The panic handler probably shouldn't take any locks
since it'll only make things worse. Otoh it's probably better to
switch things over to the atomic modeset callbacks (and disable the
panic handler for those drivers which don't implement it).

But that's both better done in separate patches.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |   23 ++++++++---------------
 include/drm/drm_fb_helper.h     |    1 -
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0439cb0..6d689f2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -261,6 +261,10 @@ bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode);
 
+/*
+ * restore fbcon display for all kms driver's using this helper, used for sysrq
+ * and panic handling.
+ */
 static bool drm_fb_helper_force_kernel_mode(void)
 {
 	bool ret, error = false;
@@ -299,20 +303,6 @@ static struct notifier_block paniced = {
 	.notifier_call = drm_fb_helper_panic,
 };
 
-/**
- * drm_fb_helper_restore - restore the framebuffer console (kernel) config
- *
- * Restore's the kernel's fbcon mode, used for lastclose & panic paths.
- */
-void drm_fb_helper_restore(void)
-{
-	bool ret;
-	ret = drm_fb_helper_force_kernel_mode();
-	if (ret == true)
-		DRM_ERROR("Failed to restore crtc configuration\n");
-}
-EXPORT_SYMBOL(drm_fb_helper_restore);
-
 static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -334,7 +324,10 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 #ifdef CONFIG_MAGIC_SYSRQ
 static void drm_fb_helper_restore_work_fn(struct work_struct *ignored)
 {
-	drm_fb_helper_restore();
+	bool ret;
+	ret = drm_fb_helper_force_kernel_mode();
+	if (ret == true)
+		DRM_ERROR("Failed to restore crtc configuration\n");
 }
 static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5120b01..ba32505 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -103,7 +103,6 @@ int drm_fb_helper_setcolreg(unsigned regno,
 			    struct fb_info *info);
 
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper);
-void drm_fb_helper_restore(void);
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height);
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-- 
1.7.10.4

[PATCH 05/16] drm/fb-helper: unexport drm_fb_helper_panic

[Daniel Vetter]

It doesn't even show up in any header files and only used iternally.
Originally it was (ab)used to restore the fbcon on lastclose, but that
died with

commit e8e7a2b8ccfdae0d4cb6bd25824bbedcd42da316
Author: Dave Airlie <airlied@redhat.com>
Date:   Thu Apr 21 22:18:32 2011 +0100

    drm/i915: restore only the mode of this driver on lastclose (v2)

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 6d689f2..ce816a5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -284,7 +284,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
 	return error;
 }
 
-int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
+static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
 			void *panic_str)
 {
 	/*
@@ -297,7 +297,6 @@ int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
 	pr_err("panic occurred, switching back to text console\n");
 	return drm_fb_helper_force_kernel_mode();
 }
-EXPORT_SYMBOL(drm_fb_helper_panic);
 
 static struct notifier_block paniced = {
 	.notifier_call = drm_fb_helper_panic,
-- 
1.7.10.4

[PATCH 06/16] drm/fb-helper: inline drm_fb_helper_single_add_all_connectors

[Daniel Vetter]

All drivers call this right after drm_fb_helper_init, and the only
thing this function does is allocate a bit of memory and set up a
bunch of pointers. No reason at all the keep this as a separate step.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_fb.c              |    1 -
 drivers/gpu/drm/cirrus/cirrus_fbdev.c     |    1 -
 drivers/gpu/drm/drm_fb_cma_helper.c       |    7 -------
 drivers/gpu/drm/drm_fb_helper.c           |    9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    7 -------
 drivers/gpu/drm/gma500/framebuffer.c      |    1 -
 drivers/gpu/drm/i915/intel_fb.c           |    2 --
 drivers/gpu/drm/mgag200/mgag200_fb.c      |    1 -
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    2 --
 drivers/gpu/drm/radeon/radeon_fb.c        |    1 -
 drivers/gpu/drm/udl/udl_fb.c              |    1 -
 drivers/staging/omapdrm/omap_fbdev.c      |    1 -
 include/drm/drm_fb_helper.h               |    1 -
 13 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 3e6584b..4330784 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -313,7 +313,6 @@ int ast_fbdev_init(struct drm_device *dev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&afbdev->helper);
 	drm_fb_helper_initial_config(&afbdev->helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index 3daea0f..d9312ee 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -290,7 +290,6 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
 		kfree(gfbdev);
 		return ret;
 	}
-	drm_fb_helper_single_add_all_connectors(&gfbdev->helper);
 	drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1b6ba2d..1ba09ba 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -326,13 +326,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 		goto err_free;
 	}
 
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret < 0) {
-		dev_err(dev->dev, "Failed to add connectors.\n");
-		goto err_drm_fb_helper_fini;
-
-	}
-
 	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
 	if (ret < 0) {
 		dev_err(dev->dev, "Failed to set inital hw configuration.\n");
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ce816a5..4549512 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -55,7 +55,7 @@ static LIST_HEAD(kernel_fb_helper_list);
  */
 
 /* simple single crtc case helper function */
-int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
+static int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_connector *connector;
@@ -80,7 +80,6 @@ fail:
 	fb_helper->connector_count = 0;
 	return -ENOMEM;
 }
-EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
 static int drm_fb_helper_parse_command_line(struct drm_fb_helper *fb_helper)
 {
@@ -426,7 +425,7 @@ int drm_fb_helper_init(struct drm_device *dev,
 		       int crtc_count, int max_conn_count)
 {
 	struct drm_crtc *crtc;
-	int i;
+	int i, ret;
 
 	fb_helper->dev = dev;
 
@@ -461,6 +460,10 @@ int drm_fb_helper_init(struct drm_device *dev,
 		i++;
 	}
 
+	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
+	if (ret)
+		goto out_free;
+
 	return 0;
 out_free:
 	drm_fb_helper_crtc_free(fb_helper);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 086d0f7..79f8903 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -288,13 +288,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 		goto err_init;
 	}
 
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret < 0) {
-		DRM_ERROR("failed to register drm_fb_helper_connector.\n");
-		goto err_setup;
-
-	}
-
 	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
 	if (ret < 0) {
 		DRM_ERROR("failed to set up hw configuration.\n");
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index c1ef37e..96ebf4f 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -615,7 +615,6 @@ int psb_fbdev_init(struct drm_device *dev)
 	drm_fb_helper_init(dev, &fbdev->psb_fb_helper, dev_priv->ops->crtcs,
 							INTELFB_CONN_LIMIT);
 
-	drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper);
 	drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 33c4f1b..302bc63 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -249,8 +249,6 @@ int intel_fbdev_init(struct drm_device *dev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 5c69b43..fd5cf18 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -277,7 +277,6 @@ int mgag200_fbdev_init(struct mga_device *mdev)
 		kfree(mfbdev);
 		return ret;
 	}
-	drm_fb_helper_single_add_all_connectors(&mfbdev->helper);
 	drm_fb_helper_initial_config(&mfbdev->helper, 32);
 
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index d4ecb4d..74916a9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -481,8 +481,6 @@ nouveau_fbcon_init(struct drm_device *dev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&fbcon->helper);
-
 	if (pfb->ram.size <= 32 * 1024 * 1024)
 		preferred_bpp = 8;
 	else
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index 515e5ee..a1ceee1 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -378,7 +378,6 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&rfbdev->helper);
 	drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index caa84f1..25f9bb1 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -582,7 +582,6 @@ int udl_fbdev_init(struct drm_device *dev)
 
 	}
 
-	drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
 	drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 2728e37..ee1900a 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -368,7 +368,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	drm_fb_helper_single_add_all_connectors(helper);
 	drm_fb_helper_initial_config(helper, 32);
 
 	priv->fbdev = helper;
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index ba32505..4e989dc 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -112,7 +112,6 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
 
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
 bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
-int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
 int drm_fb_helper_debug_enter(struct fb_info *info);
 int drm_fb_helper_debug_leave(struct fb_info *info);
 
-- 
1.7.10.4

[PATCH 07/16] drm/fb-helper: unexport drm_fb_helper_single_fb_probe

[Daniel Vetter]

Not called by anyone, and really, shouldn't be. Drivers are supposed
either drm_fb_helper_initial_config or drm_fb_helper_hotplug_event.
Originally this was done differently, but is now consolidated in the
helper functions and no longer done by drivers directly.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |    5 ++---
 include/drm/drm_fb_helper.h     |    3 ---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4549512..2377fef 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -754,8 +754,8 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
-int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
-				  int preferred_bpp)
+static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
+					 int preferred_bpp)
 {
 	int new_fb = 0;
 	int crtc_count = 0;
@@ -870,7 +870,6 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
 
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 4e989dc..62f8848 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -82,9 +82,6 @@ struct drm_fb_helper {
 	bool delayed_hotplug;
 };
 
-int drm_fb_helper_single_fb_probe(struct drm_fb_helper *helper,
-				  int preferred_bpp);
-
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *helper, int crtc_count,
 		       int max_conn);
-- 
1.7.10.4

[PATCH 08/16] drm/tegra: don't set up initial fbcon config twice

[Daniel Vetter]

drm_fbdev_cma_init does the inital fbcon setup by calling down into
drm_fb_helper_initial_config, so no need at all to restore the just
set up configuration right away ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/tegra/fb.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index 97993c6..0391495 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -39,10 +39,6 @@ int tegra_drm_fb_init(struct drm_device *drm)
 	if (IS_ERR(fbdev))
 		return PTR_ERR(fbdev);
 
-#ifndef CONFIG_FRAMEBUFFER_CONSOLE
-	drm_fbdev_cma_restore_mode(fbdev);
-#endif
-
 	host1x->fbdev = fbdev;
 
 	return 0;
-- 
1.7.10.4

[PATCH 09/16] drm/fb-helper: don't disable everything in initial_config

[Daniel Vetter]

This should be done in the drivers for two reasons:
- it gets in the way of fastboot efforts
- it links the fb helpers with the crtc helpers instead of going
  through the real interface vfuncs, forcing i915 to fake all the
  ->disable callbacks used by the crtc helper to avoid ugly Oopsen

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_fb.c              |    4 ++++
 drivers/gpu/drm/cirrus/cirrus_fbdev.c     |    3 +++
 drivers/gpu/drm/drm_fb_cma_helper.c       |    3 +++
 drivers/gpu/drm/drm_fb_helper.c           |    3 ---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    3 +++
 drivers/gpu/drm/gma500/framebuffer.c      |    3 +++
 drivers/gpu/drm/i915/intel_fb.c           |    3 +++
 drivers/gpu/drm/mgag200/mgag200_fb.c      |    3 +++
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    3 +++
 drivers/gpu/drm/radeon/radeon_fb.c        |    3 +++
 drivers/gpu/drm/udl/udl_fb.c              |    3 +++
 drivers/staging/omapdrm/omap_fbdev.c      |    3 +++
 12 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 4330784..9bfd5ab 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -40,6 +40,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 #include "ast_drv.h"
 
 static void ast_dirty_update(struct ast_fbdev *afbdev,
@@ -313,6 +314,9 @@ int ast_fbdev_init(struct drm_device *dev)
 		return ret;
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&afbdev->helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index d9312ee..b869b8b 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 
 #include <linux/fb.h>
 
@@ -290,6 +291,8 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
 		kfree(gfbdev);
 		return ret;
 	}
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(cdev->dev);
 	drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1ba09ba..fcf9fa3 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -326,6 +326,9 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 		goto err_free;
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
 	if (ret < 0) {
 		dev_err(dev->dev, "Failed to set inital hw configuration.\n");
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 2377fef..dbf0020 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1360,9 +1360,6 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	struct drm_device *dev = fb_helper->dev;
 	int count = 0;
 
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(fb_helper->dev);
-
 	drm_fb_helper_parse_command_line(fb_helper);
 
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 79f8903..77d78cd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -288,6 +288,9 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 		goto err_init;
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
 	if (ret < 0) {
 		DRM_ERROR("failed to set up hw configuration.\n");
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 96ebf4f..7288b6d 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -615,6 +615,9 @@ int psb_fbdev_init(struct drm_device *dev)
 	drm_fb_helper_init(dev, &fbdev->psb_fb_helper, dev_priv->ops->crtcs,
 							INTELFB_CONN_LIMIT);
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 302bc63..7a8f7cd 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -256,6 +256,9 @@ void intel_fbdev_initial_config(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
 	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
 }
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index fd5cf18..674f8fb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 
 #include <linux/fb.h>
 
@@ -277,6 +278,8 @@ int mgag200_fbdev_init(struct mga_device *mdev)
 		kfree(mfbdev);
 		return ret;
 	}
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(mdev->dev);
 	drm_fb_helper_initial_config(&mfbdev->helper, 32);
 
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 74916a9..7b2d231 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -489,6 +489,9 @@ nouveau_fbcon_init(struct drm_device *dev)
 	else
 		preferred_bpp = 32;
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&fbcon->helper, preferred_bpp);
 	return 0;
 }
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index a1ceee1..a44b386 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -378,6 +378,9 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 		return ret;
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(rdev->ddev);
+
 	drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 25f9bb1..7d40cb7 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -582,6 +582,9 @@ int udl_fbdev_init(struct drm_device *dev)
 
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index ee1900a..6ccaf54 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -368,6 +368,9 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
 		goto fail;
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(helper, 32);
 
 	priv->fbdev = helper;
-- 
1.7.10.4

[PATCH 10/16] drm/i915: rip out helper->disable noop functions

[Daniel Vetter]

Now that the driver is in control of whether it needs to disable
everything at take-over or not, we can rip this all out.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c     |    1 -
 drivers/gpu/drm/i915/intel_ddi.c     |    1 -
 drivers/gpu/drm/i915/intel_display.c |    9 ---------
 drivers/gpu/drm/i915/intel_dp.c      |    1 -
 drivers/gpu/drm/i915/intel_drv.h     |    1 -
 drivers/gpu/drm/i915/intel_dvo.c     |    1 -
 drivers/gpu/drm/i915/intel_fb.c      |    3 ---
 drivers/gpu/drm/i915/intel_hdmi.c    |    1 -
 drivers/gpu/drm/i915/intel_lvds.c    |    1 -
 drivers/gpu/drm/i915/intel_sdvo.c    |    1 -
 drivers/gpu/drm/i915/intel_tv.c      |    1 -
 11 files changed, 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 71a5eba..15964c8 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -684,7 +684,6 @@ static void intel_crt_reset(struct drm_connector *connector)
 static const struct drm_encoder_helper_funcs crt_encoder_funcs = {
 	.mode_fixup = intel_crt_mode_fixup,
 	.mode_set = intel_crt_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2e904a5..f3e4d3b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1454,7 +1454,6 @@ static const struct drm_encoder_funcs intel_ddi_funcs = {
 static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = {
 	.mode_fixup = intel_ddi_mode_fixup,
 	.mode_set = intel_ddi_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 void intel_ddi_init(struct drm_device *dev, enum port port)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26df9e3..055b24a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3712,10 +3712,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 	intel_crtc_update_sarea(crtc, enable);
 }
 
-static void intel_crtc_noop(struct drm_crtc *crtc)
-{
-}
-
 static void intel_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3762,10 +3758,6 @@ void intel_modeset_disable(struct drm_device *dev)
 	}
 }
 
-void intel_encoder_noop(struct drm_encoder *encoder)
-{
-}
-
 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
@@ -7239,7 +7231,6 @@ free_work:
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
-	.disable = intel_crtc_noop,
 };
 
 bool intel_encoder_check_is_cloned(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f05364a..63c6aeb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2556,7 +2556,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = {
 	.mode_fixup = intel_dp_mode_fixup,
 	.mode_set = intel_dp_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aeff0d1..559e84a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -521,7 +521,6 @@ extern void intel_modeset_disable(struct drm_device *dev);
 extern void intel_crtc_restore_mode(struct drm_crtc *crtc);
 extern void intel_crtc_load_lut(struct drm_crtc *crtc);
 extern void intel_crtc_update_dpms(struct drm_crtc *crtc);
-extern void intel_encoder_noop(struct drm_encoder *encoder);
 extern void intel_encoder_destroy(struct drm_encoder *encoder);
 extern void intel_encoder_dpms(struct intel_encoder *encoder, int mode);
 extern bool intel_encoder_check_is_cloned(struct intel_encoder *encoder);
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 15da995..00e70db 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -345,7 +345,6 @@ static void intel_dvo_destroy(struct drm_connector *connector)
 static const struct drm_encoder_helper_funcs intel_dvo_helper_funcs = {
 	.mode_fixup = intel_dvo_mode_fixup,
 	.mode_set = intel_dvo_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_dvo_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 7a8f7cd..302bc63 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -256,9 +256,6 @@ void intel_fbdev_initial_config(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(dev);
-
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
 	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
 }
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index d53b731..7918a2b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -969,7 +969,6 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 static const struct drm_encoder_helper_funcs intel_hdmi_helper_funcs = {
 	.mode_fixup = intel_hdmi_mode_fixup,
 	.mode_set = intel_hdmi_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 5e3f08e..feb43fd 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -656,7 +656,6 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
 	.mode_fixup = intel_lvds_mode_fixup,
 	.mode_set = intel_lvds_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index f01063a..33b46d9 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2043,7 +2043,6 @@ done:
 static const struct drm_encoder_helper_funcs intel_sdvo_helper_funcs = {
 	.mode_fixup = intel_sdvo_mode_fixup,
 	.mode_set = intel_sdvo_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 984a113..d808421 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1487,7 +1487,6 @@ out:
 static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
 	.mode_fixup = intel_tv_mode_fixup,
 	.mode_set = intel_tv_mode_set,
-	.disable = intel_encoder_noop,
 };
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
-- 
1.7.10.4

[PATCH 11/16] drm/fb-helper: fixup up set_config semantics

[Daniel Vetter]

While doing the modeset rework for drm/i915 I've noticed that the fb
helper is very liberal with the semantics of the ->set_config
interface:
- It doesn't bother clearing stale modes (e.g. when unpluggint a
  screen).
- It unconditionally sets and the fb, even if no mode will be set on a
  given crtc.
- The initial setup is a bit fun since we need to pick crtcs to decide
  the desired fb size, but also should set the modeset->fb pointer.
  Explain what's going on in the fixup code after the fb is allocated.

The crtc helper didn't really care, but the new i915 modeset
infrastructure did, so I've had to add a bunch of special-cases to
catch this.

Fix this all up and enforce the interface by converting the checks in
drm/i915/intel_display.c to BUG_ONs.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c      |   27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |   11 +++--------
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index dbf0020..5c73a12 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct fb_var_screeninfo *var = &info->var;
-	struct drm_crtc *crtc;
 	int ret;
 	int i;
 
@@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 
 	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 		ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
 		if (ret) {
 			drm_modeset_unlock_all(dev);
@@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 	info = fb_helper->fbdev;
 
-	/* set the fb pointer */
+	/*
+	 * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
+	 * events, but at init time drm_setup_crtcs needs to be called before
+	 * the fb is allocated (since we need to figure out the desired size of
+	 * the fb before we can allocate it ...). Hence we need to fix things up
+	 * here again.
+	 */
 	for (i = 0; i < fb_helper->crtc_count; i++)
-		fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+		if (fb_helper->crtc_info[i].mode_set.num_connectors)
+			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+
 
 	if (new_fb) {
 		info->var.pixclock = 0;
@@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		modeset = &fb_helper->crtc_info[i].mode_set;
 		modeset->num_connectors = 0;
+		modeset->fb = NULL;
 	}
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
 			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			modeset->fb = fb_helper->fb;
 		}
 	}
 
+	/* Clear out any old modes if there are no more connected outputs. */
+	for (i = 0; i < fb_helper->crtc_count; i++) {
+		modeset = &fb_helper->crtc_info[i].mode_set;
+		if (modeset->num_connectors == 0) {
+			BUG_ON(modeset->fb);
+			BUG_ON(modeset->num_connectors);
+			if (modeset->mode)
+				drm_mode_destroy(dev, modeset->mode);
+			modeset->mode = NULL;
+		}
+	}
 out:
 	kfree(crtcs);
 	kfree(modes);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 055b24a..7e3dd0f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7940,14 +7940,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	BUG_ON(!set->crtc);
 	BUG_ON(!set->crtc->helper_private);
 
-	if (!set->mode)
-		set->fb = NULL;
-
-	/* The fb helper likes to play gross jokes with ->mode_set_config.
-	 * Unfortunately the crtc helper doesn't do much at all for this case,
-	 * so we have to cope with this madness until the fb helper is fixed up. */
-	if (set->fb && set->num_connectors == 0)
-		return 0;
+	/* Enforce sane interface api - has been abused by the fb helper. */
+	BUG_ON(!set->mode && set->fb);
+	BUG_ON(set->fb && set->num_connectors == 0);
 
 	if (set->fb) {
 		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-- 
1.7.10.4

[PATCH 12/16] drm/fb-helper: directly call set_par from the hotplug handler

[Daniel Vetter]

The idea behind calling down into the driver's ->fb_probe function on each
hotplug seems to be able to reallocate the backing storage (if e.g. a screen
with higher resolution gets added). But that requires quite a bit of work in the
fb helper itself, since currently we limit new screens to the currently
allocated fb. An no kms driver supports fbdev fb resizing.

So don't bother and start to simplify the code by calling drm_fb_helper_set_par
directly from the fbdev hotplug function, since that's the only thing left in
drm_fb_helper_single_fb_probe which does not concern itself with fb allocation
and initial setup. Follow-on patches will streamline the initial setup
code.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5c73a12..90c1117 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -859,8 +859,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 				info->node, info->fix.id);
 
-	} else {
-		drm_fb_helper_set_par(info);
 	}
 
 	/* Switch back to kernel console on panic */
@@ -1436,7 +1434,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	drm_setup_crtcs(fb_helper);
 	drm_modeset_unlock_all(dev);
 
-	return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	drm_fb_helper_set_par(fb_helper->fbdev);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
-- 
1.7.10.4

[PATCH 13/16] drm/fb-helper: streamline drm_fb_helper_single_fb_probe

[Daniel Vetter]

No need to check whether we've allocated a new fb since we're not
always doing that. Also, we always need to register the fbdev and add
it to the panic notifier.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 90c1117..edbfcde 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -752,10 +752,14 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+/*
+ * Allocates the backing storage through the ->fb_probe callback and then
+ * registers the fbdev and sets up the panic notifier.
+ */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
 {
-	int new_fb = 0;
+	int ret = 0;
 	int crtc_count = 0;
 	int i;
 	struct fb_info *info;
@@ -833,9 +837,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	/* push down into drivers */
-	new_fb = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
-	if (new_fb < 0)
-		return new_fb;
+	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
+	if (ret < 0)
+		return ret;
 
 	info = fb_helper->fbdev;
 
@@ -851,15 +855,12 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
 
 
-	if (new_fb) {
-		info->var.pixclock = 0;
-		if (register_framebuffer(info) < 0)
-			return -EINVAL;
-
-		dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
-				info->node, info->fix.id);
+	info->var.pixclock = 0;
+	if (register_framebuffer(info) < 0)
+		return -EINVAL;
 
-	}
+	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
+			info->node, info->fix.id);
 
 	/* Switch back to kernel console on panic */
 	/* multi card linked list maybe */
@@ -869,8 +870,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
-	if (new_fb)
-		list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+
+	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH 14/16] drm/<drivers>: simplify ->fb_probe callback

[Daniel Vetter]

The fb helper lost its support for reallocating an fb completely, so
no need to return special success values any more.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_fb.c              |   21 +++-----------------
 drivers/gpu/drm/cirrus/cirrus_fbdev.c     |   22 +++------------------
 drivers/gpu/drm/drm_fb_cma_helper.c       |   17 +---------------
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   30 +----------------------------
 drivers/gpu/drm/gma500/framebuffer.c      |   10 +---------
 drivers/gpu/drm/i915/intel_fb.c           |   21 +++-----------------
 drivers/gpu/drm/mgag200/mgag200_fb.c      |   22 +++------------------
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   22 +++------------------
 drivers/gpu/drm/radeon/radeon_fb.c        |   21 +++-----------------
 drivers/gpu/drm/udl/udl_fb.c              |   22 +++------------------
 drivers/staging/omapdrm/omap_fbdev.c      |   17 +---------------
 11 files changed, 25 insertions(+), 200 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 9bfd5ab..3a5a5e0 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -146,9 +146,10 @@ static int astfb_create_object(struct ast_fbdev *afbdev,
 	return ret;
 }
 
-static int astfb_create(struct ast_fbdev *afbdev,
+static int astfb_create(struct drm_fb_helper *helper,
 			struct drm_fb_helper_surface_size *sizes)
 {
+	struct ast_fbdev *afbdev = (struct ast_fbdev *)helper;
 	struct drm_device *dev = afbdev->helper.dev;
 	struct drm_mode_fb_cmd2 mode_cmd;
 	struct drm_framebuffer *fb;
@@ -249,26 +250,10 @@ static void ast_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
 	*blue = ast_crtc->lut_b[regno] << 8;
 }
 
-static int ast_find_or_create_single(struct drm_fb_helper *helper,
-					  struct drm_fb_helper_surface_size *sizes)
-{
-	struct ast_fbdev *afbdev = (struct ast_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = astfb_create(afbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static struct drm_fb_helper_funcs ast_fb_helper_funcs = {
 	.gamma_set = ast_fb_gamma_set,
 	.gamma_get = ast_fb_gamma_get,
-	.fb_probe = ast_find_or_create_single,
+	.fb_probe = astfb_create,
 };
 
 static void ast_fbdev_destroy(struct drm_device *dev,
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index b869b8b..778a90b 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -121,9 +121,10 @@ static int cirrusfb_create_object(struct cirrus_fbdev *afbdev,
 	return ret;
 }
 
-static int cirrusfb_create(struct cirrus_fbdev *gfbdev,
+static int cirrusfb_create(struct drm_fb_helper *helper,
 			   struct drm_fb_helper_surface_size *sizes)
 {
+	struct cirrus_fbdev *gfbdev = (struct cirrus_fbdev *)helper;
 	struct drm_device *dev = gfbdev->helper.dev;
 	struct cirrus_device *cdev = gfbdev->helper.dev->dev_private;
 	struct fb_info *info;
@@ -220,23 +221,6 @@ out_iounmap:
 	return ret;
 }
 
-static int cirrus_fb_find_or_create_single(struct drm_fb_helper *helper,
-					   struct drm_fb_helper_surface_size
-					   *sizes)
-{
-	struct cirrus_fbdev *gfbdev = (struct cirrus_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = cirrusfb_create(gfbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static int cirrus_fbdev_destroy(struct drm_device *dev,
 				struct cirrus_fbdev *gfbdev)
 {
@@ -268,7 +252,7 @@ static int cirrus_fbdev_destroy(struct drm_device *dev,
 static struct drm_fb_helper_funcs cirrus_fb_helper_funcs = {
 	.gamma_set = cirrus_crtc_fb_gamma_set,
 	.gamma_get = cirrus_crtc_fb_gamma_get,
-	.fb_probe = cirrus_fb_find_or_create_single,
+	.fb_probe = cirrusfb_create,
 };
 
 int cirrus_fbdev_init(struct cirrus_device *cdev)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index fcf9fa3..69814d2 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -275,23 +275,8 @@ err_drm_gem_cma_free_object:
 	return ret;
 }
 
-static int drm_fbdev_cma_probe(struct drm_fb_helper *helper,
-	struct drm_fb_helper_surface_size *sizes)
-{
-	int ret = 0;
-
-	if (!helper->fb) {
-		ret = drm_fbdev_cma_create(helper, sizes);
-		if (ret < 0)
-			return ret;
-		ret = 1;
-	}
-
-	return ret;
-}
-
 static struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
-	.fb_probe = drm_fbdev_cma_probe,
+	.fb_probe = drm_fbdev_cma_create,
 };
 
 /**
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 77d78cd..ca70b91 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -226,36 +226,8 @@ out:
 	return ret;
 }
 
-static int exynos_drm_fbdev_probe(struct drm_fb_helper *helper,
-				   struct drm_fb_helper_surface_size *sizes)
-{
-	int ret = 0;
-
-	DRM_DEBUG_KMS("%s\n", __FILE__);
-
-	/*
-	 * with !helper->fb, it means that this funcion is called first time
-	 * and after that, the helper->fb would be used as clone mode.
-	 */
-	if (!helper->fb) {
-		ret = exynos_drm_fbdev_create(helper, sizes);
-		if (ret < 0) {
-			DRM_ERROR("failed to create fbdev.\n");
-			return ret;
-		}
-
-		/*
-		 * fb_helper expects a value more than 1 if succeed
-		 * because register_framebuffer() should be called.
-		 */
-		ret = 1;
-	}
-
-	return ret;
-}
-
 static struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
-	.fb_probe =	exynos_drm_fbdev_probe,
+	.fb_probe =	exynos_drm_fbdev_create,
 };
 
 int exynos_drm_fbdev_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 7288b6d..f7a458c 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -545,9 +545,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
 	struct psb_fbdev *psb_fbdev = (struct psb_fbdev *)helper;
 	struct drm_device *dev = psb_fbdev->psb_fb_helper.dev;
 	struct drm_psb_private *dev_priv = dev->dev_private;
-	int new_fb = 0;
 	int bytespp;
-	int ret;
 
 	bytespp = sizes->surface_bpp / 8;
 	if (bytespp == 3)	/* no 24bit packed */
@@ -562,13 +560,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
                 sizes->surface_depth = 16;
         }
 
-	if (!helper->fb) {
-		ret = psbfb_create(psb_fbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
+	return psbfb_create(psb_fbdev, sizes);
 }
 
 static struct drm_fb_helper_funcs psb_fb_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 302bc63..3a0d16e 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -57,9 +57,10 @@ static struct fb_ops intelfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
-static int intelfb_create(struct intel_fbdev *ifbdev,
+static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
+	struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
 	struct drm_device *dev = ifbdev->helper.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct fb_info *info;
@@ -181,26 +182,10 @@ out:
 	return ret;
 }
 
-static int intel_fb_find_or_create_single(struct drm_fb_helper *helper,
-					  struct drm_fb_helper_surface_size *sizes)
-{
-	struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = intelfb_create(ifbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.gamma_set = intel_crtc_fb_gamma_set,
 	.gamma_get = intel_crtc_fb_gamma_get,
-	.fb_probe = intel_fb_find_or_create_single,
+	.fb_probe = intelfb_create,
 };
 
 static void intel_fbdev_destroy(struct drm_device *dev,
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 674f8fb..247bbcb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -121,9 +121,10 @@ static int mgag200fb_create_object(struct mga_fbdev *afbdev,
 	return ret;
 }
 
-static int mgag200fb_create(struct mga_fbdev *mfbdev,
+static int mgag200fb_create(struct drm_fb_helper *helper,
 			   struct drm_fb_helper_surface_size *sizes)
 {
+	struct mga_fbdev *mfbdev = (struct mga_fbdev *)helper;
 	struct drm_device *dev = mfbdev->helper.dev;
 	struct drm_mode_fb_cmd2 mode_cmd;
 	struct mga_device *mdev = dev->dev_private;
@@ -210,23 +211,6 @@ out:
 	return ret;
 }
 
-static int mga_fb_find_or_create_single(struct drm_fb_helper *helper,
-					   struct drm_fb_helper_surface_size
-					   *sizes)
-{
-	struct mga_fbdev *mfbdev = (struct mga_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = mgag200fb_create(mfbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static int mga_fbdev_destroy(struct drm_device *dev,
 				struct mga_fbdev *mfbdev)
 {
@@ -257,7 +241,7 @@ static int mga_fbdev_destroy(struct drm_device *dev,
 static struct drm_fb_helper_funcs mga_fb_helper_funcs = {
 	.gamma_set = mga_crtc_fb_gamma_set,
 	.gamma_get = mga_crtc_fb_gamma_get,
-	.fb_probe = mga_fb_find_or_create_single,
+	.fb_probe = mgag200fb_create,
 };
 
 int mgag200_fbdev_init(struct mga_device *mdev)
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 7b2d231..d983371 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -251,9 +251,10 @@ nouveau_fbcon_zfill(struct drm_device *dev, struct nouveau_fbdev *fbcon)
 }
 
 static int
-nouveau_fbcon_create(struct nouveau_fbdev *fbcon,
+nouveau_fbcon_create(struct drm_fb_helper *helper,
 		     struct drm_fb_helper_surface_size *sizes)
 {
+	struct nouveau_fbdev *fbcon = (struct nouveau_fbdev *)helper;
 	struct drm_device *dev = fbcon->dev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_device *device = nv_device(drm->device);
@@ -388,23 +389,6 @@ out:
 	return ret;
 }
 
-static int
-nouveau_fbcon_find_or_create_single(struct drm_fb_helper *helper,
-				    struct drm_fb_helper_surface_size *sizes)
-{
-	struct nouveau_fbdev *fbcon = (struct nouveau_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = nouveau_fbcon_create(fbcon, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 void
 nouveau_fbcon_output_poll_changed(struct drm_device *dev)
 {
@@ -450,7 +434,7 @@ void nouveau_fbcon_gpu_lockup(struct fb_info *info)
 static struct drm_fb_helper_funcs nouveau_fbcon_helper_funcs = {
 	.gamma_set = nouveau_fbcon_gamma_set,
 	.gamma_get = nouveau_fbcon_gamma_get,
-	.fb_probe = nouveau_fbcon_find_or_create_single,
+	.fb_probe = nouveau_fbcon_create,
 };
 
 
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index a44b386..b6c73b6 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -187,9 +187,10 @@ out_unref:
 	return ret;
 }
 
-static int radeonfb_create(struct radeon_fbdev *rfbdev,
+static int radeonfb_create(struct drm_fb_helper *helper,
 			   struct drm_fb_helper_surface_size *sizes)
 {
+	struct radeon_fbdev *rfbdev = (struct radeon_fbdev *)helper;
 	struct radeon_device *rdev = rfbdev->rdev;
 	struct fb_info *info;
 	struct drm_framebuffer *fb = NULL;
@@ -300,22 +301,6 @@ out_unref:
 	return ret;
 }
 
-static int radeon_fb_find_or_create_single(struct drm_fb_helper *helper,
-					   struct drm_fb_helper_surface_size *sizes)
-{
-	struct radeon_fbdev *rfbdev = (struct radeon_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = radeonfb_create(rfbdev, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 void radeon_fb_output_poll_changed(struct radeon_device *rdev)
 {
 	drm_fb_helper_hotplug_event(&rdev->mode_info.rfbdev->helper);
@@ -349,7 +334,7 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
 static struct drm_fb_helper_funcs radeon_fb_helper_funcs = {
 	.gamma_set = radeon_crtc_fb_gamma_set,
 	.gamma_get = radeon_crtc_fb_gamma_get,
-	.fb_probe = radeon_fb_find_or_create_single,
+	.fb_probe = radeonfb_create,
 };
 
 int radeon_fbdev_init(struct radeon_device *rdev)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 7d40cb7..1b51579 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -440,9 +440,10 @@ udl_framebuffer_init(struct drm_device *dev,
 }
 
 
-static int udlfb_create(struct udl_fbdev *ufbdev,
+static int udlfb_create(struct drm_fb_helper *helper,
 			struct drm_fb_helper_surface_size *sizes)
 {
+	struct udl_fbdev *ufbdev = (struct udl_fbdev *)helper;
 	struct drm_device *dev = ufbdev->helper.dev;
 	struct fb_info *info;
 	struct device *device = &dev->usbdev->dev;
@@ -520,27 +521,10 @@ out:
 	return ret;
 }
 
-static int udl_fb_find_or_create_single(struct drm_fb_helper *helper,
-					struct drm_fb_helper_surface_size *sizes)
-{
-	struct udl_fbdev *ufbdev = (struct udl_fbdev *)helper;
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = udlfb_create(ufbdev, sizes);
-		if (ret)
-			return ret;
-
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static struct drm_fb_helper_funcs udl_fb_helper_funcs = {
 	.gamma_set = udl_crtc_fb_gamma_set,
 	.gamma_get = udl_crtc_fb_gamma_get,
-	.fb_probe = udl_fb_find_or_create_single,
+	.fb_probe = udlfb_create,
 };
 
 static void udl_fbdev_destroy(struct drm_device *dev,
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 6ccaf54..11f7dcb 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -296,25 +296,10 @@ static void omap_crtc_fb_gamma_get(struct drm_crtc *crtc,
 	DBG("fbdev: get gamma");
 }
 
-static int omap_fbdev_probe(struct drm_fb_helper *helper,
-		struct drm_fb_helper_surface_size *sizes)
-{
-	int new_fb = 0;
-	int ret;
-
-	if (!helper->fb) {
-		ret = omap_fbdev_create(helper, sizes);
-		if (ret)
-			return ret;
-		new_fb = 1;
-	}
-	return new_fb;
-}
-
 static struct drm_fb_helper_funcs omap_fb_helper_funcs = {
 	.gamma_set = omap_crtc_fb_gamma_set,
 	.gamma_get = omap_crtc_fb_gamma_get,
-	.fb_probe = omap_fbdev_probe,
+	.fb_probe = omap_fbdev_create,
 };
 
 static struct drm_fb_helper *get_fb(struct fb_info *fbi)
-- 
1.7.10.4

[PATCH 15/16] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

Now that the fbdev helper interface for drivers is trimmed down,
update the kerneldoc for all the remaining exported functions.

I've tried to beat the DocBook a bit by reordering the function
references a bit into a more sensible ordering. But that didn't work
out at all. Hence just extend the in-code DOC: section a bit.

Also remove the LOCKING: sections - especially for the setup functions
they're totally bogus. But that's not a documentation problem, but
simply an artifact of the current rather hazardous locking around drm
init and even more so around fbdev setup ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |   97 +++++++++++++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index edbfcde..b774101 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -52,6 +52,17 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mode setting driver. They can be used mostly independantely from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
+ *
+ * Initialization is done as a two-step process with drm_fb_helper_init() and
+ * drm_fb_helper_initial_config(), teardown is done with drm_fb_helper_fini().
+ *
+ * At runtime drivers should restore the fbdev console by calling
+ * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They can
+ * also notify the fb helper code from updates to the output configuration by
+ * calling drm_fb_helper_hotplug_event().
+ *
+ * All other functions exported by the fb helper library can be used to
+ * implement the fbdev driver interface by the driver.
  */
 
 /* simple single crtc case helper function */
@@ -162,6 +173,10 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);
 }
 
+/**
+ * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -207,6 +222,10 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 	return NULL;
 }
 
+/**
+ * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_leave(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -377,6 +396,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	drm_modeset_unlock_all(dev);
 }
 
+/**
+ * drm_fb_helper_blank - implementation for ->fb_blank
+ * @blank: desired blanking state
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
 	switch (blank) {
@@ -420,6 +444,24 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }
 
+/**
+ * drm_fb_helper_init - initialize a drm_fb_helper structure
+ * @dev: drm device
+ * @fb_helper: driver-allocated fbdev helper structure to initialize
+ * @crtc_count: crtc count
+ * @max_conn_count: max connector count
+ *
+ * This allocates the structures for the fbdev helper with the given limits.
+ * Note that this won't yet touch the hw (through the driver interfaces) nor
+ * register the fbdev. This is only done in drm_fb_helper_initial_config() to
+ * allow driver writes more control over the exact init sequence.
+ *
+ * Drivers must also set fb_helper->funcs before calling
+ * drm_fb_helper_initial_config().
+ *
+ * RETURNS:
+ * Zero if everything went ok, nonzero otherwise.
+ */
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
 		       int crtc_count, int max_conn_count)
@@ -552,6 +594,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_setcmap - implementation for ->fb_setcmap
+ * @cmap: cmap to set
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -591,6 +638,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
+/**
+ * drm_fb_helper_check_var - implementation for ->fb_check_var
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info)
 {
@@ -683,7 +734,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_check_var);
 
-/* this will let fbcon do the mode init */
+/**
+ * drm_fb_helper_set_par - implementation for ->fb_set_par
+ * @info: fbdev registered by the helper.
+ *
+ * This will let fbcon do the mode init and is called at initialization time by
+ * the fbdev core when registering the driver, and later on through the hotplug
+ * callback.
+ */
 int drm_fb_helper_set_par(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -715,6 +773,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
+/**
+ * drm_fb_helper_pan_display - implementation for ->fb_pan_display
+ * @var: updated screen information
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 			      struct fb_info *info)
 {
@@ -876,6 +939,12 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_fill_fix - implementation for ->fb_fill_fix
+ * @info: fbdev registered by the helper.
+ * @pitch: desired pitch
+ * @depth: desired depth
+ */
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
 {
@@ -896,6 +965,12 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
+/**
+ * drm_fb_helper_fill_var - implementation for ->fb_fill_var
+ * @info: fbdev registered by the helper.
+ * @fb_width: desired fb width
+ * @fb_height: desired fb height
+ */
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height)
 {
@@ -1358,18 +1433,17 @@ out:
 }
 
 /**
- * drm_helper_initial_config - setup a sane initial connector configuration
+ * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
  * @bpp_sel: bpp value to use for the framebuffer configuration
  *
- * LOCKING:
- * Called at init time by the driver to set up the @fb_helper initial
- * configuration, must take the mode config lock.
- *
  * Scans the CRTCs and connectors and tries to put together an initial setup.
  * At the moment, this is a cloned configuration across all heads with
  * a new framebuffer object as the backing store.
  *
+ * Note that this also registers the fbdev and so allows userspace to call into
+ * the driver through the fbdev interfaces.
+ *
  * RETURNS:
  * Zero if everything went ok, nonzero otherwise.
  */
@@ -1400,12 +1474,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  *                               probing all the outputs attached to the fb
  * @fb_helper: the drm_fb_helper
  *
- * LOCKING:
- * Called at runtime, must take mode config lock.
- *
  * Scan the connectors attached to the fb_helper and try to put together a
  * setup after *notification of a change in output configuration.
  *
+ * Called at runtime, takes the mode config locks to be able to check/change the
+ * modeset configuration. Must be run from process context (which usually means
+ * either the output polling work or a work item launch from the driver's
+ * hotplug interrupt).
+ *
+ * Note that the driver must ensure that this is only called _after_ the fb has
+ * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ *
  * RETURNS:
  * 0 on success and a non-zero error code otherwise.
  */
-- 
1.7.10.4

[PATCH 16/16] drm/fb-helper: don't sleep for screen unblank when an oopps is in progress

[Daniel Vetter]

Otherwise the system will burn even brighter and worse, leave the user
wondering what's going on exactly.

Since we already have a panic handler which will (try) to restore the
entire fbdev console mode, we can just bail out. Inspired by a patch
from Konstantin Khlebnikov. The callchain leading to this, cut&pasted
from Konstantin's original patch:

callstack:
panic()
bust_spinlocks(1)
unblank_screen()
vc->vc_sw->con_blank()
fbcon_blank()
fb_blank()
info->fbops->fb_blank()
drm_fb_helper_blank()
drm_fb_helper_dpms()
drm_modeset_lock_all()
mutex_lock(&dev->mode_config.mutex)

Note that the entire locking in the fb helper around panic/sysrq and
kdbg is ... non-existant. So we have a decent change of blowing up
everything. But since reworking this ties in with funny concepts like
the fbdev notifier chain or the impressive things which happen around
console_lock while oopsing, I'll leave that as an exercise for braver
souls than me.

Cc: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
References: https://patchwork.kernel.org/patch/1878181/
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b774101..a3b4ecf 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -371,6 +371,14 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	int i, j;
 
 	/*
+	 * fbdev->blank can be called from irq context in case of a panic.
+	 * Since we already have our own special panic handler which will
+	 * restore the fbdev console mode completely, just bail out early.
+	 */
+	if (oops_in_progress)
+		return -EBUSY;
+
+	/*
 	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
 	drm_modeset_lock_all(dev);
-- 
1.7.10.4

Re: [PATCH 01/16] omapdrm: only take crtc->mutex in crtc callbacks

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Omapdrm doesn't do anything nefarious with crtc load detection or has
> any shared resources, so this is enough. We also need to adjust the
> WARN_ON.

looks good, after I double checked the locking in setplane.  In case I
didn't send this earlier,

Reviewed-by: Rob Clark <rob@ti.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/staging/omapdrm/omap_crtc.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
> index 510942e..fb5d722 100644
> --- a/drivers/staging/omapdrm/omap_crtc.c
> +++ b/drivers/staging/omapdrm/omap_crtc.c
> @@ -274,17 +274,16 @@ static void page_flip_worker(struct work_struct *work)
>         struct omap_crtc *omap_crtc =
>                         container_of(work, struct omap_crtc, page_flip_work);
>         struct drm_crtc *crtc = &omap_crtc->base;
> -       struct drm_device *dev = crtc->dev;
>         struct drm_display_mode *mode = &crtc->mode;
>         struct drm_gem_object *bo;
>
> -       drm_modeset_lock_all(dev);
> +       mutex_lock(&crtc->mutex);
>         omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb,
>                         0, 0, mode->hdisplay, mode->vdisplay,
>                         crtc->x << 16, crtc->y << 16,
>                         mode->hdisplay << 16, mode->vdisplay << 16,
>                         vblank_cb, crtc);
> -       drm_modeset_unlock_all(dev);
> +       mutex_unlock(&crtc->mutex);
>
>         bo = omap_framebuffer_bo(crtc->fb, 0);
>         drm_gem_object_unreference_unlocked(bo);
> @@ -417,7 +416,7 @@ static void apply_worker(struct work_struct *work)
>          * the callbacks and list modification all serialized
>          * with respect to modesetting ioctls from userspace.
>          */
> -       drm_modeset_lock_all(dev);
> +       mutex_lock(&crtc->mutex);
>         dispc_runtime_get();
>
>         /*
> @@ -462,16 +461,15 @@ static void apply_worker(struct work_struct *work)
>
>  out:
>         dispc_runtime_put();
> -       drm_modeset_unlock_all(dev);
> +       mutex_unlock(&crtc->mutex);
>  }
>
>  int omap_crtc_apply(struct drm_crtc *crtc,
>                 struct omap_drm_apply *apply)
>  {
>         struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> -       struct drm_device *dev = crtc->dev;
>
> -       WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +       WARN_ON(!mutex_is_locked(&crtc->mutex));
>
>         /* no need to queue it again if it is already queued: */
>         if (apply->queued)
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/16] omapdrm: simply locking in the fb debugfs file

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We don't need to hold onto mode_config.mutex any more to keep the fb
> objects around. And locking dev->struct_mutex is also not required,
> since omap_gem_describe only reads data anyway. And for a debug
> interface it's better to grab fewer locks in case the driver is
> deadlocked already ...
>
> The only thing we need is to hold onto mode_config.fb_lock to ensure
> the user-created fbs don't disappear. The fbcon fb doesn't need any
> protection, since it lives as long as the driver (and so the debugfs
> files) itself. And if the teardown/setup isn't following the right
> sequence grabbing locks won't prevent a NULL deref on priv->fbdev if
> the fb is not yet (or no longer) there.

agreed

Reviewed-by: Rob Clark <rob@ti.com>


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/staging/omapdrm/omap_debugfs.c |   14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/staging/omapdrm/omap_debugfs.c b/drivers/staging/omapdrm/omap_debugfs.c
> index e95540b..2bf0664 100644
> --- a/drivers/staging/omapdrm/omap_debugfs.c
> +++ b/drivers/staging/omapdrm/omap_debugfs.c
> @@ -57,17 +57,6 @@ static int fb_show(struct seq_file *m, void *arg)
>         struct drm_device *dev = node->minor->dev;
>         struct omap_drm_private *priv = dev->dev_private;
>         struct drm_framebuffer *fb;
> -       int ret;
> -
> -       ret = mutex_lock_interruptible(&dev->mode_config.mutex);
> -       if (ret)
> -               return ret;
> -
> -       ret = mutex_lock_interruptible(&dev->struct_mutex);
> -       if (ret) {
> -               mutex_unlock(&dev->mode_config.mutex);
> -               return ret;
> -       }
>
>         seq_printf(m, "fbcon ");
>         omap_framebuffer_describe(priv->fbdev->fb, m);
> @@ -82,9 +71,6 @@ static int fb_show(struct seq_file *m, void *arg)
>         }
>         mutex_unlock(&dev->mode_config.fb_lock);
>
> -       mutex_unlock(&dev->struct_mutex);
> -       mutex_unlock(&dev->mode_config.mutex);
> -
>         return 0;
>  }
>
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/16] [RFC] drm fb helper cleanups

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
>
> This series is mostly just a bit of fallout from my modeset locking rework which
> just landed, and some other things I've noticed while rearchitecting the modeset
> infrastructure for i915.k in 3.7.
>
> First two patches are for omapdrm, but included here since they depend upon the
> locking rework in drm-next. Dunno what to do with these patches, so I'll just
> drop them here. Rob, you really need to put the finishing touches on
> omapdrm and move it out of staging!

Yeah, I think that omapdrm is ready to move out of staging, and it
would simplify a lot of things to not have to merge changes across
drm/staging/omapdss..

I do prefer that over time, omapdrm and omapdss merge closer together
(ie. move omapdss into omapdrm, so we can make better use of drm
helpers in the omapdss bits.. and probably this starts to make even
more sense as CDF comes into the picture).  But from a functional
standpoint, now that "drm/omap: use omapdss low level API" has landed,
the kms stuff works sanely/properly finally.

Dave/Greg, how do I go about this?

BR,
-R

> The remainder just mostly cleans up the fb helper interfaces and how the helper
> code calls down into drivers. Icing on the cake is a good doc update. I'm rather
> happy with what the fbdev helper code now looks like after this. A few areas
> with potential for improvement remain though:
>
> - Locking around setup/teardown smells a bit fishy, but that ties in with the
>   fbdev setup locking (which is insane) and the drm core setup locking (which is
>   riddled with legacy stuff and also ripe for some overhaul). I think most of
>   pertaining the fbdev emulation is safe though.
>
> - Much worse is the situation around the panic, sysrq and kdbg handlers - those
>   simply grab no locks at all. Safe for the ->blank callback, which may not
>   sleep since it can be called while the kernel panics in interrupt context. Bug
>   spotted by Konstantin Khlebnikov, I've only simplified his originally proposed
>   patch a bit.
>
> - The fb helpers have their own special interface for handling gamma/luts. I've
>   looked a bit at the code, and I'm pretty sure this can be reworked to only use
>   the real kms interfaces for updating the gamma table. That would leave the
>   fb helpers with ->best_encoder and ->fb_probe as the two only special
>   interfaces. But reworking the gamma code is a bit of work, so I've postponed
>   it for now.
>
> Comments, flames, reviews and testing highly welcome.
>
> Cheers, Daniel
>
> Daniel Vetter (16):
>   omapdrm: only take crtc->mutex in crtc callbacks
>   omapdrm: simply locking in the fb debugfs file
>   drm: review locking for drm_fb_helper_restore_fbdev_mode
>   drm/fb-helper: kill drm_fb_helper_restore
>   drm/fb-helper: unexport drm_fb_helper_panic
>   drm/fb-helper: inline drm_fb_helper_single_add_all_connectors
>   drm/fb-helper: unexport drm_fb_helper_single_fb_probe
>   drm/tegra: don't set up initial fbcon config twice
>   drm/fb-helper: don't disable everything in initial_config
>   drm/i915: rip out helper->disable noop functions
>   drm/fb-helper: fixup up set_config semantics
>   drm/fb-helper: directly call set_par from the hotplug handler
>   drm/fb-helper: streamline drm_fb_helper_single_fb_probe
>   drm/<drivers>: simplify ->fb_probe callback
>   drm/fb-helper: improve kerneldoc
>   drm/fb-helper: don't sleep for screen unblank when an oopps is in
>     progress
>
>  drivers/gpu/drm/ast/ast_fb.c              |   26 +---
>  drivers/gpu/drm/cirrus/cirrus_fbdev.c     |   26 +---
>  drivers/gpu/drm/drm_fb_cma_helper.c       |   27 +---
>  drivers/gpu/drm/drm_fb_helper.c           |  216 +++++++++++++++++++++--------
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   40 +-----
>  drivers/gpu/drm/gma500/framebuffer.c      |   14 +-
>  drivers/gpu/drm/i915/intel_crt.c          |    1 -
>  drivers/gpu/drm/i915/intel_ddi.c          |    1 -
>  drivers/gpu/drm/i915/intel_display.c      |   20 +--
>  drivers/gpu/drm/i915/intel_dp.c           |    1 -
>  drivers/gpu/drm/i915/intel_drv.h          |    1 -
>  drivers/gpu/drm/i915/intel_dvo.c          |    1 -
>  drivers/gpu/drm/i915/intel_fb.c           |   23 +--
>  drivers/gpu/drm/i915/intel_hdmi.c         |    1 -
>  drivers/gpu/drm/i915/intel_lvds.c         |    1 -
>  drivers/gpu/drm/i915/intel_sdvo.c         |    1 -
>  drivers/gpu/drm/i915/intel_tv.c           |    1 -
>  drivers/gpu/drm/mgag200/mgag200_fb.c      |   26 +---
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   27 +---
>  drivers/gpu/drm/radeon/radeon_fb.c        |   25 +---
>  drivers/gpu/drm/tegra/fb.c                |    4 -
>  drivers/gpu/drm/udl/udl_fb.c              |   26 +---
>  drivers/staging/omapdrm/omap_crtc.c       |   12 +-
>  drivers/staging/omapdrm/omap_debugfs.c    |   14 --
>  drivers/staging/omapdrm/omap_fbdev.c      |   21 +--
>  include/drm/drm_fb_helper.h               |    5 -
>  26 files changed, 227 insertions(+), 334 deletions(-)
>
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/16] [RFC] drm fb helper cleanups

[Greg KH]

On Thu, Jan 24, 2013 at 10:53:25AM -0600, Rob Clark wrote:
> On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hi all,
> >
> > This series is mostly just a bit of fallout from my modeset locking rework which
> > just landed, and some other things I've noticed while rearchitecting the modeset
> > infrastructure for i915.k in 3.7.
> >
> > First two patches are for omapdrm, but included here since they depend upon the
> > locking rework in drm-next. Dunno what to do with these patches, so I'll just
> > drop them here. Rob, you really need to put the finishing touches on
> > omapdrm and move it out of staging!
> 
> Yeah, I think that omapdrm is ready to move out of staging, and it
> would simplify a lot of things to not have to merge changes across
> drm/staging/omapdss..
> 
> I do prefer that over time, omapdrm and omapdss merge closer together
> (ie. move omapdss into omapdrm, so we can make better use of drm
> helpers in the omapdss bits.. and probably this starts to make even
> more sense as CDF comes into the picture).  But from a functional
> standpoint, now that "drm/omap: use omapdss low level API" has landed,
> the kms stuff works sanely/properly finally.
> 
> Dave/Greg, how do I go about this?

There's two ways.  One, I can just move the files to the correct
location if David has no objections.

Or, if people want to review this better, create a patch that just adds
the driver to the "correct" place.  Then, when that is accepted in the
drm tree, I'll just delete the staging version.

It's up to David which he wants me to do, either is fine with me.

greg k-h

Re: [PATCH 06/16] drm/fb-helper: inline drm_fb_helper_single_add_all_connectors

[Dave Airlie]

On Fri, Jan 25, 2013 at 2:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> All drivers call this right after drm_fb_helper_init, and the only
> thing this function does is allocate a bit of memory and set up a
> bunch of pointers. No reason at all the keep this as a separate step.
>
Doesn't this stop future drivers from only adding a subset of outputs
to the fb layer?

It's kinda funny in patch 9 you give choice to drivers, and in this
patch you take away a different choice :-)

Dave.

Re: [PATCH 06/16] drm/fb-helper: inline drm_fb_helper_single_add_all_connectors

[Daniel Vetter]

On Thu, Jan 24, 2013 at 10:27 PM, Dave Airlie <airlied@gmail.com> wrote:
> On Fri, Jan 25, 2013 at 2:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> All drivers call this right after drm_fb_helper_init, and the only
>> thing this function does is allocate a bit of memory and set up a
>> bunch of pointers. No reason at all the keep this as a separate step.
>>
> Doesn't this stop future drivers from only adding a subset of outputs
> to the fb layer?

Oh, I kinda didn't come up with the idea that this is something people
actually want ... I've also killed the choice no driver opted for to
be able to resize the fb. Imo the usecase for fbdev is to show boot
messages, for which you don't need more. And oopses, which I'm unsure
whether it's really that good at.

But I'm not terribly attached to this, so can easily take it out again.

> It's kinda funny in patch 9 you give choice to drivers, and in this
> patch you take away a different choice :-)

Well, it's the choice I care about for drm/i915. I admit, I'm biased ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/fb-helper: don't disable everything in initial_config

[Daniel Vetter]

This should be done in the drivers for two reasons:
- it gets in the way of fastboot efforts
- it links the fb helpers with the crtc helpers instead of going
  through the real interface vfuncs, forcing i915 to fake all the
  ->disable callbacks used by the crtc helper to avoid ugly Oopsen

v2: Resolve conflicts since drivers still call
drm_fb_helper_single_add_all_connectors.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_fb.c              |    5 +++++
 drivers/gpu/drm/cirrus/cirrus_fbdev.c     |    4 ++++
 drivers/gpu/drm/drm_fb_cma_helper.c       |    3 +++
 drivers/gpu/drm/drm_fb_helper.c           |    3 ---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    3 +++
 drivers/gpu/drm/gma500/framebuffer.c      |    4 ++++
 drivers/gpu/drm/i915/intel_fb.c           |    3 +++
 drivers/gpu/drm/mgag200/mgag200_fb.c      |    5 +++++
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    3 +++
 drivers/gpu/drm/radeon/radeon_fb.c        |    4 ++++
 drivers/gpu/drm/udl/udl_fb.c              |    4 ++++
 drivers/staging/omapdrm/omap_fbdev.c      |    4 ++++
 12 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 3e6584b..81763ca 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -40,6 +40,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 #include "ast_drv.h"
 
 static void ast_dirty_update(struct ast_fbdev *afbdev,
@@ -314,6 +315,10 @@ int ast_fbdev_init(struct drm_device *dev)
 	}
 
 	drm_fb_helper_single_add_all_connectors(&afbdev->helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&afbdev->helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index 3daea0f..b96605c 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 
 #include <linux/fb.h>
 
@@ -291,6 +292,9 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
 		return ret;
 	}
 	drm_fb_helper_single_add_all_connectors(&gfbdev->helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(cdev->dev);
 	drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1b6ba2d..ef3d33a 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -333,6 +333,9 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
 	if (ret < 0) {
 		dev_err(dev->dev, "Failed to set inital hw configuration.\n");
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 3132d8a..f188344 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1357,9 +1357,6 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	struct drm_device *dev = fb_helper->dev;
 	int count = 0;
 
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(fb_helper->dev);
-
 	drm_fb_helper_parse_command_line(fb_helper);
 
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 086d0f7..fe2a0f0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -295,6 +295,9 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 
 	}
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
 	if (ret < 0) {
 		DRM_ERROR("failed to set up hw configuration.\n");
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index c1ef37e..fee3bf8 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -616,6 +616,10 @@ int psb_fbdev_init(struct drm_device *dev)
 							INTELFB_CONN_LIMIT);
 
 	drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 33c4f1b..69b5b3c 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -258,6 +258,9 @@ void intel_fbdev_initial_config(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
 	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
 }
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 5c69b43..5bded5b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
 
 #include <linux/fb.h>
 
@@ -278,6 +279,10 @@ int mgag200_fbdev_init(struct mga_device *mdev)
 		return ret;
 	}
 	drm_fb_helper_single_add_all_connectors(&mfbdev->helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(mdev->dev);
+
 	drm_fb_helper_initial_config(&mfbdev->helper, 32);
 
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index d4ecb4d..b1ebfe3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -491,6 +491,9 @@ nouveau_fbcon_init(struct drm_device *dev)
 	else
 		preferred_bpp = 32;
 
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&fbcon->helper, preferred_bpp);
 	return 0;
 }
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index 515e5ee..b48d1c8 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -379,6 +379,10 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 	}
 
 	drm_fb_helper_single_add_all_connectors(&rfbdev->helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(rdev->ddev);
+
 	drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index caa84f1..b610c3c 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -583,6 +583,10 @@ int udl_fbdev_init(struct drm_device *dev)
 	}
 
 	drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 2728e37..7e66eb1 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -369,6 +369,10 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
 	}
 
 	drm_fb_helper_single_add_all_connectors(helper);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
+
 	drm_fb_helper_initial_config(helper, 32);
 
 	priv->fbdev = helper;
-- 
1.7.10.4

[PATCH] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

Now that the fbdev helper interface for drivers is trimmed down,
update the kerneldoc for all the remaining exported functions.

I've tried to beat the DocBook a bit by reordering the function
references a bit into a more sensible ordering. But that didn't work
out at all. Hence just extend the in-code DOC: section a bit.

Also remove the LOCKING: sections - especially for the setup functions
they're totally bogus. But that's not a documentation problem, but
simply an artifact of the current rather hazardous locking around drm
init and even more so around fbdev setup ...

v2: Some further improvements:
- Also add documentation for drm_fb_helper_single_add_all_connectors,
  Dave Airlie didn't want me to kill this one from the fb helper
  interface.
- Update docs for drm_fb_helper_fill_var/fix - they should be used
  from the driver's ->fb_probe callback to setup the fbdev info
  structure.
- Clarify what the ->fb_probe callback should all do - it needs to
  setup both the fbdev info and allocate the drm framebuffer used as
  backing storage.
- Add basic documentaation for the drm_fb_helper_funcs driver callback
  vfunc.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

moar kerneldoc
---
 Documentation/DocBook/drm.tmpl  |    1 +
 drivers/gpu/drm/drm_fb_helper.c |  146 +++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     |   11 +++
 3 files changed, 143 insertions(+), 15 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 6c11d77..14ad829 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
       <title>fbdev Helper Functions Reference</title>
 !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
 !Edrivers/gpu/drm/drm_fb_helper.c
+!Iinclude/drm/drm_fb_helper.h
     </sect2>
     <sect2>
       <title>Display Port Helper Functions Reference</title>
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5a92470..a7538cc 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mode setting driver. They can be used mostly independantely from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
+ *
+ * Initialization is done as a three-step process with drm_fb_helper_init(),
+ * drm_fb_helper_single_add_all_connectors() and drm_fb_helper_initial_config().
+ * Drivers with fancier requirements than the default beheviour can override the
+ * second step with their own code.  Teardown is done with drm_fb_helper_fini().
+ *
+ * At runtime drivers should restore the fbdev console by calling
+ * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They can
+ * also notify the fb helper code from updates to the output configuration by
+ * calling drm_fb_helper_hotplug_event().
+ *
+ * All other functions exported by the fb helper library can be used to
+ * implement the fbdev driver interface by the driver.
  */
 
-/* simple single crtc case helper function */
+/**
+ * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
+ * 					       emulation helper
+ * @fb_helper: fbdev initialized with drm_fb_helper_init
+ *
+ * This functions adds all the available connectors for use with the given
+ * fb_helper. This is a separate step to allow drivers to freely assign
+ * connectors to the fbdev, e.g. if some are reserved for special purposes or
+ * not adequate to be used for the fbcon.
+ *
+ * Since this is part of the initial setup before the fbdev is published, no
+ * locking is required.
+ */
 int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);
 }
 
+/**
+ * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -208,6 +237,10 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 	return NULL;
 }
 
+/**
+ * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_leave(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
  * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
  * @fb_helper: fbcon to restore
  *
- * This should be called from driver's drm->lastclose callback when implementing
- * an fbcon on top of kms using this helper. This ensures that the user isn't
- * greeted with a black screen when e.g. X dies.
+ * This should be called from driver's drm <code>->lastclose</code> callback
+ * when implementing an fbcon on top of kms using this helper. This ensures that
+ * the user isn't greeted with a black screen when e.g. X dies.
  */
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
@@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	drm_modeset_unlock_all(dev);
 }
 
+/**
+ * drm_fb_helper_blank - implementation for ->fb_blank
+ * @blank: desired blanking state
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
 	switch (blank) {
@@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }
 
+/**
+ * drm_fb_helper_init - initialize a drm_fb_helper structure
+ * @dev: drm device
+ * @fb_helper: driver-allocated fbdev helper structure to initialize
+ * @crtc_count: crtc count
+ * @max_conn_count: max connector count
+ *
+ * This allocates the structures for the fbdev helper with the given limits.
+ * Note that this won't yet touch the hw (through the driver interfaces) nor
+ * register the fbdev. This is only done in drm_fb_helper_initial_config() to
+ * allow driver writes more control over the exact init sequence.
+ *
+ * Drivers must also set fb_helper->funcs before calling
+ * drm_fb_helper_initial_config().
+ *
+ * RETURNS:
+ * Zero if everything went ok, nonzero otherwise.
+ */
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
 		       int crtc_count, int max_conn_count)
@@ -549,6 +605,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_setcmap - implementation for ->fb_setcmap
+ * @cmap: cmap to set
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -588,6 +649,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
+/**
+ * drm_fb_helper_check_var - implementation for ->fb_check_var
+ * @var: screeninfo to check
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info)
 {
@@ -680,7 +746,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_check_var);
 
-/* this will let fbcon do the mode init */
+/**
+ * drm_fb_helper_set_par - implementation for ->fb_set_par
+ * @info: fbdev registered by the helper.
+ *
+ * This will let fbcon do the mode init and is called at initialization time by
+ * the fbdev core when registering the driver, and later on through the hotplug
+ * callback.
+ */
 int drm_fb_helper_set_par(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -712,6 +785,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
+/**
+ * drm_fb_helper_pan_display - implementation for ->fb_pan_display
+ * @var: updated screen information
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 			      struct fb_info *info)
 {
@@ -750,8 +828,9 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
 /*
- * Allocates the backing storage through the ->fb_probe callback and then
- * registers the fbdev and sets up the panic notifier.
+ * 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.
  */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
@@ -873,6 +952,19 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_fill_fix - initializes fixed fbdev information
+ * @info: fbdev registered by the helper.
+ * @pitch: desired pitch
+ * @depth: desired depth
+ *
+ * Helper to fill in the fixed fbdev information useful for a non-accelerated
+ * fbdev emulations. Drivers which support acceleration methods which impose
+ * additional constraints need to set up their own limits.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * <code>->fb_probe</code> callback.
+ */
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
 {
@@ -893,6 +985,20 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
+/**
+ * drm_fb_helper_fill_var - initalizes variable fbdev information
+ * @info: fbdev instance to set up
+ * @fb_helper: fb helper instance to use as template
+ * @fb_width: desired fb width
+ * @fb_height: desired fb height
+ *
+ * Sets up the variable fbdev metainformation from the given fb helper instance
+ * and the drm framebuffer allocated in <code>fb_helper->fb</code>.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * <code>->fb_probe</code> callback after having allocated the fbdev backing
+ * storage framebuffer.
+ */
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height)
 {
@@ -1355,18 +1461,23 @@ out:
 }
 
 /**
- * drm_helper_initial_config - setup a sane initial connector configuration
+ * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
  * @bpp_sel: bpp value to use for the framebuffer configuration
  *
- * LOCKING:
- * Called at init time by the driver to set up the @fb_helper initial
- * configuration, must take the mode config lock.
- *
  * Scans the CRTCs and connectors and tries to put together an initial setup.
  * At the moment, this is a cloned configuration across all heads with
  * a new framebuffer object as the backing store.
  *
+ * Note that this also registers the fbdev and so allows userspace to call into
+ * the driver through the fbdev interfaces.
+ *
+ * This function will call down into the <code>->fb_probe</code> callback to let
+ * the driver allocate and initialize the fbdev info structure and the drm
+ * framebuffer used to back the fbdev. drm_fb_helper_fill_var() and
+ * drm_fb_helper_fill_fix() are provided as helpers to setup simple default
+ * values for the fbdev info structure.
+ *
  * RETURNS:
  * Zero if everything went ok, nonzero otherwise.
  */
@@ -1397,12 +1508,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  *                               probing all the outputs attached to the fb
  * @fb_helper: the drm_fb_helper
  *
- * LOCKING:
- * Called at runtime, must take mode config lock.
- *
  * Scan the connectors attached to the fb_helper and try to put together a
  * setup after *notification of a change in output configuration.
  *
+ * Called at runtime, takes the mode config locks to be able to check/change the
+ * modeset configuration. Must be run from process context (which usually means
+ * either the output polling work or a work item launch from the driver's
+ * hotplug interrupt).
+ *
+ * Note that the driver must ensure that this is only called _after_ the fb has
+ * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ *
  * RETURNS:
  * 0 on success and a non-zero error code otherwise.
  */
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 973402d..3c30297 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -48,6 +48,17 @@ struct drm_fb_helper_surface_size {
 	u32 surface_depth;
 };
 
+/**
+ * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation library
+ * @gamma_set: - Set the given lut register on the given crtc.
+ * @gamma_get: - Read the given lut register on the given crtc, used to safe the
+ *		 current lut when force-restoring the fbdev for e.g. kdbg.
+ * @fb_probe: - Driver callback to allocate and initialize the fbdev info
+ * 		structure. Futhermore it also needs to allocate the drm
+ * 		framebuffer used to back the fbdev.
+ *
+ * Driver callbacks used by the fbdev emulation helper library.
+ */
 struct drm_fb_helper_funcs {
 	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
 			  u16 blue, int regno);
-- 
1.7.10.4

[PATCH] drm/fb-helper: don't sleep for screen unblank when an oopps is in progress

[Daniel Vetter]

Otherwise the system will burn even brighter and worse, leave the user
wondering what's going on exactly.

Since we already have a panic handler which will (try) to restore the
entire fbdev console mode, we can just bail out. Inspired by a patch
from Konstantin Khlebnikov. The callchain leading to this, cut&pasted
from Konstantin's original patch:

callstack:
panic()
bust_spinlocks(1)
unblank_screen()
vc->vc_sw->con_blank()
fbcon_blank()
fb_blank()
info->fbops->fb_blank()
drm_fb_helper_blank()
drm_fb_helper_dpms()
drm_modeset_lock_all()
mutex_lock(&dev->mode_config.mutex)

Note that the entire locking in the fb helper around panic/sysrq and
kdbg is ... non-existant. So we have a decent change of blowing up
everything. But since reworking this ties in with funny concepts like
the fbdev notifier chain or the impressive things which happen around
console_lock while oopsing, I'll leave that as an exercise for braver
souls than me.

v2: Drop the -EBUSY return value I've copied, we don't need it since
the we'll take care of things ourselves anyway.

Cc: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
References: https://patchwork.kernel.org/patch/1878181/
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a7538cc..01099b5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -386,6 +386,14 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	int i, j;
 
 	/*
+	 * fbdev->blank can be called from irq context in case of a panic.
+	 * Since we already have our own special panic handler which will
+	 * restore the fbdev console mode completely, just bail out early.
+	 */
+	if (oops_in_progress)
+		return;
+
+	/*
 	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
 	drm_modeset_lock_all(dev);
-- 
1.7.10.4

Re: [PATCH] drm/fb-helper: improve kerneldoc

[Laurent Pinchart]

Hi Daniel,

Thanks for improving the documentation :-)

On Sunday 27 January 2013 18:42:16 Daniel Vetter wrote:
> Now that the fbdev helper interface for drivers is trimmed down,
> update the kerneldoc for all the remaining exported functions.
> 
> I've tried to beat the DocBook a bit by reordering the function
> references a bit into a more sensible ordering. But that didn't work
> out at all. Hence just extend the in-code DOC: section a bit.
> 
> Also remove the LOCKING: sections - especially for the setup functions
> they're totally bogus. But that's not a documentation problem, but
> simply an artifact of the current rather hazardous locking around drm
> init and even more so around fbdev setup ...

Please see below for comments (I've reflowed the text to ease review).

> v2: Some further improvements:
> - Also add documentation for drm_fb_helper_single_add_all_connectors,
>   Dave Airlie didn't want me to kill this one from the fb helper
>   interface.
> - Update docs for drm_fb_helper_fill_var/fix - they should be used
>   from the driver's ->fb_probe callback to setup the fbdev info
>   structure.
> - Clarify what the ->fb_probe callback should all do - it needs to
>   setup both the fbdev info and allocate the drm framebuffer used as
>   backing storage.
> - Add basic documentaation for the drm_fb_helper_funcs driver callback
>   vfunc.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> moar kerneldoc
> ---
>  Documentation/DocBook/drm.tmpl  |    1 +
>  drivers/gpu/drm/drm_fb_helper.c |  146 ++++++++++++++++++++++++++++++++----
>  include/drm/drm_fb_helper.h     |   11 +++
>  3 files changed, 143 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 6c11d77..14ad829 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
>        <title>fbdev Helper Functions Reference</title>
>  !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
>  !Edrivers/gpu/drm/drm_fb_helper.c
> +!Iinclude/drm/drm_fb_helper.h
>      </sect2>
>      <sect2>
>        <title>Display Port Helper Functions Reference</title>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c index 5a92470..a7538cc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * mode setting driver. They can be used mostly independantely from the

Now that you have removed one of the dependencies on the crtc helpers in your 
"drm/fb-helper: don't disable everything in initial_config" patch, are there 
others ? If not you can s/mostly //.

>   * crtc helper functions used by many drivers to implement the kernel mode
>   * setting interfaces.
> + *
> + * Initialization is done as a three-step process with
> + * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> + * drm_fb_helper_initial_config(). Drivers with fancier requirements than
> + * the default beheviour can override the second step with their own code.
> + * Teardown is done with drm_fb_helper_fini().
> + *
> + * At runtime drivers should restore the fbdev console by calling
> + * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They
> + * can also notify the fb helper code from updates to the output

Is it "can" or "must" ? If "can", in what conditions should they call 
drm_fb_helper_restore_fbdev_mode() and in what conditions shouldn't they ?

> + * configuration by calling drm_fb_helper_hotplug_event().
> + *
> + * All other functions exported by the fb helper library can be used to
> + * implement the fbdev driver interface by the driver.
>   */
> 
> -/* simple single crtc case helper function */
> +/**
> + * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> + * 					       emulation helper
> + * @fb_helper: fbdev initialized with drm_fb_helper_init
> + *
> + * This functions adds all the available connectors for use with the given
> + * fb_helper. This is a separate step to allow drivers to freely assign or
> + * connectors to the fbdev, e.g. if some are reserved for special purposes
> + * not adequate to be used for the fbcon.
> + *
> + * Since this is part of the initial setup before the fbdev is published,
> + * no locking is required.
> + */
>  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper
> *fb_helper)
> {
>  	struct drm_device *dev = fb_helper->dev;
> @@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct
> drm_crtc *crtc) crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0,
> crtc->gamma_size); }
> 
> +/**
> + * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_debug_enter(struct fb_info *info)
>  {
>  	struct drm_fb_helper *helper = info->par;
> @@ -208,6 +237,10 @@ static struct drm_framebuffer
> *drm_mode_config_fb(struct drm_crtc *crtc) return NULL;
>  }
> 
> +/**
> + * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_debug_leave(struct fb_info *info)
>  {
>  	struct drm_fb_helper *helper = info->par;
> @@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>   * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
>   * @fb_helper: fbcon to restore
>   *
> - * This should be called from driver's drm->lastclose callback when
> - * implementing an fbcon on top of kms using this helper. This ensures that
> - * the user isn't greeted with a black screen when e.g. X dies.
> + * This should be called from driver's drm <code>->lastclose</code>

The resulting HTML is

&lt;code&gt;-&gt;lastclose&lt;/code&gt;

I'm not sure that's what you want :-)

> + * callback when implementing an fbcon on top of kms using this helper.
> + * This ensures that the user isn't greeted with a black screen when e.g.
> + * X dies.
>   */
>  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
> @@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info,
> int dpms_mode) drm_modeset_unlock_all(dev);
>  }
> 
> +/**
> + * drm_fb_helper_blank - implementation for ->fb_blank
> + * @blank: desired blanking state
> + * @info: fbdev registered by the helper.

Nitpicking here, shouldn't you either use a full stop at the end of each 
argument, or no full stop at all (this applies to the other functions as well) 
?

> + */
>  int drm_fb_helper_blank(int blank, struct fb_info *info)
>  {
>  	switch (blank) {
> @@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct
> drm_fb_helper *helper) kfree(helper->crtc_info);
>  }
> 
> +/**
> + * drm_fb_helper_init - initialize a drm_fb_helper structure
> + * @dev: drm device
> + * @fb_helper: driver-allocated fbdev helper structure to initialize
> + * @crtc_count: crtc count
> + * @max_conn_count: max connector count
> + *
> + * This allocates the structures for the fbdev helper with the given
> + * limits. Note that this won't yet touch the hw (through the driver

s/hw/hardware/ ?

It might be a good idea to add a small description of the crtc_count 
parameter.

> + * interfaces) nor register the fbdev. This is only done in
> + * drm_fb_helper_initial_config() to allow driver writes more control over
> + * the exact init sequence.
> + *
> + * Drivers must also set fb_helper->funcs before calling

s/also // ?

> + * drm_fb_helper_initial_config().
> + *
> + * RETURNS:
> + * Zero if everything went ok, nonzero otherwise.
> + */
>  int drm_fb_helper_init(struct drm_device *dev,
>  		       struct drm_fb_helper *fb_helper,
>  		       int crtc_count, int max_conn_count)
> @@ -549,6 +605,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red,
> u16 green, return 0;
>  }
> 
> +/**
> + * drm_fb_helper_setcmap - implementation for ->fb_setcmap
> + * @cmap: cmap to set
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> @@ -588,6 +649,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct
> fb_info *info) }
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
> 
> +/**
> + * drm_fb_helper_check_var - implementation for ->fb_check_var
> + * @var: screeninfo to check
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  			    struct fb_info *info)
>  {
> @@ -680,7 +746,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo
> *var, }
>  EXPORT_SYMBOL(drm_fb_helper_check_var);
> 
> -/* this will let fbcon do the mode init */
> +/**
> + * drm_fb_helper_set_par - implementation for ->fb_set_par
> + * @info: fbdev registered by the helper.
> + *
> + * This will let fbcon do the mode init and is called at initialization
> + * time by the fbdev core when registering the driver, and later on
> + * through the hotplug callback.
> + */
>  int drm_fb_helper_set_par(struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> @@ -712,6 +785,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_set_par);
> 
> +/**
> + * drm_fb_helper_pan_display - implementation for ->fb_pan_display
> + * @var: updated screen information
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  			      struct fb_info *info)
>  {
> @@ -750,8 +828,9 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
> *var, EXPORT_SYMBOL(drm_fb_helper_pan_display);
> 
>  /*
> - * Allocates the backing storage through the ->fb_probe callback and then
> - * registers the fbdev and sets up the panic notifier.
> + * 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.
>   */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  					 int preferred_bpp)
> @@ -873,6 +952,19 @@ static int drm_fb_helper_single_fb_probe(struct
> drm_fb_helper *fb_helper, return 0;
>  }
> 
> +/**
> + * drm_fb_helper_fill_fix - initializes fixed fbdev information
> + * @info: fbdev registered by the helper.
> + * @pitch: desired pitch
> + * @depth: desired depth
> + *
> + * Helper to fill in the fixed fbdev information useful for a
> + * non-accelerated fbdev emulations. Drivers which support acceleration
> + * methods which impose additional constraints need to set up their own
> + * limits.
> + *
> + * Drivers should call this (or their equivalent setup code) from their
> + * <code>->fb_probe</code> callback.
> + */
>  void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>  			    uint32_t depth)
>  {
> @@ -893,6 +985,20 @@ void drm_fb_helper_fill_fix(struct fb_info *info,
> uint32_t pitch, }
>  EXPORT_SYMBOL(drm_fb_helper_fill_fix);
> 
> +/**
> + * drm_fb_helper_fill_var - initalizes variable fbdev information
> + * @info: fbdev instance to set up
> + * @fb_helper: fb helper instance to use as template
> + * @fb_width: desired fb width
> + * @fb_height: desired fb height
> + *
> + * Sets up the variable fbdev metainformation from the given fb helper
> + * instance and the drm framebuffer allocated in
> + * <code>fb_helper->fb</code>.
> + *
> + * Drivers should call this (or their equivalent setup code) from their
> + * <code>->fb_probe</code> callback after having allocated the fbdev
> + * backing storage framebuffer.
> + */
>  void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper
> *fb_helper, uint32_t fb_width, uint32_t fb_height)
>  {
> @@ -1355,18 +1461,23 @@ out:
>  }
> 
>  /**
> - * drm_helper_initial_config - setup a sane initial connector configuration
> + * drm_fb_helper_initial_config - setup a sane initial connector
> configuration
> * @fb_helper: fb_helper device struct
>   * @bpp_sel: bpp value to use for the framebuffer configuration
>   *
> - * LOCKING:
> - * Called at init time by the driver to set up the @fb_helper initial
> - * configuration, must take the mode config lock.
> - *
>   * Scans the CRTCs and connectors and tries to put together an initial
>   * setup. At the moment, this is a cloned configuration across all heads
>   * with a new framebuffer object as the backing store.
>   *
> + * Note that this also registers the fbdev and so allows userspace to call
> + * into the driver through the fbdev interfaces.
> + *
> + * This function will call down into the <code>->fb_probe</code> callback

<code> is displayed in the resulting HTML here as well.

> + * to let the driver allocate and initialize the fbdev info structure and
> + * the drm framebuffer used to back the fbdev. drm_fb_helper_fill_var()
> + * and drm_fb_helper_fill_fix() are provided as helpers to setup simple
> + * default values for the fbdev info structure.
> + *
>   * RETURNS:
>   * Zero if everything went ok, nonzero otherwise.
>   */
> @@ -1397,12 +1508,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   *                               probing all the outputs attached to the fb
> * @fb_helper: the drm_fb_helper
>   *
> - * LOCKING:
> - * Called at runtime, must take mode config lock.
> - *
>   * Scan the connectors attached to the fb_helper and try to put together a
>   * setup after *notification of a change in output configuration.
>   *
> + * Called at runtime, takes the mode config locks to be able to
> + * check/change the modeset configuration. Must be run from process
> + * context (which usually means either the output polling work or a work
> + * item launch from the driver's hotplug interrupt).

s/launch/launched/

> + * Note that the driver must ensure that this is only called _after_ the fb
> + * has been fully set up, i.e. after the call to
> + * drm_fb_helper_initial_config.
> + *
>   * RETURNS:
>   * 0 on success and a non-zero error code otherwise.
>   */
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 973402d..3c30297 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -48,6 +48,17 @@ struct drm_fb_helper_surface_size {
>  	u32 surface_depth;
>  };
> 
> +/**
> + * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation
> library
> + * @gamma_set: - Set the given lut register on the given crtc.
> + * @gamma_get: - Read the given lut register on the given crtc, used to
> safe the

s/lut/gamma lut/ ?

> + *		 current lut when force-restoring the fbdev for e.g. kdbg.
> + * @fb_probe: - Driver callback to allocate and initialize the fbdev info
> + * 		structure. Futhermore it also needs to allocate the drm
> + * 		framebuffer used to back the fbdev.
> + *
> + * Driver callbacks used by the fbdev emulation helper library.
> + */
>  struct drm_fb_helper_funcs {
>  	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
>  			  u16 blue, int regno);
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

On Fri, Feb 01, 2013 at 01:21:29PM +0100, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thanks for improving the documentation :-)
> 
> On Sunday 27 January 2013 18:42:16 Daniel Vetter wrote:
> > Now that the fbdev helper interface for drivers is trimmed down,
> > update the kerneldoc for all the remaining exported functions.
> > 
> > I've tried to beat the DocBook a bit by reordering the function
> > references a bit into a more sensible ordering. But that didn't work
> > out at all. Hence just extend the in-code DOC: section a bit.
> > 
> > Also remove the LOCKING: sections - especially for the setup functions
> > they're totally bogus. But that's not a documentation problem, but
> > simply an artifact of the current rather hazardous locking around drm
> > init and even more so around fbdev setup ...
> 
> Please see below for comments (I've reflowed the text to ease review).
> 
> > v2: Some further improvements:
> > - Also add documentation for drm_fb_helper_single_add_all_connectors,
> >   Dave Airlie didn't want me to kill this one from the fb helper
> >   interface.
> > - Update docs for drm_fb_helper_fill_var/fix - they should be used
> >   from the driver's ->fb_probe callback to setup the fbdev info
> >   structure.
> > - Clarify what the ->fb_probe callback should all do - it needs to
> >   setup both the fbdev info and allocate the drm framebuffer used as
> >   backing storage.
> > - Add basic documentaation for the drm_fb_helper_funcs driver callback
> >   vfunc.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > moar kerneldoc
> > ---
> >  Documentation/DocBook/drm.tmpl  |    1 +
> >  drivers/gpu/drm/drm_fb_helper.c |  146 ++++++++++++++++++++++++++++++++----
> >  include/drm/drm_fb_helper.h     |   11 +++
> >  3 files changed, 143 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 6c11d77..14ad829 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
> >        <title>fbdev Helper Functions Reference</title>
> >  !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
> >  !Edrivers/gpu/drm/drm_fb_helper.c
> > +!Iinclude/drm/drm_fb_helper.h
> >      </sect2>
> >      <sect2>
> >        <title>Display Port Helper Functions Reference</title>
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c index 5a92470..a7538cc 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
> >   * mode setting driver. They can be used mostly independantely from the
> 
> Now that you have removed one of the dependencies on the crtc helpers in your 
> "drm/fb-helper: don't disable everything in initial_config" patch, are there 
> others ? If not you can s/mostly //.

It's getting better, but a few of the driver callbacks used by the fb
helper are still in the crtc helper function tables. And there's the fb
helper private way to safe/restore gamma tables. But at least semantically
there's no dependency any longer after these patches I think.

> 
> >   * crtc helper functions used by many drivers to implement the kernel mode
> >   * setting interfaces.
> > + *
> > + * Initialization is done as a three-step process with
> > + * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> > + * drm_fb_helper_initial_config(). Drivers with fancier requirements than
> > + * the default beheviour can override the second step with their own code.
> > + * Teardown is done with drm_fb_helper_fini().
> > + *
> > + * At runtime drivers should restore the fbdev console by calling
> > + * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They
> > + * can also notify the fb helper code from updates to the output
> 
> Is it "can" or "must" ? If "can", in what conditions should they call 
> drm_fb_helper_restore_fbdev_mode() and in what conditions shouldn't they ?

I've figured that hpd support is optional, hence no "must". I've opted for
a should instead. Also added a bit of text to suggest a good place to put
this call.

> 
> > + * configuration by calling drm_fb_helper_hotplug_event().
> > + *
> > + * All other functions exported by the fb helper library can be used to
> > + * implement the fbdev driver interface by the driver.
> >   */
> > 
> > -/* simple single crtc case helper function */
> > +/**
> > + * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> > + * 					       emulation helper
> > + * @fb_helper: fbdev initialized with drm_fb_helper_init
> > + *
> > + * This functions adds all the available connectors for use with the given
> > + * fb_helper. This is a separate step to allow drivers to freely assign or
> > + * connectors to the fbdev, e.g. if some are reserved for special purposes
> > + * not adequate to be used for the fbcon.
> > + *
> > + * Since this is part of the initial setup before the fbdev is published,
> > + * no locking is required.
> > + */
> >  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper
> > *fb_helper)
> > {
> >  	struct drm_device *dev = fb_helper->dev;
> > @@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct
> > drm_crtc *crtc) crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0,
> > crtc->gamma_size); }
> > 
> > +/**
> > + * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
> > + * @info: fbdev registered by the helper.
> > + */
> >  int drm_fb_helper_debug_enter(struct fb_info *info)
> >  {
> >  	struct drm_fb_helper *helper = info->par;
> > @@ -208,6 +237,10 @@ static struct drm_framebuffer
> > *drm_mode_config_fb(struct drm_crtc *crtc) return NULL;
> >  }
> > 
> > +/**
> > + * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
> > + * @info: fbdev registered by the helper.
> > + */
> >  int drm_fb_helper_debug_leave(struct fb_info *info)
> >  {
> >  	struct drm_fb_helper *helper = info->par;
> > @@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> >   * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
> >   * @fb_helper: fbcon to restore
> >   *
> > - * This should be called from driver's drm->lastclose callback when
> > - * implementing an fbcon on top of kms using this helper. This ensures that
> > - * the user isn't greeted with a black screen when e.g. X dies.
> > + * This should be called from driver's drm <code>->lastclose</code>
> 
> The resulting HTML is
> 
> &lt;code&gt;-&gt;lastclose&lt;/code&gt;
> 
> I'm not sure that's what you want :-)

Nope, killed them all.

> 
> > + * callback when implementing an fbcon on top of kms using this helper.
> > + * This ensures that the user isn't greeted with a black screen when e.g.
> > + * X dies.
> >   */
> >  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >  {
> > @@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info,
> > int dpms_mode) drm_modeset_unlock_all(dev);
> >  }
> > 
> > +/**
> > + * drm_fb_helper_blank - implementation for ->fb_blank
> > + * @blank: desired blanking state
> > + * @info: fbdev registered by the helper.
> 
> Nitpicking here, shouldn't you either use a full stop at the end of each 
> argument, or no full stop at all (this applies to the other functions as well) 
> ?

Copy&pasta, full stop dropped everywhere.

> 
> > + */
> >  int drm_fb_helper_blank(int blank, struct fb_info *info)
> >  {
> >  	switch (blank) {
> > @@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct
> > drm_fb_helper *helper) kfree(helper->crtc_info);
> >  }
> > 
> > +/**
> > + * drm_fb_helper_init - initialize a drm_fb_helper structure
> > + * @dev: drm device
> > + * @fb_helper: driver-allocated fbdev helper structure to initialize
> > + * @crtc_count: crtc count
> > + * @max_conn_count: max connector count
> > + *
> > + * This allocates the structures for the fbdev helper with the given
> > + * limits. Note that this won't yet touch the hw (through the driver
> 
> s/hw/hardware/ ?
> 
> It might be a good idea to add a small description of the crtc_count 
> parameter.

Done. All the other corrections below I've also applied.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/16] drm/fb-helper: kill drm_fb_helper_restore

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It's only used internally for the sysrq and panic handlers provided by
> the drm fb helper implementation. Hence just inline it, kill the
> export and remove the confusing kerneldoc. Driver's are supposed to
> call drm_fb_helper_restore_fbdev_mode on lastclose.
>
> Note that locking is totally fubar - the sysrq case doesn't take any
> locks at all. The panic handler probably shouldn't take any locks
> since it'll only make things worse. Otoh it's probably better to
> switch things over to the atomic modeset callbacks (and disable the
> panic handler for those drivers which don't implement it).
>
> But that's both better done in separate patches.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c |   23 ++++++++---------------
>  include/drm/drm_fb_helper.h     |    1 -
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0439cb0..6d689f2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -261,6 +261,10 @@ bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode);
>
> +/*
> + * restore fbcon display for all kms driver's using this helper, used for sysrq
> + * and panic handling.
> + */
>  static bool drm_fb_helper_force_kernel_mode(void)
>  {
>         bool ret, error = false;
> @@ -299,20 +303,6 @@ static struct notifier_block paniced = {
>         .notifier_call = drm_fb_helper_panic,
>  };
>
> -/**
> - * drm_fb_helper_restore - restore the framebuffer console (kernel) config
> - *
> - * Restore's the kernel's fbcon mode, used for lastclose & panic paths.
> - */
> -void drm_fb_helper_restore(void)
> -{
> -       bool ret;
> -       ret = drm_fb_helper_force_kernel_mode();
> -       if (ret == true)
> -               DRM_ERROR("Failed to restore crtc configuration\n");
> -}
> -EXPORT_SYMBOL(drm_fb_helper_restore);
> -
>  static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>  {
>         struct drm_device *dev = fb_helper->dev;
> @@ -334,7 +324,10 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>  #ifdef CONFIG_MAGIC_SYSRQ
>  static void drm_fb_helper_restore_work_fn(struct work_struct *ignored)
>  {
> -       drm_fb_helper_restore();
> +       bool ret;
> +       ret = drm_fb_helper_force_kernel_mode();
> +       if (ret == true)
> +               DRM_ERROR("Failed to restore crtc configuration\n");
>  }
>  static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn);
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5120b01..ba32505 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -103,7 +103,6 @@ int drm_fb_helper_setcolreg(unsigned regno,
>                             struct fb_info *info);
>
>  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper);
> -void drm_fb_helper_restore(void);
>  void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
>                             uint32_t fb_width, uint32_t fb_height);
>  void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/16] drm: review locking for drm_fb_helper_restore_fbdev_mode

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ... it's required. Fix up exynos and the cma helper, and add a
> corresponding WARN_ON to drm_fb_helper_restore_fbdev_mode.
>
> Note that tegra calls the fbdev cma helper restore function also from
> it's driver-load callback. Which is a bit against current practice,
> since usually the call is only from ->lastclose, and initial setup is
> done by drm_fb_helper_initial_config.
>
> Also add the relevant drm DocBook entry.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

With the actual addition of the promised WARN_ON,

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c       |    2 ++
>  drivers/gpu/drm/drm_fb_helper.c           |    8 ++++++++
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    2 ++
>  3 files changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 3742bc9..1b6ba2d 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -389,8 +389,10 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
>   */
>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
>  {
> +       drm_modeset_lock_all(dev);
>         if (fbdev_cma)
>                 drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
> +       drm_modeset_unlock_all(dev);
>  }
>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0c6e25e..0439cb0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -239,6 +239,14 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> +/**
> + * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
> + * @fb_helper: fbcon to restore
> + *
> + * This should be called from driver's drm->lastclose callback when implementing
> + * an fbcon on top of kms using this helper. This ensures that the user isn't
> + * greeted with a black screen when e.g. X dies.
> + */
>  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
>         bool error = false;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 90d335c..086d0f7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -376,5 +376,7 @@ void exynos_drm_fbdev_restore_mode(struct drm_device *dev)
>         if (!private || !private->fb_helper)
>                 return;
>
> +       drm_modeset_lock_all(dev);
>         drm_fb_helper_restore_fbdev_mode(private->fb_helper);
> +       drm_modeset_unlock_all(dev);
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/16] drm/fb-helper: unexport drm_fb_helper_panic

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It doesn't even show up in any header files and only used iternally.
> Originally it was (ab)used to restore the fbcon on lastclose, but that
> died with
>
> commit e8e7a2b8ccfdae0d4cb6bd25824bbedcd42da316
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Thu Apr 21 22:18:32 2011 +0100
>
>     drm/i915: restore only the mode of this driver on lastclose (v2)
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6d689f2..ce816a5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -284,7 +284,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
>         return error;
>  }
>
> -int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
> +static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
>                         void *panic_str)
>  {
>         /*
> @@ -297,7 +297,6 @@ int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
>         pr_err("panic occurred, switching back to text console\n");
>         return drm_fb_helper_force_kernel_mode();
>  }
> -EXPORT_SYMBOL(drm_fb_helper_panic);
>
>  static struct notifier_block paniced = {
>         .notifier_call = drm_fb_helper_panic,
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/16] drm/fb-helper: unexport drm_fb_helper_single_fb_probe

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Not called by anyone, and really, shouldn't be. Drivers are supposed
> either drm_fb_helper_initial_config or drm_fb_helper_hotplug_event.
> Originally this was done differently, but is now consolidated in the
> helper functions and no longer done by drivers directly.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c |    5 ++---
>  include/drm/drm_fb_helper.h     |    3 ---
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4549512..2377fef 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -754,8 +754,8 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> -int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> -                                 int preferred_bpp)
> +static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> +                                        int preferred_bpp)
>  {
>         int new_fb = 0;
>         int crtc_count = 0;
> @@ -870,7 +870,6 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>
>         return 0;
>  }
> -EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
>
>  void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>                             uint32_t depth)
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 4e989dc..62f8848 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -82,9 +82,6 @@ struct drm_fb_helper {
>         bool delayed_hotplug;
>  };
>
> -int drm_fb_helper_single_fb_probe(struct drm_fb_helper *helper,
> -                                 int preferred_bpp);
> -
>  int drm_fb_helper_init(struct drm_device *dev,
>                        struct drm_fb_helper *helper, int crtc_count,
>                        int max_conn);
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/16] drm/fb-helper: fixup up set_config semantics

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> While doing the modeset rework for drm/i915 I've noticed that the fb
> helper is very liberal with the semantics of the ->set_config
> interface:
> - It doesn't bother clearing stale modes (e.g. when unpluggint a

s/unpluggint/unplugging/

>   screen).
> - It unconditionally sets and the fb, even if no mode will be set on a

s/and the fb/the fb/ ?

>   given crtc.
> - The initial setup is a bit fun since we need to pick crtcs to decide
>   the desired fb size, but also should set the modeset->fb pointer.
>   Explain what's going on in the fixup code after the fb is allocated.
>
> The crtc helper didn't really care, but the new i915 modeset
> infrastructure did, so I've had to add a bunch of special-cases to
> catch this.
>
> Fix this all up and enforce the interface by converting the checks in
> drm/i915/intel_display.c to BUG_ONs.
>

maybe nitpicking, but how about a matching set of BUG_ONs in crtc
helper, so someone messing with this code with a driver that still
uses crtc helper would catch the same issues?

Anyways, basically seems reasonable.

But thanks for making me look at code that so far I've mostly tried to
avoid looking at :-P

It does seem like the initial-config code could somehow be simpler than it is..

Anyways, while looking at that, it occurs to me that we should remove
mode, and saved_fb from 'struct drm_fb_helper'

BR,
-R

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c      |   27 +++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_display.c |   11 +++--------
>  2 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index dbf0020..5c73a12 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>         struct drm_fb_helper *fb_helper = info->par;
>         struct drm_device *dev = fb_helper->dev;
>         struct fb_var_screeninfo *var = &info->var;
> -       struct drm_crtc *crtc;
>         int ret;
>         int i;
>
> @@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>
>         drm_modeset_lock_all(dev);
>         for (i = 0; i < fb_helper->crtc_count; i++) {
> -               crtc = fb_helper->crtc_info[i].mode_set.crtc;
>                 ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
>                 if (ret) {
>                         drm_modeset_unlock_all(dev);
> @@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>
>         info = fb_helper->fbdev;
>
> -       /* set the fb pointer */
> +       /*
> +        * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
> +        * events, but at init time drm_setup_crtcs needs to be called before
> +        * the fb is allocated (since we need to figure out the desired size of
> +        * the fb before we can allocate it ...). Hence we need to fix things up
> +        * here again.
> +        */
>         for (i = 0; i < fb_helper->crtc_count; i++)
> -               fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +               if (fb_helper->crtc_info[i].mode_set.num_connectors)
> +                       fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +
>
>         if (new_fb) {
>                 info->var.pixclock = 0;
> @@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>         for (i = 0; i < fb_helper->crtc_count; i++) {
>                 modeset = &fb_helper->crtc_info[i].mode_set;
>                 modeset->num_connectors = 0;
> +               modeset->fb = NULL;
>         }
>
>         for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>                         modeset->mode = drm_mode_duplicate(dev,
>                                                            fb_crtc->desired_mode);
>                         modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
> +                       modeset->fb = fb_helper->fb;
>                 }
>         }
>
> +       /* Clear out any old modes if there are no more connected outputs. */
> +       for (i = 0; i < fb_helper->crtc_count; i++) {
> +               modeset = &fb_helper->crtc_info[i].mode_set;
> +               if (modeset->num_connectors == 0) {
> +                       BUG_ON(modeset->fb);
> +                       BUG_ON(modeset->num_connectors);
> +                       if (modeset->mode)
> +                               drm_mode_destroy(dev, modeset->mode);
> +                       modeset->mode = NULL;
> +               }
> +       }
>  out:
>         kfree(crtcs);
>         kfree(modes);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 055b24a..7e3dd0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7940,14 +7940,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>         BUG_ON(!set->crtc);
>         BUG_ON(!set->crtc->helper_private);
>
> -       if (!set->mode)
> -               set->fb = NULL;
> -
> -       /* The fb helper likes to play gross jokes with ->mode_set_config.
> -        * Unfortunately the crtc helper doesn't do much at all for this case,
> -        * so we have to cope with this madness until the fb helper is fixed up. */
> -       if (set->fb && set->num_connectors == 0)
> -               return 0;
> +       /* Enforce sane interface api - has been abused by the fb helper. */
> +       BUG_ON(!set->mode && set->fb);
> +       BUG_ON(set->fb && set->num_connectors == 0);
>
>         if (set->fb) {
>                 DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/16] drm/fb-helper: directly call set_par from the hotplug handler

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The idea behind calling down into the driver's ->fb_probe function on each
> hotplug seems to be able to reallocate the backing storage (if e.g. a screen
> with higher resolution gets added). But that requires quite a bit of work in the
> fb helper itself, since currently we limit new screens to the currently
> allocated fb. An no kms driver supports fbdev fb resizing.

does an non kms fbdev driver support resizing?  That might be a better
argument ;-)

w/ framebuffer vaddr all over the place, teaching fbdev to resize
seems like it might be kinda fun.. I guess if we really wanted to
resize fbdev then we'd be better off beating all the console_lock vs
notifier vs whatever else deadlocks so that we could reliably
unregister and re-register a framebuffer on last-unplug / first-plug.

Reviewed-by: Rob Clark <robdclark@gmail.com>

> So don't bother and start to simplify the code by calling drm_fb_helper_set_par
> directly from the fbdev hotplug function, since that's the only thing left in
> drm_fb_helper_single_fb_probe which does not concern itself with fb allocation
> and initial setup. Follow-on patches will streamline the initial setup
> code.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 5c73a12..90c1117 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -859,8 +859,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                 dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
>                                 info->node, info->fix.id);
>
> -       } else {
> -               drm_fb_helper_set_par(info);
>         }
>
>         /* Switch back to kernel console on panic */
> @@ -1436,7 +1434,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>         drm_setup_crtcs(fb_helper);
>         drm_modeset_unlock_all(dev);
>
> -       return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +       drm_fb_helper_set_par(fb_helper->fbdev);
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/fb-helper: fixup set_config semantics

[Daniel Vetter]

While doing the modeset rework for drm/i915 I've noticed that the fb
helper is very liberal with the semantics of the ->set_config
interface:
- It doesn't bother clearing stale modes (e.g. when unplugging a
  screen).
- It unconditionally sets the fb, even if no mode will be set on a
  given crtc.
- The initial setup is a bit fun since we need to pick crtcs to decide
  the desired fb size, but also should set the modeset->fb pointer.
  Explain what's going on in the fixup code after the fb is allocated.

The crtc helper didn't really care, but the new i915 modeset
infrastructure did, so I've had to add a bunch of special-cases to
catch this.

Fix this all up and enforce the interface by converting the checks in
drm/i915/intel_display.c to BUG_ONs.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c      |   27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |   11 +++--------
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d841b68..809ef99 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct fb_var_screeninfo *var = &info->var;
-	struct drm_crtc *crtc;
 	int ret;
 	int i;
 
@@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 
 	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 		ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
 		if (ret) {
 			drm_modeset_unlock_all(dev);
@@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 	info = fb_helper->fbdev;
 
-	/* set the fb pointer */
+	/*
+	 * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
+	 * events, but at init time drm_setup_crtcs needs to be called before
+	 * the fb is allocated (since we need to figure out the desired size of
+	 * the fb before we can allocate it ...). Hence we need to fix things up
+	 * here again.
+	 */
 	for (i = 0; i < fb_helper->crtc_count; i++)
-		fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+		if (fb_helper->crtc_info[i].mode_set.num_connectors)
+			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+
 
 	if (new_fb) {
 		info->var.pixclock = 0;
@@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		modeset = &fb_helper->crtc_info[i].mode_set;
 		modeset->num_connectors = 0;
+		modeset->fb = NULL;
 	}
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
 			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			modeset->fb = fb_helper->fb;
 		}
 	}
 
+	/* Clear out any old modes if there are no more connected outputs. */
+	for (i = 0; i < fb_helper->crtc_count; i++) {
+		modeset = &fb_helper->crtc_info[i].mode_set;
+		if (modeset->num_connectors == 0) {
+			BUG_ON(modeset->fb);
+			BUG_ON(modeset->num_connectors);
+			if (modeset->mode)
+				drm_mode_destroy(dev, modeset->mode);
+			modeset->mode = NULL;
+		}
+	}
 out:
 	kfree(crtcs);
 	kfree(modes);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24f2654..ca8d592 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7978,14 +7978,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	BUG_ON(!set->crtc);
 	BUG_ON(!set->crtc->helper_private);
 
-	if (!set->mode)
-		set->fb = NULL;
-
-	/* The fb helper likes to play gross jokes with ->mode_set_config.
-	 * Unfortunately the crtc helper doesn't do much at all for this case,
-	 * so we have to cope with this madness until the fb helper is fixed up. */
-	if (set->fb && set->num_connectors == 0)
-		return 0;
+	/* Enforce sane interface api - has been abused by the fb helper. */
+	BUG_ON(!set->mode && set->fb);
+	BUG_ON(set->fb && set->num_connectors == 0);
 
 	if (set->fb) {
 		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-- 
1.7.10.4

Re: [PATCH 13/16] drm/fb-helper: streamline drm_fb_helper_single_fb_probe

[Rob Clark]

On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> No need to check whether we've allocated a new fb since we're not
> always doing that. Also, we always need to register the fbdev and add
> it to the panic notifier.

hmm, how is this not leading to a register_framebuffer() on every hotplug event?

BR,
-R


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c |   29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 90c1117..edbfcde 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -752,10 +752,14 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> +/*
> + * Allocates the backing storage through the ->fb_probe callback and then
> + * registers the fbdev and sets up the panic notifier.
> + */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                                          int preferred_bpp)
>  {
> -       int new_fb = 0;
> +       int ret = 0;
>         int crtc_count = 0;
>         int i;
>         struct fb_info *info;
> @@ -833,9 +837,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>         }
>
>         /* push down into drivers */
> -       new_fb = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
> -       if (new_fb < 0)
> -               return new_fb;
> +       ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
> +       if (ret < 0)
> +               return ret;
>
>         info = fb_helper->fbdev;
>
> @@ -851,15 +855,12 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                         fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
>
>
> -       if (new_fb) {
> -               info->var.pixclock = 0;
> -               if (register_framebuffer(info) < 0)
> -                       return -EINVAL;
> -
> -               dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> -                               info->node, info->fix.id);
> +       info->var.pixclock = 0;
> +       if (register_framebuffer(info) < 0)
> +               return -EINVAL;
>
> -       }
> +       dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> +                       info->node, info->fix.id);
>
>         /* Switch back to kernel console on panic */
>         /* multi card linked list maybe */
> @@ -869,8 +870,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                                                &paniced);
>                 register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>         }
> -       if (new_fb)
> -               list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +
> +       list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
>
>         return 0;
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/fb-helper: remove unused members of struct drm_fb_helper

[Daniel Vetter]

Spotted by Rob Clark.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drm_fb_helper.h |    2 --
 1 file changed, 2 deletions(-)

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index a60311c..8ef4c63 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -77,9 +77,7 @@ struct drm_fb_helper_connector {
 
 struct drm_fb_helper {
 	struct drm_framebuffer *fb;
-	struct drm_framebuffer *saved_fb;
 	struct drm_device *dev;
-	struct drm_display_mode *mode;
 	int crtc_count;
 	struct drm_fb_helper_crtc *crtc_info;
 	int connector_count;
-- 
1.7.10.4

[PATCH] drm/fb-helper: fixup set_config semantics

[Daniel Vetter]

While doing the modeset rework for drm/i915 I've noticed that the fb
helper is very liberal with the semantics of the ->set_config
interface:
- It doesn't bother clearing stale modes (e.g. when unplugging a
  screen).
- It unconditionally sets the fb, even if no mode will be set on a
  given crtc.
- The initial setup is a bit fun since we need to pick crtcs to decide
  the desired fb size, but also should set the modeset->fb pointer.
  Explain what's going on in the fixup code after the fb is allocated.

The crtc helper didn't really care, but the new i915 modeset
infrastructure did, so I've had to add a bunch of special-cases to
catch this.

Fix this all up and enforce the interface by converting the checks in
drm/i915/intel_display.c to BUG_ONs.

v2: Fix commit message spell fail spotted by Rob Clark.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c      |   27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |   11 +++--------
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d841b68..809ef99 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct fb_var_screeninfo *var = &info->var;
-	struct drm_crtc *crtc;
 	int ret;
 	int i;
 
@@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
 
 	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 		ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
 		if (ret) {
 			drm_modeset_unlock_all(dev);
@@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 	info = fb_helper->fbdev;
 
-	/* set the fb pointer */
+	/*
+	 * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
+	 * events, but at init time drm_setup_crtcs needs to be called before
+	 * the fb is allocated (since we need to figure out the desired size of
+	 * the fb before we can allocate it ...). Hence we need to fix things up
+	 * here again.
+	 */
 	for (i = 0; i < fb_helper->crtc_count; i++)
-		fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+		if (fb_helper->crtc_info[i].mode_set.num_connectors)
+			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
+
 
 	if (new_fb) {
 		info->var.pixclock = 0;
@@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		modeset = &fb_helper->crtc_info[i].mode_set;
 		modeset->num_connectors = 0;
+		modeset->fb = NULL;
 	}
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
 			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			modeset->fb = fb_helper->fb;
 		}
 	}
 
+	/* Clear out any old modes if there are no more connected outputs. */
+	for (i = 0; i < fb_helper->crtc_count; i++) {
+		modeset = &fb_helper->crtc_info[i].mode_set;
+		if (modeset->num_connectors == 0) {
+			BUG_ON(modeset->fb);
+			BUG_ON(modeset->num_connectors);
+			if (modeset->mode)
+				drm_mode_destroy(dev, modeset->mode);
+			modeset->mode = NULL;
+		}
+	}
 out:
 	kfree(crtcs);
 	kfree(modes);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24f2654..ca8d592 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7978,14 +7978,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	BUG_ON(!set->crtc);
 	BUG_ON(!set->crtc->helper_private);
 
-	if (!set->mode)
-		set->fb = NULL;
-
-	/* The fb helper likes to play gross jokes with ->mode_set_config.
-	 * Unfortunately the crtc helper doesn't do much at all for this case,
-	 * so we have to cope with this madness until the fb helper is fixed up. */
-	if (set->fb && set->num_connectors == 0)
-		return 0;
+	/* Enforce sane interface api - has been abused by the fb helper. */
+	BUG_ON(!set->mode && set->fb);
+	BUG_ON(set->fb && set->num_connectors == 0);
 
 	if (set->fb) {
 		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-- 
1.7.10.4

Re: [PATCH 13/16] drm/fb-helper: streamline drm_fb_helper_single_fb_probe

[Daniel Vetter]

On Tue, Feb 12, 2013 at 1:24 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> No need to check whether we've allocated a new fb since we're not
>> always doing that. Also, we always need to register the fbdev and add
>> it to the panic notifier.
>
> hmm, how is this not leading to a register_framebuffer() on every hotplug event?

drm_fb_helper_hotplug_event calls drm_fb_helper_set_par directly, so
bypasses all the initial setup which I'm streamlining here. See
"drm/fb-helper: directly call set_par from the hotplug handler".
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: fixup set_config semantics

[Rob Clark]

On Mon, Feb 11, 2013 at 7:25 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> While doing the modeset rework for drm/i915 I've noticed that the fb
> helper is very liberal with the semantics of the ->set_config
> interface:
> - It doesn't bother clearing stale modes (e.g. when unplugging a
>   screen).
> - It unconditionally sets the fb, even if no mode will be set on a
>   given crtc.
> - The initial setup is a bit fun since we need to pick crtcs to decide
>   the desired fb size, but also should set the modeset->fb pointer.
>   Explain what's going on in the fixup code after the fb is allocated.
>
> The crtc helper didn't really care, but the new i915 modeset
> infrastructure did, so I've had to add a bunch of special-cases to
> catch this.
>
> Fix this all up and enforce the interface by converting the checks in
> drm/i915/intel_display.c to BUG_ONs.
>
> v2: Fix commit message spell fail spotted by Rob Clark.

Reviewed-by: Rob Clark <robdclark@gmail.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c      |   27 +++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_display.c |   11 +++--------
>  2 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d841b68..809ef99 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>         struct drm_fb_helper *fb_helper = info->par;
>         struct drm_device *dev = fb_helper->dev;
>         struct fb_var_screeninfo *var = &info->var;
> -       struct drm_crtc *crtc;
>         int ret;
>         int i;
>
> @@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>
>         drm_modeset_lock_all(dev);
>         for (i = 0; i < fb_helper->crtc_count; i++) {
> -               crtc = fb_helper->crtc_info[i].mode_set.crtc;
>                 ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
>                 if (ret) {
>                         drm_modeset_unlock_all(dev);
> @@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>
>         info = fb_helper->fbdev;
>
> -       /* set the fb pointer */
> +       /*
> +        * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
> +        * events, but at init time drm_setup_crtcs needs to be called before
> +        * the fb is allocated (since we need to figure out the desired size of
> +        * the fb before we can allocate it ...). Hence we need to fix things up
> +        * here again.
> +        */
>         for (i = 0; i < fb_helper->crtc_count; i++)
> -               fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +               if (fb_helper->crtc_info[i].mode_set.num_connectors)
> +                       fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +
>
>         if (new_fb) {
>                 info->var.pixclock = 0;
> @@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>         for (i = 0; i < fb_helper->crtc_count; i++) {
>                 modeset = &fb_helper->crtc_info[i].mode_set;
>                 modeset->num_connectors = 0;
> +               modeset->fb = NULL;
>         }
>
>         for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>                         modeset->mode = drm_mode_duplicate(dev,
>                                                            fb_crtc->desired_mode);
>                         modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
> +                       modeset->fb = fb_helper->fb;
>                 }
>         }
>
> +       /* Clear out any old modes if there are no more connected outputs. */
> +       for (i = 0; i < fb_helper->crtc_count; i++) {
> +               modeset = &fb_helper->crtc_info[i].mode_set;
> +               if (modeset->num_connectors == 0) {
> +                       BUG_ON(modeset->fb);
> +                       BUG_ON(modeset->num_connectors);
> +                       if (modeset->mode)
> +                               drm_mode_destroy(dev, modeset->mode);
> +                       modeset->mode = NULL;
> +               }
> +       }
>  out:
>         kfree(crtcs);
>         kfree(modes);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 24f2654..ca8d592 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7978,14 +7978,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>         BUG_ON(!set->crtc);
>         BUG_ON(!set->crtc->helper_private);
>
> -       if (!set->mode)
> -               set->fb = NULL;
> -
> -       /* The fb helper likes to play gross jokes with ->mode_set_config.
> -        * Unfortunately the crtc helper doesn't do much at all for this case,
> -        * so we have to cope with this madness until the fb helper is fixed up. */
> -       if (set->fb && set->num_connectors == 0)
> -               return 0;
> +       /* Enforce sane interface api - has been abused by the fb helper. */
> +       BUG_ON(!set->mode && set->fb);
> +       BUG_ON(set->fb && set->num_connectors == 0);
>
>         if (set->fb) {
>                 DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
> --
> 1.7.10.4
>

Re: [PATCH 13/16] drm/fb-helper: streamline drm_fb_helper_single_fb_probe

[Rob Clark]

On Mon, Feb 11, 2013 at 7:33 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Feb 12, 2013 at 1:24 AM, Rob Clark <robdclark@gmail.com> wrote:
>> On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> No need to check whether we've allocated a new fb since we're not
>>> always doing that. Also, we always need to register the fbdev and add
>>> it to the panic notifier.
>>
>> hmm, how is this not leading to a register_framebuffer() on every hotplug event?
>
> drm_fb_helper_hotplug_event calls drm_fb_helper_set_par directly, so
> bypasses all the initial setup which I'm streamlining here. See
> "drm/fb-helper: directly call set_par from the hotplug handler".

doh, that's the problem with reviewing long patchset without applying
them as I go..

ok, then looks good

Reviewed-by: Rob Clark <robdclark@gmail.com>

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: don't disable everything in initial_config

[Rob Clark]

On Sun, Jan 27, 2013 at 11:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This should be done in the drivers for two reasons:
> - it gets in the way of fastboot efforts
> - it links the fb helpers with the crtc helpers instead of going
>   through the real interface vfuncs, forcing i915 to fake all the
>   ->disable callbacks used by the crtc helper to avoid ugly Oopsen
>
> v2: Resolve conflicts since drivers still call
> drm_fb_helper_single_add_all_connectors.
>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/ast/ast_fb.c              |    5 +++++
>  drivers/gpu/drm/cirrus/cirrus_fbdev.c     |    4 ++++
>  drivers/gpu/drm/drm_fb_cma_helper.c       |    3 +++
>  drivers/gpu/drm/drm_fb_helper.c           |    3 ---
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    3 +++
>  drivers/gpu/drm/gma500/framebuffer.c      |    4 ++++
>  drivers/gpu/drm/i915/intel_fb.c           |    3 +++
>  drivers/gpu/drm/mgag200/mgag200_fb.c      |    5 +++++
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    3 +++
>  drivers/gpu/drm/radeon/radeon_fb.c        |    4 ++++
>  drivers/gpu/drm/udl/udl_fb.c              |    4 ++++
>  drivers/staging/omapdrm/omap_fbdev.c      |    4 ++++
>  12 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> index 3e6584b..81763ca 100644
> --- a/drivers/gpu/drm/ast/ast_fb.c
> +++ b/drivers/gpu/drm/ast/ast_fb.c
> @@ -40,6 +40,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_crtc_helper.h>
>  #include "ast_drv.h"
>
>  static void ast_dirty_update(struct ast_fbdev *afbdev,
> @@ -314,6 +315,10 @@ int ast_fbdev_init(struct drm_device *dev)
>         }
>
>         drm_fb_helper_single_add_all_connectors(&afbdev->helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         drm_fb_helper_initial_config(&afbdev->helper, 32);
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> index 3daea0f..b96605c 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_crtc_helper.h>
>
>  #include <linux/fb.h>
>
> @@ -291,6 +292,9 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
>                 return ret;
>         }
>         drm_fb_helper_single_add_all_connectors(&gfbdev->helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(cdev->dev);
>         drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
>
>         return 0;
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 1b6ba2d..ef3d33a 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -333,6 +333,9 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
>
>         }
>
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         ret = drm_fb_helper_initial_config(helper, preferred_bpp);
>         if (ret < 0) {
>                 dev_err(dev->dev, "Failed to set inital hw configuration.\n");
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3132d8a..f188344 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1357,9 +1357,6 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>         struct drm_device *dev = fb_helper->dev;
>         int count = 0;
>
> -       /* disable all the possible outputs/crtcs before entering KMS mode */
> -       drm_helper_disable_unused_functions(fb_helper->dev);
> -
>         drm_fb_helper_parse_command_line(fb_helper);
>
>         count = drm_fb_helper_probe_connector_modes(fb_helper,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 086d0f7..fe2a0f0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -295,6 +295,9 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>
>         }
>
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
>         if (ret < 0) {
>                 DRM_ERROR("failed to set up hw configuration.\n");
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index c1ef37e..fee3bf8 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -616,6 +616,10 @@ int psb_fbdev_init(struct drm_device *dev)
>                                                         INTELFB_CONN_LIMIT);
>
>         drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 33c4f1b..69b5b3c 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -258,6 +258,9 @@ void intel_fbdev_initial_config(struct drm_device *dev)
>  {
>         drm_i915_private_t *dev_priv = dev->dev_private;
>
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         /* Due to peculiar init order wrt to hpd handling this is separate. */
>         drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
>  }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
> index 5c69b43..5bded5b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_fb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_crtc_helper.h>
>
>  #include <linux/fb.h>
>
> @@ -278,6 +279,10 @@ int mgag200_fbdev_init(struct mga_device *mdev)
>                 return ret;
>         }
>         drm_fb_helper_single_add_all_connectors(&mfbdev->helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(mdev->dev);
> +
>         drm_fb_helper_initial_config(&mfbdev->helper, 32);
>
>         return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index d4ecb4d..b1ebfe3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -491,6 +491,9 @@ nouveau_fbcon_init(struct drm_device *dev)
>         else
>                 preferred_bpp = 32;
>
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         drm_fb_helper_initial_config(&fbcon->helper, preferred_bpp);
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> index 515e5ee..b48d1c8 100644
> --- a/drivers/gpu/drm/radeon/radeon_fb.c
> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> @@ -379,6 +379,10 @@ int radeon_fbdev_init(struct radeon_device *rdev)
>         }
>
>         drm_fb_helper_single_add_all_connectors(&rfbdev->helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(rdev->ddev);
> +
>         drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel);
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index caa84f1..b610c3c 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -583,6 +583,10 @@ int udl_fbdev_init(struct drm_device *dev)
>         }
>
>         drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
>         return 0;
>  }
> diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
> index 2728e37..7e66eb1 100644
> --- a/drivers/staging/omapdrm/omap_fbdev.c
> +++ b/drivers/staging/omapdrm/omap_fbdev.c
> @@ -369,6 +369,10 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
>         }
>
>         drm_fb_helper_single_add_all_connectors(helper);
> +
> +       /* disable all the possible outputs/crtcs before entering KMS mode */
> +       drm_helper_disable_unused_functions(dev);
> +
>         drm_fb_helper_initial_config(helper, 32);
>
>         priv->fbdev = helper;
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/16] drm/<drivers>: simplify ->fb_probe callback

[Rob Clark]

On Thu, Jan 24, 2013 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The fb helper lost its support for reallocating an fb completely, so
> no need to return special success values any more.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/ast/ast_fb.c              |   21 +++-----------------
>  drivers/gpu/drm/cirrus/cirrus_fbdev.c     |   22 +++------------------
>  drivers/gpu/drm/drm_fb_cma_helper.c       |   17 +---------------
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   30 +----------------------------
>  drivers/gpu/drm/gma500/framebuffer.c      |   10 +---------
>  drivers/gpu/drm/i915/intel_fb.c           |   21 +++-----------------
>  drivers/gpu/drm/mgag200/mgag200_fb.c      |   22 +++------------------
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   22 +++------------------
>  drivers/gpu/drm/radeon/radeon_fb.c        |   21 +++-----------------
>  drivers/gpu/drm/udl/udl_fb.c              |   22 +++------------------
>  drivers/staging/omapdrm/omap_fbdev.c      |   17 +---------------
>  11 files changed, 25 insertions(+), 200 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> index 9bfd5ab..3a5a5e0 100644
> --- a/drivers/gpu/drm/ast/ast_fb.c
> +++ b/drivers/gpu/drm/ast/ast_fb.c
> @@ -146,9 +146,10 @@ static int astfb_create_object(struct ast_fbdev *afbdev,
>         return ret;
>  }
>
> -static int astfb_create(struct ast_fbdev *afbdev,
> +static int astfb_create(struct drm_fb_helper *helper,
>                         struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct ast_fbdev *afbdev = (struct ast_fbdev *)helper;
>         struct drm_device *dev = afbdev->helper.dev;
>         struct drm_mode_fb_cmd2 mode_cmd;
>         struct drm_framebuffer *fb;
> @@ -249,26 +250,10 @@ static void ast_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
>         *blue = ast_crtc->lut_b[regno] << 8;
>  }
>
> -static int ast_find_or_create_single(struct drm_fb_helper *helper,
> -                                         struct drm_fb_helper_surface_size *sizes)
> -{
> -       struct ast_fbdev *afbdev = (struct ast_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = astfb_create(afbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static struct drm_fb_helper_funcs ast_fb_helper_funcs = {
>         .gamma_set = ast_fb_gamma_set,
>         .gamma_get = ast_fb_gamma_get,
> -       .fb_probe = ast_find_or_create_single,
> +       .fb_probe = astfb_create,
>  };
>
>  static void ast_fbdev_destroy(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> index b869b8b..778a90b 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> @@ -121,9 +121,10 @@ static int cirrusfb_create_object(struct cirrus_fbdev *afbdev,
>         return ret;
>  }
>
> -static int cirrusfb_create(struct cirrus_fbdev *gfbdev,
> +static int cirrusfb_create(struct drm_fb_helper *helper,
>                            struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct cirrus_fbdev *gfbdev = (struct cirrus_fbdev *)helper;
>         struct drm_device *dev = gfbdev->helper.dev;
>         struct cirrus_device *cdev = gfbdev->helper.dev->dev_private;
>         struct fb_info *info;
> @@ -220,23 +221,6 @@ out_iounmap:
>         return ret;
>  }
>
> -static int cirrus_fb_find_or_create_single(struct drm_fb_helper *helper,
> -                                          struct drm_fb_helper_surface_size
> -                                          *sizes)
> -{
> -       struct cirrus_fbdev *gfbdev = (struct cirrus_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = cirrusfb_create(gfbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static int cirrus_fbdev_destroy(struct drm_device *dev,
>                                 struct cirrus_fbdev *gfbdev)
>  {
> @@ -268,7 +252,7 @@ static int cirrus_fbdev_destroy(struct drm_device *dev,
>  static struct drm_fb_helper_funcs cirrus_fb_helper_funcs = {
>         .gamma_set = cirrus_crtc_fb_gamma_set,
>         .gamma_get = cirrus_crtc_fb_gamma_get,
> -       .fb_probe = cirrus_fb_find_or_create_single,
> +       .fb_probe = cirrusfb_create,
>  };
>
>  int cirrus_fbdev_init(struct cirrus_device *cdev)
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index fcf9fa3..69814d2 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -275,23 +275,8 @@ err_drm_gem_cma_free_object:
>         return ret;
>  }
>
> -static int drm_fbdev_cma_probe(struct drm_fb_helper *helper,
> -       struct drm_fb_helper_surface_size *sizes)
> -{
> -       int ret = 0;
> -
> -       if (!helper->fb) {
> -               ret = drm_fbdev_cma_create(helper, sizes);
> -               if (ret < 0)
> -                       return ret;
> -               ret = 1;
> -       }
> -
> -       return ret;
> -}
> -
>  static struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
> -       .fb_probe = drm_fbdev_cma_probe,
> +       .fb_probe = drm_fbdev_cma_create,
>  };
>
>  /**
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 77d78cd..ca70b91 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -226,36 +226,8 @@ out:
>         return ret;
>  }
>
> -static int exynos_drm_fbdev_probe(struct drm_fb_helper *helper,
> -                                  struct drm_fb_helper_surface_size *sizes)
> -{
> -       int ret = 0;
> -
> -       DRM_DEBUG_KMS("%s\n", __FILE__);
> -
> -       /*
> -        * with !helper->fb, it means that this funcion is called first time
> -        * and after that, the helper->fb would be used as clone mode.
> -        */
> -       if (!helper->fb) {
> -               ret = exynos_drm_fbdev_create(helper, sizes);
> -               if (ret < 0) {
> -                       DRM_ERROR("failed to create fbdev.\n");
> -                       return ret;
> -               }
> -
> -               /*
> -                * fb_helper expects a value more than 1 if succeed
> -                * because register_framebuffer() should be called.
> -                */
> -               ret = 1;
> -       }
> -
> -       return ret;
> -}
> -
>  static struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
> -       .fb_probe =     exynos_drm_fbdev_probe,
> +       .fb_probe =     exynos_drm_fbdev_create,
>  };
>
>  int exynos_drm_fbdev_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 7288b6d..f7a458c 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -545,9 +545,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
>         struct psb_fbdev *psb_fbdev = (struct psb_fbdev *)helper;
>         struct drm_device *dev = psb_fbdev->psb_fb_helper.dev;
>         struct drm_psb_private *dev_priv = dev->dev_private;
> -       int new_fb = 0;
>         int bytespp;
> -       int ret;
>
>         bytespp = sizes->surface_bpp / 8;
>         if (bytespp == 3)       /* no 24bit packed */
> @@ -562,13 +560,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
>                  sizes->surface_depth = 16;
>          }
>
> -       if (!helper->fb) {
> -               ret = psbfb_create(psb_fbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> +       return psbfb_create(psb_fbdev, sizes);
>  }
>
>  static struct drm_fb_helper_funcs psb_fb_helper_funcs = {
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 302bc63..3a0d16e 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -57,9 +57,10 @@ static struct fb_ops intelfb_ops = {
>         .fb_debug_leave = drm_fb_helper_debug_leave,
>  };
>
> -static int intelfb_create(struct intel_fbdev *ifbdev,
> +static int intelfb_create(struct drm_fb_helper *helper,
>                           struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
>         struct drm_device *dev = ifbdev->helper.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct fb_info *info;
> @@ -181,26 +182,10 @@ out:
>         return ret;
>  }
>
> -static int intel_fb_find_or_create_single(struct drm_fb_helper *helper,
> -                                         struct drm_fb_helper_surface_size *sizes)
> -{
> -       struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = intelfb_create(ifbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>         .gamma_set = intel_crtc_fb_gamma_set,
>         .gamma_get = intel_crtc_fb_gamma_get,
> -       .fb_probe = intel_fb_find_or_create_single,
> +       .fb_probe = intelfb_create,
>  };
>
>  static void intel_fbdev_destroy(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
> index 674f8fb..247bbcb 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_fb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
> @@ -121,9 +121,10 @@ static int mgag200fb_create_object(struct mga_fbdev *afbdev,
>         return ret;
>  }
>
> -static int mgag200fb_create(struct mga_fbdev *mfbdev,
> +static int mgag200fb_create(struct drm_fb_helper *helper,
>                            struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct mga_fbdev *mfbdev = (struct mga_fbdev *)helper;
>         struct drm_device *dev = mfbdev->helper.dev;
>         struct drm_mode_fb_cmd2 mode_cmd;
>         struct mga_device *mdev = dev->dev_private;
> @@ -210,23 +211,6 @@ out:
>         return ret;
>  }
>
> -static int mga_fb_find_or_create_single(struct drm_fb_helper *helper,
> -                                          struct drm_fb_helper_surface_size
> -                                          *sizes)
> -{
> -       struct mga_fbdev *mfbdev = (struct mga_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = mgag200fb_create(mfbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static int mga_fbdev_destroy(struct drm_device *dev,
>                                 struct mga_fbdev *mfbdev)
>  {
> @@ -257,7 +241,7 @@ static int mga_fbdev_destroy(struct drm_device *dev,
>  static struct drm_fb_helper_funcs mga_fb_helper_funcs = {
>         .gamma_set = mga_crtc_fb_gamma_set,
>         .gamma_get = mga_crtc_fb_gamma_get,
> -       .fb_probe = mga_fb_find_or_create_single,
> +       .fb_probe = mgag200fb_create,
>  };
>
>  int mgag200_fbdev_init(struct mga_device *mdev)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index 7b2d231..d983371 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -251,9 +251,10 @@ nouveau_fbcon_zfill(struct drm_device *dev, struct nouveau_fbdev *fbcon)
>  }
>
>  static int
> -nouveau_fbcon_create(struct nouveau_fbdev *fbcon,
> +nouveau_fbcon_create(struct drm_fb_helper *helper,
>                      struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct nouveau_fbdev *fbcon = (struct nouveau_fbdev *)helper;
>         struct drm_device *dev = fbcon->dev;
>         struct nouveau_drm *drm = nouveau_drm(dev);
>         struct nouveau_device *device = nv_device(drm->device);
> @@ -388,23 +389,6 @@ out:
>         return ret;
>  }
>
> -static int
> -nouveau_fbcon_find_or_create_single(struct drm_fb_helper *helper,
> -                                   struct drm_fb_helper_surface_size *sizes)
> -{
> -       struct nouveau_fbdev *fbcon = (struct nouveau_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = nouveau_fbcon_create(fbcon, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  void
>  nouveau_fbcon_output_poll_changed(struct drm_device *dev)
>  {
> @@ -450,7 +434,7 @@ void nouveau_fbcon_gpu_lockup(struct fb_info *info)
>  static struct drm_fb_helper_funcs nouveau_fbcon_helper_funcs = {
>         .gamma_set = nouveau_fbcon_gamma_set,
>         .gamma_get = nouveau_fbcon_gamma_get,
> -       .fb_probe = nouveau_fbcon_find_or_create_single,
> +       .fb_probe = nouveau_fbcon_create,
>  };
>
>
> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> index a44b386..b6c73b6 100644
> --- a/drivers/gpu/drm/radeon/radeon_fb.c
> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> @@ -187,9 +187,10 @@ out_unref:
>         return ret;
>  }
>
> -static int radeonfb_create(struct radeon_fbdev *rfbdev,
> +static int radeonfb_create(struct drm_fb_helper *helper,
>                            struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct radeon_fbdev *rfbdev = (struct radeon_fbdev *)helper;
>         struct radeon_device *rdev = rfbdev->rdev;
>         struct fb_info *info;
>         struct drm_framebuffer *fb = NULL;
> @@ -300,22 +301,6 @@ out_unref:
>         return ret;
>  }
>
> -static int radeon_fb_find_or_create_single(struct drm_fb_helper *helper,
> -                                          struct drm_fb_helper_surface_size *sizes)
> -{
> -       struct radeon_fbdev *rfbdev = (struct radeon_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = radeonfb_create(rfbdev, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  void radeon_fb_output_poll_changed(struct radeon_device *rdev)
>  {
>         drm_fb_helper_hotplug_event(&rdev->mode_info.rfbdev->helper);
> @@ -349,7 +334,7 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
>  static struct drm_fb_helper_funcs radeon_fb_helper_funcs = {
>         .gamma_set = radeon_crtc_fb_gamma_set,
>         .gamma_get = radeon_crtc_fb_gamma_get,
> -       .fb_probe = radeon_fb_find_or_create_single,
> +       .fb_probe = radeonfb_create,
>  };
>
>  int radeon_fbdev_init(struct radeon_device *rdev)
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 7d40cb7..1b51579 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -440,9 +440,10 @@ udl_framebuffer_init(struct drm_device *dev,
>  }
>
>
> -static int udlfb_create(struct udl_fbdev *ufbdev,
> +static int udlfb_create(struct drm_fb_helper *helper,
>                         struct drm_fb_helper_surface_size *sizes)
>  {
> +       struct udl_fbdev *ufbdev = (struct udl_fbdev *)helper;
>         struct drm_device *dev = ufbdev->helper.dev;
>         struct fb_info *info;
>         struct device *device = &dev->usbdev->dev;
> @@ -520,27 +521,10 @@ out:
>         return ret;
>  }
>
> -static int udl_fb_find_or_create_single(struct drm_fb_helper *helper,
> -                                       struct drm_fb_helper_surface_size *sizes)
> -{
> -       struct udl_fbdev *ufbdev = (struct udl_fbdev *)helper;
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = udlfb_create(ufbdev, sizes);
> -               if (ret)
> -                       return ret;
> -
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static struct drm_fb_helper_funcs udl_fb_helper_funcs = {
>         .gamma_set = udl_crtc_fb_gamma_set,
>         .gamma_get = udl_crtc_fb_gamma_get,
> -       .fb_probe = udl_fb_find_or_create_single,
> +       .fb_probe = udlfb_create,
>  };
>
>  static void udl_fbdev_destroy(struct drm_device *dev,
> diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
> index 6ccaf54..11f7dcb 100644
> --- a/drivers/staging/omapdrm/omap_fbdev.c
> +++ b/drivers/staging/omapdrm/omap_fbdev.c
> @@ -296,25 +296,10 @@ static void omap_crtc_fb_gamma_get(struct drm_crtc *crtc,
>         DBG("fbdev: get gamma");
>  }
>
> -static int omap_fbdev_probe(struct drm_fb_helper *helper,
> -               struct drm_fb_helper_surface_size *sizes)
> -{
> -       int new_fb = 0;
> -       int ret;
> -
> -       if (!helper->fb) {
> -               ret = omap_fbdev_create(helper, sizes);
> -               if (ret)
> -                       return ret;
> -               new_fb = 1;
> -       }
> -       return new_fb;
> -}
> -
>  static struct drm_fb_helper_funcs omap_fb_helper_funcs = {
>         .gamma_set = omap_crtc_fb_gamma_set,
>         .gamma_get = omap_crtc_fb_gamma_get,
> -       .fb_probe = omap_fbdev_probe,
> +       .fb_probe = omap_fbdev_create,
>  };
>
>  static struct drm_fb_helper *get_fb(struct fb_info *fbi)
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: don't sleep for screen unblank when an oopps is in progress

[Rob Clark]

On Sun, Jan 27, 2013 at 12:42 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Otherwise the system will burn even brighter and worse, leave the user
> wondering what's going on exactly.
>
> Since we already have a panic handler which will (try) to restore the
> entire fbdev console mode, we can just bail out. Inspired by a patch
> from Konstantin Khlebnikov. The callchain leading to this, cut&pasted
> from Konstantin's original patch:
>
> callstack:
> panic()
> bust_spinlocks(1)
> unblank_screen()
> vc->vc_sw->con_blank()
> fbcon_blank()
> fb_blank()
> info->fbops->fb_blank()
> drm_fb_helper_blank()
> drm_fb_helper_dpms()
> drm_modeset_lock_all()
> mutex_lock(&dev->mode_config.mutex)
>
> Note that the entire locking in the fb helper around panic/sysrq and
> kdbg is ... non-existant. So we have a decent change of blowing up
> everything. But since reworking this ties in with funny concepts like
> the fbdev notifier chain or the impressive things which happen around
> console_lock while oopsing, I'll leave that as an exercise for braver
> souls than me.
>
> v2: Drop the -EBUSY return value I've copied, we don't need it since
> the we'll take care of things ourselves anyway.
>
> Cc: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> References: https://patchwork.kernel.org/patch/1878181/
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a7538cc..01099b5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -386,6 +386,14 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>         int i, j;
>
>         /*
> +        * fbdev->blank can be called from irq context in case of a panic.
> +        * Since we already have our own special panic handler which will
> +        * restore the fbdev console mode completely, just bail out early.
> +        */
> +       if (oops_in_progress)
> +               return;
> +
> +       /*
>          * For each CRTC in this fb, turn the connectors on/off.
>          */
>         drm_modeset_lock_all(dev);
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel