| Message ID | 7831d4f21dde8cd0b1659e393854598995dfb249.1518338788.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote: > Introduce a helper to determine if the current task is an output poll > worker. > > This allows us to fix a long-standing deadlock in several DRM drivers > wherein the ->runtime_suspend callback waits for the output poll worker > to finish and the worker in turn calls a ->detect callback which waits > for runtime suspend to finish. The ->detect callback is invoked from > multiple call sites and waiting for runtime suspend to finish is the > correct thing to do except if it's executing in the context of the > worker. > > Cc: Dave Airlie <airlied@redhat.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++ > include/drm/drm_crtc_helper.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > b/drivers/gpu/drm/drm_probe_helper.c > index 555fbe54d6e2..019881d15ce1 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct > *work) > schedule_delayed_work(delayed_work, > DRM_OUTPUT_POLL_PERIOD); > } > > +/** > + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? > + * > + * Determine if %current task is an output poll worker. This can be used > + * to select distinct code paths for output polling versus other contexts. > + */ For this, it would be worth explicitly noting in the comments herethat this should be called by DRM drivers in order to prevent racing with hotplug polling workers, so that new drivers in the future can avoid implementing this race condition in their driver. > +bool drm_kms_helper_is_poll_worker(void) > +{ > + struct work_struct *work = current_work(); > + > + return work && work->func == output_poll_execute; > +} > +EXPORT_SYMBOL(drm_kms_helper_is_poll_worker); > + > /** > * drm_kms_helper_poll_disable - disable output polling > * @dev: drm_device > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h > index 76e237bd989b..6914633037a5 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev); > > void drm_kms_helper_poll_disable(struct drm_device *dev); > void drm_kms_helper_poll_enable(struct drm_device *dev); > +bool drm_kms_helper_is_poll_worker(void); > > #endif
Quoting Lyude Paul (2018-02-12 17:46:11) > On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote: > > Introduce a helper to determine if the current task is an output poll > > worker. > > > > This allows us to fix a long-standing deadlock in several DRM drivers > > wherein the ->runtime_suspend callback waits for the output poll worker > > to finish and the worker in turn calls a ->detect callback which waits > > for runtime suspend to finish. The ->detect callback is invoked from > > multiple call sites and waiting for runtime suspend to finish is the > > correct thing to do except if it's executing in the context of the > > worker. > > > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > --- > > drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++ > > include/drm/drm_crtc_helper.h | 1 + > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > > b/drivers/gpu/drm/drm_probe_helper.c > > index 555fbe54d6e2..019881d15ce1 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct > > *work) > > schedule_delayed_work(delayed_work, > > DRM_OUTPUT_POLL_PERIOD); > > } > > > > +/** > > + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? > > + * > > + * Determine if %current task is an output poll worker. This can be used > > + * to select distinct code paths for output polling versus other contexts. > > + */ > For this, it would be worth explicitly noting in the comments herethat this > should be called by DRM drivers in order to prevent racing with hotplug > polling workers, so that new drivers in the future can avoid implementing this > race condition in their driver. > > > +bool drm_kms_helper_is_poll_worker(void) > > +{ > > + struct work_struct *work = current_work(); > > + > > + return work && work->func == output_poll_execute; What ensures that work is accessible? Does this need rcu_read_lock protection or more? -Chris
On Mon, Feb 12, 2018 at 05:50:12PM +0000, Chris Wilson wrote: > Quoting Lyude Paul (2018-02-12 17:46:11) > > On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote: > > > Introduce a helper to determine if the current task is an output poll > > > worker. > > > > > > This allows us to fix a long-standing deadlock in several DRM drivers > > > wherein the ->runtime_suspend callback waits for the output poll worker > > > to finish and the worker in turn calls a ->detect callback which waits > > > for runtime suspend to finish. The ->detect callback is invoked from > > > multiple call sites and waiting for runtime suspend to finish is the > > > correct thing to do except if it's executing in the context of the > > > worker. > > > > > > Cc: Dave Airlie <airlied@redhat.com> > > > Cc: Ben Skeggs <bskeggs@redhat.com> > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > --- > > > drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++ > > > include/drm/drm_crtc_helper.h | 1 + > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > > > b/drivers/gpu/drm/drm_probe_helper.c > > > index 555fbe54d6e2..019881d15ce1 100644 > > > --- a/drivers/gpu/drm/drm_probe_helper.c > > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > > @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct > > > *work) > > > schedule_delayed_work(delayed_work, > > > DRM_OUTPUT_POLL_PERIOD); > > > } > > > > > > +/** > > > + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? > > > + * > > > + * Determine if %current task is an output poll worker. This can be used > > > + * to select distinct code paths for output polling versus other contexts. > > > + */ > > > +bool drm_kms_helper_is_poll_worker(void) > > > +{ > > > + struct work_struct *work = current_work(); > > > + > > > + return work && work->func == output_poll_execute; > > What ensures that work is accessible? Does this need rcu_read_lock > protection or more? The work_struct exists as long this kthread is working on it. Since this is called by the kthread itself, it is guaranteed to exist. So there's no protection needed. Thanks, Lukas
On Mon, Feb 12, 2018 at 12:46:11PM -0500, Lyude Paul wrote: > On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote: > > Introduce a helper to determine if the current task is an output poll > > worker. > > > > This allows us to fix a long-standing deadlock in several DRM drivers > > wherein the ->runtime_suspend callback waits for the output poll worker > > to finish and the worker in turn calls a ->detect callback which waits > > for runtime suspend to finish. The ->detect callback is invoked from > > multiple call sites and waiting for runtime suspend to finish is the > > correct thing to do except if it's executing in the context of the > > worker. [snip] > > +/** > > + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? > > + * > > + * Determine if %current task is an output poll worker. This can be used > > + * to select distinct code paths for output polling versus other contexts. > > + */ > > For this, it would be worth explicitly noting in the comments herethat this > should be called by DRM drivers in order to prevent racing with hotplug > polling workers, so that new drivers in the future can avoid implementing this > race condition in their driver. Good point, I've just sent out a v2 to address your comment. Let me know if this isn't what you had in mind. It may also be worth to expand the DOC section at the top of drm_probe_helper.c to explain the interaction between polling and runtime suspend in more detail, but I think this is better done in a separate patch to keep the present patch small and thus easily backportable to stable. Thanks a lot for the review, Lukas
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 555fbe54d6e2..019881d15ce1 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct *work) schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD); } +/** + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? + * + * Determine if %current task is an output poll worker. This can be used + * to select distinct code paths for output polling versus other contexts. + */ +bool drm_kms_helper_is_poll_worker(void) +{ + struct work_struct *work = current_work(); + + return work && work->func == output_poll_execute; +} +EXPORT_SYMBOL(drm_kms_helper_is_poll_worker); + /** * drm_kms_helper_poll_disable - disable output polling * @dev: drm_device diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 76e237bd989b..6914633037a5 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev); void drm_kms_helper_poll_disable(struct drm_device *dev); void drm_kms_helper_poll_enable(struct drm_device *dev); +bool drm_kms_helper_is_poll_worker(void); #endif
Introduce a helper to determine if the current task is an output poll worker. This allows us to fix a long-standing deadlock in several DRM drivers wherein the ->runtime_suspend callback waits for the output poll worker to finish and the worker in turn calls a ->detect callback which waits for runtime suspend to finish. The ->detect callback is invoked from multiple call sites and waiting for runtime suspend to finish is the correct thing to do except if it's executing in the context of the worker. Cc: Dave Airlie <airlied@redhat.com> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++ include/drm/drm_crtc_helper.h | 1 + 2 files changed, 15 insertions(+)