Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Jon Hunter]

Hi Takashi,

On 18/06/2020 15:40, Takashi Iwai wrote:
> In the end we already enabled the sync-write mode for most of HD-audio
> controllers including Intel, and it's no big merit to keep the async
> write mode for the rest.  Let's make it as default and drop the
> superfluous AZX_DCAPS_SYNC_WRITE bit flag.
> 
> Also, avoid to set the allow_bus_reset flag, which is a quite unstable
> and hackish behavior that was needed only for some early platforms
> (decades ago).  The straight fallback to the single cmd mode is more
> robust.
> 
> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>


I have noticed a regression in HDA playback on our Tegra186 Jetson TX2
platform. Bisect is pointing to this patch and reverting this does
appear to fix it. Interestingly, I am not seeing any problems on other
Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA
which is one different between the other platforms.

We can take a closer look at this for Tegra, but I am wondering if we
revert this for Tegra for now.

Cheers
Jon

-- 
nvpublic

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Jon Hunter]

On 14/07/2020 09:08, Jon Hunter wrote:
> Hi Takashi,
> 
> On 18/06/2020 15:40, Takashi Iwai wrote:
>> In the end we already enabled the sync-write mode for most of HD-audio
>> controllers including Intel, and it's no big merit to keep the async
>> write mode for the rest.  Let's make it as default and drop the
>> superfluous AZX_DCAPS_SYNC_WRITE bit flag.
>>
>> Also, avoid to set the allow_bus_reset flag, which is a quite unstable
>> and hackish behavior that was needed only for some early platforms
>> (decades ago).  The straight fallback to the single cmd mode is more
>> robust.
>>
>> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> 
> 
> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2
> platform. Bisect is pointing to this patch and reverting this does
> appear to fix it. Interestingly, I am not seeing any problems on other
> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA
> which is one different between the other platforms.
> 
> We can take a closer look at this for Tegra, but I am wondering if we
> revert this for Tegra for now.

By revert, I don't mean revert the entire change, but just disable the
sync-write for Tegra for now.

Jon

-- 
nvpublic

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Takashi Iwai]

On Tue, 14 Jul 2020 10:08:02 +0200,
Jon Hunter wrote:
> 
> Hi Takashi,
> 
> On 18/06/2020 15:40, Takashi Iwai wrote:
> > In the end we already enabled the sync-write mode for most of HD-audio
> > controllers including Intel, and it's no big merit to keep the async
> > write mode for the rest.  Let's make it as default and drop the
> > superfluous AZX_DCAPS_SYNC_WRITE bit flag.
> > 
> > Also, avoid to set the allow_bus_reset flag, which is a quite unstable
> > and hackish behavior that was needed only for some early platforms
> > (decades ago).  The straight fallback to the single cmd mode is more
> > robust.
> > 
> > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> 
> 
> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2
> platform. Bisect is pointing to this patch and reverting this does
> appear to fix it. Interestingly, I am not seeing any problems on other
> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA
> which is one different between the other platforms.
> 
> We can take a closer look at this for Tegra, but I am wondering if we
> revert this for Tegra for now.

It's a deja vu, we (or someone else in Nvidia?) discussed it in the
past?

The patch below should cure the problem temporarily, as it sets the
polling mode as default for Tegra.  But it'd be appreciated if you can
find the root cause.


thanks,

Takashi

--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card,
 	if (err < 0)
 		return err;
 
+	chip->bus.core.polling = 1;
 	chip->bus.core.needs_damn_long_delay = 1;
 
 	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Jon Hunter]

On 14/07/2020 09:30, Takashi Iwai wrote:
> On Tue, 14 Jul 2020 10:08:02 +0200,
> Jon Hunter wrote:
>>
>> Hi Takashi,
>>
>> On 18/06/2020 15:40, Takashi Iwai wrote:
>>> In the end we already enabled the sync-write mode for most of HD-audio
>>> controllers including Intel, and it's no big merit to keep the async
>>> write mode for the rest.  Let's make it as default and drop the
>>> superfluous AZX_DCAPS_SYNC_WRITE bit flag.
>>>
>>> Also, avoid to set the allow_bus_reset flag, which is a quite unstable
>>> and hackish behavior that was needed only for some early platforms
>>> (decades ago).  The straight fallback to the single cmd mode is more
>>> robust.
>>>
>>> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
>>
>>
>> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2
>> platform. Bisect is pointing to this patch and reverting this does
>> appear to fix it. Interestingly, I am not seeing any problems on other
>> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA
>> which is one different between the other platforms.
>>
>> We can take a closer look at this for Tegra, but I am wondering if we
>> revert this for Tegra for now.
> 
> It's a deja vu, we (or someone else in Nvidia?) discussed it in the
> past?
> 
> The patch below should cure the problem temporarily, as it sets the
> polling mode as default for Tegra.  But it'd be appreciated if you can
> find the root cause.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card,
>  	if (err < 0)
>  		return err;
>  
> +	chip->bus.core.polling = 1;
>  	chip->bus.core.needs_damn_long_delay = 1;
>  
>  	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> 

