| Message ID | 1451935145-24861-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/04/16 21:19, Philipp Zabel wrote: > Use HDMI connection / disconnection notifications to update an ALSA > jack object. Also make a copy of the ELD block after every change. > The jack part looks good to me AFAIU, but I think we should just update the local ELD copy with data given in the ELD notification. It then depends on the implementation details whether the get_eld() callback is needed at all or if it should be called once at the probe time. Then it is a separate issue to check if the currently played stream is still supported after an ELD update, and to decide what to do about it if it is not. Best regards, Jyri > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > include/sound/hdmi-codec.h | 6 ++++ > sound/soc/codecs/hdmi-codec.c | 72 ++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 77 insertions(+), 1 deletion(-) > > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h > index 15fe70f..8b6219d 100644 > --- a/include/sound/hdmi-codec.h > +++ b/include/sound/hdmi-codec.h > @@ -99,6 +99,12 @@ struct hdmi_codec_pdata { > int max_i2s_channels; > }; > > +struct snd_soc_codec; > +struct snd_soc_jack; > + > +int hdmi_codec_set_jack_detect(struct snd_soc_codec *codec, > + struct snd_soc_jack *jack); > + > #define HDMI_CODEC_DRV_NAME "hdmi-audio-codec" > > #endif /* __HDMI_CODEC_H__ */ > diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c > index 687332d..41f28ab 100644 > --- a/sound/soc/codecs/hdmi-codec.c > +++ b/sound/soc/codecs/hdmi-codec.c > @@ -12,9 +12,12 @@ > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > * General Public License for more details. > */ > +#include <linux/hdmi-not.h> > #include <linux/module.h> > +#include <linux/notifier.h> > #include <linux/string.h> > #include <sound/core.h> > +#include <sound/jack.h> > #include <sound/pcm.h> > #include <sound/pcm_params.h> > #include <sound/soc.h> > @@ -27,11 +30,15 @@ > struct hdmi_codec_priv { > struct hdmi_codec_pdata hcd; > struct snd_soc_dai_driver *daidrv; > + struct snd_soc_jack *jack; > struct hdmi_codec_daifmt daifmt[2]; > struct mutex current_stream_lock; > struct snd_pcm_substream *current_stream; > struct snd_pcm_hw_constraint_list ratec; > uint8_t eld[MAX_ELD_BYTES]; > + struct device *dev; > + struct notifier_block nb; > + unsigned int jack_status; > }; > > static const struct snd_soc_dapm_widget hdmi_widgets[] = { > @@ -326,6 +333,63 @@ static struct snd_soc_codec_driver hdmi_codec = { > .num_dapm_routes = ARRAY_SIZE(hdmi_routes), > }; > > +static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp, > + unsigned int jack_status) > +{ > + if (!hcp->jack) > + return; > + > + if (jack_status != hcp->jack_status) { > + snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT); > + hcp->jack_status = jack_status; > + } > +} > + > +static int hdmi_codec_notify(struct notifier_block *nb, unsigned long event, > + void *data) > +{ > + struct hdmi_codec_priv *hcp = container_of(nb, struct hdmi_codec_priv, > + nb); > + union hdmi_event *event_block = data; > + int ret; > + > + if (hcp->dev->parent != event_block->base.source) > + return NOTIFY_OK; > + > + if (!hcp->jack) > + return NOTIFY_OK; > + > + switch (event) { > + case HDMI_CONNECTED: > + hdmi_codec_jack_report(hcp, SND_JACK_LINEOUT); > + break; > + case HDMI_DISCONNECTED: > + hdmi_codec_jack_report(hcp, 0); > + break; > + case HDMI_NEW_ELD: > + if (hcp->hcd.ops->get_eld) > + ret = hcp->hcd.ops->get_eld(hcp->dev->parent, hcp->eld, > + sizeof(hcp->eld)); > + break; > + } > + > + return NOTIFY_OK; > +} > + > +int hdmi_codec_set_jack_detect(struct snd_soc_codec *codec, > + struct snd_soc_jack *jack) > +{ > + struct hdmi_codec_priv *hcp = snd_soc_codec_get_drvdata(codec); > + > + hcp->jack = jack; > + hcp->nb.notifier_call = hdmi_codec_notify; > + > + hdmi_register_notifier(&hcp->nb); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect); > + > static int hdmi_codec_probe(struct platform_device *pdev) > { > struct hdmi_codec_pdata *hcd = pdev->dev.platform_data; > @@ -370,6 +434,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) > if (hcd->spdif) > hcp->daidrv[i] = hdmi_spdif_dai; > > + dev_set_drvdata(dev, hcp); > + > ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv, > dai_count); > if (ret) { > @@ -378,12 +444,16 @@ static int hdmi_codec_probe(struct platform_device *pdev) > return ret; > } > > - dev_set_drvdata(dev, hcp); > + hcp->dev = dev; > + > return 0; > } > > static int hdmi_codec_remove(struct platform_device *pdev) > { > + struct hdmi_codec_priv *hcp = platform_get_drvdata(pdev); > + > + hdmi_unregister_notifier(&hcp->nb); > snd_soc_unregister_codec(&pdev->dev); > return 0; > } >
Hi Jyri, Am Donnerstag, den 07.01.2016, 19:09 +0200 schrieb Jyri Sarha: > On 01/04/16 21:19, Philipp Zabel wrote: > > Use HDMI connection / disconnection notifications to update an ALSA > > jack object. Also make a copy of the ELD block after every change. > > > > The jack part looks good to me AFAIU, If this is in principle an acceptable idea, this leaves the issue that the jack has an initial state and right now we can't query it but instead would have to wait for the first HDMI_(DIS)CONNECTED notification before it can be set. Would it make sense to add an is_connected callback to the hdmi_codec_ops or does this rather belong somewhere else? > but I think we should just update > the local ELD copy with data given in the ELD notification. It then > depends on the implementation details whether the get_eld() callback is > needed at all or if it should be called once at the probe time. Thanks, that sounds reasonable. > Then it is a separate issue to check if the currently played stream is > still supported after an ELD update, and to decide what to do about it > if it is not. [...] Possibly the HDMI driver itself should just mute if the format is not supported, and userspace should be notified? regards Philipp
On 01/08/16 10:43, Philipp Zabel wrote: > Hi Jyri, > > Am Donnerstag, den 07.01.2016, 19:09 +0200 schrieb Jyri Sarha: >> On 01/04/16 21:19, Philipp Zabel wrote: >>> Use HDMI connection / disconnection notifications to update an ALSA >>> jack object. Also make a copy of the ELD block after every change. >>> >> >> The jack part looks good to me AFAIU, > > If this is in principle an acceptable idea, this leaves the issue that > the jack has an initial state and right now we can't query it but > instead would have to wait for the first HDMI_(DIS)CONNECTED > notification before it can be set. Would it make sense to add an > is_connected callback to the hdmi_codec_ops or does this rather belong > somewhere else? > To me it would feel best if the hdmi notification would send the current edid, eld, and connection state when the hdmi notification is registered. But then again, I may not see the full picture. BTW, if we are going this way, the hdmi notification should be registered always in probe phase. I don't think the hdmi_codec_set_jack_detect() should be needed at all. Simple simple flags in struct hdmi_codec_pdata that tells if the jack detection is supported should do. Hmm, I am not sure if we need even that. >> but I think we should just update >> the local ELD copy with data given in the ELD notification. It then >> depends on the implementation details whether the get_eld() callback is >> needed at all or if it should be called once at the probe time. > > Thanks, that sounds reasonable. > >> Then it is a separate issue to check if the currently played stream is >> still supported after an ELD update, and to decide what to do about it >> if it is not. > [...] > > Possibly the HDMI driver itself should just mute if the format is not > supported, and userspace should be notified? > Another option would be simply aborting the playback. However having a mute control, that is forced to mute if the current stream is not supported, is an interesting idea. That would provide a way to notify the userspace without intrusively killing the stream. Cheers, Jyri
On Fri, Jan 08, 2016 at 11:57:11AM +0200, Jyri Sarha wrote: > On 01/08/16 10:43, Philipp Zabel wrote: > >Possibly the HDMI driver itself should just mute if the format is not > >supported, and userspace should be notified? > > Another option would be simply aborting the playback. However having a mute > control, that is forced to mute if the current stream is not supported, is > an interesting idea. That would provide a way to notify the userspace > without intrusively killing the stream. Firstly, remember that the sequence you'll likely see is: - disconnect - connect - new edid - new eld The disconnect/connect cycle is HDMI protocol for "please re-read the EDID" and is used (eg) by AV receivers to signal a change. When they change from HDMI pass-through mode in standby, to being powered on, they typically switch from passing the displays EDID through unmodified, to a modified version with their audio capabilities included - and vice versa when they're placed back into standby. The kernel's audio driver doesn't have any knowledge about the kind of audio being passed through it: while it knows the alleged sample rate, bits per sample, and number of channels, these are PCM details, and not compressed audio details. Compressed audio is normally sent with the kernel driver configured for 16 bit, 2 channel and an appropriate sample rate, with the IEC958 channel status data setup by the application (via libasound) to indicate non-audio amongst other details. Therefore, the kernel driver doesn't know a nature of the compressed audio stream, and can't "mute" the stream if the ELD indicates that the compressed audio stream has gone away. Given that, I'd like to throw in here another detail: iirc, i915's HDMI exports the ELD to userspace via a control called "ELD" - see eld_bytes_ctl in sound/pci/hda/patch_hdmi.c. This allows userspace to monitor, and read the ELD including which compressed audio formats are supported. I'm not currently aware of anything that makes use of this, but as there is this precedent for exporting this information, maybe it should become a standard way, so that video playback applications can then select an appropriate audio stream depending on the current properties of the connected device?
On Fri, 08 Jan 2016 11:46:23 +0100, Russell King - ARM Linux wrote: > > Given that, I'd like to throw in here another detail: iirc, i915's > HDMI exports the ELD to userspace via a control called "ELD" - see > eld_bytes_ctl in sound/pci/hda/patch_hdmi.c. This allows userspace > to monitor, and read the ELD including which compressed audio formats > are supported. I'm not currently aware of anything that makes use of > this, but as there is this precedent for exporting this information, > maybe it should become a standard way, so that video playback > applications can then select an appropriate audio stream depending on > the current properties of the connected device? Right, the ELD ctl elements were introduced exactly for such a purpose. Though, I'm not sure currently which user-space app is actually referring to this information, too. But certainly it's a good thing to have for other drivers. Takashi
On Fri, Jan 08, 2016 at 10:46:23AM +0000, Russell King - ARM Linux wrote: > Given that, I'd like to throw in here another detail: iirc, i915's > HDMI exports the ELD to userspace via a control called "ELD" - see > eld_bytes_ctl in sound/pci/hda/patch_hdmi.c. This allows userspace > to monitor, and read the ELD including which compressed audio formats > are supported. I'm not currently aware of anything that makes use of > this, but as there is this precedent for exporting this information, > maybe it should become a standard way, so that video playback > applications can then select an appropriate audio stream depending on > the current properties of the connected device? That makes sense to me.
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 15fe70f..8b6219d 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -99,6 +99,12 @@ struct hdmi_codec_pdata { int max_i2s_channels; }; +struct snd_soc_codec; +struct snd_soc_jack; + +int hdmi_codec_set_jack_detect(struct snd_soc_codec *codec, + struct snd_soc_jack *jack); + #define HDMI_CODEC_DRV_NAME "hdmi-audio-codec" #endif /* __HDMI_CODEC_H__ */ diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index 687332d..41f28ab 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -12,9 +12,12 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. */ +#include <linux/hdmi-not.h> #include <linux/module.h> +#include <linux/notifier.h> #include <linux/string.h> #include <sound/core.h> +#include <sound/jack.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -27,11 +30,15 @@ struct hdmi_codec_priv { struct hdmi_codec_pdata hcd; struct snd_soc_dai_driver *daidrv; + struct snd_soc_jack *jack; struct hdmi_codec_daifmt daifmt[2]; struct mutex current_stream_lock; struct snd_pcm_substream *current_stream; struct snd_pcm_hw_constraint_list ratec; uint8_t eld[MAX_ELD_BYTES]; + struct device *dev; + struct notifier_block nb; + unsigned int jack_status; }; static const struct snd_soc_dapm_widget hdmi_widgets[] = { @@ -326,6 +333,63 @@ static struct snd_soc_codec_driver hdmi_codec = { .num_dapm_routes = ARRAY_SIZE(hdmi_routes), }; +static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp, + unsigned int jack_status) +{ + if (!hcp->jack) + return; + + if (jack_status != hcp->jack_status) { + snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT); + hcp->jack_status = jack_status; + } +} + +static int hdmi_codec_notify(struct notifier_block *nb, unsigned long event, + void *data) +{ + struct hdmi_codec_priv *hcp = container_of(nb, struct hdmi_codec_priv, + nb); + union hdmi_event *event_block = data; + int ret; + + if (hcp->dev->parent != event_block->base.source) + return NOTIFY_OK; + + if (!hcp->jack) + return NOTIFY_OK; + + switch (event) { + case HDMI_CONNECTED: + hdmi_codec_jack_report(hcp, SND_JACK_LINEOUT); + break; + case HDMI_DISCONNECTED: + hdmi_codec_jack_report(hcp, 0); + break; + case HDMI_NEW_ELD: + if (hcp->hcd.ops->get_eld) + ret = hcp->hcd.ops->get_eld(hcp->dev->parent, hcp->eld, + sizeof(hcp->eld)); + break; + } + + return NOTIFY_OK; +} + +int hdmi_codec_set_jack_detect(struct snd_soc_codec *codec, + struct snd_soc_jack *jack) +{ + struct hdmi_codec_priv *hcp = snd_soc_codec_get_drvdata(codec); + + hcp->jack = jack; + hcp->nb.notifier_call = hdmi_codec_notify; + + hdmi_register_notifier(&hcp->nb); + + return 0; +} +EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect); + static int hdmi_codec_probe(struct platform_device *pdev) { struct hdmi_codec_pdata *hcd = pdev->dev.platform_data; @@ -370,6 +434,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) if (hcd->spdif) hcp->daidrv[i] = hdmi_spdif_dai; + dev_set_drvdata(dev, hcp); + ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv, dai_count); if (ret) { @@ -378,12 +444,16 @@ static int hdmi_codec_probe(struct platform_device *pdev) return ret; } - dev_set_drvdata(dev, hcp); + hcp->dev = dev; + return 0; } static int hdmi_codec_remove(struct platform_device *pdev) { + struct hdmi_codec_priv *hcp = platform_get_drvdata(pdev); + + hdmi_unregister_notifier(&hcp->nb); snd_soc_unregister_codec(&pdev->dev); return 0; }
Use HDMI connection / disconnection notifications to update an ALSA jack object. Also make a copy of the ELD block after every change. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- include/sound/hdmi-codec.h | 6 ++++ sound/soc/codecs/hdmi-codec.c | 72 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 1 deletion(-)