| Message ID | 1441138895-23732-2-git-send-email-eich@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Egbert, On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote: > drm_kms_helper_poll_enable() was converted to lock the mode_config > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). > > This disregarded the cases where this function is called from a context > where this mutex is already locked. Which ones would that be? > + * drm_kms_helper_poll_enable_no_lock - re-enable output polling. It seems DRM convention is to append _locked or _unlocked, e.g.: drm_fb_helper_restore_fbdev_mode_unlocked drm_gem_object_unreference_unlocked Best regards, Lukas
Lukas Wunner writes: > Hi Egbert, > > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote: > > drm_kms_helper_poll_enable() was converted to lock the mode_config > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). > > > > This disregarded the cases where this function is called from a context > > where this mutex is already locked. > > Which ones would that be? Have you missed the next patch in the series? Egbert.
Hi Egbert, On Wed, Sep 02, 2015 at 12:10:19AM +0200, Egbert Eich wrote: > Lukas Wunner writes: > > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote: > > > drm_kms_helper_poll_enable() was converted to lock the mode_config > > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). > > > > > > This disregarded the cases where this function is called from a context > > > where this mutex is already locked. > > > > Which ones would that be? > > Have you missed the next patch in the series? Sorry, I should have asked more precisely: Is this the only one? If so it may make sense to modify i915_hotplug_work_func or intel_hpd_irq_storm_disable instead. Best regards, Lukas
Lukas Wunner writes: > > It seems DRM convention is to append _locked or _unlocked, e.g.: > drm_fb_helper_restore_fbdev_mode_unlocked > drm_gem_object_unreference_unlocked > Oh, I missed that. Did you check what these functions actually do - and compare it to what I try to achieve? Egbert.
Lukas Wunner writes: > Hi Egbert, > > On Wed, Sep 02, 2015 at 12:10:19AM +0200, Egbert Eich wrote: > > Lukas Wunner writes: > > > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote: > > > > drm_kms_helper_poll_enable() was converted to lock the mode_config > > > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > > > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). > > > > > > > > This disregarded the cases where this function is called from a context > > > > where this mutex is already locked. > > > > > > Which ones would that be? > > > > Have you missed the next patch in the series? > > Sorry, I should have asked more precisely: Is this the only one? > If so it may make sense to modify i915_hotplug_work_func or > intel_hpd_irq_storm_disable instead. > The non-locking version existed before. I've just exported it so it cah be used by drivers. Moreover this is a helper function - why should it be restricted to callers which don't have the mode_config mutex grabbed? I could be talked into changing the log message. Egbert.
On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote: > drm_kms_helper_poll_enable() was converted to lock the mode_config > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). > > This disregarded the cases where this function is called from a context > where this mutex is already locked. > > Add a non-locking version as well. > > Signed-off-by: Egbert Eich <eich@suse.de> > --- > drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++--- > include/drm/drm_crtc_helper.h | 1 + > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index d734780..a93ad1b 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > return 1; > } > > +/** > + * drm_kms_helper_poll_enable_no_lock - re-enable output polling. > + * @dev: drm_device > + * > + * This function re-enables the output polling work without > + * locking the mode_config mutex. > + * > + * 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. > + */ > #define DRM_OUTPUT_POLL_PERIOD (10*HZ) > -static void __drm_kms_helper_poll_enable(struct drm_device *dev) > +void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev) Please use _locked to stay consistent with other such functions. With that address this lgtm. -Daniel > { > bool poll = false; > struct drm_connector *connector; > @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev) > if (poll) > schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); > } > +EXPORT_SYMBOL(drm_kms_helper_poll_enable_no_lock); > + > > static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector, > uint32_t maxX, uint32_t maxY, bool merge_type_bits) > @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect > > /* Re-enable polling in case the global poll config changed. */ > if (drm_kms_helper_poll != dev->mode_config.poll_running) > - __drm_kms_helper_poll_enable(dev); > + drm_kms_helper_poll_enable_no_lock(dev); > > dev->mode_config.poll_running = drm_kms_helper_poll; > > @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable); > void drm_kms_helper_poll_enable(struct drm_device *dev) > { > mutex_lock(&dev->mode_config.mutex); > - __drm_kms_helper_poll_enable(dev); > + drm_kms_helper_poll_enable_no_lock(dev); > mutex_unlock(&dev->mode_config.mutex); > } > EXPORT_SYMBOL(drm_kms_helper_poll_enable); > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h > index 2a747a9..f96703d 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -240,5 +240,6 @@ 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_no_lock(struct drm_device *dev); > > #endif > -- > 1.8.4.5 >
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index d734780..a93ad1b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) return 1; } +/** + * drm_kms_helper_poll_enable_no_lock - re-enable output polling. + * @dev: drm_device + * + * This function re-enables the output polling work without + * locking the mode_config mutex. + * + * 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. + */ #define DRM_OUTPUT_POLL_PERIOD (10*HZ) -static void __drm_kms_helper_poll_enable(struct drm_device *dev) +void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev) { bool poll = false; struct drm_connector *connector; @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev) if (poll) schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); } +EXPORT_SYMBOL(drm_kms_helper_poll_enable_no_lock); + static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector, uint32_t maxX, uint32_t maxY, bool merge_type_bits) @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect /* Re-enable polling in case the global poll config changed. */ if (drm_kms_helper_poll != dev->mode_config.poll_running) - __drm_kms_helper_poll_enable(dev); + drm_kms_helper_poll_enable_no_lock(dev); dev->mode_config.poll_running = drm_kms_helper_poll; @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable); void drm_kms_helper_poll_enable(struct drm_device *dev) { mutex_lock(&dev->mode_config.mutex); - __drm_kms_helper_poll_enable(dev); + drm_kms_helper_poll_enable_no_lock(dev); mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_kms_helper_poll_enable); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 2a747a9..f96703d 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -240,5 +240,6 @@ 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_no_lock(struct drm_device *dev); #endif
drm_kms_helper_poll_enable() was converted to lock the mode_config mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable"). This disregarded the cases where this function is called from a context where this mutex is already locked. Add a non-locking version as well. Signed-off-by: Egbert Eich <eich@suse.de> --- drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++--- include/drm/drm_crtc_helper.h | 1 + 2 files changed, 17 insertions(+), 3 deletions(-)