From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 05/19] drm/i915: add initial Runtime PM functions Date: Sun, 08 Dec 2013 10:06:39 +0100 Message-ID: References: <1385048853-1579-6-git-send-email-przanoni@gmail.com> <1385583030-2533-1-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id 0A2D8FA46A for ; Sun, 8 Dec 2013 01:06:40 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org At Fri, 6 Dec 2013 20:31:04 -0200, Paulo Zanoni wrote: > > 2013/11/29 Takashi Iwai : > > At Wed, 27 Nov 2013 18:10:30 -0200, > > Paulo Zanoni wrote: > >> > >> From: Paulo Zanoni > >> > >> 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. > >> > >> 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. > >> > >> V2: - Move some functions to intel_pm.c > >> - Remove useless pm_runtime_allow() call at init > >> - Remove useless pm_runtime_mark_last_busy() call at get > >> - Use pm_runtime_get_sync() instead of 2 calls > >> - Add a WARN to check if we're really awake > >> > >> V3: - Rebase. > >> > >> Signed-off-by: Paulo Zanoni > >> --- > >> drivers/gpu/drm/i915/i915_dma.c | 6 ++++ > >> drivers/gpu/drm/i915/i915_drv.c | 42 ++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/i915_drv.h | 7 +++++ > >> drivers/gpu/drm/i915/intel_drv.h | 4 +++ > >> drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_uncore.c | 9 ++++++ > >> 6 files changed, 124 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >> index 89e4cf1..4cdc1ee 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 > >> > >> #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS]) > >> > >> @@ -1663,6 +1665,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > >> if (IS_GEN5(dev)) > >> intel_gpu_ips_init(dev_priv); > >> > >> + intel_init_runtime_pm(dev_priv); > >> + > >> return 0; > >> > >> out_power_well: > >> @@ -1702,6 +1706,8 @@ int i915_driver_unload(struct drm_device *dev) > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> int ret; > >> > >> + intel_fini_runtime_pm(dev_priv); > >> + > >> intel_gpu_ips_teardown(); > >> > >> /* The i915.ko module is still not prepared to be loaded when > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index 0ec0fb3..d5310a0 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -502,6 +502,8 @@ static int i915_drm_freeze(struct drm_device *dev) > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct drm_crtc *crtc; > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> /* ignore lid events during suspend */ > >> mutex_lock(&dev_priv->modeset_restore_lock); > >> dev_priv->modeset_restore = MODESET_SUSPENDED; > >> @@ -688,6 +690,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > >> mutex_lock(&dev_priv->modeset_restore_lock); > >> dev_priv->modeset_restore = MODESET_DONE; > >> mutex_unlock(&dev_priv->modeset_restore_lock); > >> + > >> + intel_runtime_pm_put(dev_priv); > >> return error; > >> } > >> > >> @@ -902,6 +906,42 @@ static int i915_pm_poweroff(struct device *dev) > >> return i915_drm_freeze(drm_dev); > >> } > >> > >> +static int i915_runtime_suspend(struct device *device) > >> +{ > >> + struct pci_dev *pdev = to_pci_dev(device); > >> + struct drm_device *dev = pci_get_drvdata(pdev); > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + > >> + WARN_ON(!HAS_RUNTIME_PM(dev)); > > > > It'd be better to add runtime_idle callback for this kind of checks. > > It's called always before actually doing runtime PM, and you can > > return -EBUSY if the runtime PM isn't available. > > It's a WARN we never expect to hit, so I don't think it's worth > creating a new function just for a check like that. In the current situation with a static condition, maybe so. But in general, it'd be safer not to do conditional check in intel_runtime_pm_get()/put() but filter in runtime idle callback instead, so that you can concentrate only on consistency of refcounts in *_get() and *_put(). For example, if the condition changes dynamically, you cannot apply conditional to *_get() and *_put(); otherwise it'd result in unbalance. So the above is a recommendation from my past experiences in others drivers. And, even if you use WARN() for a static condition check there, it doesn't have to be spit at each time. It's no dynamic condition, so WARN_ON_ONCE() should suffice. Takashi