[rfc] fix output poll execute lockdep

[rfc] fix output poll execute lockdep

[Dave Airlie]

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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: allow drivers to wrap output poll execution

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

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.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 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(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 97a3289..a2896d8 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -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);
 }
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 982c299..861f9b0 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -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
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 17942c0..e9fc078 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] nouveau: wrap the output poll execution and wakeup gpu

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

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.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/nouveau/nv50_display.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index cb85cb7..75502cd 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [rfc] fix output poll execute lockdep

[Daniel Vetter]

On Tue, Jan 10, 2017 at 12:12:30PM +1000, Dave Airlie wrote:
> 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.

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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [rfc] fix output poll execute lockdep

[Dave Airlie]

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

On 10 Jan. 2017 19:50, "Daniel Vetter" <daniel@ffwll.ch> wrote:

On Tue, Jan 10, 2017 at 12:12:30PM +1000, Dave Airlie wrote:
> 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.

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

[-- Attachment #1.2: Type: text/html, Size: 2467 bytes --]

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

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

Re: [rfc] fix output poll execute lockdep

[Daniel Vetter]

On Tue, Jan 10, 2017 at 09:46:26PM +1000, Dave Airlie wrote:
> On 10 Jan. 2017 19:50, "Daniel Vetter" <daniel@ffwll.ch> wrote:
> 
> On Tue, Jan 10, 2017 at 12:12:30PM +1000, Dave Airlie wrote:
> > 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.
> 
> 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.

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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel