* [RFC PATCH 0/2] ALSA: hda - patch set for DP MST audio support @ 2015-11-03 8:22 libin.yang 2015-11-03 8:22 ` [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio libin.yang 2015-11-03 8:22 ` [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM libin.yang 0 siblings, 2 replies; 13+ messages in thread From: libin.yang @ 2015-11-03 8:22 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: Libin Yang, mengdong.lin From: Libin Yang <libin.yang@linux.intel.com> This is one of the patch sets to support DP MST audio. This patch set is to support dynamic PCM attachment to pin As there are still other patches, it may be better to put the patches in a specific branch? Libin Yang (2): ALSA: hda - add hda_dev_t typedef for DP MST audio ALSA: hda - HDMI codec driver dynamically attach PCM include/sound/hdaudio.h | 1 + sound/pci/hda/hda_codec.c | 5 +- sound/pci/hda/hda_codec.h | 1 + sound/pci/hda/patch_hdmi.c | 170 +++++++++++++++++++++++++++++++++++++++++---- 4 files changed, 163 insertions(+), 14 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio 2015-11-03 8:22 [RFC PATCH 0/2] ALSA: hda - patch set for DP MST audio support libin.yang @ 2015-11-03 8:22 ` libin.yang 2015-11-03 16:28 ` Takashi Iwai 2015-11-03 8:22 ` [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM libin.yang 1 sibling, 1 reply; 13+ messages in thread From: libin.yang @ 2015-11-03 8:22 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: Libin Yang, mengdong.lin From: Libin Yang <libin.yang@intel.com> Signed-off-by: Libin Yang <libin.yang@intel.com> --- include/sound/hdaudio.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index e2b712c..1f54964 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -15,6 +15,7 @@ /* codec node id */ typedef u16 hda_nid_t; +typedef u8 hda_dev_t; struct hdac_bus; struct hdac_stream; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio 2015-11-03 8:22 ` [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio libin.yang @ 2015-11-03 16:28 ` Takashi Iwai 2015-11-04 2:37 ` Libin Yang 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2015-11-03 16:28 UTC (permalink / raw) To: libin.yang; +Cc: Libin Yang, alsa-devel, mengdong.lin On Tue, 03 Nov 2015 09:22:52 +0100, libin.yang@linux.intel.com wrote: > > From: Libin Yang <libin.yang@intel.com> > > Signed-off-by: Libin Yang <libin.yang@intel.com> > --- > include/sound/hdaudio.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h > index e2b712c..1f54964 100644 > --- a/include/sound/hdaudio.h > +++ b/include/sound/hdaudio.h > @@ -15,6 +15,7 @@ > > /* codec node id */ > typedef u16 hda_nid_t; > +typedef u8 hda_dev_t; This isn't used in any ABI, so we don't have to define the type. Just use int instead. NID was defined there because it's used in ABI and it can represent the purpose more clearly. (Also at the early time of driver writing, the size itself wasn't 100% clear; we started writing it before HD-audio spec got solid.) thanks, Takashi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio 2015-11-03 16:28 ` Takashi Iwai @ 2015-11-04 2:37 ` Libin Yang 0 siblings, 0 replies; 13+ messages in thread From: Libin Yang @ 2015-11-04 2:37 UTC (permalink / raw) To: Takashi Iwai; +Cc: Libin Yang, alsa-devel, mengdong.lin On 11/04/2015 12:28 AM, Takashi Iwai wrote: > On Tue, 03 Nov 2015 09:22:52 +0100, > libin.yang@linux.intel.com wrote: >> >> From: Libin Yang <libin.yang@intel.com> >> >> Signed-off-by: Libin Yang <libin.yang@intel.com> >> --- >> include/sound/hdaudio.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h >> index e2b712c..1f54964 100644 >> --- a/include/sound/hdaudio.h >> +++ b/include/sound/hdaudio.h >> @@ -15,6 +15,7 @@ >> >> /* codec node id */ >> typedef u16 hda_nid_t; >> +typedef u8 hda_dev_t; > > This isn't used in any ABI, so we don't have to define the type. > Just use int instead. > > NID was defined there because it's used in ABI and it can represent > the purpose more clearly. (Also at the early time of driver writing, > the size itself wasn't 100% clear; we started writing it before > HD-audio spec got solid.) OK. I will use int type directly. Best Regards, Libin > > > thanks, > > Takashi > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-03 8:22 [RFC PATCH 0/2] ALSA: hda - patch set for DP MST audio support libin.yang 2015-11-03 8:22 ` [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio libin.yang @ 2015-11-03 8:22 ` libin.yang 2015-11-03 16:44 ` Takashi Iwai 1 sibling, 1 reply; 13+ messages in thread From: libin.yang @ 2015-11-03 8:22 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: Libin Yang, Libin Yang, mengdong.lin From: Libin Yang <libin.yang@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@linux.intel.com> --- 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); /** * 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; + } + + /* 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)) { + codec_warn(per_pin->codec, + "HDMI: monitor disconnected, try to stop playback\n"); + 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; + } 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-03 8:22 ` [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM libin.yang @ 2015-11-03 16:44 ` Takashi Iwai 2015-11-04 5:38 ` Libin Yang 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2015-11-03 16:44 UTC (permalink / raw) To: libin.yang; +Cc: Libin Yang, alsa-devel, mengdong.lin On Tue, 03 Nov 2015 09:22:53 +0100, libin.yang@linux.intel.com wrote: > > From: Libin Yang <libin.yang@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@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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-03 16:44 ` Takashi Iwai @ 2015-11-04 5:38 ` Libin Yang 2015-11-04 7:55 ` Libin Yang 2015-11-06 5:33 ` Libin Yang 0 siblings, 2 replies; 13+ messages in thread From: Libin Yang @ 2015-11-04 5:38 UTC (permalink / raw) To: Takashi Iwai; +Cc: Libin Yang, alsa-devel, mengdong.lin On 11/04/2015 12:44 AM, Takashi Iwai wrote: > On Tue, 03 Nov 2015 09:22:53 +0100, > libin.yang@linux.intel.com wrote: >> >> From: Libin Yang <libin.yang@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@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. This patch actually doesn't depend on MST. I use the dev_num to determine how many PCMs to be created. To Split the patch, how about I create the PCM number based the pin number in the patch? > > 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...? I have no other verdors platforms to test. But it seems to be OK on other platforms. The impact is, i think, it will stop the stream when monitor is disconnected. Should we apply the code by judging whehter it is Intel platform? > > 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. OK, I will add the kernel doc comment. And put it in a preparation patch. > > >> /** >> * 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. This step is used for the situation (for example, on Intel platform): if the device entry id is 1, it will prefer PCM 9; if the device entry id is 2, it will prefer PCM 10; OK, I will remove the second try. > >> + >> + /* 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. Get it. Thanks. > >> + 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. OK, I see. > >> + 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? From the spec, it says: If the Device List Lenght value is 1 - 63, it indicates the Pin Widget is multi stream capable, and 2 - 64 Device Entries are supported in the Pin Widget. So my understanding is that it should be the pin capability, not the status. My test on BDW result shows: 1. When there is not DP MST hub connected on the pin, the *_get_num_devices() will return 0, which means Pin Widget is not MST capable. 2. When there is DP MST hub connected to the pin, it will always return 2, no matter how many monitors are connected to the pin. And it will also return 2 even you disconnect the hub. > > > > 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 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-04 5:38 ` Libin Yang @ 2015-11-04 7:55 ` Libin Yang 2015-11-06 5:33 ` Libin Yang 1 sibling, 0 replies; 13+ messages in thread From: Libin Yang @ 2015-11-04 7:55 UTC (permalink / raw) To: alsa-devel On 11/04/2015 01:38 PM, Libin Yang wrote: > On 11/04/2015 12:44 AM, Takashi Iwai wrote: >> On Tue, 03 Nov 2015 09:22:53 +0100, >> libin.yang@linux.intel.com wrote: >>> >>> From: Libin Yang <libin.yang@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@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. > > This patch actually doesn't depend on MST. I use the dev_num to > determine how many PCMs to be created. > > To Split the patch, how about I create the PCM number based the pin > number in the patch? > >> >> 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...? > > I have no other verdors platforms to test. But it seems to be OK on > other platforms. The impact is, i think, it will stop the stream when > monitor is disconnected. Should we apply the code by judging whehter > it is Intel platform? Another impact is when playback on the pin which is not connected to a monitor will fail. Best Regards, Libin > >> >> 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. > > OK, I will add the kernel doc comment. And put it in a preparation patch. > >> >> >>> /** >>> * 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. > > This step is used for the situation (for example, on Intel platform): > if the device entry id is 1, it will prefer PCM 9; > if the device entry id is 2, it will prefer PCM 10; > > OK, I will remove the second try. > >> >>> + >>> + /* 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. > > Get it. Thanks. > >> >>> + 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. > > OK, I see. > >> >>> + 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? > > From the spec, it says: > If the Device List Lenght value is 1 - 63, it indicates the Pin Widget > is multi stream capable, and 2 - 64 Device Entries are supported in > the Pin Widget. > > So my understanding is that it should be the pin capability, not the > status. > > My test on BDW result shows: > 1. When there is not DP MST hub connected on the pin, the > *_get_num_devices() will return 0, which means Pin Widget is not MST > capable. > 2. When there is DP MST hub connected to the pin, it will always > return 2, no matter how many monitors are connected to the pin. And it > will also return 2 even you disconnect the hub. > >> >> >> >> 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 >>> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-04 5:38 ` Libin Yang 2015-11-04 7:55 ` Libin Yang @ 2015-11-06 5:33 ` Libin Yang 2015-11-06 9:12 ` Takashi Iwai 1 sibling, 1 reply; 13+ messages in thread From: Libin Yang @ 2015-11-06 5:33 UTC (permalink / raw) To: Takashi Iwai; +Cc: Libin Yang, alsa-devel, mengdong.lin Hi Takashi, On 11/04/2015 01:38 PM, Libin Yang wrote: > On 11/04/2015 12:44 AM, Takashi Iwai wrote: >> On Tue, 03 Nov 2015 09:22:53 +0100, >> libin.yang@linux.intel.com wrote: >>> >>> From: Libin Yang <libin.yang@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@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. > > This patch actually doesn't depend on MST. I use the dev_num to > determine how many PCMs to be created. > > To Split the patch, how about I create the PCM number based the pin > number in the patch? > >> >> 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...? > > I have no other verdors platforms to test. But it seems to be OK on > other platforms. The impact is, i think, it will stop the stream when > monitor is disconnected. Should we apply the code by judging whehter > it is Intel platform? If you are concerning the impact on other vendors, what about I create a separate mst codec driver? In the driver, I will still use virtual pin, but other vendors platform will not be supported at the moment. Other vendors can add their patches to the new codec driver to support their own mst audio. > >> >> 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. > > OK, I will add the kernel doc comment. And put it in a preparation patch. > >> >> >>> /** >>> * 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. > > This step is used for the situation (for example, on Intel platform): > if the device entry id is 1, it will prefer PCM 9; > if the device entry id is 2, it will prefer PCM 10; > > OK, I will remove the second try. > >> >>> + >>> + /* 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. > > Get it. Thanks. > >> >>> + 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. > > OK, I see. > >> >>> + 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? > > From the spec, it says: > If the Device List Lenght value is 1 - 63, it indicates the Pin Widget > is multi stream capable, and 2 - 64 Device Entries are supported in > the Pin Widget. > > So my understanding is that it should be the pin capability, not the > status. > > My test on BDW result shows: > 1. When there is not DP MST hub connected on the pin, the > *_get_num_devices() will return 0, which means Pin Widget is not MST > capable. > 2. When there is DP MST hub connected to the pin, it will always > return 2, no matter how many monitors are connected to the pin. And it > will also return 2 even you disconnect the hub. > >> >> >> >> 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 >>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-06 5:33 ` Libin Yang @ 2015-11-06 9:12 ` Takashi Iwai 2015-11-09 1:38 ` Libin Yang 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2015-11-06 9:12 UTC (permalink / raw) To: Libin Yang; +Cc: Libin Yang, alsa-devel, mengdong.lin On Fri, 06 Nov 2015 06:33:48 +0100, Libin Yang wrote: > > Hi Takashi, > > On 11/04/2015 01:38 PM, Libin Yang wrote: > > On 11/04/2015 12:44 AM, Takashi Iwai wrote: > >> On Tue, 03 Nov 2015 09:22:53 +0100, > >> libin.yang@linux.intel.com wrote: > >>> > >>> From: Libin Yang <libin.yang@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@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. > > > > This patch actually doesn't depend on MST. I use the dev_num to > > determine how many PCMs to be created. > > > > To Split the patch, how about I create the PCM number based the pin > > number in the patch? > > > >> > >> 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...? > > > > I have no other verdors platforms to test. But it seems to be OK on > > other platforms. The impact is, i think, it will stop the stream when > > monitor is disconnected. Should we apply the code by judging whehter > > it is Intel platform? > > If you are concerning the impact on other vendors, what about I create > a separate mst codec driver? In the driver, I will still use virtual > pin, but other vendors platform will not be supported at the moment. > Other vendors can add their patches to the new codec driver to support > their own mst audio. No, it's not the option. Such a behavior change is the big burden to user-space. We should provide consistent behavior as much as possible. Takashi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-06 9:12 ` Takashi Iwai @ 2015-11-09 1:38 ` Libin Yang 2015-11-09 7:55 ` Takashi Iwai 0 siblings, 1 reply; 13+ messages in thread From: Libin Yang @ 2015-11-09 1:38 UTC (permalink / raw) To: Takashi Iwai; +Cc: Libin Yang, alsa-devel, mengdong.lin Hi Takashi, On 11/06/2015 05:12 PM, Takashi Iwai wrote: > On Fri, 06 Nov 2015 06:33:48 +0100, > Libin Yang wrote: >> >> Hi Takashi, >> >> On 11/04/2015 01:38 PM, Libin Yang wrote: >>> On 11/04/2015 12:44 AM, Takashi Iwai wrote: >>>> On Tue, 03 Nov 2015 09:22:53 +0100, >>>> libin.yang@linux.intel.com wrote: >>>>> >>>>> From: Libin Yang <libin.yang@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@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. >>> >>> This patch actually doesn't depend on MST. I use the dev_num to >>> determine how many PCMs to be created. >>> >>> To Split the patch, how about I create the PCM number based the pin >>> number in the patch? >>> >>>> >>>> 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...? >>> >>> I have no other verdors platforms to test. But it seems to be OK on >>> other platforms. The impact is, i think, it will stop the stream when >>> monitor is disconnected. Should we apply the code by judging whehter >>> it is Intel platform? >> >> If you are concerning the impact on other vendors, what about I create >> a separate mst codec driver? In the driver, I will still use virtual >> pin, but other vendors platform will not be supported at the moment. >> Other vendors can add their patches to the new codec driver to support >> their own mst audio. > > No, it's not the option. Such a behavior change is the big burden to > user-space. We should provide consistent behavior as much as > possible. OK, I see. What do you think if I judge the platform before using DP MST mode? Best Regards, Libin > > > Takashi > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-09 1:38 ` Libin Yang @ 2015-11-09 7:55 ` Takashi Iwai 2015-11-09 8:45 ` Libin Yang 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2015-11-09 7:55 UTC (permalink / raw) To: Libin Yang; +Cc: Libin Yang, alsa-devel, mengdong.lin On Mon, 09 Nov 2015 02:38:18 +0100, Libin Yang wrote: > > Hi Takashi, > > On 11/06/2015 05:12 PM, Takashi Iwai wrote: > > On Fri, 06 Nov 2015 06:33:48 +0100, > > Libin Yang wrote: > >> > >> Hi Takashi, > >> > >> On 11/04/2015 01:38 PM, Libin Yang wrote: > >>> On 11/04/2015 12:44 AM, Takashi Iwai wrote: > >>>> On Tue, 03 Nov 2015 09:22:53 +0100, > >>>> libin.yang@linux.intel.com wrote: > >>>>> > >>>>> From: Libin Yang <libin.yang@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@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. > >>> > >>> This patch actually doesn't depend on MST. I use the dev_num to > >>> determine how many PCMs to be created. > >>> > >>> To Split the patch, how about I create the PCM number based the pin > >>> number in the patch? > >>> > >>>> > >>>> 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...? > >>> > >>> I have no other verdors platforms to test. But it seems to be OK on > >>> other platforms. The impact is, i think, it will stop the stream when > >>> monitor is disconnected. Should we apply the code by judging whehter > >>> it is Intel platform? > >> > >> If you are concerning the impact on other vendors, what about I create > >> a separate mst codec driver? In the driver, I will still use virtual > >> pin, but other vendors platform will not be supported at the moment. > >> Other vendors can add their patches to the new codec driver to support > >> their own mst audio. > > > > No, it's not the option. Such a behavior change is the big burden to > > user-space. We should provide consistent behavior as much as > > possible. > > OK, I see. What do you think if I judge the platform before using DP > MST mode? Yes, I'd love to have more tests before doing any radical changes. As already mentioned, basically your work has to individual changes: the stream validation and dynamic disconnect per monitor plug/unplug, and the MST support via dynamic PCM assignment. The former is definitely a generic framework, and it's worth to have even without MST support. OTOH, the former *is* the biggest change from user-space POV. User won't notice much about the latter -- the usage pattern should be almost same. So, it's the former change we need to test and evaluate carefully. Takashi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM 2015-11-09 7:55 ` Takashi Iwai @ 2015-11-09 8:45 ` Libin Yang 0 siblings, 0 replies; 13+ messages in thread From: Libin Yang @ 2015-11-09 8:45 UTC (permalink / raw) To: Takashi Iwai; +Cc: Libin Yang, alsa-devel, mengdong.lin On 11/09/2015 03:55 PM, Takashi Iwai wrote: > On Mon, 09 Nov 2015 02:38:18 +0100, > Libin Yang wrote: >> >> Hi Takashi, >> >> On 11/06/2015 05:12 PM, Takashi Iwai wrote: >>> On Fri, 06 Nov 2015 06:33:48 +0100, >>> Libin Yang wrote: >>>> >>>> Hi Takashi, >>>> >>>> On 11/04/2015 01:38 PM, Libin Yang wrote: >>>>> On 11/04/2015 12:44 AM, Takashi Iwai wrote: >>>>>> On Tue, 03 Nov 2015 09:22:53 +0100, >>>>>> libin.yang@linux.intel.com wrote: >>>>>>> >>>>>>> From: Libin Yang <libin.yang@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@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. >>>>> >>>>> This patch actually doesn't depend on MST. I use the dev_num to >>>>> determine how many PCMs to be created. >>>>> >>>>> To Split the patch, how about I create the PCM number based the pin >>>>> number in the patch? >>>>> >>>>>> >>>>>> 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...? >>>>> >>>>> I have no other verdors platforms to test. But it seems to be OK on >>>>> other platforms. The impact is, i think, it will stop the stream when >>>>> monitor is disconnected. Should we apply the code by judging whehter >>>>> it is Intel platform? >>>> >>>> If you are concerning the impact on other vendors, what about I create >>>> a separate mst codec driver? In the driver, I will still use virtual >>>> pin, but other vendors platform will not be supported at the moment. >>>> Other vendors can add their patches to the new codec driver to support >>>> their own mst audio. >>> >>> No, it's not the option. Such a behavior change is the big burden to >>> user-space. We should provide consistent behavior as much as >>> possible. >> >> OK, I see. What do you think if I judge the platform before using DP >> MST mode? > > Yes, I'd love to have more tests before doing any radical changes. > > As already mentioned, basically your work has to individual changes: > the stream validation and dynamic disconnect per monitor plug/unplug, > and the MST support via dynamic PCM assignment. > > The former is definitely a generic framework, and it's worth to have > even without MST support. OTOH, the former *is* the biggest change > from user-space POV. User won't notice much about the latter -- the > usage pattern should be almost same. So, it's the former change we > need to test and evaluate carefully. OK. I will split the patch into 2 parts: one is dynamic disconnect, the other is MST support. For the safety, the dynamic assignment and MST support will only be used on Intel platforms: HSW, BDW, SKL. I can do the test on the platforms. Best Regards, Libin > > > Takashi > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-11-09 8:47 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-03 8:22 [RFC PATCH 0/2] ALSA: hda - patch set for DP MST audio support libin.yang 2015-11-03 8:22 ` [RFC PATCH 1/2] ALSA: hda - add hda_dev_t typedef for DP MST audio libin.yang 2015-11-03 16:28 ` Takashi Iwai 2015-11-04 2:37 ` Libin Yang 2015-11-03 8:22 ` [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM libin.yang 2015-11-03 16:44 ` Takashi Iwai 2015-11-04 5:38 ` Libin Yang 2015-11-04 7:55 ` Libin Yang 2015-11-06 5:33 ` Libin Yang 2015-11-06 9:12 ` Takashi Iwai 2015-11-09 1:38 ` Libin Yang 2015-11-09 7:55 ` Takashi Iwai 2015-11-09 8:45 ` Libin Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox