Adding movable PCIe BARs support in snd_hda_intel

[Andrey Grodzovsky]

On 2021-03-23 7:39 a.m., Takashi Iwai wrote:
> On Tue, 23 Mar 2021 12:23:16 +0100,
> Andrey Grodzovsky wrote:
>>
>> Just an update, i found the issue which was actually to wake up the HW
>> before doing stop/restart (using pm_runtime_get_sync), also handled
>> protecting from concurrent snd_pcm_ioctls accessing the registers
>> while the BAR is unmapped. Can go through BAR move while aplay is
>> running now.
>>
>> Once again, would be happy for any comments on the code -
>> https://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.freedesktop.org%2F~agrodzov%2Flinux%2Flog%2F%3Fh%3Dyadro%2Fpcie_hotplug%2Fmovable_bars_v9.1&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C600bb8a982f54a2674d508d8edf045ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637520963652402231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QKlK45vaajKVF%2FXhLIWMhN%2FhGxTIMG%2FG0Y8xNC37LPw%3D&reserved=0
> 
> Hrm, this kind of change is a bit scary; we have some mechanism to
> stop the HD-audio hardware operation (e.g. for loading the DSP for
> CA0132 codec), but it's pretty hackish and error-prone.

At least I didn't have issue with HW stopping, the problem is with
unampping hdac_bus->remap_addr - as long as it's not iorampped
back no code besides the PCI BAR move should run at all as any register
access from this address will cause fatal page fault. I took care
of most of the IOCTls i think, the interrupts are disabled in azx_stop_chip
but I am probably still missing some stuff like some background work
items or other IOCTls code path.

> 
> Do we really need the BAR remap while operating the sound streaming?
> That is, can't we simply refuse the remap if it's in operation?
> If it can be refused, we may avoid a big locking or such.

Problem here is that this feature is for the sake of hot-plug of a 
device, this supposed to give enough MMIO space to place all the BARs
of newly plugging in device. Refusing to move BARs because there is some
app using the sound card on the background might then lead to failure of
plugging in the new device and this doesn't sound as a reasonable
behavior to me. Also note that the lock's impact is minimal as it's
read side lock only in the IOCTLs an so as long as hot plug not
taking place it has no impact.

Andrey

> 
> 
> thanks,
> 
> Takashi
> 
>>
>>
>> Andrey
>>
>> On 2021-03-19 5:22 p.m., Andrey Grodzovsky wrote:
>>> Hi, I am working on adding AMD related drivers support for PCIe BARs
>>> move feature developed by Sergei
>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flinuxplumbersconf.org%2Fevent%2F7%2Fcontributions%2F847%2Fattachments%2F584%2F1035%2Flpc2020_sergmir.pdf&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C600bb8a982f54a2674d508d8edf045ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637520963652402231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fegwYaNiLmfZWWmuaYXU5gcJsJ%2BwcEwry4FoI1eyiK8%3D&reserved=0).
>>>
>>> His feature is still not upstream, all his code and mine on top can
>>> be seen here -
>>> https://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.freedesktop.org%2F~agrodzov%2Flinux%2Flog%2F%3Fh%3Dyadro%2Fpcie_hotplug%2Fmovable_bars_v9.1&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C600bb8a982f54a2674d508d8edf045ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637520963652402231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QKlK45vaajKVF%2FXhLIWMhN%2FhGxTIMG%2FG0Y8xNC37LPw%3D&reserved=0
>>>
>>> I did basic implementation fro amdgpu driver and now I am doing the
>>> same for snd_hda_intel to support our on GPU Azalia audio
>>> chips. This relevant commit is here -
>>> https://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.freedesktop.org%2F~agrodzov%2Flinux%2Fcommit%2F%3Fh%3Dyadro%2Fpcie_hotplug%2Fmovable_bars_v9.1%26id%3D7ec0f60633e898cb941cebb3cd18aae1374fc365&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C600bb8a982f54a2674d508d8edf045ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637520963652402231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pk8qVsTWuGLpE5js1oVx8swM8zK2Jh3uJwuTWJ7ixlM%3D&reserved=0
>>>
>>> Any driver that wants to support movable BARs needs to implement
>>> rescan_prepare, rescan_done and bar_fixed callbacks where
>>> rescan_prepare needs to stop HW/SW and unamp all MMIO mappings and
>>> rescan_done needs to ioremap the BAR from it's new MMIO location and
>>> restart HW/SW.
>>>
>>> I am able currently to trigger BARs move by sysfs using
>>> "/sys/bus/pci/rescan" and the driver will go through the sequence I
>>> described above without hangs. Problem is that after this when i try
>>> to use mplayer I am getting following errors:
>>>
>>> andrey at andrey-test:~$ sudo mplayer -ao alsa:device=hw=0.3
>>> Downloads/file_example_MP3_5MG.mp3
>>> MPlayer 1.3.0 (Debian), built with gcc-9 (C) 2000-2016 MPlayer Team
>>> do_connect: could not connect to socket
>>> connect: No such file or directory
>>> Failed to open LIRC support. You will not be able to use your remote
>>> control.
>>>
>>> Playing Downloads/file_example_MP3_5MG.mp3.
>>> libavformat version 58.29.100 (external)
>>> Audio only file format detected.
>>> Load subtitles in Downloads/
>>> ==========================================================================
>>> Opening audio decoder: [mpg123] MPEG 1.0/2.0/2.5 layers I, II, III
>>> AUDIO: 44100 Hz, 2 ch, s16le, 320.0 kbit/22.68% (ratio: 40000->176400)
>>> Selected audio codec: [mpg123] afm: mpg123 (MPEG 1.0/2.0/2.5 layers
>>> I, II, III)
>>> ==========================================================================
>>> AO: [alsa] 44100Hz 2ch s16le (2 bytes per sample)
>>> Video: no video
>>> Starting playback...
>>> A:   0.1 (00.0) of 132.0 (02:12.0) ??,?%
>>> Audio device got stuck!
>>>
>>> and in dmesg I see
>>> [  365.355518] snd_hda_codec_hdmi hdaudioC0D0: Unable to sync
>>> register 0x2f0d00. -5
>>>
>>> Also I see 296.619014] snd_hda_intel 0000:0a:00.1: CORB reset
>>> timeout#2, CORBRP = 65535 error during the rescan_done callback
>>> execution
>>>
>>> I know it has to do with the move of BAR's MMIO address because when i
>>> disallow BAR migration by returning true from bar_fixed callback I
>>> have no such errors and mplayer works fine.
>>>
>>> I enabled MMIO trace and didn't see that post BAR move there is a
>>> wrong MMIO access - all of them are from the new MMIO base offset -
>>> 0xfcd80000 (trace attached including mmio trace and dmesg)
>>>
>>> I would be happy for any idea on this and any comment on the
>>> correctness of my sequence in the code
>>>
>>> Andrey
>>