Re: [PATCH v2] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 11 May 2018, "Kumar, Abhay" <abhay.kumar@intel.com> wrote:
> On 5/11/2018 5:33 AM, Ville Syrjälä wrote:
>> On Wed, May 09, 2018 at 06:25:32PM -0700, Abhay Kumar wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> CDCLK has to be at least twice the BLCK regardless of audio. Audio
>>> driver has to probe using this hook and increase the clock even in
>>> absence of any display.
>>>
>>> v2: Use atomic refcount for get_power, put_power so that we can
>>>      call each once(Abhay).
>>>
>>> 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      |  3 ++
>>>   drivers/gpu/drm/i915/intel_audio.c   | 66 +++++++++++++++++++++++++++++++++---
>>>   drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
>>>   drivers/gpu/drm/i915/intel_display.c |  7 +++-
>>>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>>>   5 files changed, 82 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 24c5e4765afd..9c4ea767688a 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1692,6 +1692,7 @@ struct drm_i915_private {
>>>   	unsigned int hpll_freq;
>>>   	unsigned int fdi_pll_freq;
>>>   	unsigned int czclk_freq;
>>> +	atomic_t get_put_refcount;
>>>   
>>>   	struct {
>>>   		/*
>>> @@ -1709,6 +1710,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..a1e2c4daae6e 100644
>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>> @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
>>>   
>>>   	if (!connector->eld[0])
>>>   		return;
>>> -
>>>   	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>>>   			 connector->base.id,
>>>   			 connector->name,
>>> @@ -713,14 +712,73 @@ 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);
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>>> +
>>> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
>>> +	atomic_inc(&dev_priv->get_put_refcount);
>>> +
>>> +	/* Force cdclk to 2*BCLK during first time get power call */
>>> +	if (atomic_read(&dev_priv->get_put_refcount) == 1)
>> If it needs to be atomic (ie. we have concurrent callers of get/put_power())
>> then you would also need to do the inc+check atomically. But that in itself
>> wouldn't help because only the first caller would do the cdclk change,
>> and the second call would just immediately proceed without waiting for the
>> cdclk to actually have been changed.
>>
>> So the first question we should ask is whether we can even have
>> concurrent callers, or if there's some form of mutual exclusion
>> already on the caller side?
> As per my understanding I don't think we have any concurrent callers as 
> these calls will be in sequence.
> Do you prefer to use static instead?

Static what?

BR,
Jani.

>>
>>> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>> +			glk_force_audio_cdclk(dev_priv, true);
>>>   }
>>>   
>>>   static void i915_audio_component_put_power(struct device *kdev)
>>>   {
>>> -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>>> +
>>> +	atomic_dec(&dev_priv->get_put_refcount);
>>> +
>>> +	/* Force required cdclk during last time put power call */
>>> +	if (atomic_read(&dev_priv->get_put_refcount) == 0)
>>> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>> +			glk_force_audio_cdclk(dev_priv, false);
>>> +
>>> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
>>>   }
>>>   
>>>   static void i915_audio_component_codec_wake_override(struct device *kdev,
>>> @@ -959,7 +1017,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
>>>   		/* continue with reduced functionality */
>>>   		return;
>>>   	}
>>> -
>>> +	atomic_set(&dev_priv->get_put_refcount, 0);
>>>   	dev_priv->audio_component_registered = true;
>>>   }
>>>   
>>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>>> index 704ddb4d3ca7..a0c281a90ff4 100644
>>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>>> @@ -2143,19 +2143,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>>>   	/*
>>>   	 * According to BSpec, "The CD clock frequency must be at least twice
>>>   	 * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
>>> -	 *
>>> -	 * FIXME: Check the actual, not default, BCLK being used.
>>> -	 *
>>> -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
>>> -	 * is required for audio probe, also when there are no audio capable
>>> -	 * displays connected at probe time. This leads to unnecessarily high
>>> -	 * CDCLK when audio is not required.
>>> -	 *
>>> -	 * FIXME: This limit is only applied when there are displays connected
>>> -	 * at probe time. If we probe without displays, we'll still end up using
>>> -	 * the platform minimum CDCLK, failing audio probe.
>>>   	 */
>>> -	if (INTEL_GEN(dev_priv) >= 9)
>>> +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>>>   		min_cdclk = max(2 * 96000, min_cdclk);
>>>   
>>>   	/*
>>> @@ -2195,7 +2184,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 +2245,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 +2278,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 =
>>> @@ -2361,7 +2350,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;
>>> @@ -2400,10 +2389,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);
>>>   		}
>>>   
>>> @@ -2439,7 +2428,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;
>>> @@ -2472,7 +2461,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 cdfe0951d171..0b0e519fe844 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12041,6 +12041,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;
>>> @@ -12136,7 +12140,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,
>>> @@ -12684,6 +12688,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 52337f487ebc..7b53f72d7242 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -461,6 +461,8 @@ 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
>

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