2013/10/10 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>: > If the hardware does not support package C8, then do not even schedule > work to enable it. Thereby we can eliminate a bunch of dangerous work. As I already explained, this should not be a problem since non-Haswell platforms don't have a way to make the refcount become zero (unless we have a bug). I also asked people's opinions about this specific decision in one of my cover letters, but no one said anything: http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html. Quoting the email: "Another thing worth mentioning is that all this code doesn't have IS_HASWELL checks, and on non-Haswell platforms the refcount will never reach 0, so we won't ever try to enable PC8. I'm not sure if that's what we want, so please comment on that.". That said, I'm not against your changes. But for completeness, you should probably add a WARN(!HAS_PC8()) at haswell_enable_pc8_work(). > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7ba49d1..640bff2 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1750,6 +1750,7 @@ struct drm_i915_file_private { > #define HAS_POWER_WELL(dev) (IS_HASWELL(dev)) > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > #define HAS_PSR(dev) (IS_HASWELL(dev)) > +#define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ What exactly do you mean with this comment? Did you actually mean "IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems to work fine with this code, so I thought it wouldn't be a problem. > > #define INTEL_PCH_DEVICE_ID_MASK 0xff00 > #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4fa1fd5..2e9d75d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6479,6 +6479,9 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) > > void hsw_enable_package_c8(struct drm_i915_private *dev_priv) > { > + if (!HAS_PC8(dev_priv->dev)) > + return; > + > mutex_lock(&dev_priv->pc8.lock); > __hsw_enable_package_c8(dev_priv); > mutex_unlock(&dev_priv->pc8.lock); > @@ -6486,6 +6489,9 @@ void hsw_enable_package_c8(struct drm_i915_private *dev_priv) > > void hsw_disable_package_c8(struct drm_i915_private *dev_priv) > { > + if (!HAS_PC8(dev_priv->dev)) > + return; > + > mutex_lock(&dev_priv->pc8.lock); > __hsw_disable_package_c8(dev_priv); > mutex_unlock(&dev_priv->pc8.lock); > @@ -6523,6 +6529,9 @@ static void hsw_update_package_c8(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > bool allow; > > + if (!HAS_PC8(dev_priv->dev)) > + return; > + > if (!i915_enable_pc8) > return; > > @@ -6546,6 +6555,9 @@ done: > > static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv) > { > + if (!HAS_PC8(dev_priv->dev)) > + return; > + > mutex_lock(&dev_priv->pc8.lock); > if (!dev_priv->pc8.gpu_idle) { > dev_priv->pc8.gpu_idle = true; > @@ -6556,6 +6568,9 @@ static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv) > > static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv) > { > + if (!HAS_PC8(dev_priv->dev)) > + return; > + > mutex_lock(&dev_priv->pc8.lock); > if (dev_priv->pc8.gpu_idle) { > dev_priv->pc8.gpu_idle = false; > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
