From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore
Date: Tue, 18 Jun 2019 10:00:40 +0100 [thread overview]
Message-ID: <3bf6ae46-ac6d-b5f5-7e2e-888819bdbc6b@linux.intel.com> (raw)
In-Reply-To: <20190617180935.505-5-daniele.ceraolospurio@intel.com>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-06-18 9:00 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-17 18:09 [PATCH 0/6] Display uncore prep patches Daniele Ceraolo Spurio
2019-06-17 18:09 ` [PATCH 1/6] drm/i915: use vfuncs for reg_read/write_fw_domains Daniele Ceraolo Spurio
2019-06-18 8:31 ` Tvrtko Ursulin
2019-06-17 18:09 ` [PATCH 2/6] drm/i915: kill uncore_sanitize Daniele Ceraolo Spurio
2019-06-17 18:09 ` [PATCH 3/6] drm/i915: kill uncore_to_i915 Daniele Ceraolo Spurio
2019-06-18 8:34 ` Tvrtko Ursulin
2019-06-17 18:09 ` [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore Daniele Ceraolo Spurio
2019-06-18 9:00 ` Tvrtko Ursulin [this message]
2019-06-18 21:12 ` Daniele Ceraolo Spurio
2019-06-19 22:05 ` Daniele Ceraolo Spurio
2019-06-18 10:22 ` Chris Wilson
2019-06-18 18:40 ` Daniele Ceraolo Spurio
2019-06-18 18:57 ` Chris Wilson
2019-06-17 18:09 ` [PATCH 5/6] drm/i915: dynamically allocate forcewake domains Daniele Ceraolo Spurio
2019-06-18 9:23 ` Tvrtko Ursulin
2019-06-18 23:06 ` Daniele Ceraolo Spurio
2019-06-18 23:23 ` Chris Wilson
2019-06-18 23:37 ` Daniele Ceraolo Spurio
2019-06-19 14:22 ` Tvrtko Ursulin
2019-06-17 18:09 ` [PATCH 6/6] drm/i915/gvt: decouple check_vgpu() from uncore_init() Daniele Ceraolo Spurio
2019-06-18 10:49 ` Chris Wilson
2019-06-17 18:53 ` ✗ Fi.CI.CHECKPATCH: warning for Display uncore prep patches Patchwork
2019-06-17 19:09 ` ✓ Fi.CI.BAT: success " Patchwork
2019-06-18 9:15 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3bf6ae46-ac6d-b5f5-7e2e-888819bdbc6b@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox