Re: Adding movable PCIe BARs support in snd_hda_intel

[Takashi Iwai <tiwai@suse.de>]

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://cgit.freedesktop.org/~agrodzov/linux/log/?h=yadro/pcie_hotplug/movable_bars_v9.1 

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.

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.


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://linuxplumbersconf.org/event/7/contributions/847/attachments/584/1035/lpc2020_sergmir.pdf). 
> >
> > His feature is still not upstream, all his code and mine on top can
> > be seen here - 
> > https://cgit.freedesktop.org/~agrodzov/linux/log/?h=yadro/pcie_hotplug/movable_bars_v9.1 
> >
> > 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://cgit.freedesktop.org/~agrodzov/linux/commit/?h=yadro/pcie_hotplug/movable_bars_v9.1&id=7ec0f60633e898cb941cebb3cd18aae1374fc365 
> >
> > 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@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
>