[alsa-devel] [PATCH v3 0/5] Make HDMI ELD usable for hotplug
Changes since v2: Added refactoring patch to split hdmi_eld into hdmi_eld and parsed_hdmi_eld.
David Henningsson (5): 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: Refactor hdmi_eld into parsed_hdmi_eld ALSA: hda - hdmi: Protect ELD buffer ALSA: hda - hdmi: Notify userspace when ELD control changes
sound/pci/hda/hda_eld.c | 47 ++++++++++++++---------- sound/pci/hda/hda_local.h | 23 +++++++----- sound/pci/hda/patch_hdmi.c | 86 +++++++++++++++++++++++++++++++++----------- 3 files changed, 107 insertions(+), 49 deletions(-)
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);
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; }
For better readability, the information that is parsed out of the ELD data is now put into a separate parsed_hdmi_eld struct.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- sound/pci/hda/hda_eld.c | 41 ++++++++++++++++++++++------------------- sound/pci/hda/hda_local.h | 22 +++++++++++++--------- sound/pci/hda/patch_hdmi.c | 12 ++++++------ 3 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..aecda72 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a, /* * Be careful, ELD buf could be totally rubbish! */ -static int hdmi_update_eld(struct hdmi_eld *e, - const unsigned char *buf, int size) +static int hdmi_parse_eld(struct parsed_hdmi_eld *e, + const unsigned char *buf, int size) { int mnl; int i; @@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, goto out_fail; }
- e->eld_size = size; e->baseline_len = GRAB_BITS(buf, 2, 0, 8); mnl = GRAB_BITS(buf, 4, 0, 5); e->cea_edid_ver = GRAB_BITS(buf, 4, 5, 3); @@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, if (!e->spk_alloc) e->spk_alloc = 0xffff;
- e->eld_valid = true; return 0;
out_fail: @@ -328,7 +326,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
/* * ELD size is initialized to zero in caller function. If no errors and - * ELD is valid, actual eld_size is assigned in hdmi_update_eld() + * ELD is valid, actual eld_size is assigned. */
size = snd_hdmi_get_eld_size(codec, nid); @@ -372,8 +370,11 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, buf[i] = val; }
- ret = hdmi_update_eld(eld, buf, size); + if ((ret = hdmi_parse_eld(&eld->info, buf, size)) < 0) + goto error;
+ eld->eld_size = size; + eld->eld_valid = true; error: return ret; } @@ -438,7 +439,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen) buf[j] = '\0'; /* necessary when j == 0 */ }
-void snd_hdmi_show_eld(struct hdmi_eld *e) +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e) { int i;
@@ -487,10 +488,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a, static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) { - struct hdmi_eld *e = entry->private_data; + struct hdmi_eld *eld = entry->private_data; + struct parsed_hdmi_eld *e = &eld->info; 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 +507,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
- 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) + snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present); + snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid); + if (!eld->eld_valid) 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); @@ -535,7 +537,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, static void hdmi_write_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) { - struct hdmi_eld *e = entry->private_data; + struct hdmi_eld *eld = entry->private_data; + struct parsed_hdmi_eld *e = &eld->info; char line[64]; char name[64]; char *sname; @@ -551,9 +554,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, * eld_version edid_version */ if (!strcmp(name, "monitor_present")) - e->monitor_present = val; + eld->monitor_present = val; else if (!strcmp(name, "eld_valid")) - e->eld_valid = val; + eld->eld_valid = val; else if (!strcmp(name, "connection_type")) e->conn_type = val; else if (!strcmp(name, "port_id")) @@ -627,7 +630,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) #endif /* CONFIG_PROC_FS */
/* update PCM info based on ELD */ -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo) { u32 rates; @@ -644,8 +647,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, formats = SNDRV_PCM_FMTBIT_S16_LE; maxbps = 16; channels_max = 2; - for (i = 0; i < eld->sad_count; i++) { - struct cea_sad *a = &eld->sad[i]; + for (i = 0; i < e->sad_count; i++) { + struct cea_sad *a = &e->sad[i]; rates |= a->rates; if (a->channels > channels_max) channels_max = a->channels; diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..11fd5f5 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -713,10 +713,10 @@ struct cea_sad { /* * ELD: EDID Like Data */ -struct hdmi_eld { - bool monitor_present; - bool eld_valid; - int eld_size; +struct parsed_hdmi_eld { + /* + * all fields will be cleared before updating ELD + */ int baseline_len; int eld_ver; int cea_edid_ver; @@ -731,10 +731,14 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD]; - /* - * all fields above eld_buffer will be cleared before updating ELD - */ +}; + +struct hdmi_eld { + bool monitor_present; + bool eld_valid; + int eld_size; char eld_buffer[ELD_MAX_SIZE]; + struct parsed_hdmi_eld info; #ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif @@ -742,8 +746,8 @@ struct hdmi_eld {
int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); -void snd_hdmi_show_eld(struct hdmi_eld *eld); -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e); +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo);
#ifdef CONFIG_PROC_FS diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..2ac6635 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) * expand ELD's notions to match the ones used by Audio InfoFrame. */ for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) { - if (eld->spk_alloc & (1 << i)) + if (eld->info.spk_alloc & (1 << i)) spk_mask |= eld_speaker_allocation_bits[i]; }
@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) } }
- snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf)); + snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf)); snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n", ca, channels, buf);
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, ca = 0;
memset(&ai, 0, sizeof(ai)); - if (eld->conn_type == 0) { /* HDMI */ + if (eld->info.conn_type == 0) { /* HDMI */ struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
hdmi_ai->type = 0x84; @@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, hdmi_ai->CC02_CT47 = channels - 1; hdmi_ai->CA = ca; hdmi_checksum_audio_infoframe(hdmi_ai); - } else if (eld->conn_type == 1) { /* DisplayPort */ + } else if (eld->info.conn_type == 1) { /* DisplayPort */ struct dp_audio_infoframe *dp_ai = &ai.dp;
dp_ai->type = 0x84; @@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
/* Restrict capabilities by ELD if this isn't disabled */ if (!static_hdmi_pcm && eld->eld_valid) { - snd_hdmi_eld_update_pcm_info(eld, hinfo); + snd_hdmi_eld_update_pcm_info(&eld->info, hinfo); if (hinfo->channels_min > hinfo->channels_max || !hinfo->rates || !hinfo->formats) { per_cvt->assigned = 0; @@ -1188,7 +1188,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) eld->eld_valid = false; if (eld_valid) { if (!snd_hdmi_get_eld(eld, codec, pin_nid)) - snd_hdmi_show_eld(eld); + snd_hdmi_show_eld(&eld->info); else if (repoll) { queue_delayed_work(codec->bus->workq, &per_pin->work,
At Tue, 19 Feb 2013 11:51:12 +0100, David Henningsson wrote:
For better readability, the information that is parsed out of the ELD data is now put into a separate parsed_hdmi_eld struct.
Signed-off-by: David Henningsson david.henningsson@canonical.com
You seem to have forgotten to change memset() call in hdmi_present_sense().
One thing I feel a bit uneasy is that the whole struct hdmi_eld is passed in the next patch as spec->temp_eld. It contains uninitialized proc entry and mutex.
If we split the data and the control objects, can we pass only the data struct to snd_hdmi_get_eld() instead of passing the whole struct hdmi_eld? If so, the question is where should fields like monitor_present, etc, belong to.
thanks,
Takashi
sound/pci/hda/hda_eld.c | 41 ++++++++++++++++++++++------------------- sound/pci/hda/hda_local.h | 22 +++++++++++++--------- sound/pci/hda/patch_hdmi.c | 12 ++++++------ 3 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..aecda72 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a, /*
- Be careful, ELD buf could be totally rubbish!
*/ -static int hdmi_update_eld(struct hdmi_eld *e,
const unsigned char *buf, int size)+static int hdmi_parse_eld(struct parsed_hdmi_eld *e,
const unsigned char *buf, int size){ int mnl; int i; @@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, goto out_fail; }
- e->eld_size = size; e->baseline_len = GRAB_BITS(buf, 2, 0, 8); mnl = GRAB_BITS(buf, 4, 0, 5); e->cea_edid_ver = GRAB_BITS(buf, 4, 5, 3);
@@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, if (!e->spk_alloc) e->spk_alloc = 0xffff;
- e->eld_valid = true; return 0;
out_fail: @@ -328,7 +326,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
/* * ELD size is initialized to zero in caller function. If no errors and
* ELD is valid, actual eld_size is assigned in hdmi_update_eld()
* ELD is valid, actual eld_size is assigned.*/
size = snd_hdmi_get_eld_size(codec, nid);
@@ -372,8 +370,11 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, buf[i] = val; }
- ret = hdmi_update_eld(eld, buf, size);
if ((ret = hdmi_parse_eld(&eld->info, buf, size)) < 0)
goto error;eld->eld_size = size;
eld->eld_valid = true;
error: return ret; } @@ -438,7 +439,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen) buf[j] = '\0'; /* necessary when j == 0 */ }
-void snd_hdmi_show_eld(struct hdmi_eld *e) +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e) { int i;
@@ -487,10 +488,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a, static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; 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 +507,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
- 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)
- snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
- snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
- if (!eld->eld_valid) 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]);@@ -535,7 +537,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, static void hdmi_write_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; char line[64]; char name[64]; char *sname;
@@ -551,9 +554,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, * eld_version edid_version */ if (!strcmp(name, "monitor_present"))
e->monitor_present = val;
eld->monitor_present = val;
e->eld_valid = val;
eld->eld_valid = val;@@ -627,7 +630,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) #endif /* CONFIG_PROC_FS */
/* update PCM info based on ELD */ -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo) { u32 rates; @@ -644,8 +647,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, formats = SNDRV_PCM_FMTBIT_S16_LE; maxbps = 16; channels_max = 2;
- for (i = 0; i < eld->sad_count; i++) {
struct cea_sad *a = &eld->sad[i];
- for (i = 0; i < e->sad_count; i++) {
struct cea_sad *a = &e->sad[i];diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..11fd5f5 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -713,10 +713,10 @@ struct cea_sad { /*
- ELD: EDID Like Data
*/ -struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size;
+struct parsed_hdmi_eld {
- /*
* all fields will be cleared before updating ELD*/@@ -731,10 +731,14 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD];
- /*
* all fields above eld_buffer will be cleared before updating ELD*/+};
+struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size; char eld_buffer[ELD_MAX_SIZE];
- struct parsed_hdmi_eld info;
#ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif @@ -742,8 +746,8 @@ struct hdmi_eld {
int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); -void snd_hdmi_show_eld(struct hdmi_eld *eld); -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e); +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo);
#ifdef CONFIG_PROC_FS diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..2ac6635 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) * expand ELD's notions to match the ones used by Audio InfoFrame. */ for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) {
if (eld->spk_alloc & (1 << i))
if (eld->info.spk_alloc & (1 << i)) spk_mask |= eld_speaker_allocation_bits[i];@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) } }
- snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf));
- snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf)); snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n", ca, channels, buf);
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, ca = 0;
memset(&ai, 0, sizeof(ai));
- if (eld->conn_type == 0) { /* HDMI */
if (eld->info.conn_type == 0) { /* HDMI */ struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
hdmi_ai->type = 0x84;
@@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, hdmi_ai->CC02_CT47 = channels - 1; hdmi_ai->CA = ca; hdmi_checksum_audio_infoframe(hdmi_ai);
- } else if (eld->conn_type == 1) { /* DisplayPort */
} else if (eld->info.conn_type == 1) { /* DisplayPort */ struct dp_audio_infoframe *dp_ai = &ai.dp;
dp_ai->type = 0x84;
@@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
/* Restrict capabilities by ELD if this isn't disabled */ if (!static_hdmi_pcm && eld->eld_valid) {
snd_hdmi_eld_update_pcm_info(eld, hinfo);
snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);@@ -1188,7 +1188,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) eld->eld_valid = false; if (eld_valid) { if (!snd_hdmi_get_eld(eld, codec, pin_nid))
snd_hdmi_show_eld(eld);
snd_hdmi_show_eld(&eld->info);-- 1.7.9.5
On 02/19/2013 12:07 PM, Takashi Iwai wrote:
At Tue, 19 Feb 2013 11:51:12 +0100, David Henningsson wrote:
For better readability, the information that is parsed out of the ELD data is now put into a separate parsed_hdmi_eld struct.
Signed-off-by: David Henningsson david.henningsson@canonical.com
You seem to have forgotten to change memset() call in hdmi_present_sense().
Ok, will fix.
One thing I feel a bit uneasy is that the whole struct hdmi_eld is passed in the next patch as spec->temp_eld. It contains uninitialized proc entry and mutex.
If we split the data and the control objects, can we pass only the data struct to snd_hdmi_get_eld() instead of passing the whole struct hdmi_eld? If so, the question is where should fields like monitor_present, etc, belong to.
What do you think of the attached version of patch 3/5? If you like it I'll continue with fixing up 4/5 and 5/5 to match.
thanks,
Takashi
sound/pci/hda/hda_eld.c | 41 ++++++++++++++++++++++------------------- sound/pci/hda/hda_local.h | 22 +++++++++++++--------- sound/pci/hda/patch_hdmi.c | 12 ++++++------ 3 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..aecda72 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a, /*
- Be careful, ELD buf could be totally rubbish!
*/ -static int hdmi_update_eld(struct hdmi_eld *e,
const unsigned char *buf, int size)+static int hdmi_parse_eld(struct parsed_hdmi_eld *e,
const unsigned char *buf, int size)@@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, goto out_fail; }
- e->eld_size = size; e->baseline_len = GRAB_BITS(buf, 2, 0, 8); mnl = GRAB_BITS(buf, 4, 0, 5); e->cea_edid_ver = GRAB_BITS(buf, 4, 5, 3);
@@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, if (!e->spk_alloc) e->spk_alloc = 0xffff;
e->eld_valid = true; return 0;
out_fail:
@@ -328,7 +326,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
/* * ELD size is initialized to zero in caller function. If no errors and
* ELD is valid, actual eld_size is assigned in hdmi_update_eld()
* ELD is valid, actual eld_size is assigned.*/
size = snd_hdmi_get_eld_size(codec, nid);
@@ -372,8 +370,11 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, buf[i] = val; }
- ret = hdmi_update_eld(eld, buf, size);
if ((ret = hdmi_parse_eld(&eld->info, buf, size)) < 0)
goto error;eld->eld_size = size;
eld->eld_valid = true; error: return ret; }
@@ -438,7 +439,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen) buf[j] = '\0'; /* necessary when j == 0 */ }
-void snd_hdmi_show_eld(struct hdmi_eld *e) +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e) { int i;
@@ -487,10 +488,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a, static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; 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 +507,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
- 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)
- snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
- snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
- if (!eld->eld_valid) 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]);@@ -535,7 +537,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, static void hdmi_write_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; char line[64]; char name[64]; char *sname;
@@ -551,9 +554,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, * eld_version edid_version */ if (!strcmp(name, "monitor_present"))
e->monitor_present = val;
eld->monitor_present = val;
e->eld_valid = val;
eld->eld_valid = val;@@ -627,7 +630,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) #endif /* CONFIG_PROC_FS */
/* update PCM info based on ELD */ -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo) { u32 rates; @@ -644,8 +647,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, formats = SNDRV_PCM_FMTBIT_S16_LE; maxbps = 16; channels_max = 2;
- for (i = 0; i < eld->sad_count; i++) {
struct cea_sad *a = &eld->sad[i];
- for (i = 0; i < e->sad_count; i++) {
struct cea_sad *a = &e->sad[i];diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..11fd5f5 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -713,10 +713,10 @@ struct cea_sad { /*
- ELD: EDID Like Data
*/ -struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size;
+struct parsed_hdmi_eld {
- /*
* all fields will be cleared before updating ELD*/@@ -731,10 +731,14 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD];
- /*
* all fields above eld_buffer will be cleared before updating ELD*/+};
+struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size; char eld_buffer[ELD_MAX_SIZE];
- struct parsed_hdmi_eld info; #ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif
@@ -742,8 +746,8 @@ struct hdmi_eld {
int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); -void snd_hdmi_show_eld(struct hdmi_eld *eld); -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e); +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo);
#ifdef CONFIG_PROC_FS diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..2ac6635 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) * expand ELD's notions to match the ones used by Audio InfoFrame. */ for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) {
if (eld->spk_alloc & (1 << i))
if (eld->info.spk_alloc & (1 << i)) spk_mask |= eld_speaker_allocation_bits[i];@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) } }
- snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf));
- snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf)); snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n", ca, channels, buf);
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, ca = 0;
memset(&ai, 0, sizeof(ai));
- if (eld->conn_type == 0) { /* HDMI */
if (eld->info.conn_type == 0) { /* HDMI */ struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
hdmi_ai->type = 0x84;
@@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, hdmi_ai->CC02_CT47 = channels - 1; hdmi_ai->CA = ca; hdmi_checksum_audio_infoframe(hdmi_ai);
- } else if (eld->conn_type == 1) { /* DisplayPort */
} else if (eld->info.conn_type == 1) { /* DisplayPort */ struct dp_audio_infoframe *dp_ai = &ai.dp;
dp_ai->type = 0x84;
@@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
/* Restrict capabilities by ELD if this isn't disabled */ if (!static_hdmi_pcm && eld->eld_valid) {
snd_hdmi_eld_update_pcm_info(eld, hinfo);
snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);@@ -1188,7 +1188,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) eld->eld_valid = false; if (eld_valid) { if (!snd_hdmi_get_eld(eld, codec, pin_nid))
snd_hdmi_show_eld(eld);
snd_hdmi_show_eld(&eld->info);-- 1.7.9.5
At Tue, 19 Feb 2013 12:43:46 +0100, David Henningsson wrote:
On 02/19/2013 12:07 PM, Takashi Iwai wrote:
At Tue, 19 Feb 2013 11:51:12 +0100, David Henningsson wrote:
For better readability, the information that is parsed out of the ELD data is now put into a separate parsed_hdmi_eld struct.
Signed-off-by: David Henningsson david.henningsson@canonical.com
You seem to have forgotten to change memset() call in hdmi_present_sense().
Ok, will fix.
One thing I feel a bit uneasy is that the whole struct hdmi_eld is passed in the next patch as spec->temp_eld. It contains uninitialized proc entry and mutex.
If we split the data and the control objects, can we pass only the data struct to snd_hdmi_get_eld() instead of passing the whole struct hdmi_eld? If so, the question is where should fields like monitor_present, etc, belong to.
What do you think of the attached version of patch 3/5? If you like it I'll continue with fixing up 4/5 and 5/5 to match.
This looks better. Thanks!
Takashi
thanks,
Takashi
sound/pci/hda/hda_eld.c | 41 ++++++++++++++++++++++------------------- sound/pci/hda/hda_local.h | 22 +++++++++++++--------- sound/pci/hda/patch_hdmi.c | 12 ++++++------ 3 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..aecda72 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a, /*
- Be careful, ELD buf could be totally rubbish!
*/ -static int hdmi_update_eld(struct hdmi_eld *e,
const unsigned char *buf, int size)+static int hdmi_parse_eld(struct parsed_hdmi_eld *e,
const unsigned char *buf, int size)@@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, goto out_fail; }
- e->eld_size = size; e->baseline_len = GRAB_BITS(buf, 2, 0, 8); mnl = GRAB_BITS(buf, 4, 0, 5); e->cea_edid_ver = GRAB_BITS(buf, 4, 5, 3);
@@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, if (!e->spk_alloc) e->spk_alloc = 0xffff;
e->eld_valid = true; return 0;
out_fail:
@@ -328,7 +326,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
/* * ELD size is initialized to zero in caller function. If no errors and
* ELD is valid, actual eld_size is assigned in hdmi_update_eld()
* ELD is valid, actual eld_size is assigned.*/
size = snd_hdmi_get_eld_size(codec, nid);
@@ -372,8 +370,11 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, buf[i] = val; }
- ret = hdmi_update_eld(eld, buf, size);
if ((ret = hdmi_parse_eld(&eld->info, buf, size)) < 0)
goto error;eld->eld_size = size;
eld->eld_valid = true; error: return ret; }
@@ -438,7 +439,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen) buf[j] = '\0'; /* necessary when j == 0 */ }
-void snd_hdmi_show_eld(struct hdmi_eld *e) +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e) { int i;
@@ -487,10 +488,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a, static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; 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 +507,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
- 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)
- snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
- snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
- if (!eld->eld_valid) 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]);@@ -535,7 +537,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, static void hdmi_write_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; char line[64]; char name[64]; char *sname;
@@ -551,9 +554,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, * eld_version edid_version */ if (!strcmp(name, "monitor_present"))
e->monitor_present = val;
eld->monitor_present = val;
e->eld_valid = val;
eld->eld_valid = val;@@ -627,7 +630,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) #endif /* CONFIG_PROC_FS */
/* update PCM info based on ELD */ -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo) { u32 rates; @@ -644,8 +647,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, formats = SNDRV_PCM_FMTBIT_S16_LE; maxbps = 16; channels_max = 2;
- for (i = 0; i < eld->sad_count; i++) {
struct cea_sad *a = &eld->sad[i];
- for (i = 0; i < e->sad_count; i++) {
struct cea_sad *a = &e->sad[i];diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..11fd5f5 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -713,10 +713,10 @@ struct cea_sad { /*
- ELD: EDID Like Data
*/ -struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size;
+struct parsed_hdmi_eld {
- /*
* all fields will be cleared before updating ELD*/@@ -731,10 +731,14 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD];
- /*
* all fields above eld_buffer will be cleared before updating ELD*/+};
+struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size; char eld_buffer[ELD_MAX_SIZE];
- struct parsed_hdmi_eld info; #ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif
@@ -742,8 +746,8 @@ struct hdmi_eld {
int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); -void snd_hdmi_show_eld(struct hdmi_eld *eld); -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e); +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo);
#ifdef CONFIG_PROC_FS diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..2ac6635 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) * expand ELD's notions to match the ones used by Audio InfoFrame. */ for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) {
if (eld->spk_alloc & (1 << i))
if (eld->info.spk_alloc & (1 << i)) spk_mask |= eld_speaker_allocation_bits[i];@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) } }
- snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf));
- snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf)); snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n", ca, channels, buf);
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, ca = 0;
memset(&ai, 0, sizeof(ai));
- if (eld->conn_type == 0) { /* HDMI */
if (eld->info.conn_type == 0) { /* HDMI */ struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
hdmi_ai->type = 0x84;
@@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, hdmi_ai->CC02_CT47 = channels - 1; hdmi_ai->CA = ca; hdmi_checksum_audio_infoframe(hdmi_ai);
- } else if (eld->conn_type == 1) { /* DisplayPort */
} else if (eld->info.conn_type == 1) { /* DisplayPort */ struct dp_audio_infoframe *dp_ai = &ai.dp;
dp_ai->type = 0x84;
@@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
/* Restrict capabilities by ELD if this isn't disabled */ if (!static_hdmi_pcm && eld->eld_valid) {
snd_hdmi_eld_update_pcm_info(eld, hinfo);
snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);@@ -1188,7 +1188,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) eld->eld_valid = false; if (eld_valid) { if (!snd_hdmi_get_eld(eld, codec, pin_nid))
snd_hdmi_show_eld(eld);
snd_hdmi_show_eld(&eld->info);-- 1.7.9.5
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic [2 0001-ALSA-hda-hdmi-Refactor-hdmi_eld-into-parsed_hdmi_eld.patch <text/x-patch (7bit)>]
From 80aca324ff81f630006aeb18f22b78f46428bad3 Mon Sep 17 00:00:00 2001
From: David Henningsson david.henningsson@canonical.com Date: Tue, 19 Feb 2013 10:59:34 +0100 Subject: [PATCH] ALSA: hda - hdmi: Refactor hdmi_eld into parsed_hdmi_eld
For better readability, the information that is parsed out of the ELD data is now put into a separate parsed_hdmi_eld struct.
Signed-off-by: David Henningsson david.henningsson@canonical.com
sound/pci/hda/hda_eld.c | 46 ++++++++++++++++++++------------------------ sound/pci/hda/hda_local.h | 27 ++++++++++++++++---------- sound/pci/hda/patch_hdmi.c | 24 +++++++++++++++-------- 3 files changed, 54 insertions(+), 43 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 4c054f4..16066d7 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a, /*
- Be careful, ELD buf could be totally rubbish!
*/ -static int hdmi_update_eld(struct hdmi_eld *e,
const unsigned char *buf, int size)+int snd_hdmi_parse_eld(struct parsed_hdmi_eld *e,
const unsigned char *buf, int size){ int mnl; int i; @@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, goto out_fail; }
- e->eld_size = size; e->baseline_len = GRAB_BITS(buf, 2, 0, 8); mnl = GRAB_BITS(buf, 4, 0, 5); e->cea_edid_ver = GRAB_BITS(buf, 4, 5, 3);
@@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e, if (!e->spk_alloc) e->spk_alloc = 0xffff;
- e->eld_valid = true; return 0;
out_fail: @@ -318,17 +316,16 @@ int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid) AC_DIPSIZE_ELD_BUF); }
-int snd_hdmi_get_eld(struct hdmi_eld *eld,
struct hda_codec *codec, hda_nid_t nid)+int snd_hdmi_get_eld(struct hda_codec *codec, hda_nid_t nid,
unsigned char *buf, int *eld_size){ int i; int ret; int size;
unsigned char *buf;
/*
- ELD size is initialized to zero in caller function. If no errors and
* ELD is valid, actual eld_size is assigned in hdmi_update_eld()
* ELD is valid, actual eld_size is assigned.*/
size = snd_hdmi_get_eld_size(codec, nid);
@@ -343,8 +340,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, }
/* set ELD buffer */
- buf = eld->eld_buffer;
- for (i = 0; i < size; i++) { unsigned int val = hdmi_get_eld_data(codec, nid, i); /*
@@ -372,8 +367,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, buf[i] = val; }
- ret = hdmi_update_eld(eld, buf, size);
- *eld_size = size;
error: return ret; } @@ -438,7 +432,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen) buf[j] = '\0'; /* necessary when j == 0 */ }
-void snd_hdmi_show_eld(struct hdmi_eld *e) +void snd_hdmi_show_eld(struct parsed_hdmi_eld *e) { int i;
@@ -487,10 +481,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a, static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; 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 +500,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
- 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)
- snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
- snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
- if (!eld->eld_valid) 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]);@@ -535,7 +530,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, static void hdmi_write_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) {
- struct hdmi_eld *e = entry->private_data;
- struct hdmi_eld *eld = entry->private_data;
- struct parsed_hdmi_eld *e = &eld->info; char line[64]; char name[64]; char *sname;
@@ -551,9 +547,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, * eld_version edid_version */ if (!strcmp(name, "monitor_present"))
e->monitor_present = val;
eld->monitor_present = val;
e->eld_valid = val;
eld->eld_valid = val;@@ -627,7 +623,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) #endif /* CONFIG_PROC_FS */
/* update PCM info based on ELD */ -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo) { u32 rates; @@ -644,8 +640,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, formats = SNDRV_PCM_FMTBIT_S16_LE; maxbps = 16; channels_max = 2;
- for (i = 0; i < eld->sad_count; i++) {
struct cea_sad *a = &eld->sad[i];
- for (i = 0; i < e->sad_count; i++) {
struct cea_sad *a = &e->sad[i];diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 05f1d59..363cd48 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -713,10 +713,10 @@ struct cea_sad { /*
- ELD: EDID Like Data
*/ -struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size;
+struct parsed_hdmi_eld {
- /*
* all fields will be cleared before updating ELD*/@@ -731,19 +731,26 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD];
- /*
* all fields above eld_buffer will be cleared before updating ELD*/+};
+struct hdmi_eld {
- bool monitor_present;
- bool eld_valid;
- int eld_size; char eld_buffer[ELD_MAX_SIZE];
- struct parsed_hdmi_eld info;
#ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif };
int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); -int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); -void snd_hdmi_show_eld(struct hdmi_eld *eld); -void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld, +int snd_hdmi_get_eld(struct hda_codec *codec, hda_nid_t nid,
unsigned char *buf, int *eld_size);+int snd_hdmi_parse_eld(struct parsed_hdmi_eld *e,
const unsigned char *buf, int size);+void snd_hdmi_show_eld(struct parsed_hdmi_eld *e); +void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e, struct hda_pcm_stream *hinfo);
#ifdef CONFIG_PROC_FS diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7a73dfb..dab147a 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) * expand ELD's notions to match the ones used by Audio InfoFrame. */ for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) {
if (eld->spk_alloc & (1 << i))
if (eld->info.spk_alloc & (1 << i)) spk_mask |= eld_speaker_allocation_bits[i];@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels) } }
- snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf));
- snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf)); snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n", ca, channels, buf);
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, ca = 0;
memset(&ai, 0, sizeof(ai));
- if (eld->conn_type == 0) { /* HDMI */
if (eld->info.conn_type == 0) { /* HDMI */ struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
hdmi_ai->type = 0x84;
@@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, hdmi_ai->CC02_CT47 = channels - 1; hdmi_ai->CA = ca; hdmi_checksum_audio_infoframe(hdmi_ai);
- } else if (eld->conn_type == 1) { /* DisplayPort */
} else if (eld->info.conn_type == 1) { /* DisplayPort */ struct dp_audio_infoframe *dp_ai = &ai.dp;
dp_ai->type = 0x84;
@@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
/* Restrict capabilities by ELD if this isn't disabled */ if (!static_hdmi_pcm && eld->eld_valid) {
snd_hdmi_eld_update_pcm_info(eld, hinfo);
snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);@@ -1185,10 +1185,18 @@ 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);
if (snd_hdmi_get_eld(codec, pin_nid, eld->eld_buffer,&eld->eld_size) < 0)eld_valid = false;if (eld_valid && snd_hdmi_parse_eld(&eld->info, eld->eld_buffer,eld->eld_size) < 0)eld_valid = false;if (eld_valid) {snd_hdmi_show_eld(&eld->info);eld->eld_valid = true;}-- 1.7.9.5
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 mutex to guarantee consistency.
To avoid holding the mutex while reading the ELD info from the codec, we introduce a temporary eld buffer. --- sound/pci/hda/hda_eld.c | 8 +++++++- sound/pci/hda/hda_local.h | 1 + sound/pci/hda/patch_hdmi.c | 44 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index aecda72..439c2e6 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -507,10 +507,13 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry, [4 ... 7] = "reserved" };
+ mutex_lock(&eld->lock); snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present); snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid); - if (!eld->eld_valid) + if (!eld->eld_valid) { + mutex_unlock(&eld->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]); @@ -532,6 +535,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); + mutex_unlock(&eld->lock); }
static void hdmi_write_eld_info(struct snd_info_entry *entry, @@ -545,6 +549,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, long long val; unsigned int n;
+ mutex_lock(&eld->lock); while (!snd_info_get_line(buffer, line, sizeof(line))) { if (sscanf(line, "%s %llx", name, &val) != 2) continue; @@ -596,6 +601,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry, e->sad_count = n + 1; } } + mutex_unlock(&eld->lock); }
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 11fd5f5..1f83dc3 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -739,6 +739,7 @@ struct hdmi_eld { int eld_size; 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 diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 2ac6635..0e1d69d 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;
+ mutex_lock(&eld->lock); uinfo->count = eld->eld_valid ? eld->eld_size : 0; + mutex_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;
+ mutex_lock(&eld->lock); if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) { + mutex_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); + mutex_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,45 @@ 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; + bool eld_changed = 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); + pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); + if (pin_eld->monitor_present) + 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->info); + update_eld = true; + } else if (repoll) { queue_delayed_work(codec->bus->workq, &per_pin->work, msecs_to_jiffies(300)); } } + + mutex_lock(&pin_eld->lock); + if (pin_eld->eld_valid && !eld->eld_valid) + update_eld = true; + if (update_eld) { + pin_eld->eld_valid = eld->eld_valid; + eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer, + eld->eld_size) != 0; + if (eld_changed) + memcpy(pin_eld->eld_buffer, eld->eld_buffer, + eld->eld_size); + pin_eld->eld_size = eld->eld_size; + pin_eld->info = eld->info; + } + mutex_unlock(&pin_eld->lock); }
static void hdmi_repoll_eld(struct work_struct *work) @@ -1662,6 +1687,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec) struct hdmi_eld *eld = &per_pin->sink_eld;
per_pin->codec = codec; + mutex_init(&eld->lock); INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld); snd_hda_eld_proc_new(codec, eld, pin_idx); }
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 0e1d69d..b270dc0 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; }
@@ -1216,10 +1218,16 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) if (eld_changed) memcpy(pin_eld->eld_buffer, eld->eld_buffer, eld->eld_size); + eld_changed |= pin_eld->eld_size != eld->eld_size; pin_eld->eld_size = eld->eld_size; pin_eld->info = eld->info; } mutex_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)
participants (2)
-
David Henningsson -
Takashi Iwai