Defined NM doesn't work on KBL and uses automatic N/M.

Commit Message

Wang, Quanxian May 26, 2017, 6:07 a.m. UTC

According to the bspec, when set N/M, should disable
and enable transcoder which attaching DP audio. However
with that implementation will affect performance more.

But with current implementation, KBL could not work with defined N/M.
By default, KBL will use automatic N/M.

Signed-off-by: Quanxian Wang <[e-mail]>
Reviewed-by: Libin Yang <[e-mail]>
Reviewed-by: Jani Nikula <[e-mail]>
---
 drivers/gpu/drm/i915/intel_audio.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jani Nikula May 26, 2017, 8:31 a.m. UTC | #1

On Fri, 26 May 2017, Quanxian Wang <[e-mail]> wrote:
> According to the bspec, when set N/M, should disable
> and enable transcoder which attaching DP audio. However
> with that implementation will affect performance more.
>
> But with current implementation, KBL could not work with defined N/M.
> By default, KBL will use automatic N/M.
>
> Signed-off-by: Quanxian Wang <[e-mail]>
> Reviewed-by: Libin Yang <[e-mail]>
> Reviewed-by: Jani Nikula <[e-mail]>

I have *NOT* reviewed this patch. Even if it's based on my suggestion in
[1], it does not automatically signify review.

The patch subject should start with "drm/i915/audio:".

The DP spec always expresses the values as M/N, more specifically
Maud/Naud, so it's confusing to see them expressed as N/M.

I still have trouble understanding the commit message. I am wondering if
something was fixed in Kabylake and beyond, since according to
6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset") we need this
for certain modes. In particular, should the condition check for KBL ||
gen >= 10?

BR,
Jani.


[1] http://mid.mail-archive.com/[e-mail]



> ---
>  drivers/gpu/drm/i915/intel_audio.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index d805b6e..ba4994e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -289,6 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
>  	enum pipe pipe = intel_crtc->pipe;
>  	u32 tmp;
>  
> +	/* It doesn't work on KBL and uses automatic N/M. */
> +	if (IS_KABYLAKE(dev_priv))
> +		nm = NULL;
> +
>  	if (nm)
>  		DRM_DEBUG_KMS("using Maud %u, Naud %u\n", nm->m, nm->n);
>  	else

Wang, Quanxian May 26, 2017, 9:52 a.m. UTC | #2

Hi, Jani

Thanks for your comment. My comment below.

-----Original Message-----
From: Jani Nikula [mailto:[e-mail]] 
Sent: Friday, May 26, 2017 4:31 PM
To: Wang, Quanxian <[e-mail]>; [e-mail]
Cc: Yang, Libin <[e-mail]>; Wang, Quanxian <[e-mail]>
Subject: Re: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.

On Fri, 26 May 2017, Quanxian Wang <[e-mail]> wrote:
> According to the bspec, when set N/M, should disable and enable 
> transcoder which attaching DP audio. However with that implementation 
> will affect performance more.
>
> But with current implementation, KBL could not work with defined N/M.
> By default, KBL will use automatic N/M.
>
> Signed-off-by: Quanxian Wang <[e-mail]>
> Reviewed-by: Libin Yang <[e-mail]>
> Reviewed-by: Jani Nikula <[e-mail]>

I have *NOT* reviewed this patch. Even if it's based on my suggestion in [1], it does not automatically signify review.
[Wang, Quanxian] Sorry, I accept your suggestion. So in order to respect what you said, I input your name here.

The patch subject should start with "drm/i915/audio:".
[Wang, Quanxian] Ok.

The DP spec always expresses the values as M/N, more specifically Maud/Naud, so it's confusing to see them expressed as N/M.
[Wang, Quanxian] accept it.

I still have trouble understanding the commit message. I am wondering if something was fixed in Kabylake and beyond, since according to
[Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL and more, the audio doesn't work on KBL specially, and other platform works fine. Even if with latest upstream kernel, but it works on 4.8 kernel. Therefore It is a regression issue. After investigation, we found it is caused by  commit "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper N/M in modeset". Therefore I provided this patch.
After getting suggestion from you and Libin, we do more testing on 4K monitor for audio delay issue. Also talked with Libin why not use transcoder disable/enable process described in bspec, it will affect performance.
This is the whole story. You can check LCK number 3791 in Intel JIRA about that.

6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset") we need this for certain modes. In particular, should the condition check for KBL || gen >= 10?

[Wang, Quanxian] Basically if upstream could provide a reasonable patch, it will appreciate since you are expert in this area.
Thanks for your understanding.

BR,
Jani.


[1] http://mid.mail-archive.com/[e-mail]



> ---
>  drivers/gpu/drm/i915/intel_audio.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index d805b6e..ba4994e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -289,6 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
>  	enum pipe pipe = intel_crtc->pipe;
>  	u32 tmp;
>  
> +	/* It doesn't work on KBL and uses automatic N/M. */
> +	if (IS_KABYLAKE(dev_priv))
> +		nm = NULL;
> +
>  	if (nm)
>  		DRM_DEBUG_KMS("using Maud %u, Naud %u\n", nm->m, nm->n);
>  	else

--
Jani Nikula, Intel Open Source Technology Center

Jani Nikula May 26, 2017, 11 a.m. UTC | #3

On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
> [Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL
> and more, the audio doesn't work on KBL specially, and other platform
> works fine. Even if with latest upstream kernel, but it works on 4.8
> kernel. Therefore It is a regression issue. After investigation, we
> found it is caused by commit
> "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper
> N/M in modeset". Therefore I provided this patch.

If it's a regression, we need to backport the fix to stable kernels, and
the patch needs annotation:

Fixes: 6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset")
Cc: <[e-mail]> # v4.10+

> After getting suggestion from you and Libin, we do more testing on 4K
> monitor for audio delay issue. Also talked with Libin why not use
> transcoder disable/enable process described in bspec, it will affect
> performance.
>
> This is the whole story. You can check LCK number 3791 in Intel JIRA
> about that.

Actually, I can't. And the commit message needs to have the
explanation. I still do not know what the bug is specifically, or why
Kabylake, and only Kabylake, needs this.

> [Wang, Quanxian] Basically if upstream could provide a reasonable
> patch, it will appreciate since you are expert in this area.
> Thanks for your understanding.

The patch is probably fine. But I can't invent an explanation if I don't
know what the problem is.


BR,
Jani.

Wang, Quanxian May 26, 2017, 9:36 p.m. UTC | #4

"The patch is probably fine. But I can't invent an explanation if I don't know what the problem is."

@Jani @libin, is it possible for you to help contact Intel graphics hardware design engineer to get the reason why M/N doesn't work as you implemented in kernel code in Kabylake platform? I don't find good reason in bspec. Thanks.

-----Original Message-----
From: Jani Nikula [mailto:[e-mail]] 
Sent: Friday, May 26, 2017 7:01 PM
To: Wang, Quanxian <[e-mail]>; [e-mail]
Cc: Yang, Libin <[e-mail]>
Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.

On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
> [Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL 
> and more, the audio doesn't work on KBL specially, and other platform 
> works fine. Even if with latest upstream kernel, but it works on 4.8 
> kernel. Therefore It is a regression issue. After investigation, we 
> found it is caused by commit 
> "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper 
> N/M in modeset". Therefore I provided this patch.

If it's a regression, we need to backport the fix to stable kernels, and the patch needs annotation:

Fixes: 6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset")
Cc: <[e-mail]> # v4.10+

> After getting suggestion from you and Libin, we do more testing on 4K 
> monitor for audio delay issue. Also talked with Libin why not use 
> transcoder disable/enable process described in bspec, it will affect 
> performance.
>
> This is the whole story. You can check LCK number 3791 in Intel JIRA 
> about that.

Actually, I can't. And the commit message needs to have the explanation. I still do not know what the bug is specifically, or why Kabylake, and only Kabylake, needs this.

> [Wang, Quanxian] Basically if upstream could provide a reasonable 
> patch, it will appreciate since you are expert in this area.
> Thanks for your understanding.

The patch is probably fine. But I can't invent an explanation if I don't know what the problem is.


BR,
Jani.

--
Jani Nikula, Intel Open Source Technology Center

Jani Nikula May 29, 2017, 8:29 a.m. UTC | #5

On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
> "The patch is probably fine. But I can't invent an explanation if I
> don't know what the problem is."
>
> @Jani @libin, is it possible for you to help contact Intel graphics
> hardware design engineer to get the reason why M/N doesn't work as you
> implemented in kernel code in Kabylake platform? I don't find good
> reason in bspec. Thanks.

How did you come up with the patch then? What made you try this?

BR,
Jani.


>
> -----Original Message-----
> From: Jani Nikula [mailto:[e-mail]] 
> Sent: Friday, May 26, 2017 7:01 PM
> To: Wang, Quanxian <[e-mail]>; [e-mail]
> Cc: Yang, Libin <[e-mail]>
> Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.
>
> On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
>> [Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL 
>> and more, the audio doesn't work on KBL specially, and other platform 
>> works fine. Even if with latest upstream kernel, but it works on 4.8 
>> kernel. Therefore It is a regression issue. After investigation, we 
>> found it is caused by commit 
>> "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper 
>> N/M in modeset". Therefore I provided this patch.
>
> If it's a regression, we need to backport the fix to stable kernels, and the patch needs annotation:
>
> Fixes: 6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset")
> Cc: <[e-mail]> # v4.10+
>
>> After getting suggestion from you and Libin, we do more testing on 4K 
>> monitor for audio delay issue. Also talked with Libin why not use 
>> transcoder disable/enable process described in bspec, it will affect 
>> performance.
>>
>> This is the whole story. You can check LCK number 3791 in Intel JIRA 
>> about that.
>
> Actually, I can't. And the commit message needs to have the explanation. I still do not know what the bug is specifically, or why Kabylake, and only Kabylake, needs this.
>
>> [Wang, Quanxian] Basically if upstream could provide a reasonable 
>> patch, it will appreciate since you are expert in this area.
>> Thanks for your understanding.
>
> The patch is probably fine. But I can't invent an explanation if I don't know what the problem is.
>
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel Open Source Technology Center

Wang, Quanxian May 31, 2017, 3:20 a.m. UTC | #6

-----Original Message-----
From: Jani Nikula [mailto:[e-mail]] 
Sent: Monday, May 29, 2017 4:29 PM
To: Wang, Quanxian <[e-mail]>; [e-mail]
Cc: Yang, Libin <[e-mail]>
Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.


On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
> "The patch is probably fine. But I can't invent an explanation if I 
> don't know what the problem is."
>
> @Jani @libin, is it possible for you to help contact Intel graphics 
> hardware design engineer to get the reason why M/N doesn't work as you 
> implemented in kernel code in Kabylake platform? I don't find good 
> reason in bspec. Thanks.

How did you come up with the patch then? What made you try this?
[Wang, Quanxian] I am from OSV enabling team and responsible for Ubuntu enabling. 17.04 (kernel 4.10) is released, we will do full testing for all Intel platforms including KBL/SKL... With audio testing, we found this regression issue on KBL and find patch caused this regression. Compared with previous implementation, we use automatic M/N to have a try. It works. 

BR,
Jani.


>
> -----Original Message-----
> From: Jani Nikula [mailto:[e-mail]]
> Sent: Friday, May 26, 2017 7:01 PM
> To: Wang, Quanxian <[e-mail]>; 
> [e-mail]
> Cc: Yang, Libin <[e-mail]>
> Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.
>
> On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
>> [Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL 
>> and more, the audio doesn't work on KBL specially, and other platform 
>> works fine. Even if with latest upstream kernel, but it works on 4.8 
>> kernel. Therefore It is a regression issue. After investigation, we 
>> found it is caused by commit 
>> "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper 
>> N/M in modeset". Therefore I provided this patch.
>
> If it's a regression, we need to backport the fix to stable kernels, and the patch needs annotation:
>
> Fixes: 6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset")
> Cc: <[e-mail]> # v4.10+
>
>> After getting suggestion from you and Libin, we do more testing on 4K 
>> monitor for audio delay issue. Also talked with Libin why not use 
>> transcoder disable/enable process described in bspec, it will affect 
>> performance.
>>
>> This is the whole story. You can check LCK number 3791 in Intel JIRA 
>> about that.
>
> Actually, I can't. And the commit message needs to have the explanation. I still do not know what the bug is specifically, or why Kabylake, and only Kabylake, needs this.
>
>> [Wang, Quanxian] Basically if upstream could provide a reasonable 
>> patch, it will appreciate since you are expert in this area.
>> Thanks for your understanding.
>
> The patch is probably fine. But I can't invent an explanation if I don't know what the problem is.
>
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel Open Source Technology Center

--
Jani Nikula, Intel Open Source Technology Center

Wang, Quanxian Dec. 8, 2017, 12:41 a.m. UTC | #7

Hi, Jani

I have created a Bugzilla in free desktop for tracking.
https://bugs.freedesktop.org/show_bug.cgi?id=104093

Appreciate find audio expert could help us address root cause.

Thank you.

Quanxian

-----Original Message-----
From: Intel-gfx [mailto:[e-mail]] On Behalf Of Wang, Quanxian

Sent: Wednesday, May 31, 2017 11:21 AM
To: Jani Nikula <[e-mail]>; [e-mail]
Subject: Re: [Intel-gfx] [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.



-----Original Message-----
From: Jani Nikula [mailto:[e-mail]] 

Sent: Monday, May 29, 2017 4:29 PM
To: Wang, Quanxian <[e-mail]>; [e-mail]
Cc: Yang, Libin <[e-mail]>
Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.


On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:
> "The patch is probably fine. But I can't invent an explanation if I 

> don't know what the problem is."

>

> @Jani @libin, is it possible for you to help contact Intel graphics 

> hardware design engineer to get the reason why M/N doesn't work as you 

> implemented in kernel code in Kabylake platform? I don't find good 

> reason in bspec. Thanks.


How did you come up with the patch then? What made you try this?
[Wang, Quanxian] I am from OSV enabling team and responsible for Ubuntu enabling. 17.04 (kernel 4.10) is released, we will do full testing for all Intel platforms including KBL/SKL... With audio testing, we found this regression issue on KBL and find patch caused this regression. Compared with previous implementation, we use automatic M/N to have a try. It works. 

BR,
Jani.


>

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

> From: Jani Nikula [mailto:[e-mail]]

> Sent: Friday, May 26, 2017 7:01 PM

> To: Wang, Quanxian <[e-mail]>; 

> [e-mail]

> Cc: Yang, Libin <[e-mail]>

> Subject: RE: [PATCH] Defined NM doesn't work on KBL and uses automatic N/M.

>

> On Fri, 26 May 2017, "Wang, Quanxian" <[e-mail]> wrote:

>> [Wang, Quanxian] in Ubuntu 17.04, we do audio testing on KBL/SKL/APL 

>> and more, the audio doesn't work on KBL specially, and other platform 

>> works fine. Even if with latest upstream kernel, but it works on 4.8 

>> kernel. Therefore It is a regression issue. After investigation, we 

>> found it is caused by commit 

>> "6014ac122ed081feca99217bc57b2e15c7fc1a51] drm/i915/audio: set proper 

>> N/M in modeset". Therefore I provided this patch.

>

> If it's a regression, we need to backport the fix to stable kernels, and the patch needs annotation:

>

> Fixes: 6014ac122ed0 ("drm/i915/audio: set proper N/M in modeset")

> Cc: <[e-mail]> # v4.10+

>

>> After getting suggestion from you and Libin, we do more testing on 4K 

>> monitor for audio delay issue. Also talked with Libin why not use 

>> transcoder disable/enable process described in bspec, it will affect 

>> performance.

>>

>> This is the whole story. You can check LCK number 3791 in Intel JIRA 

>> about that.

>

> Actually, I can't. And the commit message needs to have the explanation. I still do not know what the bug is specifically, or why Kabylake, and only Kabylake, needs this.

>

>> [Wang, Quanxian] Basically if upstream could provide a reasonable 

>> patch, it will appreciate since you are expert in this area.

>> Thanks for your understanding.

>

> The patch is probably fine. But I can't invent an explanation if I don't know what the problem is.

>

>

> BR,

> Jani.

>

> --

> Jani Nikula, Intel Open Source Technology Center


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

Patch

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index d805b6e..ba4994e 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -289,6 +289,10 @@  hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
 	enum pipe pipe = intel_crtc->pipe;
 	u32 tmp;
 
+	/* It doesn't work on KBL and uses automatic N/M. */
+	if (IS_KABYLAKE(dev_priv))
+		nm = NULL;
+
 	if (nm)
 		DRM_DEBUG_KMS("using Maud %u, Naud %u\n", nm->m, nm->n);
 	else