public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/mst: fix recursive sleep warning on qlock
@ 2015-01-28  3:37 Dave Airlie
  2015-01-28  9:02 ` Daniel Vetter
  2015-02-01 10:52 ` shuang.he
  0 siblings, 2 replies; 3+ messages in thread
From: Dave Airlie @ 2015-01-28  3:37 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

With drm-next, we can get a backtrace like below,
this is due to the callback checking the txmsg state taking
the mutex, which can cause a sleep inside a sleep,

Fix this my creating a spinlock protecting the txmsg state
and locking it in appropriate places.

: ------------[ cut here ]------------
: WARNING: CPU: 3 PID: 252 at kernel/sched/core.c:7300 __might_sleep+0xbd/0xd0()
: do not call blocking ops when !TASK_RUNNING; state=2 set at [<ffffffff810db8ad>] prepare_to_wait_event+0x5d/0x110
: Modules linked in: i915 i2c_algo_bit drm_kms_helper drm e1000e ptp pps_core i2c_core video
: CPU: 3 PID: 252 Comm: kworker/3:2 Not tainted 3.19.0-rc5+ #18
: Hardware name: LENOVO 20ARS25701/20ARS25701, BIOS GJET72WW (2.22 ) 02/21/2014
: Workqueue: events_long drm_dp_mst_link_probe_work [drm_kms_helper]
:  ffffffff81a4027c ffff88030a763af8 ffffffff81752699 0000000000000000
:  ffff88030a763b48 ffff88030a763b38 ffffffff810974ca ffff88030a763b38
:  ffffffff81a41123 000000000000026d 0000000000000000 0000000000000fa0
: Call Trace:
:  [<ffffffff81752699>] dump_stack+0x4c/0x65
:  [<ffffffff810974ca>] warn_slowpath_common+0x8a/0xc0
:  [<ffffffff81097546>] warn_slowpath_fmt+0x46/0x50
:  [<ffffffff810db8ad>] ? prepare_to_wait_event+0x5d/0x110
:  [<ffffffff810db8ad>] ? prepare_to_wait_event+0x5d/0x110
:  [<ffffffff810bdaad>] __might_sleep+0xbd/0xd0
:  [<ffffffff817566de>] mutex_lock_nested+0x2e/0x3e0
:  [<ffffffff810db8e8>] ? prepare_to_wait_event+0x98/0x110
:  [<ffffffffa00e43d7>] drm_dp_mst_wait_tx_reply+0xa7/0x220 [drm_kms_helper]
:  [<ffffffff810db790>] ? wait_woken+0xc0/0xc0
:  [<ffffffffa00e6401>] drm_dp_send_link_address+0x61/0x240 [drm_kms_helper]
:  [<ffffffff810b1aaf>] ? process_one_work+0x14f/0x530
:  [<ffffffffa00e6abd>] drm_dp_check_and_send_link_address+0x8d/0xa0 [drm_kms_helper]
:  [<ffffffffa00e6aec>] drm_dp_mst_link_probe_work+0x1c/0x20 [drm_kms_helper]
:  [<ffffffff810b1b26>] process_one_work+0x1c6/0x530
:  [<ffffffff810b1aaf>] ? process_one_work+0x14f/0x530
:  [<ffffffff810b224b>] worker_thread+0x6b/0x490
:  [<ffffffff810b21e0>] ? rescuer_thread+0x350/0x350
:  [<ffffffff810b73ea>] kthread+0x10a/0x120
:  [<ffffffff8175a800>] ? _raw_spin_unlock_irq+0x30/0x50
:  [<ffffffff810b72e0>] ? kthread_create_on_node+0x220/0x220
:  [<ffffffff8175b1fc>] ret_from_fork+0x7c/0xb0
:  [<ffffffff810b72e0>] ? kthread_create_on_node+0x220/0x220
: ---[ end trace bb11c9634a7217c6 ]---

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 15 +++++++++++++--
 include/drm/drm_dp_mst_helper.h       |  4 +++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 9a5b687..07662ae 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -733,10 +733,10 @@ static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_sideband_msg_tx *txmsg)
 {
 	bool ret;
-	mutex_lock(&mgr->qlock);
+	spin_lock(&mgr->state_lock);
 	ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
 	       txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
-	mutex_unlock(&mgr->qlock);
+	spin_unlock(&mgr->state_lock);
 	return ret;
 }
 
@@ -750,6 +750,7 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb,
 				 check_txmsg_state(mgr, txmsg),
 				 (4 * HZ));
 	mutex_lock(&mstb->mgr->qlock);
+	spin_lock(&mgr->state_lock);
 	if (ret > 0) {
 		if (txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT) {
 			ret = -EIO;
@@ -773,6 +774,7 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb,
 		}
 	}
 out:
+	spin_unlock(&mgr->state_lock);
 	mutex_unlock(&mgr->qlock);
 
 	return ret;
