Re: [PATCH] drm/i915/audio: not to set N/M value manually for KBL.

From: "Wang, Quanxian" <quanxian.wang@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915/audio: not to set N/M value manually	for KBL.
Date: Wed, 5 Apr 2017 02:20:39 +0000	[thread overview]
Message-ID: <50C7F552BA2A744D84D95119645BEABD7D21CF79@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <87pogs1lz7.fsf@intel.com>

Sorry, I don't find them in bspec. With our testing, SKL/APL works and KBL failed.

The commit 6014ac122ed081feca99217bc57b2e15c7fc1a51 mentioned  should not do testing on KBL DP audio.

We found the issue in the testing for Ubuntu latest release 17.04. (Almost upstream)

But really I don't know the root cause.

Also I have mentioned in commit, for general process of DP audio in bspec, transcoder should be restart to make it.
 But no code to do that. Also I don't know why driver doesn't implement that.

-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
Sent: Tuesday, April 4, 2017 6:55 PM
To: Wang, Quanxian <quanxian.wang@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Yang, Libin <libin.yang@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/audio: not to set N/M value manually for KBL.

On Sat, 01 Apr 2017, Quanxian Wang <quanxian.wang@intel.com> wrote:
> It doesn't work on KBL. Just using automatic N/M.

Please provide a more elaborate commit message. In particular, have a look at

commit 6014ac122ed081feca99217bc57b2e15c7fc1a51
Author: Libin Yang <libin.yang@linux.intel.com>
Date:   Tue Oct 25 17:54:18 2016 +0300

    drm/i915/audio: set proper N/M in modeset

and explain why that doesn't apply to KBL. And why just KBL, why not others?

> According to the bspec, when set N/M, should disable and enable 
> transcoder which attaching DP audio. but there is no such code to do 
> that.
> without this implementation except KBL platforms, seems work well.
>
> Signed-off-by: Quanxian Wang <quanxian.wang@intel.com>
> Tested-By: Wang Zhijun<zhijunx.wang@intel.com>
> Tested-By: Cui Yueping<yuepingx.cui@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 52c207e..0542031 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -300,7 +300,7 @@ hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
>  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>  	tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  
> -	if (nm) {
> +	if (!IS_KABYLAKE(dev_priv) && nm) {
>  		tmp &= ~AUD_CONFIG_N_MASK;
>  		tmp |= AUD_CONFIG_N(nm->n);
>  		tmp |= AUD_CONFIG_N_PROG_ENABLE;
> @@ -308,6 +308,9 @@ hsw_dp_audio_config_update(struct intel_crtc 
> *intel_crtc, enum port port,
>  
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> +	if (IS_KABYLAKE(dev_priv))
> +		return;
> +

The debug logging near the top of the function will be wrong for KBL, and you now bypass HSW_AUD_M_CTS_ENABLE() setting for the automatic values.

Seems to me the simplest would be to do

	if (IS_KABYLAKE(dev_priv))
        	nm = NULL;

*but* this still needs better rationale, especially why this is specific to KBL.

BR,
Jani.

>  	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
>  	tmp &= ~AUD_CONFIG_M_MASK;
>  	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;

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