[alsa-devel] [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM
Libin Yang
libin.yang at linux.intel.com
Wed Nov 4 08:55:19 CET 2015
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 at linux.intel.com wrote:
>>>
>>> From: Libin Yang <libin.yang at intel.com>
>>>
>>> Allocate the PCM based on the number of pin and device entry.
>>> The number of PCM is pin num plus device entry number per pin.
>>> For example, on Intel platform, it will be 5 PCMs.
>>>
>>> Do not attach the PCM to pin in initialization.
>>> Dynamically attach PCM to pin when monitor is connected
>>> in HDMI audio codec driver.
>>>
>>> When monitor is disconnected, detach the PCM from the pin.
>>> And if the audio is playing, stop the playback.
>>>
>>> The below is the example of device entry and PCM binding:
>>> For a monitor is plugged in, we need to dynamically assign
>>> this monitor to 5 PCM devices (on Intel platform):
>>> For a monitor at pin nid 0x05, dev index 0, it will prefer PCM 3
>>> For a monitor at pin nid 0x06, dev index 0, it will prefer PCM 7
>>> For a monitor at pin nid 0x07, dev index 0, it will prefer PCM 8
>>> For a monitor at dev index 1 (any pin), it will prefer PCM 9
>>> For a monitor at dev index 2 (any pin), it will prefer PCM 10
>>>
>>> If the preferred PCM is not available, try PCM in this order:
>>> 9, 10, 3, 7 ,8.
>>>
>>> Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
>>
>> We should split the changes to a few patches here. For example,
>> stopping the stream at discussion is basically an implementation
>> independent from the MST itself. IOW, the dynamic attach/detach can
>> be implemented and tested even without MST support. Let's code them
>> at first, then add MST support.
>
> 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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
More information about the Alsa-devel
mailing list