[PATCH 1/4] drm: Make drm_fb_unregister/remove accept NULL fb

[PATCH 1/4] drm: Make drm_fb_unregister/remove accept NULL fb

[Daniel Vetter]

These functions are used by drivers to release fbdev emulation
buffers. We need to make them resilient to NULL pointers to
make the fbdev compile/runtime knobs not cause Oopses on module
unload.

Cc: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 33d877c65ced..884690c81094 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -538,7 +538,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference);
  */
 void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
 {
-	struct drm_device *dev = fb->dev;
+	struct drm_device *dev;
+
+	if (!fb)
+		return;
+
+	dev = fb->dev;
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	/* Mark fb as reaped and drop idr ref. */
@@ -589,12 +594,17 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
  */
 void drm_framebuffer_remove(struct drm_framebuffer *fb)
 {
-	struct drm_device *dev = fb->dev;
+	struct drm_device *dev;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	struct drm_mode_set set;
 	int ret;
 
+	if (!fb)
+		return;
+
+	dev = fb->dev;
+
 	WARN_ON(!list_empty(&fb->filp_head));
 
 	/*
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/fb-helper: Use -errno return in restore_mode_unlocked

[Daniel Vetter]

Using bool and returning true upon error is very uncommon. Also an int
return value is actually what all the callers which did check it seem
to have expected.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 418d299f3b12..83aacb0cc9df 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
-static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
+static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_plane *plane;
-	bool error = false;
 	int i;
 
 	drm_warn_on_modeset_not_all_locked(dev);
@@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 		if (crtc->funcs->cursor_set) {
 			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
 			if (ret)
-				error = true;
+				return ret;
 		}
 
 		ret = drm_mode_set_config_internal(mode_set);
 		if (ret)
-			error = true;
+			return ret;
 	}
-	return error;
+
+	return 0;
 }
 
 /**
@@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
  * 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.
+ *
+ * RETURNS:
+ * Zero if everything went ok, negative error code otherwise.
  */
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
-	bool ret;
-	bool do_delayed = false;
+	bool do_delayed
+	int ret;
 
 	drm_modeset_lock_all(dev);
 	ret = restore_fbdev_mode(fb_helper);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index dbab4622b58f..67de1f10008e 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info);
 
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
 
 struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
 void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
@@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	return 0;
 }
 
-static inline bool
+static inline int
 drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
-	return true;
+	return 0;
 }
 
 static inline struct fb_info *
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Daniel Vetter]

Faster than recompiling.

Note that restore_fbdev_mode_unlocked is a bit special and the only
one which returns an error code when fbdev isn't there - i915 needs
that one to not fall over with some additional fbcon related restore
code. Everyone else just ignores the return value or only prints a
DRM_DEBUG level message.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 83aacb0cc9df..8259dec1de1f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -39,6 +39,11 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 
+static bool drm_fbdev_emulation = true;
+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
+MODULE_PARM_DESC(fbdev_emulation,
+		 "Enable legacy fbdev emulation [default=true]");
+
 static LIST_HEAD(kernel_fb_helper_list);
 
 /**
@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	struct drm_connector *connector;
 	int i;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	mutex_lock(&dev->mode_config.mutex);
 	drm_for_each_connector(connector, dev) {
 		struct drm_fb_helper_connector *fb_helper_connector;
@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 	struct drm_fb_helper_connector **temp;
 	struct drm_fb_helper_connector *fb_helper_connector;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
 	if (fb_helper->connector_count + 1 > fb_helper->connector_info_alloc_count) {
 		temp = krealloc(fb_helper->connector_info, sizeof(struct drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	struct drm_fb_helper_connector *fb_helper_connector;
 	int i, j;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -375,6 +389,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	bool do_delayed
 	int ret;
 
+	if (!drm_fbdev_emulation)
+		return -ENODEV;
+
 	drm_modeset_lock_all(dev);
 	ret = restore_fbdev_mode(fb_helper);
 
@@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int i;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	if (!max_conn_count)
 		return -EINVAL;
 
@@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
 
 void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 {
+	if (!drm_fbdev_emulation)
+		return;
+
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
@@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	struct drm_device *dev = fb_helper->dev;
 	int count = 0;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	mutex_lock(&dev->mode_config.mutex);
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
 						    dev->mode_config.max_width,
@@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	struct drm_device *dev = fb_helper->dev;
 	u32 max_width, max_height;
 
+	if (!drm_fbdev_emulation)
+		return 0;
+
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d8883ede..4e73b6f6b1c0 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	return 0;
 }
 
+static bool lockless_register_fb;
+module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400);
+MODULE_PARM_DESC(lockless_register_fb,
+	"Lockless framebuffer registration for debugging [default=off]");
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i, ret;
@@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	registered_fb[i] = fb_info;
 
 	event.info = fb_info;
-	console_lock();
+	if (!lockless_register_fb)
+		console_lock();
 	if (!lock_fb_info(fb_info)) {
-		console_unlock();
+		if (!lockless_register_fb)
+			console_unlock();
 		return -ENODEV;
 	}
 
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
 	unlock_fb_info(fb_info);
-	console_unlock();
+	if (!lockless_register_fb)
+		console_unlock();
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d8883ede..4e73b6f6b1c0 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	return 0;
 }
 
+static bool lockless_register_fb;
+module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400);
+MODULE_PARM_DESC(lockless_register_fb,
+	"Lockless framebuffer registration for debugging [default=off]");
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i, ret;
@@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	registered_fb[i] = fb_info;
 
 	event.info = fb_info;
-	console_lock();
+	if (!lockless_register_fb)
+		console_lock();
 	if (!lock_fb_info(fb_info)) {
-		console_unlock();
+		if (!lockless_register_fb)
+			console_unlock();
 		return -ENODEV;
 	}
 
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
 	unlock_fb_info(fb_info);
-	console_unlock();
+	if (!lockless_register_fb)
+		console_unlock();
 	return 0;
 }
 
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked

[Daniel Vetter]

Using bool and returning true upon error is very uncommon. Also an int
return value is actually what all the callers which did check it seem
to have expected.

v2: Restore hunk misplaced in a rebase, spotted by Rob.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++--------
 include/drm/drm_fb_helper.h     |  6 +++---
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 418d299f3b12..859134e0d72d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
-static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
+static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_plane *plane;
-	bool error = false;
 	int i;
 
 	drm_warn_on_modeset_not_all_locked(dev);
@@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 		if (crtc->funcs->cursor_set) {
 			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
 			if (ret)
-				error = true;
+				return ret;
 		}
 
 		ret = drm_mode_set_config_internal(mode_set);
 		if (ret)
-			error = true;
+			return ret;
 	}
-	return error;
+
+	return 0;
 }
 
 /**
@@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
  * 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.
+ *
+ * RETURNS:
+ * Zero if everything went ok, negative error code otherwise.
  */
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
-	bool ret;
-	bool do_delayed = false;
+	bool do_delayed;
+	int ret;
 
 	drm_modeset_lock_all(dev);
 	ret = restore_fbdev_mode(fb_helper);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index dbab4622b58f..67de1f10008e 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info);
 
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
 
 struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
 void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
@@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	return 0;
 }
 
-static inline bool
+static inline int
 drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
-	return true;
+	return 0;
 }
 
 static inline struct fb_info *
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm: Make drm_fb_unregister/remove accept NULL fb

