mbox series

[00/17] ASoC: Intel: AVS - Audio DSP for cAVS

Message ID 20220207122108.3780926-1-cezary.rojewski@intel.com (mailing list archive)
Headers show
Series ASoC: Intel: AVS - Audio DSP for cAVS | expand

Message

Cezary Rojewski Feb. 7, 2022, 12:20 p.m. UTC
A continuation of cleanup work of Intel SST solutions found in
sound/soc/intel/. With two major chapters released last year catpt [1]
and removal of haswell solution [2], time has come for Skylake-driver.

Througout 2019, 2020 and 2021 Skylake-driver has had many fixes applied
and even attempts of refactors as seen in fundamental overhaul [3], IPC
flow adjustments [4] and LARGE_CONFIG overhaul [5] series.
Unfortunately, story repeats itself - problems are found within the core
of a driver. Painting it with different colors does not change the fact
that is it still a house of cards. As changes needed to address those
issues would make Skylake solution incompatible with its previous
revisions, a decision has been made to provide a new solution instead.
In time it would deprecate and replace Skylake-driver.

That solution has been called AVS - from AudioDSP architecture name:
Audio-Voice-Speech. It is meant to provide support for the exact same
range of platforms as its predecessor: SKL, KBL, AML and APL.

Note: this series is dependent upon HDA-series [6] which exposes several
codec-organization functions allowing for reduced code size on
avs-driver side.

Note: this series does not add fully functional driver as its size would
get out of control. Here, focus is put on adding IPC protocol and code
loading code.


Changes RFC v1 [7]: -> v1:
- separated HDA codec-organization patches, path and topology handling,
  PCM and complementary features such as recovery from this series to
  ease the review process
- fixed EXPORT_SYMBOL_GPL for exported members of ASoC framework
- result of stall() is now checked when sending ROM message
- result of snd_hdac_ext_stream_set_spib() is now checked when loading
  basefw
- if basefw is not ready, notification processing is now skipped
- documented several topology parsing helpers


Changes [internal] RFC v2 -> [public] RFC v1:
- dropped any sysfs related changes from this series, moved to follow up
  one
- dropped entire subscription-mechanism found in ipc.c. Handlers that
  are delegated to service certain firmware notifications are now called
  directly
- fixed kernel doc for snd_soc_dapm_new_dai_widgets() as reported by ikp
- prefixed snd_hda_codec_device_init() as suggested by Amadeo
- improved comments for d0ix transitions for APL-based platforms as
  suggested by Pierre
- a ton of spelling related fixes in most of the commit messages
- fixed remaining warnings pointed by scan-build (variable assigned but
  not used)
- replaced most of 'cAVS X.Y' expression usages with 'platform-based'
  equivalents as suggested by Pierre e.g.: cAVS 1.5 -> SKL-based


Changes [internal] RFC v1 -> [internal] RFC v2:

- fixed memleak caused by lack of kfree(vols) if memory allocation fails
  in avs_peakvol_create() as reported by Curtis
- fixed missing 'i' iterator incrementation in avs_widget_ready()
  causing reference loss as reported by Curtis
- replace hardcode: 0x40 usage with snd_hdac_calc_stream_format as
  suggested by Curtis.
  In consequence, readability for all code loading (CL) procedues has
  increased and such approach auto-documents the CL stream preparation

- updated behavior of all index-fetching functions found in utils.c:
  avs_module_entry_index(), avs_module_id_entry_index() and follow ups:
  avs_get_module_entry(), avs_get_module_id_entry() to better conform to
  linux-kernel standard when no entry is found (return -ENOENT) rather
  than C++ standard (return -1, what in kernel case translated to -EPERM)
  as suggested by Curtis and Peter
- several suggestions have been made regarding spacing, and so far, I've
  agreed and applied with all of them. None proposed seemed out of place
  or redundant

