On Thu, 21 Mar 2019, Clinton Taylor <clinton.a.tay...@intel.com> wrote:
> On 3/20/19 6:54 AM, Imre Deak wrote:
>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>
>> CDCLK has to be at least twice the BLCK regardless of audio. Audio
>> driver has to probe using this hook and increase the clock even in
>> absence of any display.
>>
>> v2: Use atomic refcount for get_power, put_power so that we can
>>      call each once(Abhay).
>> v3: Reset power well 2 to avoid any transaction on iDisp link
>>      during cdclk change(Abhay).
>> v4: Remove Power well 2 reset workaround(Ville).
>> v5: Remove unwanted Power well 2 register defined in v4(Abhay).
>> v6:
>> - Use a dedicated flag instead of state->modeset for min CDCLK changes
>> - Make get/put audio power domain symmetric
>> - Rebased on top of intel_wakeref tracking changes.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> Signed-off-by: Abhay Kumar <abhay.ku...@intel.com>
>> Tested-by: Abhay Kumar <abhay.ku...@intel.com>
>> Signed-off-by: Imre Deak <imre.d...@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>>   drivers/gpu/drm/i915/intel_audio.c   | 64 
>> ++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_cdclk.c   | 30 ++++++-----------
>>   drivers/gpu/drm/i915/intel_display.c |  9 ++++-
>>   drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>>   5 files changed, 86 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index c65c2e6649df..6b10cee4e77f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1624,6 +1624,8 @@ struct drm_i915_private {
>>              struct intel_cdclk_state actual;
>>              /* The current hardware cdclk state */
>>              struct intel_cdclk_state hw;
>> +
>> +            int force_min_cdclk;
>>      } cdclk;
>>   
>>      /**
>> @@ -1743,6 +1745,7 @@ struct drm_i915_private {
>>       *
>>       */
>>      struct mutex av_mutex;
>> +    int audio_power_refcount;
>>   
>>      struct {
>>              struct mutex mutex;
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
>> b/drivers/gpu/drm/i915/intel_audio.c
>> index 502b57ce72ab..20324b0d34c7 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -741,18 +741,78 @@ void intel_init_audio_hooks(struct drm_i915_private 
>> *dev_priv)
>>      }
>>   }
>>   
>> +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
>> +                              bool enable)
>> +{
>> +    struct drm_modeset_acquire_ctx ctx;
>> +    struct drm_atomic_state *state;
>> +    int ret;
>> +
>> +    drm_modeset_acquire_init(&ctx, 0);
>> +    state = drm_atomic_state_alloc(&dev_priv->drm);
>> +    if (WARN_ON(!state))
>> +            return;
>> +
>> +    state->acquire_ctx = &ctx;
>> +
>> +retry:
>> +    to_intel_atomic_state(state)->cdclk.force_min_cdclk_changed = true;
>> +    to_intel_atomic_state(state)->cdclk.force_min_cdclk =
>> +            enable ? 2 * 96000 : 0;
> This will need to be revisited on later SOCs that may support cdclk 
> rates < 192 Mhz.
>> +
>> +    /*
>> +     * Protects dev_priv->cdclk.force_min_cdclk
>> +     * Need to lock this here in case we have no active pipes
>> +     * and thus wouldn't lock it during the commit otherwise.
>> +     */
>> +    ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
>> +                           &ctx);
>> +    if (!ret)
>> +            ret = drm_atomic_commit(state);
>> +
>> +    if (ret == -EDEADLK) {
>> +            drm_atomic_state_clear(state);
>> +            drm_modeset_backoff(&ctx);
>> +            goto retry;
>> +    }
>> +
>> +    WARN_ON(ret);
>> +
>> +    drm_atomic_state_put(state);
>> +
>> +    drm_modeset_drop_locks(&ctx);
>> +    drm_modeset_acquire_fini(&ctx);
>> +}
>> +
>>   static unsigned long i915_audio_component_get_power(struct device *kdev)
>>   {
>> +    struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>> +    intel_wakeref_t ret;
>> +
>>      /* Catch potential impedance mismatches before they occur! */
>>      BUILD_BUG_ON(sizeof(intel_wakeref_t) > sizeof(unsigned long));
>>   
>> -    return intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
>> +    ret = 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);
>> +
>> +    return ret;
>>   }
>>   
>>   static void i915_audio_component_put_power(struct device *kdev,
>>                                         unsigned long cookie)
>>   {
>> -    intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO, cookie);
>> +    struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>> +
>> +    /* 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);
>> +
>> +    intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie);
>>   }
>>   
>>   static void i915_audio_component_codec_wake_override(struct device *kdev,
>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c 
>> b/drivers/gpu/drm/i915/intel_cdclk.c
>> index 21fb4e0d6c4e..7dcca84f31d1 100644
>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>> @@ -2187,19 +2187,8 @@ int intel_crtc_compute_min_cdclk(const struct 
>> intel_crtc_state *crtc_state)
>>      /*
>>       * According to BSpec, "The CD clock frequency must be at least twice
>>       * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
>> -     *
>> -     * FIXME: Check the actual, not default, BCLK being used.
>> -     *
>> -     * FIXME: This does not depend on ->has_audio because the higher CDCLK
>> -     * is required for audio probe, also when there are no audio capable
>> -     * displays connected at probe time. This leads to unnecessarily high
>> -     * CDCLK when audio is not required.
>> -     *
>> -     * FIXME: This limit is only applied when there are displays connected
>> -     * at probe time. If we probe without displays, we'll still end up using
>> -     * the platform minimum CDCLK, failing audio probe.
>>       */
>> -    if (INTEL_GEN(dev_priv) >= 9)
>> +    if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>>              min_cdclk = max(2 * 96000, min_cdclk);
>
> Do we have plans to support a 48Mhz BCLK?

IIUC that can only be done when we don't care about audio, and AFAIK,
ahem, certain other OS has opted not to support this.

BR,
Jani.



>
>
>>   
>>      /*
>> @@ -2239,7 +2228,7 @@ static int intel_compute_min_cdclk(struct 
>> drm_atomic_state *state)
>>              intel_state->min_cdclk[i] = min_cdclk;
>>      }
>>   
>> -    min_cdclk = 0;
>> +    min_cdclk = intel_state->cdclk.force_min_cdclk;
>>      for_each_pipe(dev_priv, pipe)
>>              min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>>   
>> @@ -2300,7 +2289,8 @@ static int vlv_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>              vlv_calc_voltage_level(dev_priv, cdclk);
>>   
>>      if (!intel_state->active_crtcs) {
>> -            cdclk = vlv_calc_cdclk(dev_priv, 0);
>> +            cdclk = vlv_calc_cdclk(dev_priv,
>> +                                   intel_state->cdclk.force_min_cdclk);
>>   
>>              intel_state->cdclk.actual.cdclk = cdclk;
>>              intel_state->cdclk.actual.voltage_level =
>> @@ -2333,7 +2323,7 @@ static int bdw_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>              bdw_calc_voltage_level(cdclk);
>>   
>>      if (!intel_state->active_crtcs) {
>> -            cdclk = bdw_calc_cdclk(0);
>> +            cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>>   
>>              intel_state->cdclk.actual.cdclk = cdclk;
>>              intel_state->cdclk.actual.voltage_level =
>> @@ -2405,7 +2395,7 @@ static int skl_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>              skl_calc_voltage_level(cdclk);
>>   
>>      if (!intel_state->active_crtcs) {
>> -            cdclk = skl_calc_cdclk(0, vco);
>> +            cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
>>   
>>              intel_state->cdclk.actual.vco = vco;
>>              intel_state->cdclk.actual.cdclk = cdclk;
>> @@ -2444,10 +2434,10 @@ static int bxt_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>   
>>      if (!intel_state->active_crtcs) {
>>              if (IS_GEMINILAKE(dev_priv)) {
>> -                    cdclk = glk_calc_cdclk(0);
>> +                    cdclk = 
>> glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>>                      vco = glk_de_pll_vco(dev_priv, cdclk);
>>              } else {
>> -                    cdclk = bxt_calc_cdclk(0);
>> +                    cdclk = 
>> bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>>                      vco = bxt_de_pll_vco(dev_priv, cdclk);
>>              }
>>   
>> @@ -2483,7 +2473,7 @@ static int cnl_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>                  cnl_compute_min_voltage_level(intel_state));
>>   
>>      if (!intel_state->active_crtcs) {
>> -            cdclk = cnl_calc_cdclk(0);
>> +            cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>>              vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>>   
>>              intel_state->cdclk.actual.vco = vco;
>> @@ -2519,7 +2509,7 @@ static int icl_modeset_calc_cdclk(struct 
>> drm_atomic_state *state)
>>                  cnl_compute_min_voltage_level(intel_state));
>>   
>>      if (!intel_state->active_crtcs) {
>> -            cdclk = icl_calc_cdclk(0, ref);
>> +            cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
>>              vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
>>   
>>              intel_state->cdclk.actual.vco = vco;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 61acbaf2af75..b4199cd53349 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12973,6 +12973,11 @@ static int intel_modeset_checks(struct 
>> drm_atomic_state *state)
>>              return -EINVAL;
>>      }
>>   
>> +    /* keep the current setting */
>> +    if (!intel_state->cdclk.force_min_cdclk_changed)
>> +            intel_state->cdclk.force_min_cdclk =
>> +                    dev_priv->cdclk.force_min_cdclk;
>> +
>>      intel_state->modeset = true;
>>      intel_state->active_crtcs = dev_priv->active_crtcs;
>>      intel_state->cdclk.logical = dev_priv->cdclk.logical;
>> @@ -13068,7 +13073,7 @@ static int intel_atomic_check(struct drm_device *dev,
>>      struct drm_crtc *crtc;
>>      struct drm_crtc_state *old_crtc_state, *crtc_state;
>>      int ret, i;
>> -    bool any_ms = false;
>> +    bool any_ms = intel_state->cdclk.force_min_cdclk_changed;
>>   
>>      /* Catch I915_MODE_FLAG_INHERITED */
>>      for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>> @@ -13660,6 +13665,8 @@ static int intel_atomic_commit(struct drm_device 
>> *dev,
>>              dev_priv->active_crtcs = intel_state->active_crtcs;
>>              dev_priv->cdclk.logical = intel_state->cdclk.logical;
>>              dev_priv->cdclk.actual = intel_state->cdclk.actual;
>> +            dev_priv->cdclk.force_min_cdclk =
>> +                    intel_state->cdclk.force_min_cdclk;
>>      }
>>   
>>      drm_atomic_state_get(state);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d9f188ef21f4..0b84e557c267 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -556,6 +556,9 @@ struct intel_atomic_state {
>>               * state only when all crtc's are DPMS off.
>>               */
>>              struct intel_cdclk_state actual;
>> +
>> +            int force_min_cdclk;
>> +            bool force_min_cdclk_changed;
>>      } cdclk;
>>   
>>      bool dpll_set, modeset;
>
> Some concerns about hard coded 96Mhz BCLK. however, that is the only 
> frequency we currently support.
>
> Reviewed-by: Clint Taylor <clinton.a.tay...@intel.com>
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to