| Message ID | 20190614071023.17929-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/39] drm/i915: Discard some redundant cache domain flushes | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > While we need to flush the wakeref before parking, we do not need to > perform the i915_gem_park() itself underneath the wakeref lock, merely > the struct_mutex. If we rearrange the locks, we can avoid the unnecessary > tainting. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gem/i915_gem_pm.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > index 6e75702c5671..a33f69610d6f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > @@ -30,23 +30,22 @@ static void idle_work_handler(struct work_struct *work) > { > struct drm_i915_private *i915 = > container_of(work, typeof(*i915), gem.idle_work); > - bool restart = true; > + bool park; > can_park...meh. > - cancel_delayed_work(&i915->gem.retire_work); > + cancel_delayed_work_sync(&i915->gem.retire_work); > mutex_lock(&i915->drm.struct_mutex); > > intel_wakeref_lock(&i915->gt.wakeref); > - if (!intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work)) { > - i915_gem_park(i915); > - restart = false; > - } > + park = !intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work); > intel_wakeref_unlock(&i915->gt.wakeref); > - > - mutex_unlock(&i915->drm.struct_mutex); > - if (restart) > + if (park) > + i915_gem_park(i915); Did not find anything beneath gem park that would need wakeref. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > + else > queue_delayed_work(i915->wq, > &i915->gem.retire_work, > round_jiffies_up_relative(HZ)); > + > + mutex_unlock(&i915->drm.struct_mutex); > } > > static void retire_work_handler(struct work_struct *work) > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2019-06-14 15:47:46) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > While we need to flush the wakeref before parking, we do not need to > > perform the i915_gem_park() itself underneath the wakeref lock, merely > > the struct_mutex. If we rearrange the locks, we can avoid the unnecessary > > tainting. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_pm.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > > index 6e75702c5671..a33f69610d6f 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > > @@ -30,23 +30,22 @@ static void idle_work_handler(struct work_struct *work) > > { > > struct drm_i915_private *i915 = > > container_of(work, typeof(*i915), gem.idle_work); > > - bool restart = true; > > + bool park; > > > > can_park...meh. > > > - cancel_delayed_work(&i915->gem.retire_work); > > + cancel_delayed_work_sync(&i915->gem.retire_work); > > mutex_lock(&i915->drm.struct_mutex); > > > > intel_wakeref_lock(&i915->gt.wakeref); > > - if (!intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work)) { > > - i915_gem_park(i915); > > - restart = false; > > - } > > + park = !intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work); > > intel_wakeref_unlock(&i915->gt.wakeref); > > - > > - mutex_unlock(&i915->drm.struct_mutex); > > - if (restart) > > + if (park) > > + i915_gem_park(i915); > > Did not find anything beneath gem park that would need wakeref. In fact, mostly planning to move this to a timer and not use the global idle. Certainly for i915_vma_parked() which is just supposed to be cache of userspace vma and is causing me some pain with i915_vma.kref :) -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c index 6e75702c5671..a33f69610d6f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c @@ -30,23 +30,22 @@ static void idle_work_handler(struct work_struct *work) { struct drm_i915_private *i915 = container_of(work, typeof(*i915), gem.idle_work); - bool restart = true; + bool park; - cancel_delayed_work(&i915->gem.retire_work); + cancel_delayed_work_sync(&i915->gem.retire_work); mutex_lock(&i915->drm.struct_mutex); intel_wakeref_lock(&i915->gt.wakeref); - if (!intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work)) { - i915_gem_park(i915); - restart = false; - } + park = !intel_wakeref_active(&i915->gt.wakeref) && !work_pending(work); intel_wakeref_unlock(&i915->gt.wakeref); - - mutex_unlock(&i915->drm.struct_mutex); - if (restart) + if (park) + i915_gem_park(i915); + else queue_delayed_work(i915->wq, &i915->gem.retire_work, round_jiffies_up_relative(HZ)); + + mutex_unlock(&i915->drm.struct_mutex); } static void retire_work_handler(struct work_struct *work)
While we need to flush the wakeref before parking, we do not need to perform the i915_gem_park() itself underneath the wakeref lock, merely the struct_mutex. If we rearrange the locks, we can avoid the unnecessary tainting. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gem/i915_gem_pm.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)