| Message ID | 20230926080623.43927-1-cezary.rojewski@intel.com |
|---|---|
| Headers | show |
| Series | ALSA: hda: Abstract and update HOST-stream setup procedure | expand |
On 2023-09-26 10:06 AM, Cezary Rojewski wrote: > Software shall read SDxFIFOS calculated by the hardware and notify if > invalid value is programmed before continuing the stream preparation. ... > @@ -300,6 +302,12 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) > /* set the interrupt enable bits in the descriptor control register */ > snd_hdac_stream_updatel(azx_dev, SD_CTL, 0, SD_INT_MASK); > > + /* Once SDxFMT is set, the controller programs SDxFIFOS to non-zero value. */ > + ret = snd_hdac_stream_readw_poll(azx_dev, SD_FIFOSIZE, reg, reg & AZX_SD_FIFOSIZE_MASK, > + 3, 300); > + if (ret) > + dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n", > + AZX_REG_SD_FIFOSIZE, ret); There is one (negligible?) side effect. AudioDSP firmware is the one who kicks SDxFIFOS calculation when a stream is decoupled mode. During firmware bring up procedure, there is no firmware running _and_ the code-loading stream is always a decoupled one. So, there is none to trigger the calculation and we end up with debug -110 messages. It looks like to do this in complete fashion some refactoring is needed in hdac_stream.c/hdac_ext_stream.c. Czarek > azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE) + 1; > > /* when LPIB delay correction gives a small negative value,
On Tue, 26 Sep 2023 10:06:19 +0200, Cezary Rojewski wrote: > > The patchset targets two intertwined topics: > > The driver shall poll SDxFIFOS to ensure a valid value is set by the > controller after programming SDxFMT. Due to amount of users and > limited-number of configuration available in our CI when compared to > overall possibilities on the market, the check is non-blocking. > > Second topic relates to stream setup procedure. The procedure differs > between HDAudio controller device revisions. Right now those differences > are handled directly by a platform driver. Existing top-level > 'if (pci->device == APL)' could be replaced by a abstraction in lower > parts of the code instead. > > With that done, the two users are updated accordingly. In avs-driver > case, this updates the flow to the recommended one. > > Changes in v3: > - fixed issues pointed out by scripts/kernel-doc > > Changes in v2: > - fixed ->host_setup assignment in patch 02/04 > > Cezary Rojewski (4): > ALSA: hda: Poll SDxFIFOS after programming SDxFMT > ALSA: hda: Introduce HOST stream setup mechanism > ASoC: Intel: avs: Use helper to setup HOST stream > ASoC: Intel: Skylake: Use helper to setup HOST stream Sorry for the late reaction, as I've been (still) off since the last week. Now applied now to for-next branch. thanks, Takashi
On 2023-10-06 10:44 AM, Takashi Iwai wrote: > On Tue, 26 Sep 2023 10:06:19 +0200, > Cezary Rojewski wrote: >> >> The patchset targets two intertwined topics: >> >> The driver shall poll SDxFIFOS to ensure a valid value is set by the >> controller after programming SDxFMT. Due to amount of users and >> limited-number of configuration available in our CI when compared to >> overall possibilities on the market, the check is non-blocking. >> >> Second topic relates to stream setup procedure. The procedure differs >> between HDAudio controller device revisions. Right now those differences >> are handled directly by a platform driver. Existing top-level >> 'if (pci->device == APL)' could be replaced by a abstraction in lower >> parts of the code instead. >> >> With that done, the two users are updated accordingly. In avs-driver >> case, this updates the flow to the recommended one. >> >> Changes in v3: >> - fixed issues pointed out by scripts/kernel-doc >> >> Changes in v2: >> - fixed ->host_setup assignment in patch 02/04 >> >> Cezary Rojewski (4): >> ALSA: hda: Poll SDxFIFOS after programming SDxFMT >> ALSA: hda: Introduce HOST stream setup mechanism >> ASoC: Intel: avs: Use helper to setup HOST stream >> ASoC: Intel: Skylake: Use helper to setup HOST stream > > Sorry for the late reaction, as I've been (still) off since the last > week. > > Now applied now to for-next branch. Hello Takashi, Now I'm the one sorry for the late replay - I've found two new things when fixing the debug message (reported by me in patch 1/4). - null-ptr-deref when assigning hdac_stream (when type=COUPLED) - azx_dev->fifo_size is initialized incorrectly It's good to debug things, you never know what you may find! May I send v4 as a collective update? It would address the two above and the false-positive debug message that appears when code-loading AudioDSP firmware. Kind regards, Czarek
On Fri, 06 Oct 2023 11:01:44 +0200, Cezary Rojewski wrote: > > On 2023-10-06 10:44 AM, Takashi Iwai wrote: > > On Tue, 26 Sep 2023 10:06:19 +0200, > > Cezary Rojewski wrote: > >> > >> The patchset targets two intertwined topics: > >> > >> The driver shall poll SDxFIFOS to ensure a valid value is set by the > >> controller after programming SDxFMT. Due to amount of users and > >> limited-number of configuration available in our CI when compared to > >> overall possibilities on the market, the check is non-blocking. > >> > >> Second topic relates to stream setup procedure. The procedure differs > >> between HDAudio controller device revisions. Right now those differences > >> are handled directly by a platform driver. Existing top-level > >> 'if (pci->device == APL)' could be replaced by a abstraction in lower > >> parts of the code instead. > >> > >> With that done, the two users are updated accordingly. In avs-driver > >> case, this updates the flow to the recommended one. > >> > >> Changes in v3: > >> - fixed issues pointed out by scripts/kernel-doc > >> > >> Changes in v2: > >> - fixed ->host_setup assignment in patch 02/04 > >> > >> Cezary Rojewski (4): > >> ALSA: hda: Poll SDxFIFOS after programming SDxFMT > >> ALSA: hda: Introduce HOST stream setup mechanism > >> ASoC: Intel: avs: Use helper to setup HOST stream > >> ASoC: Intel: Skylake: Use helper to setup HOST stream > > > > Sorry for the late reaction, as I've been (still) off since the last > > week. > > > > Now applied now to for-next branch. > > Hello Takashi, > > Now I'm the one sorry for the late replay - I've found two new things > when fixing the debug message (reported by me in patch 1/4). > - null-ptr-deref when assigning hdac_stream (when type=COUPLED) > - azx_dev->fifo_size is initialized incorrectly > > It's good to debug things, you never know what you may find! > > May I send v4 as a collective update? It would address the two above > and the false-positive debug message that appears when code-loading > AudioDSP firmware. As other patches have been already applied, could you submit the incremental fixes on top of v3 instead? You can see the latest state in for-next branch of sound.git tree. thanks, Takashi