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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 23 Sep 2015, Daniel Vetter <daniel@ffwll.ch> 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 <eich@suse.de>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> 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(&dev->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(&dev->mode_config.mutex);
>> -	__drm_kms_helper_poll_enable(dev);
>> +	drm_kms_helper_poll_enable_locked(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..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