diff mbox

drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled

Message ID 1525034353-4675-1-git-send-email-abhay.kumar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kumar, Abhay April 29, 2018, 8:39 p.m. UTC
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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
 drivers/gpu/drm/i915/intel_display.c |  7 +++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 5 files changed, 63 insertions(+), 27 deletions(-)

Comments

Du, Wenkai April 30, 2018, 6:22 p.m. UTC | #1
On 4/29/2018 1:39 PM, Abhay Kumar wrote:
> 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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
Tested-by: Wenkai Du <wenkai.du@intel.com>

Thanks,
Wenkai
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>   drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>   drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>   5 files changed, 63 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 193176bcddf5..34c31ef0761e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1708,6 +1708,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;
> 
>          /**
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3ea566f99450..f001fcf05d3a 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder,
>          I915_WRITE(aud_config, tmp);
>   }
> 
> +
>   /**
>    * intel_audio_codec_enable - Enable the audio codec for HD audio
>    * @encoder: encoder on which to enable audio
> @@ -713,6 +714,48 @@ 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)->modeset = true;
> +       to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> +               enable ? 2 * 96000 : 0;
> +
> +       /*
> +        * 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 void i915_audio_component_get_power(struct device *kdev)
>   {
>          intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> @@ -732,6 +775,9 @@ static void i915_audio_component_codec_wake_override(struct device *kdev,
>          if (!IS_GEN9(dev_priv))
>                  return;
> 
> +       if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +               glk_force_audio_cdclk(dev_priv, true);
> +
>          i915_audio_component_get_power(kdev);
> 
>          /*
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index ebca83a44d9b..4086730018f9 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2141,24 +2141,6 @@ 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)
> -               min_cdclk = max(2 * 96000, min_cdclk);
> -
> -       /*
>           * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>           * than 320000KHz.
>           */
> @@ -2195,7 +2177,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);
> 
> @@ -2256,7 +2238,7 @@ 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 =
> @@ -2289,7 +2271,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 =
> @@ -2328,7 +2310,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;
> @@ -2367,10 +2349,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);
>                  }
> 
> @@ -2406,7 +2388,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;
> @@ -2439,7 +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>          intel_state->cdclk.logical.cdclk = cdclk;
> 
>          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 84ce66be88f2..7c369e15f193 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>                  return -EINVAL;
>          }
> 
> +       /* keep the current setting */
> +       if (!intel_state->modeset)
> +               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;
> @@ -12118,7 +12122,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->modeset;
> 
>          /* Catch I915_MODE_FLAG_INHERITED */
>          for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> @@ -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>                   * state only when all crtc's are DPMS off.
>                   */
>                  struct intel_cdclk_state actual;
> +               int force_min_cdclk;
>          } cdclk;
> 
>          bool dpll_set, modeset;
> --
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Saarinen, Jani May 1, 2018, 9:15 a.m. UTC | #2
HI, 
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> Du,Wenkai

> Sent: maanantai 30. huhtikuuta 2018 21.23

> To: Kumar, Abhay <abhay.kumar@intel.com>; intel-gfx@lists.freedesktop.org

> Cc: Nikula, Jani <jani.nikula@intel.com>

> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Force 2*96 MHz cdclk on glk/cnl when

> audio power is enabled

> 

> 

> 

> On 4/29/2018 1:39 PM, Abhay Kumar wrote:

> > 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.

> >

> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>

> Tested-by: Wenkai Du <wenkai.du@intel.com>

Please note that failed on CI/GLK: https://patchwork.freedesktop.org/series/42459/
    ==== Possible regressions ====
    igt@drv_module_reload@basic-reload:
      shard-glk:          PASS -> INCOMPLETE

Br,
Jani
> 

> Thanks,

> Wenkai

> > ---

> >   drivers/gpu/drm/i915/i915_drv.h      |  2 ++

> >   drivers/gpu/drm/i915/intel_audio.c   | 46

> ++++++++++++++++++++++++++++++++++++

> >   drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------

> >   drivers/gpu/drm/i915/intel_display.c |  7 +++++-

> >   drivers/gpu/drm/i915/intel_drv.h     |  1 +

> >   5 files changed, 63 insertions(+), 27 deletions(-)

> >

> > diff --git a/drivers/gpu/drm/i915/i915_drv.h

> > b/drivers/gpu/drm/i915/i915_drv.h index 193176bcddf5..34c31ef0761e

> > 100644

> > --- a/drivers/gpu/drm/i915/i915_drv.h

> > +++ b/drivers/gpu/drm/i915/i915_drv.h

> > @@ -1708,6 +1708,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;

> >

