| Message ID | E1Yr1yD-0006lK-RC@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 09, 2015 at 11:26:57AM +0100, Russell King wrote: > Parse the ELD (EDID like data) stored from the HDMI driver to restrict > the sample rates and channels which are available to ALSA. This causes > the ALSA device to reflect the capabilities of the overall audio path, > not just what is supported at the HDMI source interface level. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> I completely missed the new eld helpers in this series, so fairly late with a few questions here. In x86/desktop gpus we pass around the eld in hw, and there's an interrupt on the snd side every time that materially changes. Same goes for other state changes like the display pipe getting lit up/disabled again. How is hotplug handling done here? You update the eld in get_modes, but I don't see any notification from hdmi->audio (it only seems to get updated at open time, no idea whether that's enough). And it looks like there's a potential use-after-free when the drm side updates the eld and frees the old one, while the snd side tries to access this. Just curious questions really, I probably don't understand what's exactly going on. But I do think that we need a more formal way for drm/snd to talk to each another (i915 is growing quite a few hairy things in that area outside of eld atm). Problems we have are around shared power wells and clocks (x86 doesn't help here without all the dt goodness). But there's also stuff like figuring out the right dividers for a given audio/video clock. eld and hotplug is still done in hw, but I've heard noises that we need to have a sw approach too on some platforms at least. Cheers, Daniel > --- > drivers/gpu/drm/bridge/Kconfig | 1 + > drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c | 6 ++++++ > drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h | 1 + > drivers/gpu/drm/bridge/dw_hdmi.c | 3 +++ > 4 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 56ed35fe0734..204861bfb867 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -7,6 +7,7 @@ config DRM_DW_HDMI_AHB_AUDIO > tristate "Synopsis Designware AHB Audio interface" > depends on DRM_DW_HDMI && SND > select SND_PCM > + select SND_PCM_ELD > select SND_PCM_IEC958 > help > Support the AHB Audio interface which is part of the Synopsis > diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c > index e98c291268f4..2bb68bda3cb0 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c > @@ -12,11 +12,13 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <drm/bridge/dw_hdmi.h> > +#include <drm/drm_edid.h> > > #include <sound/asoundef.h> > #include <sound/core.h> > #include <sound/initval.h> > #include <sound/pcm.h> > +#include <sound/pcm_drm_eld.h> > #include <sound/pcm_iec958.h> > > #include "dw_hdmi-ahb-audio.h" > @@ -284,6 +286,10 @@ static int dw_hdmi_open(struct snd_pcm_substream *substream) > > runtime->hw = dw_hdmi_hw; > > + ret = snd_pcm_hw_constraint_eld(runtime, dw->data.eld); > + if (ret < 0) > + return ret; > + > ret = snd_pcm_limit_hw_rates(runtime); > if (ret < 0) > return ret; > diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h > index 1e840118d90a..91f631beecc7 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h > +++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h > @@ -8,6 +8,7 @@ struct dw_hdmi_audio_data { > void __iomem *base; > int irq; > struct dw_hdmi *hdmi; > + u8 *eld; > }; > > #endif > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > index 1cb427935ed2..dc0aed18019d 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > @@ -1446,6 +1446,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) > > drm_mode_connector_update_edid_property(connector, edid); > ret = drm_add_edid_modes(connector, edid); > + /* Store the ELD */ > + drm_edid_to_eld(connector, edid); > kfree(edid); > } else { > dev_dbg(hdmi->dev, "failed to get edid\n"); > @@ -1725,6 +1727,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > audio.base = hdmi->regs; > audio.irq = irq; > audio.hdmi = hdmi; > + audio.eld = hdmi->connector.eld; > > pdevinfo.name = "dw-hdmi-ahb-audio"; > pdevinfo.data = &audio; > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 27, 2015 at 12:43:08PM +0200, Daniel Vetter wrote: > Just curious questions really, I probably don't understand what's exactly > going on. But I do think that we need a more formal way for drm/snd to > talk to each another (i915 is growing quite a few hairy things in that > area outside of eld atm). Problems we have are around shared power wells Yes, I agree strongly with this - I've been asking for a while for more code sharing and common code in this path, the helpers in here are really good from that point of view and I'm very pleased we've got them applied. > and clocks (x86 doesn't help here without all the dt goodness). But > there's also stuff like figuring out the right dividers for a given > audio/video clock. eld and hotplug is still done in hw, but I've heard > noises that we need to have a sw approach too on some platforms at least. That seems to be the case, yes - FWIW quite a few of the embedded systems are basically connecting a standard S/PDIF or I2S output to a HDMI encoder chip or IP block so the audio controller knows nothing of HDMI directly.
On Wed, May 27, 2015 at 12:43:08PM +0200, Daniel Vetter wrote: > On Sat, May 09, 2015 at 11:26:57AM +0100, Russell King wrote: > > Parse the ELD (EDID like data) stored from the HDMI driver to restrict > > the sample rates and channels which are available to ALSA. This causes > > the ALSA device to reflect the capabilities of the overall audio path, > > not just what is supported at the HDMI source interface level. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > I completely missed the new eld helpers in this series, so fairly late > with a few questions here. > > In x86/desktop gpus we pass around the eld in hw, and there's an interrupt > on the snd side every time that materially changes. Same goes for other > state changes like the display pipe getting lit up/disabled again. > > How is hotplug handling done here? You update the eld in get_modes, but I > don't see any notification from hdmi->audio (it only seems to get updated > at open time, no idea whether that's enough). And it looks like there's a > potential use-after-free when the drm side updates the eld and frees the > old one, while the snd side tries to access this. It doesn't handle hotplug at all, per se. The problem with hotplug is that (afaik) ALSA has no way to notify userspace that something has changed: if I were to add such a notification from the DRM side of the driver to the audio side, I wouldn't know what to do with it when the audio side received it. In any case, I tend to think of unplugging the display as being much like unplugging the 3.5mm line out jack to your hifi system - just because you've disconnected the speakers does not stop the audio playing - the audio continues despite you not being able to hear it. Sure, there's the problem that if you plug in a display with different capabilities, the existing format may not be supported, but I don't see a reasonable method which could provoke what effectively would be a restart on the ALSA side. In any case, hotplug is kind-of outside of the remit of these helpers - these helpers are more to do with decoding an existing ELD and limiting the capabilities of the audio side to that specified in the ELD. It's up to the driver how it gets the ELD, and it's left to the driver to determine how to handle the change-of-ELD notification. Given your description of i915 hardware above, I think this makes sense anyway: if you have a hardware notification system, it makes sense to use that rather than some software implementation, and requiring a software implementation for everything to me sounds too limiting. That's not to say that we couldn't have a software implementation in the future: right now, I merely concentrated on the parsing issue. The dw-hdmi audio driver isn't ready for mainlining yet - there's two variants of this at the moment (one for iMX6 and a different one for Rockchip - same IP but configured with different interfaces for getting the audio data into the HDMI block.) There's still some work to be done to properly unify the two approaches into something sane.
On Wed, May 27, 2015 at 7:31 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, May 27, 2015 at 12:43:08PM +0200, Daniel Vetter wrote: >> On Sat, May 09, 2015 at 11:26:57AM +0100, Russell King wrote: >> > Parse the ELD (EDID like data) stored from the HDMI driver to restrict >> > the sample rates and channels which are available to ALSA. This causes >> > the ALSA device to reflect the capabilities of the overall audio path, >> > not just what is supported at the HDMI source interface level. >> > >> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> >> I completely missed the new eld helpers in this series, so fairly late >> with a few questions here. >> >> In x86/desktop gpus we pass around the eld in hw, and there's an interrupt >> on the snd side every time that materially changes. Same goes for other >> state changes like the display pipe getting lit up/disabled again. >> >> How is hotplug handling done here? You update the eld in get_modes, but I >> don't see any notification from hdmi->audio (it only seems to get updated >> at open time, no idea whether that's enough). And it looks like there's a >> potential use-after-free when the drm side updates the eld and frees the >> old one, while the snd side tries to access this. > > It doesn't handle hotplug at all, per se. > > The problem with hotplug is that (afaik) ALSA has no way to notify > userspace that something has changed: if I were to add such a > notification from the DRM side of the driver to the audio side, I > wouldn't know what to do with it when the audio side received it. > > In any case, I tend to think of unplugging the display as being much > like unplugging the 3.5mm line out jack to your hifi system - just > because you've disconnected the speakers does not stop the audio > playing - the audio continues despite you not being able to hear it. Yeah, afaik that's the same that happens on intel. > Sure, there's the problem that if you plug in a display with different > capabilities, the existing format may not be supported, but I don't > see a reasonable method which could provoke what effectively would be > a restart on the ALSA side. > > In any case, hotplug is kind-of outside of the remit of these helpers - > these helpers are more to do with decoding an existing ELD and limiting > the capabilities of the audio side to that specified in the ELD. It's > up to the driver how it gets the ELD, and it's left to the driver to > determine how to handle the change-of-ELD notification. > > Given your description of i915 hardware above, I think this makes sense > anyway: if you have a hardware notification system, it makes sense to use > that rather than some software implementation, and requiring a software > implementation for everything to me sounds too limiting. > > That's not to say that we couldn't have a software implementation in the > future: right now, I merely concentrated on the parsing issue. > > The dw-hdmi audio driver isn't ready for mainlining yet - there's two > variants of this at the moment (one for iMX6 and a different one for > Rockchip - same IP but configured with different interfaces for getting > the audio data into the HDMI block.) There's still some work to be done > to properly unify the two approaches into something sane. I agree that it makes sense to start with the parsing issues. And some of the reconfiguration work we need to do in i915 like retuning derived clocks when the master display clock changes is definitely solved better already in the arm world I think ;-) I also don't think that hotplug handling is all that important, at least a quick read through the intel snd stuff suggest it doesn't handle any of the reconfiguration all that well at all. The only issue that might be there with your sw approach is that a concurrent probe/hotplug in drm might free the edid and hence the eld, while the snd side is trying to copy that. I'm not sure how that's prevented. Not really a problem with the helpers though. -Daniel
On Wed, May 27, 2015 at 11:29:52PM +0200, Daniel Vetter wrote: > The only issue that might be > there with your sw approach is that a concurrent probe/hotplug in drm > might free the edid and hence the eld, while the snd side is trying to > copy that. Talking only about the particular case of dw-hdmi-ahb-audio... The worst that may happen is that the ELD will be overwritten with an update on hotplug - it won't be freed unless the underlying connector is freed, as the eld is stored as an array inside struct drm_connector. Before the DRM connector is freed, we pull the audio driver's struct device, which will destroy the ALSA device. Hopefully ALSA already copes gracefully with the removal of an in-use device... it should do, it's no different from having a USB audio device and you pull it out of the USB socket while the device is in-use. (It's also worth noting as we're talking about this, that as the audio struct device is a child of the HDMI device, any PM activity will be correctly ordered: the child gets suspended before the parent... the audio device gets suspended before the video, and resume is of course the reverse order.)
At Wed, 27 May 2015 18:31:25 +0100, Russell King - ARM Linux wrote: > > On Wed, May 27, 2015 at 12:43:08PM +0200, Daniel Vetter wrote: > > On Sat, May 09, 2015 at 11:26:57AM +0100, Russell King wrote: > > > Parse the ELD (EDID like data) stored from the HDMI driver to restrict > > > the sample rates and channels which are available to ALSA. This causes > > > the ALSA device to reflect the capabilities of the overall audio path, > > > not just what is supported at the HDMI source interface level. > > > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > > > I completely missed the new eld helpers in this series, so fairly late > > with a few questions here. > > > > In x86/desktop gpus we pass around the eld in hw, and there's an interrupt > > on the snd side every time that materially changes. Same goes for other > > state changes like the display pipe getting lit up/disabled again. > > > > How is hotplug handling done here? You update the eld in get_modes, but I > > don't see any notification from hdmi->audio (it only seems to get updated > > at open time, no idea whether that's enough). And it looks like there's a > > potential use-after-free when the drm side updates the eld and frees the > > old one, while the snd side tries to access this. > > It doesn't handle hotplug at all, per se. > > The problem with hotplug is that (afaik) ALSA has no way to notify > userspace that something has changed: if I were to add such a > notification from the DRM side of the driver to the audio side, I > wouldn't know what to do with it when the audio side received it. The notification itself is present. For HD-audio HDMI/DP, the driver creates jack kctl for each port for notifying the monitor/ELD change. The recent PA actually listens to it and handles accordingly. But it doesn't mean that the PCM and other state changes are proactively dealt inside the driver. Rather it gives the notification, keeps running and waits for the further action from user-space. > In any case, I tend to think of unplugging the display as being much > like unplugging the 3.5mm line out jack to your hifi system - just > because you've disconnected the speakers does not stop the audio > playing - the audio continues despite you not being able to hear it. Ditto for HD-audio. The interface is still present, even playable, without the connection. Maybe we should return an error explicitly when played during unconnected? The handling of HDMI/DP hotplug has been discussed many times, and I vaguely remember of pros/cons with the above shown in past threads. Takashi
On Wed, May 27, 2015 at 10:44:40PM +0100, Russell King - ARM Linux wrote: > On Wed, May 27, 2015 at 11:29:52PM +0200, Daniel Vetter wrote: > > The only issue that might be > > there with your sw approach is that a concurrent probe/hotplug in drm > > might free the edid and hence the eld, while the snd side is trying to > > copy that. > > Talking only about the particular case of dw-hdmi-ahb-audio... > > The worst that may happen is that the ELD will be overwritten with an > update on hotplug - it won't be freed unless the underlying connector > is freed, as the eld is stored as an array inside struct drm_connector. > Before the DRM connector is freed, we pull the audio driver's struct > device, which will destroy the ALSA device. Oh right I've forgotten that eld is stored as a full array and only the edid floats around as a pointer in drm_connector. > Hopefully ALSA already copes gracefully with the removal of an in-use > device... it should do, it's no different from having a USB audio device > and you pull it out of the USB socket while the device is in-use. > > (It's also worth noting as we're talking about this, that as the audio > struct device is a child of the HDMI device, any PM activity will be > correctly ordered: the child gets suspended before the parent... the > audio device gets suspended before the video, and resume is of course > the reverse order.) Yeah, being able to control the nesting with DT seems so nice compared to intel ... -Daniel
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 56ed35fe0734..204861bfb867 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -7,6 +7,7 @@ config DRM_DW_HDMI_AHB_AUDIO tristate "Synopsis Designware AHB Audio interface" depends on DRM_DW_HDMI && SND select SND_PCM + select SND_PCM_ELD select SND_PCM_IEC958 help Support the AHB Audio interface which is part of the Synopsis diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c index e98c291268f4..2bb68bda3cb0 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c +++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c @@ -12,11 +12,13 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <drm/bridge/dw_hdmi.h> +#include <drm/drm_edid.h> #include <sound/asoundef.h> #include <sound/core.h> #include <sound/initval.h> #include <sound/pcm.h> +#include <sound/pcm_drm_eld.h> #include <sound/pcm_iec958.h> #include "dw_hdmi-ahb-audio.h" @@ -284,6 +286,10 @@ static int dw_hdmi_open(struct snd_pcm_substream *substream) runtime->hw = dw_hdmi_hw; + ret = snd_pcm_hw_constraint_eld(runtime, dw->data.eld); + if (ret < 0) + return ret; + ret = snd_pcm_limit_hw_rates(runtime); if (ret < 0) return ret; diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h index 1e840118d90a..91f631beecc7 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h +++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h @@ -8,6 +8,7 @@ struct dw_hdmi_audio_data { void __iomem *base; int irq; struct dw_hdmi *hdmi; + u8 *eld; }; #endif diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index 1cb427935ed2..dc0aed18019d 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -1446,6 +1446,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); + /* Store the ELD */ + drm_edid_to_eld(connector, edid); kfree(edid); } else { dev_dbg(hdmi->dev, "failed to get edid\n"); @@ -1725,6 +1727,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, audio.base = hdmi->regs; audio.irq = irq; audio.hdmi = hdmi; + audio.eld = hdmi->connector.eld; pdevinfo.name = "dw-hdmi-ahb-audio"; pdevinfo.data = &audio;
Parse the ELD (EDID like data) stored from the HDMI driver to restrict the sample rates and channels which are available to ALSA. This causes the ALSA device to reflect the capabilities of the overall audio path, not just what is supported at the HDMI source interface level. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c | 6 ++++++ drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h | 1 + drivers/gpu/drm/bridge/dw_hdmi.c | 3 +++ 4 files changed, 11 insertions(+)