Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Jani Nikula]

On Wed, 14 Feb 2018, "Kumar, Abhijeet"  wrote:
> On 2/14/2018 4:48 PM, Takashi Iwai wrote:
>> On Wed, 14 Feb 2018 10:06:19 +0100,
>> Kumar, Abhijeet wrote:
>>>
>>>
>>> On 2/14/2018 2:17 PM, Chris Wilson wrote:
 Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
> On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:
>
>   From: Abhijeet Kumar 
>
>   ---
>sound/pci/hda/hda_codec.c | 2 +-
>1 file changed, 1 insertion(+), 1 deletion(-)
>
>   diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>   index 8c1b07e300a8..377d5719b4cd 100644
>   --- a/sound/pci/hda/hda_codec.c
>   +++ b/sound/pci/hda/hda_codec.c
>   @@ -2714,7 +2714,7 @@ static unsigned int 
> hda_sync_power_state(struct hda_codec *codec,
>   int count;
>
>   for (count = 0;count < 500; count++) {
>   -   state = snd_hda_codec_read(codec, fg, 0,
>   +   state = snd_hdac_codec_read(>core, fg, 0,
>  AC_VERB_GET_POWER_STATE, 
> 0);
>   if (state & AC_PWRST_ERROR){
>   msleep(20);
>
>
> Both tests are passing on hsw and bdw devices.I can conclude that none of 
> my
> changes
 Where did you run this against CI? (Due to the nature of patchwork it
 will not have picked this up as a new revision.)
>>> You can find it here https://patchwork.freedesktop.org/series/38212/.
>>> I've reverted my patch and made my changes in hda_codec inorder to
>>> demonstrate my changes is not
>>> breaking it.
> in "ALSA: hda: Make use of core codec functions to sync power state" is "
> directly" causing the regression.
> As this patch series changes the previously defined sync function similar 
> to
> the latest one (the one defined
> in the defaulter patch).
 If you have no answer, we will apply the revert to our CI so that we do
 not lose coverage.
>>> I guess, I don't have any issue by reverting this single patch alone
>>> as i already said this patch had
>>> no functional change! It just had few optimization which  i believe we
>>> can skip for now.  :)
>> Well, it still doesn't explain.  The loop count is 500 and we have
>> msleep(1), so it should be almost identical with the jiffies timeout.
>
> Even when the loop count is 500 and we have msleep(1) in earlier defined 
> sync function.
> We dont see wait_for_suspended assertion failing. See results for rev1 here
> https://patchwork.freedesktop.org/series/38188/
>> We need more investigation, in which code path the bug is triggered.
>
> I'm convinced.It seems like some other issue which was hidden earlier 
> had come into play
> because of trivial code movement. It needs more triage. Can we enable 
> debugs logs and
> try the test again? I'm afraid I dont have the hardware with me locally 
> right now.I'll try arranging.
> Meanwhile maybe someone from CI team can help with logs ?
>
> https://bugs.freedesktop.org/show_bug.cgi?id=105069

This remains an issue. The regressing commit is now in v4.17-rc1. We've
reverted it in our local branches to unblock CI.

Can we please get some closure here, if nothing else works then please
revert and go back to the drawing board.

BR,
Jani.



>
> Warm Regards,
> Abhijeet
>>
>>
>> thanks,
>>
>> Takashi
>
> ___
> 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

Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

On 2/14/2018 4:48 PM, Takashi Iwai wrote:

On Wed, 14 Feb 2018 10:06:19 +0100,
Kumar, Abhijeet wrote:



On 2/14/2018 2:17 PM, Chris Wilson wrote:

Quoting Kumar, Abhijeet (2018-02-14 04:53:57)

On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:

  From: Abhijeet Kumar 

  ---
   sound/pci/hda/hda_codec.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

  diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
  index 8c1b07e300a8..377d5719b4cd 100644
  --- a/sound/pci/hda/hda_codec.c
  +++ b/sound/pci/hda/hda_codec.c
  @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct 
hda_codec *codec,
  int count;

  for (count = 0;count < 500; count++) {
  -   state = snd_hda_codec_read(codec, fg, 0,
  +   state = snd_hdac_codec_read(>core, fg, 0,
 AC_VERB_GET_POWER_STATE, 0);
  if (state & AC_PWRST_ERROR){
  msleep(20);


Both tests are passing on hsw and bdw devices.I can conclude that none of my
changes

Where did you run this against CI? (Due to the nature of patchwork it
will not have picked this up as a new revision.)

You can find it here https://patchwork.freedesktop.org/series/38212/.
I've reverted my patch and made my changes in hda_codec inorder to
demonstrate my changes is not
breaking it.

in "ALSA: hda: Make use of core codec functions to sync power state" is "
directly" causing the regression.
As this patch series changes the previously defined sync function similar to
the latest one (the one defined
in the defaulter patch).

If you have no answer, we will apply the revert to our CI so that we do
not lose coverage.

I guess, I don't have any issue by reverting this single patch alone
as i already said this patch had
no functional change! It just had few optimization which  i believe we
can skip for now.  :)

Well, it still doesn't explain.  The loop count is 500 and we have
msleep(1), so it should be almost identical with the jiffies timeout.


Even when the loop count is 500 and we have msleep(1) in earlier defined 
sync function.

We dont see wait_for_suspended assertion failing. See results for rev1 here
https://patchwork.freedesktop.org/series/38188/

We need more investigation, in which code path the bug is triggered.


I'm convinced.It seems like some other issue which was hidden earlier 
had come into play
because of trivial code movement. It needs more triage. Can we enable 
debugs logs and
try the test again? I'm afraid I dont have the hardware with me locally 
right now.I'll try arranging.

Meanwhile maybe someone from CI team can help with logs ?

https://bugs.freedesktop.org/show_bug.cgi?id=105069

Warm Regards,
Abhijeet



thanks,

Takashi


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Takashi Iwai]

On Wed, 14 Feb 2018 10:06:19 +0100,
Kumar, Abhijeet wrote:
> 
> 
> 
> On 2/14/2018 2:17 PM, Chris Wilson wrote:
> > Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
> >>
> >> On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:
> >>
> >>  From: Abhijeet Kumar 
> >>
> >>  ---
> >>   sound/pci/hda/hda_codec.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>  diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> >>  index 8c1b07e300a8..377d5719b4cd 100644
> >>  --- a/sound/pci/hda/hda_codec.c
> >>  +++ b/sound/pci/hda/hda_codec.c
> >>  @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct 
> >> hda_codec *codec,
> >>  int count;
> >>
> >>  for (count = 0;count < 500; count++) {
> >>  -   state = snd_hda_codec_read(codec, fg, 0,
> >>  +   state = snd_hdac_codec_read(>core, fg, 0,
> >> AC_VERB_GET_POWER_STATE, 
> >> 0);
> >>  if (state & AC_PWRST_ERROR){
> >>  msleep(20);
> >>
> >>
> >> Both tests are passing on hsw and bdw devices.I can conclude that none of 
> >> my
> >> changes
> > Where did you run this against CI? (Due to the nature of patchwork it
> > will not have picked this up as a new revision.)
> 
> You can find it here https://patchwork.freedesktop.org/series/38212/.
> I've reverted my patch and made my changes in hda_codec inorder to
> demonstrate my changes is not
> breaking it.
> >
> >> in "ALSA: hda: Make use of core codec functions to sync power state" is "
> >> directly" causing the regression.
> >> As this patch series changes the previously defined sync function similar 
> >> to
> >> the latest one (the one defined
> >> in the defaulter patch).
> > If you have no answer, we will apply the revert to our CI so that we do
> > not lose coverage.
> 
> I guess, I don't have any issue by reverting this single patch alone
> as i already said this patch had
> no functional change! It just had few optimization which  i believe we
> can skip for now.  :)

Well, it still doesn't explain.  The loop count is 500 and we have
msleep(1), so it should be almost identical with the jiffies timeout.

We need more investigation, in which code path the bug is triggered.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

On 2/14/2018 2:17 PM, Chris Wilson wrote:

Quoting Kumar, Abhijeet (2018-02-14 04:53:57)


On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:

 From: Abhijeet Kumar 

 ---
  sound/pci/hda/hda_codec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
 index 8c1b07e300a8..377d5719b4cd 100644
 --- a/sound/pci/hda/hda_codec.c
 +++ b/sound/pci/hda/hda_codec.c
 @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct 
hda_codec *codec,
 int count;

 for (count = 0;count < 500; count++) {
 -   state = snd_hda_codec_read(codec, fg, 0,
 +   state = snd_hdac_codec_read(>core, fg, 0,
AC_VERB_GET_POWER_STATE, 0);
 if (state & AC_PWRST_ERROR){
 msleep(20);


Both tests are passing on hsw and bdw devices.I can conclude that none of my
changes

Where did you run this against CI? (Due to the nature of patchwork it
will not have picked this up as a new revision.)


You can find it here https://patchwork.freedesktop.org/series/38212/.
I've reverted my patch and made my changes in hda_codec inorder to 
demonstrate my changes is not

breaking it.



in "ALSA: hda: Make use of core codec functions to sync power state" is "
directly" causing the regression.
As this patch series changes the previously defined sync function similar to
the latest one (the one defined
in the defaulter patch).

If you have no answer, we will apply the revert to our CI so that we do
not lose coverage.


I guess, I don't have any issue by reverting this single patch alone as 
i already said this patch had
no functional change! It just had few optimization which  i believe we 
can skip for now.  :)


+Takashi to comment.

-Abhijeet

-Chris


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Chris Wilson]

Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
> 
> 
> On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:
> 
> From: Abhijeet Kumar 
> 
> ---
>  sound/pci/hda/hda_codec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 8c1b07e300a8..377d5719b4cd 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct 
> hda_codec *codec,
> int count;
> 
> for (count = 0;count < 500; count++) {
> -   state = snd_hda_codec_read(codec, fg, 0,
> +   state = snd_hdac_codec_read(>core, fg, 0,
>AC_VERB_GET_POWER_STATE, 0);
> if (state & AC_PWRST_ERROR){
> msleep(20);
> 
> 
> Both tests are passing on hsw and bdw devices.I can conclude that none of my
> changes

Where did you run this against CI? (Due to the nature of patchwork it
will not have picked this up as a new revision.)

> in "ALSA: hda: Make use of core codec functions to sync power state" is "
> directly" causing the regression.
> As this patch series changes the previously defined sync function similar to
> the latest one (the one defined
> in the defaulter patch).

If you have no answer, we will apply the revert to our CI so that we do
not lose coverage.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

On 2/14/2018 9:36 AM, abhijeet.ku...@intel.com wrote:

From: Abhijeet Kumar 

---
  sound/pci/hda/hda_codec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8c1b07e300a8..377d5719b4cd 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec 
*codec,
int count;
  
  	for (count = 0;count < 500; count++) {

-   state = snd_hda_codec_read(codec, fg, 0,
+   state = snd_hdac_codec_read(>core, fg, 0,
   AC_VERB_GET_POWER_STATE, 0);
if (state & AC_PWRST_ERROR){
msleep(20);


Both tests are passing on hsw and bdw devices.I can conclude that none 
of my changes
in "ALSA: hda: Make use of core codec functions to sync power state" is 
"*directly*" causing the regression.
As this patch series changes the previously defined sync function 
similar to the latest one (the one defined

in the defaulter patch).

Sidenote- Sorry for the missing commit message, it was just a HACK patch. :)

-Abhijeet
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[abhijeet . kumar]

From: Abhijeet Kumar 

---
 sound/pci/hda/hda_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8c1b07e300a8..377d5719b4cd 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec 
*codec,
int count;
 
for (count = 0;count < 500; count++) {
-   state = snd_hda_codec_read(codec, fg, 0,
+   state = snd_hdac_codec_read(>core, fg, 0,
   AC_VERB_GET_POWER_STATE, 0);
if (state & AC_PWRST_ERROR){
msleep(20);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[abhijeet . kumar]

From: Abhijeet Kumar 

---
 sound/pci/hda/hda_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8c1b07e300a8..377d5719b4cd 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec 
*codec,
int count;
 
for (count = 0;count < 500; count++) {
-   state = snd_hda_codec_read(codec, fg, 0,
+   state = snd_hdac_codec_read(>core, fg, 0,
   AC_VERB_GET_POWER_STATE, 0);
if (state & AC_PWRST_ERROR){
msleep(20);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx