[Intel-gfx] [PATCH v2 01/11] drm/i915: Change i915_request power well handling

[Paulo Zanoni]

2013/9/16  <ville.syrjala at linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Reorganize the internal i915_request power well handling to use the
> reference count just like everyone else. This way all we need to do is
> check the reference count and we know whether the power well needs to be
> enabled of disabled.
>
> v2: Split he intel_display_power_{get,put} change to another patch.
>     Add intel_resume_power_well() to make sure we enable the power
>     well on resume
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 43 +++++++++++++++++++++++++++++++---------
>  2 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 774ebb6..8853f53 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -765,6 +765,7 @@ extern bool intel_display_power_enabled(struct drm_device *dev,
>                                         enum intel_display_power_domain domain);
>  extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> +extern void intel_resume_power_well(struct drm_device *dev);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void ironlake_teardown_rc6(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8cffef4..310d2ed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5342,8 +5342,7 @@ void i915_request_power_well(void)
>                 return;
>
>         spin_lock_irq(&hsw_pwr->lock);
> -       if (!hsw_pwr->count++ &&
> -                       !hsw_pwr->i915_request)
> +       if (!hsw_pwr->count++)
>                 __intel_set_power_well(hsw_pwr->device, true);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5357,8 +5356,7 @@ void i915_release_power_well(void)
>
>         spin_lock_irq(&hsw_pwr->lock);
>         WARN_ON(!hsw_pwr->count);
> -       if (!--hsw_pwr->count &&
> -                      !hsw_pwr->i915_request)
> +       if (!--hsw_pwr->count)
>                 __intel_set_power_well(hsw_pwr->device, false);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5394,15 +5392,41 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 return;
>
>         spin_lock_irq(&power_well->lock);
> +
> +       /*
> +        * This function will only ever contribute one
> +        * to the power well reference count. i915_request
> +        * is what tracks whether we have or have not
> +        * added the one to the reference count.
> +        */
> +       if (power_well->i915_request == enable)
> +               goto out;
> +
>         power_well->i915_request = enable;
>
> -       /* only reject "disable" power well request */
> -       if (power_well->count && !enable) {
> -               spin_unlock_irq(&power_well->lock);
> -               return;
> +       if (enable) {
> +               if (!power_well->count++)
> +                       __intel_set_power_well(dev, true);
> +       } else {
> +               WARN_ON(!power_well->count);
> +               if (!--power_well->count)
> +                       __intel_set_power_well(dev, false);
>         }
>
> -       __intel_set_power_well(dev, enable);
> + out:
> +       spin_unlock_irq(&power_well->lock);
> +}
> +
> +void intel_resume_power_well(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_well *power_well = &dev_priv->power_well;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return;
> +
> +       spin_lock_irq(&power_well->lock);
> +       __intel_set_power_well(dev, power_well->count > 0);
>         spin_unlock_irq(&power_well->lock);
>  }
>
> @@ -5421,6 +5445,7 @@ void intel_init_power_well(struct drm_device *dev)
>
>         /* For now, we need the power well to be always enabled. */
>         intel_set_power_well(dev, true);
> +       intel_resume_power_well(dev);

I find this a little bit confusing because we basically have 2
functions that maybe call __intel_set_power_well, and in the init code
we end calling it twice. It would be nicer if we had only 1 codepath
leading to __intel_set_power_well.


>
>         /* We're taking over the BIOS, so clear any requests made by it since
>          * the driver is in charge now. */
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni