* [PATCH v2 0/4] Make HDMI ELD usable for hotplug @ 2013-02-19 8:47 David Henningsson 2013-02-19 8:47 ` [PATCH v2 1/4] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: David Henningsson @ 2013-02-19 8:47 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: fengguang.wu, pierre-louis.bossart, David Henningsson New day, new patches. This time there is a proper lock around access to the ELD buffer. I hope this satisfies the previous comments. I've also tested the patches together with some PulseAudio patches I'm writing and the notification works just fine. The ELD data shows up 300 ms after hotplug, so it's likely the first repoll giving me new ELD data). David Henningsson (4): ALSA: hda - hdmi: ELD shouldn't be valid after unplug ALSA: hda - hdmi: Do not expose eld data when eld is invalid ALSA: hda - hdmi: protect access to ELD buffer ALSA: hda - hdmi: Notify userspace when ELD control changes sound/pci/hda/hda_eld.c | 12 ++++++-- sound/pci/hda/hda_local.h | 3 ++ sound/pci/hda/patch_hdmi.c | 67 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 66 insertions(+), 16 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/4] ALSA: hda - hdmi: ELD shouldn't be valid after unplug 2013-02-19 8:47 [PATCH v2 0/4] Make HDMI ELD usable for hotplug David Henningsson @ 2013-02-19 8:47 ` David Henningsson 2013-02-19 8:47 ` [PATCH v2 2/4] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: David Henningsson @ 2013-02-19 8:47 UTC (permalink / raw) To: alsa-devel, tiwai Cc: fengguang.wu, pierre-louis.bossart, David Henningsson, stable Currently, eld_valid is never set to false, except at kernel module load time. This patch makes sure that eld is no longer valid when the cable is (hot-)unplugged. Cc: stable@kernel.org Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/patch_hdmi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index b9af281b..32adaa6 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1176,6 +1176,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", codec->addr, pin_nid, eld->monitor_present, eld_valid); + eld->eld_valid = false; if (eld_valid) { if (!snd_hdmi_get_eld(eld, codec, pin_nid)) snd_hdmi_show_eld(eld); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/4] ALSA: hda - hdmi: Do not expose eld data when eld is invalid 2013-02-19 8:47 [PATCH v2 0/4] Make HDMI ELD usable for hotplug David Henningsson 2013-02-19 8:47 ` [PATCH v2 1/4] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson @ 2013-02-19 8:47 ` David Henningsson 2013-02-19 8:47 ` [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer David Henningsson 2013-02-19 8:47 ` [PATCH v2 4/4] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson 3 siblings, 0 replies; 7+ messages in thread From: David Henningsson @ 2013-02-19 8:47 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: fengguang.wu, pierre-louis.bossart, David Henningsson Previously, it was possible to read the eld data of the previous monitor connected. This should not be allowed. Also refactor the function slightly. Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/patch_hdmi.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 32adaa6..7a73dfb 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -343,14 +343,16 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - struct hdmi_spec *spec; + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld; int pin_idx; - spec = codec->spec; uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES; pin_idx = kcontrol->private_value; - uinfo->count = spec->pins[pin_idx].sink_eld.eld_size; + eld = &spec->pins[pin_idx].sink_eld; + + uinfo->count = eld->eld_valid ? eld->eld_size : 0; return 0; } @@ -359,14 +361,21 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - struct hdmi_spec *spec; + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld; int pin_idx; - spec = codec->spec; pin_idx = kcontrol->private_value; + eld = &spec->pins[pin_idx].sink_eld; + + if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) { + snd_BUG(); + return -EINVAL; + } - memcpy(ucontrol->value.bytes.data, - spec->pins[pin_idx].sink_eld.eld_buffer, ELD_MAX_SIZE); + if (eld->eld_valid) + memcpy(ucontrol->value.bytes.data, eld->eld_buffer, + eld->eld_size); return 0; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer 2013-02-19 8:47 [PATCH v2 0/4] Make HDMI ELD usable for hotplug David Henningsson 2013-02-19 8:47 ` [PATCH v2 1/4] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson 2013-02-19 8:47 ` [PATCH v2 2/4] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson @ 2013-02-19 8:47 ` David Henningsson 2013-02-19 9:15 ` Takashi Iwai 2013-02-19 8:47 ` [PATCH v2 4/4] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson 3 siblings, 1 reply; 7+ messages in thread From: David Henningsson @ 2013-02-19 8:47 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: fengguang.wu, pierre-louis.bossart, David Henningsson Because the eld buffer can be simultaneously accessed from both workqueue context (updating) and process context (kcontrol read), we need to protect it with a spinlock to guarantee consistency. To avoid holding the spinlock while reading the ELD info from the codec, we introduce a temporary eld buffer. Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/hda_eld.c | 12 +++++++++--- sound/pci/hda/hda_local.h | 3 +++ sound/pci/hda/patch_hdmi.c | 32 +++++++++++++++++++++++++------- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..766f011 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, struct hdmi_eld *e = entry->private_data; char buf[SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE]; int i; - static char *eld_versoin_names[32] = { + static char *eld_version_names[32] = { "reserved", "reserved", "CEA-861D or below", @@ -505,15 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" }; + spin_lock(&e->lock); snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present); snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid); - if (!e->eld_valid) + if (!e->eld_valid) { + spin_unlock(&e->lock); return; + } snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name); snd_iprintf(buffer, "connection_type\t\t%s\n", eld_connection_type_names[e->conn_type]); snd_iprintf(buffer, "eld_version\t\t[0x%x] %s\n", e->eld_ver, - eld_versoin_names[e->eld_ver]); + eld_version_names[e->eld_ver]); snd_iprintf(buffer, "edid_version\t\t[0x%x] %s\n", e->cea_edid_ver, cea_edid_version_names[e->cea_edid_ver]); snd_iprintf(buffer, "manufacture_id\t\t0x%x\n", e->manufacture_id); @@ -530,6 +533,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, for (i = 0; i < e->sad_count; i++) hdmi_print_sad_info(i, e->sad + i, buffer); + spin_unlock(&e->lock); } static void hdmi_write_eld_info(struct snd_info_entry *entry, @@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, long long val; unsigned int n; + spin_lock(&e->lock); while (!snd_info_get_line(buffer, line, sizeof(line))) { if (sscanf(line, "%s %llx", name, &val) != 2) continue; @@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, e->sad_count = n + 1; } } + spin_unlock(&e->lock); } diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..b2d6a9e 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -712,8 +712,11 @@ struct cea_sad { /* * ELD: EDID Like Data + * + * Note: The order of these fields is used in hdmi_present_sense */ struct hdmi_eld { + spinlock_t lock; bool monitor_present; bool eld_valid; int eld_size; diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..52b1afc 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -91,6 +91,7 @@ struct hdmi_spec { struct hda_pcm pcm_rec[MAX_HDMI_PINS]; unsigned int channels_max; /* max over all cvts */ + struct hdmi_eld temp_eld; /* * Non-generic ATI/NVIDIA specific */ @@ -352,7 +353,9 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol, pin_idx = kcontrol->private_value; eld = &spec->pins[pin_idx].sink_eld; + spin_lock(&eld->lock); uinfo->count = eld->eld_valid ? eld->eld_size : 0; + spin_unlock(&eld->lock); return 0; } @@ -368,7 +371,9 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, pin_idx = kcontrol->private_value; eld = &spec->pins[pin_idx].sink_eld; + spin_lock(&eld->lock); if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) { + spin_unlock(&eld->lock); snd_BUG(); return -EINVAL; } @@ -376,6 +381,7 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, if (eld->eld_valid) memcpy(ucontrol->value.bytes.data, eld->eld_buffer, eld->eld_size); + spin_unlock(&eld->lock); return 0; } @@ -1162,7 +1168,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx) static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_codec *codec = per_pin->codec; - struct hdmi_eld *eld = &per_pin->sink_eld; + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld = &spec->temp_eld; + struct hdmi_eld *pin_eld = &per_pin->sink_eld; hda_nid_t pin_nid = per_pin->pin_nid; /* * Always execute a GetPinSense verb here, even when called from @@ -1173,28 +1181,37 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) * the unsolicited response to avoid custom WARs. */ int present = snd_hda_pin_sense(codec, pin_nid); - bool eld_valid = false; + bool update_eld = false; memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer)); eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); if (eld->monitor_present) - eld_valid = !!(present & AC_PINSENSE_ELDV); + eld->eld_valid = !!(present & AC_PINSENSE_ELDV); _snd_printd(SND_PR_VERBOSE, "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", - codec->addr, pin_nid, eld->monitor_present, eld_valid); + codec->addr, pin_nid, eld->monitor_present, eld->eld_valid); - eld->eld_valid = false; - if (eld_valid) { - if (!snd_hdmi_get_eld(eld, codec, pin_nid)) + if (eld->eld_valid) { + if (!snd_hdmi_get_eld(eld, codec, pin_nid)) { snd_hdmi_show_eld(eld); + update_eld = true; + } else if (repoll) { queue_delayed_work(codec->bus->workq, &per_pin->work, msecs_to_jiffies(300)); } } + + spin_lock(&pin_eld->lock); + if (pin_eld->eld_valid && !eld->eld_valid) + update_eld = true; + if (update_eld) + memcpy(&pin_eld->monitor_present, &eld->monitor_present, + sizeof(struct hdmi_eld) - sizeof(spinlock_t)); + spin_unlock(&pin_eld->lock); } static void hdmi_repoll_eld(struct work_struct *work) @@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec) struct hdmi_eld *eld = &per_pin->sink_eld; per_pin->codec = codec; + spin_lock_init(&eld->lock); INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld); snd_hda_eld_proc_new(codec, eld, pin_idx); } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer 2013-02-19 8:47 ` [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer David Henningsson @ 2013-02-19 9:15 ` Takashi Iwai 2013-02-19 9:45 ` David Henningsson 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2013-02-19 9:15 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart At Tue, 19 Feb 2013 09:47:36 +0100, David Henningsson wrote: > > Because the eld buffer can be simultaneously accessed from both > workqueue context (updating) and process context (kcontrol read), > we need to protect it with a spinlock to guarantee consistency. We can't use a spinlock for snd_iprintf(), as this involves with vmalloc(). Use a mutex intead. > To avoid holding the spinlock while reading the ELD info from the > codec, we introduce a temporary eld buffer. Looking through the patches, it's better to split to a few parts now: struct parsed_hdmi_eld { bool monitor_present; .... }; struct hdmi_eld { char eld_buffer[ELD_MAX_SIZE]; struct parsed_hdmi_eld info; struct mutex lock; #ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif }; In that way, you can avoid the hack like offset() calculation in patch_hdmi.c. The comparison can be done only for eld_buffer[], too. thanks, Takashi > > Signed-off-by: David Henningsson <david.henningsson@canonical.com> > --- > sound/pci/hda/hda_eld.c | 12 +++++++++--- > sound/pci/hda/hda_local.h | 3 +++ > sound/pci/hda/patch_hdmi.c | 32 +++++++++++++++++++++++++------- > 3 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c > index 4c054f4..766f011 100644 > --- a/sound/pci/hda/hda_eld.c > +++ b/sound/pci/hda/hda_eld.c > @@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, > struct hdmi_eld *e = entry->private_data; > char buf[SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE]; > int i; > - static char *eld_versoin_names[32] = { > + static char *eld_version_names[32] = { > "reserved", > "reserved", > "CEA-861D or below", > @@ -505,15 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, > [4 ... 7] = "reserved" > }; > > + spin_lock(&e->lock); > snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present); > snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid); > - if (!e->eld_valid) > + if (!e->eld_valid) { > + spin_unlock(&e->lock); > return; > + } > snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name); > snd_iprintf(buffer, "connection_type\t\t%s\n", > eld_connection_type_names[e->conn_type]); > snd_iprintf(buffer, "eld_version\t\t[0x%x] %s\n", e->eld_ver, > - eld_versoin_names[e->eld_ver]); > + eld_version_names[e->eld_ver]); > snd_iprintf(buffer, "edid_version\t\t[0x%x] %s\n", e->cea_edid_ver, > cea_edid_version_names[e->cea_edid_ver]); > snd_iprintf(buffer, "manufacture_id\t\t0x%x\n", e->manufacture_id); > @@ -530,6 +533,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, > > for (i = 0; i < e->sad_count; i++) > hdmi_print_sad_info(i, e->sad + i, buffer); > + spin_unlock(&e->lock); > } > > static void hdmi_write_eld_info(struct snd_info_entry *entry, > @@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, > long long val; > unsigned int n; > > + spin_lock(&e->lock); > while (!snd_info_get_line(buffer, line, sizeof(line))) { > if (sscanf(line, "%s %llx", name, &val) != 2) > continue; > @@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, > e->sad_count = n + 1; > } > } > + spin_unlock(&e->lock); > } > > > diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h > index 05f1d59..b2d6a9e 100644 > --- a/sound/pci/hda/hda_local.h > +++ b/sound/pci/hda/hda_local.h > @@ -712,8 +712,11 @@ struct cea_sad { > > /* > * ELD: EDID Like Data > + * > + * Note: The order of these fields is used in hdmi_present_sense > */ > struct hdmi_eld { > + spinlock_t lock; > bool monitor_present; > bool eld_valid; > int eld_size; > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 7a73dfb..52b1afc 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -91,6 +91,7 @@ struct hdmi_spec { > struct hda_pcm pcm_rec[MAX_HDMI_PINS]; > unsigned int channels_max; /* max over all cvts */ > > + struct hdmi_eld temp_eld; > /* > * Non-generic ATI/NVIDIA specific > */ > @@ -352,7 +353,9 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol, > pin_idx = kcontrol->private_value; > eld = &spec->pins[pin_idx].sink_eld; > > + spin_lock(&eld->lock); > uinfo->count = eld->eld_valid ? eld->eld_size : 0; > + spin_unlock(&eld->lock); > > return 0; > } > @@ -368,7 +371,9 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, > pin_idx = kcontrol->private_value; > eld = &spec->pins[pin_idx].sink_eld; > > + spin_lock(&eld->lock); > if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) { > + spin_unlock(&eld->lock); > snd_BUG(); > return -EINVAL; > } > @@ -376,6 +381,7 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, > if (eld->eld_valid) > memcpy(ucontrol->value.bytes.data, eld->eld_buffer, > eld->eld_size); > + spin_unlock(&eld->lock); > > return 0; > } > @@ -1162,7 +1168,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx) > static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) > { > struct hda_codec *codec = per_pin->codec; > - struct hdmi_eld *eld = &per_pin->sink_eld; > + struct hdmi_spec *spec = codec->spec; > + struct hdmi_eld *eld = &spec->temp_eld; > + struct hdmi_eld *pin_eld = &per_pin->sink_eld; > hda_nid_t pin_nid = per_pin->pin_nid; > /* > * Always execute a GetPinSense verb here, even when called from > @@ -1173,28 +1181,37 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) > * the unsolicited response to avoid custom WARs. > */ > int present = snd_hda_pin_sense(codec, pin_nid); > - bool eld_valid = false; > + bool update_eld = false; > > memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer)); > > eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); > if (eld->monitor_present) > - eld_valid = !!(present & AC_PINSENSE_ELDV); > + eld->eld_valid = !!(present & AC_PINSENSE_ELDV); > > _snd_printd(SND_PR_VERBOSE, > "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", > - codec->addr, pin_nid, eld->monitor_present, eld_valid); > + codec->addr, pin_nid, eld->monitor_present, eld->eld_valid); > > - eld->eld_valid = false; > - if (eld_valid) { > - if (!snd_hdmi_get_eld(eld, codec, pin_nid)) > + if (eld->eld_valid) { > + if (!snd_hdmi_get_eld(eld, codec, pin_nid)) { > snd_hdmi_show_eld(eld); > + update_eld = true; > + } > else if (repoll) { > queue_delayed_work(codec->bus->workq, > &per_pin->work, > msecs_to_jiffies(300)); > } > } > + > + spin_lock(&pin_eld->lock); > + if (pin_eld->eld_valid && !eld->eld_valid) > + update_eld = true; > + if (update_eld) > + memcpy(&pin_eld->monitor_present, &eld->monitor_present, > + sizeof(struct hdmi_eld) - sizeof(spinlock_t)); > + spin_unlock(&pin_eld->lock); > } > > static void hdmi_repoll_eld(struct work_struct *work) > @@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec) > struct hdmi_eld *eld = &per_pin->sink_eld; > > per_pin->codec = codec; > + spin_lock_init(&eld->lock); > INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld); > snd_hda_eld_proc_new(codec, eld, pin_idx); > } > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer 2013-02-19 9:15 ` Takashi Iwai @ 2013-02-19 9:45 ` David Henningsson 0 siblings, 0 replies; 7+ messages in thread From: David Henningsson @ 2013-02-19 9:45 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart On 02/19/2013 10:15 AM, Takashi Iwai wrote: > At Tue, 19 Feb 2013 09:47:36 +0100, > David Henningsson wrote: >> >> Because the eld buffer can be simultaneously accessed from both >> workqueue context (updating) and process context (kcontrol read), >> we need to protect it with a spinlock to guarantee consistency. > > We can't use a spinlock for snd_iprintf(), as this involves with > vmalloc(). Use a mutex intead. Ok, thanks for spotting. >> To avoid holding the spinlock while reading the ELD info from the >> codec, we introduce a temporary eld buffer. > > Looking through the patches, it's better to split to a few parts now: > > struct parsed_hdmi_eld { > bool monitor_present; > .... > }; > > struct hdmi_eld { > char eld_buffer[ELD_MAX_SIZE]; > struct parsed_hdmi_eld info; > struct mutex lock; > #ifdef CONFIG_PROC_FS > struct snd_info_entry *proc_entry; > #endif > }; > > In that way, you can avoid the hack like offset() calculation in > patch_hdmi.c. The comparison can be done only for eld_buffer[], too. This sounds like a good idea. I will do it. I think eld_valid and monitor_present should be in hdmi_eld and not parsed_hdmi_eld, because those fields do not come from the raw ELD info. > > > thanks, > > Takashi > >> >> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >> --- >> sound/pci/hda/hda_eld.c | 12 +++++++++--- >> sound/pci/hda/hda_local.h | 3 +++ >> sound/pci/hda/patch_hdmi.c | 32 +++++++++++++++++++++++++------- >> 3 files changed, 37 insertions(+), 10 deletions(-) >> >> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c >> index 4c054f4..766f011 100644 >> --- a/sound/pci/hda/hda_eld.c >> +++ b/sound/pci/hda/hda_eld.c >> @@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, >> struct hdmi_eld *e = entry->private_data; >> char buf[SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE]; >> int i; >> - static char *eld_versoin_names[32] = { >> + static char *eld_version_names[32] = { >> "reserved", >> "reserved", >> "CEA-861D or below", >> @@ -505,15 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, >> [4 ... 7] = "reserved" >> }; >> >> + spin_lock(&e->lock); >> snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present); >> snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid); >> - if (!e->eld_valid) >> + if (!e->eld_valid) { >> + spin_unlock(&e->lock); >> return; >> + } >> snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name); >> snd_iprintf(buffer, "connection_type\t\t%s\n", >> eld_connection_type_names[e->conn_type]); >> snd_iprintf(buffer, "eld_version\t\t[0x%x] %s\n", e->eld_ver, >> - eld_versoin_names[e->eld_ver]); >> + eld_version_names[e->eld_ver]); >> snd_iprintf(buffer, "edid_version\t\t[0x%x] %s\n", e->cea_edid_ver, >> cea_edid_version_names[e->cea_edid_ver]); >> snd_iprintf(buffer, "manufacture_id\t\t0x%x\n", e->manufacture_id); >> @@ -530,6 +533,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, >> >> for (i = 0; i < e->sad_count; i++) >> hdmi_print_sad_info(i, e->sad + i, buffer); >> + spin_unlock(&e->lock); >> } >> >> static void hdmi_write_eld_info(struct snd_info_entry *entry, >> @@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, >> long long val; >> unsigned int n; >> >> + spin_lock(&e->lock); >> while (!snd_info_get_line(buffer, line, sizeof(line))) { >> if (sscanf(line, "%s %llx", name, &val) != 2) >> continue; >> @@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, >> e->sad_count = n + 1; >> } >> } >> + spin_unlock(&e->lock); >> } >> >> >> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h >> index 05f1d59..b2d6a9e 100644 >> --- a/sound/pci/hda/hda_local.h >> +++ b/sound/pci/hda/hda_local.h >> @@ -712,8 +712,11 @@ struct cea_sad { >> >> /* >> * ELD: EDID Like Data >> + * >> + * Note: The order of these fields is used in hdmi_present_sense >> */ >> struct hdmi_eld { >> + spinlock_t lock; >> bool monitor_present; >> bool eld_valid; >> int eld_size; >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >> index 7a73dfb..52b1afc 100644 >> --- a/sound/pci/hda/patch_hdmi.c >> +++ b/sound/pci/hda/patch_hdmi.c >> @@ -91,6 +91,7 @@ struct hdmi_spec { >> struct hda_pcm pcm_rec[MAX_HDMI_PINS]; >> unsigned int channels_max; /* max over all cvts */ >> >> + struct hdmi_eld temp_eld; >> /* >> * Non-generic ATI/NVIDIA specific >> */ >> @@ -352,7 +353,9 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol, >> pin_idx = kcontrol->private_value; >> eld = &spec->pins[pin_idx].sink_eld; >> >> + spin_lock(&eld->lock); >> uinfo->count = eld->eld_valid ? eld->eld_size : 0; >> + spin_unlock(&eld->lock); >> >> return 0; >> } >> @@ -368,7 +371,9 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, >> pin_idx = kcontrol->private_value; >> eld = &spec->pins[pin_idx].sink_eld; >> >> + spin_lock(&eld->lock); >> if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) { >> + spin_unlock(&eld->lock); >> snd_BUG(); >> return -EINVAL; >> } >> @@ -376,6 +381,7 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, >> if (eld->eld_valid) >> memcpy(ucontrol->value.bytes.data, eld->eld_buffer, >> eld->eld_size); >> + spin_unlock(&eld->lock); >> >> return 0; >> } >> @@ -1162,7 +1168,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx) >> static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) >> { >> struct hda_codec *codec = per_pin->codec; >> - struct hdmi_eld *eld = &per_pin->sink_eld; >> + struct hdmi_spec *spec = codec->spec; >> + struct hdmi_eld *eld = &spec->temp_eld; >> + struct hdmi_eld *pin_eld = &per_pin->sink_eld; >> hda_nid_t pin_nid = per_pin->pin_nid; >> /* >> * Always execute a GetPinSense verb here, even when called from >> @@ -1173,28 +1181,37 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) >> * the unsolicited response to avoid custom WARs. >> */ >> int present = snd_hda_pin_sense(codec, pin_nid); >> - bool eld_valid = false; >> + bool update_eld = false; >> >> memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer)); >> >> eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); >> if (eld->monitor_present) >> - eld_valid = !!(present & AC_PINSENSE_ELDV); >> + eld->eld_valid = !!(present & AC_PINSENSE_ELDV); >> >> _snd_printd(SND_PR_VERBOSE, >> "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", >> - codec->addr, pin_nid, eld->monitor_present, eld_valid); >> + codec->addr, pin_nid, eld->monitor_present, eld->eld_valid); >> >> - eld->eld_valid = false; >> - if (eld_valid) { >> - if (!snd_hdmi_get_eld(eld, codec, pin_nid)) >> + if (eld->eld_valid) { >> + if (!snd_hdmi_get_eld(eld, codec, pin_nid)) { >> snd_hdmi_show_eld(eld); >> + update_eld = true; >> + } >> else if (repoll) { >> queue_delayed_work(codec->bus->workq, >> &per_pin->work, >> msecs_to_jiffies(300)); >> } >> } >> + >> + spin_lock(&pin_eld->lock); >> + if (pin_eld->eld_valid && !eld->eld_valid) >> + update_eld = true; >> + if (update_eld) >> + memcpy(&pin_eld->monitor_present, &eld->monitor_present, >> + sizeof(struct hdmi_eld) - sizeof(spinlock_t)); >> + spin_unlock(&pin_eld->lock); >> } >> >> static void hdmi_repoll_eld(struct work_struct *work) >> @@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec) >> struct hdmi_eld *eld = &per_pin->sink_eld; >> >> per_pin->codec = codec; >> + spin_lock_init(&eld->lock); >> INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld); >> snd_hda_eld_proc_new(codec, eld, pin_idx); >> } >> -- >> 1.7.9.5 >> > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 4/4] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-19 8:47 [PATCH v2 0/4] Make HDMI ELD usable for hotplug David Henningsson ` (2 preceding siblings ...) 2013-02-19 8:47 ` [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer David Henningsson @ 2013-02-19 8:47 ` David Henningsson 3 siblings, 0 replies; 7+ messages in thread From: David Henningsson @ 2013-02-19 8:47 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: fengguang.wu, pierre-louis.bossart, David Henningsson ELD validity can change during the lifetime of a presence detect, so we need to be able to listen for changes on the ELD control. Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/patch_hdmi.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 52b1afc..bfaa0ee 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -75,6 +75,7 @@ struct hdmi_spec_per_pin { struct hda_codec *codec; struct hdmi_eld sink_eld; struct delayed_work work; + struct snd_kcontrol *eld_ctl; int repoll_count; bool non_pcm; bool chmap_set; /* channel-map override by ALSA API? */ @@ -411,6 +412,7 @@ static int hdmi_create_eld_ctl(struct hda_codec *codec, int pin_idx, if (err < 0) return err; + spec->pins[pin_idx].eld_ctl = kctl; return 0; } @@ -1182,6 +1184,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) */ int present = snd_hda_pin_sense(codec, pin_nid); bool update_eld = false; + bool eld_changed = false; memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer)); @@ -1208,10 +1211,20 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) spin_lock(&pin_eld->lock); if (pin_eld->eld_valid && !eld->eld_valid) update_eld = true; - if (update_eld) - memcpy(&pin_eld->monitor_present, &eld->monitor_present, - sizeof(struct hdmi_eld) - sizeof(spinlock_t)); + if (update_eld) { + void *dest = &pin_eld->monitor_present; + void *src = &eld->monitor_present; + size_t size = sizeof(struct hdmi_eld) - sizeof(spinlock_t); + eld_changed = memcmp(dest, src, size) != 0; + if (eld_changed) + memcpy(dest, src, size); + } spin_unlock(&pin_eld->lock); + + if (eld_changed) + snd_ctl_notify(codec->bus->card, + SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO, + &per_pin->eld_ctl->id); } static void hdmi_repoll_eld(struct work_struct *work) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-02-19 9:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-19 8:47 [PATCH v2 0/4] Make HDMI ELD usable for hotplug David Henningsson 2013-02-19 8:47 ` [PATCH v2 1/4] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson 2013-02-19 8:47 ` [PATCH v2 2/4] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson 2013-02-19 8:47 ` [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer David Henningsson 2013-02-19 9:15 ` Takashi Iwai 2013-02-19 9:45 ` David Henningsson 2013-02-19 8:47 ` [PATCH v2 4/4] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).