Re: [PATCH v3 24/25] drm/i915: propagate the error code from runtime PM callbacks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> > > ---
> > >  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.

> 
> --Imre
> 
> > This patch itself looks ok to me:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > >  
> > > @@ -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@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

-- 
Ville Syrjälä
Intel OTC