| Message ID | 1509889183-2094-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sagar Arun Kamble (2017-11-05 13:39:40) > Before GT device suspend, i915 should release GuC client doorbells by > stopping doorbell controller snooping and doorbell deallocation through > GuC. They need to be acquired again on resume. This will also resolve > the driver unload issue with GuC, where doorbell deallocation was being > done post GEM suspend. > Release function is called from guc_suspend prior to sending sleep action > during runtime and drm suspend. Acquiral is different in runtime and drm > resume paths as on drm resume we are currently doing full reinit. Release > should be done in synchronization with acquire hence GuC suspend/resume > along with doorbell release/acquire should be under struct_mutex. Upcoming > suspend and resume restructuring for GuC will update these changes. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > drivers/gpu/drm/i915/i915_gem.c | 5 +++-- > drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++---- > drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ > drivers/gpu/drm/i915/intel_guc.c | 2 ++ > 5 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e7e9e06..3df8a7d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -47,6 +47,7 @@ > #include <drm/i915_drm.h> > > #include "i915_drv.h" > +#include "i915_guc_submission.h" > #include "i915_trace.h" > #include "i915_vgpu.h" > #include "intel_drv.h" > @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev) > > intel_guc_resume(dev_priv); > > + i915_guc_clients_acquire_doorbells(&dev_priv->guc); intel_guc_acquire_doorbells(); Though, if we need to specialise between resume and runtime_resume, I suggest adding intel_guc_resume() and intel_guc_runtime_resume() entry points. (We probably should find a better way to distinguish those two paths, system_resume vs runtime_resume?) -Chris
On 11/6/2017 5:43 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2017-11-05 13:39:40) >> Before GT device suspend, i915 should release GuC client doorbells by >> stopping doorbell controller snooping and doorbell deallocation through >> GuC. They need to be acquired again on resume. This will also resolve >> the driver unload issue with GuC, where doorbell deallocation was being >> done post GEM suspend. >> Release function is called from guc_suspend prior to sending sleep action >> during runtime and drm suspend. Acquiral is different in runtime and drm >> resume paths as on drm resume we are currently doing full reinit. Release >> should be done in synchronization with acquire hence GuC suspend/resume >> along with doorbell release/acquire should be under struct_mutex. Upcoming >> suspend and resume restructuring for GuC will update these changes. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 3 +++ >> drivers/gpu/drm/i915/i915_gem.c | 5 +++-- >> drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++---- >> drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ >> drivers/gpu/drm/i915/intel_guc.c | 2 ++ >> 5 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index e7e9e06..3df8a7d 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -47,6 +47,7 @@ >> #include <drm/i915_drm.h> >> >> #include "i915_drv.h" >> +#include "i915_guc_submission.h" >> #include "i915_trace.h" >> #include "i915_vgpu.h" >> #include "intel_drv.h" >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev) >> >> intel_guc_resume(dev_priv); >> >> + i915_guc_clients_acquire_doorbells(&dev_priv->guc); > intel_guc_acquire_doorbells(); Prefixed "i915_guc_clients" since this modifies submission state (clients/doorbells). Should have kept dev_priv as parameter. what should be the correct option here: intel_guc*(guc) or i915_guc*(dev_priv) > > Though, if we need to specialise between resume and runtime_resume, I > suggest adding intel_guc_resume() and intel_guc_runtime_resume() entry > points. (We probably should find a better way to distinguish those two > paths, system_resume vs runtime_resume?) Yes. these will be added in the upcoming GuC suspend/resume restructuring series. Functionality along runtime/system suspend path will be same for GuC. Will defer in the resume path since we are doing full gem reinit on resume from system suspend. > -Chris
Quoting Sagar Arun Kamble (2017-11-07 06:05:01) > > > On 11/6/2017 5:43 PM, Chris Wilson wrote: > > Quoting Sagar Arun Kamble (2017-11-05 13:39:40) > >> Before GT device suspend, i915 should release GuC client doorbells by > >> stopping doorbell controller snooping and doorbell deallocation through > >> GuC. They need to be acquired again on resume. This will also resolve > >> the driver unload issue with GuC, where doorbell deallocation was being > >> done post GEM suspend. > >> Release function is called from guc_suspend prior to sending sleep action > >> during runtime and drm suspend. Acquiral is different in runtime and drm > >> resume paths as on drm resume we are currently doing full reinit. Release > >> should be done in synchronization with acquire hence GuC suspend/resume > >> along with doorbell release/acquire should be under struct_mutex. Upcoming > >> suspend and resume restructuring for GuC will update these changes. > >> > >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Michał Winiarski <michal.winiarski@intel.com> > >> Cc: Michel Thierry <michel.thierry@intel.com> > >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.c | 3 +++ > >> drivers/gpu/drm/i915/i915_gem.c | 5 +++-- > >> drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++---- > >> drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ > >> drivers/gpu/drm/i915/intel_guc.c | 2 ++ > >> 5 files changed, 26 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index e7e9e06..3df8a7d 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -47,6 +47,7 @@ > >> #include <drm/i915_drm.h> > >> > >> #include "i915_drv.h" > >> +#include "i915_guc_submission.h" > >> #include "i915_trace.h" > >> #include "i915_vgpu.h" > >> #include "intel_drv.h" > >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev) > >> > >> intel_guc_resume(dev_priv); > >> > >> + i915_guc_clients_acquire_doorbells(&dev_priv->guc); > > intel_guc_acquire_doorbells(); > Prefixed "i915_guc_clients" since this modifies submission state > (clients/doorbells). Should have kept dev_priv as parameter. > what should be the correct option here: intel_guc*(guc) or > i915_guc*(dev_priv) GuC submission is not i915. It is not part of the user facing api. Operate on intel_guc as you were. -Chris
On Tue, 07 Nov 2017 10:21:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Sagar Arun Kamble (2017-11-07 06:05:01) >> >> >> On 11/6/2017 5:43 PM, Chris Wilson wrote: >> > Quoting Sagar Arun Kamble (2017-11-05 13:39:40) >> >> Before GT device suspend, i915 should release GuC client doorbells by >> >> stopping doorbell controller snooping and doorbell deallocation >> through >> >> GuC. They need to be acquired again on resume. This will also resolve >> >> the driver unload issue with GuC, where doorbell deallocation was >> being >> >> done post GEM suspend. >> >> Release function is called from guc_suspend prior to sending sleep >> action >> >> during runtime and drm suspend. Acquiral is different in runtime and >> drm >> >> resume paths as on drm resume we are currently doing full reinit. >> Release >> >> should be done in synchronization with acquire hence GuC >> suspend/resume >> >> along with doorbell release/acquire should be under struct_mutex. >> Upcoming >> >> suspend and resume restructuring for GuC will update these changes. >> >> >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> >> Cc: Michel Thierry <michel.thierry@intel.com> >> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> >> --- >> >> drivers/gpu/drm/i915/i915_drv.c | 3 +++ >> >> drivers/gpu/drm/i915/i915_gem.c | 5 +++-- >> >> drivers/gpu/drm/i915/i915_guc_submission.c | 20 >> ++++++++++++++++---- >> >> drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ >> >> drivers/gpu/drm/i915/intel_guc.c | 2 ++ >> >> 5 files changed, 26 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> >> index e7e9e06..3df8a7d 100644 >> >> --- a/drivers/gpu/drm/i915/i915_drv.c >> >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> >> @@ -47,6 +47,7 @@ >> >> #include <drm/i915_drm.h> >> >> >> >> #include "i915_drv.h" >> >> +#include "i915_guc_submission.h" >> >> #include "i915_trace.h" >> >> #include "i915_vgpu.h" >> >> #include "intel_drv.h" >> >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device >> *kdev) >> >> >> >> intel_guc_resume(dev_priv); >> >> >> >> + i915_guc_clients_acquire_doorbells(&dev_priv->guc); >> > intel_guc_acquire_doorbells(); >> Prefixed "i915_guc_clients" since this modifies submission state >> (clients/doorbells). Should have kept dev_priv as parameter. >> what should be the correct option here: intel_guc*(guc) or >> i915_guc*(dev_priv) > > GuC submission is not i915. It is not part of the user facing api. > Operate on intel_guc as you were. So if GuC submission is not i915 than maybe to avoid confusion we should rename i915_guc_submission.c to intel_guc_submission.c ? Michal
On 11/7/2017 3:57 PM, Michal Wajdeczko wrote: > On Tue, 07 Nov 2017 10:21:17 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > >> Quoting Sagar Arun Kamble (2017-11-07 06:05:01) >>> >>> >>> On 11/6/2017 5:43 PM, Chris Wilson wrote: >>> > Quoting Sagar Arun Kamble (2017-11-05 13:39:40) >>> >> Before GT device suspend, i915 should release GuC client >>> doorbells by >>> >> stopping doorbell controller snooping and doorbell deallocation >>> through >>> >> GuC. They need to be acquired again on resume. This will also >>> resolve >>> >> the driver unload issue with GuC, where doorbell deallocation was >>> being >>> >> done post GEM suspend. >>> >> Release function is called from guc_suspend prior to sending >>> sleep action >>> >> during runtime and drm suspend. Acquiral is different in runtime >>> and drm >>> >> resume paths as on drm resume we are currently doing full reinit. >>> Release >>> >> should be done in synchronization with acquire hence GuC >>> suspend/resume >>> >> along with doorbell release/acquire should be under struct_mutex. >>> Upcoming >>> >> suspend and resume restructuring for GuC will update these changes. >>> >> >>> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >>> >> Cc: Michel Thierry <michel.thierry@intel.com> >>> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >>> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> >> --- >>> >> drivers/gpu/drm/i915/i915_drv.c | 3 +++ >>> >> drivers/gpu/drm/i915/i915_gem.c | 5 +++-- >>> >> drivers/gpu/drm/i915/i915_guc_submission.c | 20 >>> ++++++++++++++++---- >>> >> drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ >>> >> drivers/gpu/drm/i915/intel_guc.c | 2 ++ >>> >> 5 files changed, 26 insertions(+), 6 deletions(-) >>> >> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >>> b/drivers/gpu/drm/i915/i915_drv.c >>> >> index e7e9e06..3df8a7d 100644 >>> >> --- a/drivers/gpu/drm/i915/i915_drv.c >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.c >>> >> @@ -47,6 +47,7 @@ >>> >> #include <drm/i915_drm.h> >>> >> >>> >> #include "i915_drv.h" >>> >> +#include "i915_guc_submission.h" >>> >> #include "i915_trace.h" >>> >> #include "i915_vgpu.h" >>> >> #include "intel_drv.h" >>> >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct >>> device *kdev) >>> >> >>> >> intel_guc_resume(dev_priv); >>> >> >>> >> + i915_guc_clients_acquire_doorbells(&dev_priv->guc); >>> > intel_guc_acquire_doorbells(); >>> Prefixed "i915_guc_clients" since this modifies submission state >>> (clients/doorbells). Should have kept dev_priv as parameter. >>> what should be the correct option here: intel_guc*(guc) or >>> i915_guc*(dev_priv) >> >> GuC submission is not i915. It is not part of the user facing api. >> Operate on intel_guc as you were. > > So if GuC submission is not i915 than maybe to avoid confusion we should > rename i915_guc_submission.c to intel_guc_submission.c ? Yes. Will need to update functions that are i915_guc* as well. (spotted some in guc_log.c as well.) > > Michal
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e7e9e06..3df8a7d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -47,6 +47,7 @@ #include <drm/i915_drm.h> #include "i915_drv.h" +#include "i915_guc_submission.h" #include "i915_trace.h" #include "i915_vgpu.h" #include "intel_drv.h" @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev) intel_guc_resume(dev_priv); + i915_guc_clients_acquire_doorbells(&dev_priv->guc); + if (IS_GEN9_LP(dev_priv)) { bxt_disable_dc9(dev_priv); bxt_display_core_init(dev_priv, true); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6a71805..cbce714 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -29,13 +29,14 @@ #include <drm/drm_vma_manager.h> #include <drm/i915_drm.h> #include "i915_drv.h" +#include "i915_gemfs.h" #include "i915_gem_clflush.h" -#include "i915_vgpu.h" +#include "i915_guc_submission.h" #include "i915_trace.h" +#include "i915_vgpu.h" #include "intel_drv.h" #include "intel_frontbuffer.h" #include "intel_mocs.h" -#include "i915_gemfs.h" #include <linux/dma-fence-array.h> #include <linux/kthread.h> #include <linux/reservation.h> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index b6486dc..b989edf 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1047,10 +1047,6 @@ static void guc_client_free(struct i915_guc_client *client) * Be sure to drop any locks */ - /* FIXME: in many cases, by the time we get here the GuC has been - * reset, so we cannot destroy the doorbell properly. Ignore the - * error message for now */ - destroy_doorbell(client, true); guc_stage_desc_fini(client->guc, client); i915_gem_object_unpin_map(client->vma->obj); i915_vma_unpin_and_release(&client->vma); @@ -1102,6 +1098,21 @@ static void guc_clients_destroy(struct intel_guc *guc) guc_client_free(client); } +void i915_guc_clients_acquire_doorbells(struct intel_guc *guc) +{ + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) + return; + + create_doorbell(guc->execbuf_client); + create_doorbell(guc->preempt_client); +} + +void i915_guc_clients_release_doorbells(struct intel_guc *guc) +{ + destroy_doorbell(guc->preempt_client, true); + destroy_doorbell(guc->execbuf_client, true); +} + static void guc_policy_init(struct guc_policy *policy) { policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US; @@ -1423,6 +1434,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) } else { guc_reset_wq(guc->execbuf_client); guc_reset_wq(guc->preempt_client); + i915_guc_clients_acquire_doorbells(guc); } err = intel_guc_sample_forcewake(guc); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.h b/drivers/gpu/drm/i915/i915_guc_submission.h index cb4353b..1d5cf22 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.h +++ b/drivers/gpu/drm/i915/i915_guc_submission.h @@ -72,6 +72,8 @@ struct i915_guc_client { u64 submissions[I915_NUM_ENGINES]; }; +void i915_guc_clients_acquire_doorbells(struct intel_guc *guc); +void i915_guc_clients_release_doorbells(struct intel_guc *guc); int i915_guc_submission_init(struct drm_i915_private *dev_priv); int i915_guc_submission_enable(struct drm_i915_private *dev_priv); void i915_guc_submission_disable(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 9678630..b6eb5ac 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -274,6 +274,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) return 0; + i915_guc_clients_release_doorbells(&dev_priv->guc); + gen9_disable_guc_interrupts(dev_priv); data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
Before GT device suspend, i915 should release GuC client doorbells by stopping doorbell controller snooping and doorbell deallocation through GuC. They need to be acquired again on resume. This will also resolve the driver unload issue with GuC, where doorbell deallocation was being done post GEM suspend. Release function is called from guc_suspend prior to sending sleep action during runtime and drm suspend. Acquiral is different in runtime and drm resume paths as on drm resume we are currently doing full reinit. Release should be done in synchronization with acquire hence GuC suspend/resume along with doorbell release/acquire should be under struct_mutex. Upcoming suspend and resume restructuring for GuC will update these changes. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 3 +++ drivers/gpu/drm/i915/i915_gem.c | 5 +++-- drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++---- drivers/gpu/drm/i915/i915_guc_submission.h | 2 ++ drivers/gpu/drm/i915/intel_guc.c | 2 ++ 5 files changed, 26 insertions(+), 6 deletions(-)