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

[Imre Deak <imre.deak@intel.com>]

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

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@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
> > > 
> > 
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx