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

[Wang, Quanxian]

Hi, Jani

After testing on 4K monitor. It is fine to get audio with our patch on 
KBL platform. We have tested with 'speaker' command and "play mp3/mp4 directly", not 
found delay for DP audio.

Libin is fine with this patch.

Any comment, please let me know.

Thanks

Quanxian Wang


-----Original Message-----
From: Wang, Quanxian 
Sent: Wednesday, April 5, 2017 10:36 AM
To: Yang, Libin <libin.yang at intel.com>
Cc: Jani Nikula <jani.nikula at linux.intel.com>; intel-gfx at lists.freedesktop.org; Cui, YuepingX <yuepingx.cui at intel.com>; Wang, Quanxian <quanxian.wang at intel.com>
Subject: RE: [Intel-gfx] [PATCH] drm/i915/audio: not to set N/M value manually for KBL.

Hi, Libin

We don't have 4K resolution monitor. We will double check others resolutions.

-----Original Message-----
From: Yang, Libin 
Sent: Wednesday, April 5, 2017 10:22 AM
To: Jani Nikula <jani.nikula at linux.intel.com>; Wang, Quanxian <quanxian.wang at intel.com>; intel-gfx at lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH] drm/i915/audio: not to set N/M value manually for KBL.


>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Tuesday, April 4, 2017 6:55 PM
>To: Wang, Quanxian <quanxian.wang at intel.com>; intel- 
>gfx at lists.freedesktop.org
>Cc: Yang, Libin <libin.yang at 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 at 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 at 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.

Is this a mandatory requirement on the new platform?
[Wang, Quanxian] yes. At least bspec said that with 'NOTE'.
We saw "disable and enable transcoder" requirement on old platforms.
But it will dramatically impact on the user experience. After discussion, we skipped this step and did a lot of test on the old platforms. They all works.
[Wang, Quanxian] Seems to be in old platforms.

I suggested that we can talk with silicon team to figure out a proper solution, because on the new platform we may also meet the same situations like old platforms that some resolution will not work properly.

BTW: please test the playback on 4K resolution to see whether the playback and delay is normal. 4K is often an issue.

Regards,
Libin

>>
>> Signed-off-by: Quanxian Wang <quanxian.wang at intel.com>
>> Tested-By: Wang Zhijun<zhijunx.wang at intel.com>
>> Tested-By: Cui Yueping<yuepingx.cui at 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