[Intel-gfx] [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Paulo Zanoni]

2013/10/10 Chris Wilson <chris at chris-wilson.co.uk>:
> 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 at chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni