From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/2] drm/i915/vlv: Set D3_hot for vlv during runtime_suspend Date: Wed, 11 Jun 2014 12:17:17 +0200 Message-ID: <20140611101717.GF5821@phenom.ffwll.local> References: <1402340250-3909-1-git-send-email-sagar.a.kamble@intel.com> <1402340250-3909-3-git-send-email-sagar.a.kamble@intel.com> <1402404183.7876.67.camel@intelbox> <1402421727.3724.28.camel@sagar-desktop> <1402422680.2882.22.camel@ideak-mobl> <1402475029.3724.30.camel@sagar-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f176.google.com (mail-wi0-f176.google.com [209.85.212.176]) by gabe.freedesktop.org (Postfix) with ESMTP id 3298B6E31A for ; Wed, 11 Jun 2014 03:17:26 -0700 (PDT) Received: by mail-wi0-f176.google.com with SMTP id n3so5028249wiv.15 for ; Wed, 11 Jun 2014 03:17:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402475029.3724.30.camel@sagar-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Sagar Arun Kamble Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, shashidhar.hiremath@intel.com List-Id: intel-gfx@lists.freedesktop.org On Wed, Jun 11, 2014 at 01:53:49PM +0530, Sagar Arun Kamble wrote: > This patch can be marked as "abandoned". > Have verified locally that pci driver is taking care of D0ix transitions > and state save/restore. That still leaves the question why you originally thought this is required. What did blow up here in your testing? Thanks, Daniel > > Thanks Imre for the clarification. > > On Tue, 2014-06-10 at 20:51 +0300, Imre Deak wrote: > > On Tue, 2014-06-10 at 23:05 +0530, Sagar Arun Kamble wrote: > > > On Tue, 2014-06-10 at 15:43 +0300, Imre Deak wrote: > > > > On Tue, 2014-06-10 at 00:27 +0530, sagar.a.kamble@intel.com wrote: > > > > > From: Sagar Kamble > > > > > > > > > > To do a platform wide S0i3 transition, Gfx is required to go > > > > > to D3_hot state. pci_save_state and pci_restore_state needed to avoid ring > > > > > hangs across D3_hot transitions. > > > > > > > > > > Cc: Daniel Vetter (supporter:INTEL DRM DRIVERS...) > > > > > Cc: Jani Nikula (supporter:INTEL DRM DRIVERS...) > > > > > Signed-off-by: Sagar Kamble > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 11 +++++++++++ > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 5a08c86..70bb456 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -1412,6 +1412,11 @@ static int intel_runtime_suspend(struct device *device) > > > > > * via the suspend path. > > > > > */ > > > > > intel_opregion_notify_adapter(dev, PCI_D1); > > > > > + if (IS_VALLEYVIEW(dev)) { > > > > > + pci_save_state(pdev); > > > > > + pci_disable_device(pdev); > > > > > + pci_set_power_state(pdev, PCI_D3hot); > > > > > + } > > > > > > > > > > DRM_DEBUG_KMS("Device suspended\n"); > > > > > return 0; > > > > > @@ -1428,6 +1433,12 @@ static int intel_runtime_resume(struct device *device) > > > > > > > > > > DRM_DEBUG_KMS("Resuming device\n"); > > > > > > > > > > + if (IS_VALLEYVIEW(dev)) { > > > > > + pci_set_power_state(pdev, PCI_D0); > > > > > + pci_restore_state(pdev); > > > > > + pci_enable_device(pdev); > > > > > + } > > > > > > > > Setting the proper Dx state and saving/restoring the PCI config space is > > > > already done for us by the PCI runtime PM framework, see > > > > pci_pm_runtime_suspend/resume(). > > > > > > Where exactly it calls pci_pm_set_powerstate to set D3_hot ? > > > > It's > > pci_pm_runtime_suspend()->pci_finish_runtime_suspend()->pci_set_power_state() . > > > > > There seems to be bug in the pci driver in the suspend path. it is not > > > saving the pci configuration space although code for same exists. > > > pci driver checks if client driver has saved if not it will exit from > > > suspend path. > > > check the code snippet below: > > > > > > if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 > > > && pci_dev->current_state != PCI_UNKNOWN) { > > > WARN_ONCE(pci_dev->current_state != prev, > > > "PCI PM: State of device not saved by %pF\n", > > > pm->runtime_suspend); > > > return 0; > > > } > > > > > > if (!pci_dev->state_saved) { > > > pci_save_state(pci_dev); > > > pci_finish_runtime_suspend(pci_dev); > > > } > > > > I don't think this is a bug, but something the PCI framework requires > > from drivers. That is if they set the power state to something else than > > PCI_D0 or PCI_UNKNOWN they also have to save the state themselves. > > > > I can't check this right now, but I haven't ever seen the above WARN and > > by the look of it we are doing the right thing in the driver. That is > > leave the power state in D0 and let the PCI framework handle the state > > saving/power state transitioning. > > > > --Imre > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch