[alsa-devel] [PATCH 0/3] Make HDMI ELD usable with hotplug
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Could use advice on which ones of these that should or should not be sent to stable.
David Henningsson (3): 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: Notify userspace when ELD control changes
sound/pci/hda/patch_hdmi.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 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.
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 | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 32adaa6..9236cdb 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -343,14 +343,17 @@ 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; + + if (eld->eld_valid) + uinfo->count = eld->eld_valid ? eld->eld_size : 0;
return 0; } @@ -359,14 +362,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; }
At Mon, 18 Feb 2013 14:11:12 +0100, David Henningsson wrote:
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 | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 32adaa6..9236cdb 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -343,14 +343,17 @@ 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;
- if (eld->eld_valid)
uinfo->count = eld->eld_valid ? eld->eld_size : 0;
You don't have to check eld->eld_valid twice.
Takashi
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld; + struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm; @@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false; + bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } } + + if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) { + 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)
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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;
}
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
Takashi
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD? eld->eld_valid is never set to TRUE when repoll happens.
At Mon, 18 Feb 2013 15:46:04 +0100, David Henningsson wrote:
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD?
Yes.
eld->eld_valid is never set to TRUE when repoll happens.
But old_eld_valid might be TRUE, and you cleared eld->eld_valid to FALSE now before the actual probe result. Thus the check above passes and sends a notification although the actual probe isn't done.
Takashi
On 02/18/2013 03:55 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 15:46:04 +0100, David Henningsson wrote:
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD?
Yes.
eld->eld_valid is never set to TRUE when repoll happens.
But old_eld_valid might be TRUE, and you cleared eld->eld_valid to FALSE now before the actual probe result. Thus the check above passes and sends a notification although the actual probe isn't done.
If old_eld_valid is true, then we're talking about an unplug event. Why would there be a repoll when the monitor has disappeared?
But yes, the patch is based on the fact that ELD info never actually changes from one valid state to another valid state, without being invalid in between. If you think that could actually happen, maybe I need to add a memory compare of the old and new ELDs?
At Mon, 18 Feb 2013 16:09:37 +0100, David Henningsson wrote:
On 02/18/2013 03:55 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 15:46:04 +0100, David Henningsson wrote:
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD?
Yes.
eld->eld_valid is never set to TRUE when repoll happens.
But old_eld_valid might be TRUE, and you cleared eld->eld_valid to FALSE now before the actual probe result. Thus the check above passes and sends a notification although the actual probe isn't done.
If old_eld_valid is true, then we're talking about an unplug event. Why would there be a repoll when the monitor has disappeared?
But yes, the patch is based on the fact that ELD info never actually changes from one valid state to another valid state, without being invalid in between. If you think that could actually happen, maybe I need to add a memory compare of the old and new ELDs?
It'd be more robust, yes. We should take it into account that some unsol events might be missing or wrongly delivered.
Takashi
On 02/18/2013 04:14 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 16:09:37 +0100, David Henningsson wrote:
On 02/18/2013 03:55 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 15:46:04 +0100, David Henningsson wrote:
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote:
Since ELD sometimes becomes available a while after we have detected presence, 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9236cdb..d3b1a93 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin {
struct hda_codec *codec; struct hdmi_eld sink_eld;
- struct snd_kcontrol *eld_ctl; struct delayed_work work; int repoll_count; bool non_pcm;
@@ -406,6 +407,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; }
@@ -1175,6 +1177,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 eld_valid = false;
bool old_eld_valid = eld->eld_valid;
memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));@@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) msecs_to_jiffies(300)); } }
- if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) {
snd_ctl_notify(codec->bus->card,SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,&per_pin->eld_ctl->id);- }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD?
Yes.
eld->eld_valid is never set to TRUE when repoll happens.
But old_eld_valid might be TRUE, and you cleared eld->eld_valid to FALSE now before the actual probe result. Thus the check above passes and sends a notification although the actual probe isn't done.
If old_eld_valid is true, then we're talking about an unplug event. Why would there be a repoll when the monitor has disappeared?
But yes, the patch is based on the fact that ELD info never actually changes from one valid state to another valid state, without being invalid in between. If you think that could actually happen, maybe I need to add a memory compare of the old and new ELDs?
It'd be more robust, yes. We should take it into account that some unsol events might be missing or wrongly delivered.
The problem here is that there is no copy of the eld buffer. If we're talking about the - very hypothetical, I believe - case where we currently have valid eld data, and suddenly go into a repoll for some reason, the eld buffer might still have changed, e g if the mnl byte is wrong. Even if the lack of snd_ctl_notify makes it more unlikely that the temporary invalid ELD data will be read, there is still a chance that it will be.
Do you think we should allocate a temporary struct hdmi_eld, and only copy over if successful? (And if so, on the stack or through kalloc?) That seems to me like the only choice if we want total consistency on the ELD bytes kcontrol.
At Mon, 18 Feb 2013 17:08:36 +0100, David Henningsson wrote:
On 02/18/2013 04:14 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 16:09:37 +0100, David Henningsson wrote:
On 02/18/2013 03:55 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 15:46:04 +0100, David Henningsson wrote:
On 02/18/2013 03:41 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:13 +0100, David Henningsson wrote: > > Since ELD sometimes becomes available a while after we have detected > presence, 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 | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 9236cdb..d3b1a93 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -74,6 +74,7 @@ struct hdmi_spec_per_pin { > > struct hda_codec *codec; > struct hdmi_eld sink_eld; > + struct snd_kcontrol *eld_ctl; > struct delayed_work work; > int repoll_count; > bool non_pcm; > @@ -406,6 +407,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; > } > > @@ -1175,6 +1177,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 eld_valid = false; > + bool old_eld_valid = eld->eld_valid; > > memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer)); > > @@ -1196,6 +1199,13 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) > msecs_to_jiffies(300)); > } > } > + > + if (eld->eld_valid != old_eld_valid && per_pin->eld_ctl) { > + snd_ctl_notify(codec->bus->card, > + SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO, > + &per_pin->eld_ctl->id); > + }
This notification should be skipped when the pin sensing is requeued.
I don't understand this comment. Are you referring to repolling the ELD?
Yes.
eld->eld_valid is never set to TRUE when repoll happens.
But old_eld_valid might be TRUE, and you cleared eld->eld_valid to FALSE now before the actual probe result. Thus the check above passes and sends a notification although the actual probe isn't done.
If old_eld_valid is true, then we're talking about an unplug event. Why would there be a repoll when the monitor has disappeared?
But yes, the patch is based on the fact that ELD info never actually changes from one valid state to another valid state, without being invalid in between. If you think that could actually happen, maybe I need to add a memory compare of the old and new ELDs?
It'd be more robust, yes. We should take it into account that some unsol events might be missing or wrongly delivered.
The problem here is that there is no copy of the eld buffer. If we're talking about the - very hypothetical, I believe - case where we currently have valid eld data, and suddenly go into a repoll for some reason, the eld buffer might still have changed, e g if the mnl byte is wrong. Even if the lack of snd_ctl_notify makes it more unlikely that the temporary invalid ELD data will be read, there is still a chance that it will be.
Do you think we should allocate a temporary struct hdmi_eld, and only copy over if successful? (And if so, on the stack or through kalloc?) That seems to me like the only choice if we want total consistency on the ELD bytes kcontrol.
This would be one option. Or, if sending the notification early doesn't matter much for user-space, we should implement some locks against the race, at least. Sending a notification before finishing the whole read may trigger a race pretty easily.
Takashi
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao xingchao.wang@intel.com Date: Tue Jan 22 23:25:25 2013 +0800
drm/i915: HDMI/DP - ELD info refresh support for Haswell
Takashi
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for Haswell
Thanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
At Mon, 18 Feb 2013 14:50:49 +0100, David Henningsson wrote:
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for HaswellThanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
OK, then it's not.
You are testing relatively new kernel, right? For example, the commits for drm/i915 b98b60167279df3acac9422c3c9820d9ebbcf9fb and 3cce574f0190dd149472059fb69267cf83d290f9 are relevant with HDMI audio to make unsol event working at hotplug/unplug. They should be in 3.7.
Takashi
At Mon, 18 Feb 2013 15:28:48 +0100, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:50:49 +0100, David Henningsson wrote:
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for HaswellThanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
OK, then it's not.
You are testing relatively new kernel, right? For example, the commits for drm/i915 b98b60167279df3acac9422c3c9820d9ebbcf9fb and 3cce574f0190dd149472059fb69267cf83d290f9 are relevant with HDMI audio to make unsol event working at hotplug/unplug. They should be in 3.7.
In anyway, your patches seem individual from the video driver side but the fixes only in the sound driver side, so better to apply.
Will comment on each patch later.
Takashi
On Mon, Feb 18, 2013 at 02:50:49PM +0100, David Henningsson wrote:
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for HaswellThanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
While reviewing that patch I asked whether we need something like that on other platforms. Unfortunately I didn't get a real answer. The logic in the patch would seem to apply equally to all platforms, not just HSW. We also seem to be doing silly things like flipping the ELD valid bit in the register on and off a few times when we update the ELD. But I'm not all that familiar with the topic so take my ramblings with a pinch of salt.
But in any case, if you're seeing a problem with the behaviour of i915 you should definitely report it.
On Tue, Feb 19, 2013 at 12:39:42AM +0200, Ville Syrjälä wrote:
On Mon, Feb 18, 2013 at 02:50:49PM +0100, David Henningsson wrote:
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for HaswellThanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
While reviewing that patch I asked whether we need something like that on other platforms. Unfortunately I didn't get a real answer. The logic in the patch would seem to apply equally to all platforms, not just HSW. We also seem to be doing silly things like flipping the ELD valid bit
Yes i agree it's a common issue should be fixed not only for HSW. In fact i started to fix the hotplug issue when it's reported from Chromeos, and the hardware is for Ivybridge/Sandybridge only. Later we find HSW has same problem and we fix it too. I donot have Arrandle HW for test/verify, but if it's same issue, i can continue to fix it.
thanks --xingchao
in the register on and off a few times when we update the ELD. But I'm not all that familiar with the topic so take my ramblings with a pinch of salt.
But in any case, if you're seeing a problem with the behaviour of i915 you should definitely report it.
-- Ville Syrjälä syrjala@sci.fi http://www.sci.fi/~syrjala/ _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 02/18/2013 11:39 PM, Ville Syrjälä wrote:
On Mon, Feb 18, 2013 at 02:50:49PM +0100, David Henningsson wrote:
On 02/18/2013 02:35 PM, Takashi Iwai wrote:
At Mon, 18 Feb 2013 14:11:10 +0100, David Henningsson wrote:
I'm experimenting with using ELD information in PulseAudio and found that it didn't work very well with hotplugging. Neither the proc file nor the ELD kcontrol was cleared correctly after unplug, and there was no way (except polling) to find out when ELD has become available.
Did you try Xingchao's patch for drm/i915? It's queued in drm-intel-next:
commit 7b9f35a6dd72f89452c58bbdbaf063027bf857ec Author: Wang Xingchao <xingchao.wang@intel.com> Date: Tue Jan 22 23:25:25 2013 +0800 drm/i915: HDMI/DP - ELD info refresh support for HaswellThanks for the hint. I'm currently developing not on a Haswell machine, but an (I think) Arrandale that I bought a few years ago. So I guess this patch does not have any effect on my hw?
While reviewing that patch I asked whether we need something like that on other platforms. Unfortunately I didn't get a real answer. The logic in the patch would seem to apply equally to all platforms, not just HSW. We also seem to be doing silly things like flipping the ELD valid bit in the register on and off a few times when we update the ELD. But I'm not all that familiar with the topic so take my ramblings with a pinch of salt.
But in any case, if you're seeing a problem with the behaviour of i915 you should definitely report it.
Looking at the HDA spec [1], e g "Figure 72: PD and ELDV unsolicited responses flow for digital display codecs" it looks like it's not valid to first send the PD bit and then ELDV a while later. So then I guess it's an issue that this actually seem to happen on the machine I use for testing this code.
From the same figure, it also looks like ELDV can be flipped back and forth (due to mode changes) when PD is true; so making patches that deal with PD and ELDV independently seem to be the right thing nonetheless.
participants (4)
-
David Henningsson -
Takashi Iwai -
Ville Syrjälä -
Wang xingchao