[alsa-devel] [RFC PATCH 2/2] ALSA: hda - HDMI codec driver dynamically attach PCM
Libin Yang
libin.yang at linux.intel.com
Fri Nov 6 06:33:48 CET 2015
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 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?
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
>>>
More information about the Alsa-devel
mailing list