@@ -1318,10 +1320,12 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr,
 
 	memset(&hdr, 0, sizeof(struct drm_dp_sideband_msg_hdr));
 
+	spin_lock(&mgr->state_lock);
 	if (txmsg->state == DRM_DP_SIDEBAND_TX_QUEUED) {
 		txmsg->seqno = -1;
 		txmsg->state = DRM_DP_SIDEBAND_TX_START_SEND;
 	}
+	spin_unlock(&mgr->state_lock);
 
 	/* make hdr from dst mst - for replies use seqno
 	   otherwise assign one */
@@ -1357,7 +1361,9 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr,
 
 	txmsg->cur_offset += tosend;
 	if (txmsg->cur_offset == txmsg->cur_len) {
+		spin_lock(&mgr->state_lock);
 		txmsg->state = DRM_DP_SIDEBAND_TX_SENT;
+		spin_unlock(&mgr->state_lock);
 		return 1;
 	}
 	return 0;
@@ -1386,7 +1392,9 @@ static void process_single_down_tx_qlock(struct drm_dp_mst_topology_mgr *mgr)
 		list_del(&txmsg->next);
 		if (txmsg->seqno != -1)
 			txmsg->dst->tx_slots[txmsg->seqno] = NULL;
+		spin_lock(&mgr->state_lock);
 		txmsg->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
+		spin_unlock(&mgr->state_lock);
 		wake_up(&mgr->tx_waitq);
 	}
 	if (list_empty(&mgr->tx_msg_downq)) {
@@ -2083,7 +2091,9 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 		drm_dp_put_mst_branch_device(mstb);
 
 		mutex_lock(&mgr->qlock);
+		spin_lock(&mgr->state_lock);
 		txmsg->state = DRM_DP_SIDEBAND_TX_RX;
+		spin_unlock(&mgr->state_lock);
 		mstb->tx_slots[slot] = NULL;
 		mutex_unlock(&mgr->qlock);
 
@@ -2633,6 +2643,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 	mutex_init(&mgr->lock);
 	mutex_init(&mgr->qlock);
 	mutex_init(&mgr->payload_lock);
+	spin_lock_init(&mgr->state_lock);
 	INIT_LIST_HEAD(&mgr->tx_msg_upq);
 	INIT_LIST_HEAD(&mgr->tx_msg_downq);
 	INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 00c1da9..7cd1735 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -445,8 +445,10 @@ struct drm_dp_mst_topology_mgr {
 
 	/* messages to be transmitted */
 	/* qlock protects the upq/downq and in_progress,
-	   the mstb tx_slots and txmsg->state once they are queued */
+	   the mstb tx_slots once they are queued */
 	struct mutex qlock;
+	/* state lock protects txmsg->state */
+	spinlock_t state_lock;
 	struct list_head tx_msg_downq;
 	struct list_head tx_msg_upq;
 	bool tx_down_in_progress;
-- 
2.1.0

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] drm/mst: fix recursive sleep warning on qlock
  2015-01-28  3:37 [PATCH] drm/mst: fix recursive sleep warning on qlock Dave Airlie
@ 2015-01-28  9:02 ` Daniel Vetter
  2015-02-01 10:52 ` shuang.he
  1 sibling, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2015-01-28  9:02 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Jan 28, 2015 at 04:37:01PM +1300, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> With drm-next, we can get a backtrace like below,
> this is due to the callback checking the txmsg state taking
> the mutex, which can cause a sleep inside a sleep,
> 
> Fix this my creating a spinlock protecting the txmsg state
> and locking it in appropriate places.
> 
> : ------------[ cut here ]------------
> : WARNING: CPU: 3 PID: 252 at kernel/sched/core.c:7300 __might_sleep+0xbd/0xd0()
> : do not call blocking ops when !TASK_RUNNING; state=2 set at [<ffffffff810db8ad>] prepare_to_wait_event+0x5d/0x110
> : Modules linked in: i915 i2c_algo_bit drm_kms_helper drm e1000e ptp pps_core i2c_core video
> : CPU: 3 PID: 252 Comm: kworker/3:2 Not tainted 3.19.0-rc5+ #18
> : Hardware name: LENOVO 20ARS25701/20ARS25701, BIOS GJET72WW (2.22 ) 02/21/2014
> : Workqueue: events_long drm_dp_mst_link_probe_work [drm_kms_helper]
> :  ffffffff81a4027c ffff88030a763af8 ffffffff81752699 0000000000000000
> :  ffff88030a763b48 ffff88030a763b38 ffffffff810974ca ffff88030a763b38
> :  ffffffff81a41123 000000000000026d 0000000000000000 0000000000000fa0
> : Call Trace:
> :  [<ffffffff81752699>] dump_stack+0x4c/0x65
> :  [<ffffffff810974ca>] warn_slowpath_common+0x8a/0xc0
> :  [<ffffffff81097546>] warn_slowpath_fmt+0x46/0x50
> :  [<ffffffff810db8ad>] ? prepare_to_wait_event+0x5d/0x110
> :  [<ffffffff810db8ad>] ? prepare_to_wait_event+0x5d/0x110
> :  [<ffffffff810bdaad>] __might_sleep+0xbd/0xd0
> :  [<ffffffff817566de>] mutex_lock_nested+0x2e/0x3e0
> :  [<ffffffff810db8e8>] ? prepare_to_wait_event+0x98/0x110
> :  [<ffffffffa00e43d7>] drm_dp_mst_wait_tx_reply+0xa7/0x220 [drm_kms_helper]
> :  [<ffffffff810db790>] ? wait_woken+0xc0/0xc0
> :  [<ffffffffa00e6401>] drm_dp_send_link_address+0x61/0x240 [drm_kms_helper]
> :  [<ffffffff810b1aaf>] ? process_one_work+0x14f/0x530
> :  [<ffffffffa00e6abd>] drm_dp_check_and_send_link_address+0x8d/0xa0 [drm_kms_helper]
> :  [<ffffffffa00e6aec>] drm_dp_mst_link_probe_work+0x1c/0x20 [drm_kms_helper]
> :  [<ffffffff810b1b26>] process_one_work+0x1c6/0x530
> :  [<ffffffff810b1aaf>] ? process_one_work+0x14f/0x530
> :  [<ffffffff810b224b>] worker_thread+0x6b/0x490
> :  [<ffffffff810b21e0>] ? rescuer_thread+0x350/0x350
> :  [<ffffffff810b73ea>] kthread+0x10a/0x120
> :  [<ffffffff8175a800>] ? _raw_spin_unlock_irq+0x30/0x50
> :  [<ffffffff810b72e0>] ? kthread_create_on_node+0x220/0x220
> :  [<ffffffff8175b1fc>] ret_from_fork+0x7c/0xb0
> :  [<ffffffff810b72e0>] ? kthread_create_on_node+0x220/0x220
> : ---[ end trace bb11c9634a7217c6 ]---
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Imo much simpler with the below diff. Not tested though.
-Daniel
--
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 9a5b68717ec8..379ab4555756 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -733,10 +733,14 @@ static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_sideband_msg_tx *txmsg)
 {
 	bool ret;
-	mutex_lock(&mgr->qlock);
+
+	/*
+	 * All updates to txmsg->state are protected by mgr->qlock, and the two
+	 * cases we check here are terminal states. For those the barriers
+	 * provided by the wake_up/wait_event pair are enough.
+	 */
 	ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
 	       txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
-	mutex_unlock(&mgr->qlock);
 	return ret;
 }
 
@@ -1363,12 +1367,13 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr,
 	return 0;
 }
 
-/* must be called holding qlock */
 static void process_single_down_tx_qlock(struct drm_dp_mst_topology_mgr *mgr)
 {
 	struct drm_dp_sideband_msg_tx *txmsg;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&mgr->qlock));
+
 	/* construct a chunk from the first msg in the tx_msg queue */
 	if (list_empty(&mgr->tx_msg_downq)) {
 		mgr->tx_down_in_progress = false;
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] drm/mst: fix recursive sleep warning on qlock
  2015-01-28  3:37 [PATCH] drm/mst: fix recursive sleep warning on qlock Dave Airlie
  2015-01-28  9:02 ` Daniel Vetter
@ 2015-02-01 10:52 ` shuang.he
  1 sibling, 0 replies; 3+ messages in thread
From: shuang.he @ 2015-02-01 10:52 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, airlied

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5672
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  355/355              355/355
SNB                                  400/422              400/422
IVB              +2                 485/487              487/487
BYT                                  296/296              296/296
HSW              +1-2              507/508              506/508
BDW                 -2              401/402              399/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(6, M34M21)PASS(9, M4M34)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(6, M34M4)PASS(16, M34M4M21)      PASS(1, M34)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(1, M40)PASS(20, M40M20)      PASS(1, M20)
*HSW  igt_gem_storedw_loop_blt      PASS(3, M40M20)      DMESG_WARN(1, M20)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(2, M20)PASS(4, M40M20)      DMESG_WARN(1, M20)
 BDW  igt_gem_pwrite_pread_display-pwrite-blt-gtt_mmap-performance      DMESG_WARN(5, M28)PASS(2, M30)      DMESG_WARN(1, M28)
*BDW  igt_gem_pwrite_pread_uncached-pwrite-blt-gtt_mmap-performance      PASS(7, M30M28)      DMESG_WARN(1, M28)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-02-01 10:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-28  3:37 [PATCH] drm/mst: fix recursive sleep warning on qlock Dave Airlie
2015-01-28  9:02 ` Daniel Vetter
2015-02-01 10:52 ` shuang.he

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox