fix output poll execute lockdep

On runtime resume, nouveau can try and take the mode_config mutex in the poll reenable, however a poll can race with this, and take the mutex and get stuck waiting for the runtime to finish completion. These two patches allow the driver to get hooked in before the mutex is taken.

I think radeon/amdgpu will also need similar patches to nouveau.

I found this while trying to track down a runtime regression on W541 laptop, I'm not sure I found what the reporter was seeing yet.

Dave.

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Dave Airlie <[email protected]>

In order to avoid a lockdep between pm resume and output poll over the mode_config mutex we need to order the wakeup of the device before we take the mode config mutex (as pm resume can also try and take the mode config mutex).

This introduces an API for drivers to insert themselves into the output poll path to do pm gets before taking the mutex.

---
 drivers/gpu/drm/drm_probe_helper.c | 25 ++++++++++++++++++++++---
 include/drm/drm_crtc_helper.h      |  2 +-
 include/drm/drm_mode_config.h      |  8 ++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

index 97a3289..a2896d8 100644
@@ -380,10 +380,15 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
 
-static void output_poll_execute(struct work_struct *work)
+/**
+ * drm_kms_helper_poll_outputs - poll all the outputs for hotplug
+ * @dev: drm_device to probe outputs on.
+ *
+ * This is to be called by drivers that need to wrap the output poll
+ * execution for power management purposes.
+ */
+bool drm_kms_helper_poll_outputs(struct drm_device *dev)
 {
-	struct delayed_work *delayed_work = to_delayed_work(work);
-	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;

@@ -463,6 +468,20 @@ static void output_poll_execute(struct work_struct *work)
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
 
+	return repoll;
+}
+EXPORT_SYMBOL(drm_kms_helper_poll_outputs);
+
+static void output_poll_execute(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
+	bool repoll;
+
+	if (dev->mode_config.funcs->output_poll_execute)
+		repoll = dev->mode_config.funcs->output_poll_execute(dev);
+	else
+		repoll = drm_kms_helper_poll_outputs(dev);
 	if (repoll)
 		schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
 }

index 982c299..861f9b0 100644
@@ -74,5 +74,5 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
-
+extern bool drm_kms_helper_poll_outputs(struct drm_device *dev);
 #endif

index 17942c0..e9fc078 100644
@@ -87,6 +87,14 @@ struct drm_mode_config_funcs {
 	void (*output_poll_changed)(struct drm_device *dev);
 
 	/**
+	 * @output_poll_execute:
+	 *
+	 * Used to allow driver to wrap the output polling for power management
+	 * purposes and avoid locking issues.
+	 */
+	bool (*output_poll_execute)(struct drm_device *dev);
+
+	/**
 	 * @atomic_check:
 	 *
 	 * This is the only hook to validate an atomic modeset update. This

2.9.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Dave Airlie <[email protected]>

This wraps the output poll and fixes a problem where pm resume would try and take the mode config mutex but the output poll thread would already have taken it.

I found this while looking for another bug, I've no idea yet if this fixes that problem.

---
 drivers/gpu/drm/nouveau/nv50_display.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index cb85cb7..75502cd 100644
@@ -4323,10 +4323,26 @@ nv50_disp_atomic_state_alloc(struct drm_device *dev)
 	return &atom->state;
 }
 
+static bool
+nv50_disp_output_poll_execute(struct drm_device *dev)
+{
+	int ret;
+	bool repoll;
+	ret = pm_runtime_get_sync(dev->dev);
+	if (ret < 0 && ret != -EACCES)
+		return true;
+
+	repoll = drm_kms_helper_poll_outputs(dev);
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+	return repoll;
+}
+
 static const struct drm_mode_config_funcs
 nv50_disp_func = {
 	.fb_create = nouveau_user_framebuffer_create,
 	.output_poll_changed = nouveau_fbcon_output_poll_changed,
+	.output_poll_execute = nv50_disp_output_poll_execute,
 	.atomic_check = nv50_disp_atomic_check,
 	.atomic_commit = nv50_disp_atomic_commit,
 	.atomic_state_alloc = nv50_disp_atomic_state_alloc,

2.9.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hm, we fixed this by doing the rpm stuff always within any of the big core locks. And I think that's the much more reasonable design, at least as soon as you have more fine-grained locking.

But maybe there's a cheap way out - why does nouveau take the modeset lock? Ime you can remove a lot of the kms locking super easily because it's all cargo-culted. The last hold-out was connector_list walking, but that's fixed now and also doesn't need any of the heavy-weight kms locks anymore.

Asking since if you really need kms locks in rpm paths, and the general design is to grab/drop rpm outside of everything (as nouveau does with the overall ioctl wrapper trick), then there's fundamentally a deadlock, or at least a nice inversion. It shouldn't really be needed ... -Daniel

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 10 Jan. 2017 19:50, "Daniel Vetter" <[email protected]> wrote:

Hm, we fixed this by doing the rpm stuff always within any of the big core locks. And I think that's the much more reasonable design, at least as soon as you have more fine-grained locking.

But maybe there's a cheap way out - why does nouveau take the modeset lock? Ime you can remove a lot of the kms locking super easily because it's all cargo-culted. The last hold-out was connector_list walking, but that's fixed now and also doesn't need any of the heavy-weight kms locks anymore.

Reenable polling takes the lock.

Dave.

Asking since if you really need kms locks in rpm paths, and the general design is to grab/drop rpm outside of everything (as nouveau does with the overall ioctl wrapper trick), then there's fundamentally a deadlock, or at least a nice inversion. It shouldn't really be needed ... -Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

I think with the revamped connector_locking we can nuke that. All the other stuff checked (poll_enabled and similar) are either intentionally racy, or synchronized through the right order of the driver setup/teardown code.

I'll be typing some patches.
-Daniel

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel