Re: [PATCH 05/14] drm/i915: Remove delay for idle_work

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/05/2019 12:45, Chris Wilson wrote:
> The original intent for the delay before running the idle_work was to
> provide a hysteresis to avoid ping-ponging the device runtime-pm. Since
> then we have also pulled in some memory management and general device
> management for parking. But with the inversion of the wakeref handling,
> GEM is no longer responsible for the wakeref and by the time we call the
> idle_work, the device is asleep. It seems appropriate now to drop the
> delay and just run the worker immediately to flush the cached GEM state
> before sleeping.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c           |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h               |  2 +-
>   drivers/gpu/drm/i915/i915_gem_pm.c            | 21 +++++++------------
>   .../gpu/drm/i915/selftests/i915_gem_object.c  |  2 +-
>   .../gpu/drm/i915/selftests/mock_gem_device.c  |  4 ++--
>   5 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0e4dffcd4da4..7e8898d0b78b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3935,8 +3935,8 @@ i915_drop_caches_set(void *data, u64 val)
>   	if (val & DROP_IDLE) {
>   		do {
>   			flush_delayed_work(&i915->gem.retire_work);
> -			drain_delayed_work(&i915->gem.idle_work);
>   		} while (READ_ONCE(i915->gt.awake));
> +		flush_work(&i915->gem.idle_work);
>   	}
>   
>   	if (val & DROP_FREED)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 13270e19eb87..cbf4a7d8bdae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2035,7 +2035,7 @@ struct drm_i915_private {
>   		 * arrive within a small period of time, we fire
>   		 * off the idle_work.
>   		 */
> -		struct delayed_work idle_work;
> +		struct work_struct idle_work;
>   	} gem;
>   
>   	/* For i945gm vblank irq vs. C3 workaround */
> diff --git a/drivers/gpu/drm/i915/i915_gem_pm.c b/drivers/gpu/drm/i915/i915_gem_pm.c
> index 49b0ce594f20..ae91ad7cb31e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/i915_gem_pm.c
> @@ -29,12 +29,12 @@ static void i915_gem_park(struct drm_i915_private *i915)
>   static void idle_work_handler(struct work_struct *work)
>   {
>   	struct drm_i915_private *i915 =
> -		container_of(work, typeof(*i915), gem.idle_work.work);
> +		container_of(work, typeof(*i915), gem.idle_work);
>   
>   	mutex_lock(&i915->drm.struct_mutex);
>   
>   	intel_wakeref_lock(&i915->gt.wakeref);
> -	if (!intel_wakeref_active(&i915->gt.wakeref))
> +	if (!intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work))

What is the reason for the !work_pending check?

Regards,

Tvrtko

>   		i915_gem_park(i915);
>   	intel_wakeref_unlock(&i915->gt.wakeref);
>   
> @@ -74,9 +74,7 @@ static int pm_notifier(struct notifier_block *nb,
>   		break;
>   
>   	case INTEL_GT_PARK:
> -		mod_delayed_work(i915->wq,
> -				 &i915->gem.idle_work,
> -				 msecs_to_jiffies(100));
> +		queue_work(i915->wq, &i915->gem.idle_work);
>   		break;
>   	}
>   
> @@ -142,16 +140,11 @@ void i915_gem_suspend(struct drm_i915_private *i915)
>   	 * Assert that we successfully flushed all the work and
>   	 * reset the GPU back to its idle, low power state.
>   	 */
> -	GEM_BUG_ON(i915->gt.awake);
> -	cancel_delayed_work_sync(&i915->gpu_error.hangcheck_work);
> -
>   	drain_delayed_work(&i915->gem.retire_work);
> +	GEM_BUG_ON(i915->gt.awake);
> +	flush_work(&i915->gem.idle_work);
>   
> -	/*
> -	 * As the idle_work is rearming if it detects a race, play safe and
> -	 * repeat the flush until it is definitely idle.
> -	 */
> -	drain_delayed_work(&i915->gem.idle_work);
> +	cancel_delayed_work_sync(&i915->gpu_error.hangcheck_work);
>   
>   	i915_gem_drain_freed_objects(i915);
>   
> @@ -242,7 +235,7 @@ void i915_gem_resume(struct drm_i915_private *i915)
>   
>   void i915_gem_init__pm(struct drm_i915_private *i915)
>   {
> -	INIT_DELAYED_WORK(&i915->gem.idle_work, idle_work_handler);
> +	INIT_WORK(&i915->gem.idle_work, idle_work_handler);
>   	INIT_DELAYED_WORK(&i915->gem.retire_work, retire_work_handler);
>   
>   	i915->gem.pm_notifier.notifier_call = pm_notifier;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_object.c b/drivers/gpu/drm/i915/selftests/i915_gem_object.c
> index 088b2aa05dcd..b926d1cd165d 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_object.c
> @@ -509,7 +509,7 @@ static void disable_retire_worker(struct drm_i915_private *i915)
>   	intel_gt_pm_get(i915);
>   
>   	cancel_delayed_work_sync(&i915->gem.retire_work);
> -	cancel_delayed_work_sync(&i915->gem.idle_work);
> +	flush_work(&i915->gem.idle_work);
>   }
>   
>   static void restore_retire_worker(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index e4033d0576c4..d919f512042c 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -59,7 +59,7 @@ static void mock_device_release(struct drm_device *dev)
>   	mutex_unlock(&i915->drm.struct_mutex);
>   
>   	drain_delayed_work(&i915->gem.retire_work);
> -	drain_delayed_work(&i915->gem.idle_work);
> +	flush_work(&i915->gem.idle_work);
>   	i915_gem_drain_workqueue(i915);
>   
>   	mutex_lock(&i915->drm.struct_mutex);
> @@ -195,7 +195,7 @@ struct drm_i915_private *mock_gem_device(void)
>   	mock_init_contexts(i915);
>   
>   	INIT_DELAYED_WORK(&i915->gem.retire_work, mock_retire_work_handler);
> -	INIT_DELAYED_WORK(&i915->gem.idle_work, mock_idle_work_handler);
> +	INIT_WORK(&i915->gem.idle_work, mock_idle_work_handler);
>   
>   	i915->gt.awake = true;
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx