| Message ID | 20170110151744.GK5700@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote: > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote: > > It was only needed to protect the connector_list walking, see > > > > commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Thu Jul 9 23:44:26 2015 +0200 > > > > drm/probe-helper: Grab mode_config.mutex in poll_init/enable > > > > Unfortunately the commit message of that patch fails to mention that > > the new locking check was for the connector_list. > > > > But that requirement disappeared in > > > > commit c36a3254f7857f1ad9badbe3578ccc92be541a8e > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Thu Dec 15 16:58:43 2016 +0100 > > > > drm: Convert all helpers to drm_connector_list_iter > > > > and so we can drop this again. > > > > This fixes a locking inversion on nouveau, where the rpm code needs to > > re-enable. But in other places the rpm_get() calls are nested within > > the big modeset locks. > > > > While at it, also improve the kerneldoc for these two functions a > > notch. > > > > Cc: Dave Airlie <airlied@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_probe_helper.c | 41 ++++++++++-------------------------- > > drivers/gpu/drm/i915/intel_hotplug.c | 4 ++-- > > include/drm/drm_crtc_helper.h | 1 - > > 3 files changed, 13 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > index 060211ac74a1..7c70f2a52250 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -115,25 +115,24 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > > > > #define DRM_OUTPUT_POLL_PERIOD (10*HZ) > > /** > > - * drm_kms_helper_poll_enable_locked - re-enable output polling. > > + * drm_kms_helper_poll_enable - re-enable output polling. > > * @dev: drm_device > > * > > - * This function re-enables the output polling work without > > - * locking the mode_config mutex. > > + * This function re-enables the output polling work, after it has been > > + * temporarily disabled using drm_kms_helper_poll_disable(), for example over > > + * suspend/resume. > > * > > - * This is like drm_kms_helper_poll_enable() however it is to be > > - * called from a context where the mode_config mutex is locked > > - * already. > > + * Drivers can call this helper from their device resume implementation. It is > > + * an error to call this when the output polling support has not yet been set > > + * up. > > */ > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev) > > +void drm_kms_helper_poll_enable(struct drm_device *dev) > > { > > bool poll = false; > > struct drm_connector *connector; > > struct drm_connector_list_iter conn_iter; > > unsigned long delay = DRM_OUTPUT_POLL_PERIOD; > > > > - WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); > > - > > if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > > return; > > Hmm, output_poll_execute() itself is not checking this correctly, > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 84b75709af90..cb3febc6e719 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work) > changed = dev->mode_config.delayed_event; > dev->mode_config.delayed_event = false; > > - if (!drm_kms_helper_poll) > + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > goto out; The cancel_work_sync in poll_disable should make this impossible, but it might be a good thing to check this with a WARN_ON? Care to type that patch as a follow up please? > if (!mutex_trylock(&dev->mode_config.mutex)) { > > The connector list is locked, but the other reads are all racy > (connector->polled, delayed_event). Those races seem intrinsic and handled > by e.g. intel_hotplug.c. > > Since the locking here was only covering the connector list and that now > has its own lock, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks for the review, I think I'll wait for Dave to supply bugzilla link or confirmation it solves his lockdep issue before merging. -Daniel
On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote: > On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote: > > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote: > > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev) > > > +void drm_kms_helper_poll_enable(struct drm_device *dev) > > > { > > > bool poll = false; > > > struct drm_connector *connector; > > > struct drm_connector_list_iter conn_iter; > > > unsigned long delay = DRM_OUTPUT_POLL_PERIOD; > > > > > > - WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); > > > - > > > if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > > > return; > > > > Hmm, output_poll_execute() itself is not checking this correctly, > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > index 84b75709af90..cb3febc6e719 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work) > > changed = dev->mode_config.delayed_event; > > dev->mode_config.delayed_event = false; > > > > - if (!drm_kms_helper_poll) > > + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > > goto out; > > The cancel_work_sync in poll_disable should make this impossible, but it > might be a good thing to check this with a WARN_ON? Care to type that > patch as a follow up please? Imagine a drm_kms_helper_poll_disable() run concurrently with drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check and begins iterating the list, meanwhile the disable() cancels the work and turns off the helper. The enable() is unaware of this and so reschedules the work, and as output_poll_execute() doesn't check for dev->mode_config.poll_enabled it stays active. -Chris
On Tue, Jan 10, 2017 at 05:10:50PM +0000, Chris Wilson wrote: > On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote: > > On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote: > > > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote: > > > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev) > > > > +void drm_kms_helper_poll_enable(struct drm_device *dev) > > > > { > > > > bool poll = false; > > > > struct drm_connector *connector; > > > > struct drm_connector_list_iter conn_iter; > > > > unsigned long delay = DRM_OUTPUT_POLL_PERIOD; > > > > > > > > - WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); > > > > - > > > > if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > > > > return; > > > > > > Hmm, output_poll_execute() itself is not checking this correctly, > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > > index 84b75709af90..cb3febc6e719 100644 > > > --- a/drivers/gpu/drm/drm_probe_helper.c > > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work) > > > changed = dev->mode_config.delayed_event; > > > dev->mode_config.delayed_event = false; > > > > > > - if (!drm_kms_helper_poll) > > > + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) > > > goto out; > > > > The cancel_work_sync in poll_disable should make this impossible, but it > > might be a good thing to check this with a WARN_ON? Care to type that > > patch as a follow up please? > > Imagine a drm_kms_helper_poll_disable() run concurrently with > drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check > and begins iterating the list, meanwhile the disable() cancels the work > and turns off the helper. The enable() is unaware of this and so > reschedules the work, and as output_poll_execute() doesn't check for > dev->mode_config.poll_enabled it stays active. I thought I also added that this case is a driver bug to the kernel-docs, but I failed. Driver needs to ensure this is all ordered reasonably well (which it is, if you only call it from suspend/resume callbacks). I'll respin with that fixed. -Daniel
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 84b75709af90..cb3febc6e719 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work) changed = dev->mode_config.delayed_event; dev->mode_config.delayed_event = false; - if (!drm_kms_helper_poll) + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) goto out; if (!mutex_trylock(&dev->mode_config.mutex)) {