| Message ID | 1609256210-19863-1-git-send-email-stefan.wahren@i2se.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/vc4: hdmi: Avoid ASoC error messages on startup | expand |
Hi Maxime, Am 29.12.20 um 16:36 schrieb Stefan Wahren: > During startup of Raspberry Pi 4 there seems to be a race between > VC4 probing and Pulseaudio trying to open its PCM device: > > ASoC: error at snd_soc_dai_startup on fef05700.hdmi: -19 > > Avoid these errors by returning EPROBE_DEFER in this situation. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> should i resend without RFC? Best regards Stefan
Hi Stefan, On Sat, Jan 09, 2021 at 12:41:57PM +0100, Stefan Wahren wrote: > Hi Maxime, > > Am 29.12.20 um 16:36 schrieb Stefan Wahren: > > During startup of Raspberry Pi 4 there seems to be a race between > > VC4 probing and Pulseaudio trying to open its PCM device: > > > > ASoC: error at snd_soc_dai_startup on fef05700.hdmi: -19 > > > > Avoid these errors by returning EPROBE_DEFER in this situation. > > > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > should i resend without RFC? Yeah, it looks reasonable enough to me :) I'd like to get Mark's opinion before merging though Maxime
On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote:
> I'd like to get Mark's opinion before merging though
I'm not sure what the question is here? I get CCed on a bunch of not
obviously relevant DRM patches so there's a good chance I've just
deleted some relevnat discussion.
On Tue, 2020-12-29 at 16:36 +0100, Stefan Wahren wrote: > During startup of Raspberry Pi 4 there seems to be a race between > VC4 probing and Pulseaudio trying to open its PCM device: > > ASoC: error at snd_soc_dai_startup on fef05700.hdmi: -19 > > Avoid these errors by returning EPROBE_DEFER in this situation. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- Seems reasonable to me: Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Regards, Nicolas
Hi, On Wed, Jan 13, 2021 at 11:42:23AM +0000, Mark Brown wrote: > On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote: > > > I'd like to get Mark's opinion before merging though > > I'm not sure what the question is here? I get CCed on a bunch of not > obviously relevant DRM patches so there's a good chance I've just > deleted some relevnat discussion. The patch is question is here: https://lore.kernel.org/dri-devel/1609256210-19863-1-git-send-email-stefan.wahren@i2se.com/ In particular, I'm not so sure whether it's fine to return -EPROBE_DEFER in the startup hook. Thanks! Maxime
On Fri, Jan 15, 2021 at 07:14:37PM +0100, Maxime Ripard wrote: > On Wed, Jan 13, 2021 at 11:42:23AM +0000, Mark Brown wrote: > > On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote: > > > I'd like to get Mark's opinion before merging though > > I'm not sure what the question is here? I get CCed on a bunch of not > > obviously relevant DRM patches so there's a good chance I've just > > deleted some relevnat discussion. > The patch is question is here: > https://lore.kernel.org/dri-devel/1609256210-19863-1-git-send-email-stefan.wahren@i2se.com/ > In particular, I'm not so sure whether it's fine to return -EPROBE_DEFER > in the startup hook. I wouldn't expect that to do anything useful and IIRC that error code will end up in userspace which isn't good. If the driver wants to defer probe it should defer it from the probe() routine, after the driver has been instantiated I'm not sure what the expectation would be. In general a driver should acquire all resources it needs when probing.
Hi, Am 15.01.21 um 19:39 schrieb Mark Brown: > On Fri, Jan 15, 2021 at 07:14:37PM +0100, Maxime Ripard wrote: >> On Wed, Jan 13, 2021 at 11:42:23AM +0000, Mark Brown wrote: >>> On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote: >>>> I'd like to get Mark's opinion before merging though >>> I'm not sure what the question is here? I get CCed on a bunch of not >>> obviously relevant DRM patches so there's a good chance I've just >>> deleted some relevnat discussion. >> The patch is question is here: >> https://lore.kernel.org/dri-devel/1609256210-19863-1-git-send-email-stefan.wahren@i2se.com/ >> In particular, I'm not so sure whether it's fine to return -EPROBE_DEFER >> in the startup hook. > I wouldn't expect that to do anything useful and IIRC that error code > will end up in userspace which isn't good. If the driver wants to defer > probe it should defer it from the probe() routine, after the driver has > been instantiated I'm not sure what the expectation would be. In > general a driver should acquire all resources it needs when probing. understand. Unfortunately, currently i don't have the time to investigate how we can achieve this with this drm driver. Maybe some else can take over? Regards Stefan
Hi Stefan, Maxime, On Mon, 2021-01-18 at 12:28 +0100, Stefan Wahren wrote: > Hi, > > Am 15.01.21 um 19:39 schrieb Mark Brown: > > On Fri, Jan 15, 2021 at 07:14:37PM +0100, Maxime Ripard wrote: > > > On Wed, Jan 13, 2021 at 11:42:23AM +0000, Mark Brown wrote: > > > > On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote: > > > > > I'd like to get Mark's opinion before merging though > > > > I'm not sure what the question is here? I get CCed on a bunch of not > > > > obviously relevant DRM patches so there's a good chance I've just > > > > deleted some relevnat discussion. > > > The patch is question is here: > > > https://lore.kernel.org/dri-devel/1609256210-19863-1-git-send-email-stefan.wahren@i2se.com/ > > > In particular, I'm not so sure whether it's fine to return -EPROBE_DEFER > > > in the startup hook. > > I wouldn't expect that to do anything useful and IIRC that error code > > will end up in userspace which isn't good. If the driver wants to defer > > probe it should defer it from the probe() routine, after the driver has > > been instantiated I'm not sure what the expectation would be. In > > general a driver should acquire all resources it needs when probing. > > understand. Unfortunately, currently i don't have the time to > investigate how we can achieve this with this drm driver. > > Maybe some else can take over? My two cents: IIUC it's a tricky one since components don't have a way to express dependencies. Somewhat similar to what happened with the DSI bus/display race. To what extent vc4-crtc has a dependency with vc4-hdmi? Couldn't we move vc4-hdmi component's registration at the end of vc4_crtc_bind()? Regards, Nicolas
On Mon, Jan 18, 2021 at 01:10:40PM +0100, Nicolas Saenz Julienne wrote: > Hi Stefan, Maxime, > > On Mon, 2021-01-18 at 12:28 +0100, Stefan Wahren wrote: > > Hi, > > > > Am 15.01.21 um 19:39 schrieb Mark Brown: > > > On Fri, Jan 15, 2021 at 07:14:37PM +0100, Maxime Ripard wrote: > > > > On Wed, Jan 13, 2021 at 11:42:23AM +0000, Mark Brown wrote: > > > > > On Wed, Jan 13, 2021 at 10:19:57AM +0100, Maxime Ripard wrote: > > > > > > I'd like to get Mark's opinion before merging though > > > > > I'm not sure what the question is here? I get CCed on a bunch of not > > > > > obviously relevant DRM patches so there's a good chance I've just > > > > > deleted some relevnat discussion. > > > > The patch is question is here: > > > > https://lore.kernel.org/dri-devel/1609256210-19863-1-git-send-email-stefan.wahren@i2se.com/ > > > > In particular, I'm not so sure whether it's fine to return -EPROBE_DEFER > > > > in the startup hook. > > > I wouldn't expect that to do anything useful and IIRC that error code > > > will end up in userspace which isn't good. If the driver wants to defer > > > probe it should defer it from the probe() routine, after the driver has > > > been instantiated I'm not sure what the expectation would be. In > > > general a driver should acquire all resources it needs when probing. > > > > understand. Unfortunately, currently i don't have the time to > > investigate how we can achieve this with this drm driver. > > > > Maybe some else can take over? > > My two cents: IIUC it's a tricky one since components don't have a way to > express dependencies. There's a way though :) Components are bound in the order they've been added to the component list, so as long as the component we have a dependency on is added first, it's fine. The issue here is that the HDMI devices are added first: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_drv.c#L334 We could just move the HDMI driver after the CRTC one and we would solve it. I'm not sure if it has any other side effects though. > Somewhat similar to what happened with the DSI bus/display race. DSI is a bit different, especially on vc4 since the panel driver doesn't sit on the DSI bus in DT and as such isn't added by the bus code when the controller is added. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 5551062..e0f9357 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -909,12 +909,14 @@ static int vc4_hdmi_audio_startup(struct snd_pcm_substream *substream, vc4_hdmi->audio.substream = substream; + if (!encoder->crtc) + return -EPROBE_DEFER; + /* - * If the HDMI encoder hasn't probed, or the encoder is - * currently in DVI mode, treat the codec dai as missing. + * If the HDMI encoder is currently in DVI mode, + * treat the codec dai as missing. */ - if (!encoder->crtc || !(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & - VC4_HDMI_RAM_PACKET_ENABLE)) + if (!(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & VC4_HDMI_RAM_PACKET_ENABLE)) return -ENODEV; ret = snd_pcm_hw_constraint_eld(substream->runtime, connector->eld);
During startup of Raspberry Pi 4 there seems to be a race between VC4 probing and Pulseaudio trying to open its PCM device: ASoC: error at snd_soc_dai_startup on fef05700.hdmi: -19 Avoid these errors by returning EPROBE_DEFER in this situation. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)