- avs_path_stop() renamed to avs_path_pause() pipeline states are
  represented by RESET/PAUSED/RUNNING. avs_path_reset() and
  avs_path_run() were already there and avs_path_stop() just didn't look
  cohesive
- added missing parsers for num_output_pin and num_input_pin which are
  required for custom modules such as WAVES or DSM
- dropped 'priv_param_length' from custom module descriptor as this
  field is obsolete in firmware

- parse_dictionary() has been split into parse_dictionary_header() and
  parse_dictionary_entries() to drop code duplication present in several
  parsing function which could not re-use entire parse_dictionary()
- added avs_tplg_vendor_array_lookup_next() and
  avs_tplg_vendor_entry_next() to drop code duplicated present in several
  parsing functions. This change greatly impacted readability of all
  parsers
- parsing helpers such avs_tplg_vendor_array_lookup() now return offset
  by updating specified in function argument list u32 *offset variable.
  This is to address problem when u32 offset would be greater than max
  int, which is the return type for these functions
- AVS_DEFINE_PTR_PARSER() macro has been introduced to drop code
  duplication for all ptr-parsing users

- all struct avs_path_module creators have had their declaration
  updated: function argument *owner ceased to exist as it could already
  be accessed by mod->owner

- fixed the order of operation for conditional paths (e.g.: echo
  reference) so these are no longer controlled by "source" path and
  instead are impacted by state changes of source and sink paths both.
  Previously only source path e.g. playback sourcing echo reference
  would trigger RUNNING status for conditional path. Equivalent RUNNING
  on WoV path which is in this case sink path, would not do so, leading
  to order-of-operation problems. Behavior has been changed to: both
  source and sink need to be RUNNING for conditional path to be set to
  RUNNING too. PAUSED for either source or sink will cause PAUSED
  transition for conditional path.
- to achieve the above, path states are now saved in 'state' i.e. new
  u32 field for struct avs_path

- resigned from fw_filename field usage in favour of newly added
  tplg_filename for machine board descriptors as suggested by Pierre
- platform descriptor fields have had their names update better reflect
  their purpose as suggested by Pierre
- fixed comp_list missing locking when manipulated
- all message senders now accept request as pointer as suggeseted by
  Peter
- resigned of AZX_ usage for all ADSP-related registers, leaving them
  only for HOST memory space related operations
- fixed disable path for core DSP operations: power/reset/stall as
  reported by Peter

- safety when locking between received responses (reply vs notification)
  has been lowered as suggested by Pierre. Most usages are not performed
  in IRQ context and none is done in hard-IRQ one
- s/master/main/ plus AVS_MAIN_CORE_MASK has replaced ->master_mask
- several functions have had their logging updated - logs have been
  moved to lower level functions as suggested by Pierre
- hdac_ext_stream usage has been streamlined to estream, hdac_streams
  are represented by hstream instead
- hw_params() are resilient to scenarios when they are called mutliple
  times as reported by Pierre
- avs_dsp_enable() now collapses if any of its steps fails as reported
  by Pierre and Peter
- avs_module_ida_empty() now returns value of type bool as suggested by
  Bard


[1]: https://www.spinics.net/lists/alsa-devel/msg116440.html
[2]: https://www.spinics.net/lists/alsa-devel/msg116901.html
[3]: https://www.spinics.net/lists/alsa-devel/msg94199.html
[4]: https://www.spinics.net/lists/alsa-devel/msg92588.html
[5]: https://lore.kernel.org/all/20190808181549.12521-1-cezary.rojewski@intel.com/
[6]: https://lore.kernel.org/alsa-devel/20220207114906.3759800-1-cezary.rojewski@intel.com/T/#t
[7]: https://lore.kernel.org/all/20211208111301.1817725-1-cezary.rojewski@intel.com/


Cezary Rojewski (17):
  ALSA: hda: Add helper macros for DSP capable devices
  ASoC: Export DAI register and widget ctor and dctor functions
  ASoC: Intel: Introduce AVS driver
  ASoC: Intel: avs: Inter process communication
  ASoC: Intel: avs: Add code loading requests
  ASoC: Intel: avs: Add pipeline management requests
  ASoC: Intel: avs: Add module management requests
  ASoC: Intel: avs: Add power management requests
  ASoC: Intel: avs: Add ROM requests
  ASoC: Intel: avs: Add basefw runtime-parameter requests
  ASoC: Intel: avs: Firmware resources management utilities
  ASoC: Intel: avs: Declare module configuration types
  ASoC: Intel: avs: Dynamic firmware resources management
  ASoC: Intel: avs: General code loading flow
  ASoC: Intel: avs: Implement CLDMA transfer
  ASoC: Intel: avs: Code loading over CLDMA
  ASoC: Intel: avs: Code loading over HDA

 include/sound/hdaudio.h         |   2 +
 include/sound/hdaudio_ext.h     |  49 ++
 include/sound/soc-dapm.h        |   1 +
 sound/soc/intel/Kconfig         |  15 +
 sound/soc/intel/Makefile        |   1 +
 sound/soc/intel/avs/Makefile    |   6 +
 sound/soc/intel/avs/avs.h       | 226 ++++++++++
 sound/soc/intel/avs/cldma.c     | 328 ++++++++++++++
 sound/soc/intel/avs/cldma.h     |  29 ++
 sound/soc/intel/avs/core.c      |  62 +++
 sound/soc/intel/avs/dsp.c       | 303 +++++++++++++
 sound/soc/intel/avs/ipc.c       | 410 +++++++++++++++++
 sound/soc/intel/avs/loader.c    | 594 +++++++++++++++++++++++++
 sound/soc/intel/avs/messages.c  | 642 +++++++++++++++++++++++++++
 sound/soc/intel/avs/messages.h  | 762 ++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/registers.h |  75 ++++
 sound/soc/intel/avs/utils.c     | 282 ++++++++++++
 sound/soc/soc-core.c            |   1 +
 sound/soc/soc-dapm.c            |  15 +
 19 files changed, 3803 insertions(+)
 create mode 100644 sound/soc/intel/avs/Makefile
 create mode 100644 sound/soc/intel/avs/avs.h
 create mode 100644 sound/soc/intel/avs/cldma.c
 create mode 100644 sound/soc/intel/avs/cldma.h
 create mode 100644 sound/soc/intel/avs/core.c
 create mode 100644 sound/soc/intel/avs/dsp.c
 create mode 100644 sound/soc/intel/avs/ipc.c
 create mode 100644 sound/soc/intel/avs/loader.c
 create mode 100644 sound/soc/intel/avs/messages.c
 create mode 100644 sound/soc/intel/avs/messages.h
 create mode 100644 sound/soc/intel/avs/registers.h
 create mode 100644 sound/soc/intel/avs/utils.c

Comments

Cezary Rojewski Feb. 21, 2022, 11:51 a.m. UTC | #1
On 2022-02-07 1:20 PM, Cezary Rojewski wrote:
> A continuation of cleanup work of Intel SST solutions found in
> sound/soc/intel/. With two major chapters released last year catpt [1]
> and removal of haswell solution [2], time has come for Skylake-driver.
> 
> Througout 2019, 2020 and 2021 Skylake-driver has had many fixes applied
> and even attempts of refactors as seen in fundamental overhaul [3], IPC
> flow adjustments [4] and LARGE_CONFIG overhaul [5] series.
> Unfortunately, story repeats itself - problems are found within the core
> of a driver. Painting it with different colors does not change the fact
> that is it still a house of cards. As changes needed to address those
> issues would make Skylake solution incompatible with its previous
> revisions, a decision has been made to provide a new solution instead.
> In time it would deprecate and replace Skylake-driver.
> 
> That solution has been called AVS - from AudioDSP architecture name:
> Audio-Voice-Speech. It is meant to provide support for the exact same
> range of platforms as its predecessor: SKL, KBL, AML and APL.
> 
> Note: this series is dependent upon HDA-series [6] which exposes several
> codec-organization functions allowing for reduced code size on
> avs-driver side.