Did you mean ...

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 0cc5fad1af8a..5637f0129932 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card,
        if (err < 0)
                return err;

+       chip->bus.core.sync_write = 0;
        chip->bus.core.needs_damn_long_delay = 1;
        chip->bus.core.aligned_mmio = 1;

The above works for me.

Cheers
Jon

-- 
nvpublic

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Takashi Iwai]

On Tue, 14 Jul 2020 11:08:18 +0200,
Jon Hunter wrote:
> 
> 
> On 14/07/2020 09:30, Takashi Iwai wrote:
> > On Tue, 14 Jul 2020 10:08:02 +0200,
> > Jon Hunter wrote:
> >>
> >> Hi Takashi,
> >>
> >> On 18/06/2020 15:40, Takashi Iwai wrote:
> >>> In the end we already enabled the sync-write mode for most of HD-audio
> >>> controllers including Intel, and it's no big merit to keep the async
> >>> write mode for the rest.  Let's make it as default and drop the
> >>> superfluous AZX_DCAPS_SYNC_WRITE bit flag.
> >>>
> >>> Also, avoid to set the allow_bus_reset flag, which is a quite unstable
> >>> and hackish behavior that was needed only for some early platforms
> >>> (decades ago).  The straight fallback to the single cmd mode is more
> >>> robust.
> >>>
> >>> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> >>
> >>
> >> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2
> >> platform. Bisect is pointing to this patch and reverting this does
> >> appear to fix it. Interestingly, I am not seeing any problems on other
> >> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA
> >> which is one different between the other platforms.
> >>
> >> We can take a closer look at this for Tegra, but I am wondering if we
> >> revert this for Tegra for now.
> > 
> > It's a deja vu, we (or someone else in Nvidia?) discussed it in the
> > past?
> > 
> > The patch below should cure the problem temporarily, as it sets the
> > polling mode as default for Tegra.  But it'd be appreciated if you can
> > find the root cause.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/pci/hda/hda_tegra.c
> > +++ b/sound/pci/hda/hda_tegra.c
> > @@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	chip->bus.core.polling = 1;
> >  	chip->bus.core.needs_damn_long_delay = 1;
> >  
> >  	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> > 
> 
> Did you mean ...
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index 0cc5fad1af8a..5637f0129932 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card,
>         if (err < 0)
>                 return err;
> 
> +       chip->bus.core.sync_write = 0;
>         chip->bus.core.needs_damn_long_delay = 1;
>         chip->bus.core.aligned_mmio = 1;
> 
> The above works for me.

Yeah, sorry, that was a wrong patch :)
I'm fine for applying it with some more comments.

Care to submit a proper patch?


thanks,

Takashi

Re: [PATCH] ALSA: hda: Enable sync-write operation as default for all controllers

[Jon Hunter]

On 14/07/2020 10:12, Takashi Iwai wrote:

...

>> Did you mean ...
>>
>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>> index 0cc5fad1af8a..5637f0129932 100644
>> --- a/sound/pci/hda/hda_tegra.c
>> +++ b/sound/pci/hda/hda_tegra.c
>> @@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card,
>>         if (err < 0)
>>                 return err;
>>
>> +       chip->bus.core.sync_write = 0;
>>         chip->bus.core.needs_damn_long_delay = 1;
>>         chip->bus.core.aligned_mmio = 1;
>>
>> The above works for me.
> 
> Yeah, sorry, that was a wrong patch :)
> I'm fine for applying it with some more comments.
> 
> Care to submit a proper patch?


No problem. I will submit a patch.

Jon

-- 
nvpublic