[alsa-devel] [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM
Takashi Iwai
tiwai at suse.de
Tue Nov 3 17:44:15 CET 2015
On Tue, 03 Nov 2015 09:22:53 +0100,
libin.yang at linux.intel.com wrote:
>
> From: Libin Yang <libin.yang at intel.com>
>
> Allocate the PCM based on the number of pin and device entry.
> The number of PCM is pin num plus device entry number per pin.
> For example, on Intel platform, it will be 5 PCMs.
>
> Do not attach the PCM to pin in initialization.
> Dynamically attach PCM to pin when monitor is connected
> in HDMI audio codec driver.
>
> When monitor is disconnected, detach the PCM from the pin.
> And if the audio is playing, stop the playback.
>
> The below is the example of device entry and PCM binding:
> For a monitor is plugged in, we need to dynamically assign
> this monitor to 5 PCM devices (on Intel platform):
> For a monitor at pin nid 0x05, dev index 0, it will prefer PCM 3
> For a monitor at pin nid 0x06, dev index 0, it will prefer PCM 7
> For a monitor at pin nid 0x07, dev index 0, it will prefer PCM 8
> For a monitor at dev index 1 (any pin), it will prefer PCM 9
> For a monitor at dev index 2 (any pin), it will prefer PCM 10
>
> If the preferred PCM is not available, try PCM in this order:
> 9, 10, 3, 7 ,8.
>
> Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
We should split the changes to a few patches here. For example,
stopping the stream at discussion is basically an implementation
independent from the MST itself. IOW, the dynamic attach/detach can
be implemented and tested even without MST support. Let's code them
at first, then add MST support.
Also, it's an open question whether we apply the dynamic attach/detach
to all devices that use the generic hdmi framework. It means
including AMD and Nvidia. You hard-coded it to be applied
unconditionally. Would it break anything potentially...?
Some more comments below.
> ---
> sound/pci/hda/hda_codec.c | 5 +-
> sound/pci/hda/hda_codec.h | 1 +
> sound/pci/hda/patch_hdmi.c | 170 +++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 162 insertions(+), 14 deletions(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 8374188..e1d4648 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -313,7 +313,7 @@ EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);
>
>
> /* return DEVLIST_LEN parameter of the given widget */
> -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
> +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid)
> {
> unsigned int wcaps = get_wcaps(codec, nid);
> unsigned int parm;
> @@ -327,6 +327,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
> parm = 0;
> return parm & AC_DEV_LIST_LEN_MASK;
> }
> +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices);
Once when exporting, add the proper kernel doc comment, too.
I guess this can be split as a preparation patch, too.
> /**
> * snd_hda_get_devices - copy device list without cache
> @@ -344,7 +345,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
> unsigned int parm;
> int i, dev_len, devices;
>
> - parm = get_num_devices(codec, nid);
> + parm = snd_hda_get_num_devices(codec, nid);
> if (!parm) /* not multi-stream capable */
> return 0;
>
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 373fcad..ad4da41 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -347,6 +347,7 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums,
> const hda_nid_t *list);
> int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux,
> hda_nid_t nid, int recursive);
> +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid);
> int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
> u8 *dev_list, int max_devices);
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index f503a88..8d31366 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -72,6 +72,9 @@ struct hdmi_spec_per_cvt {
>
> struct hdmi_spec_per_pin {
> hda_nid_t pin_nid;
> + hda_dev_t dev_id;
> + /* real pin idx. device entries on same pin share same pin_nid_idx */
> + int pin_nid_idx;
> int num_mux_nids;
> hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> int mux_idx;
> @@ -82,6 +85,9 @@ struct hdmi_spec_per_pin {
> struct mutex lock;
> struct delayed_work work;
> struct snd_kcontrol *eld_ctl;
> + struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] */
> + int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */
> + bool pcm_detaching;
> int repoll_count;
> bool setup; /* the stream has been set up by prepare callback */
> int channels; /* current number of channels */
> @@ -134,6 +140,10 @@ struct hdmi_spec {
> int num_pins;
> struct snd_array pins; /* struct hdmi_spec_per_pin */
> struct hda_pcm *pcm_rec[16];
> + unsigned long pcm_bitmap;
> + struct mutex pcm_lock;
> + int pcm_used; /* counter of pcm_rec[] */
> + int dev_num;
> unsigned int channels_max; /* max over all cvts */
>
> struct hdmi_eld temp_eld;
> @@ -378,12 +388,18 @@ static int hinfo_to_pin_index(struct hda_codec *codec,
> struct hda_pcm_stream *hinfo)
> {
> struct hdmi_spec *spec = codec->spec;
> + struct hdmi_spec_per_pin *per_pin;
> int pin_idx;
>
> - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
> - if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
> + mutex_lock(&spec->pcm_lock);
> + for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> + per_pin = get_pin(spec, pin_idx);
> + if (per_pin->pcm && per_pin->pcm->stream == hinfo) {
> + mutex_unlock(&spec->pcm_lock);
> return pin_idx;
> -
> + }
> + }
> + mutex_unlock(&spec->pcm_lock);
> codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo);
> return -EINVAL;
> }
> @@ -1451,7 +1467,8 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>
> /* Validate hinfo */
> pin_idx = hinfo_to_pin_index(codec, hinfo);
> - if (snd_BUG_ON(pin_idx < 0))
> + /* if no pin is attached, return fail */
> + if (pin_idx < 0)
> return -EINVAL;
> per_pin = get_pin(spec, pin_idx);
> eld = &per_pin->sink_eld;
> @@ -1529,6 +1546,102 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
> return 0;
> }
>
> +static int get_preferred_pcm_slot(struct hdmi_spec *spec,
> + struct hdmi_spec_per_pin *per_pin)
> +{
> + if (per_pin->dev_id == 0) {
> + if ((spec->pcm_bitmap & (1 << per_pin->pin_nid_idx)) == 0)
> + return per_pin->pin_nid_idx;
> + }
> + return -1;
> +}
> +
> +static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
> + struct hdmi_spec_per_pin *per_pin)
> +{
> + int slot;
> + int slot_offset;
> + int i;
> +
> + /* try the prefer PCM */
> + slot = get_preferred_pcm_slot(spec, per_pin);
> + if (slot != -1)
> + return slot;
> +
> + /* have a second try, try backup PCMs from specified offset */
> + if (per_pin->dev_id == 0)
> + slot_offset = 0;
> + else
> + slot_offset = per_pin->dev_id - 1;
> + for (i = spec->num_pins + slot_offset; i < spec->pcm_used; i++) {
> + if ((spec->pcm_bitmap & (1 << i)) == 0)
> + return i;
> + }
IMO, this step can be skipped and just check through the backup PCMs.
> +
> + /* have a third try, try all backup PCMs */
> + for (i = spec->num_pins; i < spec->pcm_used; i++) {
> + if ((spec->pcm_bitmap & (1 << i)) == 0)
> + return i;
> + }
> +
> + /* the last try, try all PCMs */
> + for (i = 0; i < spec->pcm_used; i++) {
> + if ((spec->pcm_bitmap & (1 << i)) == 0)
> + return i;
> + }
> + return -ENODEV;
> +}
> +
> +static void hdmi_attach_hda_pcm(struct hdmi_spec *spec,
> + struct hdmi_spec_per_pin *per_pin)
> +{
> + int idx;
> +
> + /* pcm already be attached to the pin */
> + if (per_pin->pcm)
> + return;
> + mutex_lock(&spec->pcm_lock);
> + idx = hdmi_find_pcm_slot(spec, per_pin);
> + if (idx == -ENODEV) {
> + mutex_unlock(&spec->pcm_lock);
> + return;
> + }
> + per_pin->pcm_idx = idx;
> + per_pin->pcm = spec->pcm_rec[idx];
> + set_bit(idx, &spec->pcm_bitmap);
> + mutex_unlock(&spec->pcm_lock);
> +}
> +
> +static void hdmi_detach_hda_pcm(struct hdmi_spec *spec,
> + struct hdmi_spec_per_pin *per_pin)
> +{
> + int idx;
> + struct snd_pcm_substream *substream;
> + /* pcm already be detached from the pin */
> + if (!per_pin->pcm)
> + return;
> + mutex_lock(&spec->pcm_lock);
> + idx = per_pin->pcm_idx;
> + per_pin->pcm_idx = -1;
> + /* only for playback */
> + substream = per_pin->pcm->pcm->streams[0].substream;
> + clear_bit(idx, &spec->pcm_bitmap);
> +
> + if (substream && substream->runtime &&
> + snd_pcm_running(substream)) {
Maybe better to protect this in snd_pcm_stream_lock_irq() before.
The check itself seems racy.
> + codec_warn(per_pin->codec,
> + "HDMI: monitor disconnected, try to stop playback\n");
This shouldn't be a warning. It's the very normal behavior that user
unplug HDMI while playing. At most, it's a debug message.
> + per_pin->pcm_detaching = true;
> + mutex_unlock(&spec->pcm_lock);
> + snd_pcm_stream_lock_irq(substream);
> + snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
> + snd_pcm_stream_unlock_irq(substream);
> + } else {
> + per_pin->pcm = NULL;
> + mutex_unlock(&spec->pcm_lock);
> + }
> +}
> +
> static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> {
> struct hda_jack_tbl *jack;
> @@ -1586,6 +1699,12 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> }
> }
>
> + /* need be protected by spec->lock */
> + if (eld->eld_valid)
> + hdmi_attach_hda_pcm(spec, per_pin);
> + else
> + hdmi_detach_hda_pcm(spec, per_pin);
> +
> if (pin_eld->eld_valid != eld->eld_valid)
> eld_changed = true;
>
> @@ -1662,6 +1781,7 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> int pin_idx;
> struct hdmi_spec_per_pin *per_pin;
> int err;
> + int dev_num;
>
> caps = snd_hda_query_pin_caps(codec, pin_nid);
> if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))
> @@ -1671,8 +1791,14 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> if (get_defcfg_connect(config) == AC_JACK_PORT_NONE)
> return 0;
>
> - if (is_haswell_plus(codec))
> + if (is_haswell_plus(codec)) {
> intel_haswell_fixup_connect_list(codec, pin_nid);
> + spec->dev_num = 3;
> + } else {
> + dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;
> + spec->dev_num = (spec->dev_num > dev_num) ?
> + spec->dev_num : dev_num;
Is this correct? I thought *_get_num_devices() returns the number of
currently plugged devices. Or does it return the max number of
devices for the pin?
Takashi
> + }
>
> pin_idx = spec->num_pins;
> per_pin = snd_array_new(&spec->pins);
> @@ -1680,6 +1806,9 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> return -ENOMEM;
>
> per_pin->pin_nid = pin_nid;
> + per_pin->pin_nid_idx = pin_idx; /* to be fixed for mst audio */
> + per_pin->dev_id = 0; /* Not support MST audio so far */
> + per_pin->pcm_idx = -1;
> per_pin->non_pcm = false;
>
> err = hdmi_read_pin_conn(codec, pin_idx);
> @@ -1799,13 +1928,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> hda_nid_t cvt_nid = hinfo->nid;
> struct hdmi_spec *spec = codec->spec;
> int pin_idx = hinfo_to_pin_index(codec, hinfo);
> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> - hda_nid_t pin_nid = per_pin->pin_nid;
> + struct hdmi_spec_per_pin *per_pin;
> + hda_nid_t pin_nid;
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct i915_audio_component *acomp = codec->bus->core.audio_component;
> bool non_pcm;
> int pinctl;
>
> + /* no pin is attached */
> + if (pin_idx < 0)
> + return -EINVAL;
> + per_pin = get_pin(spec, pin_idx);
> + pin_nid = per_pin->pin_nid;
> +
> if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> /* Verify pin:cvt selections to avoid silent audio after S3.
> * After S3, the audio driver restores pin:cvt selections
> @@ -1877,6 +2012,12 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
> if (snd_BUG_ON(pin_idx < 0))
> return -EINVAL;
> per_pin = get_pin(spec, pin_idx);
> + if (per_pin->pcm_detaching) {
> + mutex_lock(&spec->pcm_lock);
> + per_pin->pcm = NULL;
> + per_pin->pcm_detaching = false;
> + mutex_unlock(&spec->pcm_lock);
> + }
>
> if (spec->dyn_pin_out) {
> pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
> @@ -1896,7 +2037,6 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
> per_pin->channels = 0;
> mutex_unlock(&per_pin->lock);
> }
> -
> return 0;
> }
>
> @@ -2068,22 +2208,26 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol,
> static int generic_hdmi_build_pcms(struct hda_codec *codec)
> {
> struct hdmi_spec *spec = codec->spec;
> - int pin_idx;
> + int idx;
>
> - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> + for (idx = 0; idx < spec->num_pins + spec->dev_num - 1; idx++) {
> struct hda_pcm *info;
> struct hda_pcm_stream *pstr;
>
> - info = snd_hda_codec_pcm_new(codec, "HDMI %d", pin_idx);
> + info = snd_hda_codec_pcm_new(codec, "HDMI %d", idx);
> if (!info)
> return -ENOMEM;
> - spec->pcm_rec[pin_idx] = info;
> + spec->pcm_rec[idx] = info;
> + spec->pcm_used++;
> info->pcm_type = HDA_PCM_TYPE_HDMI;
> info->own_chmap = true;
>
> pstr = &info->stream[SNDRV_PCM_STREAM_PLAYBACK];
> pstr->substreams = 1;
> pstr->ops = generic_ops;
> + /* pcm number is less than 16 */
> + if (spec->pcm_used >= 16)
> + break;
> /* other pstr fields are set in open */
> }
>
> @@ -2360,6 +2504,8 @@ static int patch_generic_hdmi(struct hda_codec *codec)
> return -ENOMEM;
>
> spec->ops = generic_standard_hdmi_ops;
> + spec->dev_num = 1; /* initialize to 1 */
> + mutex_init(&spec->pcm_lock);
> codec->spec = spec;
> hdmi_array_init(spec, 4);
>
> --
> 1.9.1
>
More information about the Alsa-devel
mailing list