On Mon, 2014-05-05 at 15:44 +0300, Ville Syrjälä wrote:
> On Wed, Apr 30, 2014 at 09:53:08PM +0300, Imre Deak wrote:
> > On Wed, 2014-04-30 at 21:05 +0300, Ville Syrjälä wrote:
> > > 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 <imre.deak@xxxxxxxxx>
> > > > ---
> > > > 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 = 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 *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;
> > > > + 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 *device)
> > > > 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 = 0;
> > > > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > > > + ret = hsw_runtime_suspend(dev_priv);
> > > > + } else {
> > > > + ret = -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?
> >
> > We take an RPM ref on the fault path, so at least this couldn't race
> > with user space either way. But if you meant that the platform hooks
> > could save/restore some stale state because of this ordering, then I
> > agree it'd be safer to move it before calling the platform hook. But I
> > don't see what this state would be.
>
> My main worry is that something may get turned off in the platform
> specific code which is required by the hardware to service GTT
> accesses. A concurrect GTT access may then happen after the platform
> spefic code has turned off the hardware but before the mmaps have been
> released and hence there's no fault when the access happens.
Right, for existing maps this may be a problem. I'll follow-up with a
patch moving i915_gem_release_all_mmaps() earlier.
> > --Imre
> >
> > > This patch itself looks ok to me:
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > >
> > > > @@ -989,6 +1005,7 @@ static int intel_runtime_resume(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;
> > > > + int ret;
> > > >
> > > > WARN_ON(!HAS_RUNTIME_PM(dev));
> > > >
> > > > @@ -997,21 +1014,31 @@ static int intel_runtime_resume(struct device *device)
> > > > intel_opregion_notify_adapter(dev, PCI_D0);
> > > > dev_priv->pm.suspended = 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 = snb_runtime_resume(dev_priv);
> > > > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > > > + ret = hsw_runtime_resume(dev_priv);
> > > > + } else {
> > > > WARN_ON(1);
> > > > + ret = -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 = {
> > > > --
> > > > 1.8.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
