Re: [Intel-gfx] [PATCH 05/14] drm/i915: Remove delay for idle_work
Quoting Tvrtko Ursulin (2019-05-02 14:51:31)
>
> On 02/05/2019 14:22, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-02 14:19:38)
> >>
> >> On 01/05/2019 12:45, Chris Wilson wrote:
> >>> 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(>drm.struct_mutex);
> >>>
> >>>intel_wakeref_lock(>gt.wakeref);
> >>> - if (!intel_wakeref_active(>gt.wakeref))
> >>> + if (!intel_wakeref_active(>gt.wakeref) && !work_pending(work))
> >>
> >> What is the reason for the !work_pending check?
> >
> > Just that we are going to be called again, so wait until the next time to
> > see if we still need to park.
>
> When does it get called again? If a whole new cycle of unpark-park
> happened before the previous park was able to finish?
work_pending() implies that we've done at least one cycle while we
waited for the locks and the work is already queued to be rerun.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/14] drm/i915: Remove delay for idle_work
On 02/05/2019 14:22, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-02 14:19:38)
On 01/05/2019 12:45, Chris Wilson wrote:
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(>drm.struct_mutex);
intel_wakeref_lock(>gt.wakeref);
- if (!intel_wakeref_active(>gt.wakeref))
+ if (!intel_wakeref_active(>gt.wakeref) && !work_pending(work))
What is the reason for the !work_pending check?
Just that we are going to be called again, so wait until the next time to
see if we still need to park.
When does it get called again? If a whole new cycle of unpark-park
happened before the previous park was able to finish?
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/14] drm/i915: Remove delay for idle_work
Quoting Tvrtko Ursulin (2019-05-02 14:19:38)
>
> On 01/05/2019 12:45, Chris Wilson wrote:
> > 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(>drm.struct_mutex);
> >
> > intel_wakeref_lock(>gt.wakeref);
> > - if (!intel_wakeref_active(>gt.wakeref))
> > + if (!intel_wakeref_active(>gt.wakeref) && !work_pending(work))
>
> What is the reason for the !work_pending check?
Just that we are going to be called again, so wait until the next time to
see if we still need to park.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/14] drm/i915: Remove delay for idle_work
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
---
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(>gem.retire_work);
- drain_delayed_work(>gem.idle_work);
} while (READ_ONCE(i915->gt.awake));
+ flush_work(>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(>drm.struct_mutex);
intel_wakeref_lock(>gt.wakeref);
- if (!intel_wakeref_active(>gt.wakeref))
+ if (!intel_wakeref_active(>gt.wakeref) && !work_pending(work))
What is the reason for the !work_pending check?
Regards,
Tvrtko
i915_gem_park(i915);
intel_wakeref_unlock(>gt.wakeref);
@@ -74,9 +74,7 @@ static int pm_notifier(struct notifier_block *nb,
break;
case INTEL_GT_PARK:
- mod_delayed_work(i915->wq,
->gem.idle_work,
-msecs_to_jiffies(100));
+ queue_work(i915->wq, >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(>gpu_error.hangcheck_work);
-
drain_delayed_work(>gem.retire_work);
+ GEM_BUG_ON(i915->gt.awake);
+ flush_work(>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(>gem.idle_work);
+ cancel_delayed_work_sync(>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(>gem.idle_work, idle_work_handler);
+ INIT_WORK(>gem.idle_work, idle_work_handler);
INIT_DELAYED_WORK(>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(>gem.retire_work);
- cancel_delayed_work_sync(>gem.idle_work);
+ flush_work(>gem.idle_work);
}
static void restore_retire_worker(struct drm_i915_private *i915)
diff
[Intel-gfx] [PATCH 05/14] drm/i915: Remove delay for idle_work
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
---
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(>gem.retire_work);
- drain_delayed_work(>gem.idle_work);
} while (READ_ONCE(i915->gt.awake));
+ flush_work(>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(>drm.struct_mutex);
intel_wakeref_lock(>gt.wakeref);
- if (!intel_wakeref_active(>gt.wakeref))
+ if (!intel_wakeref_active(>gt.wakeref) && !work_pending(work))
i915_gem_park(i915);
intel_wakeref_unlock(>gt.wakeref);
@@ -74,9 +74,7 @@ static int pm_notifier(struct notifier_block *nb,
break;
case INTEL_GT_PARK:
- mod_delayed_work(i915->wq,
->gem.idle_work,
-msecs_to_jiffies(100));
+ queue_work(i915->wq, >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(>gpu_error.hangcheck_work);
-
drain_delayed_work(>gem.retire_work);
+ GEM_BUG_ON(i915->gt.awake);
+ flush_work(>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(>gem.idle_work);
+ cancel_delayed_work_sync(>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(>gem.idle_work, idle_work_handler);
+ INIT_WORK(>gem.idle_work, idle_work_handler);
INIT_DELAYED_WORK(>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(>gem.retire_work);
- cancel_delayed_work_sync(>gem.idle_work);
+ flush_work(>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