> >          /**

> > diff --git a/drivers/gpu/drm/i915/intel_audio.c

> > b/drivers/gpu/drm/i915/intel_audio.c

> > index 3ea566f99450..f001fcf05d3a 100644

> > --- a/drivers/gpu/drm/i915/intel_audio.c

> > +++ b/drivers/gpu/drm/i915/intel_audio.c

> > @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct

> intel_encoder *encoder,

> >          I915_WRITE(aud_config, tmp);

> >   }

> >

> > +

> >   /**

> >    * intel_audio_codec_enable - Enable the audio codec for HD audio

> >    * @encoder: encoder on which to enable audio @@ -713,6 +714,48 @@

> > 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)->modeset = true;

> > +       to_intel_atomic_state(state)->cdclk.force_min_cdclk =

> > +               enable ? 2 * 96000 : 0;

> > +

> > +       /*

> > +        * 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 void i915_audio_component_get_power(struct device *kdev)

> >   {

> >          intel_display_power_get(kdev_to_i915(kdev),

> > POWER_DOMAIN_AUDIO); @@ -732,6 +775,9 @@ static void

> i915_audio_component_codec_wake_override(struct device *kdev,

> >          if (!IS_GEN9(dev_priv))

> >                  return;

> >

> > +       if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))

> > +               glk_force_audio_cdclk(dev_priv, true);

> > +

> >          i915_audio_component_get_power(kdev);

> >

> >          /*

> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c

> > b/drivers/gpu/drm/i915/intel_cdclk.c

> > index ebca83a44d9b..4086730018f9 100644

> > --- a/drivers/gpu/drm/i915/intel_cdclk.c

> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c

> > @@ -2141,24 +2141,6 @@ 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)

> > -               min_cdclk = max(2 * 96000, min_cdclk);

> > -

> > -       /*

> >           * On Valleyview some DSI panels lose (v|h)sync when the clock is lower

> >           * than 320000KHz.

> >           */

> > @@ -2195,7 +2177,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);

> >

> > @@ -2256,7 +2238,7 @@ 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 = @@ -2289,7

> > +2271,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 = @@ -2328,7

> > +2310,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; @@ -2367,10

> > +2349,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);

> >                  }

> >

> > @@ -2406,7 +2388,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; @@ -2439,7

> > +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state

> *state)

> >          intel_state->cdclk.logical.cdclk = cdclk;

> >

> >          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 84ce66be88f2..7c369e15f193 100644

> > --- a/drivers/gpu/drm/i915/intel_display.c

> > +++ b/drivers/gpu/drm/i915/intel_display.c

> > @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct

> drm_atomic_state *state)

> >                  return -EINVAL;

> >          }

> >

> > +       /* keep the current setting */

> > +       if (!intel_state->modeset)

> > +               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; @@

> > -12118,7 +12122,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->modeset;

> >

> >          /* Catch I915_MODE_FLAG_INHERITED */

> >          for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@

> > -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644

> > --- a/drivers/gpu/drm/i915/intel_drv.h

> > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > @@ -461,6 +461,7 @@ struct intel_atomic_state {

> >                   * state only when all crtc's are DPMS off.

> >                   */

> >                  struct intel_cdclk_state actual;

> > +               int force_min_cdclk;

> >          } cdclk;

> >

> >          bool dpll_set, modeset;

> > --

> > 2.7.4

> >

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

> >

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kumar, Abhay May 1, 2018, 11:42 p.m. UTC | #3
On 5/1/2018 2:15 AM, Saarinen, Jani wrote:
> HI,
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>> Du,Wenkai
>> Sent: maanantai 30. huhtikuuta 2018 21.23
>> To: Kumar, Abhay <abhay.kumar@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Nikula, Jani <jani.nikula@intel.com>
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Force 2*96 MHz cdclk on glk/cnl when
>> audio power is enabled
>>
>>
>>
>> On 4/29/2018 1:39 PM, Abhay Kumar wrote:
>>> 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.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
>> Tested-by: Wenkai Du <wenkai.du@intel.com>
> Please note that failed on CI/GLK: https://patchwork.freedesktop.org/series/42459/
>      ==== Possible regressions ====
>      igt@drv_module_reload@basic-reload:
>        shard-glk:          PASS -> INCOMPLETE
>
> Br,
> Jani
Hi Jani,

   Saw panic 
@https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8838/shard-glk8/pstore11-1525091313_Panic_3.log 
and  was trying to reproduce this on my end with IGT but it passed for 
me and never failed with DINQ, drm-tip both.

with or without external monitor connected it never fails.

ocalhost /usr/libexec/intel-gpu-tools # ./drv_module_reload
IGT-Version: 1.20-NOT-GIT (x86_64) (Linux: 
4.17.0-rc3-g844dd95837ab-dirty x86_64)
Subtest basic-reload: SUCCESS (0.212s)
Reloading i915 with disable_display=1

Subtest basic-no-display: SUCCESS (0.054s)
Reloading i915 with inject_load_failure=0

Reloading i915 with inject_load_failure=1

Reloading i915 with inject_load_failure=2

Reloading i915 with inject_load_failure=3

Subtest basic-reload-inject: SUCCESS (0.329s)
localhost /usr/libexec/intel-gpu-tools # uname -r
4.17.0-rc3-g844dd95837ab-dirty
localhost /usr/libexec/intel-gpu-tools # aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: PCH [HDA Intel PCH], device 3: HDMI 0 [HDMI 0]
   Subdevices: 1/1
   Subdevice #0: subdevice #0
card 0: PCH [HDA Intel PCH], device 7: HDMI 1 [HDMI 1]
   Subdevices: 1/1
   Subdevice #0: subdevice #0
card 0: PCH [HDA Intel PCH], device 8: HDMI 2 [HDMI 2]
   Subdevices: 1/1
   Subdevice #0: subdevice #0
card 0: PCH [HDA Intel PCH], device 9: HDMI 3 [HDMI 3]
   Subdevices: 1/1
   Subdevice #0: subdevice #0
card 0: PCH [HDA Intel PCH], device 10: HDMI 4 [HDMI 4]
   Subdevices: 1/1
   Subdevice #0: subdevice #0
localhost /usr/libexec/intel-gpu-tools #

Regards,
Abhay


>> Thanks,
>> Wenkai
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>>    drivers/gpu/drm/i915/intel_audio.c   | 46
>> ++++++++++++++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>>>    drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>>>    drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>    5 files changed, 63 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h index 193176bcddf5..34c31ef0761e
>>> 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1708,6 +1708,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;
>>>
>>>           /**
>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>>> b/drivers/gpu/drm/i915/intel_audio.c
>>> index 3ea566f99450..f001fcf05d3a 100644
>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct
>> intel_encoder *encoder,
>>>           I915_WRITE(aud_config, tmp);
>>>    }
>>>
>>> +
>>>    /**
>>>     * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>     * @encoder: encoder on which to enable audio @@ -713,6 +714,48 @@
>>> 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)->modeset = true;
>>> +       to_intel_atomic_state(state)->cdclk.force_min_cdclk =
>>> +               enable ? 2 * 96000 : 0;
>>> +
>>> +       /*
>>> +        * 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 void i915_audio_component_get_power(struct device *kdev)
>>>    {
>>>           intel_display_power_get(kdev_to_i915(kdev),
>>> POWER_DOMAIN_AUDIO); @@ -732,6 +775,9 @@ static void
>> i915_audio_component_codec_wake_override(struct device *kdev,
>>>           if (!IS_GEN9(dev_priv))
>>>                   return;
>>>
>>> +       if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>> +               glk_force_audio_cdclk(dev_priv, true);
>>> +
>>>           i915_audio_component_get_power(kdev);
>>>
>>>           /*
>>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
>>> b/drivers/gpu/drm/i915/intel_cdclk.c
>>> index ebca83a44d9b..4086730018f9 100644
>>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>>> @@ -2141,24 +2141,6 @@ 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)
>>> -               min_cdclk = max(2 * 96000, min_cdclk);
>>> -
>>> -       /*
>>>            * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>>>            * than 320000KHz.
>>>            */
>>> @@ -2195,7 +2177,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);
>>>
>>> @@ -2256,7 +2238,7 @@ 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 = @@ -2289,7
>>> +2271,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 = @@ -2328,7
>>> +2310,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; @@ -2367,10
>>> +2349,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);
>>>                   }
>>>
>>> @@ -2406,7 +2388,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; @@ -2439,7
>>> +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state
>> *state)
>>>           intel_state->cdclk.logical.cdclk = cdclk;
>>>
>>>           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 84ce66be88f2..7c369e15f193 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct
>> drm_atomic_state *state)
>>>                   return -EINVAL;
>>>           }
>>>
>>> +       /* keep the current setting */
>>> +       if (!intel_state->modeset)
>>> +               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; @@
>>> -12118,7 +12122,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->modeset;
>>>
>>>           /* Catch I915_MODE_FLAG_INHERITED */
>>>           for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@
>>> -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>>>                    * state only when all crtc's are DPMS off.
>>>                    */
>>>                   struct intel_cdclk_state actual;
>>> +               int force_min_cdclk;
>>>           } cdclk;
>>>
>>>           bool dpll_set, modeset;
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kumar, Abhay May 1, 2018, 11:47 p.m. UTC | #4
+ Ville


On 5/1/2018 4:42 PM, Kumar, Abhay wrote:
>
>
>
> On 5/1/2018 2:15 AM, Saarinen, Jani wrote:
>> HI,
>>> -----Original Message-----
>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>>> Du,Wenkai
>>> Sent: maanantai 30. huhtikuuta 2018 21.23
>>> To: Kumar, Abhay<abhay.kumar@intel.com>;intel-gfx@lists.freedesktop.org
>>> Cc: Nikula, Jani<jani.nikula@intel.com>
>>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Force 2*96 MHz cdclk on glk/cnl when
>>> audio power is enabled
>>>
>>>
>>>
>>> On 4/29/2018 1:39 PM, Abhay Kumar wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Ville Syrjälä<ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Abhay Kumar<abhay.kumar@intel.com>
>>> Tested-by: Wenkai Du<wenkai.du@intel.com>
>> Please note that failed on CI/GLK:https://patchwork.freedesktop.org/series/42459/
>>      ==== Possible regressions ====
>>      igt@drv_module_reload@basic-reload:
>>        shard-glk:          PASS -> INCOMPLETE
>>
>> Br,
>> Jani
> Hi Jani,
>
>   Saw panic 
> @https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8838/shard-glk8/pstore11-1525091313_Panic_3.log 
> and  was trying to reproduce this on my end with IGT but it passed for 
> me and never failed with DINQ, drm-tip both.
>
> with or without external monitor connected it never fails.
>
> ocalhost /usr/libexec/intel-gpu-tools # ./drv_module_reload
> IGT-Version: 1.20-NOT-GIT (x86_64) (Linux: 
> 4.17.0-rc3-g844dd95837ab-dirty x86_64)
> Subtest basic-reload: SUCCESS (0.212s)
> Reloading i915 with disable_display=1
>
> Subtest basic-no-display: SUCCESS (0.054s)
> Reloading i915 with inject_load_failure=0
>
> Reloading i915 with inject_load_failure=1
>
> Reloading i915 with inject_load_failure=2
>
> Reloading i915 with inject_load_failure=3
>
> Subtest basic-reload-inject: SUCCESS (0.329s)
> localhost /usr/libexec/intel-gpu-tools # uname -r
> 4.17.0-rc3-g844dd95837ab-dirty
> localhost /usr/libexec/intel-gpu-tools # aplay -l
> **** List of PLAYBACK Hardware Devices ****
> card 0: PCH [HDA Intel PCH], device 3: HDMI 0 [HDMI 0]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 0: PCH [HDA Intel PCH], device 7: HDMI 1 [HDMI 1]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 0: PCH [HDA Intel PCH], device 8: HDMI 2 [HDMI 2]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 0: PCH [HDA Intel PCH], device 9: HDMI 3 [HDMI 3]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 0: PCH [HDA Intel PCH], device 10: HDMI 4 [HDMI 4]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> localhost /usr/libexec/intel-gpu-tools #
>
> Regards,
> Abhay
>
>
>>> Thanks,
>>> Wenkai
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>>>    drivers/gpu/drm/i915/intel_audio.c   | 46
>>> ++++++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>>>>    drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>>>>    drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>>    5 files changed, 63 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h index 193176bcddf5..34c31ef0761e
>>>> 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1708,6 +1708,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;
>>>>
>>>>           /**
>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>>>> b/drivers/gpu/drm/i915/intel_audio.c
>>>> index 3ea566f99450..f001fcf05d3a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct
>>> intel_encoder *encoder,
>>>>           I915_WRITE(aud_config, tmp);
>>>>    }
>>>>
>>>> +
>>>>    /**
>>>>     * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>>     * @encoder: encoder on which to enable audio @@ -713,6 +714,48 @@
>>>> 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)->modeset = true;
>>>> +       to_intel_atomic_state(state)->cdclk.force_min_cdclk =
>>>> +               enable ? 2 * 96000 : 0;
>>>> +
>>>> +       /*
>>>> +        * 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 void i915_audio_component_get_power(struct device *kdev)
>>>>    {
>>>>           intel_display_power_get(kdev_to_i915(kdev),
>>>> POWER_DOMAIN_AUDIO); @@ -732,6 +775,9 @@ static void
>>> i915_audio_component_codec_wake_override(struct device *kdev,
>>>>           if (!IS_GEN9(dev_priv))
>>>>                   return;
>>>>
>>>> +       if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>>> +               glk_force_audio_cdclk(dev_priv, true);
>>>> +
>>>>           i915_audio_component_get_power(kdev);
>>>>
>>>>           /*
>>>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
>>>> b/drivers/gpu/drm/i915/intel_cdclk.c
>>>> index ebca83a44d9b..4086730018f9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>>>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>>>> @@ -2141,24 +2141,6 @@ 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)
>>>> -               min_cdclk = max(2 * 96000, min_cdclk);
>>>> -
>>>> -       /*
>>>>            * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>>>>            * than 320000KHz.
>>>>            */
>>>> @@ -2195,7 +2177,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);
>>>>
>>>> @@ -2256,7 +2238,7 @@ 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 = @@ -2289,7
>>>> +2271,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 = @@ -2328,7
>>>> +2310,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; @@ -2367,10
>>>> +2349,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);
>>>>                   }
>>>>
>>>> @@ -2406,7 +2388,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; @@ -2439,7
>>>> +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state
>>> *state)
>>>>           intel_state->cdclk.logical.cdclk = cdclk;
>>>>
>>>>           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 84ce66be88f2..7c369e15f193 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct
>>> drm_atomic_state *state)
>>>>                   return -EINVAL;
>>>>           }
>>>>
>>>> +       /* keep the current setting */
>>>> +       if (!intel_state->modeset)
>>>> +               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; @@
>>>> -12118,7 +12122,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->modeset;
>>>>
>>>>           /* Catch I915_MODE_FLAG_INHERITED */
>>>>           for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@
>>>> -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>>>>                    * state only when all crtc's are DPMS off.
>>>>                    */
>>>>                   struct intel_cdclk_state actual;
>>>> +               int force_min_cdclk;
>>>>           } cdclk;
>>>>
>>>>           bool dpll_set, modeset;
>>>> --
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Ville Syrjala May 2, 2018, 3:12 p.m. UTC | #5
On Sun, Apr 29, 2018 at 01:39:13PM -0700, Abhay Kumar wrote:

From: me

mostly

> 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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>  drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  5 files changed, 63 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 193176bcddf5..34c31ef0761e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1708,6 +1708,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;
>  
>  	/**
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3ea566f99450..f001fcf05d3a 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder,
>  	I915_WRITE(aud_config, tmp);
>  }
>  
> +
>  /**
>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>   * @encoder: encoder on which to enable audio
> @@ -713,6 +714,48 @@ 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)->modeset = true;
> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> +		enable ? 2 * 96000 : 0;
> +
> +	/*
> +	 * 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 void i915_audio_component_get_power(struct device *kdev)
>  {
>  	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> @@ -732,6 +775,9 @@ static void i915_audio_component_codec_wake_override(struct device *kdev,
>  	if (!IS_GEN9(dev_priv))
>  		return;
>  
> +	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +		glk_force_audio_cdclk(dev_priv, true);
> +

Where did the put_power counterpart go?

>  	i915_audio_component_get_power(kdev);
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index ebca83a44d9b..4086730018f9 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2141,24 +2141,6 @@ 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)
> -		min_cdclk = max(2 * 96000, min_cdclk);

I suspect we just want to revert the commit that made this uncoditional.
Otherwise the user may get a display blink every time audio playback is
started/stopped.

> -
> -	/*
>  	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>  	 * than 320000KHz.
>  	 */
> @@ -2195,7 +2177,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);
>  
> @@ -2256,7 +2238,7 @@ 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 =
> @@ -2289,7 +2271,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 =
> @@ -2328,7 +2310,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;
> @@ -2367,10 +2349,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);
>  		}
>  
> @@ -2406,7 +2388,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;
> @@ -2439,7 +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	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 84ce66be88f2..7c369e15f193 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  		return -EINVAL;
>  	}
>  
> +	/* keep the current setting */
> +	if (!intel_state->modeset)
> +		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;
> @@ -12118,7 +12122,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->modeset;
>  
>  	/* Catch I915_MODE_FLAG_INHERITED */
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> @@ -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>  		 * state only when all crtc's are DPMS off.
>  		 */
>  		struct intel_cdclk_state actual;
> +		int force_min_cdclk;
>  	} cdclk;
>  
>  	bool dpll_set, modeset;
> -- 
> 2.7.4
Kumar, Abhay May 2, 2018, 4:57 p.m. UTC | #6
On 5/2/2018 8:12 AM, Ville Syrjälä wrote:
> On Sun, Apr 29, 2018 at 01:39:13PM -0700, Abhay Kumar wrote:
>
> From: me
>
> mostly
>
>> 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.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>   drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>>   drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   5 files changed, 63 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 193176bcddf5..34c31ef0761e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1708,6 +1708,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;
>>   
>>   	/**
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>> index 3ea566f99450..f001fcf05d3a 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder,
>>   	I915_WRITE(aud_config, tmp);
>>   }
>>   
>> +
>>   /**
>>    * intel_audio_codec_enable - Enable the audio codec for HD audio
>>    * @encoder: encoder on which to enable audio
>> @@ -713,6 +714,48 @@ 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)->modeset = true;
>> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
>> +		enable ? 2 * 96000 : 0;
>> +
>> +	/*
>> +	 * 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 void i915_audio_component_get_power(struct device *kdev)
>>   {
>>   	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
>> @@ -732,6 +775,9 @@ static void i915_audio_component_codec_wake_override(struct device *kdev,
>>   	if (!IS_GEN9(dev_priv))
>>   		return;
>>   
>> +	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>> +		glk_force_audio_cdclk(dev_priv, true);
>> +
> Where did the put_power counterpart go?
with put_power counterpart the cdclk again goes back to low and then HDA 
doesn't get detected.  that's why i just kept the bump up.

>
>>   	i915_audio_component_get_power(kdev);
>>   
>>   	/*
>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>> index ebca83a44d9b..4086730018f9 100644
>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>> @@ -2141,24 +2141,6 @@ 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)
>> -		min_cdclk = max(2 * 96000, min_cdclk);
> I suspect we just want to revert the commit that made this uncoditional.
> Otherwise the user may get a display blink every time audio playback is
> started/stopped.
yeah. we should keep it. I removed this thought of redundant code. but 
will keep in next patchset and just revert recent patch.
>
>> -
>> -	/*
>>   	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>>   	 * than 320000KHz.
>>   	 */
>> @@ -2195,7 +2177,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);
>>   
>> @@ -2256,7 +2238,7 @@ 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 =
>> @@ -2289,7 +2271,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 =
>> @@ -2328,7 +2310,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;
>> @@ -2367,10 +2349,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);
>>   		}
>>   
>> @@ -2406,7 +2388,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;
>> @@ -2439,7 +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>>   	intel_state->cdclk.logical.cdclk = cdclk;
>>   
>>   	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 84ce66be88f2..7c369e15f193 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* keep the current setting */
>> +	if (!intel_state->modeset)
>> +		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;
>> @@ -12118,7 +12122,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->modeset;
>>   
>>   	/* Catch I915_MODE_FLAG_INHERITED */
>>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>> @@ -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>>   		 * state only when all crtc's are DPMS off.
>>   		 */
>>   		struct intel_cdclk_state actual;
>> +		int force_min_cdclk;
>>   	} cdclk;
>>   
>>   	bool dpll_set, modeset;
>> -- 
>> 2.7.4
Ville Syrjala May 2, 2018, 5:14 p.m. UTC | #7
On Wed, May 02, 2018 at 09:57:01AM -0700, Kumar, Abhay wrote:
> 
> 
> On 5/2/2018 8:12 AM, Ville Syrjälä wrote:
> > On Sun, Apr 29, 2018 at 01:39:13PM -0700, Abhay Kumar wrote:
> >
> > From: me
> >
> > mostly
> >
> >> 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.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >>   drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
> >>   drivers/gpu/drm/i915/intel_display.c |  7 +++++-
> >>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>   5 files changed, 63 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 193176bcddf5..34c31ef0761e 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1708,6 +1708,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;
> >>   
> >>   	/**
> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> >> index 3ea566f99450..f001fcf05d3a 100644
> >> --- a/drivers/gpu/drm/i915/intel_audio.c
> >> +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder,
> >>   	I915_WRITE(aud_config, tmp);
> >>   }
> >>   
> >> +
> >>   /**
> >>    * intel_audio_codec_enable - Enable the audio codec for HD audio
> >>    * @encoder: encoder on which to enable audio
> >> @@ -713,6 +714,48 @@ 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)->modeset = true;
> >> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> >> +		enable ? 2 * 96000 : 0;
> >> +
> >> +	/*
> >> +	 * 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 void i915_audio_component_get_power(struct device *kdev)
> >>   {
> >>   	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> >> @@ -732,6 +775,9 @@ static void i915_audio_component_codec_wake_override(struct device *kdev,
> >>   	if (!IS_GEN9(dev_priv))
> >>   		return;
> >>   
> >> +	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> >> +		glk_force_audio_cdclk(dev_priv, true);
> >> +
> > Where did the put_power counterpart go?
> with put_power counterpart the cdclk again goes back to low and then HDA 
> doesn't get detected.  that's why i just kept the bump up.

Then fix hda to grab the power when it needs it?

Otherwise we permanently lock the cdclk to >=2*96 MHz. Ie. it would
be no different to what we have now, except a lot more complex.

> 
> >
> >>   	i915_audio_component_get_power(kdev);
> >>   
> >>   	/*
> >> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> >> index ebca83a44d9b..4086730018f9 100644
> >> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> >> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> >> @@ -2141,24 +2141,6 @@ 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)
> >> -		min_cdclk = max(2 * 96000, min_cdclk);
> > I suspect we just want to revert the commit that made this uncoditional.
> > Otherwise the user may get a display blink every time audio playback is
> > started/stopped.
> yeah. we should keep it. I removed this thought of redundant code. but 
> will keep in next patchset and just revert recent patch.
> >
> >> -
> >> -	/*
> >>   	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
> >>   	 * than 320000KHz.
> >>   	 */
> >> @@ -2195,7 +2177,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);
> >>   
> >> @@ -2256,7 +2238,7 @@ 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 =
> >> @@ -2289,7 +2271,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 =
> >> @@ -2328,7 +2310,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;
> >> @@ -2367,10 +2349,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);
> >>   		}
> >>   
> >> @@ -2406,7 +2388,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;
> >> @@ -2439,7 +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >>   	intel_state->cdclk.logical.cdclk = cdclk;
> >>   
> >>   	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 84ce66be88f2..7c369e15f193 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	/* keep the current setting */
> >> +	if (!intel_state->modeset)
> >> +		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;
> >> @@ -12118,7 +12122,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->modeset;
> >>   
> >>   	/* Catch I915_MODE_FLAG_INHERITED */
> >>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> >> @@ -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -461,6 +461,7 @@ struct intel_atomic_state {
> >>   		 * state only when all crtc's are DPMS off.
> >>   		 */
> >>   		struct intel_cdclk_state actual;
> >> +		int force_min_cdclk;
> >>   	} cdclk;
> >>   
> >>   	bool dpll_set, modeset;
> >> -- 
> >> 2.7.4
Kumar, Abhay May 2, 2018, 6:47 p.m. UTC | #8
On 5/2/2018 10:14 AM, Ville Syrjälä wrote:
> On Wed, May 02, 2018 at 09:57:01AM -0700, Kumar, Abhay wrote:
>>
>> On 5/2/2018 8:12 AM, Ville Syrjälä wrote:
>>> On Sun, Apr 29, 2018 at 01:39:13PM -0700, Abhay Kumar wrote:
>>>
>>> From: me
>>>
>>> mostly
>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>>>    drivers/gpu/drm/i915/intel_audio.c   | 46 ++++++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++-------------------
>>>>    drivers/gpu/drm/i915/intel_display.c |  7 +++++-
>>>>    drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>>    5 files changed, 63 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 193176bcddf5..34c31ef0761e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1708,6 +1708,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;
>>>>    
>>>>    	/**
>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>>> index 3ea566f99450..f001fcf05d3a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>> @@ -594,6 +594,7 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder,
>>>>    	I915_WRITE(aud_config, tmp);
>>>>    }
>>>>    
>>>> +
>>>>    /**
>>>>     * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>>     * @encoder: encoder on which to enable audio
>>>> @@ -713,6 +714,48 @@ 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)->modeset = true;
>>>> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
>>>> +		enable ? 2 * 96000 : 0;
>>>> +
>>>> +	/*
>>>> +	 * 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 void i915_audio_component_get_power(struct device *kdev)
>>>>    {
>>>>    	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
>>>> @@ -732,6 +775,9 @@ static void i915_audio_component_codec_wake_override(struct device *kdev,
>>>>    	if (!IS_GEN9(dev_priv))
>>>>    		return;
>>>>    
>>>> +	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>>> +		glk_force_audio_cdclk(dev_priv, true);
>>>> +
>>> Where did the put_power counterpart go?
>> with put_power counterpart the cdclk again goes back to low and then HDA
>> doesn't get detected.  that's why i just kept the bump up.
> Then fix hda to grab the power when it needs it?
I am afraid that this leads lot more changes as everywhere where there 
is a probe we need to bump the clock.
>
> Otherwise we permanently lock the cdclk to >=2*96 MHz. Ie. it would
> be no different to what we have now, except a lot more complex.
It is different specially in resume/s0ix path. with the one we have 
right now during resume time the cdclk is still 79.2 and
thus either the hda doesn't comeup or we have 4sec delay in coming up as 
it loops and wait for hda verb command response.
This patch makes sure that all the time at probe we have right cdclk. 
one more thing. we did probe power numbers and there was
not much significant delta.
>
>>>>    	i915_audio_component_get_power(kdev);
>>>>    
>>>>    	/*
>>>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>>>> index ebca83a44d9b..4086730018f9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>>>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>>>> @@ -2141,24 +2141,6 @@ 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)
>>>> -		min_cdclk = max(2 * 96000, min_cdclk);
>>> I suspect we just want to revert the commit that made this uncoditional.
>>> Otherwise the user may get a display blink every time audio playback is
>>> started/stopped.
>> yeah. we should keep it. I removed this thought of redundant code. but
>> will keep in next patchset and just revert recent patch.
>>>> -
>>>> -	/*
>>>>    	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>>>>    	 * than 320000KHz.
>>>>    	 */
>>>> @@ -2195,7 +2177,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);
>>>>    
>>>> @@ -2256,7 +2238,7 @@ 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 =
>>>> @@ -2289,7 +2271,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 =
>>>> @@ -2328,7 +2310,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;
>>>> @@ -2367,10 +2349,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);
>>>>    		}
>>>>    
>>>> @@ -2406,7 +2388,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;
>>>> @@ -2439,7 +2421,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>>>>    	intel_state->cdclk.logical.cdclk = cdclk;
>>>>    
>>>>    	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 84ce66be88f2..7c369e15f193 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12023,6 +12023,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>>>    		return -EINVAL;
>>>>    	}
>>>>    
>>>> +	/* keep the current setting */
>>>> +	if (!intel_state->modeset)
>>>> +		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;
>>>> @@ -12118,7 +12122,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->modeset;
>>>>    
>>>>    	/* Catch I915_MODE_FLAG_INHERITED */
>>>>    	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>>>> @@ -12666,6 +12670,7 @@ 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 11a1932cde6e..79928505d0d0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -461,6 +461,7 @@ struct intel_atomic_state {
>>>>    		 * state only when all crtc's are DPMS off.
>>>>    		 */
>>>>    		struct intel_cdclk_state actual;
>>>> +		int force_min_cdclk;
>>>>    	} cdclk;
>>>>    
>>>>    	bool dpll_set, modeset;
>>>> -- 
>>>> 2.7.4
Saarinen, Jani June 12, 2018, 6:09 p.m. UTC | #9
HI, 

> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Patchwork

> Sent: tiistai 12. kesäkuuta 2018 11.38

> To: Kumar, Abhay <abhay.kumar@intel.com>

> Cc: intel-gfx@lists.freedesktop.org

> Subject: [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Force 2*96 MHz cdclk on

> glk/cnl when audio power is enabled (rev3)

> 

> == Series Details ==

> 

> Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is

> enabled (rev3)

> URL   : https://patchwork.freedesktop.org/series/42459/

> State : success

> 

> == Summary ==

> 

> = CI Bug Log - changes from CI_DRM_4304_full -> Patchwork_9268_full =

> 

> == Summary - WARNING ==

> 

>   Minor unknown changes coming with Patchwork_9268_full need to be

> verified

>   manually.

> 

>   If you think the reported changes have nothing to do with the changes

>   introduced in Patchwork_9268_full, please notify your bug team to allow

> them

>   to document this new failure mode, which will reduce false positives in CI.

> 

> 

> 

> == Possible new issues ==

> 

>   Here are the unknown changes that may have been introduced in

> Patchwork_9268_full:

> 

>   === IGT changes ===

> 

>     ==== Warnings ====

> 

>     igt@gem_exec_schedule@deep-bsd2:

>       shard-kbl:          PASS -> SKIP

> 

>     igt@gem_exec_schedule@deep-vebox:

>       shard-kbl:          SKIP -> PASS

> 

> 

> == Known issues ==

> 

>   Here are the changes found in Patchwork_9268_full that come from known

> issues:

> 

>   === IGT changes ===

> 

>     ==== Issues hit ====

> 

>     igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:

>       shard-hsw:          PASS -> FAIL (fdo#102887)

> 

>     igt@kms_setmode@basic:

>       shard-kbl:          PASS -> FAIL (fdo#99912)

> 

> 

>     ==== Possible fixes ====

> 

>     igt@drv_selftest@live_gtt:

>       shard-kbl:          FAIL (fdo#105347) -> PASS

> 

>     igt@drv_suspend@shrink:

>       shard-hsw:          INCOMPLETE (fdo#103540) -> PASS

> 

>     igt@kms_rotation_crc@sprite-rotation-180:

>       shard-hsw:          FAIL (fdo#103925, fdo#104724) -> PASS

> 

> 

>   fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887

>   fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540

>   fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925

>   fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724

>   fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347

>   fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

> 

> 

> == Participating hosts (5 -> 4) ==

> 

>   Missing    (1): shard-glk

There glk's are but some issues:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9268/shard-glk1/
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9268/shard-glk2/run2.log
etc...

Worth to investigate...

+ Tomi too. 

> 

> 

> == Build changes ==

> 

>     * Linux: CI_DRM_4304 -> Patchwork_9268

> 

>   CI_DRM_4304: 2313a1dc588ef63d9650ccbaaf576bc4b47dc255 @

> git://anongit.freedesktop.org/gfx-ci/linux

>   IGT_4515: a0f2d23b7d3d4226a0a7637a9240bfa86f08c1d3 @

> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

>   Patchwork_9268: 7e66d7400ee9f80e00633e6cfdecc354dda8e049 @

> git://anongit.freedesktop.org/gfx-ci/linux

>   piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @

> git://anongit.freedesktop.org/piglit

> 

> == Logs ==

> 

> For more details see: https://intel-gfx-ci.01.org/tree/drm-

> tip/Patchwork_9268/shards.html

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kumar, Abhay June 12, 2018, 11:51 p.m. UTC | #10
On 6/12/2018 11:09 AM, Saarinen, Jani wrote:
> HI,
>
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Patchwork
>> Sent: tiistai 12. kesäkuuta 2018 11.38
>> To: Kumar, Abhay <abhay.kumar@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Subject: [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Force 2*96 MHz cdclk on
>> glk/cnl when audio power is enabled (rev3)
>>
>> == Series Details ==
>>
>> Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is
>> enabled (rev3)
>> URL   : https://patchwork.freedesktop.org/series/42459/
>> State : success
>>
>> == Summary ==
>>
>> = CI Bug Log - changes from CI_DRM_4304_full -> Patchwork_9268_full =
>>
>> == Summary - WARNING ==
>>
>>    Minor unknown changes coming with Patchwork_9268_full need to be
>> verified
>>    manually.
>>
>>    If you think the reported changes have nothing to do with the changes
>>    introduced in Patchwork_9268_full, please notify your bug team to allow
>> them
>>    to document this new failure mode, which will reduce false positives in CI.
>>
>>
>>
>> == Possible new issues ==
>>
>>    Here are the unknown changes that may have been introduced in
>> Patchwork_9268_full:
>>
>>    === IGT changes ===
>>
>>      ==== Warnings ====
>>
>>      igt@gem_exec_schedule@deep-bsd2:
>>        shard-kbl:          PASS -> SKIP
>>
>>      igt@gem_exec_schedule@deep-vebox:
>>        shard-kbl:          SKIP -> PASS
>>
>>
>> == Known issues ==
>>
>>    Here are the changes found in Patchwork_9268_full that come from known
>> issues:
>>
>>    === IGT changes ===
>>
>>      ==== Issues hit ====
>>
>>      igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
>>        shard-hsw:          PASS -> FAIL (fdo#102887)
>>
>>      igt@kms_setmode@basic:
>>        shard-kbl:          PASS -> FAIL (fdo#99912)
>>
>>
>>      ==== Possible fixes ====
>>
>>      igt@drv_selftest@live_gtt:
>>        shard-kbl:          FAIL (fdo#105347) -> PASS
>>
>>      igt@drv_suspend@shrink:
>>        shard-hsw:          INCOMPLETE (fdo#103540) -> PASS
>>
>>      igt@kms_rotation_crc@sprite-rotation-180:
>>        shard-hsw:          FAIL (fdo#103925, fdo#104724) -> PASS
>>
>>
>>    fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
>>    fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
>>    fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
>>    fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
>>    fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
>>    fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
>>
>>
>> == Participating hosts (5 -> 4) ==
>>
>>    Missing    (1): shard-glk
> There glk's are but some issues:
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9268/shard-glk1/
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9268/shard-glk2/run2.log
> etc...
>
> Worth to investigate...

May be this is due to turning off and on Power well 2. Sent another 
patchset where we restart power well2 for any cdclk change only for glk.
>
> + Tomi too.
>
>>
>> == Build changes ==
>>
>>      * Linux: CI_DRM_4304 -> Patchwork_9268
>>
>>    CI_DRM_4304: 2313a1dc588ef63d9650ccbaaf576bc4b47dc255 @
>> git://anongit.freedesktop.org/gfx-ci/linux
>>    IGT_4515: a0f2d23b7d3d4226a0a7637a9240bfa86f08c1d3 @
>> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>>    Patchwork_9268: 7e66d7400ee9f80e00633e6cfdecc354dda8e049 @
>> git://anongit.freedesktop.org/gfx-ci/linux
>>    piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @
>> git://anongit.freedesktop.org/piglit
>>
>> == Logs ==
>>
>> For more details see: https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_9268/shards.html
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 193176bcddf5..34c31ef0761e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1708,6 +1708,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;
 
 	/**
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 3ea566f99450..f001fcf05d3a 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -594,6 +594,7 @@  static void ilk_audio_codec_enable(struct intel_encoder *encoder,
 	I915_WRITE(aud_config, tmp);
 }
 
+
 /**
  * intel_audio_codec_enable - Enable the audio codec for HD audio
  * @encoder: encoder on which to enable audio
@@ -713,6 +714,48 @@  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)->modeset = true;
+	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
+		enable ? 2 * 96000 : 0;
+
+	/*
+	 * 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 void i915_audio_component_get_power(struct device *kdev)
 {
 	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
@@ -732,6 +775,9 @@  static void i915_audio_component_codec_wake_override(struct device *kdev,
 	if (!IS_GEN9(dev_priv))
 		return;
 
+	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+		glk_force_audio_cdclk(dev_priv, true);
+
 	i915_audio_component_get_power(kdev);
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index ebca83a44d9b..4086730018f9 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2141,24 +2141,6 @@  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)
-		min_cdclk = max(2 * 96000, min_cdclk);
-
-	/*
 	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
 	 * than 320000KHz.
 	 */
@@ -2195,7 +2177,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);
 
@@ -2256,7 +2238,7 @@  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 =
@@ -2289,7 +2271,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 =
@@ -2328,7 +2310,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;
@@ -2367,10 +2349,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);
 		}
 
@@ -2406,7 +2388,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;
@@ -2439,7 +2421,7 @@  static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	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 84ce66be88f2..7c369e15f193 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12023,6 +12023,10 @@  static int intel_modeset_checks(struct drm_atomic_state *state)
 		return -EINVAL;
 	}
 
+	/* keep the current setting */
+	if (!intel_state->modeset)
+		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;
@@ -12118,7 +12122,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->modeset;
 
 	/* Catch I915_MODE_FLAG_INHERITED */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
@@ -12666,6 +12670,7 @@  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 11a1932cde6e..79928505d0d0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -461,6 +461,7 @@  struct intel_atomic_state {
 		 * state only when all crtc's are DPMS off.
 		 */
 		struct intel_cdclk_state actual;
+		int force_min_cdclk;
 	} cdclk;
 
 	bool dpll_set, modeset;