[Rob Clark]

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> These functions are used by drivers to release fbdev emulation
> buffers. We need to make them resilient to NULL pointers to
> make the fbdev compile/runtime knobs not cause Oopses on module
> unload.
>
> Cc: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 33d877c65ced..884690c81094 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -538,7 +538,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference);
>   */
>  void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
>  {
> -       struct drm_device *dev = fb->dev;
> +       struct drm_device *dev;
> +
> +       if (!fb)
> +               return;
> +
> +       dev = fb->dev;
>
>         mutex_lock(&dev->mode_config.fb_lock);
>         /* Mark fb as reaped and drop idr ref. */
> @@ -589,12 +594,17 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
>   */
>  void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  {
> -       struct drm_device *dev = fb->dev;
> +       struct drm_device *dev;
>         struct drm_crtc *crtc;
>         struct drm_plane *plane;
>         struct drm_mode_set set;
>         int ret;
>
> +       if (!fb)
> +               return;
> +
> +       dev = fb->dev;
> +
>         WARN_ON(!list_empty(&fb->filp_head));
>
>         /*
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked

[Rob Clark]

On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Using bool and returning true upon error is very uncommon. Also an int
> return value is actually what all the callers which did check it seem
> to have expected.
>
> v2: Restore hunk misplaced in a rebase, spotted by Rob.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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


> ---
>  drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++--------
>  include/drm/drm_fb_helper.h     |  6 +++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 418d299f3b12..859134e0d72d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
>         struct drm_device *dev = fb_helper->dev;
>         struct drm_plane *plane;
> -       bool error = false;
>         int i;
>
>         drm_warn_on_modeset_not_all_locked(dev);
> @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>                 if (crtc->funcs->cursor_set) {
>                         ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
>                         if (ret)
> -                               error = true;
> +                               return ret;
>                 }
>
>                 ret = drm_mode_set_config_internal(mode_set);
>                 if (ret)
> -                       error = true;
> +                       return ret;
>         }
> -       return error;
> +
> +       return 0;
>  }
>
>  /**
> @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>   * 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.
> + *
> + * RETURNS:
> + * Zero if everything went ok, negative error code otherwise.
>   */
> -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  {
>         struct drm_device *dev = fb_helper->dev;
> -       bool ret;
> -       bool do_delayed = false;
> +       bool do_delayed;
> +       int ret;
>
>         drm_modeset_lock_all(dev);
>         ret = restore_fbdev_mode(fb_helper);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index dbab4622b58f..67de1f10008e 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
>  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>                             struct fb_info *info);
>
> -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
>
>  struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
>  void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
> @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>         return 0;
>  }
>
> -static inline bool
> +static inline int
>  drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  {
> -       return true;
> +       return 0;
>  }
>
>  static inline struct fb_info *
> --
> 1.8.3.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
>   ->fb notifier chain calls into fbcon
>     ->fbcon sets up console on new fbi
>       ->fbi->set_par
>         ->drm_fb_helper_set_par exercises full kms api
>
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
>
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.
>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

BR,
-R

> ---
>  drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 0705d8883ede..4e73b6f6b1c0 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
>         return 0;
>  }
>
> +static bool lockless_register_fb;
> +module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400);
> +MODULE_PARM_DESC(lockless_register_fb,
> +       "Lockless framebuffer registration for debugging [default=off]");
> +
>  static int do_register_framebuffer(struct fb_info *fb_info)
>  {
>         int i, ret;
> @@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
>         registered_fb[i] = fb_info;
>
>         event.info = fb_info;
> -       console_lock();
> +       if (!lockless_register_fb)
> +               console_lock();
>         if (!lock_fb_info(fb_info)) {
> -               console_unlock();
> +               if (!lockless_register_fb)
> +                       console_unlock();
>                 return -ENODEV;
>         }
>
>         fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
>         unlock_fb_info(fb_info);
> -       console_unlock();
> +       if (!lockless_register_fb)
> +               console_unlock();
>         return 0;
>  }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
>   ->fb notifier chain calls into fbcon
>     ->fbcon sets up console on new fbi
>       ->fbi->set_par
>         ->drm_fb_helper_set_par exercises full kms api
>
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
>
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.
>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

BR,
-R

> ---
>  drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 0705d8883ede..4e73b6f6b1c0 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
>         return 0;
>  }
>
> +static bool lockless_register_fb;
> +module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400);
> +MODULE_PARM_DESC(lockless_register_fb,
> +       "Lockless framebuffer registration for debugging [default=off]");
> +
>  static int do_register_framebuffer(struct fb_info *fb_info)
>  {
>         int i, ret;
> @@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
>         registered_fb[i] = fb_info;
>
>         event.info = fb_info;
> -       console_lock();
> +       if (!lockless_register_fb)
> +               console_lock();
>         if (!lock_fb_info(fb_info)) {
> -               console_unlock();
> +               if (!lockless_register_fb)
> +                       console_unlock();
>                 return -ENODEV;
>         }
>
>         fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
>         unlock_fb_info(fb_info);
> -       console_unlock();
> +       if (!lockless_register_fb)
> +               console_unlock();
>         return 0;
>  }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Archit Taneja]

On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> Faster than recompiling.
>
> Note that restore_fbdev_mode_unlocked is a bit special and the only
> one which returns an error code when fbdev isn't there - i915 needs
> that one to not fall over with some additional fbcon related restore
> code. Everyone else just ignores the return value or only prints a
> DRM_DEBUG level message.

Reviewed-by:Archit Taneja <architt@codeaurora.org>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 83aacb0cc9df..8259dec1de1f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -39,6 +39,11 @@
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_crtc_helper.h>
>
> +static bool drm_fbdev_emulation = true;
> +module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> +MODULE_PARM_DESC(fbdev_emulation,
> +		 "Enable legacy fbdev emulation [default=true]");
> +
>   static LIST_HEAD(kernel_fb_helper_list);
>
>   /**
> @@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>   	struct drm_connector *connector;
>   	int i;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	mutex_lock(&dev->mode_config.mutex);
>   	drm_for_each_connector(connector, dev) {
>   		struct drm_fb_helper_connector *fb_helper_connector;
> @@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>   	struct drm_fb_helper_connector **temp;
>   	struct drm_fb_helper_connector *fb_helper_connector;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>   	if (fb_helper->connector_count + 1 > fb_helper->connector_info_alloc_count) {
>   		temp = krealloc(fb_helper->connector_info, sizeof(struct drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
> @@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>   	struct drm_fb_helper_connector *fb_helper_connector;
>   	int i, j;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>
>   	for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -375,6 +389,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>   	bool do_delayed
>   	int ret;
>
> +	if (!drm_fbdev_emulation)
> +		return -ENODEV;
> +
>   	drm_modeset_lock_all(dev);
>   	ret = restore_fbdev_mode(fb_helper);
>
> @@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
>   	struct drm_crtc *crtc;
>   	int i;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	if (!max_conn_count)
>   		return -EINVAL;
>
> @@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
>
>   void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>   {
> +	if (!drm_fbdev_emulation)
> +		return;
> +
>   	if (!list_empty(&fb_helper->kernel_fb_list)) {
>   		list_del(&fb_helper->kernel_fb_list);
>   		if (list_empty(&kernel_fb_helper_list)) {
> @@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>   	struct drm_device *dev = fb_helper->dev;
>   	int count = 0;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	mutex_lock(&dev->mode_config.mutex);
>   	count = drm_fb_helper_probe_connector_modes(fb_helper,
>   						    dev->mode_config.max_width,
> @@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>   	struct drm_device *dev = fb_helper->dev;
>   	u32 max_width, max_height;
>
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
>   	mutex_lock(&fb_helper->dev->mode_config.mutex);
>   	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>   		fb_helper->delayed_hotplug = true;
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Archit Taneja]

On 08/26/2015 10:42 AM, Archit Taneja wrote:
>
>
> On 08/25/2015 07:15 PM, Daniel Vetter wrote:
>> Faster than recompiling.
>>
>> Note that restore_fbdev_mode_unlocked is a bit special and the only
>> one which returns an error code when fbdev isn't there - i915 needs
>> that one to not fall over with some additional fbcon related restore
>> code. Everyone else just ignores the return value or only prints a
>> DRM_DEBUG level message.
>
> Reviewed-by:Archit Taneja <architt@codeaurora.org>


With the module param, and the drivers should see the following state(
based on the truth table below):

module param | config option
    true      |    true       -> real helper funcs called, driver
                                 allocated drm_fb_helper is correctly
                                 populated.

    false     |    true       -> real helper funcs called, but bailed
                                 out early, driver allocated
                                 drm_fb_helper is non-NULL, but we won't
                                 end up using it.

     X        |    false      -> stub functions called, drm_fb_helper is
                                 NULL

I wonder if the 2nd combination could leave space for errors in other
drivers. Drivers tend to check whether the fb_helper struct is NULL
or not, and make decisions based on that. If the decisions are to
just call a drm_fb_helper function, then we're okay. If they do 
something more than that, then we could have an issue.

I'll prepare the remainder of 'Step 2' part of the series over this,
and we can test to check if we don't hit any corner cases.

Archit

>
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 83aacb0cc9df..8259dec1de1f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -39,6 +39,11 @@
>>   #include <drm/drm_fb_helper.h>
>>   #include <drm/drm_crtc_helper.h>
>>
>> +static bool drm_fbdev_emulation = true;
>> +module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>> +MODULE_PARM_DESC(fbdev_emulation,
>> +         "Enable legacy fbdev emulation [default=true]");
>> +
>>   static LIST_HEAD(kernel_fb_helper_list);
>>
>>   /**
>> @@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
>> drm_fb_helper *fb_helper)
>>       struct drm_connector *connector;
>>       int i;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       mutex_lock(&dev->mode_config.mutex);
>>       drm_for_each_connector(connector, dev) {
>>           struct drm_fb_helper_connector *fb_helper_connector;
>> @@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>>       struct drm_fb_helper_connector **temp;
>>       struct drm_fb_helper_connector *fb_helper_connector;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>>       if (fb_helper->connector_count + 1 >
>> fb_helper->connector_info_alloc_count) {
>>           temp = krealloc(fb_helper->connector_info, sizeof(struct
>> drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
>> GFP_KERNEL);
>> @@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
>> drm_fb_helper *fb_helper,
>>       struct drm_fb_helper_connector *fb_helper_connector;
>>       int i, j;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>>
>>       for (i = 0; i < fb_helper->connector_count; i++) {
>> @@ -375,6 +389,9 @@ int
>> drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
>> *fb_helper)
>>       bool do_delayed
>>       int ret;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return -ENODEV;
>> +
>>       drm_modeset_lock_all(dev);
>>       ret = restore_fbdev_mode(fb_helper);
>>
>> @@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
>>       struct drm_crtc *crtc;
>>       int i;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       if (!max_conn_count)
>>           return -EINVAL;
>>
>> @@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
>>
>>   void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>>   {
>> +    if (!drm_fbdev_emulation)
>> +        return;
>> +
>>       if (!list_empty(&fb_helper->kernel_fb_list)) {
>>           list_del(&fb_helper->kernel_fb_list);
>>           if (list_empty(&kernel_fb_helper_list)) {
>> @@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct
>> drm_fb_helper *fb_helper, int bpp_sel)
>>       struct drm_device *dev = fb_helper->dev;
>>       int count = 0;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       mutex_lock(&dev->mode_config.mutex);
>>       count = drm_fb_helper_probe_connector_modes(fb_helper,
>>                               dev->mode_config.max_width,
>> @@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct
>> drm_fb_helper *fb_helper)
>>       struct drm_device *dev = fb_helper->dev;
>>       u32 max_width, max_height;
>>
>> +    if (!drm_fbdev_emulation)
>> +        return 0;
>> +
>>       mutex_lock(&fb_helper->dev->mode_config.mutex);
>>       if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>>           fb_helper->delayed_hotplug = true;
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Daniel Vetter]

On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
> 
> 
> On 08/26/2015 10:42 AM, Archit Taneja wrote:
> >
> >
> >On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> >>Faster than recompiling.
> >>
> >>Note that restore_fbdev_mode_unlocked is a bit special and the only
> >>one which returns an error code when fbdev isn't there - i915 needs
> >>that one to not fall over with some additional fbcon related restore
> >>code. Everyone else just ignores the return value or only prints a
> >>DRM_DEBUG level message.
> >
> >Reviewed-by:Archit Taneja <architt@codeaurora.org>
> 
> 
> With the module param, and the drivers should see the following state(
> based on the truth table below):
> 
> module param | config option
>    true      |    true       -> real helper funcs called, driver
>                                 allocated drm_fb_helper is correctly
>                                 populated.
> 
>    false     |    true       -> real helper funcs called, but bailed
>                                 out early, driver allocated
>                                 drm_fb_helper is non-NULL, but we won't
>                                 end up using it.

Hm I tried to give drivers the same semantics here as with the stub
functions. Where did I screw up? The goal really was to match the end
result for drivers ...
-Daniel

>     X        |    false      -> stub functions called, drm_fb_helper is
>                                 NULL
> 
> I wonder if the 2nd combination could leave space for errors in other
> drivers. Drivers tend to check whether the fb_helper struct is NULL
> or not, and make decisions based on that. If the decisions are to
> just call a drm_fb_helper function, then we're okay. If they do something
> more than that, then we could have an issue.
> 
> I'll prepare the remainder of 'Step 2' part of the series over this,
> and we can test to check if we don't hit any corner cases.

> 
> Archit
> 
> >
> >>
> >>Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>---
> >>  drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >>b/drivers/gpu/drm/drm_fb_helper.c
> >>index 83aacb0cc9df..8259dec1de1f 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -39,6 +39,11 @@
> >>  #include <drm/drm_fb_helper.h>
> >>  #include <drm/drm_crtc_helper.h>
> >>
> >>+static bool drm_fbdev_emulation = true;
> >>+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >>+MODULE_PARM_DESC(fbdev_emulation,
> >>+         "Enable legacy fbdev emulation [default=true]");
> >>+
> >>  static LIST_HEAD(kernel_fb_helper_list);
> >>
> >>  /**
> >>@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
> >>drm_fb_helper *fb_helper)
> >>      struct drm_connector *connector;
> >>      int i;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&dev->mode_config.mutex);
> >>      drm_for_each_connector(connector, dev) {
> >>          struct drm_fb_helper_connector *fb_helper_connector;
> >>@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
> >>drm_fb_helper *fb_helper, struct drm_
> >>      struct drm_fb_helper_connector **temp;
> >>      struct drm_fb_helper_connector *fb_helper_connector;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >>      if (fb_helper->connector_count + 1 >
> >>fb_helper->connector_info_alloc_count) {
> >>          temp = krealloc(fb_helper->connector_info, sizeof(struct
> >>drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
> >>GFP_KERNEL);
> >>@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
> >>drm_fb_helper *fb_helper,
> >>      struct drm_fb_helper_connector *fb_helper_connector;
> >>      int i, j;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >>
> >>      for (i = 0; i < fb_helper->connector_count; i++) {
> >>@@ -375,6 +389,9 @@ int
> >>drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> >>*fb_helper)
> >>      bool do_delayed
> >>      int ret;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return -ENODEV;
> >>+
> >>      drm_modeset_lock_all(dev);
> >>      ret = restore_fbdev_mode(fb_helper);
> >>
> >>@@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
> >>      struct drm_crtc *crtc;
> >>      int i;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      if (!max_conn_count)
> >>          return -EINVAL;
> >>
> >>@@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
> >>
> >>  void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> >>  {
> >>+    if (!drm_fbdev_emulation)
> >>+        return;
> >>+
> >>      if (!list_empty(&fb_helper->kernel_fb_list)) {
> >>          list_del(&fb_helper->kernel_fb_list);
> >>          if (list_empty(&kernel_fb_helper_list)) {
> >>@@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct
> >>drm_fb_helper *fb_helper, int bpp_sel)
> >>      struct drm_device *dev = fb_helper->dev;
> >>      int count = 0;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&dev->mode_config.mutex);
> >>      count = drm_fb_helper_probe_connector_modes(fb_helper,
> >>                              dev->mode_config.max_width,
> >>@@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct
> >>drm_fb_helper *fb_helper)
> >>      struct drm_device *dev = fb_helper->dev;
> >>      u32 max_width, max_height;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&fb_helper->dev->mode_config.mutex);
> >>      if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> >>          fb_helper->delayed_hotplug = true;
> >>
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

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

Re: [PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked

[Daniel Vetter]

On Tue, Aug 25, 2015 at 03:20:02PM -0400, Rob Clark wrote:
> On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Using bool and returning true upon error is very uncommon. Also an int
> > return value is actually what all the callers which did check it seem
> > to have expected.
> >
> > v2: Restore hunk misplaced in a rebase, spotted by Rob.
> >
> > Cc: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Merged the first 2 patches from this series to drm-misc, thanks for your
review.
-Daniel

> 
> 
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++--------
> >  include/drm/drm_fb_helper.h     |  6 +++---
> >  2 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 418d299f3b12..859134e0d72d 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> >
> > -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >  {
> >         struct drm_device *dev = fb_helper->dev;
> >         struct drm_plane *plane;
> > -       bool error = false;
> >         int i;
> >
> >         drm_warn_on_modeset_not_all_locked(dev);
> > @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >                 if (crtc->funcs->cursor_set) {
> >                         ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> >                         if (ret)
> > -                               error = true;
> > +                               return ret;
> >                 }
> >
> >                 ret = drm_mode_set_config_internal(mode_set);
> >                 if (ret)
> > -                       error = true;
> > +                       return ret;
> >         }
> > -       return error;
> > +
> > +       return 0;
> >  }
> >
> >  /**
> > @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >   * 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.
> > + *
> > + * RETURNS:
> > + * Zero if everything went ok, negative error code otherwise.
> >   */
> > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> >  {
> >         struct drm_device *dev = fb_helper->dev;
> > -       bool ret;
> > -       bool do_delayed = false;
> > +       bool do_delayed;
> > +       int ret;
> >
> >         drm_modeset_lock_all(dev);
> >         ret = restore_fbdev_mode(fb_helper);
> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > index dbab4622b58f..67de1f10008e 100644
> > --- a/include/drm/drm_fb_helper.h
> > +++ b/include/drm/drm_fb_helper.h
> > @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
> >  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >                             struct fb_info *info);
> >
> > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
> > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
> >
> >  struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
> >  void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
> > @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >         return 0;
> >  }
> >
> > -static inline bool
> > +static inline int
> >  drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> >  {
> > -       return true;
> > +       return 0;
> >  }
> >
> >  static inline struct fb_info *
> > --
> > 1.8.3.1
> >

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

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Daniel Vetter]

On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
> > 
> > 
> > On 08/26/2015 10:42 AM, Archit Taneja wrote:
> > >
> > >
> > >On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> > >>Faster than recompiling.
> > >>
> > >>Note that restore_fbdev_mode_unlocked is a bit special and the only
> > >>one which returns an error code when fbdev isn't there - i915 needs
> > >>that one to not fall over with some additional fbcon related restore
> > >>code. Everyone else just ignores the return value or only prints a
> > >>DRM_DEBUG level message.
> > >
> > >Reviewed-by:Archit Taneja <architt@codeaurora.org>
> > 
> > 
> > With the module param, and the drivers should see the following state(
> > based on the truth table below):
> > 
> > module param | config option
> >    true      |    true       -> real helper funcs called, driver
> >                                 allocated drm_fb_helper is correctly
> >                                 populated.
> > 
> >    false     |    true       -> real helper funcs called, but bailed
> >                                 out early, driver allocated
> >                                 drm_fb_helper is non-NULL, but we won't
> >                                 end up using it.
> 
> Hm I tried to give drivers the same semantics here as with the stub
> functions. Where did I screw up? The goal really was to match the end
> result for drivers ...

Note that at least for i915 we can't make it perfectly equal since i915
compiles out a few more things with FBDEV_EMULATION=n than just the stubs.
Long-term we might want to push some of that into helpers too perhaps.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Archit Taneja]

On 08/26/2015 05:04 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
>>
>>
>> On 08/26/2015 10:42 AM, Archit Taneja wrote:
>>>
>>>
>>> On 08/25/2015 07:15 PM, Daniel Vetter wrote:
>>>> Faster than recompiling.
>>>>
>>>> Note that restore_fbdev_mode_unlocked is a bit special and the only
>>>> one which returns an error code when fbdev isn't there - i915 needs
>>>> that one to not fall over with some additional fbcon related restore
>>>> code. Everyone else just ignores the return value or only prints a
>>>> DRM_DEBUG level message.
>>>
>>> Reviewed-by:Archit Taneja <architt@codeaurora.org>
>>
>>
>> With the module param, and the drivers should see the following state(
>> based on the truth table below):
>>
>> module param | config option
>>     true      |    true       -> real helper funcs called, driver
>>                                  allocated drm_fb_helper is correctly
>>                                  populated.
>>
>>     false     |    true       -> real helper funcs called, but bailed
>>                                  out early, driver allocated
>>                                  drm_fb_helper is non-NULL, but we won't
>>                                  end up using it.
>
> Hm I tried to give drivers the same semantics here as with the stub
> functions. Where did I screw up? The goal really was to match the end
> result for drivers ...

Right, they would behave like the stub functions. But driver level
fbdev functions would still be called.

For example, with DRM_FBDEV_EMULATION not set, a stub
intel_fbdev_init() would be called. With DRM_FBDEV_EMULATION set but
the module param not set, the non-stub intel_fbdev_init() would still
be called, allocating an empty ifbdev. The ifbdev->helper would be
passed to the fb_helper funcs, but the module param check will bail
us out immediately.

So, the code flow isn't exactly the same as compared to when
DRM_FBDEV_EMULATION is disabled. The end result should be the same.

My only concern was whether other drivers might get confused with
a non-NULL fb_helper pointer. It most likely shouldn't be, though.

Archit

> -Daniel
>
>>      X        |    false      -> stub functions called, drm_fb_helper is
>>                                  NULL
>>
>> I wonder if the 2nd combination could leave space for errors in other
>> drivers. Drivers tend to check whether the fb_helper struct is NULL
>> or not, and make decisions based on that. If the decisions are to
>> just call a drm_fb_helper function, then we're okay. If they do something
>> more than that, then we could have an issue.
>>
>> I'll prepare the remainder of 'Step 2' part of the series over this,
>> and we can test to check if we don't hit any corner cases.
>
>>
>> Archit
>>
>>>
>>>>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>>   drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++
>>>>   1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 83aacb0cc9df..8259dec1de1f 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -39,6 +39,11 @@
>>>>   #include <drm/drm_fb_helper.h>
>>>>   #include <drm/drm_crtc_helper.h>
>>>>
>>>> +static bool drm_fbdev_emulation = true;
>>>> +module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>>>> +MODULE_PARM_DESC(fbdev_emulation,
>>>> +         "Enable legacy fbdev emulation [default=true]");
>>>> +
>>>>   static LIST_HEAD(kernel_fb_helper_list);
>>>>
>>>>   /**
>>>> @@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
>>>> drm_fb_helper *fb_helper)
>>>>       struct drm_connector *connector;
>>>>       int i;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       mutex_lock(&dev->mode_config.mutex);
>>>>       drm_for_each_connector(connector, dev) {
>>>>           struct drm_fb_helper_connector *fb_helper_connector;
>>>> @@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
>>>> drm_fb_helper *fb_helper, struct drm_
>>>>       struct drm_fb_helper_connector **temp;
>>>>       struct drm_fb_helper_connector *fb_helper_connector;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>>>>       if (fb_helper->connector_count + 1 >
>>>> fb_helper->connector_info_alloc_count) {
>>>>           temp = krealloc(fb_helper->connector_info, sizeof(struct
>>>> drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
>>>> GFP_KERNEL);
>>>> @@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
>>>> drm_fb_helper *fb_helper,
>>>>       struct drm_fb_helper_connector *fb_helper_connector;
>>>>       int i, j;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>>>>
>>>>       for (i = 0; i < fb_helper->connector_count; i++) {
>>>> @@ -375,6 +389,9 @@ int
>>>> drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
>>>> *fb_helper)
>>>>       bool do_delayed
>>>>       int ret;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return -ENODEV;
>>>> +
>>>>       drm_modeset_lock_all(dev);
>>>>       ret = restore_fbdev_mode(fb_helper);
>>>>
>>>> @@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
>>>>       struct drm_crtc *crtc;
>>>>       int i;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       if (!max_conn_count)
>>>>           return -EINVAL;
>>>>
>>>> @@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
>>>>
>>>>   void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>>>>   {
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return;
>>>> +
>>>>       if (!list_empty(&fb_helper->kernel_fb_list)) {
>>>>           list_del(&fb_helper->kernel_fb_list);
>>>>           if (list_empty(&kernel_fb_helper_list)) {
>>>> @@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct
>>>> drm_fb_helper *fb_helper, int bpp_sel)
>>>>       struct drm_device *dev = fb_helper->dev;
>>>>       int count = 0;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       mutex_lock(&dev->mode_config.mutex);
>>>>       count = drm_fb_helper_probe_connector_modes(fb_helper,
>>>>                               dev->mode_config.max_width,
>>>> @@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct
>>>> drm_fb_helper *fb_helper)
>>>>       struct drm_device *dev = fb_helper->dev;
>>>>       u32 max_width, max_height;
>>>>
>>>> +    if (!drm_fbdev_emulation)
>>>> +        return 0;
>>>> +
>>>>       mutex_lock(&fb_helper->dev->mode_config.mutex);
>>>>       if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>>>>           fb_helper->delayed_hotplug = true;
>>>>
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Archit Taneja]

On 08/26/2015 05:07 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
>>>
>>>
>>> On 08/26/2015 10:42 AM, Archit Taneja wrote:
>>>>
>>>>
>>>> On 08/25/2015 07:15 PM, Daniel Vetter wrote:
>>>>> Faster than recompiling.
>>>>>
>>>>> Note that restore_fbdev_mode_unlocked is a bit special and the only
>>>>> one which returns an error code when fbdev isn't there - i915 needs
>>>>> that one to not fall over with some additional fbcon related restore
>>>>> code. Everyone else just ignores the return value or only prints a
>>>>> DRM_DEBUG level message.
>>>>
>>>> Reviewed-by:Archit Taneja <architt@codeaurora.org>
>>>
>>>
>>> With the module param, and the drivers should see the following state(
>>> based on the truth table below):
>>>
>>> module param | config option
>>>     true      |    true       -> real helper funcs called, driver
>>>                                  allocated drm_fb_helper is correctly
>>>                                  populated.
>>>
>>>     false     |    true       -> real helper funcs called, but bailed
>>>                                  out early, driver allocated
>>>                                  drm_fb_helper is non-NULL, but we won't
>>>                                  end up using it.
>>
>> Hm I tried to give drivers the same semantics here as with the stub
>> functions. Where did I screw up? The goal really was to match the end
>> result for drivers ...
>
> Note that at least for i915 we can't make it perfectly equal since i915
> compiles out a few more things with FBDEV_EMULATION=n than just the stubs.
> Long-term we might want to push some of that into helpers too perhaps.

Ah, I missed looking at this mail.

Yeah, that's what I wanted to mainly point out. It looks okay
otherwise.

Since the param is 'true' by default. Things should be okay for all
drivers. If someone reports an issue with a driver with the above
combination, we could fix it individually.

I guess the next step now is to remove the custom config fbdev
emulation options and module params from drivers that have
those.

After that, we could start with scary process of removing the
CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Daniel Vetter]

On Wed, Aug 26, 2015 at 05:59:14PM +0530, Archit Taneja wrote:
> 
> 
> On 08/26/2015 05:07 PM, Daniel Vetter wrote:
> >On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote:
> >>On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
> >>>
> >>>
> >>>On 08/26/2015 10:42 AM, Archit Taneja wrote:
> >>>>
> >>>>
> >>>>On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> >>>>>Faster than recompiling.
> >>>>>
> >>>>>Note that restore_fbdev_mode_unlocked is a bit special and the only
> >>>>>one which returns an error code when fbdev isn't there - i915 needs
> >>>>>that one to not fall over with some additional fbcon related restore
> >>>>>code. Everyone else just ignores the return value or only prints a
> >>>>>DRM_DEBUG level message.
> >>>>
> >>>>Reviewed-by:Archit Taneja <architt@codeaurora.org>
> >>>
> >>>
> >>>With the module param, and the drivers should see the following state(
> >>>based on the truth table below):
> >>>
> >>>module param | config option
> >>>    true      |    true       -> real helper funcs called, driver
> >>>                                 allocated drm_fb_helper is correctly
> >>>                                 populated.
> >>>
> >>>    false     |    true       -> real helper funcs called, but bailed
> >>>                                 out early, driver allocated
> >>>                                 drm_fb_helper is non-NULL, but we won't
> >>>                                 end up using it.
> >>
> >>Hm I tried to give drivers the same semantics here as with the stub
> >>functions. Where did I screw up? The goal really was to match the end
> >>result for drivers ...
> >
> >Note that at least for i915 we can't make it perfectly equal since i915
> >compiles out a few more things with FBDEV_EMULATION=n than just the stubs.
> >Long-term we might want to push some of that into helpers too perhaps.
> 
> Ah, I missed looking at this mail.

Yeah we had to go ahead with removing I915_FBDEV since it was causing
trouble if you disable one but not the other. I think i915 is the only
driver where this can happen though, the others with their own fbdev
Kconfig option disable a lot less.

> Yeah, that's what I wanted to mainly point out. It looks okay
> otherwise.
> 
> Since the param is 'true' by default. Things should be okay for all
> drivers. If someone reports an issue with a driver with the above
> combination, we could fix it individually.
> 
> I guess the next step now is to remove the custom config fbdev
> emulation options and module params from drivers that have
> those.
> 
> After that, we could start with scary process of removing the
> CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver.

Yeah, definitely should do that for 4.4. I'll pull in this one here with
your r-b too, thanks for the feedback.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7254
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              302/302              300/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]



On 25/08/15 22:24, Rob Clark wrote:
> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> When the usual fbcon legacy options are enabled we have
>> ->register_framebuffer
>>   ->fb notifier chain calls into fbcon
>>     ->fbcon sets up console on new fbi
>>       ->fbi->set_par
>>         ->drm_fb_helper_set_par exercises full kms api
>>
>> And because of locking inversion hilarity all of register_framebuffer
>> is done with the console lock held. Which means that the first time on
>> driver load we exercise _all_ the kms code (all probe paths and
>> modeset paths for everything connected) is under the console lock.
>> That means if anything goes belly-up in that big pile of code nothing
>> ever reaches logfiles (and the machine is dead).
>>
>> Usual tactic to debug that is to temporarily remove those console_lock
>> calls to be able to capture backtraces. I'm fed up writing this patch
>> and recompiling kernels. Hence this patch here to add an unsafe,
>> kernel-taining option to do this at runtime.
>>
>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: linux-fbdev@vger.kernel.org
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> This one was causing me some problems, if I tried to enable
> lockless_register_fb.  It *looks* like it should work, so I'm not
> quite sure what the deal is.  But I'm 110% fan of getting something
> like this working, because console_lock is pretty much the bane of kms
> developer's existence..
> 
> I'll have to debug further on a system where I can see more than the
> bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1794 bytes --]



On 25/08/15 22:24, Rob Clark wrote:
> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> When the usual fbcon legacy options are enabled we have
>> ->register_framebuffer
>>   ->fb notifier chain calls into fbcon
>>     ->fbcon sets up console on new fbi
>>       ->fbi->set_par
>>         ->drm_fb_helper_set_par exercises full kms api
>>
>> And because of locking inversion hilarity all of register_framebuffer
>> is done with the console lock held. Which means that the first time on
>> driver load we exercise _all_ the kms code (all probe paths and
>> modeset paths for everything connected) is under the console lock.
>> That means if anything goes belly-up in that big pile of code nothing
>> ever reaches logfiles (and the machine is dead).
>>
>> Usual tactic to debug that is to temporarily remove those console_lock
>> calls to be able to capture backtraces. I'm fed up writing this patch
>> and recompiling kernels. Hence this patch here to add an unsafe,
>> kernel-taining option to do this at runtime.
>>
>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: linux-fbdev@vger.kernel.org
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> This one was causing me some problems, if I tried to enable
> lockless_register_fb.  It *looks* like it should work, so I'm not
> quite sure what the deal is.  But I'm 110% fan of getting something
> like this working, because console_lock is pretty much the bane of kms
> developer's existence..
> 
> I'll have to debug further on a system where I can see more than the
> bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>
> On 25/08/15 22:24, Rob Clark wrote:
>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> When the usual fbcon legacy options are enabled we have
>>> ->register_framebuffer
>>>   ->fb notifier chain calls into fbcon
>>>     ->fbcon sets up console on new fbi
>>>       ->fbi->set_par
>>>         ->drm_fb_helper_set_par exercises full kms api
>>>
>>> And because of locking inversion hilarity all of register_framebuffer
>>> is done with the console lock held. Which means that the first time on
>>> driver load we exercise _all_ the kms code (all probe paths and
>>> modeset paths for everything connected) is under the console lock.
>>> That means if anything goes belly-up in that big pile of code nothing
>>> ever reaches logfiles (and the machine is dead).
>>>
>>> Usual tactic to debug that is to temporarily remove those console_lock
>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>> kernel-taining option to do this at runtime.
>>>
>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: linux-fbdev@vger.kernel.org
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> This one was causing me some problems, if I tried to enable
>> lockless_register_fb.  It *looks* like it should work, so I'm not
>> quite sure what the deal is.  But I'm 110% fan of getting something
>> like this working, because console_lock is pretty much the bane of kms
>> developer's existence..
>>
>> I'll have to debug further on a system where I can see more than the
>> bottom three lines of the second to last backtrace..
>
> Any idea if anyone has ever looked at properly fixing this?

I hadn't had a chance to look at it further yet..  I think Daniel
claimed it worked for him, but he was probably on intel-next, where I
was on drm-next at the time which seemed to be having some unrelated
i915 issues (when I was trying to debug atomic fb-helper patches).  So
can't really say that the issue I had was actually related to this
patch.  I'll try again later this week or next, when hopefully i915 in
drm-next is in better shape..

BR,
-R

>  Tomi
>

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>
> On 25/08/15 22:24, Rob Clark wrote:
>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> When the usual fbcon legacy options are enabled we have
>>> ->register_framebuffer
>>>   ->fb notifier chain calls into fbcon
>>>     ->fbcon sets up console on new fbi
>>>       ->fbi->set_par
>>>         ->drm_fb_helper_set_par exercises full kms api
>>>
>>> And because of locking inversion hilarity all of register_framebuffer
>>> is done with the console lock held. Which means that the first time on
>>> driver load we exercise _all_ the kms code (all probe paths and
>>> modeset paths for everything connected) is under the console lock.
>>> That means if anything goes belly-up in that big pile of code nothing
>>> ever reaches logfiles (and the machine is dead).
>>>
>>> Usual tactic to debug that is to temporarily remove those console_lock
>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>> kernel-taining option to do this at runtime.
>>>
>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: linux-fbdev@vger.kernel.org
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> This one was causing me some problems, if I tried to enable
>> lockless_register_fb.  It *looks* like it should work, so I'm not
>> quite sure what the deal is.  But I'm 110% fan of getting something
>> like this working, because console_lock is pretty much the bane of kms
>> developer's existence..
>>
>> I'll have to debug further on a system where I can see more than the
>> bottom three lines of the second to last backtrace..
>
> Any idea if anyone has ever looked at properly fixing this?

I hadn't had a chance to look at it further yet..  I think Daniel
claimed it worked for him, but he was probably on intel-next, where I
was on drm-next at the time which seemed to be having some unrelated
i915 issues (when I was trying to debug atomic fb-helper patches).  So
can't really say that the issue I had was actually related to this
patch.  I'll try again later this week or next, when hopefully i915 in
drm-next is in better shape..

BR,
-R

>  Tomi
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]



On 01/09/15 17:34, Rob Clark wrote:
> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>
>> On 25/08/15 22:24, Rob Clark wrote:
>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>> When the usual fbcon legacy options are enabled we have
>>>> ->register_framebuffer
>>>>   ->fb notifier chain calls into fbcon
>>>>     ->fbcon sets up console on new fbi
>>>>       ->fbi->set_par
>>>>         ->drm_fb_helper_set_par exercises full kms api
>>>>
>>>> And because of locking inversion hilarity all of register_framebuffer
>>>> is done with the console lock held. Which means that the first time on
>>>> driver load we exercise _all_ the kms code (all probe paths and
>>>> modeset paths for everything connected) is under the console lock.
>>>> That means if anything goes belly-up in that big pile of code nothing
>>>> ever reaches logfiles (and the machine is dead).
>>>>
>>>> Usual tactic to debug that is to temporarily remove those console_lock
>>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>>> kernel-taining option to do this at runtime.
>>>>
>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> Cc: linux-fbdev@vger.kernel.org
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> This one was causing me some problems, if I tried to enable
>>> lockless_register_fb.  It *looks* like it should work, so I'm not
>>> quite sure what the deal is.  But I'm 110% fan of getting something
>>> like this working, because console_lock is pretty much the bane of kms
>>> developer's existence..
>>>
>>> I'll have to debug further on a system where I can see more than the
>>> bottom three lines of the second to last backtrace..
>>
>> Any idea if anyone has ever looked at properly fixing this?
> 
> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Oh, I didn't mean this patch, but the whole console lock in general.
I've also banged my head to a wall because of it =).

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2587 bytes --]



On 01/09/15 17:34, Rob Clark wrote:
> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>
>> On 25/08/15 22:24, Rob Clark wrote:
>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>> When the usual fbcon legacy options are enabled we have
>>>> ->register_framebuffer
>>>>   ->fb notifier chain calls into fbcon
>>>>     ->fbcon sets up console on new fbi
>>>>       ->fbi->set_par
>>>>         ->drm_fb_helper_set_par exercises full kms api
>>>>
>>>> And because of locking inversion hilarity all of register_framebuffer
>>>> is done with the console lock held. Which means that the first time on
>>>> driver load we exercise _all_ the kms code (all probe paths and
>>>> modeset paths for everything connected) is under the console lock.
>>>> That means if anything goes belly-up in that big pile of code nothing
>>>> ever reaches logfiles (and the machine is dead).
>>>>
>>>> Usual tactic to debug that is to temporarily remove those console_lock
>>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>>> kernel-taining option to do this at runtime.
>>>>
>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> Cc: linux-fbdev@vger.kernel.org
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> This one was causing me some problems, if I tried to enable
>>> lockless_register_fb.  It *looks* like it should work, so I'm not
>>> quite sure what the deal is.  But I'm 110% fan of getting something
>>> like this working, because console_lock is pretty much the bane of kms
>>> developer's existence..
>>>
>>> I'll have to debug further on a system where I can see more than the
>>> bottom three lines of the second to last backtrace..
>>
>> Any idea if anyone has ever looked at properly fixing this?
> 
> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Oh, I didn't mean this patch, but the whole console lock in general.
I've also banged my head to a wall because of it =).

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>
> On 01/09/15 17:34, Rob Clark wrote:
>> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>
>>>
>>> On 25/08/15 22:24, Rob Clark wrote:
>>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>>> When the usual fbcon legacy options are enabled we have
>>>>> ->register_framebuffer
>>>>>   ->fb notifier chain calls into fbcon
>>>>>     ->fbcon sets up console on new fbi
>>>>>       ->fbi->set_par
>>>>>         ->drm_fb_helper_set_par exercises full kms api
>>>>>
>>>>> And because of locking inversion hilarity all of register_framebuffer
>>>>> is done with the console lock held. Which means that the first time on
>>>>> driver load we exercise _all_ the kms code (all probe paths and
>>>>> modeset paths for everything connected) is under the console lock.
>>>>> That means if anything goes belly-up in that big pile of code nothing
>>>>> ever reaches logfiles (and the machine is dead).
>>>>>
>>>>> Usual tactic to debug that is to temporarily remove those console_lock
>>>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>>>> kernel-taining option to do this at runtime.
>>>>>
>>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>> Cc: linux-fbdev@vger.kernel.org
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>> This one was causing me some problems, if I tried to enable
>>>> lockless_register_fb.  It *looks* like it should work, so I'm not
>>>> quite sure what the deal is.  But I'm 110% fan of getting something
>>>> like this working, because console_lock is pretty much the bane of kms
>>>> developer's existence..
>>>>
>>>> I'll have to debug further on a system where I can see more than the
>>>> bottom three lines of the second to last backtrace..
>>>
>>> Any idea if anyone has ever looked at properly fixing this?
>>
>> I hadn't had a chance to look at it further yet..  I think Daniel
>> claimed it worked for him, but he was probably on intel-next, where I
>> was on drm-next at the time which seemed to be having some unrelated
>> i915 issues (when I was trying to debug atomic fb-helper patches).  So
>> can't really say that the issue I had was actually related to this
>> patch.  I'll try again later this week or next, when hopefully i915 in
>> drm-next is in better shape..
>
> Oh, I didn't mean this patch, but the whole console lock in general.
> I've also banged my head to a wall because of it =).

oh, not sure.. every time I've started looking closer at
console/console_lock I run away screaming..  I guess if it were
possible to push the lock down further so only drivers that needed the
lock (presumably serial/net/etc) could take it, that would be nice..
but not sure I am that brave..

BR,
-R

>  Tomi
>

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Rob Clark]

On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>
> On 01/09/15 17:34, Rob Clark wrote:
>> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>
>>>
>>> On 25/08/15 22:24, Rob Clark wrote:
>>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>>> When the usual fbcon legacy options are enabled we have
>>>>> ->register_framebuffer
>>>>>   ->fb notifier chain calls into fbcon
>>>>>     ->fbcon sets up console on new fbi
>>>>>       ->fbi->set_par
>>>>>         ->drm_fb_helper_set_par exercises full kms api
>>>>>
>>>>> And because of locking inversion hilarity all of register_framebuffer
>>>>> is done with the console lock held. Which means that the first time on
>>>>> driver load we exercise _all_ the kms code (all probe paths and
>>>>> modeset paths for everything connected) is under the console lock.
>>>>> That means if anything goes belly-up in that big pile of code nothing
>>>>> ever reaches logfiles (and the machine is dead).
>>>>>
>>>>> Usual tactic to debug that is to temporarily remove those console_lock
>>>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>>>> kernel-taining option to do this at runtime.
>>>>>
>>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>> Cc: linux-fbdev@vger.kernel.org
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>> This one was causing me some problems, if I tried to enable
>>>> lockless_register_fb.  It *looks* like it should work, so I'm not
>>>> quite sure what the deal is.  But I'm 110% fan of getting something
>>>> like this working, because console_lock is pretty much the bane of kms
>>>> developer's existence..
>>>>
>>>> I'll have to debug further on a system where I can see more than the
>>>> bottom three lines of the second to last backtrace..
>>>
>>> Any idea if anyone has ever looked at properly fixing this?
>>
>> I hadn't had a chance to look at it further yet..  I think Daniel
>> claimed it worked for him, but he was probably on intel-next, where I
>> was on drm-next at the time which seemed to be having some unrelated
>> i915 issues (when I was trying to debug atomic fb-helper patches).  So
>> can't really say that the issue I had was actually related to this
>> patch.  I'll try again later this week or next, when hopefully i915 in
>> drm-next is in better shape..
>
> Oh, I didn't mean this patch, but the whole console lock in general.
> I've also banged my head to a wall because of it =).

oh, not sure.. every time I've started looking closer at
console/console_lock I run away screaming..  I guess if it were
possible to push the lock down further so only drivers that needed the
lock (presumably serial/net/etc) could take it, that would be nice..
but not sure I am that brave..

BR,
-R

>  Tomi
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

On Tue, Sep 01, 2015 at 11:12:11AM -0400, Rob Clark wrote:
> On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> >
> > On 01/09/15 17:34, Rob Clark wrote:
> >> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>>
> >>>
> >>> On 25/08/15 22:24, Rob Clark wrote:
> >>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >>>>> When the usual fbcon legacy options are enabled we have
> >>>>> ->register_framebuffer
> >>>>>   ->fb notifier chain calls into fbcon
> >>>>>     ->fbcon sets up console on new fbi
> >>>>>       ->fbi->set_par
> >>>>>         ->drm_fb_helper_set_par exercises full kms api
> >>>>>
> >>>>> And because of locking inversion hilarity all of register_framebuffer
> >>>>> is done with the console lock held. Which means that the first time on
> >>>>> driver load we exercise _all_ the kms code (all probe paths and
> >>>>> modeset paths for everything connected) is under the console lock.
> >>>>> That means if anything goes belly-up in that big pile of code nothing
> >>>>> ever reaches logfiles (and the machine is dead).
> >>>>>
> >>>>> Usual tactic to debug that is to temporarily remove those console_lock
> >>>>> calls to be able to capture backtraces. I'm fed up writing this patch
> >>>>> and recompiling kernels. Hence this patch here to add an unsafe,
> >>>>> kernel-taining option to do this at runtime.
> >>>>>
> >>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> >>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>>> Cc: linux-fbdev@vger.kernel.org
> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>
> >>>> This one was causing me some problems, if I tried to enable
> >>>> lockless_register_fb.  It *looks* like it should work, so I'm not
> >>>> quite sure what the deal is.  But I'm 110% fan of getting something
> >>>> like this working, because console_lock is pretty much the bane of kms
> >>>> developer's existence..
> >>>>
> >>>> I'll have to debug further on a system where I can see more than the
> >>>> bottom three lines of the second to last backtrace..
> >>>
> >>> Any idea if anyone has ever looked at properly fixing this?
> >>
> >> I hadn't had a chance to look at it further yet..  I think Daniel
> >> claimed it worked for him, but he was probably on intel-next, where I
> >> was on drm-next at the time which seemed to be having some unrelated
> >> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> >> can't really say that the issue I had was actually related to this
> >> patch.  I'll try again later this week or next, when hopefully i915 in
> >> drm-next is in better shape..
> >
> > Oh, I didn't mean this patch, but the whole console lock in general.
> > I've also banged my head to a wall because of it =).
> 
> oh, not sure.. every time I've started looking closer at
> console/console_lock I run away screaming..  I guess if it were
> possible to push the lock down further so only drivers that needed the
> lock (presumably serial/net/etc) could take it, that would be nice..
> but not sure I am that brave..

console_lock is pretty much unfixable without rewriting half of fbdev.
Which I don't expect to ever happen. For the curious look at all the
commits changing locking in fbdev over the past few years.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

On Tue, Sep 01, 2015 at 11:12:11AM -0400, Rob Clark wrote:
> On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> >
> > On 01/09/15 17:34, Rob Clark wrote:
> >> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>>
> >>>
> >>> On 25/08/15 22:24, Rob Clark wrote:
> >>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >>>>> When the usual fbcon legacy options are enabled we have
> >>>>> ->register_framebuffer
> >>>>>   ->fb notifier chain calls into fbcon
> >>>>>     ->fbcon sets up console on new fbi
> >>>>>       ->fbi->set_par
> >>>>>         ->drm_fb_helper_set_par exercises full kms api
> >>>>>
> >>>>> And because of locking inversion hilarity all of register_framebuffer
> >>>>> is done with the console lock held. Which means that the first time on
> >>>>> driver load we exercise _all_ the kms code (all probe paths and
> >>>>> modeset paths for everything connected) is under the console lock.
> >>>>> That means if anything goes belly-up in that big pile of code nothing
> >>>>> ever reaches logfiles (and the machine is dead).
> >>>>>
> >>>>> Usual tactic to debug that is to temporarily remove those console_lock
> >>>>> calls to be able to capture backtraces. I'm fed up writing this patch
> >>>>> and recompiling kernels. Hence this patch here to add an unsafe,
> >>>>> kernel-taining option to do this at runtime.
> >>>>>
> >>>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> >>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>>> Cc: linux-fbdev@vger.kernel.org
> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>
> >>>> This one was causing me some problems, if I tried to enable
> >>>> lockless_register_fb.  It *looks* like it should work, so I'm not
> >>>> quite sure what the deal is.  But I'm 110% fan of getting something
> >>>> like this working, because console_lock is pretty much the bane of kms
> >>>> developer's existence..
> >>>>
> >>>> I'll have to debug further on a system where I can see more than the
> >>>> bottom three lines of the second to last backtrace..
> >>>
> >>> Any idea if anyone has ever looked at properly fixing this?
> >>
> >> I hadn't had a chance to look at it further yet..  I think Daniel
> >> claimed it worked for him, but he was probably on intel-next, where I
> >> was on drm-next at the time which seemed to be having some unrelated
> >> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> >> can't really say that the issue I had was actually related to this
> >> patch.  I'll try again later this week or next, when hopefully i915 in
> >> drm-next is in better shape..
> >
> > Oh, I didn't mean this patch, but the whole console lock in general.
> > I've also banged my head to a wall because of it =).
> 
> oh, not sure.. every time I've started looking closer at
> console/console_lock I run away screaming..  I guess if it were
> possible to push the lock down further so only drivers that needed the
> lock (presumably serial/net/etc) could take it, that would be nice..
> but not sure I am that brave..

console_lock is pretty much unfixable without rewriting half of fbdev.
Which I don't expect to ever happen. For the curious look at all the
commits changing locking in fbdev over the past few years.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]



On 01/09/15 17:34, Rob Clark wrote:

> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Rob, did you have a chance to test this?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 563 bytes --]



On 01/09/15 17:34, Rob Clark wrote:

> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Rob, did you have a chance to test this?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]


On 25/08/15 16:45, Daniel Vetter wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
>   ->fb notifier chain calls into fbcon
>     ->fbcon sets up console on new fbi
>       ->fbi->set_par
>         ->drm_fb_helper_set_par exercises full kms api
> 
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
> 
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1321 bytes --]


On 25/08/15 16:45, Daniel Vetter wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
>   ->fb notifier chain calls into fbcon
>     ->fbcon sets up console on new fbi
>       ->fbi->set_par
>         ->drm_fb_helper_set_par exercises full kms api
> 
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
> 
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:
> 
> On 25/08/15 16:45, Daniel Vetter wrote:
> > When the usual fbcon legacy options are enabled we have
> > ->register_framebuffer
> >   ->fb notifier chain calls into fbcon
> >     ->fbcon sets up console on new fbi
> >       ->fbi->set_par
> >         ->drm_fb_helper_set_par exercises full kms api
> > 
> > And because of locking inversion hilarity all of register_framebuffer
> > is done with the console lock held. Which means that the first time on
> > driver load we exercise _all_ the kms code (all probe paths and
> > modeset paths for everything connected) is under the console lock.
> > That means if anything goes belly-up in that big pile of code nothing
> > ever reaches logfiles (and the machine is dead).
> > 
> > Usual tactic to debug that is to temporarily remove those console_lock
> > calls to be able to capture backtraces. I'm fed up writing this patch
> > and recompiling kernels. Hence this patch here to add an unsafe,
> > kernel-taining option to do this at runtime.
> 
> I think this was never merged. This was part 4 of 4, were there
> dependencies or...? Should I apply this for 4.5?

Patches 1-3 have all already landed in drm, and patch 4 is free standing.
Would be great if you can pull it in.
 
> But then... I think my issues with console lock have been later at
> runtime, not at register. Maybe we need a module option to disable the
> console lock altogether. I wonder how much havoc that might create, though.

Hm, where in fbdev do you hold the console_lock outside of
registering/unregistering an fbdev (because of fbcon)? There's of course
general trouble with console_lock deadlocks and fun like that, but ime
that all got a lot more manageable since I added lockdep annotations to
console_lock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Daniel Vetter]

On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:
> 
> On 25/08/15 16:45, Daniel Vetter wrote:
> > When the usual fbcon legacy options are enabled we have
> > ->register_framebuffer
> >   ->fb notifier chain calls into fbcon
> >     ->fbcon sets up console on new fbi
> >       ->fbi->set_par
> >         ->drm_fb_helper_set_par exercises full kms api
> > 
> > And because of locking inversion hilarity all of register_framebuffer
> > is done with the console lock held. Which means that the first time on
> > driver load we exercise _all_ the kms code (all probe paths and
> > modeset paths for everything connected) is under the console lock.
> > That means if anything goes belly-up in that big pile of code nothing
> > ever reaches logfiles (and the machine is dead).
> > 
> > Usual tactic to debug that is to temporarily remove those console_lock
> > calls to be able to capture backtraces. I'm fed up writing this patch
> > and recompiling kernels. Hence this patch here to add an unsafe,
> > kernel-taining option to do this at runtime.
> 
> I think this was never merged. This was part 4 of 4, were there
> dependencies or...? Should I apply this for 4.5?

Patches 1-3 have all already landed in drm, and patch 4 is free standing.
Would be great if you can pull it in.
 
> But then... I think my issues with console lock have been later at
> runtime, not at register. Maybe we need a module option to disable the
> console lock altogether. I wonder how much havoc that might create, though.

Hm, where in fbdev do you hold the console_lock outside of
registering/unregistering an fbdev (because of fbcon)? There's of course
general trouble with console_lock deadlocks and fun like that, but ime
that all got a lot more manageable since I added lockdep annotations to
console_lock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]



On 08/12/15 10:19, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:
>>
>> On 25/08/15 16:45, Daniel Vetter wrote:
>>> When the usual fbcon legacy options are enabled we have
>>> ->register_framebuffer
>>>   ->fb notifier chain calls into fbcon
>>>     ->fbcon sets up console on new fbi
>>>       ->fbi->set_par
>>>         ->drm_fb_helper_set_par exercises full kms api
>>>
>>> And because of locking inversion hilarity all of register_framebuffer
>>> is done with the console lock held. Which means that the first time on
>>> driver load we exercise _all_ the kms code (all probe paths and
>>> modeset paths for everything connected) is under the console lock.
>>> That means if anything goes belly-up in that big pile of code nothing
>>> ever reaches logfiles (and the machine is dead).
>>>
>>> Usual tactic to debug that is to temporarily remove those console_lock
>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>> kernel-taining option to do this at runtime.
>>
>> I think this was never merged. This was part 4 of 4, were there
>> dependencies or...? Should I apply this for 4.5?
> 
> Patches 1-3 have all already landed in drm, and patch 4 is free standing.
> Would be great if you can pull it in.

Ok, I'll apply for 4.5.

>> But then... I think my issues with console lock have been later at
>> runtime, not at register. Maybe we need a module option to disable the
>> console lock altogether. I wonder how much havoc that might create, though.
> 
> Hm, where in fbdev do you hold the console_lock outside of
> registering/unregistering an fbdev (because of fbcon)? There's of course
> general trouble with console_lock deadlocks and fun like that, but ime
> that all got a lot more manageable since I added lockdep annotations to
> console_lock.

I don't know... I just have a vague recollection that I was having
trouble with the lock with... crashes during blanking, perhaps. I really
can't remember, so possibly things are better now, or I just remember wrong.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2168 bytes --]



On 08/12/15 10:19, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:
>>
>> On 25/08/15 16:45, Daniel Vetter wrote:
>>> When the usual fbcon legacy options are enabled we have
>>> ->register_framebuffer
>>>   ->fb notifier chain calls into fbcon
>>>     ->fbcon sets up console on new fbi
>>>       ->fbi->set_par
>>>         ->drm_fb_helper_set_par exercises full kms api
>>>
>>> And because of locking inversion hilarity all of register_framebuffer
>>> is done with the console lock held. Which means that the first time on
>>> driver load we exercise _all_ the kms code (all probe paths and
>>> modeset paths for everything connected) is under the console lock.
>>> That means if anything goes belly-up in that big pile of code nothing
>>> ever reaches logfiles (and the machine is dead).
>>>
>>> Usual tactic to debug that is to temporarily remove those console_lock
>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>> kernel-taining option to do this at runtime.
>>
>> I think this was never merged. This was part 4 of 4, were there
>> dependencies or...? Should I apply this for 4.5?
> 
> Patches 1-3 have all already landed in drm, and patch 4 is free standing.
> Would be great if you can pull it in.

Ok, I'll apply for 4.5.

>> But then... I think my issues with console lock have been later at
>> runtime, not at register. Maybe we need a module option to disable the
>> console lock altogether. I wonder how much havoc that might create, though.
> 
> Hm, where in fbdev do you hold the console_lock outside of
> registering/unregistering an fbdev (because of fbcon)? There's of course
> general trouble with console_lock deadlocks and fun like that, but ime
> that all got a lot more manageable since I added lockdep annotations to
> console_lock.

I don't know... I just have a vague recollection that I was having
trouble with the lock with... crashes during blanking, perhaps. I really
can't remember, so possibly things are better now, or I just remember wrong.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx