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

[Takashi Iwai <tiwai@suse.de>]

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