[PATCH 0/3] Make HDMI ELD usable with hotplug

[PATCH 0/3] Make HDMI ELD usable with hotplug

[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

[PATCH 1/3] ALSA: hda - hdmi: ELD shouldn't be valid after unplug

[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

[PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid

[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

[PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[Takashi Iwai]

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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[David Henningsson]

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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[Takashi Iwai]

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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[Takashi Iwai]

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

Re: [PATCH 2/3] ALSA: hda - hdmi: Do not expose eld data when eld is invalid

[Takashi Iwai]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[Takashi Iwai]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[David Henningsson]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[Takashi Iwai]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[David Henningsson]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[Takashi Iwai]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[David Henningsson]

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

Re: [PATCH 3/3] ALSA: hda - hdmi: Notify userspace when ELD control changes

[Takashi Iwai]

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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[Ville Syrjälä]

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/

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[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

Re: [PATCH 0/3] Make HDMI ELD usable with hotplug

[David Henningsson]

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