| Message ID | 20190617180935.505-5-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display uncore prep patches | expand |
On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote: > We always call some of the setup/cleanup functions for forcewake, even > if the feature is not actually available. Skipping these operations if > forcewake is not available saves us some operations on older gens and > prepares us for having a forcewake-less display uncore. > The suspend/resume functions have also been renamed to clearly indicate > that they only operate on forcewake status. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 15 +-- > drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- > drivers/gpu/drm/i915/intel_uncore.h | 8 +- > 3 files changed, 101 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index d113296cbe34..95b36fe17f99 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > > intel_device_info_init_mmio(dev_priv); > > - intel_uncore_prune_mmio_domains(&dev_priv->uncore); > + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); > > intel_uc_init_mmio(dev_priv); > > @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > i915_gem_suspend_late(dev_priv); > > - intel_uncore_suspend(&dev_priv->uncore); > + intel_uncore_forcewake_suspend(&dev_priv->uncore); > > intel_power_domains_suspend(dev_priv, > get_suspend_mode(dev_priv, hibernation)); > @@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > DRM_ERROR("Resume prepare failed: %d, continuing anyway\n", > ret); > > - intel_uncore_resume_early(&dev_priv->uncore); > + if (intel_uncore_unclaimed_mmio(&dev_priv->uncore)) > + DRM_DEBUG("unclaimed mmio detected on resume, clearing\n"); > + Why does this bit needs to be pulled up to this level? My first line of thinking is that we should aim to keep the component specific steps down, if possible. > + intel_uncore_forcewake_resume_early(&dev_priv->uncore); > > i915_check_and_clear_faults(dev_priv); > > @@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device *kdev) > > intel_runtime_pm_disable_interrupts(dev_priv); > > - intel_uncore_suspend(&dev_priv->uncore); > + intel_uncore_forcewake_suspend(&dev_priv->uncore); > > ret = 0; > if (INTEL_GEN(dev_priv) >= 11) { > @@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device *kdev) > > if (ret) { > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > - intel_uncore_runtime_resume(&dev_priv->uncore); > + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); > > intel_runtime_pm_enable_interrupts(dev_priv); > > @@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device *kdev) > ret = vlv_resume_prepare(dev_priv, true); > } > > - intel_uncore_runtime_resume(&dev_priv->uncore); > + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); > > intel_runtime_pm_enable_interrupts(dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 88a69bf713c9..c0f5567ee096 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore) > return ret; > } > > -static void __intel_uncore_early_sanitize(struct intel_uncore *uncore, > +static void forcewake_early_sanitize(struct intel_uncore *uncore, > unsigned int restore_forcewake) > { > - /* clear out unclaimed reg detection bit */ > - if (check_for_unclaimed_mmio(uncore)) > - DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); > + if (!intel_uncore_has_forcewake(uncore)) > + return; > > /* WaDisableShadowRegForCpd:chv */ > if (IS_CHERRYVIEW(uncore->i915)) { > @@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore, > iosf_mbi_punit_release(); > } > > -void intel_uncore_suspend(struct intel_uncore *uncore) > +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore) > { > + if (!intel_uncore_has_forcewake(uncore)) > + return; > + > iosf_mbi_punit_acquire(); > iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > &uncore->pmic_bus_access_nb); > @@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore *uncore) > iosf_mbi_punit_release(); > } > > -void intel_uncore_resume_early(struct intel_uncore *uncore) > +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore) > { > unsigned int restore_forcewake; > > + if (!intel_uncore_has_forcewake(uncore)) > + return; > + > restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved); > - __intel_uncore_early_sanitize(uncore, restore_forcewake); > + forcewake_early_sanitize(uncore, restore_forcewake); This call already handles !has_forcewake, so function handles it twice in source. Is this what you intended? Maybe just add double-underscore version for early sanitize without the check but GEM_BUG_ON? > > iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); > } > > -void intel_uncore_runtime_resume(struct intel_uncore *uncore) > +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore) > { > + if (!intel_uncore_has_forcewake(uncore)) > + return; > + > iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); > } > > @@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore) > { > struct drm_i915_private *i915 = uncore->i915; > > - if (!intel_uncore_has_forcewake(uncore)) > - return; > + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); > > if (INTEL_GEN(i915) >= 11) { > int i; > @@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct drm_i915_private *i915, > uncore->rpm = &i915->runtime_pm; > } > > -int intel_uncore_init_mmio(struct intel_uncore *uncore) > +static void uncore_raw_init(struct intel_uncore *uncore) > { > - struct drm_i915_private *i915 = uncore->i915; > - int ret; > + GEM_BUG_ON(intel_uncore_has_forcewake(uncore)); > > - ret = uncore_mmio_setup(uncore); > - if (ret) > - return ret; > + if (IS_GEN(uncore->i915, 5)) { > + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); > + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); > + } else { > + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); > + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); > + } > +} > > - i915_check_vgpu(i915); > +static void uncore_forcewake_init(struct intel_uncore *uncore) > +{ > + struct drm_i915_private *i915 = uncore->i915; > > - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) > - uncore->flags |= UNCORE_HAS_FORCEWAKE; > + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); > > intel_uncore_fw_domains_init(uncore); > - __intel_uncore_early_sanitize(uncore, 0); > + forcewake_early_sanitize(uncore, 0); > > - uncore->unclaimed_mmio_check = 1; > - uncore->pmic_bus_access_nb.notifier_call = > - i915_pmic_bus_access_notifier; > - > - if (!intel_uncore_has_forcewake(uncore)) { > - if (IS_GEN(i915, 5)) { > - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); > - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); > - } else { > - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); > - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); > - } > - } else if (IS_GEN_RANGE(i915, 6, 7)) { > + if (IS_GEN_RANGE(i915, 6, 7)) { > ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6); > > if (IS_VALLEYVIEW(i915)) { > @@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges); > ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable); > ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable); > - > } else { > ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8); > ASSIGN_READ_MMIO_VFUNCS(uncore, gen6); > @@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable); > } > > + uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier; > + iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); > +} > + > +int intel_uncore_init_mmio(struct intel_uncore *uncore) > +{ > + struct drm_i915_private *i915 = uncore->i915; > + int ret; > + > + ret = uncore_mmio_setup(uncore); > + if (ret) > + return ret; > + > + i915_check_vgpu(i915); > + > + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) > + uncore->flags |= UNCORE_HAS_FORCEWAKE; > + > + uncore->unclaimed_mmio_check = 1; > + > + if (!intel_uncore_has_forcewake(uncore)) > + uncore_raw_init(uncore); Is any of the remaining code in this function relevant after this branch has been taken? If not this could be changed to: if (!intel_uncore_has_forcewake(uncore)) { uncore_raw_init(uncore); return; } uncore_forcewake_init(uncore); ... Hm, also is "unclaimed_mmio_check = 1;" above possible/relevant where no forcwake? Doesn't look like it. Unless vgpu? > + else > + uncore_forcewake_init(uncore); > + > /* make sure fw funcs are set if and only if we have fw*/ > GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_get); > GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_put); > @@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > if (IS_GEN_RANGE(i915, 6, 7)) > uncore->flags |= UNCORE_HAS_FIFO; > > - iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); > + /* clear out unclaimed reg detection bit */ > + if (check_for_unclaimed_mmio(uncore)) > + DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); > > return 0; > } > @@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > * the forcewake domains. Prune them, to make sure they only reference existing > * engines. > */ > -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore) > +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore) > { > struct drm_i915_private *i915 = uncore->i915; > + enum forcewake_domains fw_domains = uncore->fw_domains; > + enum forcewake_domain_id domain_id; > + int i; > > - if (INTEL_GEN(i915) >= 11) { > - enum forcewake_domains fw_domains = uncore->fw_domains; > - enum forcewake_domain_id domain_id; > - int i; > + if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11) > + return; > > - for (i = 0; i < I915_MAX_VCS; i++) { > - domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; > + for (i = 0; i < I915_MAX_VCS; i++) { > + domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; > > - if (HAS_ENGINE(i915, _VCS(i))) > - continue; > + if (HAS_ENGINE(i915, _VCS(i))) > + continue; > > - if (fw_domains & BIT(domain_id)) > - fw_domain_fini(uncore, domain_id); > - } > + if (fw_domains & BIT(domain_id)) > + fw_domain_fini(uncore, domain_id); > + } > > - for (i = 0; i < I915_MAX_VECS; i++) { > - domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; > + for (i = 0; i < I915_MAX_VECS; i++) { > + domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; > > - if (HAS_ENGINE(i915, _VECS(i))) > - continue; > + if (HAS_ENGINE(i915, _VECS(i))) > + continue; > > - if (fw_domains & BIT(domain_id)) > - fw_domain_fini(uncore, domain_id); > - } > + if (fw_domains & BIT(domain_id)) > + fw_domain_fini(uncore, domain_id); > } > } > > void intel_uncore_fini_mmio(struct intel_uncore *uncore) > { > - iosf_mbi_punit_acquire(); > - iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > - &uncore->pmic_bus_access_nb); > - intel_uncore_forcewake_reset(uncore); > - iosf_mbi_punit_release(); > + if (intel_uncore_has_forcewake(uncore)) { To avoid hyphotetical obnoxious diffs in the future, like the one for intel_uncore_prune_mmio_domains above in this patch, maybe invert this to early return straight away. > + iosf_mbi_punit_acquire(); > + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + &uncore->pmic_bus_access_nb); > + intel_uncore_forcewake_reset(uncore); > + iosf_mbi_punit_release(); > + } > + > uncore_mmio_cleanup(uncore); > } > > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h > index 912616188ff5..879252735bba 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.h > +++ b/drivers/gpu/drm/i915/intel_uncore.h > @@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore) > void intel_uncore_init_early(struct drm_i915_private *i915, > struct intel_uncore *uncore); > int intel_uncore_init_mmio(struct intel_uncore *uncore); > -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore); > +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore); > bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore); > bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore); > void intel_uncore_fini_mmio(struct intel_uncore *uncore); > -void intel_uncore_suspend(struct intel_uncore *uncore); > -void intel_uncore_resume_early(struct intel_uncore *uncore); > -void intel_uncore_runtime_resume(struct intel_uncore *uncore); > +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore); > +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore); > +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore); > > void assert_forcewakes_inactive(struct intel_uncore *uncore); > void assert_forcewakes_active(struct intel_uncore *uncore, > Regards, Tvrtko
Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33) > We always call some of the setup/cleanup functions for forcewake, even > if the feature is not actually available. Skipping these operations if > forcewake is not available saves us some operations on older gens and > prepares us for having a forcewake-less display uncore. > The suspend/resume functions have also been renamed to clearly indicate > that they only operate on forcewake status. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 15 +-- > drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- > drivers/gpu/drm/i915/intel_uncore.h | 8 +- > 3 files changed, 101 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index d113296cbe34..95b36fe17f99 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > > intel_device_info_init_mmio(dev_priv); > > - intel_uncore_prune_mmio_domains(&dev_priv->uncore); > + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); The _prune_ was the exceptional case... > intel_uc_init_mmio(dev_priv); > > @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > i915_gem_suspend_late(dev_priv); > > - intel_uncore_suspend(&dev_priv->uncore); > + intel_uncore_forcewake_suspend(&dev_priv->uncore); But are you sure you want to delegate all the fw control to i915_drv.c, and not keep this as a call into intel_uncore_suspend() ? It is meant to be telling the uncore functionality that it is time to suspend, and it has to work out what to save. I am not sold on this yet. (One day this will just be a bunch of async tasks plugged into signals with ordering determined by their self-declared dependency tree. One day.) So sell me on why we want an uncore_forcewake object, as currently you are proposing a intel_uncore_suspend_forcewake(). -Chris
On 6/18/19 3:22 AM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33) >> We always call some of the setup/cleanup functions for forcewake, even >> if the feature is not actually available. Skipping these operations if >> forcewake is not available saves us some operations on older gens and >> prepares us for having a forcewake-less display uncore. >> The suspend/resume functions have also been renamed to clearly indicate >> that they only operate on forcewake status. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 15 +-- >> drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- >> drivers/gpu/drm/i915/intel_uncore.h | 8 +- >> 3 files changed, 101 insertions(+), 69 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index d113296cbe34..95b36fe17f99 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) >> >> intel_device_info_init_mmio(dev_priv); >> >> - intel_uncore_prune_mmio_domains(&dev_priv->uncore); >> + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); > > The _prune_ was the exceptional case... > >> intel_uc_init_mmio(dev_priv); >> >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) >> >> i915_gem_suspend_late(dev_priv); >> >> - intel_uncore_suspend(&dev_priv->uncore); >> + intel_uncore_forcewake_suspend(&dev_priv->uncore); > > But are you sure you want to delegate all the fw control to i915_drv.c, > and not keep this as a call into intel_uncore_suspend() ? It is meant to > be telling the uncore functionality that it is time to suspend, and it > has to work out what to save. > > I am not sold on this yet. (One day this will just be a bunch of async > tasks plugged into signals with ordering determined by their > self-declared dependency tree. One day.) > > So sell me on why we want an uncore_forcewake object, as currently you > are proposing a intel_uncore_suspend_forcewake(). > -Chris > My aim was to make it clear that this call will not be required for display_uncore since there is nothing to do on suspend/resume if there is no forcewake (and you're right, intel_uncore_suspend_forcewake is a better naming). Ultimately I'd like to transition the GT uncore inside intel_gt and call this inside intel_gt_suspend(). However, I don't mind keeping the current naming and calling it for display uncore as well to be more generic, there are checks everywhere so the overhead should me minimal. What's your preference? Daniele
Quoting Daniele Ceraolo Spurio (2019-06-18 19:40:40) > > > On 6/18/19 3:22 AM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33) > >> We always call some of the setup/cleanup functions for forcewake, even > >> if the feature is not actually available. Skipping these operations if > >> forcewake is not available saves us some operations on older gens and > >> prepares us for having a forcewake-less display uncore. > >> The suspend/resume functions have also been renamed to clearly indicate > >> that they only operate on forcewake status. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.c | 15 +-- > >> drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- > >> drivers/gpu/drm/i915/intel_uncore.h | 8 +- > >> 3 files changed, 101 insertions(+), 69 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index d113296cbe34..95b36fe17f99 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > >> > >> intel_device_info_init_mmio(dev_priv); > >> > >> - intel_uncore_prune_mmio_domains(&dev_priv->uncore); > >> + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); > > > > The _prune_ was the exceptional case... > > > >> intel_uc_init_mmio(dev_priv); > >> > >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > >> > >> i915_gem_suspend_late(dev_priv); > >> > >> - intel_uncore_suspend(&dev_priv->uncore); > >> + intel_uncore_forcewake_suspend(&dev_priv->uncore); > > > > But are you sure you want to delegate all the fw control to i915_drv.c, > > and not keep this as a call into intel_uncore_suspend() ? It is meant to > > be telling the uncore functionality that it is time to suspend, and it > > has to work out what to save. > > > > I am not sold on this yet. (One day this will just be a bunch of async > > tasks plugged into signals with ordering determined by their > > self-declared dependency tree. One day.) > > > > So sell me on why we want an uncore_forcewake object, as currently you > > are proposing a intel_uncore_suspend_forcewake(). > > -Chris > > > > My aim was to make it clear that this call will not be required for > display_uncore since there is nothing to do on suspend/resume if there > is no forcewake (and you're right, intel_uncore_suspend_forcewake is a > better naming). Ultimately I'd like to transition the GT uncore inside > intel_gt and call this inside intel_gt_suspend(). However, I don't mind > keeping the current naming and calling it for display uncore as well to > be more generic, there are checks everywhere so the overhead should me > minimal. What's your preference? I think, for the time being a sketch like i915_drm_suspend: intel_uncore_suspend(i915->gt.uncore) intel_uncore_suspend(i915->display.uncore) is preferable. Rather than pulling the knowlegde that we need only suspend the gt.uncore because it has forcewake into the high level i915_drm_suspend. A couple of ifs or a vfunc go a long way to keeping as simple as it can be (and no simpler). At the i915_drm_suspend level it is hard enough to manage a list of components and the correct order in which to call them :) -Chris
On 6/18/19 2:00 AM, Tvrtko Ursulin wrote: > > On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote: >> We always call some of the setup/cleanup functions for forcewake, even >> if the feature is not actually available. Skipping these operations if >> forcewake is not available saves us some operations on older gens and >> prepares us for having a forcewake-less display uncore. >> The suspend/resume functions have also been renamed to clearly indicate >> that they only operate on forcewake status. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 15 +-- >> drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- >> drivers/gpu/drm/i915/intel_uncore.h | 8 +- >> 3 files changed, 101 insertions(+), 69 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index d113296cbe34..95b36fe17f99 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct >> drm_i915_private *dev_priv) >> intel_device_info_init_mmio(dev_priv); >> - intel_uncore_prune_mmio_domains(&dev_priv->uncore); >> + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); >> intel_uc_init_mmio(dev_priv); >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct >> drm_device *dev, bool hibernation) >> i915_gem_suspend_late(dev_priv); >> - intel_uncore_suspend(&dev_priv->uncore); >> + intel_uncore_forcewake_suspend(&dev_priv->uncore); >> intel_power_domains_suspend(dev_priv, >> get_suspend_mode(dev_priv, hibernation)); >> @@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct >> drm_device *dev) >> DRM_ERROR("Resume prepare failed: %d, continuing anyway\n", >> ret); >> - intel_uncore_resume_early(&dev_priv->uncore); >> + if (intel_uncore_unclaimed_mmio(&dev_priv->uncore)) >> + DRM_DEBUG("unclaimed mmio detected on resume, clearing\n"); >> + > > Why does this bit needs to be pulled up to this level? My first line of > thinking is that we should aim to keep the component specific steps > down, if possible. The idea was to split this out to have the function below act on forcewake only. Chris didn't like it either, so I'm going to roll back these changes. > >> + intel_uncore_forcewake_resume_early(&dev_priv->uncore); >> i915_check_and_clear_faults(dev_priv); >> @@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device >> *kdev) >> intel_runtime_pm_disable_interrupts(dev_priv); >> - intel_uncore_suspend(&dev_priv->uncore); >> + intel_uncore_forcewake_suspend(&dev_priv->uncore); >> ret = 0; >> if (INTEL_GEN(dev_priv) >= 11) { >> @@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device >> *kdev) >> if (ret) { >> DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); >> - intel_uncore_runtime_resume(&dev_priv->uncore); >> + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); >> intel_runtime_pm_enable_interrupts(dev_priv); >> @@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device >> *kdev) >> ret = vlv_resume_prepare(dev_priv, true); >> } >> - intel_uncore_runtime_resume(&dev_priv->uncore); >> + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); >> intel_runtime_pm_enable_interrupts(dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index 88a69bf713c9..c0f5567ee096 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore >> *uncore) >> return ret; >> } >> -static void __intel_uncore_early_sanitize(struct intel_uncore *uncore, >> +static void forcewake_early_sanitize(struct intel_uncore *uncore, >> unsigned int restore_forcewake) >> { >> - /* clear out unclaimed reg detection bit */ >> - if (check_for_unclaimed_mmio(uncore)) >> - DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); >> + if (!intel_uncore_has_forcewake(uncore)) >> + return; >> /* WaDisableShadowRegForCpd:chv */ >> if (IS_CHERRYVIEW(uncore->i915)) { >> @@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct >> intel_uncore *uncore, >> iosf_mbi_punit_release(); >> } >> -void intel_uncore_suspend(struct intel_uncore *uncore) >> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore) >> { >> + if (!intel_uncore_has_forcewake(uncore)) >> + return; >> + >> iosf_mbi_punit_acquire(); >> iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >> &uncore->pmic_bus_access_nb); >> @@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore >> *uncore) >> iosf_mbi_punit_release(); >> } >> -void intel_uncore_resume_early(struct intel_uncore *uncore) >> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore) >> { >> unsigned int restore_forcewake; >> + if (!intel_uncore_has_forcewake(uncore)) >> + return; >> + >> restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved); >> - __intel_uncore_early_sanitize(uncore, restore_forcewake); >> + forcewake_early_sanitize(uncore, restore_forcewake); > > This call already handles !has_forcewake, so function handles it twice > in source. Is this what you intended? Maybe just add double-underscore > version for early sanitize without the check but GEM_BUG_ON? will do. > >> >> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); >> } >> -void intel_uncore_runtime_resume(struct intel_uncore *uncore) >> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore) >> { >> + if (!intel_uncore_has_forcewake(uncore)) >> + return; >> + >> >> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); >> } >> @@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct >> intel_uncore *uncore) >> { >> struct drm_i915_private *i915 = uncore->i915; >> - if (!intel_uncore_has_forcewake(uncore)) >> - return; >> + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); >> if (INTEL_GEN(i915) >= 11) { >> int i; >> @@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct >> drm_i915_private *i915, >> uncore->rpm = &i915->runtime_pm; >> } >> -int intel_uncore_init_mmio(struct intel_uncore *uncore) >> +static void uncore_raw_init(struct intel_uncore *uncore) >> { >> - struct drm_i915_private *i915 = uncore->i915; >> - int ret; >> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore)); >> - ret = uncore_mmio_setup(uncore); >> - if (ret) >> - return ret; >> + if (IS_GEN(uncore->i915, 5)) { >> + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); >> + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); >> + } else { >> + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); >> + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); >> + } >> +} >> - i915_check_vgpu(i915); >> +static void uncore_forcewake_init(struct intel_uncore *uncore) >> +{ >> + struct drm_i915_private *i915 = uncore->i915; >> - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) >> - uncore->flags |= UNCORE_HAS_FORCEWAKE; >> + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); >> intel_uncore_fw_domains_init(uncore); >> - __intel_uncore_early_sanitize(uncore, 0); >> + forcewake_early_sanitize(uncore, 0); >> - uncore->unclaimed_mmio_check = 1; >> - uncore->pmic_bus_access_nb.notifier_call = >> - i915_pmic_bus_access_notifier; >> - >> - if (!intel_uncore_has_forcewake(uncore)) { >> - if (IS_GEN(i915, 5)) { >> - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); >> - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); >> - } else { >> - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); >> - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); >> - } >> - } else if (IS_GEN_RANGE(i915, 6, 7)) { >> + if (IS_GEN_RANGE(i915, 6, 7)) { >> ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6); >> if (IS_VALLEYVIEW(i915)) { >> @@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore >> *uncore) >> ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges); >> ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable); >> ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable); >> - >> } else { >> ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8); >> ASSIGN_READ_MMIO_VFUNCS(uncore, gen6); >> @@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore >> *uncore) >> ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable); >> } >> + uncore->pmic_bus_access_nb.notifier_call = >> i915_pmic_bus_access_notifier; >> + >> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); >> +} >> + >> +int intel_uncore_init_mmio(struct intel_uncore *uncore) >> +{ >> + struct drm_i915_private *i915 = uncore->i915; >> + int ret; >> + >> + ret = uncore_mmio_setup(uncore); >> + if (ret) >> + return ret; >> + >> + i915_check_vgpu(i915); >> + >> + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) >> + uncore->flags |= UNCORE_HAS_FORCEWAKE; >> + >> + uncore->unclaimed_mmio_check = 1; >> + >> + if (!intel_uncore_has_forcewake(uncore)) >> + uncore_raw_init(uncore); > > Is any of the remaining code in this function relevant after this branch > has been taken? If not this could be changed to: > > if (!intel_uncore_has_forcewake(uncore)) { > uncore_raw_init(uncore); > return; > } > > uncore_forcewake_init(uncore); > ... > > Hm, also is "unclaimed_mmio_check = 1;" above possible/relevant where no > forcwake? Doesn't look like it. Unless vgpu? > The unclaimed mmio stuff (including the flags) below this point is not tied to forcewake from a functional POV, but it indeed true that all the platforms with the mmio debug do have forcewake. I'd still prefer to avoid connecting the 2 features under the same check just because they happen to be on the same platforms. Also as you mentioned GVT traps the FPGA_DBG register used by the unclaimed mmio logic, so they do have the capability when forcewake is disabled. >> + else >> + uncore_forcewake_init(uncore); >> + >> /* make sure fw funcs are set if and only if we have fw*/ >> GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != >> !!uncore->funcs.force_wake_get); >> GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != >> !!uncore->funcs.force_wake_put); >> @@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore >> *uncore) >> if (IS_GEN_RANGE(i915, 6, 7)) >> uncore->flags |= UNCORE_HAS_FIFO; >> - >> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); >> + /* clear out unclaimed reg detection bit */ >> + if (check_for_unclaimed_mmio(uncore)) >> + DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); >> return 0; >> } >> @@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore >> *uncore) >> * the forcewake domains. Prune them, to make sure they only >> reference existing >> * engines. >> */ >> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore) >> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore) >> { >> struct drm_i915_private *i915 = uncore->i915; >> + enum forcewake_domains fw_domains = uncore->fw_domains; >> + enum forcewake_domain_id domain_id; >> + int i; >> - if (INTEL_GEN(i915) >= 11) { >> - enum forcewake_domains fw_domains = uncore->fw_domains; >> - enum forcewake_domain_id domain_id; >> - int i; >> + if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11) >> + return; >> - for (i = 0; i < I915_MAX_VCS; i++) { >> - domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; >> + for (i = 0; i < I915_MAX_VCS; i++) { >> + domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; >> - if (HAS_ENGINE(i915, _VCS(i))) >> - continue; >> + if (HAS_ENGINE(i915, _VCS(i))) >> + continue; >> - if (fw_domains & BIT(domain_id)) >> - fw_domain_fini(uncore, domain_id); >> - } >> + if (fw_domains & BIT(domain_id)) >> + fw_domain_fini(uncore, domain_id); >> + } >> - for (i = 0; i < I915_MAX_VECS; i++) { >> - domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; >> + for (i = 0; i < I915_MAX_VECS; i++) { >> + domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; >> - if (HAS_ENGINE(i915, _VECS(i))) >> - continue; >> + if (HAS_ENGINE(i915, _VECS(i))) >> + continue; >> - if (fw_domains & BIT(domain_id)) >> - fw_domain_fini(uncore, domain_id); >> - } >> + if (fw_domains & BIT(domain_id)) >> + fw_domain_fini(uncore, domain_id); >> } >> } >> void intel_uncore_fini_mmio(struct intel_uncore *uncore) >> { >> - iosf_mbi_punit_acquire(); >> - iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >> - &uncore->pmic_bus_access_nb); >> - intel_uncore_forcewake_reset(uncore); >> - iosf_mbi_punit_release(); >> + if (intel_uncore_has_forcewake(uncore)) { > > To avoid hyphotetical obnoxious diffs in the future, like the one for > intel_uncore_prune_mmio_domains above in this patch, maybe invert this > to early return straight away. will do. Daniele > >> + iosf_mbi_punit_acquire(); >> + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >> + &uncore->pmic_bus_access_nb); >> + intel_uncore_forcewake_reset(uncore); >> + iosf_mbi_punit_release(); >> + } >> + >> uncore_mmio_cleanup(uncore); >> } >> diff --git a/drivers/gpu/drm/i915/intel_uncore.h >> b/drivers/gpu/drm/i915/intel_uncore.h >> index 912616188ff5..879252735bba 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.h >> +++ b/drivers/gpu/drm/i915/intel_uncore.h >> @@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore >> *uncore) >> void intel_uncore_init_early(struct drm_i915_private *i915, >> struct intel_uncore *uncore); >> int intel_uncore_init_mmio(struct intel_uncore *uncore); >> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore); >> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore); >> bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore); >> bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore >> *uncore); >> void intel_uncore_fini_mmio(struct intel_uncore *uncore); >> -void intel_uncore_suspend(struct intel_uncore *uncore); >> -void intel_uncore_resume_early(struct intel_uncore *uncore); >> -void intel_uncore_runtime_resume(struct intel_uncore *uncore); >> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore); >> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore); >> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore); >> void assert_forcewakes_inactive(struct intel_uncore *uncore); >> void assert_forcewakes_active(struct intel_uncore *uncore, >> > > Regards, > > Tvrtko
<snip> >>> } >>> void intel_uncore_fini_mmio(struct intel_uncore *uncore) >>> { >>> - iosf_mbi_punit_acquire(); >>> - iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >>> - &uncore->pmic_bus_access_nb); >>> - intel_uncore_forcewake_reset(uncore); >>> - iosf_mbi_punit_release(); >>> + if (intel_uncore_has_forcewake(uncore)) { >> >> To avoid hyphotetical obnoxious diffs in the future, like the one for >> intel_uncore_prune_mmio_domains above in this patch, maybe invert this >> to early return straight away. > Just realized that I hadn't done that in the first place because there is a call to uncore_mmio_cleanup() below that we need to always perform and on platforms with forcewake it has to be done after clearing that, so can't return early. Daniele > will do. > > Daniele > >> >>> + iosf_mbi_punit_acquire(); >>> + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >>> + &uncore->pmic_bus_access_nb); >>> + intel_uncore_forcewake_reset(uncore); >>> + iosf_mbi_punit_release(); >>> + } >>> + >>> uncore_mmio_cleanup(uncore); >>> }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d113296cbe34..95b36fe17f99 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) intel_device_info_init_mmio(dev_priv); - intel_uncore_prune_mmio_domains(&dev_priv->uncore); + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); intel_uc_init_mmio(dev_priv); @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) i915_gem_suspend_late(dev_priv); - intel_uncore_suspend(&dev_priv->uncore); + intel_uncore_forcewake_suspend(&dev_priv->uncore); intel_power_domains_suspend(dev_priv, get_suspend_mode(dev_priv, hibernation)); @@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct drm_device *dev) DRM_ERROR("Resume prepare failed: %d, continuing anyway\n", ret); - intel_uncore_resume_early(&dev_priv->uncore); + if (intel_uncore_unclaimed_mmio(&dev_priv->uncore)) + DRM_DEBUG("unclaimed mmio detected on resume, clearing\n"); + + intel_uncore_forcewake_resume_early(&dev_priv->uncore); i915_check_and_clear_faults(dev_priv); @@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device *kdev) intel_runtime_pm_disable_interrupts(dev_priv); - intel_uncore_suspend(&dev_priv->uncore); + intel_uncore_forcewake_suspend(&dev_priv->uncore); ret = 0; if (INTEL_GEN(dev_priv) >= 11) { @@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device *kdev) if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); - intel_uncore_runtime_resume(&dev_priv->uncore); + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); intel_runtime_pm_enable_interrupts(dev_priv); @@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device *kdev) ret = vlv_resume_prepare(dev_priv, true); } - intel_uncore_runtime_resume(&dev_priv->uncore); + intel_uncore_forcewake_runtime_resume(&dev_priv->uncore); intel_runtime_pm_enable_interrupts(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 88a69bf713c9..c0f5567ee096 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore) return ret; } -static void __intel_uncore_early_sanitize(struct intel_uncore *uncore, +static void forcewake_early_sanitize(struct intel_uncore *uncore, unsigned int restore_forcewake) { - /* clear out unclaimed reg detection bit */ - if (check_for_unclaimed_mmio(uncore)) - DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); + if (!intel_uncore_has_forcewake(uncore)) + return; /* WaDisableShadowRegForCpd:chv */ if (IS_CHERRYVIEW(uncore->i915)) { @@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore, iosf_mbi_punit_release(); } -void intel_uncore_suspend(struct intel_uncore *uncore) +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore) { + if (!intel_uncore_has_forcewake(uncore)) + return; + iosf_mbi_punit_acquire(); iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( &uncore->pmic_bus_access_nb); @@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore *uncore) iosf_mbi_punit_release(); } -void intel_uncore_resume_early(struct intel_uncore *uncore) +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore) { unsigned int restore_forcewake; + if (!intel_uncore_has_forcewake(uncore)) + return; + restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved); - __intel_uncore_early_sanitize(uncore, restore_forcewake); + forcewake_early_sanitize(uncore, restore_forcewake); iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); } -void intel_uncore_runtime_resume(struct intel_uncore *uncore) +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore) { + if (!intel_uncore_has_forcewake(uncore)) + return; + iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); } @@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore) { struct drm_i915_private *i915 = uncore->i915; - if (!intel_uncore_has_forcewake(uncore)) - return; + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); if (INTEL_GEN(i915) >= 11) { int i; @@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct drm_i915_private *i915, uncore->rpm = &i915->runtime_pm; } -int intel_uncore_init_mmio(struct intel_uncore *uncore) +static void uncore_raw_init(struct intel_uncore *uncore) { - struct drm_i915_private *i915 = uncore->i915; - int ret; + GEM_BUG_ON(intel_uncore_has_forcewake(uncore)); - ret = uncore_mmio_setup(uncore); - if (ret) - return ret; + if (IS_GEN(uncore->i915, 5)) { + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); + } else { + ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); + ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); + } +} - i915_check_vgpu(i915); +static void uncore_forcewake_init(struct intel_uncore *uncore) +{ + struct drm_i915_private *i915 = uncore->i915; - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) - uncore->flags |= UNCORE_HAS_FORCEWAKE; + GEM_BUG_ON(!intel_uncore_has_forcewake(uncore)); intel_uncore_fw_domains_init(uncore); - __intel_uncore_early_sanitize(uncore, 0); + forcewake_early_sanitize(uncore, 0); - uncore->unclaimed_mmio_check = 1; - uncore->pmic_bus_access_nb.notifier_call = - i915_pmic_bus_access_notifier; - - if (!intel_uncore_has_forcewake(uncore)) { - if (IS_GEN(i915, 5)) { - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5); - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5); - } else { - ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2); - ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2); - } - } else if (IS_GEN_RANGE(i915, 6, 7)) { + if (IS_GEN_RANGE(i915, 6, 7)) { ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6); if (IS_VALLEYVIEW(i915)) { @@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges); ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable); ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable); - } else { ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8); ASSIGN_READ_MMIO_VFUNCS(uncore, gen6); @@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable); } + uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier; + iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); +} + +int intel_uncore_init_mmio(struct intel_uncore *uncore) +{ + struct drm_i915_private *i915 = uncore->i915; + int ret; + + ret = uncore_mmio_setup(uncore); + if (ret) + return ret; + + i915_check_vgpu(i915); + + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) + uncore->flags |= UNCORE_HAS_FORCEWAKE; + + uncore->unclaimed_mmio_check = 1; + + if (!intel_uncore_has_forcewake(uncore)) + uncore_raw_init(uncore); + else + uncore_forcewake_init(uncore); + /* make sure fw funcs are set if and only if we have fw*/ GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_get); GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_put); @@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) if (IS_GEN_RANGE(i915, 6, 7)) uncore->flags |= UNCORE_HAS_FIFO; - iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb); + /* clear out unclaimed reg detection bit */ + if (check_for_unclaimed_mmio(uncore)) + DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); return 0; } @@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) * the forcewake domains. Prune them, to make sure they only reference existing * engines. */ -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore) +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore) { struct drm_i915_private *i915 = uncore->i915; + enum forcewake_domains fw_domains = uncore->fw_domains; + enum forcewake_domain_id domain_id; + int i; - if (INTEL_GEN(i915) >= 11) { - enum forcewake_domains fw_domains = uncore->fw_domains; - enum forcewake_domain_id domain_id; - int i; + if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11) + return; - for (i = 0; i < I915_MAX_VCS; i++) { - domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; + for (i = 0; i < I915_MAX_VCS; i++) { + domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i; - if (HAS_ENGINE(i915, _VCS(i))) - continue; + if (HAS_ENGINE(i915, _VCS(i))) + continue; - if (fw_domains & BIT(domain_id)) - fw_domain_fini(uncore, domain_id); - } + if (fw_domains & BIT(domain_id)) + fw_domain_fini(uncore, domain_id); + } - for (i = 0; i < I915_MAX_VECS; i++) { - domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; + for (i = 0; i < I915_MAX_VECS; i++) { + domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i; - if (HAS_ENGINE(i915, _VECS(i))) - continue; + if (HAS_ENGINE(i915, _VECS(i))) + continue; - if (fw_domains & BIT(domain_id)) - fw_domain_fini(uncore, domain_id); - } + if (fw_domains & BIT(domain_id)) + fw_domain_fini(uncore, domain_id); } } void intel_uncore_fini_mmio(struct intel_uncore *uncore) { - iosf_mbi_punit_acquire(); - iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( - &uncore->pmic_bus_access_nb); - intel_uncore_forcewake_reset(uncore); - iosf_mbi_punit_release(); + if (intel_uncore_has_forcewake(uncore)) { + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + &uncore->pmic_bus_access_nb); + intel_uncore_forcewake_reset(uncore); + iosf_mbi_punit_release(); + } + uncore_mmio_cleanup(uncore); } diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 912616188ff5..879252735bba 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore) void intel_uncore_init_early(struct drm_i915_private *i915, struct intel_uncore *uncore); int intel_uncore_init_mmio(struct intel_uncore *uncore); -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore); +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore); bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore); bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore); void intel_uncore_fini_mmio(struct intel_uncore *uncore); -void intel_uncore_suspend(struct intel_uncore *uncore); -void intel_uncore_resume_early(struct intel_uncore *uncore); -void intel_uncore_runtime_resume(struct intel_uncore *uncore); +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore); +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore); +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore); void assert_forcewakes_inactive(struct intel_uncore *uncore); void assert_forcewakes_active(struct intel_uncore *uncore,
We always call some of the setup/cleanup functions for forcewake, even if the feature is not actually available. Skipping these operations if forcewake is not available saves us some operations on older gens and prepares us for having a forcewake-less display uncore. The suspend/resume functions have also been renamed to clearly indicate that they only operate on forcewake status. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 15 +-- drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- drivers/gpu/drm/i915/intel_uncore.h | 8 +- 3 files changed, 101 insertions(+), 69 deletions(-)