Hello,

Despite HDA-series being updated to v2 [1], no changes are required on 
the side of this series.


Mark,

Should I resend this one regardless of the above?
Also, is there anything else I can help with or explain further 
regarding code-loading and IPC protocol which this series implements?


Regards,
Czarek


[1]: 
https://lore.kernel.org/alsa-devel/20220214101404.4074026-1-cezary.rojewski@intel.com/T/#t

> Note: this series does not add fully functional driver as its size would
> get out of control. Here, focus is put on adding IPC protocol and code
> loading code.

...

> [1]: https://www.spinics.net/lists/alsa-devel/msg116440.html
> [2]: https://www.spinics.net/lists/alsa-devel/msg116901.html
> [3]: https://www.spinics.net/lists/alsa-devel/msg94199.html
> [4]: https://www.spinics.net/lists/alsa-devel/msg92588.html
> [5]: https://lore.kernel.org/all/20190808181549.12521-1-cezary.rojewski@intel.com/
> [6]: https://lore.kernel.org/alsa-devel/20220207114906.3759800-1-cezary.rojewski@intel.com/T/#t
> [7]: https://lore.kernel.org/all/20211208111301.1817725-1-cezary.rojewski@intel.com/
Pierre-Louis Bossart Feb. 25, 2022, 2:35 a.m. UTC | #2
> Note: this series does not add fully functional driver as its size would
> get out of control. Here, focus is put on adding IPC protocol and code
> loading code.

This series is much simpler indeed, see comments in patches, but that
leaves the next steps completely open. It's not quite clear to me how
the previous feedback on trying to up-level the DSP management
functionality might be handled, and if/when you are planning to submit
follow-up patchsets that would implement the required functionality to
at least match what the Skylake driver can do today.

To repeat my previous points: the existing DPCM FE/BE split does not
even being to represent how a DSP might be handled. The BE typically
represents a physical DAI connected to a codec, and the FE pretty much
everything else between a host DMA and the DAI. All the internal format
conversions, mixers and processing are not really represented other than
with DAPM logical widgets, and that's a big miss. There's room for a lot
of improvements that would be of interest to all DSP users.
Cezary Rojewski Feb. 25, 2022, 3:44 p.m. UTC | #3
On 2022-02-25 3:35 AM, Pierre-Louis Bossart wrote:
> 
>> Note: this series does not add fully functional driver as its size would
>> get out of control. Here, focus is put on adding IPC protocol and code
>> loading code.
> 
> This series is much simpler indeed, see comments in patches, but that
> leaves the next steps completely open. It's not quite clear to me how
> the previous feedback on trying to up-level the DSP management
> functionality might be handled, and if/when you are planning to submit
> follow-up patchsets that would implement the required functionality to
> at least match what the Skylake driver can do today.
> 
> To repeat my previous points: the existing DPCM FE/BE split does not
> even being to represent how a DSP might be handled. The BE typically
> represents a physical DAI connected to a codec, and the FE pretty much
> everything else between a host DMA and the DAI. All the internal format
> conversions, mixers and processing are not really represented other than
> with DAPM logical widgets, and that's a big miss. There's room for a lot
> of improvements that would be of interest to all DSP users.


Thanks for taking time to provide round of review for the series!

The request was to split the initial series into smaller chunks and 
separate the driver-specific stuff from parts that _could_ get 
incorporated into the framework to level it up in regard to DSP 
management. Series: "Intel: avs: Topology and path management" [1] has 
been provided for such discussion.

Given the request, we are planning to upstream avs-driver in four chunks:
- IPC protocol and code loading (this one)
- Topology and path management [1]
- secondary flows e.g.: DSP recovery
- machine boards


In regard to DPCM FE/BE, ASoC already has DAI-link components: let codec 
operations stay with codec component, leaving DSP related operations as 
platform component responsibility. FE for DSP drivers typically comes 
from topology and drives the HOST DMA part whereas BE deals with LINK 
(hardware, data transfer interface such as PDM or I2S) side, including 
its configuration.
I'm happy to continue the discussion regarding "path" in the dedicated 
series [1] as current series covers IPC protocol and code loading -only.


Regards,
Czarek


[1]: 
https://lore.kernel.org/alsa-devel/20220207132532.3782412-1-cezary.rojewski@intel.com/T/#t
Pierre-Louis Bossart Feb. 25, 2022, 4:33 p.m. UTC | #4
> The request was to split the initial series into smaller chunks and
> separate the driver-specific stuff from parts that _could_ get
> incorporated into the framework to level it up in regard to DSP
> management. Series: "Intel: avs: Topology and path management" [1] has
> been provided for such discussion.
> 
> Given the request, we are planning to upstream avs-driver in four chunks:
> - IPC protocol and code loading (this one)
> - Topology and path management [1]
> - secondary flows e.g.: DSP recovery
> - machine boards
> 
> 
> In regard to DPCM FE/BE, ASoC already has DAI-link components: let codec
> operations stay with codec component, leaving DSP related operations as
> platform component responsibility. FE for DSP drivers typically comes
> from topology and drives the HOST DMA part whereas BE deals with LINK
> (hardware, data transfer interface such as PDM or I2S) side, including
> its configuration.

I respectfully disagree with your analysis, we cannot dissociate DSP and
link management. The intersection between the BE dailink handling and
the DSP management is the configuration of the cpu-dai on the host side.

When the DSP firmware programs the DAI registers, as we do on the Intel
side for SSP, DMIC and ALH/SoundWire, then the format information needs
to be exposed back to the DSP platform driver so that the codec can be
informed of the configuration. Most interfaces can support multiple
formats, and currently we don't have a good way to know what the
firmware changes and how to match PCM hw_params with link configuration.

The current work-around we use is to rely on the dailink fixup to force
the dailink to operate at a rate consistent with the topology, but
that's really not good at all. What would be needed is that all format
changes through the DSP graph are propagated all the way to the DAI and
used for the dailink configuration. That would also enable us to remove
unnecessary SRCs or format conversions, which I believe is a capability
at the heart of your AVS path proposal.

That's really my point, you cannot really think of DSP management
without factoring in DPCM.

It's not just me blabbering into the wind btw, others have voiced the
need to improve FE->BE format handling and add constraints, see

https://lore.kernel.org/alsa-devel/20210323114327.3969072-1-codrin.ciubotariu@microchip.com/

> I'm happy to continue the discussion regarding "path" in the dedicated
> series [1] as current series covers IPC protocol and code loading -only.

this RFC series was not mentioned in the cover letter for this patchset,
so it wouldn't be surprising if others also missed the connection.

> [1]:
> https://lore.kernel.org/alsa-devel/20220207132532.3782412-1-cezary.rojewski@intel.com/T/#t
Mark Brown Feb. 25, 2022, 6:07 p.m. UTC | #5
On Thu, Feb 24, 2022 at 08:35:50PM -0600, Pierre-Louis Bossart wrote:

> leaves the next steps completely open. It's not quite clear to me how
> the previous feedback on trying to up-level the DSP management
> functionality might be handled, and if/when you are planning to submit
> follow-up patchsets that would implement the required functionality to
> at least match what the Skylake driver can do today.

I think it's fine that none of the complicated stuff is considered here,
one of the objectives with splitting things up into multiple serieses is
to ensure that the simpler stuff doesn't obscure the bits that need more
attention paying to them.