Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Wed, 23 Sep 2015, Daniel Vetter  wrote:
> On Wed, Sep 23, 2015 at 04:13:00PM +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.
>> 
>> Changes since v1:
>> - use function name suffix '_locked' for the function that
>>   is to be called from a locked context.
>> 
>> Signed-off-by: Egbert Eich 
>
> Reviewed-by: Daniel Vetter 
>
> Jani can you please pick these two up for -fixes since the 2nd patch fixes
> a regression?

Pushed to drm-intel-fixes, thanks for the patches and review.

BR,
Jani.


>
> Thanks, Daniel
>
>> ---
>>  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..2b9ce37 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_locked - 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_locked(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_locked);
>> +
>>  
>>  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_locked(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_locked(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..3febb4b 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_locked(struct drm_device *dev);
>>  
>>  #endif
>> -- 
>> 1.8.4.5
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Fri, 25 Sep 2015, Jani Nikula  wrote:
> On Fri, 25 Sep 2015, Egbert Eich  wrote:
>> Jani Nikula writes:
>>  > 
>>  > Shouldn't this be _unlocked?
>>  > 
>>  > I thought the convention was that functions that do not acquire locks
>>  > are called _unlocked (although they may require a lock to be held when
>>  > called). And you might have foo() that grabs locks around a call to
>>  > foo_unlocked().
>>  > 
>>
>> Looking into this, functions that are to be called in a context where
>> the lock is already held should receive the suffix _locked while
>> those which do locking themselves and thus need to be called from
>> a context that doesn't hold this lock already receive the suffix 
>> _unlocked: the past tense refers to what has happened before.
>
> I'm afraid existing conventions trump what makes sense.

Egbert, I'm full of shit. Sorry. $BEVERAGE on me next time.

I'll queue these once I figure out through which tree.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

Jani Nikula writes:
 > 
 > Shouldn't this be _unlocked?
 > 
 > I thought the convention was that functions that do not acquire locks
 > are called _unlocked (although they may require a lock to be held when
 > called). And you might have foo() that grabs locks around a call to
 > foo_unlocked().
 > 

Looking into this, functions that are to be called in a context where
the lock is already held should receive the suffix _locked while
those which do locking themselves and thus need to be called from
a context that doesn't hold this lock already receive the suffix 
_unlocked: the past tense refers to what has happened before.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Fri, 25 Sep 2015, Egbert Eich  wrote:
> Jani Nikula writes:
>  > 
>  > Shouldn't this be _unlocked?
>  > 
>  > I thought the convention was that functions that do not acquire locks
>  > are called _unlocked (although they may require a lock to be held when
>  > called). And you might have foo() that grabs locks around a call to
>  > foo_unlocked().
>  > 
>
> Looking into this, functions that are to be called in a context where
> the lock is already held should receive the suffix _locked while
> those which do locking themselves and thus need to be called from
> a context that doesn't hold this lock already receive the suffix 
> _unlocked: the past tense refers to what has happened before.

I'm afraid existing conventions trump what makes sense.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Wed, 23 Sep 2015, Daniel Vetter  wrote:
> On Wed, Sep 23, 2015 at 04:13:00PM +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.
>> 
>> Changes since v1:
>> - use function name suffix '_locked' for the function that
>>   is to be called from a locked context.
>> 
>> Signed-off-by: Egbert Eich 
>
> Reviewed-by: Daniel Vetter 
>
> Jani can you please pick these two up for -fixes since the 2nd patch fixes
> a regression?
>
> Thanks, Daniel
>
>> ---
>>  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..2b9ce37 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_locked - 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_locked(struct drm_device *dev)

Shouldn't this be _unlocked?

I thought the convention was that functions that do not acquire locks
are called _unlocked (although they may require a lock to be held when
called). And you might have foo() that grabs locks around a call to
foo_unlocked().

BR,
Jani.



>>  {
>>  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_locked);
>> +
>>  
>>  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_locked(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_locked(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..3febb4b 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_locked(struct drm_device *dev);
>>  
>>  #endif
>> -- 
>> 1.8.4.5
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

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.

Changes since v1:
- use function name suffix '_locked' for the function that
  is to be called from a locked context.

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..2b9ce37 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_locked - 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_locked(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_locked);
+
 
 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_locked(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_locked(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..3febb4b 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_locked(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/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Daniel Vetter]

On Wed, Sep 23, 2015 at 04:13:00PM +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.
> 
> Changes since v1:
> - use function name suffix '_locked' for the function that
>   is to be called from a locked context.
> 
> Signed-off-by: Egbert Eich 

Reviewed-by: Daniel Vetter 

Jani can you please pick these two up for -fixes since the 2nd patch fixes
a regression?

Thanks, Daniel

> ---
>  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..2b9ce37 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_locked - 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_locked(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_locked);
> +
>  
>  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_locked(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_locked(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..3febb4b 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_locked(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