From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v3 24/25] drm/i915: propagate the error code from runtime PM callbacks Date: Wed, 30 Apr 2014 21:05:30 +0300 Message-ID: <20140430180530.GP18465@intel.com> References: <1397496286-29649-25-git-send-email-imre.deak@intel.com> <1397569185-3353-1-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id E7FC46E1B3 for ; Wed, 30 Apr 2014 11:06:15 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1397569185-3353-1-git-send-email-imre.deak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Imre Deak Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Apr 15, 2014 at 04:39:45PM +0300, Imre Deak wrote: > Atm, none of the RPM callbacks can fail, but the next patch adding > RPM support for VLV changes this, so prepare for it. > = > In case one of these callbacks return error RPM will get permanently > disabled until the error is explicitly cleared. In the future we could > add support for re-enabling it, for example after resetting the HW, but > for now - hopefully - we can live with the simpler solution. > = > v2: > - propagate the error from the resume callbacks too (Paulo) > v3: > - fix rebase fail typo around IS_GEN6() check in intel_runtime_suspend() > = > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.c | 57 ++++++++++++++++++++++++++++++-----= ------ > 1 file changed, 42 insertions(+), 15 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_= drv.c > index 845e1e1..aeb7dec 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -888,21 +888,27 @@ static int i915_pm_poweroff(struct device *dev) > return i915_drm_freeze(drm_dev); > } > = > -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) > +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv) > { > hsw_enable_pc8(dev_priv); > + > + return 0; > } > = > -static void snb_runtime_resume(struct drm_i915_private *dev_priv) > +static int snb_runtime_resume(struct drm_i915_private *dev_priv) > { > struct drm_device *dev =3D dev_priv->dev; > = > intel_init_pch_refclk(dev); > + > + return 0; > } > = > -static void hsw_runtime_resume(struct drm_i915_private *dev_priv) > +static int hsw_runtime_resume(struct drm_i915_private *dev_priv) > { > hsw_disable_pc8(dev_priv); > + > + return 0; > } > = > int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) > @@ -947,6 +953,7 @@ static int intel_runtime_suspend(struct device *devic= e) > 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; > + int ret; > = > if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev)))) > return -ENODEV; > @@ -959,12 +966,21 @@ static int intel_runtime_suspend(struct device *dev= ice) > intel_runtime_pm_disable_interrupts(dev); > cancel_work_sync(&dev_priv->rps.work); > = > - if (IS_GEN6(dev)) > - ; > - else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - hsw_runtime_suspend(dev_priv); > - else > + if (IS_GEN6(dev)) { > + ret =3D 0; > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret =3D hsw_runtime_suspend(dev_priv); > + } else { > + ret =3D -ENODEV; > WARN_ON(1); > + } > + > + if (ret) { > + DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > + intel_runtime_pm_restore_interrupts(dev); > + > + return ret; > + } > = > i915_gem_release_all_mmaps(dev_priv); Not strictly related to this patch, but shouldn't we nuke the mmaps before calling the platform specific runtime suspend function? This patch itself looks ok to me: Reviewed-by: Ville Syrj=E4l=E4 > = > @@ -989,6 +1005,7 @@ static int intel_runtime_resume(struct device *devic= e) > 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; > + int ret; > = > WARN_ON(!HAS_RUNTIME_PM(dev)); > = > @@ -997,21 +1014,31 @@ static int intel_runtime_resume(struct device *dev= ice) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended =3D false; > = > - if (IS_GEN6(dev)) > - snb_runtime_resume(dev_priv); > - else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - hsw_runtime_resume(dev_priv); > - else > + if (IS_GEN6(dev)) { > + ret =3D snb_runtime_resume(dev_priv); > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret =3D hsw_runtime_resume(dev_priv); > + } else { > WARN_ON(1); > + ret =3D -ENODEV; > + } > = > + /* > + * 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). > + */ > i915_gem_init_swizzling(dev); > gen6_update_ring_freq(dev); > intel_reset_gt_powersave(dev); > = > intel_runtime_pm_restore_interrupts(dev); > = > - DRM_DEBUG_KMS("Device resumed\n"); > - return 0; > + if (ret) > + DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret); > + else > + DRM_DEBUG_KMS("Device resumed\n"); > + > + return ret; > } > = > static const struct dev_pm_ops i915_pm_ops =3D { > -- = > 1.8.4 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC