* [PATCH 0/3] Make HDMI ELD usable with hotplug
@ 2013-02-18 13:11 David Henningsson
2013-02-18 13:11 ` [PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: David Henningsson @ 2013-02-18 13:11 UTC (permalink / raw)
To: tiwai, alsa-devel, fengguang.wu, pierre-louis.bossart; +Cc: David Henningsson
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(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug 2013-02-18 13:11 [PATCH 0/3] Make HDMI ELD usable with hotplug David Henningsson @ 2013-02-18 13:11 ` David Henningsson 2013-02-18 13:11 ` [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: David Henningsson @ 2013-02-18 13:11 UTC (permalink / raw) To: tiwai, alsa-devel, fengguang.wu, pierre-louis.bossart; +Cc: David Henningsson 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); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid 2013-02-18 13:11 [PATCH 0/3] Make HDMI ELD usable with hotplug David Henningsson 2013-02-18 13:11 ` [PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson @ 2013-02-18 13:11 ` David Henningsson 2013-02-18 14:39 ` Takashi Iwai 2013-02-18 13:11 ` [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson 2013-02-18 13:35 ` [PATCH 0/3] Make HDMI ELD usable with hotplug Takashi Iwai 3 siblings, 1 reply; 19+ messages in thread From: David Henningsson @ 2013-02-18 13:11 UTC (permalink / raw) To: tiwai, alsa-devel, fengguang.wu, pierre-louis.bossart; +Cc: 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 | 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; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid 2013-02-18 13:11 ` [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson @ 2013-02-18 14:39 ` Takashi Iwai 0 siblings, 0 replies; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 14:39 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 13:11 [PATCH 0/3] Make HDMI ELD usable with hotplug David Henningsson 2013-02-18 13:11 ` [PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson 2013-02-18 13:11 ` [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson @ 2013-02-18 13:11 ` David Henningsson 2013-02-18 14:41 ` Takashi Iwai 2013-02-18 13:35 ` [PATCH 0/3] Make HDMI ELD usable with hotplug Takashi Iwai 3 siblings, 1 reply; 19+ messages in thread From: David Henningsson @ 2013-02-18 13:11 UTC (permalink / raw) To: tiwai, alsa-devel, fengguang.wu, pierre-louis.bossart; +Cc: David Henningsson 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) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 13:11 ` [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson @ 2013-02-18 14:41 ` Takashi Iwai 2013-02-18 14:46 ` David Henningsson 0 siblings, 1 reply; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 14:41 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 14:41 ` Takashi Iwai @ 2013-02-18 14:46 ` David Henningsson 2013-02-18 14:55 ` Takashi Iwai 0 siblings, 1 reply; 19+ messages in thread From: David Henningsson @ 2013-02-18 14:46 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 14:46 ` David Henningsson @ 2013-02-18 14:55 ` Takashi Iwai 2013-02-18 15:09 ` David Henningsson 0 siblings, 1 reply; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 14:55 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 14:55 ` Takashi Iwai @ 2013-02-18 15:09 ` David Henningsson 2013-02-18 15:14 ` Takashi Iwai 0 siblings, 1 reply; 19+ messages in thread From: David Henningsson @ 2013-02-18 15:09 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 15:09 ` David Henningsson @ 2013-02-18 15:14 ` Takashi Iwai 2013-02-18 16:08 ` David Henningsson 0 siblings, 1 reply; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 15:14 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 15:14 ` Takashi Iwai @ 2013-02-18 16:08 ` David Henningsson 2013-02-18 16:21 ` Takashi Iwai 0 siblings, 1 reply; 19+ messages in thread From: David Henningsson @ 2013-02-18 16:08 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes 2013-02-18 16:08 ` David Henningsson @ 2013-02-18 16:21 ` Takashi Iwai 0 siblings, 0 replies; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 16:21 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, fengguang.wu, pierre-louis.bossart 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 13:11 [PATCH 0/3] Make HDMI ELD usable with hotplug David Henningsson ` (2 preceding siblings ...) 2013-02-18 13:11 ` [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson @ 2013-02-18 13:35 ` Takashi Iwai 2013-02-18 13:50 ` David Henningsson 3 siblings, 1 reply; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 13:35 UTC (permalink / raw) To: David Henningsson Cc: alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 13:35 ` [PATCH 0/3] Make HDMI ELD usable with hotplug Takashi Iwai @ 2013-02-18 13:50 ` David Henningsson 2013-02-18 14:28 ` Takashi Iwai 2013-02-18 22:39 ` Ville Syrjälä 0 siblings, 2 replies; 19+ messages in thread From: David Henningsson @ 2013-02-18 13:50 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 13:50 ` David Henningsson @ 2013-02-18 14:28 ` Takashi Iwai 2013-02-18 14:33 ` Takashi Iwai 2013-02-18 22:39 ` Ville Syrjälä 1 sibling, 1 reply; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 14:28 UTC (permalink / raw) To: David Henningsson Cc: alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 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? 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 14:28 ` Takashi Iwai @ 2013-02-18 14:33 ` Takashi Iwai 0 siblings, 0 replies; 19+ messages in thread From: Takashi Iwai @ 2013-02-18 14:33 UTC (permalink / raw) To: David Henningsson Cc: alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 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? > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 13:50 ` David Henningsson 2013-02-18 14:28 ` Takashi Iwai @ 2013-02-18 22:39 ` Ville Syrjälä 2013-02-19 2:39 ` Wang xingchao 2013-02-19 5:55 ` David Henningsson 1 sibling, 2 replies; 19+ messages in thread From: Ville Syrjälä @ 2013-02-18 22:39 UTC (permalink / raw) To: David Henningsson Cc: Takashi Iwai, alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 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? 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. -- Ville Syrjälä syrjala@sci.fi http://www.sci.fi/~syrjala/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 22:39 ` Ville Syrjälä @ 2013-02-19 2:39 ` Wang xingchao 2013-02-19 5:55 ` David Henningsson 1 sibling, 0 replies; 19+ messages in thread From: Wang xingchao @ 2013-02-19 2:39 UTC (permalink / raw) To: David Henningsson, Takashi Iwai, alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 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? > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Make HDMI ELD usable with hotplug 2013-02-18 22:39 ` Ville Syrjälä 2013-02-19 2:39 ` Wang xingchao @ 2013-02-19 5:55 ` David Henningsson 1 sibling, 0 replies; 19+ messages in thread From: David Henningsson @ 2013-02-19 5:55 UTC (permalink / raw) To: Takashi Iwai, alsa-devel, fengguang.wu, pierre-louis.bossart, Wang Xingchao 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 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? > > 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. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic [1] http://www.intel.com/content/www/us/en/standards/high-definition-audio-specification.html ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-02-19 5:55 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-18 13:11 [PATCH 0/3] Make HDMI ELD usable with hotplug David Henningsson 2013-02-18 13:11 ` [PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug David Henningsson 2013-02-18 13:11 ` [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid David Henningsson 2013-02-18 14:39 ` Takashi Iwai 2013-02-18 13:11 ` [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes David Henningsson 2013-02-18 14:41 ` Takashi Iwai 2013-02-18 14:46 ` David Henningsson 2013-02-18 14:55 ` Takashi Iwai 2013-02-18 15:09 ` David Henningsson 2013-02-18 15:14 ` Takashi Iwai 2013-02-18 16:08 ` David Henningsson 2013-02-18 16:21 ` Takashi Iwai 2013-02-18 13:35 ` [PATCH 0/3] Make HDMI ELD usable with hotplug Takashi Iwai 2013-02-18 13:50 ` David Henningsson 2013-02-18 14:28 ` Takashi Iwai 2013-02-18 14:33 ` Takashi Iwai 2013-02-18 22:39 ` Ville Syrjälä 2013-02-19 2:39 ` Wang xingchao 2013-02-19 5:55 ` David Henningsson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.