* [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
@ 2016-11-29 12:02 Chris Wilson
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Chris Wilson @ 2016-11-29 12:02 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
The fb_helper->connector_count is modified when a new connector is
constructed following a hotplug event (e.g. DP-MST). This causes trouble
for drm_setup_crtcs() and friends that assume that fb_helper is
constant:
[ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608
[ 1250.873020] Write of size 40 by task kworker/u8:3/480
[ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G U 4.9.0-rc6+ #285
[ 1250.873043] Hardware name: /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
[ 1250.873050] Workqueue: events_unbound async_run_entry_fn
[ 1250.873056] ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608
[ 1250.873067] ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600
[ 1250.873079] ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005
[ 1250.873090] Call Trace:
[ 1250.873099] [<ffffffff814b72aa>] dump_stack+0x67/0x9d
[ 1250.873106] [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70
[ 1250.873113] [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0
[ 1250.873120] [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140
[ 1250.873127] [<ffffffff81250ac3>] kasan_report+0x53/0x60
[ 1250.873134] [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80
[ 1250.873142] [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0
[ 1250.873147] [<ffffffff8124f5f3>] memset+0x23/0x40
[ 1250.873154] [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80
[ 1250.873161] [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80
[ 1250.873169] [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0
[ 1250.873176] [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0
[ 1250.873183] [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90
[ 1250.873303] [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915]
[ 1250.873387] [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915]
[ 1250.873391] [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270
[ 1250.873394] [<ffffffff810a64b0>] process_one_work+0x3d0/0x960
[ 1250.873398] [<ffffffff810a641d>] ? process_one_work+0x33d/0x960
[ 1250.873401] [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0
[ 1250.873406] [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0
[ 1250.873413] [<ffffffff810a767d>] worker_thread+0x8d/0x840
[ 1250.873419] [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0
[ 1250.873426] [<ffffffff810b0454>] kthread+0x194/0x1c0
[ 1250.873432] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873438] [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10
[ 1250.873446] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873453] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873457] [<ffffffff81b12277>] ret_from_fork+0x27/0x40
[ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32
However, when holding the mode_config.lock around the fb_helper, we have
to be careful of any callbacks that may reenter the fb_helper and so try
to reacquire the mode_config.lock (e.g. register_framebuffer). To avoid
the mutex recursion, we have to rearrange the sequence to move the
registration into the caller outside of the mode_config.lock.
v2: drop the 1; following the lockdep assertion inside the for(;;), I
anticipated an error that doesn't happen!
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1f26634f53d8..380a0ec01033 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
* mmap page writes.
*/
+#define drm_fb_helper_for_each_connector(fbh, i__) \
+ for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
+ i__ = 0; i__ < (fbh)->connector_count; i__++)
+
/**
* drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
* emulation helper
@@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
mutex_unlock(&dev->mode_config.mutex);
return 0;
fail:
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
struct drm_fb_helper_connector *fb_helper_connector =
fb_helper->connector_info[i];
@@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
continue;
/* Walk the connectors & encoders on this fb turning them on/off */
- for (j = 0; j < fb_helper->connector_count; j++) {
+ drm_fb_helper_for_each_connector(fb_helper, j) {
connector = fb_helper->connector_info[j]->connector;
connector->funcs->dpms(connector, dpms_mode);
drm_object_property_set_value(&connector->base,
@@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
int ret = 0;
int crtc_count = 0;
int i;
- struct fb_info *info;
struct drm_fb_helper_surface_size sizes;
int gamma_size = 0;
@@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
/* first up get a count of crtcs now in use and new min/maxes width/heights */
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
struct drm_cmdline_mode *cmdline_mode;
@@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
if (ret < 0)
return ret;
- info = fb_helper->fbdev;
-
/*
* Set the fb pointer - usually drm_setup_crtcs does this for hotplug
* events, but at init time drm_setup_crtcs needs to be called before
@@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
if (fb_helper->crtc_info[i].mode_set.num_connectors)
fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
-
- info->var.pixclock = 0;
- if (register_framebuffer(info) < 0)
- return -EINVAL;
-
- dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
- info->node, info->fix.id);
-
- if (list_empty(&kernel_fb_helper_list)) {
- register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
- }
-
- list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
-
return 0;
}
@@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
int count = 0;
int i;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
connector = fb_helper->connector_info[i]->connector;
count += connector->funcs->fill_modes(connector, maxX, maxY);
}
@@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
struct drm_connector *connector;
int i = 0;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
connector = fb_helper->connector_info[i]->connector;
enabled[i] = drm_connector_enabled(connector, true);
DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
@@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
if (any_enabled)
return;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
connector = fb_helper->connector_info[i]->connector;
enabled[i] = drm_connector_enabled(connector, false);
}
@@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
return false;
count = 0;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
if (enabled[i])
count++;
}
@@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
/* check the command line or if nothing common pick 1024x768 */
can_clone = true;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
if (!enabled[i])
continue;
fb_helper_conn = fb_helper->connector_info[i];
@@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
can_clone = true;
dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false);
- for (i = 0; i < fb_helper->connector_count; i++) {
-
+ drm_fb_helper_for_each_connector(fb_helper, i) {
if (!enabled[i])
continue;
@@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper,
int i;
int hoffset = 0, voffset = 0;
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
fb_helper_conn = fb_helper->connector_info[i];
if (!fb_helper_conn->connector->has_tile)
continue;
@@ -1965,7 +1951,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
int i;
retry:
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
fb_helper_conn = fb_helper->connector_info[i];
if (conn_configured & BIT_ULL(i))
@@ -2128,6 +2114,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
width = dev->mode_config.max_width;
height = dev->mode_config.max_height;
+ /* prevent concurrent modification of connector_count by hotplug */
+ lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
+
crtcs = kcalloc(fb_helper->connector_count,
sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
modes = kcalloc(fb_helper->connector_count,
@@ -2141,7 +2130,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
goto out;
}
-
drm_enable_connectors(fb_helper, enabled);
if (!(fb_helper->funcs->initial_config &&
@@ -2170,7 +2158,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
drm_fb_helper_modeset_release(fb_helper,
&fb_helper->crtc_info[i].mode_set);
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
struct drm_display_mode *mode = modes[i];
struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
struct drm_fb_offset *offset = &offsets[i];
@@ -2247,7 +2235,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
{
struct drm_device *dev = fb_helper->dev;
+ struct fb_info *info;
int count = 0;
+ int ret;
if (!drm_fbdev_emulation)
return 0;
@@ -2256,7 +2246,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
count = drm_fb_helper_probe_connector_modes(fb_helper,
dev->mode_config.max_width,
dev->mode_config.max_height);
- mutex_unlock(&dev->mode_config.mutex);
/*
* we shouldn't end up with no modes here.
*/
@@ -2264,8 +2253,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
drm_setup_crtcs(fb_helper);
+ ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+ mutex_unlock(&dev->mode_config.mutex);
+ if (ret)
+ return ret;
- return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+ info = fb_helper->fbdev;
+ info->var.pixclock = 0;
+ ret = register_framebuffer(info);
+ if (ret < 0)
+ return ret;
+
+ dev_info(dev->dev, "fb%d: %s frame buffer device\n",
+ info->node, info->fix.id);
+
+ if (list_empty(&kernel_fb_helper_list))
+ register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
+
+ list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+
+ return 0;
}
EXPORT_SYMBOL(drm_fb_helper_initial_config);
--
2.10.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
@ 2016-11-29 12:02 ` Chris Wilson
2016-11-29 15:28 ` Sean Paul
2016-11-29 20:29 ` Daniel Vetter
2016-11-29 12:02 ` [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2016-11-29 12:02 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
drm_fb_helper_probe_connector_modes() is always called before
drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
small bit of code compaction.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
1 file changed, 11 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 380a0ec01033..90da28d2fcf3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
return best_score;
}
-static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
+static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
+ u32 width, u32 height)
{
struct drm_device *dev = fb_helper->dev;
struct drm_fb_helper_crtc **crtcs;
struct drm_display_mode **modes;
struct drm_fb_offset *offsets;
bool *enabled;
- int width, height;
int i;
DRM_DEBUG_KMS("\n");
-
- width = dev->mode_config.max_width;
- height = dev->mode_config.max_height;
+ if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+ DRM_DEBUG_KMS("No connectors reported connected with modes\n");
/* prevent concurrent modification of connector_count by hotplug */
lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
@@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
{
struct drm_device *dev = fb_helper->dev;
struct fb_info *info;
- int count = 0;
int ret;
if (!drm_fbdev_emulation)
return 0;
mutex_lock(&dev->mode_config.mutex);
- count = drm_fb_helper_probe_connector_modes(fb_helper,
- dev->mode_config.max_width,
- dev->mode_config.max_height);
- /*
- * we shouldn't end up with no modes here.
- */
- if (count == 0)
- dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
-
- drm_setup_crtcs(fb_helper);
+ drm_setup_crtcs(fb_helper,
+ dev->mode_config.max_width,
+ dev->mode_config.max_height);
ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
mutex_unlock(&dev->mode_config.mutex);
if (ret)
@@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
- u32 max_width, max_height;
if (!drm_fbdev_emulation)
return 0;
- mutex_lock(&fb_helper->dev->mode_config.mutex);
+ mutex_lock(&dev->mode_config.mutex);
if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
- mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&dev->mode_config.mutex);
return 0;
}
DRM_DEBUG_KMS("\n");
- max_width = fb_helper->fb->width;
- max_height = fb_helper->fb->height;
+ drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
- drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
- mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&dev->mode_config.mutex);
- drm_modeset_lock_all(dev);
- drm_setup_crtcs(fb_helper);
- drm_modeset_unlock_all(dev);
drm_fb_helper_set_par(fb_helper->fbdev);
return 0;
--
2.10.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
@ 2016-11-29 12:02 ` Chris Wilson
2016-11-29 15:21 ` Sean Paul
2016-11-29 20:57 ` Sean Paul
2016-11-29 15:15 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Patchwork
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2016-11-29 12:02 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Though we only walk the kernel_fb_helper_list inside a panic (or single
thread debugging), we still need to protect the list manipulation on
creating/removing a framebuffer device in order to prevent list
corruption.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_fb_helper.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 90da28d2fcf3..e934b541feea 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
"Enable legacy fbdev emulation [default=true]");
static LIST_HEAD(kernel_fb_helper_list);
+static DEFINE_MUTEX(kernel_fb_helper_lock);
/**
* DOC: fbdev helpers
@@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return;
+ mutex_lock(&kernel_fb_helper_lock);
if (!list_empty(&fb_helper->kernel_fb_list)) {
list_del(&fb_helper->kernel_fb_list);
if (list_empty(&kernel_fb_helper_list)) {
unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
}
}
+ mutex_unlock(&kernel_fb_helper_lock);
drm_fb_helper_crtc_free(fb_helper);
@@ -2258,10 +2261,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
dev_info(dev->dev, "fb%d: %s frame buffer device\n",
info->node, info->fix.id);
+ mutex_lock(&kernel_fb_helper_lock);
if (list_empty(&kernel_fb_helper_list))
register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+ mutex_unlock(&kernel_fb_helper_lock);
return 0;
}
--
2.10.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
2016-11-29 12:02 ` [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
@ 2016-11-29 15:15 ` Patchwork
2016-11-29 15:20 ` [PATCH v2 1/3] " Sean Paul
2016-11-29 20:56 ` [Intel-gfx] " Sean Paul
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2016-11-29 15:15 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
URL : https://patchwork.freedesktop.org/series/16094/
State : success
== Summary ==
Series 16094v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16094/revisions/1/mbox/
fi-bdw-5557u total:245 pass:230 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:245 pass:205 dwarn:0 dfail:0 fail:0 skip:40
fi-byt-j1900 total:245 pass:217 dwarn:0 dfail:0 fail:0 skip:28
fi-byt-n2820 total:245 pass:213 dwarn:0 dfail:0 fail:0 skip:32
fi-hsw-4770 total:245 pass:225 dwarn:0 dfail:0 fail:0 skip:20
fi-hsw-4770r total:245 pass:225 dwarn:0 dfail:0 fail:0 skip:20
fi-ilk-650 total:245 pass:192 dwarn:0 dfail:0 fail:0 skip:53
fi-ivb-3520m total:245 pass:223 dwarn:0 dfail:0 fail:0 skip:22
fi-ivb-3770 total:245 pass:223 dwarn:0 dfail:0 fail:0 skip:22
fi-kbl-7500u total:245 pass:223 dwarn:0 dfail:0 fail:0 skip:22
fi-skl-6260u total:245 pass:231 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:245 pass:224 dwarn:0 dfail:0 fail:0 skip:21
fi-skl-6700k total:245 pass:223 dwarn:1 dfail:0 fail:0 skip:21
fi-skl-6770hq total:245 pass:231 dwarn:0 dfail:0 fail:0 skip:14
fi-snb-2520m total:245 pass:213 dwarn:0 dfail:0 fail:0 skip:32
fi-snb-2600 total:245 pass:212 dwarn:0 dfail:0 fail:0 skip:33
676d4ee0b28bd4c5f9b7b3aee61b043c98ab9cc5 drm-tip: 2016y-11m-29d-12h-54m-36s UTC integration manifest
3f0170a drm: Protect fb_helper list manipulation with a mutex
3920274 drm: Pull together probe + setup for drm_fb_helper
5cacc96 drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3138/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
` (2 preceding siblings ...)
2016-11-29 15:15 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Patchwork
@ 2016-11-29 15:20 ` Sean Paul
2016-11-29 20:56 ` [Intel-gfx] " Sean Paul
4 siblings, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 15:20 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The fb_helper->connector_count is modified when a new connector is
> constructed following a hotplug event (e.g. DP-MST). This causes trouble
> for drm_setup_crtcs() and friends that assume that fb_helper is
> constant:
>
> [ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608
> [ 1250.873020] Write of size 40 by task kworker/u8:3/480
> [ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G U 4.9.0-rc6+ #285
> [ 1250.873043] Hardware name: /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
> [ 1250.873050] Workqueue: events_unbound async_run_entry_fn
> [ 1250.873056] ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608
> [ 1250.873067] ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600
> [ 1250.873079] ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005
> [ 1250.873090] Call Trace:
> [ 1250.873099] [<ffffffff814b72aa>] dump_stack+0x67/0x9d
> [ 1250.873106] [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70
> [ 1250.873113] [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0
> [ 1250.873120] [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140
> [ 1250.873127] [<ffffffff81250ac3>] kasan_report+0x53/0x60
> [ 1250.873134] [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80
> [ 1250.873142] [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0
> [ 1250.873147] [<ffffffff8124f5f3>] memset+0x23/0x40
> [ 1250.873154] [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80
> [ 1250.873161] [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80
> [ 1250.873169] [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0
> [ 1250.873176] [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0
> [ 1250.873183] [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90
> [ 1250.873303] [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915]
> [ 1250.873387] [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915]
> [ 1250.873391] [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270
> [ 1250.873394] [<ffffffff810a64b0>] process_one_work+0x3d0/0x960
> [ 1250.873398] [<ffffffff810a641d>] ? process_one_work+0x33d/0x960
> [ 1250.873401] [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0
> [ 1250.873406] [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0
> [ 1250.873413] [<ffffffff810a767d>] worker_thread+0x8d/0x840
> [ 1250.873419] [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0
> [ 1250.873426] [<ffffffff810b0454>] kthread+0x194/0x1c0
> [ 1250.873432] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873438] [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10
> [ 1250.873446] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873453] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873457] [<ffffffff81b12277>] ret_from_fork+0x27/0x40
> [ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32
>
> However, when holding the mode_config.lock around the fb_helper, we have
> to be careful of any callbacks that may reenter the fb_helper and so try
> to reacquire the mode_config.lock (e.g. register_framebuffer). To avoid
> the mutex recursion, we have to rearrange the sequence to move the
> registration into the caller outside of the mode_config.lock.
>
> v2: drop the 1; following the lockdep assertion inside the for(;;), I
> anticipated an error that doesn't happen!
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1f26634f53d8..380a0ec01033 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
> * mmap page writes.
> */
>
> +#define drm_fb_helper_for_each_connector(fbh, i__) \
> + for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
> + i__ = 0; i__ < (fbh)->connector_count; i__++)
> +
> /**
> * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> * emulation helper
> @@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
> mutex_unlock(&dev->mode_config.mutex);
> return 0;
> fail:
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_fb_helper_connector *fb_helper_connector =
> fb_helper->connector_info[i];
>
> @@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> continue;
>
> /* Walk the connectors & encoders on this fb turning them on/off */
> - for (j = 0; j < fb_helper->connector_count; j++) {
> + drm_fb_helper_for_each_connector(fb_helper, j) {
> connector = fb_helper->connector_info[j]->connector;
> connector->funcs->dpms(connector, dpms_mode);
> drm_object_property_set_value(&connector->base,
> @@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> int ret = 0;
> int crtc_count = 0;
> int i;
> - struct fb_info *info;
> struct drm_fb_helper_surface_size sizes;
> int gamma_size = 0;
>
> @@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>
> /* first up get a count of crtcs now in use and new min/maxes width/heights */
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
> struct drm_cmdline_mode *cmdline_mode;
>
> @@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (ret < 0)
> return ret;
>
> - info = fb_helper->fbdev;
> -
> /*
> * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
> * events, but at init time drm_setup_crtcs needs to be called before
> @@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (fb_helper->crtc_info[i].mode_set.num_connectors)
> fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
>
> -
> - info->var.pixclock = 0;
> - if (register_framebuffer(info) < 0)
> - return -EINVAL;
> -
> - dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> - info->node, info->fix.id);
> -
> - if (list_empty(&kernel_fb_helper_list)) {
> - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> - }
> -
> - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -
> return 0;
> }
>
> @@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
> int count = 0;
> int i;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> count += connector->funcs->fill_modes(connector, maxX, maxY);
> }
> @@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
> struct drm_connector *connector;
> int i = 0;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> enabled[i] = drm_connector_enabled(connector, true);
> DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> @@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
> if (any_enabled)
> return;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> enabled[i] = drm_connector_enabled(connector, false);
> }
> @@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
> return false;
>
> count = 0;
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (enabled[i])
> count++;
> }
> @@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
>
> /* check the command line or if nothing common pick 1024x768 */
> can_clone = true;
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (!enabled[i])
> continue;
> fb_helper_conn = fb_helper->connector_info[i];
> @@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
> can_clone = true;
> dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false);
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> -
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (!enabled[i])
> continue;
>
> @@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper,
> int i;
> int hoffset = 0, voffset = 0;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> fb_helper_conn = fb_helper->connector_info[i];
> if (!fb_helper_conn->connector->has_tile)
> continue;
> @@ -1965,7 +1951,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> int i;
>
> retry:
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> fb_helper_conn = fb_helper->connector_info[i];
>
> if (conn_configured & BIT_ULL(i))
> @@ -2128,6 +2114,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> width = dev->mode_config.max_width;
> height = dev->mode_config.max_height;
>
> + /* prevent concurrent modification of connector_count by hotplug */
> + lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> +
> crtcs = kcalloc(fb_helper->connector_count,
> sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
> modes = kcalloc(fb_helper->connector_count,
> @@ -2141,7 +2130,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> goto out;
> }
>
> -
> drm_enable_connectors(fb_helper, enabled);
>
> if (!(fb_helper->funcs->initial_config &&
> @@ -2170,7 +2158,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> drm_fb_helper_modeset_release(fb_helper,
> &fb_helper->crtc_info[i].mode_set);
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_display_mode *mode = modes[i];
> struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
> struct drm_fb_offset *offset = &offsets[i];
> @@ -2247,7 +2235,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> {
> struct drm_device *dev = fb_helper->dev;
> + struct fb_info *info;
> int count = 0;
> + int ret;
>
> if (!drm_fbdev_emulation)
> return 0;
> @@ -2256,7 +2246,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> count = drm_fb_helper_probe_connector_modes(fb_helper,
> dev->mode_config.max_width,
> dev->mode_config.max_height);
> - mutex_unlock(&dev->mode_config.mutex);
> /*
> * we shouldn't end up with no modes here.
> */
> @@ -2264,8 +2253,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
>
> drm_setup_crtcs(fb_helper);
> + ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> + mutex_unlock(&dev->mode_config.mutex);
> + if (ret)
> + return ret;
>
> - return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> + info = fb_helper->fbdev;
> + info->var.pixclock = 0;
> + ret = register_framebuffer(info);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> + info->node, info->fix.id);
> +
> + if (list_empty(&kernel_fb_helper_list))
> + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +
> + return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_initial_config);
>
> --
> 2.10.2
>
> _______________________________________________
> 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] 15+ messages in thread
* Re: [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex
2016-11-29 12:02 ` [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
@ 2016-11-29 15:21 ` Sean Paul
2016-11-29 20:57 ` Sean Paul
1 sibling, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 15:21 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Though we only walk the kernel_fb_helper_list inside a panic (or single
> thread debugging), we still need to protect the list manipulation on
> creating/removing a framebuffer device in order to prevent list
> corruption.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 90da28d2fcf3..e934b541feea 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
> "Enable legacy fbdev emulation [default=true]");
>
> static LIST_HEAD(kernel_fb_helper_list);
> +static DEFINE_MUTEX(kernel_fb_helper_lock);
>
> /**
> * DOC: fbdev helpers
> @@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> if (!drm_fbdev_emulation)
> return;
>
> + mutex_lock(&kernel_fb_helper_lock);
> if (!list_empty(&fb_helper->kernel_fb_list)) {
> list_del(&fb_helper->kernel_fb_list);
> if (list_empty(&kernel_fb_helper_list)) {
> unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> }
> }
> + mutex_unlock(&kernel_fb_helper_lock);
>
> drm_fb_helper_crtc_free(fb_helper);
>
> @@ -2258,10 +2261,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> info->node, info->fix.id);
>
> + mutex_lock(&kernel_fb_helper_lock);
> if (list_empty(&kernel_fb_helper_list))
> register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>
> list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> + mutex_unlock(&kernel_fb_helper_lock);
>
> return 0;
> }
> --
> 2.10.2
>
> _______________________________________________
> 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] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
@ 2016-11-29 15:28 ` Sean Paul
2016-11-29 15:34 ` Chris Wilson
2016-11-29 20:29 ` Daniel Vetter
1 sibling, 1 reply; 15+ messages in thread
From: Sean Paul @ 2016-11-29 15:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> drm_fb_helper_probe_connector_modes() is always called before
> drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
> small bit of code compaction.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
> 1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 380a0ec01033..90da28d2fcf3 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> return best_score;
> }
>
> -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> + u32 width, u32 height)
> {
> struct drm_device *dev = fb_helper->dev;
> struct drm_fb_helper_crtc **crtcs;
> struct drm_display_mode **modes;
> struct drm_fb_offset *offsets;
> bool *enabled;
> - int width, height;
> int i;
>
> DRM_DEBUG_KMS("\n");
> -
> - width = dev->mode_config.max_width;
> - height = dev->mode_config.max_height;
> + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>
> /* prevent concurrent modification of connector_count by hotplug */
> lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> {
> struct drm_device *dev = fb_helper->dev;
> struct fb_info *info;
> - int count = 0;
> int ret;
>
> if (!drm_fbdev_emulation)
> return 0;
>
> mutex_lock(&dev->mode_config.mutex);
> - count = drm_fb_helper_probe_connector_modes(fb_helper,
> - dev->mode_config.max_width,
> - dev->mode_config.max_height);
> - /*
> - * we shouldn't end up with no modes here.
> - */
> - if (count == 0)
> - dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
> -
> - drm_setup_crtcs(fb_helper);
> + drm_setup_crtcs(fb_helper,
> + dev->mode_config.max_width,
> + dev->mode_config.max_height);
> ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> mutex_unlock(&dev->mode_config.mutex);
> if (ret)
> @@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> - u32 max_width, max_height;
>
> if (!drm_fbdev_emulation)
> return 0;
>
> - mutex_lock(&fb_helper->dev->mode_config.mutex);
> + mutex_lock(&dev->mode_config.mutex);
> if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> fb_helper->delayed_hotplug = true;
> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> + mutex_unlock(&dev->mode_config.mutex);
> return 0;
> }
> DRM_DEBUG_KMS("\n");
>
> - max_width = fb_helper->fb->width;
> - max_height = fb_helper->fb->height;
> + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>
> - drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> + mutex_unlock(&dev->mode_config.mutex);
>
> - drm_modeset_lock_all(dev);
Stupid question: Why don't we need to do this any longer?
Sean
> - drm_setup_crtcs(fb_helper);
> - drm_modeset_unlock_all(dev);
> drm_fb_helper_set_par(fb_helper->fbdev);
>
> return 0;
> --
> 2.10.2
>
> _______________________________________________
> 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] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 15:28 ` Sean Paul
@ 2016-11-29 15:34 ` Chris Wilson
2016-11-29 16:00 ` Sean Paul
0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2016-11-29 15:34 UTC (permalink / raw)
To: Sean Paul; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 10:28:03AM -0500, Sean Paul wrote:
> On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > drm_fb_helper_probe_connector_modes() is always called before
> > drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
> > small bit of code compaction.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
> > 1 file changed, 11 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 380a0ec01033..90da28d2fcf3 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> > return best_score;
> > }
> >
> > -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> > +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> > + u32 width, u32 height)
> > {
> > struct drm_device *dev = fb_helper->dev;
> > struct drm_fb_helper_crtc **crtcs;
> > struct drm_display_mode **modes;
> > struct drm_fb_offset *offsets;
> > bool *enabled;
> > - int width, height;
> > int i;
> >
> > DRM_DEBUG_KMS("\n");
> > -
> > - width = dev->mode_config.max_width;
> > - height = dev->mode_config.max_height;
> > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> > + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> >
> > /* prevent concurrent modification of connector_count by hotplug */
> > lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> > @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> > {
> > struct drm_device *dev = fb_helper->dev;
> > struct fb_info *info;
> > - int count = 0;
> > int ret;
> >
> > if (!drm_fbdev_emulation)
> > return 0;
> >
> > mutex_lock(&dev->mode_config.mutex);
> > - count = drm_fb_helper_probe_connector_modes(fb_helper,
> > - dev->mode_config.max_width,
> > - dev->mode_config.max_height);
> > - /*
> > - * we shouldn't end up with no modes here.
> > - */
> > - if (count == 0)
> > - dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
> > -
> > - drm_setup_crtcs(fb_helper);
> > + drm_setup_crtcs(fb_helper,
> > + dev->mode_config.max_width,
> > + dev->mode_config.max_height);
> > ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> > mutex_unlock(&dev->mode_config.mutex);
> > if (ret)
> > @@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> > {
> > struct drm_device *dev = fb_helper->dev;
> > - u32 max_width, max_height;
> >
> > if (!drm_fbdev_emulation)
> > return 0;
> >
> > - mutex_lock(&fb_helper->dev->mode_config.mutex);
> > + mutex_lock(&dev->mode_config.mutex);
> > if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> > fb_helper->delayed_hotplug = true;
> > - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> > + mutex_unlock(&dev->mode_config.mutex);
> > return 0;
> > }
> > DRM_DEBUG_KMS("\n");
> >
> > - max_width = fb_helper->fb->width;
> > - max_height = fb_helper->fb->height;
> > + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> >
> > - drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
> > - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> > + mutex_unlock(&dev->mode_config.mutex);
> >
> > - drm_modeset_lock_all(dev);
>
> Stupid question: Why don't we need to do this any longer?
We don't do (never did) any modesets along this path, so locking
everything is overkill and misleading, i.e. I was trimming it down to
the locking as required by setup_crtc which is just that the config is
stable.
-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] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 15:34 ` Chris Wilson
@ 2016-11-29 16:00 ` Sean Paul
0 siblings, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 16:00 UTC (permalink / raw)
To: Chris Wilson, Sean Paul, dri-devel, Intel Graphics Development
On Tue, Nov 29, 2016 at 10:34 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Nov 29, 2016 at 10:28:03AM -0500, Sean Paul wrote:
>> On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > drm_fb_helper_probe_connector_modes() is always called before
>> > drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
>> > small bit of code compaction.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
>> > ---
>> > drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
>> > 1 file changed, 11 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > index 380a0ec01033..90da28d2fcf3 100644
>> > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>> > return best_score;
>> > }
>> >
>> > -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>> > +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>> > + u32 width, u32 height)
>> > {
>> > struct drm_device *dev = fb_helper->dev;
>> > struct drm_fb_helper_crtc **crtcs;
>> > struct drm_display_mode **modes;
>> > struct drm_fb_offset *offsets;
>> > bool *enabled;
>> > - int width, height;
>> > int i;
>> >
>> > DRM_DEBUG_KMS("\n");
>> > -
>> > - width = dev->mode_config.max_width;
>> > - height = dev->mode_config.max_height;
>> > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
>> > + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>> >
>> > /* prevent concurrent modification of connector_count by hotplug */
>> > lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
>> > @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>> > {
>> > struct drm_device *dev = fb_helper->dev;
>> > struct fb_info *info;
>> > - int count = 0;
>> > int ret;
>> >
>> > if (!drm_fbdev_emulation)
>> > return 0;
>> >
>> > mutex_lock(&dev->mode_config.mutex);
>> > - count = drm_fb_helper_probe_connector_modes(fb_helper,
>> > - dev->mode_config.max_width,
>> > - dev->mode_config.max_height);
>> > - /*
>> > - * we shouldn't end up with no modes here.
>> > - */
>> > - if (count == 0)
>> > - dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
>> > -
>> > - drm_setup_crtcs(fb_helper);
>> > + drm_setup_crtcs(fb_helper,
>> > + dev->mode_config.max_width,
>> > + dev->mode_config.max_height);
>> > ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
>> > mutex_unlock(&dev->mode_config.mutex);
>> > if (ret)
>> > @@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>> > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>> > {
>> > struct drm_device *dev = fb_helper->dev;
>> > - u32 max_width, max_height;
>> >
>> > if (!drm_fbdev_emulation)
>> > return 0;
>> >
>> > - mutex_lock(&fb_helper->dev->mode_config.mutex);
>> > + mutex_lock(&dev->mode_config.mutex);
>> > if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>> > fb_helper->delayed_hotplug = true;
>> > - mutex_unlock(&fb_helper->dev->mode_config.mutex);
>> > + mutex_unlock(&dev->mode_config.mutex);
>> > return 0;
>> > }
>> > DRM_DEBUG_KMS("\n");
>> >
>> > - max_width = fb_helper->fb->width;
>> > - max_height = fb_helper->fb->height;
>> > + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>> >
>> > - drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
>> > - mutex_unlock(&fb_helper->dev->mode_config.mutex);
>> > + mutex_unlock(&dev->mode_config.mutex);
>> >
>> > - drm_modeset_lock_all(dev);
>>
>> Stupid question: Why don't we need to do this any longer?
>
> We don't do (never did) any modesets along this path, so locking
> everything is overkill and misleading, i.e. I was trimming it down to
> the locking as required by setup_crtc which is just that the config is
> stable.
Thanks for the explanation, looks good to me.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> -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] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
2016-11-29 15:28 ` Sean Paul
@ 2016-11-29 20:29 ` Daniel Vetter
2016-11-29 20:56 ` Sean Paul
2016-11-29 22:26 ` Chris Wilson
1 sibling, 2 replies; 15+ messages in thread
From: Daniel Vetter @ 2016-11-29 20:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, dri-devel
On Tue, Nov 29, 2016 at 12:02:16PM +0000, Chris Wilson wrote:
> drm_fb_helper_probe_connector_modes() is always called before
> drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
> small bit of code compaction.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
rb not entirely correct, since it's missing the crucial note I asked for
to understand things:
"Note that register_framebuffer will do a modeset (when fbcon is enabled)
and hence must be moved out of the critical section. A follow-up patch
will add new locking for the fb list, hence move all the related
registration code together."
Sean, since you reviewed all, can you pls add this note to the commit
message when merging?
Thanks, Daniel
> ---
> drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
> 1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 380a0ec01033..90da28d2fcf3 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> return best_score;
> }
>
> -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> + u32 width, u32 height)
> {
> struct drm_device *dev = fb_helper->dev;
> struct drm_fb_helper_crtc **crtcs;
> struct drm_display_mode **modes;
> struct drm_fb_offset *offsets;
> bool *enabled;
> - int width, height;
> int i;
>
> DRM_DEBUG_KMS("\n");
> -
> - width = dev->mode_config.max_width;
> - height = dev->mode_config.max_height;
> + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>
> /* prevent concurrent modification of connector_count by hotplug */
> lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> {
> struct drm_device *dev = fb_helper->dev;
> struct fb_info *info;
> - int count = 0;
> int ret;
>
> if (!drm_fbdev_emulation)
> return 0;
>
> mutex_lock(&dev->mode_config.mutex);
> - count = drm_fb_helper_probe_connector_modes(fb_helper,
> - dev->mode_config.max_width,
> - dev->mode_config.max_height);
> - /*
> - * we shouldn't end up with no modes here.
> - */
> - if (count == 0)
> - dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
> -
> - drm_setup_crtcs(fb_helper);
> + drm_setup_crtcs(fb_helper,
> + dev->mode_config.max_width,
> + dev->mode_config.max_height);
> ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> mutex_unlock(&dev->mode_config.mutex);
> if (ret)
> @@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> - u32 max_width, max_height;
>
> if (!drm_fbdev_emulation)
> return 0;
>
> - mutex_lock(&fb_helper->dev->mode_config.mutex);
> + mutex_lock(&dev->mode_config.mutex);
> if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> fb_helper->delayed_hotplug = true;
> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> + mutex_unlock(&dev->mode_config.mutex);
> return 0;
> }
> DRM_DEBUG_KMS("\n");
>
> - max_width = fb_helper->fb->width;
> - max_height = fb_helper->fb->height;
> + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>
> - drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
> + mutex_unlock(&dev->mode_config.mutex);
>
> - drm_modeset_lock_all(dev);
> - drm_setup_crtcs(fb_helper);
> - drm_modeset_unlock_all(dev);
> drm_fb_helper_set_par(fb_helper->fbdev);
>
> return 0;
> --
> 2.10.2
>
--
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] 15+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
` (3 preceding siblings ...)
2016-11-29 15:20 ` [PATCH v2 1/3] " Sean Paul
@ 2016-11-29 20:56 ` Sean Paul
4 siblings, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 20:56 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The fb_helper->connector_count is modified when a new connector is
> constructed following a hotplug event (e.g. DP-MST). This causes trouble
> for drm_setup_crtcs() and friends that assume that fb_helper is
> constant:
>
> [ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608
> [ 1250.873020] Write of size 40 by task kworker/u8:3/480
> [ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G U 4.9.0-rc6+ #285
> [ 1250.873043] Hardware name: /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
> [ 1250.873050] Workqueue: events_unbound async_run_entry_fn
> [ 1250.873056] ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608
> [ 1250.873067] ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600
> [ 1250.873079] ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005
> [ 1250.873090] Call Trace:
> [ 1250.873099] [<ffffffff814b72aa>] dump_stack+0x67/0x9d
> [ 1250.873106] [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70
> [ 1250.873113] [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0
> [ 1250.873120] [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140
> [ 1250.873127] [<ffffffff81250ac3>] kasan_report+0x53/0x60
> [ 1250.873134] [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80
> [ 1250.873142] [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0
> [ 1250.873147] [<ffffffff8124f5f3>] memset+0x23/0x40
> [ 1250.873154] [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80
> [ 1250.873161] [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80
> [ 1250.873169] [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0
> [ 1250.873176] [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0
> [ 1250.873183] [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90
> [ 1250.873303] [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915]
> [ 1250.873387] [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915]
> [ 1250.873391] [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270
> [ 1250.873394] [<ffffffff810a64b0>] process_one_work+0x3d0/0x960
> [ 1250.873398] [<ffffffff810a641d>] ? process_one_work+0x33d/0x960
> [ 1250.873401] [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0
> [ 1250.873406] [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0
> [ 1250.873413] [<ffffffff810a767d>] worker_thread+0x8d/0x840
> [ 1250.873419] [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0
> [ 1250.873426] [<ffffffff810b0454>] kthread+0x194/0x1c0
> [ 1250.873432] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873438] [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10
> [ 1250.873446] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873453] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873457] [<ffffffff81b12277>] ret_from_fork+0x27/0x40
> [ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32
>
> However, when holding the mode_config.lock around the fb_helper, we have
> to be careful of any callbacks that may reenter the fb_helper and so try
> to reacquire the mode_config.lock (e.g. register_framebuffer). To avoid
> the mutex recursion, we have to rearrange the sequence to move the
> registration into the caller outside of the mode_config.lock.
>
> v2: drop the 1; following the lockdep assertion inside the for(;;), I
> anticipated an error that doesn't happen!
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
Applied to drm-misc
Sean
> ---
> drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1f26634f53d8..380a0ec01033 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
> * mmap page writes.
> */
>
> +#define drm_fb_helper_for_each_connector(fbh, i__) \
> + for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
> + i__ = 0; i__ < (fbh)->connector_count; i__++)
> +
> /**
> * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> * emulation helper
> @@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
> mutex_unlock(&dev->mode_config.mutex);
> return 0;
> fail:
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_fb_helper_connector *fb_helper_connector =
> fb_helper->connector_info[i];
>
> @@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> continue;
>
> /* Walk the connectors & encoders on this fb turning them on/off */
> - for (j = 0; j < fb_helper->connector_count; j++) {
> + drm_fb_helper_for_each_connector(fb_helper, j) {
> connector = fb_helper->connector_info[j]->connector;
> connector->funcs->dpms(connector, dpms_mode);
> drm_object_property_set_value(&connector->base,
> @@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> int ret = 0;
> int crtc_count = 0;
> int i;
> - struct fb_info *info;
> struct drm_fb_helper_surface_size sizes;
> int gamma_size = 0;
>
> @@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>
> /* first up get a count of crtcs now in use and new min/maxes width/heights */
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
> struct drm_cmdline_mode *cmdline_mode;
>
> @@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (ret < 0)
> return ret;
>
> - info = fb_helper->fbdev;
> -
> /*
> * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
> * events, but at init time drm_setup_crtcs needs to be called before
> @@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (fb_helper->crtc_info[i].mode_set.num_connectors)
> fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
>
> -
> - info->var.pixclock = 0;
> - if (register_framebuffer(info) < 0)
> - return -EINVAL;
> -
> - dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> - info->node, info->fix.id);
> -
> - if (list_empty(&kernel_fb_helper_list)) {
> - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> - }
> -
> - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -
> return 0;
> }
>
> @@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
> int count = 0;
> int i;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> count += connector->funcs->fill_modes(connector, maxX, maxY);
> }
> @@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
> struct drm_connector *connector;
> int i = 0;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> enabled[i] = drm_connector_enabled(connector, true);
> DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> @@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
> if (any_enabled)
> return;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> connector = fb_helper->connector_info[i]->connector;
> enabled[i] = drm_connector_enabled(connector, false);
> }
> @@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
> return false;
>
> count = 0;
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (enabled[i])
> count++;
> }
> @@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
>
> /* check the command line or if nothing common pick 1024x768 */
> can_clone = true;
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (!enabled[i])
> continue;
> fb_helper_conn = fb_helper->connector_info[i];
> @@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
> can_clone = true;
> dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false);
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> -
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> if (!enabled[i])
> continue;
>
> @@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper,
> int i;
> int hoffset = 0, voffset = 0;
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> fb_helper_conn = fb_helper->connector_info[i];
> if (!fb_helper_conn->connector->has_tile)
> continue;
> @@ -1965,7 +1951,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> int i;
>
> retry:
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> fb_helper_conn = fb_helper->connector_info[i];
>
> if (conn_configured & BIT_ULL(i))
> @@ -2128,6 +2114,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> width = dev->mode_config.max_width;
> height = dev->mode_config.max_height;
>
> + /* prevent concurrent modification of connector_count by hotplug */
> + lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> +
> crtcs = kcalloc(fb_helper->connector_count,
> sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
> modes = kcalloc(fb_helper->connector_count,
> @@ -2141,7 +2130,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> goto out;
> }
>
> -
> drm_enable_connectors(fb_helper, enabled);
>
> if (!(fb_helper->funcs->initial_config &&
> @@ -2170,7 +2158,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> drm_fb_helper_modeset_release(fb_helper,
> &fb_helper->crtc_info[i].mode_set);
>
> - for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_fb_helper_for_each_connector(fb_helper, i) {
> struct drm_display_mode *mode = modes[i];
> struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
> struct drm_fb_offset *offset = &offsets[i];
> @@ -2247,7 +2235,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> {
> struct drm_device *dev = fb_helper->dev;
> + struct fb_info *info;
> int count = 0;
> + int ret;
>
> if (!drm_fbdev_emulation)
> return 0;
> @@ -2256,7 +2246,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> count = drm_fb_helper_probe_connector_modes(fb_helper,
> dev->mode_config.max_width,
> dev->mode_config.max_height);
> - mutex_unlock(&dev->mode_config.mutex);
> /*
> * we shouldn't end up with no modes here.
> */
> @@ -2264,8 +2253,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
>
> drm_setup_crtcs(fb_helper);
> + ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> + mutex_unlock(&dev->mode_config.mutex);
> + if (ret)
> + return ret;
>
> - return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> + info = fb_helper->fbdev;
> + info->var.pixclock = 0;
> + ret = register_framebuffer(info);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> + info->node, info->fix.id);
> +
> + if (list_empty(&kernel_fb_helper_list))
> + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +
> + return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_initial_config);
>
> --
> 2.10.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 20:29 ` Daniel Vetter
@ 2016-11-29 20:56 ` Sean Paul
2016-11-29 22:26 ` Chris Wilson
1 sibling, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 20:56 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 3:29 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Nov 29, 2016 at 12:02:16PM +0000, Chris Wilson wrote:
>> drm_fb_helper_probe_connector_modes() is always called before
>> drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
>> small bit of code compaction.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
>
> rb not entirely correct, since it's missing the crucial note I asked for
> to understand things:
>
> "Note that register_framebuffer will do a modeset (when fbcon is enabled)
> and hence must be moved out of the critical section. A follow-up patch
> will add new locking for the fb list, hence move all the related
> registration code together."
>
> Sean, since you reviewed all, can you pls add this note to the commit
> message when merging?
>
Applied to misc with the added note.
Sean
> Thanks, Daniel
>
>> ---
>> drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
>> 1 file changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 380a0ec01033..90da28d2fcf3 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>> return best_score;
>> }
>>
>> -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>> +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>> + u32 width, u32 height)
>> {
>> struct drm_device *dev = fb_helper->dev;
>> struct drm_fb_helper_crtc **crtcs;
>> struct drm_display_mode **modes;
>> struct drm_fb_offset *offsets;
>> bool *enabled;
>> - int width, height;
>> int i;
>>
>> DRM_DEBUG_KMS("\n");
>> -
>> - width = dev->mode_config.max_width;
>> - height = dev->mode_config.max_height;
>> + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
>> + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>>
>> /* prevent concurrent modification of connector_count by hotplug */
>> lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
>> @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>> {
>> struct drm_device *dev = fb_helper->dev;
>> struct fb_info *info;
>> - int count = 0;
>> int ret;
>>
>> if (!drm_fbdev_emulation)
>> return 0;
>>
>> mutex_lock(&dev->mode_config.mutex);
>> - count = drm_fb_helper_probe_connector_modes(fb_helper,
>> - dev->mode_config.max_width,
>> - dev->mode_config.max_height);
>> - /*
>> - * we shouldn't end up with no modes here.
>> - */
>> - if (count == 0)
>> - dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
>> -
>> - drm_setup_crtcs(fb_helper);
>> + drm_setup_crtcs(fb_helper,
>> + dev->mode_config.max_width,
>> + dev->mode_config.max_height);
>> ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
>> mutex_unlock(&dev->mode_config.mutex);
>> if (ret)
>> @@ -2300,28 +2291,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>> {
>> struct drm_device *dev = fb_helper->dev;
>> - u32 max_width, max_height;
>>
>> if (!drm_fbdev_emulation)
>> return 0;
>>
>> - mutex_lock(&fb_helper->dev->mode_config.mutex);
>> + mutex_lock(&dev->mode_config.mutex);
>> if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>> fb_helper->delayed_hotplug = true;
>> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
>> + mutex_unlock(&dev->mode_config.mutex);
>> return 0;
>> }
>> DRM_DEBUG_KMS("\n");
>>
>> - max_width = fb_helper->fb->width;
>> - max_height = fb_helper->fb->height;
>> + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>>
>> - drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
>> - mutex_unlock(&fb_helper->dev->mode_config.mutex);
>> + mutex_unlock(&dev->mode_config.mutex);
>>
>> - drm_modeset_lock_all(dev);
>> - drm_setup_crtcs(fb_helper);
>> - drm_modeset_unlock_all(dev);
>> drm_fb_helper_set_par(fb_helper->fbdev);
>>
>> return 0;
>> --
>> 2.10.2
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex
2016-11-29 12:02 ` [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
2016-11-29 15:21 ` Sean Paul
@ 2016-11-29 20:57 ` Sean Paul
1 sibling, 0 replies; 15+ messages in thread
From: Sean Paul @ 2016-11-29 20:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel
On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Though we only walk the kernel_fb_helper_list inside a panic (or single
> thread debugging), we still need to protect the list manipulation on
> creating/removing a framebuffer device in order to prevent list
> corruption.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Applied to drm-misc
Sean
> ---
> drivers/gpu/drm/drm_fb_helper.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 90da28d2fcf3..e934b541feea 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
> "Enable legacy fbdev emulation [default=true]");
>
> static LIST_HEAD(kernel_fb_helper_list);
> +static DEFINE_MUTEX(kernel_fb_helper_lock);
>
> /**
> * DOC: fbdev helpers
> @@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> if (!drm_fbdev_emulation)
> return;
>
> + mutex_lock(&kernel_fb_helper_lock);
> if (!list_empty(&fb_helper->kernel_fb_list)) {
> list_del(&fb_helper->kernel_fb_list);
> if (list_empty(&kernel_fb_helper_list)) {
> unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> }
> }
> + mutex_unlock(&kernel_fb_helper_lock);
>
> drm_fb_helper_crtc_free(fb_helper);
>
> @@ -2258,10 +2261,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> info->node, info->fix.id);
>
> + mutex_lock(&kernel_fb_helper_lock);
> if (list_empty(&kernel_fb_helper_list))
> register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>
> list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> + mutex_unlock(&kernel_fb_helper_lock);
>
> return 0;
> }
> --
> 2.10.2
>
> _______________________________________________
> 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] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 20:29 ` Daniel Vetter
2016-11-29 20:56 ` Sean Paul
@ 2016-11-29 22:26 ` Chris Wilson
2016-11-29 22:35 ` Daniel Vetter
1 sibling, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2016-11-29 22:26 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
On Tue, Nov 29, 2016 at 09:29:06PM +0100, Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 12:02:16PM +0000, Chris Wilson wrote:
> > drm_fb_helper_probe_connector_modes() is always called before
> > drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
> > small bit of code compaction.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
>
> rb not entirely correct, since it's missing the crucial note I asked for
> to understand things:
>
> "Note that register_framebuffer will do a modeset (when fbcon is enabled)
> and hence must be moved out of the critical section. A follow-up patch
> will add new locking for the fb list, hence move all the related
> registration code together."
Which is in patch 1 where the move was required.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper
2016-11-29 22:26 ` Chris Wilson
@ 2016-11-29 22:35 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2016-11-29 22:35 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, dri-devel, intel-gfx
On Tue, Nov 29, 2016 at 11:26 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Nov 29, 2016 at 09:29:06PM +0100, Daniel Vetter wrote:
>> On Tue, Nov 29, 2016 at 12:02:16PM +0000, Chris Wilson wrote:
>> > drm_fb_helper_probe_connector_modes() is always called before
>> > drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
>> > small bit of code compaction.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com>
>>
>> rb not entirely correct, since it's missing the crucial note I asked for
>> to understand things:
>>
>> "Note that register_framebuffer will do a modeset (when fbcon is enabled)
>> and hence must be moved out of the critical section. A follow-up patch
>> will add new locking for the fb list, hence move all the related
>> registration code together."
>
> Which is in patch 1 where the move was required.
Oh dear did I screw up :( My apologies for the mess I caused, should
have checked things properly ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-11-29 22:35 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-29 12:02 [PATCH v2 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
2016-11-29 12:02 ` [PATCH v2 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
2016-11-29 15:28 ` Sean Paul
2016-11-29 15:34 ` Chris Wilson
2016-11-29 16:00 ` Sean Paul
2016-11-29 20:29 ` Daniel Vetter
2016-11-29 20:56 ` Sean Paul
2016-11-29 22:26 ` Chris Wilson
2016-11-29 22:35 ` Daniel Vetter
2016-11-29 12:02 ` [PATCH v2 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
2016-11-29 15:21 ` Sean Paul
2016-11-29 20:57 ` Sean Paul
2016-11-29 15:15 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Patchwork
2016-11-29 15:20 ` [PATCH v2 1/3] " Sean Paul
2016-11-29 20:56 ` [Intel-gfx] " Sean Paul
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.