Hi
2013/9/12 <ville.syrjala@xxxxxxxxxxxxxxx>:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Add APIs to get/put power well references for specific purposes.
>
> Also 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.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 4 ++
> drivers/gpu/drm/i915/intel_pm.c | 92 ++++++++++++++++++++++++++++++++++++----
> 2 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 774ebb6..2ecd3d2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
>
> extern bool intel_display_power_enabled(struct drm_device *dev,
> enum intel_display_power_domain domain);
> +extern void intel_display_power_get(struct drm_device *dev,
> + enum intel_display_power_domain domain);
> +extern void intel_display_power_put(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_enable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8cffef4..4962303 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> }
> }
>
> +void intel_display_power_get(struct drm_device *dev,
> + enum intel_display_power_domain domain)
> +{
> + 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;
> +
> + switch (domain) {
> + case POWER_DOMAIN_PIPE_A:
> + case POWER_DOMAIN_TRANSCODER_EDP:
> + return;
> + case POWER_DOMAIN_PIPE_B:
> + case POWER_DOMAIN_PIPE_C:
> + case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> + case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> + case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> + case POWER_DOMAIN_TRANSCODER_A:
> + case POWER_DOMAIN_TRANSCODER_B:
> + case POWER_DOMAIN_TRANSCODER_C:
I know I'm the one who added all these domains, but I have to say I
only did this because of the reviewers, I don't really like the
interface. With your addition there's a new problem: you can get the
POWER_DOMAIN_PIPE_B interface and then put the
POWER_COMAIN_PIPE_C_PANEL_FITTER and no one will notice. I really
think the power well itself should be the domain. Also, in cases like
the suspend/resume code we don't have any domain that makes sense. But
what's *not* ugly about the power well code?
I'm not suggesting you to fix that, I'm more kinda asking for ideas, I
may want to reorganize this code yet again when doing the D3 feature.
(Just because every single time we touch the power well code we have
to refactor it!)
> + spin_lock_irq(&power_well->lock);
> + if (!power_well->count++)
> + __intel_set_power_well(power_well->device, true);
> + spin_unlock_irq(&power_well->lock);
> + return;
> + default:
> + BUG();
> + }
> +}
> +
> +void intel_display_power_put(struct drm_device *dev,
> + enum intel_display_power_domain domain)
> +{
> + 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;
> +
> + switch (domain) {
> + case POWER_DOMAIN_PIPE_A:
> + case POWER_DOMAIN_TRANSCODER_EDP:
> + return;
> + case POWER_DOMAIN_PIPE_B:
> + case POWER_DOMAIN_PIPE_C:
> + case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> + case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> + case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> + case POWER_DOMAIN_TRANSCODER_A:
> + case POWER_DOMAIN_TRANSCODER_B:
> + case POWER_DOMAIN_TRANSCODER_C:
> + spin_lock_irq(&power_well->lock);
> + WARN_ON(!power_well->count);
> + if (!--power_well->count)
> + __intel_set_power_well(power_well->device, false);
> + spin_unlock_irq(&power_well->lock);
> + return;
> + default:
> + BUG();
> + }
> +}
> +
> static struct i915_power_well *hsw_pwr;
>
> /* Display audio driver power well request */
> @@ -5342,8 +5405,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 +5419,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 +5455,28 @@ 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;
And now to the real problem of the patch: previously whenever we got a
call to "enable" we'd call __intel_set_power_well and certainly write
the register. Now with this patch we may not do this due to
i915_request and the count. This breaks suspend/resume where just
after we resume we call intel_set_power_well(dev, true) but then the
new code doesn't really writes the register since i915_request is
already true. As a consequence, we see "unclaimed register" messages
complaining about registers 70008, 71008 and 72008. Perhaps in the
resume path we should fix our tracking and force the "enable" somehow.
> + 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);
> }
>
> --
> 1.8.1.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
