Re: [RFC PATCH 0/7] adapt SOF to use snd-hda-codec-hdmi

[Takashi Iwai <tiwai@suse.de>]

On Thu, 29 Aug 2019 15:53:41 +0200,
Kai Vehmanen wrote:
> 
> Hi all,
> here's a RFC patch series that adapts SOF (and one example machine
> driver) to use snd-hda-codec-hdmi (patch_hdmi.c) codec driver
> instead of hdac_hdmi (soc/codecs/hdac_hdmi.c). The primary goal
> is unify the HDMI codec implementation between DSP and non-DSP HDA
> configurations, offer same interface to user-space and reduce
> maintenance load for all.
> 
> Main points I'd like your input on:
> 
> 1) Is the high-level approach ok?
>  - SOF already uses pci/hda/ for all others codecs, so HDMI
>    has been the sole exception where code has been duplicated.
>  - I've tried to keep changes to hda/hdmi minimal.
>  - This series implements logic to parse the PCM topology
>    in a dynamic fashion, so we do not have to change all
>    existing (and future) DSP topologis to use fixed PCM device
>    numbers for HDMI, and we avoid need to hardcode PCM
>    device numbers in machine driver code.
> 
> 2) Can we drop hdac_hdmi and its support from machine drivers, or
>    do we need to make it optional and keep it around?
> 
>  - Current series does not add any Kconfig options, but
>    simply switches SOF to use HDA codecs for all, including
>    HDMI/DP. This means hdac_hdmi is never used with SOF
>    and could be dropped (if SST is ok as well).
>  - This may break some usage with SST (input is welcome!)
>  - The change is visible to applications. The ALSA mixer
>    interface is different (OTOH with the new driver, playback
>    works out-of-the-box while with hdac_hdmi you needed to
>    set the multiple controls first, to have any audio out).
>  - Alternatively I could add a KConfig option and we could
>    have a deprecation period for hdac_hdmi, allowing people
>    to compile with the old driver during transition time.
>    This will require #ifdef'ing in all the machine drivers.
> 
> .. once these are addressed, I can proceed to extend the patchset
> with all affected machine drivers.
> 
> Feature and testing info:
> 
>  - Tested on multiple Intel platforms supported by SOF.
>  - Tested with ALSA console tools as well as with Pulseaudio.
>       - requires Pulseaudio 12.x or newer, see
>         https://lists.freedesktop.org/archives/pulseaudio-discuss/2019-August/031358.html
>  - HDMI, DP, DP-MST with multi-monitor use-scenarios work ok.
>  - New feature for SOF: ELD /proc fs works just like in
>    DSP-less mode.
>  - New feature for SOF: jack detection works out-of-the-box
>    with Pulseaudio (no need for card specific UCM for HDMI)
>  - Pre-reviewed at:
>    https://github.com/thesofproject/linux/pull/1155

IMO, the only and the most important point is whether it works as-is
without changing the existing user-space, or exactly what scenario
would be broken.  If the breakage is significant, we may introduce a
Kconfig, as you suggested.

I don't think the mixer contents change are problematic.  In the case
of HDMI/DP, it's mostly read-only for fetching ELD or jack state.

Other than that, I like the idea, the code change looks simple
enough, and it'd make maintenance easier.


thanks,

Takashi

> 
> Kai Vehmanen (7):
>   ALSA: hda - add mst_no_extra_pcms flag to hda_codec
>   ASoC: Intel: skl-hda-dsp-generic: use snd-hda-codec-hdmi
>   ASoC: hdac_hda: add support for HDMI/DP as a HDA codec
>   ALSA: hda/hdmi - allow control creation without a linked pcm
>   ALSA: hda/hdmi - implement mst_no_extra_pcms flag
>   ALSA: hda/hdmi - complete pcm_setup_pin without snd_pcm link
>   ASoC: SOF: Intel: load hda codec module also for HDMI/DP
> 
>  include/sound/hda_codec.h                    |  1 +
>  sound/pci/hda/patch_hdmi.c                   | 31 ++++---
>  sound/soc/codecs/hdac_hda.c                  | 95 +++++++++++++++++---
>  sound/soc/codecs/hdac_hda.h                  | 10 ++-
>  sound/soc/intel/boards/skl_hda_dsp_common.c  | 27 ++----
>  sound/soc/intel/boards/skl_hda_dsp_common.h  | 61 +++++++++++++
>  sound/soc/intel/boards/skl_hda_dsp_generic.c |  7 --
>  sound/soc/sof/intel/hda-codec.c              | 11 +--
>  sound/soc/sof/intel/hda.h                    |  5 +-
>  9 files changed, 189 insertions(+), 59 deletions(-)
> 
> -- 
> 2.17.1
>