* [REGRESSION] system hang on ILK/SNB/IVB
@ 2016-03-30 17:20 Gabriel Feceoru
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
` (3 more replies)
0 siblings, 4 replies; 34+ messages in thread
From: Gabriel Feceoru @ 2016-03-30 17:20 UTC (permalink / raw)
To: lukas, daniel.vetter; +Cc: Tomi Sarvela, intel-gfx
This commit causes a hang while running kms suspend tests
(kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
Probably the same problem with the one in v2, but on older HW.
commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
Author: Lukas Wunner <lukas@wunner.de>
Date: Wed Mar 9 12:52:53 2016 +0100
drm/i915: Fix races on fbdev
The ->lastclose callback invokes intel_fbdev_restore_mode() and has
been witnessed to run before intel_fbdev_initial_config_async()
has finished.
We might likewise receive hotplug events before we've had a chance to
fully set up the fbdev.
Fix by waiting for the asynchronous thread to finish.
v2:
An async_synchronize_full() was also added to intel_fbdev_set_suspend()
in v1 which turned out to be entirely gratuitous. It caused a deadlock
on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
for CI support) and was unnecessary since a device is never suspended
until its ->probe callback (and all asynchronous tasks it scheduled)
have finished. See dpm_prepare(), which calls wait_for_device_probe(),
which calls async_synchronize_full().
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link:
http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D3@gabe.freedesktop.org
Regards,
Gabriel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:20 [REGRESSION] system hang on ILK/SNB/IVB Gabriel Feceoru
@ 2016-03-30 17:57 ` Chris Wilson
2016-03-30 18:10 ` kbuild test robot
` (4 more replies)
2016-03-30 18:47 ` [REGRESSION] system hang on ILK/SNB/IVB Daniel Vetter
` (2 subsequent siblings)
3 siblings, 5 replies; 34+ messages in thread
From: Chris Wilson @ 2016-03-30 17:57 UTC (permalink / raw)
To: intel-gfx, Gabriel Feceoru
If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_fbdev.c | 48 ++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 51a5e39e52f2..0bae91268a12 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -114,6 +114,24 @@ static struct fb_ops intelfb_ops = {
.fb_debug_leave = drm_fb_helper_debug_leave,
};
+static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
+{
+ struct drm_i915_device *dev_priv = to_i915(dev);
+ struct fb_info *info;
+
+ if (dev_priv->fbdev == NULL)
+ return NULL;
+
+ info = ifbdev->helper.fbdev;
+ if (info->screen_base == NULL)
+ return NULL;
+
+ if (info->state != FBINFO_STATE_RUNNING)
+ return NULL;
+
+ return dev_priv->fbdev;
+}
+
static int intelfb_alloc(struct drm_fb_helper *helper,
struct drm_fb_helper_surface_size *sizes)
{
@@ -764,6 +782,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
return;
info = ifbdev->helper.fbdev;
+ if (info->screen_base == NULL)
+ return;
if (synchronous) {
/* Flush any pending work to turn the console on, and then
@@ -805,32 +825,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
+
+ if (ifbdev == NULL)
+ return;
- async_synchronize_full();
- if (dev_priv->fbdev)
- drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+ drm_fb_helper_hotplug_event(&ifbdev->helper);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
{
- int ret;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct drm_fb_helper *fb_helper;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
- async_synchronize_full();
- if (!ifbdev)
+ if (ifbdev == NULL)
return;
- fb_helper = &ifbdev->helper;
-
- ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
- if (ret) {
- DRM_DEBUG("failed to restore crtc mode\n");
- } else {
- mutex_lock(&fb_helper->dev->struct_mutex);
+ if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
+ mutex_lock(&dev->struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
- mutex_unlock(&fb_helper->dev->struct_mutex);
+ mutex_unlock(&dev->struct_mutex);
}
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
@ 2016-03-30 18:10 ` kbuild test robot
2016-03-30 18:10 ` kbuild test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-03-30 18:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5701 bytes --]
Hi Chris,
[auto build test WARNING on drm-intel/for-linux-next]
[cannot apply to v4.6-rc1 next-20160330]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Protect-fbdev-across-slow-or-failed-initialisation/20160331-015912
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x015-03310059 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_fbdev.c: In function 'intel_fbdev_get_if_active':
drivers/gpu/drm/i915/intel_fbdev.c:119:37: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
struct drm_i915_device *dev_priv = to_i915(dev);
^
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/async.h:15,
from drivers/gpu/drm/i915/intel_fbdev.c:27:
drivers/gpu/drm/i915/intel_fbdev.c:122:14: error: dereferencing pointer to incomplete type 'struct drm_i915_device'
if (dev_priv->fbdev == NULL)
^
include/linux/compiler.h:147:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/i915/intel_fbdev.c:122:2: note: in expansion of macro 'if'
if (dev_priv->fbdev == NULL)
^
drivers/gpu/drm/i915/intel_fbdev.c:125:9: error: 'ifbdev' undeclared (first use in this function)
info = ifbdev->helper.fbdev;
^
drivers/gpu/drm/i915/intel_fbdev.c:125:9: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/intel_fbdev.c:133:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +/if +122 drivers/gpu/drm/i915/intel_fbdev.c
21 * DEALINGS IN THE SOFTWARE.
22 *
23 * Authors:
24 * David Airlie
25 */
26
> 27 #include <linux/async.h>
28 #include <linux/module.h>
29 #include <linux/kernel.h>
30 #include <linux/console.h>
31 #include <linux/errno.h>
32 #include <linux/string.h>
33 #include <linux/mm.h>
34 #include <linux/tty.h>
35 #include <linux/sysrq.h>
36 #include <linux/delay.h>
37 #include <linux/fb.h>
38 #include <linux/init.h>
39 #include <linux/vga_switcheroo.h>
40
41 #include <drm/drmP.h>
42 #include <drm/drm_crtc.h>
43 #include <drm/drm_fb_helper.h>
44 #include "intel_drv.h"
45 #include <drm/i915_drm.h>
46 #include "i915_drv.h"
47
48 static int intel_fbdev_set_par(struct fb_info *info)
49 {
50 struct drm_fb_helper *fb_helper = info->par;
51 struct intel_fbdev *ifbdev =
52 container_of(fb_helper, struct intel_fbdev, helper);
53 int ret;
54
55 ret = drm_fb_helper_set_par(info);
56
57 if (ret == 0) {
58 mutex_lock(&fb_helper->dev->struct_mutex);
59 intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
60 mutex_unlock(&fb_helper->dev->struct_mutex);
61 }
62
63 return ret;
64 }
65
66 static int intel_fbdev_blank(int blank, struct fb_info *info)
67 {
68 struct drm_fb_helper *fb_helper = info->par;
69 struct intel_fbdev *ifbdev =
70 container_of(fb_helper, struct intel_fbdev, helper);
71 int ret;
72
73 ret = drm_fb_helper_blank(blank, info);
74
75 if (ret == 0) {
76 mutex_lock(&fb_helper->dev->struct_mutex);
77 intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
78 mutex_unlock(&fb_helper->dev->struct_mutex);
79 }
80
81 return ret;
82 }
83
84 static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
85 struct fb_info *info)
86 {
87 struct drm_fb_helper *fb_helper = info->par;
88 struct intel_fbdev *ifbdev =
89 container_of(fb_helper, struct intel_fbdev, helper);
90
91 int ret;
92 ret = drm_fb_helper_pan_display(var, info);
93
94 if (ret == 0) {
95 mutex_lock(&fb_helper->dev->struct_mutex);
96 intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
97 mutex_unlock(&fb_helper->dev->struct_mutex);
98 }
99
100 return ret;
101 }
102
103 static struct fb_ops intelfb_ops = {
104 .owner = THIS_MODULE,
105 .fb_check_var = drm_fb_helper_check_var,
106 .fb_set_par = intel_fbdev_set_par,
107 .fb_fillrect = drm_fb_helper_cfb_fillrect,
108 .fb_copyarea = drm_fb_helper_cfb_copyarea,
109 .fb_imageblit = drm_fb_helper_cfb_imageblit,
110 .fb_pan_display = intel_fbdev_pan_display,
111 .fb_blank = intel_fbdev_blank,
112 .fb_setcmap = drm_fb_helper_setcmap,
113 .fb_debug_enter = drm_fb_helper_debug_enter,
114 .fb_debug_leave = drm_fb_helper_debug_leave,
115 };
116
117 static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
118 {
119 struct drm_i915_device *dev_priv = to_i915(dev);
120 struct fb_info *info;
121
> 122 if (dev_priv->fbdev == NULL)
123 return NULL;
124
125 info = ifbdev->helper.fbdev;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24904 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
2016-03-30 18:10 ` kbuild test robot
@ 2016-03-30 18:10 ` kbuild test robot
2016-03-30 18:26 ` kbuild test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-03-30 18:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to v4.6-rc1 next-20160330]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Protect-fbdev-across-slow-or-failed-initialisation/20160331-015912
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x012-03310059 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_fbdev.c: In function 'intel_fbdev_get_if_active':
>> drivers/gpu/drm/i915/intel_fbdev.c:119:37: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
struct drm_i915_device *dev_priv = to_i915(dev);
^
>> drivers/gpu/drm/i915/intel_fbdev.c:122:14: error: dereferencing pointer to incomplete type 'struct drm_i915_device'
if (dev_priv->fbdev == NULL)
^
>> drivers/gpu/drm/i915/intel_fbdev.c:125:9: error: 'ifbdev' undeclared (first use in this function)
info = ifbdev->helper.fbdev;
^
drivers/gpu/drm/i915/intel_fbdev.c:125:9: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_fbdev.c:133:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +122 drivers/gpu/drm/i915/intel_fbdev.c
113 .fb_debug_enter = drm_fb_helper_debug_enter,
114 .fb_debug_leave = drm_fb_helper_debug_leave,
115 };
116
117 static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
118 {
> 119 struct drm_i915_device *dev_priv = to_i915(dev);
120 struct fb_info *info;
121
> 122 if (dev_priv->fbdev == NULL)
123 return NULL;
124
> 125 info = ifbdev->helper.fbdev;
126 if (info->screen_base == NULL)
127 return NULL;
128
129 if (info->state != FBINFO_STATE_RUNNING)
130 return NULL;
131
132 return dev_priv->fbdev;
> 133 }
134
135 static int intelfb_alloc(struct drm_fb_helper *helper,
136 struct drm_fb_helper_surface_size *sizes)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23574 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
2016-03-30 18:10 ` kbuild test robot
2016-03-30 18:10 ` kbuild test robot
@ 2016-03-30 18:26 ` kbuild test robot
2016-03-30 18:30 ` Chris Wilson
2016-03-30 18:56 ` [PATCH v3] " Chris Wilson
4 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-03-30 18:26 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to v4.6-rc1 next-20160330]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Protect-fbdev-across-slow-or-failed-initialisation/20160331-015912
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_fbdev.c: In function 'intel_fbdev_get_if_active':
drivers/gpu/drm/i915/intel_fbdev.c:119:37: warning: initialization from incompatible pointer type
struct drm_i915_device *dev_priv = to_i915(dev);
^
>> drivers/gpu/drm/i915/intel_fbdev.c:122:14: error: dereferencing pointer to incomplete type
if (dev_priv->fbdev == NULL)
^
drivers/gpu/drm/i915/intel_fbdev.c:125:9: error: 'ifbdev' undeclared (first use in this function)
info = ifbdev->helper.fbdev;
^
drivers/gpu/drm/i915/intel_fbdev.c:125:9: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/intel_fbdev.c:132:17: error: dereferencing pointer to incomplete type
return dev_priv->fbdev;
^
drivers/gpu/drm/i915/intel_fbdev.c:133:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +122 drivers/gpu/drm/i915/intel_fbdev.c
113 .fb_debug_enter = drm_fb_helper_debug_enter,
114 .fb_debug_leave = drm_fb_helper_debug_leave,
115 };
116
117 static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
118 {
> 119 struct drm_i915_device *dev_priv = to_i915(dev);
120 struct fb_info *info;
121
> 122 if (dev_priv->fbdev == NULL)
123 return NULL;
124
125 info = ifbdev->helper.fbdev;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36120 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
` (2 preceding siblings ...)
2016-03-30 18:26 ` kbuild test robot
@ 2016-03-30 18:30 ` Chris Wilson
2016-03-30 18:56 ` [PATCH v3] " Chris Wilson
4 siblings, 0 replies; 34+ messages in thread
From: Chris Wilson @ 2016-03-30 18:30 UTC (permalink / raw)
To: intel-gfx, Gabriel Feceoru
On Wed, Mar 30, 2016 at 06:57:14PM +0100, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure. Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 48 ++++++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 51a5e39e52f2..0bae91268a12 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,24 @@ static struct fb_ops intelfb_ops = {
> .fb_debug_leave = drm_fb_helper_debug_leave,
> };
>
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> + struct drm_i915_device *dev_priv = to_i915(dev);
As kbuild reported, the danger of retyping a patch from email and
forgetting you compiled on a machine without fbdev.
s/drm_i915_device/drm_i915_private/
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
` (3 preceding siblings ...)
2016-03-30 18:30 ` Chris Wilson
@ 2016-03-30 18:56 ` Chris Wilson
2016-03-31 12:00 ` Gabriel Feceoru
4 siblings, 1 reply; 34+ messages in thread
From: Chris Wilson @ 2016-03-30 18:56 UTC (permalink / raw)
To: intel-gfx, Gabriel Feceoru
If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).
v3: To create a typo introduced when retyping
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_fbdev.c | 48 ++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 153ea7a3fcf6..5029f927fe0d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -114,6 +114,24 @@ static struct fb_ops intelfb_ops = {
.fb_debug_leave = drm_fb_helper_debug_leave,
};
+static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct fb_info *info;
+
+ if (dev_priv->fbdev == NULL)
+ return NULL;
+
+ info = dev_priv->fbdev->helper.fbdev;
+ if (info->screen_base == NULL)
+ return NULL;
+
+ if (info->state != FBINFO_STATE_RUNNING)
+ return NULL;
+
+ return dev_priv->fbdev;
+}
+
static int intelfb_alloc(struct drm_fb_helper *helper,
struct drm_fb_helper_surface_size *sizes)
{
@@ -766,6 +784,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
return;
info = ifbdev->helper.fbdev;
+ if (info->screen_base == NULL)
+ return;
if (synchronous) {
/* Flush any pending work to turn the console on, and then
@@ -807,32 +827,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
+
+ if (ifbdev == NULL)
+ return;
- async_synchronize_full();
- if (dev_priv->fbdev)
- drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+ drm_fb_helper_hotplug_event(&ifbdev->helper);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
{
- int ret;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct drm_fb_helper *fb_helper;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
- async_synchronize_full();
- if (!ifbdev)
+ if (ifbdev == NULL)
return;
- fb_helper = &ifbdev->helper;
-
- ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
- if (ret) {
- DRM_DEBUG("failed to restore crtc mode\n");
- } else {
- mutex_lock(&fb_helper->dev->struct_mutex);
+ if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
+ mutex_lock(&dev->struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
- mutex_unlock(&fb_helper->dev->struct_mutex);
+ mutex_unlock(&dev->struct_mutex);
}
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-30 18:56 ` [PATCH v3] " Chris Wilson
@ 2016-03-31 12:00 ` Gabriel Feceoru
2016-03-31 13:57 ` [PATCH v4 1/2] " Chris Wilson
0 siblings, 1 reply; 34+ messages in thread
From: Gabriel Feceoru @ 2016-03-31 12:00 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
This almost fixes the problem , but with this:
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 5029f92..a6d3c58 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -123,8 +123,9 @@ static struct intel_fbdev
*intel_fbdev_get_if_active(struct drm_device *dev)
return NULL;
info = dev_priv->fbdev->helper.fbdev;
- if (info->screen_base == NULL)
+ if (info == NULL || info->screen_base == NULL)
return NULL;
if (info->state != FBINFO_STATE_RUNNING)
return NULL;
I got initially a page fault during init, but with this fix, the suspend
test passes.
Thank you, please consider this.
Regards,
Gabriel
On 30.03.2016 21:56, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure. Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> v3: To create a typo introduced when retyping
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 48 ++++++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 153ea7a3fcf6..5029f927fe0d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,24 @@ static struct fb_ops intelfb_ops = {
> .fb_debug_leave = drm_fb_helper_debug_leave,
> };
>
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct fb_info *info;
> +
> + if (dev_priv->fbdev == NULL)
> + return NULL;
> +
> + info = dev_priv->fbdev->helper.fbdev;
> + if (info->screen_base == NULL)
> + return NULL;
> +
> + if (info->state != FBINFO_STATE_RUNNING)
> + return NULL;
> +
> + return dev_priv->fbdev;
> +}
> +
> static int intelfb_alloc(struct drm_fb_helper *helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> @@ -766,6 +784,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> return;
>
> info = ifbdev->helper.fbdev;
> + if (info->screen_base == NULL)
> + return;
>
> if (synchronous) {
> /* Flush any pending work to turn the console on, and then
> @@ -807,32 +827,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> +
> + if (ifbdev == NULL)
> + return;
>
> - async_synchronize_full();
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> + drm_fb_helper_hotplug_event(&ifbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
>
> - async_synchronize_full();
> - if (!ifbdev)
> + if (ifbdev == NULL)
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> }
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-31 12:00 ` Gabriel Feceoru
@ 2016-03-31 13:57 ` Chris Wilson
2016-03-31 13:57 ` [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev Chris Wilson
2016-03-31 16:05 ` [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation Joonas Lahtinen
0 siblings, 2 replies; 34+ messages in thread
From: Chris Wilson @ 2016-03-31 13:57 UTC (permalink / raw)
To: intel-gfx, Gabriel Feceoru
If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).
v3: To create a typo introduced when retyping
v4: Prevent info==NULL dereference in early boot
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Tested-by: Gabriel Feceoru <gabriel.feceoru@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_fbdev.c | 72 +++++++++++++++++++++++++-------------
1 file changed, 48 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 153ea7a3fcf6..5d4be71bdf22 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
dev_priv->fbdev = NULL;
}
+static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct fb_info *info;
+
+ if (dev_priv->fbdev == NULL)
+ return NULL;
+
+ info = dev_priv->fbdev->helper.fbdev;
+ if (info == NULL)
+ return NULL;
+
+ if (info->screen_base == NULL)
+ return NULL;
+
+ return dev_priv->fbdev;
+}
+
+static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
+{
+ struct intel_fbdev *ifbdev;
+
+ ifbdev = intel_fbdev_get(dev);
+ if (ifbdev == NULL)
+ return NULL;
+
+ if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
+ return NULL;
+
+ return ifbdev;
+}
+
void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct fb_info *info;
+ struct intel_fbdev *ifbdev;
- if (!ifbdev)
+ ifbdev = intel_fbdev_get(dev);
+ if (ifbdev == NULL)
return;
- info = ifbdev->helper.fbdev;
-
if (synchronous) {
/* Flush any pending work to turn the console on, and then
* wait to turn it off. It must be synchronous as we are
@@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
* been restored from swap. If the object is stolen however, it will be
* full of whatever garbage was left in there.
*/
- if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
+ if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
+ struct fb_info *info = ifbdev->helper.fbdev;
memset_io(info->screen_base, 0, info->screen_size);
+ }
drm_fb_helper_set_suspend(&ifbdev->helper, state);
console_unlock();
@@ -807,32 +839,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
+
+ if (ifbdev == NULL)
+ return;
- async_synchronize_full();
- if (dev_priv->fbdev)
- drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+ drm_fb_helper_hotplug_event(&ifbdev->helper);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
{
- int ret;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct drm_fb_helper *fb_helper;
+ struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
- async_synchronize_full();
- if (!ifbdev)
+ if (ifbdev == NULL)
return;
- fb_helper = &ifbdev->helper;
-
- ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
- if (ret) {
- DRM_DEBUG("failed to restore crtc mode\n");
- } else {
- mutex_lock(&fb_helper->dev->struct_mutex);
+ if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
+ mutex_lock(&dev->struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
- mutex_unlock(&fb_helper->dev->struct_mutex);
+ mutex_unlock(&dev->struct_mutex);
}
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev
2016-03-31 13:57 ` [PATCH v4 1/2] " Chris Wilson
@ 2016-03-31 13:57 ` Chris Wilson
2016-03-31 15:22 ` Joonas Lahtinen
2016-03-31 16:05 ` [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation Joonas Lahtinen
1 sibling, 1 reply; 34+ messages in thread
From: Chris Wilson @ 2016-03-31 13:57 UTC (permalink / raw)
To: intel-gfx, Gabriel Feceoru
Since the suspend_work is entirely internal to intel_fbdev.c, move it
from the top level drm_i915_private and into struct intel_fbdev. This
requires splitting the internal interface for the suspend worker from
the external interface used by the higher layers to initiate suspend.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.c | 4 +-
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/intel_drv.h | 5 +-
drivers/gpu/drm/i915/intel_fbdev.c | 117 ++++++++++++++++++++-----------------
4 files changed, 68 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 020a31c5e2bb..100d0d92b1e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -634,7 +634,7 @@ static int i915_drm_suspend(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
dev_priv->suspend_count++;
@@ -784,7 +784,7 @@ static int i915_drm_resume(struct drm_device *dev)
intel_opregion_init(dev);
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
mutex_lock(&dev_priv->modeset_restore_lock);
dev_priv->modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 820c91f551ba..973a602c5077 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1880,7 +1880,6 @@ struct drm_i915_private {
#ifdef CONFIG_DRM_FBDEV_EMULATION
/* list of fbdev register on this device */
struct intel_fbdev *fbdev;
- struct work_struct fbdev_suspend_work;
#endif
struct drm_property *broadcast_rgb_property;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6ac46d921cde..00b6c60c1cb8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -158,6 +158,7 @@ struct intel_framebuffer {
struct intel_fbdev {
struct drm_fb_helper helper;
struct intel_framebuffer *fb;
+ struct work_struct suspend_work;
int preferred_bpp;
};
@@ -1335,7 +1336,7 @@ void intel_dvo_init(struct drm_device *dev);
extern int intel_fbdev_init(struct drm_device *dev);
extern void intel_fbdev_initial_config_async(struct drm_device *dev);
extern void intel_fbdev_fini(struct drm_device *dev);
-extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
+extern void intel_fbdev_set_suspend(struct drm_device *dev, int state);
extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
extern void intel_fbdev_restore_mode(struct drm_device *dev);
#else
@@ -1352,7 +1353,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev)
{
}
-static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
+static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state)
{
}
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 5d4be71bdf22..66bb79613660 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -534,8 +534,7 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
.fb_probe = intelfb_create,
};
-static void intel_fbdev_destroy(struct drm_device *dev,
- struct intel_fbdev *ifbdev)
+static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
{
/* We rely on the object-free to release the VMA pinning for
* the info->screen_base mmaping. Leaking the VMA is simpler than
@@ -681,13 +680,56 @@ out:
return false;
}
+static void __intel_fbdev_set_suspend(struct intel_fbdev *ifbdev,
+ int state, bool synchronous)
+{
+ if (synchronous) {
+ /* Flush any pending work to turn the console on, and then
+ * wait to turn it off. It must be synchronous as we are
+ * about to suspend or unload the driver.
+ *
+ * Note that from within the work-handler, we cannot flush
+ * ourselves, so only flush outstanding work upon suspend!
+ */
+ if (state != FBINFO_STATE_RUNNING)
+ flush_work(&ifbdev->suspend_work);
+ console_lock();
+ } else {
+ /*
+ * The console lock can be pretty contented on resume due
+ * to all the printk activity. Try to keep it out of the hot
+ * path of resume if possible.
+ */
+ WARN_ON(state != FBINFO_STATE_RUNNING);
+ if (!console_trylock()) {
+ /* Don't block our own workqueue as this can
+ * be run in parallel with other i915.ko tasks.
+ */
+ schedule_work(&ifbdev->suspend_work);
+ return;
+ }
+ }
+
+ /* On resume from hibernation: If the object is shmemfs backed, it has
+ * been restored from swap. If the object is stolen however, it will be
+ * full of whatever garbage was left in there.
+ */
+ if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
+ struct fb_info *info = ifbdev->helper.fbdev;
+ memset_io(info->screen_base, 0, info->screen_size);
+ }
+
+ drm_fb_helper_set_suspend(&ifbdev->helper, state);
+ console_unlock();
+}
+
static void intel_fbdev_suspend_worker(struct work_struct *work)
{
- intel_fbdev_set_suspend(container_of(work,
- struct drm_i915_private,
- fbdev_suspend_work)->dev,
- FBINFO_STATE_RUNNING,
- true);
+ __intel_fbdev_set_suspend(container_of(work,
+ struct intel_fbdev,
+ suspend_work),
+ FBINFO_STATE_RUNNING,
+ true);
}
int intel_fbdev_init(struct drm_device *dev)
@@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev)
}
ifbdev->helper.atomic = true;
+ INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker);
dev_priv->fbdev = ifbdev;
- INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
@@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev)
void intel_fbdev_fini(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- if (!dev_priv->fbdev)
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_fbdev *ifbdev;
+
+ ifbdev = dev_priv->fbdev;
+ if (ifbdev == NULL)
return;
- flush_work(&dev_priv->fbdev_suspend_work);
+ dev_priv->fbdev = NULL;
if (!current_is_async())
async_synchronize_full();
- intel_fbdev_destroy(dev, dev_priv->fbdev);
- kfree(dev_priv->fbdev);
- dev_priv->fbdev = NULL;
+ flush_work(&ifbdev->suspend_work);
+
+ intel_fbdev_destroy(ifbdev);
+ kfree(ifbdev);
}
static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
@@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
return ifbdev;
}
-void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
+void intel_fbdev_set_suspend(struct drm_device *dev, int state)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_fbdev *ifbdev;
ifbdev = intel_fbdev_get(dev);
if (ifbdev == NULL)
return;
- if (synchronous) {
- /* Flush any pending work to turn the console on, and then
- * wait to turn it off. It must be synchronous as we are
- * about to suspend or unload the driver.
- *
- * Note that from within the work-handler, we cannot flush
- * ourselves, so only flush outstanding work upon suspend!
- */
- if (state != FBINFO_STATE_RUNNING)
- flush_work(&dev_priv->fbdev_suspend_work);
- console_lock();
- } else {
- /*
- * The console lock can be pretty contented on resume due
- * to all the printk activity. Try to keep it out of the hot
- * path of resume if possible.
- */
- WARN_ON(state != FBINFO_STATE_RUNNING);
- if (!console_trylock()) {
- /* Don't block our own workqueue as this can
- * be run in parallel with other i915.ko tasks.
- */
- schedule_work(&dev_priv->fbdev_suspend_work);
- return;
- }
- }
-
- /* On resume from hibernation: If the object is shmemfs backed, it has
- * been restored from swap. If the object is stolen however, it will be
- * full of whatever garbage was left in there.
- */
- if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
- struct fb_info *info = ifbdev->helper.fbdev;
- memset_io(info->screen_base, 0, info->screen_size);
- }
-
- drm_fb_helper_set_suspend(&ifbdev->helper, state);
- console_unlock();
+ __intel_fbdev_set_suspend(ifbdev, state,
+ state != FBINFO_STATE_RUNNING);
}
void intel_fbdev_output_poll_changed(struct drm_device *dev)
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev
2016-03-31 13:57 ` [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev Chris Wilson
@ 2016-03-31 15:22 ` Joonas Lahtinen
2016-03-31 15:30 ` Chris Wilson
0 siblings, 1 reply; 34+ messages in thread
From: Joonas Lahtinen @ 2016-03-31 15:22 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Gabriel Feceoru
On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> Since the suspend_work is entirely internal to intel_fbdev.c, move it
> from the top level drm_i915_private and into struct intel_fbdev. This
> requires splitting the internal interface for the suspend worker from
> the external interface used by the higher layers to initiate suspend.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4 +-
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/intel_drv.h | 5 +-
> drivers/gpu/drm/i915/intel_fbdev.c | 117 ++++++++++++++++++++-----------------
> 4 files changed, 68 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 020a31c5e2bb..100d0d92b1e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -634,7 +634,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> intel_uncore_forcewake_reset(dev, false);
> intel_opregion_fini(dev);
>
> - intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> + intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
>
> dev_priv->suspend_count++;
>
> @@ -784,7 +784,7 @@ static int i915_drm_resume(struct drm_device *dev)
>
> intel_opregion_init(dev);
>
> - intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> + intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
>
> mutex_lock(&dev_priv->modeset_restore_lock);
> dev_priv->modeset_restore = MODESET_DONE;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 820c91f551ba..973a602c5077 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1880,7 +1880,6 @@ struct drm_i915_private {
> #ifdef CONFIG_DRM_FBDEV_EMULATION
> /* list of fbdev register on this device */
> struct intel_fbdev *fbdev;
> - struct work_struct fbdev_suspend_work;
> #endif
>
> struct drm_property *broadcast_rgb_property;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6ac46d921cde..00b6c60c1cb8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -158,6 +158,7 @@ struct intel_framebuffer {
> struct intel_fbdev {
> struct drm_fb_helper helper;
> struct intel_framebuffer *fb;
> + struct work_struct suspend_work;
> int preferred_bpp;
> };
>
> @@ -1335,7 +1336,7 @@ void intel_dvo_init(struct drm_device *dev);
> extern int intel_fbdev_init(struct drm_device *dev);
> extern void intel_fbdev_initial_config_async(struct drm_device *dev);
> extern void intel_fbdev_fini(struct drm_device *dev);
> -extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
> +extern void intel_fbdev_set_suspend(struct drm_device *dev, int state);
> extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
> extern void intel_fbdev_restore_mode(struct drm_device *dev);
> #else
> @@ -1352,7 +1353,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev)
> {
> }
>
> -static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
> +static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> {
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 5d4be71bdf22..66bb79613660 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -534,8 +534,7 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> .fb_probe = intelfb_create,
> };
>
> -static void intel_fbdev_destroy(struct drm_device *dev,
> - struct intel_fbdev *ifbdev)
> +static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> {
> /* We rely on the object-free to release the VMA pinning for
> * the info->screen_base mmaping. Leaking the VMA is simpler than
> @@ -681,13 +680,56 @@ out:
> return false;
> }
>
> +static void __intel_fbdev_set_suspend(struct intel_fbdev *ifbdev,
> + int state, bool synchronous)
> +{
> + if (synchronous) {
> + /* Flush any pending work to turn the console on, and then
> + * wait to turn it off. It must be synchronous as we are
> + * about to suspend or unload the driver.
> + *
> + * Note that from within the work-handler, we cannot flush
> + * ourselves, so only flush outstanding work upon suspend!
> + */
> + if (state != FBINFO_STATE_RUNNING)
> + flush_work(&ifbdev->suspend_work);
> + console_lock();
> + } else {
> + /*
> + * The console lock can be pretty contented on resume due
> + * to all the printk activity. Try to keep it out of the hot
> + * path of resume if possible.
> + */
> + WARN_ON(state != FBINFO_STATE_RUNNING);
> + if (!console_trylock()) {
> + /* Don't block our own workqueue as this can
> + * be run in parallel with other i915.ko tasks.
> + */
> + schedule_work(&ifbdev->suspend_work);
> + return;
> + }
> + }
> +
> + /* On resume from hibernation: If the object is shmemfs backed, it has
> + * been restored from swap. If the object is stolen however, it will be
> + * full of whatever garbage was left in there.
> + */
> + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> + struct fb_info *info = ifbdev->helper.fbdev;
> + memset_io(info->screen_base, 0, info->screen_size);
> + }
> +
> + drm_fb_helper_set_suspend(&ifbdev->helper, state);
> + console_unlock();
> +}
> +
> static void intel_fbdev_suspend_worker(struct work_struct *work)
> {
> - intel_fbdev_set_suspend(container_of(work,
> - struct drm_i915_private,
> - fbdev_suspend_work)->dev,
> - FBINFO_STATE_RUNNING,
> - true);
> + __intel_fbdev_set_suspend(container_of(work,
> + struct intel_fbdev,
> + suspend_work),
Have the container_of on separate line at least, maybe even with a
macro.
> + FBINFO_STATE_RUNNING,
> + true);
> }
>
> int intel_fbdev_init(struct drm_device *dev)
> @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev)
> }
>
> ifbdev->helper.atomic = true;
> + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker);
>
> dev_priv->fbdev = ifbdev;
> - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
>
> drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
>
> @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev)
>
> void intel_fbdev_fini(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - if (!dev_priv->fbdev)
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_fbdev *ifbdev;
> +
> + ifbdev = dev_priv->fbdev;
Straight assignment.
> + if (ifbdev == NULL)
> return;
>
> - flush_work(&dev_priv->fbdev_suspend_work);
> + dev_priv->fbdev = NULL;
>
> if (!current_is_async())
> async_synchronize_full();
> - intel_fbdev_destroy(dev, dev_priv->fbdev);
> - kfree(dev_priv->fbdev);
> - dev_priv->fbdev = NULL;
> + flush_work(&ifbdev->suspend_work);
> +
> + intel_fbdev_destroy(ifbdev);
> + kfree(ifbdev);
> }
>
> static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> return ifbdev;
> }
>
> -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
> +void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_fbdev *ifbdev;
>
> ifbdev = intel_fbdev_get(dev);
> if (ifbdev == NULL)
> return;
>
> - if (synchronous) {
> - /* Flush any pending work to turn the console on, and then
> - * wait to turn it off. It must be synchronous as we are
> - * about to suspend or unload the driver.
> - *
> - * Note that from within the work-handler, we cannot flush
> - * ourselves, so only flush outstanding work upon suspend!
> - */
> - if (state != FBINFO_STATE_RUNNING)
> - flush_work(&dev_priv->fbdev_suspend_work);
> - console_lock();
> - } else {
> - /*
> - * The console lock can be pretty contented on resume due
> - * to all the printk activity. Try to keep it out of the hot
> - * path of resume if possible.
> - */
> - WARN_ON(state != FBINFO_STATE_RUNNING);
> - if (!console_trylock()) {
> - /* Don't block our own workqueue as this can
> - * be run in parallel with other i915.ko tasks.
> - */
> - schedule_work(&dev_priv->fbdev_suspend_work);
> - return;
> - }
> - }
> -
> - /* On resume from hibernation: If the object is shmemfs backed, it has
> - * been restored from swap. If the object is stolen however, it will be
> - * full of whatever garbage was left in there.
> - */
> - if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> - struct fb_info *info = ifbdev->helper.fbdev;
> - memset_io(info->screen_base, 0, info->screen_size);
> - }
> -
> - drm_fb_helper_set_suspend(&ifbdev->helper, state);
> - console_unlock();
> + __intel_fbdev_set_suspend(ifbdev, state,
> + state != FBINFO_STATE_RUNNING);
Could have changed this function name to make it easier to spot what
you changed or not. Code motion as separate patches.
With above addressed,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> }
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev
2016-03-31 15:22 ` Joonas Lahtinen
@ 2016-03-31 15:30 ` Chris Wilson
2016-03-31 15:56 ` Joonas Lahtinen
0 siblings, 1 reply; 34+ messages in thread
From: Chris Wilson @ 2016-03-31 15:30 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Thu, Mar 31, 2016 at 06:22:01PM +0300, Joonas Lahtinen wrote:
> On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > static void intel_fbdev_suspend_worker(struct work_struct *work)
> > {
> > - intel_fbdev_set_suspend(container_of(work,
> > - struct drm_i915_private,
> > - fbdev_suspend_work)->dev,
> > - FBINFO_STATE_RUNNING,
> > - true);
> > + __intel_fbdev_set_suspend(container_of(work,
> > + struct intel_fbdev,
> > + suspend_work),
>
> Have the container_of on separate line at least, maybe even with a
> macro.
Sure.
> > + FBINFO_STATE_RUNNING,
> > + true);
> > }
> >
> > int intel_fbdev_init(struct drm_device *dev)
> > @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev)
> > }
> >
> > ifbdev->helper.atomic = true;
> > + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker);
> >
> > dev_priv->fbdev = ifbdev;
> > - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
> >
> > drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
> >
> > @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev)
> >
> > void intel_fbdev_fini(struct drm_device *dev)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - if (!dev_priv->fbdev)
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct intel_fbdev *ifbdev;
> > +
> > + ifbdev = dev_priv->fbdev;
>
> Straight assignment.
>
> > + if (ifbdev == NULL)
I was trying to group the local assignment with the if, and I like to leave
whitespace after the locals. By placing these two lines together this
function looks like the majority of the other functions.
> > return;
> >
> > - flush_work(&dev_priv->fbdev_suspend_work);
> > + dev_priv->fbdev = NULL;
> >
> > if (!current_is_async())
> > async_synchronize_full();
> > - intel_fbdev_destroy(dev, dev_priv->fbdev);
> > - kfree(dev_priv->fbdev);
> > - dev_priv->fbdev = NULL;
> > + flush_work(&ifbdev->suspend_work);
> > +
> > + intel_fbdev_destroy(ifbdev);
> > + kfree(ifbdev);
> > }
> >
> > static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> > @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> > return ifbdev;
> > }
> >
> > -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
> > +void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > struct intel_fbdev *ifbdev;
> >
> > ifbdev = intel_fbdev_get(dev);
> > if (ifbdev == NULL)
> > return;
> >
> > - if (synchronous) {
> > - /* Flush any pending work to turn the console on, and then
> > - * wait to turn it off. It must be synchronous as we are
> > - * about to suspend or unload the driver.
> > - *
> > - * Note that from within the work-handler, we cannot flush
> > - * ourselves, so only flush outstanding work upon suspend!
> > - */
> > - if (state != FBINFO_STATE_RUNNING)
> > - flush_work(&dev_priv->fbdev_suspend_work);
> > - console_lock();
> > - } else {
> > - /*
> > - * The console lock can be pretty contented on resume due
> > - * to all the printk activity. Try to keep it out of the hot
> > - * path of resume if possible.
> > - */
> > - WARN_ON(state != FBINFO_STATE_RUNNING);
> > - if (!console_trylock()) {
> > - /* Don't block our own workqueue as this can
> > - * be run in parallel with other i915.ko tasks.
> > - */
> > - schedule_work(&dev_priv->fbdev_suspend_work);
> > - return;
> > - }
> > - }
> > -
> > - /* On resume from hibernation: If the object is shmemfs backed, it has
> > - * been restored from swap. If the object is stolen however, it will be
> > - * full of whatever garbage was left in there.
> > - */
> > - if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> > - struct fb_info *info = ifbdev->helper.fbdev;
> > - memset_io(info->screen_base, 0, info->screen_size);
> > - }
> > -
> > - drm_fb_helper_set_suspend(&ifbdev->helper, state);
> > - console_unlock();
> > + __intel_fbdev_set_suspend(ifbdev, state,
> > + state != FBINFO_STATE_RUNNING);
>
> Could have changed this function name to make it easier to spot what
> you changed or not. Code motion as separate patches.
This was a separate patch! The purpose of this patch was to move
code/data about :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev
2016-03-31 15:30 ` Chris Wilson
@ 2016-03-31 15:56 ` Joonas Lahtinen
0 siblings, 0 replies; 34+ messages in thread
From: Joonas Lahtinen @ 2016-03-31 15:56 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On to, 2016-03-31 at 16:30 +0100, Chris Wilson wrote:
> On Thu, Mar 31, 2016 at 06:22:01PM +0300, Joonas Lahtinen wrote:
> >
> > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > >
> > > static void intel_fbdev_suspend_worker(struct work_struct *work)
> > > {
> > > - intel_fbdev_set_suspend(container_of(work,
> > > - struct drm_i915_private,
> > > - fbdev_suspend_work)->dev,
> > > - FBINFO_STATE_RUNNING,
> > > - true);
> > > + __intel_fbdev_set_suspend(container_of(work,
> > > + struct intel_fbdev,
> > > + suspend_work),
> > Have the container_of on separate line at least, maybe even with a
> > macro.
> Sure.
>
> >
> > >
> > > + FBINFO_STATE_RUNNING,
> > > + true);
> > > }
> > >
> > > int intel_fbdev_init(struct drm_device *dev)
> > > @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev)
> > > }
> > >
> > > ifbdev->helper.atomic = true;
> > > + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker);
> > >
> > > dev_priv->fbdev = ifbdev;
> > > - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
> > >
> > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
> > >
> > > @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev)
> > >
> > > void intel_fbdev_fini(struct drm_device *dev)
> > > {
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > - if (!dev_priv->fbdev)
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > + struct intel_fbdev *ifbdev;
> > > +
> > > + ifbdev = dev_priv->fbdev;
> > Straight assignment.
> >
> > >
> > > + if (ifbdev == NULL)
> I was trying to group the local assignment with the if, and I like to leave
> whitespace after the locals. By placing these two lines together this
> function looks like the majority of the other functions.
>
> >
> > >
> > > return;
> > >
> > > - flush_work(&dev_priv->fbdev_suspend_work);
> > > + dev_priv->fbdev = NULL;
> > >
> > > if (!current_is_async())
> > > async_synchronize_full();
> > > - intel_fbdev_destroy(dev, dev_priv->fbdev);
> > > - kfree(dev_priv->fbdev);
> > > - dev_priv->fbdev = NULL;
> > > + flush_work(&ifbdev->suspend_work);
> > > +
> > > + intel_fbdev_destroy(ifbdev);
> > > + kfree(ifbdev);
> > > }
> > >
> > > static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> > > @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> > > return ifbdev;
> > > }
> > >
> > > -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
> > > +void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> > > {
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct intel_fbdev *ifbdev;
> > >
> > > ifbdev = intel_fbdev_get(dev);
> > > if (ifbdev == NULL)
> > > return;
> > >
> > > - if (synchronous) {
> > > - /* Flush any pending work to turn the console on, and then
> > > - * wait to turn it off. It must be synchronous as we are
> > > - * about to suspend or unload the driver.
> > > - *
> > > - * Note that from within the work-handler, we cannot flush
> > > - * ourselves, so only flush outstanding work upon suspend!
> > > - */
> > > - if (state != FBINFO_STATE_RUNNING)
> > > - flush_work(&dev_priv->fbdev_suspend_work);
> > > - console_lock();
> > > - } else {
> > > - /*
> > > - * The console lock can be pretty contented on resume due
> > > - * to all the printk activity. Try to keep it out of the hot
> > > - * path of resume if possible.
> > > - */
> > > - WARN_ON(state != FBINFO_STATE_RUNNING);
> > > - if (!console_trylock()) {
> > > - /* Don't block our own workqueue as this can
> > > - * be run in parallel with other i915.ko tasks.
> > > - */
> > > - schedule_work(&dev_priv->fbdev_suspend_work);
> > > - return;
> > > - }
> > > - }
> > > -
> > > - /* On resume from hibernation: If the object is shmemfs backed, it has
> > > - * been restored from swap. If the object is stolen however, it will be
> > > - * full of whatever garbage was left in there.
> > > - */
> > > - if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> > > - struct fb_info *info = ifbdev->helper.fbdev;
> > > - memset_io(info->screen_base, 0, info->screen_size);
> > > - }
> > > -
> > > - drm_fb_helper_set_suspend(&ifbdev->helper, state);
> > > - console_unlock();
> > > + __intel_fbdev_set_suspend(ifbdev, state,
> > > + state != FBINFO_STATE_RUNNING);
> > Could have changed this function name to make it easier to spot what
> > you changed or not. Code motion as separate patches.
> This was a separate patch! The purpose of this patch was to move
> code/data about :)
It also changed order of some stuff, so I got suspicious ;) But looks
like the function stayed the same, so OK for me.
> -Chris
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-31 13:57 ` [PATCH v4 1/2] " Chris Wilson
2016-03-31 13:57 ` [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev Chris Wilson
@ 2016-03-31 16:05 ` Joonas Lahtinen
2016-03-31 16:13 ` Chris Wilson
1 sibling, 1 reply; 34+ messages in thread
From: Joonas Lahtinen @ 2016-03-31 16:05 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Gabriel Feceoru
On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure. Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> v3: To create a typo introduced when retyping
> v4: Prevent info==NULL dereference in early boot
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Tested-by: Gabriel Feceoru <gabriel.feceoru@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 72 +++++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 153ea7a3fcf6..5d4be71bdf22 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
> dev_priv->fbdev = NULL;
> }
>
> +static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct fb_info *info;
> +
> + if (dev_priv->fbdev == NULL)
> + return NULL;
> +
> + info = dev_priv->fbdev->helper.fbdev;
> + if (info == NULL)
> + return NULL;
> +
> + if (info->screen_base == NULL)
> + return NULL;
> +
This is rather verbose to my liking, I'd rather be dropping those '==
NULL' and convert to !. But either way to me.
> + return dev_priv->fbdev;
> +}
> +
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> + struct intel_fbdev *ifbdev;
> +
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
> + return NULL;
> +
> + if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
> + return NULL;
> +
> + return ifbdev;
> +}
> +
> void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct fb_info *info;
> + struct intel_fbdev *ifbdev;
>
> - if (!ifbdev)
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
> return;
>
> - info = ifbdev->helper.fbdev;
> -
> if (synchronous) {
> /* Flush any pending work to turn the console on, and then
> * wait to turn it off. It must be synchronous as we are
> @@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> * been restored from swap. If the object is stolen however, it will be
> * full of whatever garbage was left in there.
> */
> - if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> + struct fb_info *info = ifbdev->helper.fbdev;
> memset_io(info->screen_base, 0, info->screen_size);
> + }
>
> drm_fb_helper_set_suspend(&ifbdev->helper, state);
> console_unlock();
> @@ -807,32 +839,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> +
> + if (ifbdev == NULL)
> + return;
>
> - async_synchronize_full();
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> + drm_fb_helper_hotplug_event(&ifbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
>
> - async_synchronize_full();
What's with the async_synchronize_full() begin removed completely?
> - if (!ifbdev)
> + if (ifbdev == NULL)
Argh.
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
Above addressed,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> }
> }
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-31 16:05 ` [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation Joonas Lahtinen
@ 2016-03-31 16:13 ` Chris Wilson
2016-03-31 16:28 ` Joonas Lahtinen
2016-03-31 16:30 ` Lukas Wunner
0 siblings, 2 replies; 34+ messages in thread
From: Chris Wilson @ 2016-03-31 16:13 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Thu, Mar 31, 2016 at 07:05:21PM +0300, Joonas Lahtinen wrote:
> On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > void intel_fbdev_restore_mode(struct drm_device *dev)
> > {
> > - int ret;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > - struct drm_fb_helper *fb_helper;
> > + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> >
> > - async_synchronize_full();
>
> What's with the async_synchronize_full() begin removed completely?
Because it's not just wrong, but completely broken imo.
During suspend, we want to freeze the async task not flush. Then here
and during resume we skip the restoration of the unregistered fbdev, and
then when the task is woken it can complete the registration and present
the vanilla ifbdev.
During hibernation, we really just want to cancel the task and start
from scratch on resume.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-31 16:13 ` Chris Wilson
@ 2016-03-31 16:28 ` Joonas Lahtinen
2016-03-31 16:30 ` Lukas Wunner
1 sibling, 0 replies; 34+ messages in thread
From: Joonas Lahtinen @ 2016-03-31 16:28 UTC (permalink / raw)
To: Chris Wilson, Lukas Wunner, Daniel Vetter; +Cc: intel-gfx
On to, 2016-03-31 at 17:13 +0100, Chris Wilson wrote:
> On Thu, Mar 31, 2016 at 07:05:21PM +0300, Joonas Lahtinen wrote:
> >
> > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > >
> > > void intel_fbdev_restore_mode(struct drm_device *dev)
> > > {
> > > - int ret;
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > > - struct drm_fb_helper *fb_helper;
> > > + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> > >
> > > - async_synchronize_full();
> > What's with the async_synchronize_full() begin removed completely?
> Because it's not just wrong, but completely broken imo.
>
> During suspend, we want to freeze the async task not flush. Then here
> and during resume we skip the restoration of the unregistered fbdev, and
> then when the task is woken it can complete the registration and present
> the vanilla ifbdev.
>
> During hibernation, we really just want to cancel the task and start
> from scratch on resume.
Maybe Ack from Lukas with those? As this is effectively a revert of :
commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
Author: Lukas Wunner <lukas@wunner.de>
Date: Wed Mar 9 12:52:53 2016 +0100
drm/i915: Fix races on fbdev
Committed without R-b by Daniel, so maybe he has a comment on it too.
Regards, Joonas
> -Chris
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation
2016-03-31 16:13 ` Chris Wilson
2016-03-31 16:28 ` Joonas Lahtinen
@ 2016-03-31 16:30 ` Lukas Wunner
1 sibling, 0 replies; 34+ messages in thread
From: Lukas Wunner @ 2016-03-31 16:30 UTC (permalink / raw)
To: Chris Wilson, Joonas Lahtinen, intel-gfx, Gabriel Feceoru
Hi Chris,
On Thu, Mar 31, 2016 at 05:13:55PM +0100, Chris Wilson wrote:
> On Thu, Mar 31, 2016 at 07:05:21PM +0300, Joonas Lahtinen wrote:
> > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > > void intel_fbdev_restore_mode(struct drm_device *dev)
> > > {
> > > - int ret;
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > > - struct drm_fb_helper *fb_helper;
> > > + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> > >
> > > - async_synchronize_full();
> >
> > What's with the async_synchronize_full() begin removed completely?
>
> Because it's not just wrong, but completely broken imo.
>
> During suspend, we want to freeze the async task not flush.
> Then here and during resume we skip the restoration of the unregistered
> fbdev, and then when the task is woken it can complete the registration
> and present the vanilla ifbdev.
No. The fbdev initialization is guaranteed to have finished before
suspend. So "we want to freeze the async task" is wrong thinking.
There is no async task to freeze.
Please read the commit message of a7442b93cf32 ("drm/i915: Fix races
on fbdev"):
"a device is never suspended until its ->probe callback (and all
asynchronous tasks it scheduled) have finished. See dpm_prepare(),
which calls wait_for_device_probe(), which calls async_synchronize_full()."
Best regards,
Lukas
> During hibernation, we really just want to cancel the task and start
> from scratch on resume.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-30 17:20 [REGRESSION] system hang on ILK/SNB/IVB Gabriel Feceoru
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
@ 2016-03-30 18:47 ` Daniel Vetter
2016-03-30 21:35 ` Lukas Wunner
2016-03-31 14:42 ` ✗ Fi.CI.BAT: failure for drm/i915: Protect fbdev across slow or failed initialisation (rev2) Patchwork
3 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2016-03-30 18:47 UTC (permalink / raw)
To: Gabriel Feceoru; +Cc: Tomi Sarvela, daniel.vetter, intel-gfx
On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> This commit causes a hang while running kms suspend tests
> (kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
>
> Probably the same problem with the one in v2, but on older HW.
I did check the patchwork/BAT-CI results and it looked clean. Is this a
new machine? Should I just revert for now until we have a proper fix?
-Daniel
>
>
> commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> Author: Lukas Wunner <lukas@wunner.de>
> Date: Wed Mar 9 12:52:53 2016 +0100
>
> drm/i915: Fix races on fbdev
>
> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> been witnessed to run before intel_fbdev_initial_config_async()
> has finished.
>
> We might likewise receive hotplug events before we've had a chance to
> fully set up the fbdev.
>
> Fix by waiting for the asynchronous thread to finish.
>
> v2:
> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
> in v1 which turned out to be entirely gratuitous. It caused a deadlock
> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
> for CI support) and was unnecessary since a device is never suspended
> until its ->probe callback (and all asynchronous tasks it scheduled)
> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> which calls async_synchronize_full().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Link: http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D3@gabe.freedesktop.org
>
>
> Regards,
> Gabriel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-30 17:20 [REGRESSION] system hang on ILK/SNB/IVB Gabriel Feceoru
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
2016-03-30 18:47 ` [REGRESSION] system hang on ILK/SNB/IVB Daniel Vetter
@ 2016-03-30 21:35 ` Lukas Wunner
2016-03-31 7:21 ` Tomi Sarvela
2016-03-31 7:42 ` Gabriel Feceoru
2016-03-31 14:42 ` ✗ Fi.CI.BAT: failure for drm/i915: Protect fbdev across slow or failed initialisation (rev2) Patchwork
3 siblings, 2 replies; 34+ messages in thread
From: Lukas Wunner @ 2016-03-30 21:35 UTC (permalink / raw)
To: Gabriel Feceoru; +Cc: Tomi Sarvela, daniel.vetter, intel-gfx
Hi Gabriel,
On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> This commit causes a hang while running kms suspend tests
> (kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
This happened with v1 but not with v2 of the patch.
Please check if somehow v1 ended up in your tree.
v2 passed CI fine, save for one warning not caused by the patch:
https://patchwork.freedesktop.org/series/4068/
For comparison, this was v1:
https://patchwork.freedesktop.org/patch/75840/
Best regards,
Lukas
>
> Probably the same problem with the one in v2, but on older HW.
>
>
> commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> Author: Lukas Wunner <lukas@wunner.de>
> Date: Wed Mar 9 12:52:53 2016 +0100
>
> drm/i915: Fix races on fbdev
>
> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> been witnessed to run before intel_fbdev_initial_config_async()
> has finished.
>
> We might likewise receive hotplug events before we've had a chance to
> fully set up the fbdev.
>
> Fix by waiting for the asynchronous thread to finish.
>
> v2:
> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
> in v1 which turned out to be entirely gratuitous. It caused a deadlock
> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
> for CI support) and was unnecessary since a device is never suspended
> until its ->probe callback (and all asynchronous tasks it scheduled)
> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> which calls async_synchronize_full().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Link: http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D3@gabe.freedesktop.org
>
>
> Regards,
> Gabriel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-30 21:35 ` Lukas Wunner
@ 2016-03-31 7:21 ` Tomi Sarvela
2016-03-31 20:35 ` Lukas Wunner
2016-03-31 7:42 ` Gabriel Feceoru
1 sibling, 1 reply; 34+ messages in thread
From: Tomi Sarvela @ 2016-03-31 7:21 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx, daniel.vetter
Hello Lukas,
The problem with the results in your link is that there is no HSW, ILK, IVB or
SNB results. This might give the impression that everything is well.
Most damning is lack of HSW-gt2 and SNB-dellxps: those machines hang on to
APC, and have run quite stably for every Patchwork run. The case isn't strong
enough yet that series should fail if either of those won't run, but it might
be so in future.
Tomi
On Wednesday 30 March 2016 23:35:08 Lukas Wunner wrote:
> Hi Gabriel,
>
> On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> > This commit causes a hang while running kms suspend tests
> > (kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
>
> This happened with v1 but not with v2 of the patch.
> Please check if somehow v1 ended up in your tree.
>
> v2 passed CI fine, save for one warning not caused by the patch:
> https://patchwork.freedesktop.org/series/4068/
>
> For comparison, this was v1:
> https://patchwork.freedesktop.org/patch/75840/
>
> Best regards,
>
> Lukas
>
> > Probably the same problem with the one in v2, but on older HW.
> >
> >
> > commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> > Author: Lukas Wunner <lukas@wunner.de>
> > Date: Wed Mar 9 12:52:53 2016 +0100
> >
> > drm/i915: Fix races on fbdev
> >
> > The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> > been witnessed to run before intel_fbdev_initial_config_async()
> > has finished.
> >
> > We might likewise receive hotplug events before we've had a chance to
> > fully set up the fbdev.
> >
> > Fix by waiting for the asynchronous thread to finish.
> >
> > v2:
> > An async_synchronize_full() was also added to
> > intel_fbdev_set_suspend()
> > in v1 which turned out to be entirely gratuitous. It caused a deadlock
> > on suspend (discovered by CI, thanks to Damien Lespiau and Tomi
> > Sarvela
> > for CI support) and was unnecessary since a device is never suspended
> > until its ->probe callback (and all asynchronous tasks it scheduled)
> > have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> > which calls async_synchronize_full().
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> > Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Link:
> > http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D
> > 3@gabe.freedesktop.org>
> > Regards,
> > Gabriel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-31 7:21 ` Tomi Sarvela
@ 2016-03-31 20:35 ` Lukas Wunner
2016-04-01 7:59 ` Tomi Sarvela
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wunner @ 2016-03-31 20:35 UTC (permalink / raw)
To: Tomi Sarvela; +Cc: intel-gfx, daniel.vetter
Hi Tomi,
On Thu, Mar 31, 2016 at 10:21:16AM +0300, Tomi Sarvela wrote:
> The problem with the results in your link is that there is no HSW, ILK, IVB
> or SNB results. This might give the impression that everything is well.
>
> Most damning is lack of HSW-gt2 and SNB-dellxps: those machines hang on to
> APC, and have run quite stably for every Patchwork run. The case isn't strong
> enough yet that series should fail if either of those won't run, but it might
> be so in future.
So my patch seeking to fix the hangs has passed Romania CI with "success":
https://patchwork.freedesktop.org/series/5125/
However I don't see HSW-gt2 and SNB-dellxps in their hardware lineup.
And I would still like to know what the actual cause of the hangs is
since they do not occur on my IVB laptop.
If you get the chance maybe you can repeat the test and include a
"dump_stack();" at the beginning of intel_fbdev_output_poll_changed()
and intel_fbdev_restore_mode(). This should show in the logs which of
the two functions is called during suspend/resume and from where it's
called. My guess is that this particular hardware causes a hotplug
signal to be generated upon waking up. If the hangs do not stop with
this patch, booting with "no_console_suspend" should at least show
what's going on.
Thank you & best regards,
Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-31 20:35 ` Lukas Wunner
@ 2016-04-01 7:59 ` Tomi Sarvela
0 siblings, 0 replies; 34+ messages in thread
From: Tomi Sarvela @ 2016-04-01 7:59 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx, daniel.vetter
Hi Lukas,
Ran this patch through the farm, and it seems that this patch might've helped
HSW, maybe even BSW.
ILK, IVB and SNB are still hanging hard to the same igt-test,
kms_pipe_crc_basic@suspend-read-crc-pipe-?
I'll try to make a kernel with the changes you proposed, but as I'm not
familiar with the driver innards, it might take a while.
Tomi
On Thursday 31 March 2016 22:35:17 Lukas Wunner wrote:
> Hi Tomi,
>
> On Thu, Mar 31, 2016 at 10:21:16AM +0300, Tomi Sarvela wrote:
> > The problem with the results in your link is that there is no HSW, ILK,
> > IVB
> > or SNB results. This might give the impression that everything is well.
> >
> > Most damning is lack of HSW-gt2 and SNB-dellxps: those machines hang on to
> > APC, and have run quite stably for every Patchwork run. The case isn't
> > strong enough yet that series should fail if either of those won't run,
> > but it might be so in future.
>
> So my patch seeking to fix the hangs has passed Romania CI with "success":
> https://patchwork.freedesktop.org/series/5125/
>
> However I don't see HSW-gt2 and SNB-dellxps in their hardware lineup.
> And I would still like to know what the actual cause of the hangs is
> since they do not occur on my IVB laptop.
>
> If you get the chance maybe you can repeat the test and include a
> "dump_stack();" at the beginning of intel_fbdev_output_poll_changed()
> and intel_fbdev_restore_mode(). This should show in the logs which of
> the two functions is called during suspend/resume and from where it's
> called. My guess is that this particular hardware causes a hotplug
> signal to be generated upon waking up. If the hangs do not stop with
> this patch, booting with "no_console_suspend" should at least show
> what's going on.
>
> Thank you & best regards,
>
> Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-30 21:35 ` Lukas Wunner
2016-03-31 7:21 ` Tomi Sarvela
@ 2016-03-31 7:42 ` Gabriel Feceoru
2016-03-31 15:23 ` Lukas Wunner
1 sibling, 1 reply; 34+ messages in thread
From: Gabriel Feceoru @ 2016-03-31 7:42 UTC (permalink / raw)
To: Lukas Wunner; +Cc: Tomi Sarvela, daniel.vetter, intel-gfx
On 31.03.2016 00:35, Lukas Wunner wrote:
> Hi Gabriel,
>
> On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
>> This commit causes a hang while running kms suspend tests
>> (kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
>
> This happened with v1 but not with v2 of the patch.
> Please check if somehow v1 ended up in your tree.
It's v2.
Tomi already replied, meantime I also looked at the results.
The current regression is for ILK/SNB/IVB only (v1 seemed to affect more
platforms).
Unfortunately these machines were not available when v2 was tested, so
this couldn't be detected.
Regards,
Gabriel.
>
> v2 passed CI fine, save for one warning not caused by the patch:
> https://patchwork.freedesktop.org/series/4068/
>
> For comparison, this was v1:
> https://patchwork.freedesktop.org/patch/75840/
>
> Best regards,
>
> Lukas
>
>>
>> Probably the same problem with the one in v2, but on older HW.
>>
>>
>> commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
>> Author: Lukas Wunner <lukas@wunner.de>
>> Date: Wed Mar 9 12:52:53 2016 +0100
>>
>> drm/i915: Fix races on fbdev
>>
>> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
>> been witnessed to run before intel_fbdev_initial_config_async()
>> has finished.
>>
>> We might likewise receive hotplug events before we've had a chance to
>> fully set up the fbdev.
>>
>> Fix by waiting for the asynchronous thread to finish.
>>
>> v2:
>> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
>> in v1 which turned out to be entirely gratuitous. It caused a deadlock
>> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
>> for CI support) and was unnecessary since a device is never suspended
>> until its ->probe callback (and all asynchronous tasks it scheduled)
>> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
>> which calls async_synchronize_full().
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
>> Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
>> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Link: http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D3@gabe.freedesktop.org
>>
>>
>> Regards,
>> Gabriel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [REGRESSION] system hang on ILK/SNB/IVB
2016-03-31 7:42 ` Gabriel Feceoru
@ 2016-03-31 15:23 ` Lukas Wunner
0 siblings, 0 replies; 34+ messages in thread
From: Lukas Wunner @ 2016-03-31 15:23 UTC (permalink / raw)
To: Gabriel Feceoru; +Cc: Tomi Sarvela, daniel.vetter, intel-gfx
Hi Gabriel,
On Thu, Mar 31, 2016 at 10:42:37AM +0300, Gabriel Feceoru wrote:
> On 31.03.2016 00:35, Lukas Wunner wrote:
> >On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> >>This commit causes a hang while running kms suspend tests
> >>(kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
>
> Tomi already replied, meantime I also looked at the results.
> The current regression is for ILK/SNB/IVB only (v1 seemed to affect more
> platforms).
> Unfortunately these machines were not available when v2 was tested, so this
> couldn't be detected.
I dev on an IVB machine and cannot reproduce this. Suspend works fine.
All the patch does is call async_synchronize_full()
(1) when a hotplug event arrives or
(2) when the last DRM client closes the connection.
Either of these two things seems to be happening on your test machines
when running the suspend test.
The PM core suspends and resumes individual devices asynchronously and
calls async_synchronize_full() in a couple of places. If a device's PM
callbacks also call async_synchronize_full(), the machine deadlocks.
It is unnecessary that we call async_synchronize_full(), we only need
to synchronize up to a specific cookie (which represents initialization
of the fbdev). So I've just posted a patch to replace the calls to
async_synchronize_full() with async_synchronize_cookie(). This should
make things less fragile and hopefully also solve the hangs you're seeing.
Best regards,
Lukas
> >>
> >>Probably the same problem with the one in v2, but on older HW.
> >>
> >>
> >>commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> >>Author: Lukas Wunner <lukas@wunner.de>
> >>Date: Wed Mar 9 12:52:53 2016 +0100
> >>
> >> drm/i915: Fix races on fbdev
> >>
> >> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> >> been witnessed to run before intel_fbdev_initial_config_async()
> >> has finished.
> >>
> >> We might likewise receive hotplug events before we've had a chance to
> >> fully set up the fbdev.
> >>
> >> Fix by waiting for the asynchronous thread to finish.
> >>
> >> v2:
> >> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
> >> in v1 which turned out to be entirely gratuitous. It caused a deadlock
> >> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
> >> for CI support) and was unnecessary since a device is never suspended
> >> until its ->probe callback (and all asynchronous tasks it scheduled)
> >> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> >> which calls async_synchronize_full().
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> >> Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com>
> >> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Link: http://patchwork.freedesktop.org/patch/msgid/20160309115147.67B2B6E0D3@gabe.freedesktop.org
> >>
> >>
> >>Regards,
> >>Gabriel
> >v2 passed CI fine, save for one warning not caused by the patch:
> >https://patchwork.freedesktop.org/series/4068/
> >
> >For comparison, this was v1:
> >https://patchwork.freedesktop.org/patch/75840/
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Protect fbdev across slow or failed initialisation (rev2)
2016-03-30 17:20 [REGRESSION] system hang on ILK/SNB/IVB Gabriel Feceoru
` (2 preceding siblings ...)
2016-03-30 21:35 ` Lukas Wunner
@ 2016-03-31 14:42 ` Patchwork
3 siblings, 0 replies; 34+ messages in thread
From: Patchwork @ 2016-03-31 14:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Protect fbdev across slow or failed initialisation (rev2)
URL : https://patchwork.freedesktop.org/series/5065/
State : failure
== Summary ==
Series 5065v2 drm/i915: Protect fbdev across slow or failed initialisation
http://patchwork.freedesktop.org/api/1.0/series/5065/revisions/2/mbox/
Test drv_module_reload_basic:
pass -> INCOMPLETE (bsw-nuc-2)
pass -> INCOMPLETE (skl-nuci5)
pass -> INCOMPLETE (byt-nuc)
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (bsw-nuc-2)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
dmesg-warn -> PASS (bsw-nuc-2)
Test pm_rpm:
Subgroup basic-rte:
pass -> DMESG-WARN (bsw-nuc-2)
bsw-nuc-2 total:154 pass:122 dwarn:2 dfail:0 fail:0 skip:29
byt-nuc total:57 pass:46 dwarn:0 dfail:0 fail:0 skip:10
hsw-brixbox total:196 pass:174 dwarn:0 dfail:0 fail:0 skip:22
skl-nuci5 total:25 pass:22 dwarn:0 dfail:0 fail:0 skip:2
Results at /archive/results/CI_IGT_test/Patchwork_1761/
03c0f854e93263563f559d2bc8e47fb51adae697 drm-intel-nightly: 2016y-03m-31d-10h-50m-15s UTC integration manifest
ab3a2f88b9f8b6a3109c325d42770c3c5fd714a4 drm/i915: Protect fbdev across slow or failed initialisation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
@ 2016-02-04 9:21 Li, Weinan Z
2016-02-05 0:27 ` Lukas Wunner
0 siblings, 1 reply; 34+ messages in thread
From: Li, Weinan Z @ 2016-02-04 9:21 UTC (permalink / raw)
To: gustav.fagerlind, Lukas Wunner; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 5982 bytes --]
Hi Wilson,
We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails") this 2 patches can’t cover this case. It’s access ifbdev->fb before the initialization
finished, but not initialization failed. If don’t have any other patches or code update relative, it may still have in 4.5.
add info NULL check should be better, it is also initialized in the async queue
> info = ifbdev->helper.fbdev;
> + if (info == NULL)
> + return false;
> if (!info->screen_base)
BRs,
Weinan Li
From: Li, Weinan Z
Sent: Thursday, February 04, 2016 10:34 AM
To: 'gustav.fagerlind@gmail.com'; Lukas Wunner
Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
Thanks for your quick response.
Yes it is not easily be reproduced in native. In iVGT we startup several VMs simultaneously, it can be reproduced in several cycles, upon 1/10 fail rate.
Need to use GUI mode but not text mode to reproduce this issue.
BRs,
Weinan Li
From: Gustav Fägerlind [mailto:gustav.fagerlind@gmail.com]
Sent: Thursday, February 04, 2016 1:08 AM
To: Lukas Wunner
Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>; Li, Weinan Z
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
Cool, thank you.
I dont believe I can easily reproduce it, it has only happend few times (and i reboot my lappy >2 times per day).
//
Gustav
2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de<mailto:lukas@wunner.de>>:
Hi,
On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure.
This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails").
Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
interesting to know if it can be reproduced at all with 4.5-rc2.
Best regards,
Lukas
> Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com<mailto:weinan.z.li@intel.com>>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4380f9..6218bc5370a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> .fb_debug_leave = drm_fb_helper_debug_leave,
> };
>
> +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> +{
> + struct fb_info *info;
> +
> + if (ifbdev == NULL)
> + return false;
> +
> + info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return false;
> +
> + return info->state == FBINFO_STATE_RUNNING;
> +}
> +
> static int intelfb_alloc(struct drm_fb_helper *helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> return;
>
> info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return;
>
> if (synchronous) {
> /* Flush any pending work to turn the console on, and then
> @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - if (dev_priv->fbdev)
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (intel_fbdev_active(dev_priv->fbdev))
> drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>
> - if (!ifbdev)
> + if (!intel_fbdev_active(ifbdev))
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> + } else {
> + DRM_DEBUG("failed to restore crtc mode\n");
> }
> }
> --
> 2.7.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[-- Attachment #1.2: Type: text/html, Size: 17071 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-04 9:21 [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Li, Weinan Z
@ 2016-02-05 0:27 ` Lukas Wunner
2016-02-05 11:09 ` Chris Wilson
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wunner @ 2016-02-05 0:27 UTC (permalink / raw)
To: Li, Weinan Z; +Cc: intel-gfx, gustav.fagerlind
Hi,
On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.
Okay, I see.
>
> add info NULL check should be better, it is also initialized in the async queue
> > info = ifbdev->helper.fbdev;
> > + if (info == NULL)
> > + return false;
> > if (!info->screen_base)
So if there's indeed a race between fbdev initialization and the call to
intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
- intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
- it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
- it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev
Instead of checking all that it's probably simpler to call
async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
as Li Weinan suggested. I'll submit the corresponding one-liner patch
tomorrow if noone else does.
Chris' patch also modified intel_fbdev_set_suspend() as well as
intel_fbdev_output_poll_changed(), not sure if these are racy as well.
At least the stack traces posted by Li Weinan and Gustav Fägerlind
only indicate that lastclose is racy.
Best regards,
Lukas
> From: Li, Weinan Z
> Sent: Thursday, February 04, 2016 10:34 AM
> To: 'gustav.fagerlind@gmail.com'; Lukas Wunner
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
>
> Thanks for your quick response.
> Yes it is not easily be reproduced in native. In iVGT we startup several VMs simultaneously, it can be reproduced in several cycles, upon 1/10 fail rate.
> Need to use GUI mode but not text mode to reproduce this issue.
>
> BRs,
> Weinan Li
>
>
> From: Gustav Fägerlind [mailto:gustav.fagerlind@gmail.com]
> Sent: Thursday, February 04, 2016 1:08 AM
> To: Lukas Wunner
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>; Li, Weinan Z
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
>
> Cool, thank you.
> I dont believe I can easily reproduce it, it has only happend few times (and i reboot my lappy >2 times per day).
>
> //
> Gustav
>
> 2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de<mailto:lukas@wunner.de>>:
> Hi,
>
> On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> > If the initialisation fails, we may be left with a dangling pointer with
> > an incomplete fbdev structure.
>
> This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
> fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
> See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails").
>
> Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
> interesting to know if it can be reproduced at all with 4.5-rc2.
>
> Best regards,
>
> Lukas
>
> > Here we want to disable internal calls
> > into fbdev. Similarly, the initialisation may be slow and we haven't yet
> > enabled the fbdev (e.g. quick suspend or last-close before the async init
> > completes).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com<mailto:weinan.z.li@intel.com>>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>>
> > ---
> > drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
> > 1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 09840f4380f9..6218bc5370a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> > .fb_debug_leave = drm_fb_helper_debug_leave,
> > };
> >
> > +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> > +{
> > + struct fb_info *info;
> > +
> > + if (ifbdev == NULL)
> > + return false;
> > +
> > + info = ifbdev->helper.fbdev;
> > + if (!info->screen_base)
> > + return false;
> > +
> > + return info->state == FBINFO_STATE_RUNNING;
> > +}
> > +
> > static int intelfb_alloc(struct drm_fb_helper *helper,
> > struct drm_fb_helper_surface_size *sizes)
> > {
> > @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> > return;
> >
> > info = ifbdev->helper.fbdev;
> > + if (!info->screen_base)
> > + return;
> >
> > if (synchronous) {
> > /* Flush any pending work to turn the console on, and then
> > @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> >
> > void intel_fbdev_output_poll_changed(struct drm_device *dev)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - if (dev_priv->fbdev)
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > + if (intel_fbdev_active(dev_priv->fbdev))
> > drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> > }
> >
> > void intel_fbdev_restore_mode(struct drm_device *dev)
> > {
> > - int ret;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > - struct drm_fb_helper *fb_helper;
> > + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> >
> > - if (!ifbdev)
> > + if (!intel_fbdev_active(ifbdev))
> > return;
> >
> > - fb_helper = &ifbdev->helper;
> > -
> > - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> > - if (ret) {
> > - DRM_DEBUG("failed to restore crtc mode\n");
> > - } else {
> > - mutex_lock(&fb_helper->dev->struct_mutex);
> > + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> > + mutex_lock(&dev->struct_mutex);
> > intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> > - mutex_unlock(&fb_helper->dev->struct_mutex);
> > + mutex_unlock(&dev->struct_mutex);
> > + } else {
> > + DRM_DEBUG("failed to restore crtc mode\n");
> > }
> > }
> > --
> > 2.7.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org<mailto: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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-05 0:27 ` Lukas Wunner
@ 2016-02-05 11:09 ` Chris Wilson
2016-02-05 14:58 ` Lukas Wunner
0 siblings, 1 reply; 34+ messages in thread
From: Chris Wilson @ 2016-02-05 11:09 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx, gustav.fagerlind, Li, Weinan Z
On Fri, Feb 05, 2016 at 01:27:10AM +0100, Lukas Wunner wrote:
> Hi,
>
> On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> > We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> > failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> > initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> > finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.
>
> Okay, I see.
>
> >
> > add info NULL check should be better, it is also initialized in the async queue
> > > info = ifbdev->helper.fbdev;
> > > + if (info == NULL)
> > > + return false;
> > > if (!info->screen_base)
>
> So if there's indeed a race between fbdev initialization and the call to
> intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
> - intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
> - it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
> - it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev
>
> Instead of checking all that it's probably simpler to call
> async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
> as Li Weinan suggested. I'll submit the corresponding one-liner patch
> tomorrow if noone else does.
>
> Chris' patch also modified intel_fbdev_set_suspend() as well as
> intel_fbdev_output_poll_changed(), not sure if these are racy as well.
> At least the stack traces posted by Li Weinan and Gustav Fägerlind
> only indicate that lastclose is racy.
We called set-suspend internally, but we don't do any checks before
hand. I was concerned we may be able to get into a situation where
.fb_probe fails and leaves a dangling dev_priv->ifbdev for which we
would then trip over the NULL info->screen_base. So I was looking for a
suitable guard.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-05 11:09 ` Chris Wilson
@ 2016-02-05 14:58 ` Lukas Wunner
2016-02-15 16:32 ` Daniel Vetter
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wunner @ 2016-02-05 14:58 UTC (permalink / raw)
To: Chris Wilson, Li, Weinan Z, gustav.fagerlind, intel-gfx
Hi Chris,
On Fri, Feb 05, 2016 at 11:09:27AM +0000, Chris Wilson wrote:
> On Fri, Feb 05, 2016 at 01:27:10AM +0100, Lukas Wunner wrote:
> > On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> > > We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> > > failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> > > initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> > > finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.
> >
> > Okay, I see.
> >
> > >
> > > add info NULL check should be better, it is also initialized in the async queue
> > > > info = ifbdev->helper.fbdev;
> > > > + if (info == NULL)
> > > > + return false;
> > > > if (!info->screen_base)
> >
> > So if there's indeed a race between fbdev initialization and the call to
> > intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
> > - intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
> > - it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
> > - it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev
> >
> > Instead of checking all that it's probably simpler to call
> > async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
> > as Li Weinan suggested. I'll submit the corresponding one-liner patch
> > tomorrow if noone else does.
> >
> > Chris' patch also modified intel_fbdev_set_suspend() as well as
> > intel_fbdev_output_poll_changed(), not sure if these are racy as well.
> > At least the stack traces posted by Li Weinan and Gustav Fägerlind
> > only indicate that lastclose is racy.
>
> We called set-suspend internally, but we don't do any checks before
> hand. I was concerned we may be able to get into a situation where
> .fb_probe fails and leaves a dangling dev_priv->ifbdev for which we
> would then trip over the NULL info->screen_base. So I was looking for a
> suitable guard.
Yes, looking at this with a fresh pair of eyeballs I think you were
totally right, i915_pm_ops is declared as part of i915_pci_driver and
it might indeed happen that i915_pm_suspend() is called before the fbdev
is fully initialized.
As for intel_fbdev_output_poll_changed(), there's even a comment in
i915_load_modeset_init() stating that hotplug events might occur
during fdbev initial config.
Below patch adds the requisite async_synchronize_full() to the three
functions that you also modified and updates that comment.
However AFAICT a corner case remains where we're still vulnerable to races:
async_schedule() runs synchronously "if we're out of memory or if there's
too much work pending already" (see __async_schedule() in kernel/async.c).
In this case no entry is added to the pending list and
async_synchronize_full() might theoretically return before the synchronously
executed function has finished.
The existing call to async_synchronize_full() in intel_fbdev_fini()
seems to be susceptible to the same race condition, but it's probably
very hard to trigger it. (Unload the module before the fbdev is fully
initialized.)
To make it bullet proof we could declare a completion in intel_fbdev.c
and wait for that instead. Opinions?
Thanks,
Lukas
-- >8 --
Subject: [PATCH] drm/i915: Fix races on fbdev
The ->lastclose callback invokes intel_fbdev_restore_mode() and has
been witnessed to run before intel_fbdev_initial_config_async()
has finished.
We might likewise receive hotplug events or be suspended before
we've had a chance to fully set up the fbdev.
Fix by waiting for the asynchronous thread to finish.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav FÀgerlind <gustav.fagerlind@gmail.com>
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/gpu/drm/i915/i915_dma.c | 8 +++-----
drivers/gpu/drm/i915/intel_fbdev.c | 4 ++++
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a0f5659..a76b528 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -437,11 +437,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
* Some ports require correctly set-up hpd registers for detection to
* work properly (leading to ghost connected connector status), e.g. VGA
* on gm45. Hence we can only set up the initial fbdev config after hpd
- * irqs are fully enabled. Now we should scan for the initial config
- * only once hotplug handling is enabled, but due to screwed-up locking
- * around kms/fbdev init we can't protect the fdbev initial config
- * scanning against hotplug events. Hence do this first and ignore the
- * tiny window where we will loose hotplug notifactions.
+ * irqs are fully enabled. We protect the fbdev initial config scanning
+ * against hotplug events by waiting in intel_fbdev_output_poll_changed
+ * until the asynchronous thread has finished.
*/
intel_fbdev_initial_config_async(dev);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 09840f4..2002b13 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -749,6 +749,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct fb_info *info;
+ async_synchronize_full();
if (!ifbdev)
return;
@@ -795,6 +796,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+
+ async_synchronize_full();
if (dev_priv->fbdev)
drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
}
@@ -806,6 +809,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct drm_fb_helper *fb_helper;
+ async_synchronize_full();
if (!ifbdev)
return;
--
1.8.5.2 (Apple Git-48)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-05 14:58 ` Lukas Wunner
@ 2016-02-15 16:32 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2016-02-15 16:32 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx, Li, Weinan Z, gustav.fagerlind
On Fri, Feb 05, 2016 at 03:58:31PM +0100, Lukas Wunner wrote:
> Hi Chris,
>
> On Fri, Feb 05, 2016 at 11:09:27AM +0000, Chris Wilson wrote:
> > On Fri, Feb 05, 2016 at 01:27:10AM +0100, Lukas Wunner wrote:
> > > On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> > > > We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> > > > failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> > > > initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> > > > finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.
> > >
> > > Okay, I see.
> > >
> > > >
> > > > add info NULL check should be better, it is also initialized in the async queue
> > > > > info = ifbdev->helper.fbdev;
> > > > > + if (info == NULL)
> > > > > + return false;
> > > > > if (!info->screen_base)
> > >
> > > So if there's indeed a race between fbdev initialization and the call to
> > > intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
> > > - intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
> > > - it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
> > > - it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev
> > >
> > > Instead of checking all that it's probably simpler to call
> > > async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
> > > as Li Weinan suggested. I'll submit the corresponding one-liner patch
> > > tomorrow if noone else does.
> > >
> > > Chris' patch also modified intel_fbdev_set_suspend() as well as
> > > intel_fbdev_output_poll_changed(), not sure if these are racy as well.
> > > At least the stack traces posted by Li Weinan and Gustav Fägerlind
> > > only indicate that lastclose is racy.
> >
> > We called set-suspend internally, but we don't do any checks before
> > hand. I was concerned we may be able to get into a situation where
> > .fb_probe fails and leaves a dangling dev_priv->ifbdev for which we
> > would then trip over the NULL info->screen_base. So I was looking for a
> > suitable guard.
>
> Yes, looking at this with a fresh pair of eyeballs I think you were
> totally right, i915_pm_ops is declared as part of i915_pci_driver and
> it might indeed happen that i915_pm_suspend() is called before the fbdev
> is fully initialized.
>
> As for intel_fbdev_output_poll_changed(), there's even a comment in
> i915_load_modeset_init() stating that hotplug events might occur
> during fdbev initial config.
>
> Below patch adds the requisite async_synchronize_full() to the three
> functions that you also modified and updates that comment.
>
> However AFAICT a corner case remains where we're still vulnerable to races:
> async_schedule() runs synchronously "if we're out of memory or if there's
> too much work pending already" (see __async_schedule() in kernel/async.c).
> In this case no entry is added to the pending list and
> async_synchronize_full() might theoretically return before the synchronously
> executed function has finished.
>
> The existing call to async_synchronize_full() in intel_fbdev_fini()
> seems to be susceptible to the same race condition, but it's probably
> very hard to trigger it. (Unload the module before the fbdev is fully
> initialized.)
>
> To make it bullet proof we could declare a completion in intel_fbdev.c
> and wait for that instead. Opinions?
>
> Thanks,
>
> Lukas
>
> -- >8 --
>
> Subject: [PATCH] drm/i915: Fix races on fbdev
>
> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> been witnessed to run before intel_fbdev_initial_config_async()
> has finished.
>
> We might likewise receive hotplug events or be suspended before
> we've had a chance to fully set up the fbdev.
>
> Fix by waiting for the asynchronous thread to finish.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: Gustav FÀgerlind <gustav.fagerlind@gmail.com>
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Can you pls resubmit this patch stand-alone? Patchwork (and therefore CI)
won't pick up inlined patches like this one here unfortunately.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_dma.c | 8 +++-----
> drivers/gpu/drm/i915/intel_fbdev.c | 4 ++++
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a0f5659..a76b528 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -437,11 +437,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
> * Some ports require correctly set-up hpd registers for detection to
> * work properly (leading to ghost connected connector status), e.g. VGA
> * on gm45. Hence we can only set up the initial fbdev config after hpd
> - * irqs are fully enabled. Now we should scan for the initial config
> - * only once hotplug handling is enabled, but due to screwed-up locking
> - * around kms/fbdev init we can't protect the fdbev initial config
> - * scanning against hotplug events. Hence do this first and ignore the
> - * tiny window where we will loose hotplug notifactions.
> + * irqs are fully enabled. We protect the fbdev initial config scanning
> + * against hotplug events by waiting in intel_fbdev_output_poll_changed
> + * until the asynchronous thread has finished.
> */
> intel_fbdev_initial_config_async(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4..2002b13 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -749,6 +749,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> struct intel_fbdev *ifbdev = dev_priv->fbdev;
> struct fb_info *info;
>
> + async_synchronize_full();
> if (!ifbdev)
> return;
>
> @@ -795,6 +796,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + async_synchronize_full();
> if (dev_priv->fbdev)
> drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> }
> @@ -806,6 +809,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
> struct intel_fbdev *ifbdev = dev_priv->fbdev;
> struct drm_fb_helper *fb_helper;
>
> + async_synchronize_full();
> if (!ifbdev)
> return;
>
> --
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread
* intel_fbdev_restore_mode derefence null pointer
@ 2016-02-03 7:49 Li, Weinan Z
2016-02-03 9:17 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
0 siblings, 1 reply; 34+ messages in thread
From: Li, Weinan Z @ 2016-02-03 7:49 UTC (permalink / raw)
To: intel-gfx
[-- Attachment #1.1.1: Type: text/plain, Size: 5702 bytes --]
I met one kenel panic issue in iVGT, usually can be easily reproduced with multi-vms.
unable to handle kernel NULL pointer dereference at 00000000000000a0IP: [<
(d29) ffffffffa025a52b>] intel_fbdev_restore_mode+0x57/0x73 [i915]
The drm_device ->dev_private -> fbdev ->fb access function run before the initialization of it.
Since the "intel_fbdev_initial_config" run in "async_schedule", before the ifbdev->fb initialization, one access from
drm_release -> drm_lastclose->i915_driver_lastclose-> intel_fbdev_restore_mode occurred, then got kernel panic.
Do we need to add NULL pointer or async_synchronize_cookie() to avoid this issue?
I also find similar issue in bugs.freedesktop
https://bugs.freedesktop.org/show_bug.cgi?id=93580
Below is the error log:
d29) init: failsafe main process (1412) killed by TERM signal
(d29) init: bluetooth main process (1574) terminated with status 1
(d29) init: bluetooth main process ended, respawning
(d29) init: bluetooth main process (1642) terminated with status 1
(d29) init: bluetooth main process ended, respawning
(d29) init: bluetooth main process (1689) terminated with status 1
(d29) init: bluetooth respawning too fast, stopped
(d29) e1000: eth3 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
(d29) [drm] failed to retrieve link info, disabling eDP
(d29) i915 0000:00:02.0: Direct firmware load for i915/skl_guc_ver4.bin failed with e
(d29) rror -2SUBSYSTEM=pciDEVICE=+pci:0000:00:02.0
(d29) [drm:intel_guc_ucode_init [i915]] *ERROR* Failed to fetch GuC firmware from i91
(d29) 5/skl_guc_ver4.bin (error -2)
(d29) [drm] VGT ballooning configuration:
(d29) [drm] Mappable graphic memory: base 0x1e000000 size 131072KiB
(d29) [drm] Unmappable graphic memory: base 0x88000000 size 393216KiB
(d29) [drm] balloon space: range [ 0x40000000 - 0x88000000 ] 1179648 KiB.
(d29) [drm] balloon space: range [ 0xa0000000 - 0xfffff000 ] 1572860 KiB.
(d29) [drm] balloon space: range [ 0x0 - 0x1e000000 ] 491520 KiB.
(d29) [drm] balloon space: range [ 0x26000000 - 0x40000000 ] 425984 KiB.
(d29) [drm] VGT balloon successfully
[ 4506.568318] vGT info:(ring_pp_mode_write:744) EXECLIST enabling on ring 0.
[ 4506.576307] vGT-3: add to render run queue!
[ 4506.582888] vGT info:(ring_pp_mode_write:744) EXECLIST enabling on ring 1.
[ 4506.591714] vGT info:(ring_pp_mode_write:744) EXECLIST enabling on ring 2.
[ 4506.600092] vGT info:(ring_pp_mode_write:744) EXECLIST enabling on ring 3.
[ 4506.608928] vGT info:(ring_pp_mode_write:744) EXECLIST enabling on ring 4.
(d29) [drm:intel_opregion_init [i915]] *ERROR* No ACPI video bus found
(d29) [drm] Initialized i915 1.6.0 20151010 for 0000:00:02.0 on minor 0
(d29) BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0IP: [<
(d29) ffffffffa025a52b>] intel_fbdev_restore_mode+0x57/0x73 [i915]PGD 38a79067 PUD 38
(d29) a78067 PMD 0 Oops: 0000 [#1] SMP Modules linked in: fuse microcode parport_pc i
(d29) 915 drm_kms_helper i2c_algo_bit serio_raw acpi_cpufreq ppdev drm i2c_piix4 lp p
(d29) arport ext4 crc16 jbd2 mbcache e1000 uhci_hcd ata_generic pata_acpiCPU: 1 PID:
(d29) 1749 Comm: gpu-manager Tainted: G U 4.3.0-rc6-vgt+ #1
(d29) Hardware name: Xen HVM domU, BIOS 4.6.0 01/15/2016
(d29) task: ffff88003be9d700 ti: ffff880038a80000 task.ti: ffff880038a80000
(d29) RIP: 0010:[<ffffffffa025a52b>] [<ffffffffa025a52b>] intel_fbdev_restore_mode+0
(d29) x57/0x73 [i915]RSP: 0018:ffff880038a83d48 EFLAGS: 00010246
(d29) RAX: 0000000000000000 RBX: ffff8800357cb800 RCX: ffff88003d2c2400
(d29) RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff88003c139060
(d29) RBP: ffff880038a83d50 R08: 00000000ffffffff R09: ffff88003cc00000
(d29) R10: ffff88003cc001b0 R11: ffffffffa012c9a3 R12: ffff88003c139000
(d29) R13: ffff88003c139060 R14: ffff88003ba7d8e0 R15: ffff88003c139088
(d29) FS: 00007fd3bdce5740(0000) GS:ffff88003d620000(0000) knlGS:0000000000000000
(d29) CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
(d29) CR2: 00000000000000a0 CR3: 0000000000078000 CR4: 00000000003406e0
(d29) DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
(d29) DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
(d29) Stack:
(d29) ffff88003c139000 ffff880038a83d60 ffffffffa028696c ffff880038a83d80 ffffffffa0
(d29) 1170f7 0000000000000000 ffff88003c139000 ffff880038a83de0 ffffffffa0117592 ffff
(d29) 880038b87210 ffff88003c139198 0000000000000246Call Trace:
(d29) [<ffffffffa028696c>] i915_driver_lastclose+0x9/0xb [i915]
(d29) [<ffffffffa01170f7>] drm_lastclose+0x3a/0x103 [drm]
(d29) [<ffffffffa0117592>] drm_release+0x3d2/0x40b [drm]
(d29) [<ffffffff81147d26>] __fput+0xec/0x1a7
(d29) [<ffffffff81147e0d>] ____fput+0x9/0xb
(d29) [<ffffffff8106ac15>] task_work_run+0x62/0x78
(d29) [<ffffffff8100380c>] prepare_exit_to_usermode+0x93/0xaf
(d29) [<ffffffff8100398d>] syscall_return_slowpath+0x165/0x19e
(d29) [<ffffffff81155125>] ? do_vfs_ioctl+0x360/0x41a
(d29) [<ffffffff8106ab44>] ? task_work_add+0x3f/0x4e
(d29) [<ffffffff81147e87>] ? fput+0x78/0x7f
(d29) [<ffffffff81144f91>] ? filp_close+0x63/0x6d
(d29) [<ffffffff814f09cc>] int_ret_from_sys_call+0x25/0x8f
(d29) Code: c6 9c 29 2f a0 48 c7 c7 90 24 2d a0 31 c0 e8 2e 0e ec ff eb 2f 48 8b 43 0
(d29) 8 48 8d 78 60 e8 68 49 29 e1 48 8b 83 a0 00 00 00 31 f6 <48> 8b b8 a0 00 00 00
(d29) e8 1b 5f ff ff 48 8b 7b 08 48 83 c7 60 e8 RIP [<ffffffffa025a52b>] intel_fbdev
(d29) _restore_mode+0x57/0x73 [i915] RSP <ffff880038a83d48>
(d29) CR2: 00000000000000a0
(d29) ---[ end trace e656291822c44c35 ]---
(d29) Kernel panic - not syncing: Fatal exception
(d29) Kernel Offset: disabled
BRs,
Weinan Li
[-- Attachment #1.1.2: Type: text/html, Size: 32864 bytes --]
[-- Attachment #1.2: image001.gif --]
[-- Type: image/gif, Size: 92 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-03 7:49 intel_fbdev_restore_mode derefence null pointer Li, Weinan Z
@ 2016-02-03 9:17 ` Chris Wilson
2016-02-03 13:25 ` Lukas Wunner
0 siblings, 1 reply; 34+ messages in thread
From: Chris Wilson @ 2016-02-03 9:17 UTC (permalink / raw)
To: intel-gfx
If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 09840f4380f9..6218bc5370a1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
.fb_debug_leave = drm_fb_helper_debug_leave,
};
+static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
+{
+ struct fb_info *info;
+
+ if (ifbdev == NULL)
+ return false;
+
+ info = ifbdev->helper.fbdev;
+ if (!info->screen_base)
+ return false;
+
+ return info->state == FBINFO_STATE_RUNNING;
+}
+
static int intelfb_alloc(struct drm_fb_helper *helper,
struct drm_fb_helper_surface_size *sizes)
{
@@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
return;
info = ifbdev->helper.fbdev;
+ if (!info->screen_base)
+ return;
if (synchronous) {
/* Flush any pending work to turn the console on, and then
@@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- if (dev_priv->fbdev)
+ struct drm_i915_private *dev_priv = to_i915(dev);
+
+ if (intel_fbdev_active(dev_priv->fbdev))
drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
{
- int ret;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct drm_fb_helper *fb_helper;
+ struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
- if (!ifbdev)
+ if (!intel_fbdev_active(ifbdev))
return;
- fb_helper = &ifbdev->helper;
-
- ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
- if (ret) {
- DRM_DEBUG("failed to restore crtc mode\n");
- } else {
- mutex_lock(&fb_helper->dev->struct_mutex);
+ if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
+ mutex_lock(&dev->struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
- mutex_unlock(&fb_helper->dev->struct_mutex);
+ mutex_unlock(&dev->struct_mutex);
+ } else {
+ DRM_DEBUG("failed to restore crtc mode\n");
}
}
--
2.7.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-03 9:17 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
@ 2016-02-03 13:25 ` Lukas Wunner
2016-02-03 17:08 ` Gustav Fägerlind
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wunner @ 2016-02-03 13:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, gustav.fagerlind, Li, Weinan Z
Hi,
On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure.
This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails").
Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
interesting to know if it can be reproduced at all with 4.5-rc2.
Best regards,
Lukas
> Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4380f9..6218bc5370a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> .fb_debug_leave = drm_fb_helper_debug_leave,
> };
>
> +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> +{
> + struct fb_info *info;
> +
> + if (ifbdev == NULL)
> + return false;
> +
> + info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return false;
> +
> + return info->state == FBINFO_STATE_RUNNING;
> +}
> +
> static int intelfb_alloc(struct drm_fb_helper *helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> return;
>
> info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return;
>
> if (synchronous) {
> /* Flush any pending work to turn the console on, and then
> @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - if (dev_priv->fbdev)
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (intel_fbdev_active(dev_priv->fbdev))
> drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>
> - if (!ifbdev)
> + if (!intel_fbdev_active(ifbdev))
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> + } else {
> + DRM_DEBUG("failed to restore crtc mode\n");
> }
> }
> --
> 2.7.0
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-03 13:25 ` Lukas Wunner
@ 2016-02-03 17:08 ` Gustav Fägerlind
2016-02-04 2:34 ` Li, Weinan Z
0 siblings, 1 reply; 34+ messages in thread
From: Gustav Fägerlind @ 2016-02-03 17:08 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx, Li, Weinan Z
[-- Attachment #1.1: Type: text/plain, Size: 4415 bytes --]
Cool, thank you.
I dont believe I can easily reproduce it, it has only happend few times
(and i reboot my lappy >2 times per day).
//
Gustav
2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de>:
> Hi,
>
> On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> > If the initialisation fails, we may be left with a dangling pointer with
> > an incomplete fbdev structure.
>
> This shouldn't happen with 4.5, the fbdev is now clobbered if
> initialization
> fails, the existing "if (dev_priv->fbdev)" checks should thus be
> sufficient.
> See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails").
>
> Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
> interesting to know if it can be reproduced at all with 4.5-rc2.
>
> Best regards,
>
> Lukas
>
> > Here we want to disable internal calls
> > into fbdev. Similarly, the initialisation may be slow and we haven't yet
> > enabled the fbdev (e.g. quick suspend or last-close before the async init
> > completes).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_fbdev.c | 41
> ++++++++++++++++++++++++--------------
> > 1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 09840f4380f9..6218bc5370a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> > .fb_debug_leave = drm_fb_helper_debug_leave,
> > };
> >
> > +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> > +{
> > + struct fb_info *info;
> > +
> > + if (ifbdev == NULL)
> > + return false;
> > +
> > + info = ifbdev->helper.fbdev;
> > + if (!info->screen_base)
> > + return false;
> > +
> > + return info->state == FBINFO_STATE_RUNNING;
> > +}
> > +
> > static int intelfb_alloc(struct drm_fb_helper *helper,
> > struct drm_fb_helper_surface_size *sizes)
> > {
> > @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev,
> int state, bool synchronous
> > return;
> >
> > info = ifbdev->helper.fbdev;
> > + if (!info->screen_base)
> > + return;
> >
> > if (synchronous) {
> > /* Flush any pending work to turn the console on, and then
> > @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device
> *dev, int state, bool synchronous
> >
> > void intel_fbdev_output_poll_changed(struct drm_device *dev)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - if (dev_priv->fbdev)
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > + if (intel_fbdev_active(dev_priv->fbdev))
> > drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> > }
> >
> > void intel_fbdev_restore_mode(struct drm_device *dev)
> > {
> > - int ret;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > - struct drm_fb_helper *fb_helper;
> > + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> >
> > - if (!ifbdev)
> > + if (!intel_fbdev_active(ifbdev))
> > return;
> >
> > - fb_helper = &ifbdev->helper;
> > -
> > - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> > - if (ret) {
> > - DRM_DEBUG("failed to restore crtc mode\n");
> > - } else {
> > - mutex_lock(&fb_helper->dev->struct_mutex);
> > + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) ==
> 0) {
> > + mutex_lock(&dev->struct_mutex);
> > intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> > - mutex_unlock(&fb_helper->dev->struct_mutex);
> > + mutex_unlock(&dev->struct_mutex);
> > + } else {
> > + DRM_DEBUG("failed to restore crtc mode\n");
> > }
> > }
> > --
> > 2.7.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
[-- Attachment #1.2: Type: text/html, Size: 6159 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
2016-02-03 17:08 ` Gustav Fägerlind
@ 2016-02-04 2:34 ` Li, Weinan Z
0 siblings, 0 replies; 34+ messages in thread
From: Li, Weinan Z @ 2016-02-04 2:34 UTC (permalink / raw)
To: gustav.fagerlind, Lukas Wunner; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 4990 bytes --]
Thanks for your quick response.
Yes it is not easily be reproduced in native. In iVGT we startup several VMs simultaneously, it can be reproduced in several cycles, upon 1/10 fail rate.
Need to use GUI mode but not text mode to reproduce this issue.
BRs,
Weinan Li
From: Gustav Fägerlind [mailto:gustav.fagerlind@gmail.com]
Sent: Thursday, February 04, 2016 1:08 AM
To: Lukas Wunner
Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Li, Weinan Z
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
Cool, thank you.
I dont believe I can easily reproduce it, it has only happend few times (and i reboot my lappy >2 times per day).
//
Gustav
2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de<mailto:lukas@wunner.de>>:
Hi,
On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure.
This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails").
Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
interesting to know if it can be reproduced at all with 4.5-rc2.
Best regards,
Lukas
> Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com<mailto:weinan.z.li@intel.com>>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4380f9..6218bc5370a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> .fb_debug_leave = drm_fb_helper_debug_leave,
> };
>
> +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> +{
> + struct fb_info *info;
> +
> + if (ifbdev == NULL)
> + return false;
> +
> + info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return false;
> +
> + return info->state == FBINFO_STATE_RUNNING;
> +}
> +
> static int intelfb_alloc(struct drm_fb_helper *helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> return;
>
> info = ifbdev->helper.fbdev;
> + if (!info->screen_base)
> + return;
>
> if (synchronous) {
> /* Flush any pending work to turn the console on, and then
> @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - if (dev_priv->fbdev)
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (intel_fbdev_active(dev_priv->fbdev))
> drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>
> - if (!ifbdev)
> + if (!intel_fbdev_active(ifbdev))
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> + } else {
> + DRM_DEBUG("failed to restore crtc mode\n");
> }
> }
> --
> 2.7.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[-- Attachment #1.2: Type: text/html, Size: 12185 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
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2016-04-01 7:59 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-30 17:20 [REGRESSION] system hang on ILK/SNB/IVB Gabriel Feceoru
2016-03-30 17:57 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
2016-03-30 18:10 ` kbuild test robot
2016-03-30 18:10 ` kbuild test robot
2016-03-30 18:26 ` kbuild test robot
2016-03-30 18:30 ` Chris Wilson
2016-03-30 18:56 ` [PATCH v3] " Chris Wilson
2016-03-31 12:00 ` Gabriel Feceoru
2016-03-31 13:57 ` [PATCH v4 1/2] " Chris Wilson
2016-03-31 13:57 ` [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev Chris Wilson
2016-03-31 15:22 ` Joonas Lahtinen
2016-03-31 15:30 ` Chris Wilson
2016-03-31 15:56 ` Joonas Lahtinen
2016-03-31 16:05 ` [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation Joonas Lahtinen
2016-03-31 16:13 ` Chris Wilson
2016-03-31 16:28 ` Joonas Lahtinen
2016-03-31 16:30 ` Lukas Wunner
2016-03-30 18:47 ` [REGRESSION] system hang on ILK/SNB/IVB Daniel Vetter
2016-03-30 21:35 ` Lukas Wunner
2016-03-31 7:21 ` Tomi Sarvela
2016-03-31 20:35 ` Lukas Wunner
2016-04-01 7:59 ` Tomi Sarvela
2016-03-31 7:42 ` Gabriel Feceoru
2016-03-31 15:23 ` Lukas Wunner
2016-03-31 14:42 ` ✗ Fi.CI.BAT: failure for drm/i915: Protect fbdev across slow or failed initialisation (rev2) Patchwork
-- strict thread matches above, loose matches on Subject: below --
2016-02-04 9:21 [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Li, Weinan Z
2016-02-05 0:27 ` Lukas Wunner
2016-02-05 11:09 ` Chris Wilson
2016-02-05 14:58 ` Lukas Wunner
2016-02-15 16:32 ` Daniel Vetter
2016-02-03 7:49 intel_fbdev_restore_mode derefence null pointer Li, Weinan Z
2016-02-03 9:17 ` [PATCH] drm/i915: Protect fbdev across slow or failed initialisation Chris Wilson
2016-02-03 13:25 ` Lukas Wunner
2016-02-03 17:08 ` Gustav Fägerlind
2016-02-04 2:34 ` Li, Weinan Z
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.