[alsa-devel] [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 at 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