Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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
> ---
> 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(>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(>mode_config.mutex);
> - __drm_kms_helper_poll_enable(dev);
> + drm_kms_helper_poll_enable_no_lock(dev);
> mutex_unlock(>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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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
---
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)
{
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(>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(>mode_config.mutex);
- __drm_kms_helper_poll_enable(dev);
+ drm_kms_helper_poll_enable_no_lock(dev);
mutex_unlock(>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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().
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.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
