On Mon, Jun 3, 2019 at 4:27 PM Jian-Hong Pan <jian-hong@xxxxxxxxxxxx> wrote:
> static void i915_audio_component_get_power(struct device *kdev)
> {
> - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> + struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +
> + /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */
> + if (dev_priv->audio_power_refcount++ == 0)
> + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> + glk_force_audio_cdclk(dev_priv, true);
> }
>
> static void i915_audio_component_put_power(struct device *kdev)
> {
> - intel_display_power_put_unchecked(kdev_to_i915(kdev),
> - POWER_DOMAIN_AUDIO);
> + struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> + intel_wakeref_t cookie;
> +
> + /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */
> + if (--dev_priv->audio_power_refcount == 0)
> + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> + glk_force_audio_cdclk(dev_priv, false);
> +
> + cookie = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> + intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie);
> }
The code above is the rediffed part of the patch. I think the last 2
lines here are not quite right.
Here, get means "increment reference count" and put means "decrease
reference count".
Before your patch, i915_audio_component_get_power() does
intel_display_power_get(), and i915_audio_component_put_power() does
intel_display_power_put_unchecked(). You can see the symmetry.
After your patch, i915_audio_component_get_power() does
intel_display_power_get() as before (good), but
i915_audio_component_put_power() does a 2nd get and then a single put.
So the reference count is now unbalanced.
I think the last 2 lines of this function should just be left the same
as they were before:
intel_display_power_put_unchecked(kdev_to_i915(kdev),
POWER_DOMAIN_AUDIO);
Daniel
