From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22FABC432BE for ; Fri, 27 Aug 2021 16:31:35 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D9C9660E99 for ; Fri, 27 Aug 2021 16:31:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D9C9660E99 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2A2716E993; Fri, 27 Aug 2021 16:31:34 +0000 (UTC) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2F1546E993 for ; Fri, 27 Aug 2021 16:31:32 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10089"; a="214861708" X-IronPort-AV: E=Sophos;i="5.84,357,1620716400"; d="scan'208";a="214861708" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Aug 2021 09:31:31 -0700 X-IronPort-AV: E=Sophos;i="5.84,357,1620716400"; d="scan'208";a="517385403" Received: from ideak-desk.fi.intel.com ([10.237.68.141]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Aug 2021 09:31:30 -0700 Date: Fri, 27 Aug 2021 19:31:26 +0300 From: Imre Deak To: Rodrigo Vivi Cc: intel-gfx@lists.freedesktop.org, Tilak Tangudu Message-ID: <20210827163126.GA3286573@ideak-desk.fi.intel.com> References: <20210825152233.2151037-1-rodrigo.vivi@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210825152233.2151037-1-rodrigo.vivi@intel.com> Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/runtime_pm: Consolidate runtime_pm functions X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, Aug 25, 2021 at 11:22:30AM -0400, Rodrigo Vivi wrote: > No functional changes. Just revamping the functions with > s/dev_priv/i915 > and consolidating along with other runtime_pm functions. > > v2: avoid the extra redirection (Imre) > > Cc: Imre Deak > Cc: Tilak Tangudu > Signed-off-by: Rodrigo Vivi Looks ok: Reviewed-by: Imre Deak One nit below. > --- > drivers/gpu/drm/i915/i915_drv.c | 145 +----------------------- > drivers/gpu/drm/i915/intel_runtime_pm.c | 145 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_runtime_pm.h | 2 + > 3 files changed, 149 insertions(+), 143 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 59fb4c710c8c..a40b5d806321 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -64,7 +64,6 @@ > #include "gem/i915_gem_mman.h" > #include "gem/i915_gem_pm.h" > #include "gt/intel_gt.h" > -#include "gt/intel_gt_pm.h" > #include "gt/intel_rc6.h" > > #include "i915_debugfs.h" > @@ -1517,146 +1516,6 @@ static int i915_pm_restore(struct device *kdev) > return i915_pm_resume(kdev); > } > > -static int intel_runtime_suspend(struct device *kdev) > -{ > - struct drm_i915_private *dev_priv = kdev_to_i915(kdev); > - struct intel_runtime_pm *rpm = &dev_priv->runtime_pm; > - int ret; > - > - if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv))) > - return -ENODEV; > - > - drm_dbg_kms(&dev_priv->drm, "Suspending device\n"); > - > - disable_rpm_wakeref_asserts(rpm); > - > - /* > - * We are safe here against re-faults, since the fault handler takes > - * an RPM reference. > - */ > - i915_gem_runtime_suspend(dev_priv); > - > - intel_gt_runtime_suspend(&dev_priv->gt); > - > - intel_runtime_pm_disable_interrupts(dev_priv); > - > - intel_uncore_suspend(&dev_priv->uncore); > - > - intel_display_power_suspend(dev_priv); > - > - ret = vlv_suspend_complete(dev_priv); > - if (ret) { > - drm_err(&dev_priv->drm, > - "Runtime suspend failed, disabling it (%d)\n", ret); > - intel_uncore_runtime_resume(&dev_priv->uncore); > - > - intel_runtime_pm_enable_interrupts(dev_priv); > - > - intel_gt_runtime_resume(&dev_priv->gt); > - > - enable_rpm_wakeref_asserts(rpm); > - > - return ret; > - } > - > - enable_rpm_wakeref_asserts(rpm); > - intel_runtime_pm_driver_release(rpm); > - > - if (intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore)) > - drm_err(&dev_priv->drm, > - "Unclaimed access detected prior to suspending\n"); > - > - rpm->suspended = true; > - > - /* > - * FIXME: We really should find a document that references the arguments > - * used below! > - */ > - if (IS_BROADWELL(dev_priv)) { > - /* > - * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop > - * being detected, and the call we do at intel_runtime_resume() > - * won't be able to restore them. Since PCI_D3hot matches the > - * actual specification and appears to be working, use it. > - */ > - intel_opregion_notify_adapter(dev_priv, PCI_D3hot); > - } else { > - /* > - * current versions of firmware which depend on this opregion > - * notification have repurposed the D1 definition to mean > - * "runtime suspended" vs. what you would normally expect (D3) > - * to distinguish it from notifications that might be sent via > - * the suspend path. > - */ > - intel_opregion_notify_adapter(dev_priv, PCI_D1); > - } > - > - assert_forcewakes_inactive(&dev_priv->uncore); > - > - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) > - intel_hpd_poll_enable(dev_priv); > - > - drm_dbg_kms(&dev_priv->drm, "Device suspended\n"); > - return 0; > -} > - > -static int intel_runtime_resume(struct device *kdev) > -{ > - struct drm_i915_private *dev_priv = kdev_to_i915(kdev); > - struct intel_runtime_pm *rpm = &dev_priv->runtime_pm; > - int ret; > - > - if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv))) > - return -ENODEV; > - > - drm_dbg_kms(&dev_priv->drm, "Resuming device\n"); > - > - drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm->wakeref_count)); > - disable_rpm_wakeref_asserts(rpm); > - > - intel_opregion_notify_adapter(dev_priv, PCI_D0); > - rpm->suspended = false; > - if (intel_uncore_unclaimed_mmio(&dev_priv->uncore)) > - drm_dbg(&dev_priv->drm, > - "Unclaimed access during suspend, bios?\n"); > - > - intel_display_power_resume(dev_priv); > - > - ret = vlv_resume_prepare(dev_priv, true); > - > - intel_uncore_runtime_resume(&dev_priv->uncore); > - > - intel_runtime_pm_enable_interrupts(dev_priv); > - > - /* > - * No point of rolling back things in case of an error, as the best > - * we can do is to hope that things will still work (and disable RPM). > - */ > - intel_gt_runtime_resume(&dev_priv->gt); > - > - /* > - * On VLV/CHV display interrupts are part of the display > - * power well, so hpd is reinitialized from there. For > - * everyone else do it here. > - */ > - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) { > - intel_hpd_init(dev_priv); > - intel_hpd_poll_disable(dev_priv); > - } > - > - intel_enable_ipc(dev_priv); > - > - enable_rpm_wakeref_asserts(rpm); > - > - if (ret) > - drm_err(&dev_priv->drm, > - "Runtime resume failed, disabling it (%d)\n", ret); > - else > - drm_dbg_kms(&dev_priv->drm, "Device resumed\n"); > - > - return ret; > -} > - > const struct dev_pm_ops i915_pm_ops = { > /* > * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, > @@ -1693,8 +1552,8 @@ const struct dev_pm_ops i915_pm_ops = { > .restore = i915_pm_restore, > > /* S0ix (via runtime suspend) event handlers */ > - .runtime_suspend = intel_runtime_suspend, > - .runtime_resume = intel_runtime_resume, > + .runtime_suspend = intel_runtime_pm_suspend, > + .runtime_resume = intel_runtime_pm_resume, > }; > > static const struct file_operations i915_driver_fops = { > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index eaf7688f517d..f28b5bab61b4 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -32,6 +32,11 @@ > > #include "i915_drv.h" > #include "i915_trace.h" > +#include "gt/intel_gt.h" > +#include "gt/intel_gt_pm.h" > +#include "intel_pm.h" > +#include "vlv_suspend.h" > +#include "display/intel_hotplug.h" I suppose these should be in the following order: display/*.h gt/*.h i915_*.h ... (taken from intel_display.c) Could add docs for the exported funcs at one point. > > /** > * DOC: runtime pm > @@ -652,3 +657,143 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm) > > init_intel_runtime_pm_wakeref(rpm); > } > + > +int intel_runtime_pm_suspend(struct device *kdev) > +{ > + struct drm_i915_private *i915 = kdev_to_i915(kdev); > + struct intel_runtime_pm *rpm = &i915->runtime_pm; > + int ret; > + > + if (drm_WARN_ON_ONCE(&i915->drm, !HAS_RUNTIME_PM(i915))) > + return -ENODEV; > + > + drm_dbg_kms(&i915->drm, "Suspending device\n"); > + > + disable_rpm_wakeref_asserts(rpm); > + > + /* > + * We are safe here against re-faults, since the fault handler takes > + * an RPM reference. > + */ > + i915_gem_runtime_suspend(i915); > + > + intel_gt_runtime_suspend(&i915->gt); > + > + intel_runtime_pm_disable_interrupts(i915); > + > + intel_uncore_suspend(&i915->uncore); > + > + intel_display_power_suspend(i915); > + > + ret = vlv_suspend_complete(i915); > + if (ret) { > + drm_err(&i915->drm, > + "Runtime suspend failed, disabling it (%d)\n", ret); > + intel_uncore_runtime_resume(&i915->uncore); > + > + intel_runtime_pm_enable_interrupts(i915); > + > + intel_gt_runtime_resume(&i915->gt); > + > + enable_rpm_wakeref_asserts(rpm); > + > + return ret; > + } > + > + enable_rpm_wakeref_asserts(rpm); > + intel_runtime_pm_driver_release(rpm); > + > + if (intel_uncore_arm_unclaimed_mmio_detection(&i915->uncore)) > + drm_err(&i915->drm, > + "Unclaimed access detected prior to suspending\n"); > + > + rpm->suspended = true; > + > + /* > + * FIXME: We really should find a document that references the arguments > + * used below! > + */ > + if (IS_BROADWELL(i915)) { > + /* > + * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop > + * being detected, and the call we do at intel_runtime_resume() > + * won't be able to restore them. Since PCI_D3hot matches the > + * actual specification and appears to be working, use it. > + */ > + intel_opregion_notify_adapter(i915, PCI_D3hot); > + } else { > + /* > + * current versions of firmware which depend on this opregion > + * notification have repurposed the D1 definition to mean > + * "runtime suspended" vs. what you would normally expect (D3) > + * to distinguish it from notifications that might be sent via > + * the suspend path. > + */ > + intel_opregion_notify_adapter(i915, PCI_D1); > + } > + > + assert_forcewakes_inactive(&i915->uncore); > + > + if (!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915)) > + intel_hpd_poll_enable(i915); > + > + drm_dbg_kms(&i915->drm, "Device suspended\n"); > + return 0; > +} > + > +int intel_runtime_pm_resume(struct device *kdev) > +{ > + struct drm_i915_private *i915 = kdev_to_i915(kdev); > + struct intel_runtime_pm *rpm = &i915->runtime_pm; > + int ret; > + > + if (drm_WARN_ON_ONCE(&i915->drm, !HAS_RUNTIME_PM(i915))) > + return -ENODEV; > + > + drm_dbg_kms(&i915->drm, "Resuming device\n"); > + > + drm_WARN_ON_ONCE(&i915->drm, atomic_read(&rpm->wakeref_count)); > + disable_rpm_wakeref_asserts(rpm); > + > + intel_opregion_notify_adapter(i915, PCI_D0); > + rpm->suspended = false; > + if (intel_uncore_unclaimed_mmio(&i915->uncore)) > + drm_dbg(&i915->drm, > + "Unclaimed access during suspend, bios?\n"); > + > + intel_display_power_resume(i915); > + > + ret = vlv_resume_prepare(i915, true); > + > + intel_uncore_runtime_resume(&i915->uncore); > + > + intel_runtime_pm_enable_interrupts(i915); > + > + /* > + * No point of rolling back things in case of an error, as the best > + * we can do is to hope that things will still work (and disable RPM). > + */ > + intel_gt_runtime_resume(&i915->gt); > + > + /* > + * On VLV/CHV display interrupts are part of the display > + * power well, so hpd is reinitialized from there. For > + * everyone else do it here. > + */ > + if (!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915)) { > + intel_hpd_init(i915); > + intel_hpd_poll_disable(i915); > + } > + > + intel_enable_ipc(i915); > + > + enable_rpm_wakeref_asserts(rpm); > + > + if (ret) > + drm_err(&i915->drm, > + "Runtime resume failed, disabling it (%d)\n", ret); > + else > + drm_dbg_kms(&i915->drm, "Device resumed\n"); > + > + return ret; > +} > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h > index 47a85fab4130..88ca531165f7 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h > @@ -172,6 +172,8 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm); > void intel_runtime_pm_enable(struct intel_runtime_pm *rpm); > void intel_runtime_pm_disable(struct intel_runtime_pm *rpm); > void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm); > +int intel_runtime_pm_suspend(struct device *kdev); > +int intel_runtime_pm_resume(struct device *kdev); > > intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm); > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm); > -- > 2.31.1 >