From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [RFC 1/6] drm/i915: add initial Runtime PM functions Date: Mon, 28 Oct 2013 15:21:28 +0200 Message-ID: <1382966488.5775.72.camel@intelbox> References: <1382470214-1597-1-git-send-email-przanoni@gmail.com> <1382470214-1597-2-git-send-email-przanoni@gmail.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1222199032==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 14AE8E626E for ; Mon, 28 Oct 2013 06:21:31 -0700 (PDT) In-Reply-To: <1382470214-1597-2-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org --===============1222199032== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-MEQ6w62dGraEk82z0PT7" --=-MEQ6w62dGraEk82z0PT7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote: > From: Paulo Zanoni >=20 > This patch adds the initial infrastructure to allow a Runtime PM > implementation that sets the device to its D3 state. The patch just > adds the necessary callbacks and the initial infrastructure. >=20 > We still don't have any platform that actually uses this > infrastructure, we still don't call get/put in all the places we need > to, and we don't have any function to save/restore the state of the > registers. This is not a problem since no platform uses the code added > by this patch. We have a few people simultaneously working on runtime > PM, so this initial code could help everybody make their plans. >=20 > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_dma.c | 38 +++++++++++++++++++++++++++++++= ++ > drivers/gpu/drm/i915/i915_drv.c | 42 +++++++++++++++++++++++++++++++= ++++++ > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 9 ++++++++ > 6 files changed, 124 insertions(+) >=20 > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_= dma.c > index fd848ef..6aa044e 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -42,6 +42,8 @@ > #include > #include > #include > +#include > +#include > =20 > #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS]) > =20 > @@ -1449,6 +1451,38 @@ static void i915_dump_device_info(struct drm_i915_= private *dev_priv) > #undef SEP_COMMA > } > =20 > +static void i915_init_runtime_pm(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct device *device =3D &dev->pdev->dev; > + > + dev_priv->pm.suspended =3D false; > + > + if (!HAS_RUNTIME_PM(dev)) > + return; > + > + pm_runtime_set_active(device); > + pm_runtime_enable(device); > + > + pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */ > + pm_runtime_mark_last_busy(device); > + pm_runtime_use_autosuspend(device); > + pm_runtime_allow(device); This shouldn't be needed as we get here already with an allowed state. It's not a problem as it's just a nop here, but imo it's confusing that we don't have the corresponding pm_runtime_forbid() in i915_fini_runtime_pm(). > +} > + > +static void i915_fini_runtime_pm(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct device *device =3D &dev->pdev->dev; > + > + if (!HAS_RUNTIME_PM(dev)) > + return; > + > + /* Make sure we're not suspended first. */ > + pm_runtime_get_sync(device); > + pm_runtime_disable(device); > +} > + Could we have the above functions in intel_pm.c? > /** > * i915_driver_load - setup chip and create an initial config > * @dev: DRM device > @@ -1664,6 +1698,8 @@ int i915_driver_load(struct drm_device *dev, unsign= ed long flags) > if (IS_GEN5(dev)) > intel_gpu_ips_init(dev_priv); > =20 > + i915_init_runtime_pm(dev_priv); > + > return 0; > =20 > out_power_well: > @@ -1704,6 +1740,8 @@ int i915_driver_unload(struct drm_device *dev) > struct drm_i915_private *dev_priv =3D dev->dev_private; > int ret; > =20 > + i915_fini_runtime_pm(dev_priv); > + > intel_gpu_ips_teardown(); > =20 > if (HAS_POWER_WELL(dev)) { > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_= drv.c > index 85ae0dc..08fc7ea 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -469,6 +469,8 @@ static int i915_drm_freeze(struct drm_device *dev) > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct drm_crtc *crtc; > =20 > + intel_runtime_pm_get(dev_priv); > + > /* ignore lid events during suspend */ > mutex_lock(&dev_priv->modeset_restore_lock); > dev_priv->modeset_restore =3D MODESET_SUSPENDED; > @@ -653,6 +655,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bo= ol restore_gtt_mappings) > mutex_lock(&dev_priv->modeset_restore_lock); > dev_priv->modeset_restore =3D MODESET_DONE; > mutex_unlock(&dev_priv->modeset_restore_lock); > + > + intel_runtime_pm_put(dev_priv); > return error; > } > =20 > @@ -880,6 +884,42 @@ static int i915_pm_poweroff(struct device *dev) > return i915_drm_freeze(drm_dev); > } > =20 > +static int i915_runtime_suspend(struct device *device) > +{ > + struct pci_dev *pdev =3D to_pci_dev(device); > + struct drm_device *dev =3D pci_get_drvdata(pdev); > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + > + WARN_ON(!HAS_RUNTIME_PM(dev)); > + > + DRM_DEBUG_KMS("Suspending device\n"); > + > + dev_priv->pm.suspended =3D true; > + > + pci_save_state(pdev); > + pci_set_power_state(pdev, PCI_D3cold); > + > + return 0; > +} > + > +static int i915_runtime_resume(struct device *device) > +{ > + struct pci_dev *pdev =3D to_pci_dev(device); > + struct drm_device *dev =3D pci_get_drvdata(pdev); > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + > + WARN_ON(!HAS_RUNTIME_PM(dev)); > + > + DRM_DEBUG_KMS("Resuming device\n"); > + > + pci_set_power_state(pdev, PCI_D0); > + pci_restore_state(pdev); > + > + dev_priv->pm.suspended =3D false; > + > + return 0; > +} > + > static const struct dev_pm_ops i915_pm_ops =3D { > .suspend =3D i915_pm_suspend, > .resume =3D i915_pm_resume, > @@ -887,6 +927,8 @@ static const struct dev_pm_ops i915_pm_ops =3D { > .thaw =3D i915_pm_thaw, > .poweroff =3D i915_pm_poweroff, > .restore =3D i915_pm_resume, > + .runtime_suspend =3D i915_runtime_suspend, > + .runtime_resume =3D i915_runtime_resume, > }; > =20 > static const struct vm_operations_struct i915_gem_vm_ops =3D { > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 80957ca..74f2b5d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1230,6 +1230,10 @@ struct i915_package_c8 { > } regsave; > }; > =20 > +struct i915_runtime_pm { > + bool suspended; > +}; > + > enum intel_pipe_crc_source { > INTEL_PIPE_CRC_SOURCE_NONE, > INTEL_PIPE_CRC_SOURCE_PLANE1, > @@ -1462,6 +1466,8 @@ typedef struct drm_i915_private { > =20 > struct i915_package_c8 pc8; > =20 > + struct i915_runtime_pm pm; > + > /* Old dri1 support infrastructure, beware the dragons ya fools enterin= g > * here! */ > struct i915_dri1_state dri1; > @@ -1779,6 +1785,7 @@ struct drm_i915_file_private { > #define HAS_POWER_WELL(dev) (IS_HASWELL(dev)) > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > #define HAS_PSR(dev) (IS_HASWELL(dev)) > +#define HAS_RUNTIME_PM(dev) false > =20 > #define INTEL_PCH_DEVICE_ID_MASK 0xff00 > #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00 > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index af1553c..80b5a80 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -840,6 +840,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)= ; > void gen6_rps_boost(struct drm_i915_private *dev_priv); > void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv); > void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv); > +void intel_runtime_pm_get(struct drm_i915_private *dev_priv); > +void intel_runtime_pm_put(struct drm_i915_private *dev_priv); > void ilk_wm_get_hw_state(struct drm_device *dev); > =20 >=20 > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index 3e140ab..a973f35 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -31,6 +31,7 @@ > #include "../../../platform/x86/intel_ips.h" > #include > #include > +#include > =20 > /** > * RC6 is a special power stage which allows the GPU to enter an very > @@ -5779,6 +5780,31 @@ void intel_aux_display_runtime_put(struct drm_i915= _private *dev_priv) > hsw_enable_package_c8(dev_priv); > } > =20 > +void intel_runtime_pm_get(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct device *device =3D &dev->pdev->dev; > + > + if (!HAS_RUNTIME_PM(dev)) > + return; > + > + pm_runtime_mark_last_busy(device); What's the point of calling this here? We call pm_runtime_get below, which will prevent an auto suspend anyway and we call pm_runtime_mark_last_busy() then intel_runtime_pm_put(). > + pm_runtime_get(device); > + pm_runtime_barrier(device); Why not using pm_runtime_get_sync() instead of the above 2 calls? --Imre > +} > + > +void intel_runtime_pm_put(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct device *device =3D &dev->pdev->dev; > + > + if (!HAS_RUNTIME_PM(dev)) > + return; > + > + pm_runtime_mark_last_busy(device); > + pm_runtime_put_autosuspend(device); > +} > + > /* Set up chip specific power management-related functions */ > void intel_init_pm(struct drm_device *dev) > { > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/i= ntel_uncore.c > index f6fae35..f5a2a6d 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -343,6 +343,13 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev= _priv, u32 reg) > } > } > =20 > +static void > +assert_device_not_suspended(struct drm_i915_private *dev_priv) > +{ > + WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended, > + "Device suspended\n"); > +} > + > #define REG_READ_HEADER(x) \ > unsigned long irqflags; \ > u##x val =3D 0; \ > @@ -435,6 +442,7 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_= t reg, u##x val, bool trace > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > __fifo_ret =3D __gen6_gt_wait_for_fifo(dev_priv); \ > } \ > + assert_device_not_suspended(dev_priv); \ > __raw_i915_write##x(dev_priv, reg, val); \ > if (unlikely(__fifo_ret)) { \ > gen6_gt_check_fifodbg(dev_priv); \ > @@ -450,6 +458,7 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t= reg, u##x val, bool trace) > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > __fifo_ret =3D __gen6_gt_wait_for_fifo(dev_priv); \ > } \ > + assert_device_not_suspended(dev_priv); \ > hsw_unclaimed_reg_clear(dev_priv, reg); \ > __raw_i915_write##x(dev_priv, reg, val); \ > if (unlikely(__fifo_ret)) { \ --=-MEQ6w62dGraEk82z0PT7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQEcBAABAgAGBQJSbmTYAAoJEORIIAnNuWDFvm4IAMx+RE+EtFTSqZy/Cvd/IxiM 78dA2MJp/j7UdNwiPRFjAyJH6uZjgB3gzST3WMM7Dxtrkh//r37lvz4pMzrWDM6e EQifvwotqbs/YunkRjTyW/p3Aoblb6y2NaixjjNIc+cTIwkX70iR9STYL4OLuocI 6fEMCvXHr8zn8DG8tESt3bpe/yBifDQ7ht+B6SaWz504fY1X3mK+uTXH+d6MfHqr CH/kDCvHyOReQ6rnUKgkr2XbQmpSn9UEswTZclhfJPFHkjORIyo4VBcCDf77zBTn NNXXLS7NGvXCw6KeeY4Ufxk6oef3WgxzdmEaV5URl4AKSMXR4sdT0bScCnQab30= =xrOo -----END PGP SIGNATURE----- --=-MEQ6w62dGraEk82z0PT7-- --===============1222199032== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1222199032==--