| Message ID | 20190729234727.28286-7-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Call uC functions from GT ones | expand |
On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote: > For symmetry with intel_gt_resume and to hide more stuff from the top > level under intel_gt. Note that the switch_to_kernel_context_sync has > not been moved dure to the locking and ordering requirements that exist Typo in due. Should we add intel_gt_suspend_early/late, with the former dealing with switch_to_kernel_context_sync? Although that raises an interesting conundrum since it operates on GEM and GT. Best then to leave it unwrapped for now I think. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > at the moment. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_pm.c | 9 +-------- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 13 +++++++++++++ > drivers/gpu/drm/i915/gt/intel_gt_pm.h | 1 + > 3 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > index 25610de3961b..9f72ce8b5a6f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c > @@ -163,18 +163,11 @@ void i915_gem_suspend(struct drm_i915_private *i915) > > mutex_unlock(&i915->drm.struct_mutex); > > - /* > - * 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); > flush_work(&i915->gem.idle_work); > > - cancel_delayed_work_sync(&i915->gt.hangcheck.work); > + intel_gt_suspend(&i915->gt); > > i915_gem_drain_freed_objects(i915); > - > - intel_uc_suspend(&i915->gt.uc); > } > > static struct drm_i915_gem_object *first_mm_object(struct list_head *list) > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index 2250ffbd2f32..9c9efcde994d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -127,6 +127,19 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force) > __intel_engine_reset(engine, false); > } > > +void intel_gt_suspend(struct intel_gt *gt) > +{ > + /* > + * Assert that we successfully flushed all the work and > + * reset the GPU back to its idle, low power state. > + */ > + GEM_BUG_ON(gt->awake); > + > + cancel_delayed_work_sync(>->hangcheck.work); > + > + intel_uc_suspend(>->uc); > +} > + > int intel_gt_resume(struct intel_gt *gt) > { > struct intel_engine_cs *engine; > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h > index 527894fe1345..4f29ac880ca8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h > @@ -22,6 +22,7 @@ void intel_gt_pm_put(struct intel_gt *gt); > void intel_gt_pm_init_early(struct intel_gt *gt); > > void intel_gt_sanitize(struct intel_gt *gt, bool force); > +void intel_gt_suspend(struct intel_gt *gt); > int intel_gt_resume(struct intel_gt *gt); > void intel_gt_runtime_suspend(struct intel_gt *gt); > int intel_gt_runtime_resume(struct intel_gt *gt); >
Quoting Tvrtko Ursulin (2019-07-30 08:33:28) > > On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote: > > For symmetry with intel_gt_resume and to hide more stuff from the top > > level under intel_gt. Note that the switch_to_kernel_context_sync has > > not been moved dure to the locking and ordering requirements that exist > > Typo in due. > > Should we add intel_gt_suspend_early/late, with the former dealing with > switch_to_kernel_context_sync? Although that raises an interesting > conundrum since it operates on GEM and GT. Best then to leave it > unwrapped for now I think. > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Ah, I wouldn't apply this patch yet for exactly that reason: we shouldn't be driving GT suspend from inside GEM, so would wait until we have a few more pieces of the puzzle reviewed^W unravelled. -Chris
On 7/30/19 1:12 AM, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-07-30 08:33:28) >> >> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote: >>> For symmetry with intel_gt_resume and to hide more stuff from the top >>> level under intel_gt. Note that the switch_to_kernel_context_sync has >>> not been moved dure to the locking and ordering requirements that exist >> >> Typo in due. >> >> Should we add intel_gt_suspend_early/late, with the former dealing with >> switch_to_kernel_context_sync? Although that raises an interesting >> conundrum since it operates on GEM and GT. Best then to leave it >> unwrapped for now I think. >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Ah, I wouldn't apply this patch yet for exactly that reason: we > shouldn't be driving GT suspend from inside GEM, so would wait until we > have a few more pieces of the puzzle reviewed^W unravelled. > -Chris > Ok, I'll drop the last 2 patches in the series for now. Daniele
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c index 25610de3961b..9f72ce8b5a6f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c @@ -163,18 +163,11 @@ void i915_gem_suspend(struct drm_i915_private *i915) mutex_unlock(&i915->drm.struct_mutex); - /* - * 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); flush_work(&i915->gem.idle_work); - cancel_delayed_work_sync(&i915->gt.hangcheck.work); + intel_gt_suspend(&i915->gt); i915_gem_drain_freed_objects(i915); - - intel_uc_suspend(&i915->gt.uc); } static struct drm_i915_gem_object *first_mm_object(struct list_head *list) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 2250ffbd2f32..9c9efcde994d 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -127,6 +127,19 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force) __intel_engine_reset(engine, false); } +void intel_gt_suspend(struct intel_gt *gt) +{ + /* + * Assert that we successfully flushed all the work and + * reset the GPU back to its idle, low power state. + */ + GEM_BUG_ON(gt->awake); + + cancel_delayed_work_sync(>->hangcheck.work); + + intel_uc_suspend(>->uc); +} + int intel_gt_resume(struct intel_gt *gt) { struct intel_engine_cs *engine; diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h index 527894fe1345..4f29ac880ca8 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h @@ -22,6 +22,7 @@ void intel_gt_pm_put(struct intel_gt *gt); void intel_gt_pm_init_early(struct intel_gt *gt); void intel_gt_sanitize(struct intel_gt *gt, bool force); +void intel_gt_suspend(struct intel_gt *gt); int intel_gt_resume(struct intel_gt *gt); void intel_gt_runtime_suspend(struct intel_gt *gt); int intel_gt_runtime_resume(struct intel_gt *gt);
For symmetry with intel_gt_resume and to hide more stuff from the top level under intel_gt. Note that the switch_to_kernel_context_sync has not been moved dure to the locking and ordering requirements that exist at the moment. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_pm.c | 9 +-------- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 13 +++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_pm.h | 1 + 3 files changed, 15 insertions(+), 8 deletions(-)