[Intel-gfx] [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Tue Jun 18 21:12:45 UTC 2019
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 at 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
More information about the Intel-gfx
mailing list