On Thu, 2020-06-11 at 19:59 +0200, Takashi Iwai wrote:
> On Thu, 11 Jun 2020 19:09:08 +0200,
> Lu, Brent wrote:
> >
> > > Hi Brent,
> > >
> > > Thanks for the patch. Is this fix for a specific issue you're
> > > seeing?
> > > If so, could you please give us some details about it?
> > >
> > > Thanks,
> > > Ranjani
> >
> > Hi Ranjani,
> >
> > It's reported to happen on GLK Chromebook 'Fleex' that sometimes it
> > cannot output the audio stream to external display. The kernel is
> > Chrome v4.14 branch. Following is the reproduce step provided by
> > ODM but I could reproduce it simply running aplay or
> > cras_test_client
> > so I think it's not about the cable plug/unplug handling.
> >
> > What steps will reproduce the problem?
> > 1. Play YouTube video on Chromebook and connect it to external
> > monitor with Type C to DP dongle
> > 2. Press monitor power button to turn off the monitor
> > 3. Press monitor power button again to turn on the monitor
> > 4. Continue to play YouTube video and check audio playback
> > 5. No sound comes out from built-in speaker of external
> > monitor when turn on external monitor
> >
> > I added debug messages to print the RIRBWP register and realize
> > that
> > response could come between the read of RIRBWP in the
> > snd_hdac_bus_update_rirb() function and the interrupt clear in the
> > hda_dsp_stream_interrupt() function. The response is not handled
> > but
> > the interrupt is already cleared. It will cause timeout unless more
> > responses coming to RIRB.
>
> Now I noticed that the legacy driver already addressed it recently
> via
> commit 6d011d5057ff
> ALSA: hda: Clear RIRB status before reading WP
>
> We should have checked SOF at the same time, too...
Thanks, Takashi. But the legacy driver but doesnt remove the loop. The
loop added in the SOF driver was based on the legacy driver and
specifically to handle missed stream interrupts. Is there any harm in
keeping the loop?
Thanks,
Ranjani
On Thu, 11 Jun 2020 20:12:53 +0200,
Ranjani Sridharan wrote:
>
> On Thu, 2020-06-11 at 19:59 +0200, Takashi Iwai wrote:
> > On Thu, 11 Jun 2020 19:09:08 +0200,
> > Lu, Brent wrote:
> > >
> > > > Hi Brent,
> > > >
> > > > Thanks for the patch. Is this fix for a specific issue you're
> > > > seeing?
> > > > If so, could you please give us some details about it?
> > > >
> > > > Thanks,
> > > > Ranjani
> > >
> > > Hi Ranjani,
> > >
> > > It's reported to happen on GLK Chromebook 'Fleex' that sometimes it
> > > cannot output the audio stream to external display. The kernel is
> > > Chrome v4.14 branch. Following is the reproduce step provided by
> > > ODM but I could reproduce it simply running aplay or
> > > cras_test_client
> > > so I think it's not about the cable plug/unplug handling.
> > >
> > > What steps will reproduce the problem?
> > > 1. Play YouTube video on Chromebook and connect it to external
> > > monitor with Type C to DP dongle
> > > 2. Press monitor power button to turn off the monitor
> > > 3. Press monitor power button again to turn on the monitor
> > > 4. Continue to play YouTube video and check audio playback
> > > 5. No sound comes out from built-in speaker of external
> > > monitor when turn on external monitor
> > >
> > > I added debug messages to print the RIRBWP register and realize
> > > that
> > > response could come between the read of RIRBWP in the
> > > snd_hdac_bus_update_rirb() function and the interrupt clear in the
> > > hda_dsp_stream_interrupt() function. The response is not handled
> > > but
> > > the interrupt is already cleared. It will cause timeout unless more
> > > responses coming to RIRB.
> >
> > Now I noticed that the legacy driver already addressed it recently
> > via
> > commit 6d011d5057ff
> > ALSA: hda: Clear RIRB status before reading WP
> >
> > We should have checked SOF at the same time, too...
>
> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The
> loop added in the SOF driver was based on the legacy driver and
> specifically to handle missed stream interrupts. Is there any harm in
> keeping the loop?
A loop there might be safer to keep, indeed. That's basically for a
difference kind of race, and it can still happen theoretically.
Though, SOF is with the threaded interrupt, and it's interesting how
the behavior differs. I can imagine that, if a thread irq is running
while a new IRQ is re-triggered, the hard irq handler won't queue it
again. But I might be wrong here, need some checks.
Takashi
>>>> I added debug messages to print the RIRBWP register and realize
>>>> that
>>>> response could come between the read of RIRBWP in the
>>>> snd_hdac_bus_update_rirb() function and the interrupt clear in the
>>>> hda_dsp_stream_interrupt() function. The response is not handled
>>>> but
>>>> the interrupt is already cleared. It will cause timeout unless more
>>>> responses coming to RIRB.
>>>
>>> Now I noticed that the legacy driver already addressed it recently
>>> via
>>> commit 6d011d5057ff
>>> ALSA: hda: Clear RIRB status before reading WP
>>>
>>> We should have checked SOF at the same time, too...
>>
>> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The
>> loop added in the SOF driver was based on the legacy driver and
>> specifically to handle missed stream interrupts. Is there any harm in
>> keeping the loop?
>
> A loop there might be safer to keep, indeed. That's basically for a
> difference kind of race, and it can still happen theoretically.
>
> Though, SOF is with the threaded interrupt, and it's interesting how
> the behavior differs. I can imagine that, if a thread irq is running
> while a new IRQ is re-triggered, the hard irq handler won't queue it
> again. But I might be wrong here, need some checks.
IIRC we added this loop before merging all interrupt handling in one
thread, somehow the MSI mode never worked reliably without this change,
so maybe we don't need this loop any longer.
I'd really prefer it if we didn't tie the RIRB handing change to this
loop change, removing the loop should only be done with *a lot of testing*.
>
> IIRC we added this loop before merging all interrupt handling in one thread,
> somehow the MSI mode never worked reliably without this change, so
> maybe we don't need this loop any longer.
>
> I'd really prefer it if we didn't tie the RIRB handing change to this loop change,
> removing the loop should only be done with *a lot of testing*.
The reason I removed the loop because I thought it's for the unsolicited response,
apparently it's not. I'd like to port the commit 6d011d5057ff
ALSA: hda: Clear RIRB status before reading WP
to SOF driver and upload a version 2. Thanks.
Regards,
Brent