| Message ID | 4914bea9fc3ef3deaffa39ab691dbd9a76461e97.1551711042.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Allow for more boot-time configuration | expand |
On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > In some cases, in order to accomodate with displays with poor EDIDs, we > need to ignore that the monitor alledgedly supports audio output and > disable the audio output. *sad trombone* Trying to figure this out automatically in kernel is better than a quirk. A quirk is better than requiring the user to provide an override EDID via the firmware loader (drm.edid_firmware parameter). Requiring an override EDID is better than adding a module parameter. I'd much rather we exhausted the other options before adding module parameters to address specific issues with EDIDs. That's a rabbit hole with no end. BR, Jani. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 990b1909f9d7..c0258b011bb2 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > } > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > +static bool ignore_edid_audio = false; > +module_param(ignore_edid_audio, bool, 0644); > +MODULE_PARM_DESC(ignore_edid_audio, > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > + > /** > * drm_detect_monitor_audio - check monitor audio capability > * @edid: EDID block to scan > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > bool has_audio = false; > int start_offset, end_offset; > > + if (ignore_edid_audio) > + goto end; > + > edid_ext = drm_find_cea_extension(edid); > if (!edid_ext) > goto end;
On Mon, 2019-03-04 at 17:47 +0200, Jani Nikula wrote: > On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > In some cases, in order to accomodate with displays with poor EDIDs, we > > need to ignore that the monitor alledgedly supports audio output and > > disable the audio output. > > *sad trombone* > > Trying to figure this out automatically in kernel is better than a > quirk. > > A quirk is better than requiring the user to provide an override EDID > via the firmware loader (drm.edid_firmware parameter). > > Requiring an override EDID is better than adding a module parameter. Also, this and 3/ apply to _every_ monitor attached to the system. No. - ajax
On Mon, Mar 04, 2019 at 03:52:35PM +0100, Maxime Ripard wrote: > In some cases, in order to accomodate with displays with poor EDIDs, we > need to ignore that the monitor alledgedly supports audio output and > disable the audio output. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 990b1909f9d7..c0258b011bb2 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > } > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > +static bool ignore_edid_audio = false; > +module_param(ignore_edid_audio, bool, 0644); > +MODULE_PARM_DESC(ignore_edid_audio, > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > + I would suggest that this is not the best apporach. Years of experience from i915 says that more modparams means random forums full of people trading cargo culted settings. And as soon as the average user comes across the magic incantation that works they are likely to not file the appropriate bug report. Also years later we still see people using modparams that stopped working five hardware generations ago. So at least for i915 new modparams are generally frowned upon. Bad EDIDs at least should be quirked. Which means we really need the bug reports, and hence a modparam can be somewhat counter productive. For allowing the user to force the DVI vs. HDMI and audio vs. not i915 does have the "audio" connector property. Other drivers could adopt the same thing. Though I'm not sure even i915 should be exposing this for the reasons already mentioned. There is one hardware generation where it can actually be useful on i915 as the hardware is only capably of sending infoframes/audio to a single HDMI port at a time. So with this property the user can at least select which display gets to do those things. I do agree that there is an unfortnate problem with fbcon vs. initial property values. I've sometimes pondered about exposing kms properties in a generic fashion via sysfs and/or kernel cmdline somehow. IIRC devicetree/something similar has also been proposed occasionally to solve this problem. > /** > * drm_detect_monitor_audio - check monitor audio capability > * @edid: EDID block to scan > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > bool has_audio = false; > int start_offset, end_offset; > > + if (ignore_edid_audio) > + goto end; > + > edid_ext = drm_find_cea_extension(edid); > if (!edid_ext) > goto end; > -- > git-series 0.9.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maxime Ripard <maxime.ripard@bootlin.com> writes: > In some cases, in order to accomodate with displays with poor EDIDs, we > need to ignore that the monitor alledgedly supports audio output and > disable the audio output. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 990b1909f9d7..c0258b011bb2 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > } > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > +static bool ignore_edid_audio = false; > +module_param(ignore_edid_audio, bool, 0644); > +MODULE_PARM_DESC(ignore_edid_audio, > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > + > /** > * drm_detect_monitor_audio - check monitor audio capability > * @edid: EDID block to scan > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > bool has_audio = false; > int start_offset, end_offset; > > + if (ignore_edid_audio) > + goto end; > + > edid_ext = drm_find_cea_extension(edid); > if (!edid_ext) > goto end; It looks like the motivation for the original flag on Raspberry Pi was "I've got a non-audio monitor, but the system comes up trying to play audio to HDMI instead of the analog jack". Do we have some way for DRM to communicate to ALSA that this is not the right place to try to play audio by default?
On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > need to ignore that the monitor alledgedly supports audio output and > > disable the audio output. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 990b1909f9d7..c0258b011bb2 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > } > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > +static bool ignore_edid_audio = false; > > +module_param(ignore_edid_audio, bool, 0644); > > +MODULE_PARM_DESC(ignore_edid_audio, > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > + > > /** > > * drm_detect_monitor_audio - check monitor audio capability > > * @edid: EDID block to scan > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > bool has_audio = false; > > int start_offset, end_offset; > > > > + if (ignore_edid_audio) > > + goto end; > > + > > edid_ext = drm_find_cea_extension(edid); > > if (!edid_ext) > > goto end; > > It looks like the motivation for the original flag on Raspberry Pi was > "I've got a non-audio monitor, but the system comes up trying to play > audio to HDMI instead of the analog jack". Do we have some way for DRM > to communicate to ALSA that this is not the right place to try to play > audio by default? Apparently not. We have users using debug knobs in our drivers to disable display audio because ALSA defaults to that rather than other audio. Alex
On Mon, Mar 04, 2019 at 05:47:09PM +0200, Jani Nikula wrote: > On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > In some cases, in order to accomodate with displays with poor EDIDs, we > > need to ignore that the monitor alledgedly supports audio output and > > disable the audio output. > > *sad trombone* > > Trying to figure this out automatically in kernel is better than a > quirk. > > A quirk is better than requiring the user to provide an override EDID > via the firmware loader (drm.edid_firmware parameter). > > Requiring an override EDID is better than adding a module parameter. > > I'd much rather we exhausted the other options before adding module > parameters to address specific issues with EDIDs. That's a rabbit hole > with no end. We should also consider the usability of these solutions. Sure, the quirks are the ideal solution long term, but do we really expect the average user that just got its device from Amazon and connected it to its display to figure out: - That if it's display doesn't work, it's because the display is broken - That it is broken due to poor EDIDs - To find out that it's supposed to be handled in DRM through a quirk - How to make such a quirk - How to recompile the kernel on its distro of choice - That they need to send a patch later on to upstream Linux, and then wait for a year or so (depending on their distro) before it's actually working. Chances are that they would stop at 1, call the device trash and never submit any quirk, therefore making the quirk approach useless in the process. Maxime
On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > need to ignore that the monitor alledgedly supports audio output and > > > disable the audio output. > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > --- > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > index 990b1909f9d7..c0258b011bb2 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > } > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > +static bool ignore_edid_audio = false; > > > +module_param(ignore_edid_audio, bool, 0644); > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > + > > > /** > > > * drm_detect_monitor_audio - check monitor audio capability > > > * @edid: EDID block to scan > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > bool has_audio = false; > > > int start_offset, end_offset; > > > > > > + if (ignore_edid_audio) > > > + goto end; > > > + > > > edid_ext = drm_find_cea_extension(edid); > > > if (!edid_ext) > > > goto end; > > > > It looks like the motivation for the original flag on Raspberry Pi was > > "I've got a non-audio monitor, but the system comes up trying to play > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > to communicate to ALSA that this is not the right place to try to play > > audio by default? > > Apparently not. We have users using debug knobs in our drivers to > disable display audio because ALSA defaults to that rather than other > audio. I guess one way to do this would be to register the card only when an audio-capable monitor is connected instead of doing this at probe time. I'm not sure how convenient it is for userspace though. Maxime
On Tue, 05 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Mon, Mar 04, 2019 at 05:47:09PM +0200, Jani Nikula wrote: >> On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: >> > In some cases, in order to accomodate with displays with poor EDIDs, we >> > need to ignore that the monitor alledgedly supports audio output and >> > disable the audio output. >> >> *sad trombone* >> >> Trying to figure this out automatically in kernel is better than a >> quirk. >> >> A quirk is better than requiring the user to provide an override EDID >> via the firmware loader (drm.edid_firmware parameter). >> >> Requiring an override EDID is better than adding a module parameter. >> >> I'd much rather we exhausted the other options before adding module >> parameters to address specific issues with EDIDs. That's a rabbit hole >> with no end. > > We should also consider the usability of these solutions. > > Sure, the quirks are the ideal solution long term, but do we really > expect the average user that just got its device from Amazon and > connected it to its display to figure out: > > - That if it's display doesn't work, it's because the display is > broken > - That it is broken due to poor EDIDs > - To find out that it's supposed to be handled in DRM through a quirk > - How to make such a quirk > - How to recompile the kernel on its distro of choice > - That they need to send a patch later on to upstream Linux, and then > wait for a year or so (depending on their distro) before it's > actually working. > > Chances are that they would stop at 1, call the device trash and never > submit any quirk, therefore making the quirk approach useless in the > process. It only takes one user to reach the end of the list, and have the quirk figured out and backported to stable kernels, fixing it for the rest of the average users. Adding this to drm core means *I* will also have to care about this for i915 users that find ways to abuse this for whatever reason. And they *will* find ways, because hey, someone on some forum wrote that this fixed their issue on some random other machine. We've added too many driver specific debug knobs as module parameters over the years, and a good portion of the bug reports we get about basically anything come with a combination of random module parameters, some of which have ceased to exist years ago. People just copy-paste them from forums and wikis. Looking at [1], I'm sure it didn't start out that massive either. It was probably a knob or two, as a quick fix for the problem at hand, and then it was easy to add more. What looks like the easy route now really isn't. BR, Jani. [1] https://www.raspberrypi.org/documentation/configuration/config-txt/video.md
On Tue, 2019-03-05 at 09:08 +0100, Maxime Ripard wrote: > Chances are that they would stop at 1, call the device trash and never > submit any quirk, therefore making the quirk approach useless in the > process. But the device _is_ trash. It's not like writing an accurate EDID is difficult, if you're the manufacturer. Why enable hardware vendors to be incompetent? - ajax
On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > need to ignore that the monitor alledgedly supports audio output and > > > > disable the audio output. > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > --- > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > } > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > +static bool ignore_edid_audio = false; > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > + > > > > /** > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > * @edid: EDID block to scan > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > bool has_audio = false; > > > > int start_offset, end_offset; > > > > > > > > + if (ignore_edid_audio) > > > > + goto end; > > > > + > > > > edid_ext = drm_find_cea_extension(edid); > > > > if (!edid_ext) > > > > goto end; > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > "I've got a non-audio monitor, but the system comes up trying to play > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > to communicate to ALSA that this is not the right place to try to play > > > audio by default? > > > > Apparently not. We have users using debug knobs in our drivers to > > disable display audio because ALSA defaults to that rather than other > > audio. > > I guess one way to do this would be to register the card only when an > audio-capable monitor is connected instead of doing this at probe > time. I'm not sure how convenient it is for userspace though. We already provide the ELD to alsa. I'm pretty sure pulseaudio uses that stuff somehow to figure out whether to play audio over HDMI. But since I don't use pulseaudio myself I can't be 100% sure. Cc:ing Takashi/alsa folks for confirmation.
Maxime Ripard <maxime.ripard@bootlin.com> writes: > [ Unknown signature status ] > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: >> > >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: >> > >> > > In some cases, in order to accomodate with displays with poor EDIDs, we >> > > need to ignore that the monitor alledgedly supports audio output and >> > > disable the audio output. >> > > >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> > > --- >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ >> > > 1 file changed, 8 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> > > index 990b1909f9d7..c0258b011bb2 100644 >> > > --- a/drivers/gpu/drm/drm_edid.c >> > > +++ b/drivers/gpu/drm/drm_edid.c >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) >> > > } >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); >> > > >> > > +static bool ignore_edid_audio = false; >> > > +module_param(ignore_edid_audio, bool, 0644); >> > > +MODULE_PARM_DESC(ignore_edid_audio, >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); >> > > + >> > > /** >> > > * drm_detect_monitor_audio - check monitor audio capability >> > > * @edid: EDID block to scan >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) >> > > bool has_audio = false; >> > > int start_offset, end_offset; >> > > >> > > + if (ignore_edid_audio) >> > > + goto end; >> > > + >> > > edid_ext = drm_find_cea_extension(edid); >> > > if (!edid_ext) >> > > goto end; >> > >> > It looks like the motivation for the original flag on Raspberry Pi was >> > "I've got a non-audio monitor, but the system comes up trying to play >> > audio to HDMI instead of the analog jack". Do we have some way for DRM >> > to communicate to ALSA that this is not the right place to try to play >> > audio by default? >> >> Apparently not. We have users using debug knobs in our drivers to >> disable display audio because ALSA defaults to that rather than other >> audio. > > I guess one way to do this would be to register the card only when an > audio-capable monitor is connected instead of doing this at probe > time. I'm not sure how convenient it is for userspace though. Yeah, I have no idea how this is supposed to work, but pulseaudio keeps doing reasonable things on my intel desktop so I'm wondering if we're just missing some bit of the HDMI driver communicating to ALSA about the state of the audio sink.
On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > > need to ignore that the monitor alledgedly supports audio output and > > > > > disable the audio output. > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > --- > > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > > } > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > > + > > > > > /** > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > * @edid: EDID block to scan > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > > bool has_audio = false; > > > > > int start_offset, end_offset; > > > > > > > > > > + if (ignore_edid_audio) > > > > > + goto end; > > > > > + > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > if (!edid_ext) > > > > > goto end; > > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > > "I've got a non-audio monitor, but the system comes up trying to play > > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > > to communicate to ALSA that this is not the right place to try to play > > > > audio by default? > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > disable display audio because ALSA defaults to that rather than other > > > audio. > > > > I guess one way to do this would be to register the card only when an > > audio-capable monitor is connected instead of doing this at probe > > time. I'm not sure how convenient it is for userspace though. > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > that stuff somehow to figure out whether to play audio over HDMI. > But since I don't use pulseaudio myself I can't be 100% sure. > > Cc:ing Takashi/alsa folks for confirmation. I forgot that the .pin_eld_notify() stuff is i915 specific. But I see some kind of hdmi_codec_ops thing used by some other drivers. I guess that is supposed to achieve the same thing more or less? I'm not immediately seeing any kind of drm->alsa notification hook in there though.
On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > > > need to ignore that the monitor alledgedly supports audio output and > > > > > > disable the audio output. > > > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > --- > > > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > > > } > > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > > > + > > > > > > /** > > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > > * @edid: EDID block to scan > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > > > bool has_audio = false; > > > > > > int start_offset, end_offset; > > > > > > > > > > > > + if (ignore_edid_audio) > > > > > > + goto end; > > > > > > + > > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > > if (!edid_ext) > > > > > > goto end; > > > > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > > > "I've got a non-audio monitor, but the system comes up trying to play > > > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > > > to communicate to ALSA that this is not the right place to try to play > > > > > audio by default? > > > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > > disable display audio because ALSA defaults to that rather than other > > > > audio. > > > > > > I guess one way to do this would be to register the card only when an > > > audio-capable monitor is connected instead of doing this at probe > > > time. I'm not sure how convenient it is for userspace though. > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > > that stuff somehow to figure out whether to play audio over HDMI. > > But since I don't use pulseaudio myself I can't be 100% sure. > > > > Cc:ing Takashi/alsa folks for confirmation. > > I forgot that the .pin_eld_notify() stuff is i915 specific. But > I see some kind of hdmi_codec_ops thing used by some other drivers. > I guess that is supposed to achieve the same thing more or less? > I'm not immediately seeing any kind of drm->alsa notification > hook in there though. On AMD hw, the GPU has backdoor access to some of the audio state, so when stuff happens on the GPU side, it's reflected on the audio side automatically. Alex
On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote: > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > > > > need to ignore that the monitor alledgedly supports audio output and > > > > > > > disable the audio output. > > > > > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > > > > } > > > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > > > > + > > > > > > > /** > > > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > > > * @edid: EDID block to scan > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > > > > bool has_audio = false; > > > > > > > int start_offset, end_offset; > > > > > > > > > > > > > > + if (ignore_edid_audio) > > > > > > > + goto end; > > > > > > > + > > > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > > > if (!edid_ext) > > > > > > > goto end; > > > > > > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > > > > "I've got a non-audio monitor, but the system comes up trying to play > > > > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > > > > to communicate to ALSA that this is not the right place to try to play > > > > > > audio by default? > > > > > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > > > disable display audio because ALSA defaults to that rather than other > > > > > audio. > > > > > > > > I guess one way to do this would be to register the card only when an > > > > audio-capable monitor is connected instead of doing this at probe > > > > time. I'm not sure how convenient it is for userspace though. > > > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > > > that stuff somehow to figure out whether to play audio over HDMI. > > > But since I don't use pulseaudio myself I can't be 100% sure. > > > > > > Cc:ing Takashi/alsa folks for confirmation. > > > > I forgot that the .pin_eld_notify() stuff is i915 specific. But > > I see some kind of hdmi_codec_ops thing used by some other drivers. > > I guess that is supposed to achieve the same thing more or less? > > I'm not immediately seeing any kind of drm->alsa notification > > hook in there though. > > On AMD hw, the GPU has backdoor access to some of the audio state, so > when stuff happens on the GPU side, it's reflected on the audio side > automatically. Right. i915 has a similar thing (my theory is that it's basically an industry wide hardware workaround for inflexible Windows driver architecture). But that was problematic for some power management related reasons (IIRC) so we added a software mechanism for it. Though I believe we still write the ELD into the hardware buffer as well.
Maxime Ripard <maxime.ripard@bootlin.com> writes: > [ Unknown signature status ] > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: >> > >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: >> > >> > > In some cases, in order to accomodate with displays with poor EDIDs, we >> > > need to ignore that the monitor alledgedly supports audio output and >> > > disable the audio output. >> > > >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> > > --- >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ >> > > 1 file changed, 8 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> > > index 990b1909f9d7..c0258b011bb2 100644 >> > > --- a/drivers/gpu/drm/drm_edid.c >> > > +++ b/drivers/gpu/drm/drm_edid.c >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) >> > > } >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); >> > > >> > > +static bool ignore_edid_audio = false; >> > > +module_param(ignore_edid_audio, bool, 0644); >> > > +MODULE_PARM_DESC(ignore_edid_audio, >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); >> > > + >> > > /** >> > > * drm_detect_monitor_audio - check monitor audio capability >> > > * @edid: EDID block to scan >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) >> > > bool has_audio = false; >> > > int start_offset, end_offset; >> > > >> > > + if (ignore_edid_audio) >> > > + goto end; >> > > + >> > > edid_ext = drm_find_cea_extension(edid); >> > > if (!edid_ext) >> > > goto end; >> > >> > It looks like the motivation for the original flag on Raspberry Pi was >> > "I've got a non-audio monitor, but the system comes up trying to play >> > audio to HDMI instead of the analog jack". Do we have some way for DRM >> > to communicate to ALSA that this is not the right place to try to play >> > audio by default? >> >> Apparently not. We have users using debug knobs in our drivers to >> disable display audio because ALSA defaults to that rather than other >> audio. > > I guess one way to do this would be to register the card only when an > audio-capable monitor is connected instead of doing this at probe > time. I'm not sure how convenient it is for userspace though. Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets to use that. So, open source is already doing the right thing, and the problem was that the old driver talking to the firmware wouldn't, thus the need for a flag.
On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > [ Unknown signature status ] > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > >> > > >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > > >> > > In some cases, in order to accomodate with displays with poor EDIDs, we > >> > > need to ignore that the monitor alledgedly supports audio output and > >> > > disable the audio output. > >> > > > >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > >> > > --- > >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > >> > > 1 file changed, 8 insertions(+) > >> > > > >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >> > > index 990b1909f9d7..c0258b011bb2 100644 > >> > > --- a/drivers/gpu/drm/drm_edid.c > >> > > +++ b/drivers/gpu/drm/drm_edid.c > >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > >> > > } > >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > >> > > > >> > > +static bool ignore_edid_audio = false; > >> > > +module_param(ignore_edid_audio, bool, 0644); > >> > > +MODULE_PARM_DESC(ignore_edid_audio, > >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > >> > > + > >> > > /** > >> > > * drm_detect_monitor_audio - check monitor audio capability > >> > > * @edid: EDID block to scan > >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > >> > > bool has_audio = false; > >> > > int start_offset, end_offset; > >> > > > >> > > + if (ignore_edid_audio) > >> > > + goto end; > >> > > + > >> > > edid_ext = drm_find_cea_extension(edid); > >> > > if (!edid_ext) > >> > > goto end; > >> > > >> > It looks like the motivation for the original flag on Raspberry Pi was > >> > "I've got a non-audio monitor, but the system comes up trying to play > >> > audio to HDMI instead of the analog jack". Do we have some way for DRM > >> > to communicate to ALSA that this is not the right place to try to play > >> > audio by default? > >> > >> Apparently not. We have users using debug knobs in our drivers to > >> disable display audio because ALSA defaults to that rather than other > >> audio. > > > > I guess one way to do this would be to register the card only when an > > audio-capable monitor is connected instead of doing this at probe > > time. I'm not sure how convenient it is for userspace though. > > Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets > to use that. So, open source is already doing the right thing, and the > problem was that the old driver talking to the firmware wouldn't, thus > the need for a flag. Ok, I'll drop that patch then. Thanks! Maxime
On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > [ Unknown signature status ] > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > >> > > >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > > >> > > In some cases, in order to accomodate with displays with poor EDIDs, we > >> > > need to ignore that the monitor alledgedly supports audio output and > >> > > disable the audio output. > >> > > > >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > >> > > --- > >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > >> > > 1 file changed, 8 insertions(+) > >> > > > >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >> > > index 990b1909f9d7..c0258b011bb2 100644 > >> > > --- a/drivers/gpu/drm/drm_edid.c > >> > > +++ b/drivers/gpu/drm/drm_edid.c > >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > >> > > } > >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > >> > > > >> > > +static bool ignore_edid_audio = false; > >> > > +module_param(ignore_edid_audio, bool, 0644); > >> > > +MODULE_PARM_DESC(ignore_edid_audio, > >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > >> > > + > >> > > /** > >> > > * drm_detect_monitor_audio - check monitor audio capability > >> > > * @edid: EDID block to scan > >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > >> > > bool has_audio = false; > >> > > int start_offset, end_offset; > >> > > > >> > > + if (ignore_edid_audio) > >> > > + goto end; > >> > > + > >> > > edid_ext = drm_find_cea_extension(edid); > >> > > if (!edid_ext) > >> > > goto end; > >> > > >> > It looks like the motivation for the original flag on Raspberry Pi was > >> > "I've got a non-audio monitor, but the system comes up trying to play > >> > audio to HDMI instead of the analog jack". Do we have some way for DRM > >> > to communicate to ALSA that this is not the right place to try to play > >> > audio by default? > >> > >> Apparently not. We have users using debug knobs in our drivers to > >> disable display audio because ALSA defaults to that rather than other > >> audio. > > > > I guess one way to do this would be to register the card only when an > > audio-capable monitor is connected instead of doing this at probe > > time. I'm not sure how convenient it is for userspace though. > > Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets > to use that. So, open source is already doing the right thing, and the > problem was that the old driver talking to the firmware wouldn't, thus > the need for a flag. I started to look into this, and while on my laptop, the ELD seems to be exposed as part of /proc/asound/card0/eld*, there's no such file on the RPi with a 5.0 kernel (and an HDMI monitor with audio support, obviously). Is it something that used to work at some point? Maxime
Maxime Ripard <maxime.ripard@bootlin.com> writes: > [ Unknown signature status ] > On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > [ Unknown signature status ] >> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: >> >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: >> >> > >> >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > >> >> > > In some cases, in order to accomodate with displays with poor EDIDs, we >> >> > > need to ignore that the monitor alledgedly supports audio output and >> >> > > disable the audio output. >> >> > > >> >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> >> > > --- >> >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ >> >> > > 1 file changed, 8 insertions(+) >> >> > > >> >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> >> > > index 990b1909f9d7..c0258b011bb2 100644 >> >> > > --- a/drivers/gpu/drm/drm_edid.c >> >> > > +++ b/drivers/gpu/drm/drm_edid.c >> >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) >> >> > > } >> >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); >> >> > > >> >> > > +static bool ignore_edid_audio = false; >> >> > > +module_param(ignore_edid_audio, bool, 0644); >> >> > > +MODULE_PARM_DESC(ignore_edid_audio, >> >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); >> >> > > + >> >> > > /** >> >> > > * drm_detect_monitor_audio - check monitor audio capability >> >> > > * @edid: EDID block to scan >> >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) >> >> > > bool has_audio = false; >> >> > > int start_offset, end_offset; >> >> > > >> >> > > + if (ignore_edid_audio) >> >> > > + goto end; >> >> > > + >> >> > > edid_ext = drm_find_cea_extension(edid); >> >> > > if (!edid_ext) >> >> > > goto end; >> >> > >> >> > It looks like the motivation for the original flag on Raspberry Pi was >> >> > "I've got a non-audio monitor, but the system comes up trying to play >> >> > audio to HDMI instead of the analog jack". Do we have some way for DRM >> >> > to communicate to ALSA that this is not the right place to try to play >> >> > audio by default? >> >> >> >> Apparently not. We have users using debug knobs in our drivers to >> >> disable display audio because ALSA defaults to that rather than other >> >> audio. >> > >> > I guess one way to do this would be to register the card only when an >> > audio-capable monitor is connected instead of doing this at probe >> > time. I'm not sure how convenient it is for userspace though. >> >> Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets >> to use that. So, open source is already doing the right thing, and the >> problem was that the old driver talking to the firmware wouldn't, thus >> the need for a flag. > > I started to look into this, and while on my laptop, the ELD seems to > be exposed as part of /proc/asound/card0/eld*, there's no such file on > the RPi with a 5.0 kernel (and an HDMI monitor with audio support, > obviously). Is it something that used to work at some point? I don't know, personally. Sounds like it's worth investigating.
On Tue, Mar 05, 2019 at 10:11:51AM -0800, Eric Anholt wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > [ Unknown signature status ] > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > >> > > >> > Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > > >> > > In some cases, in order to accomodate with displays with poor EDIDs, we > >> > > need to ignore that the monitor alledgedly supports audio output and > >> > > disable the audio output. > >> > > > >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > >> > > --- > >> > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > >> > > 1 file changed, 8 insertions(+) > >> > > > >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >> > > index 990b1909f9d7..c0258b011bb2 100644 > >> > > --- a/drivers/gpu/drm/drm_edid.c > >> > > +++ b/drivers/gpu/drm/drm_edid.c > >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > >> > > } > >> > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > >> > > > >> > > +static bool ignore_edid_audio = false; > >> > > +module_param(ignore_edid_audio, bool, 0644); > >> > > +MODULE_PARM_DESC(ignore_edid_audio, > >> > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > >> > > + > >> > > /** > >> > > * drm_detect_monitor_audio - check monitor audio capability > >> > > * @edid: EDID block to scan > >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > >> > > bool has_audio = false; > >> > > int start_offset, end_offset; > >> > > > >> > > + if (ignore_edid_audio) > >> > > + goto end; > >> > > + > >> > > edid_ext = drm_find_cea_extension(edid); > >> > > if (!edid_ext) > >> > > goto end; > >> > > >> > It looks like the motivation for the original flag on Raspberry Pi was > >> > "I've got a non-audio monitor, but the system comes up trying to play > >> > audio to HDMI instead of the analog jack". Do we have some way for DRM > >> > to communicate to ALSA that this is not the right place to try to play > >> > audio by default? > >> > >> Apparently not. We have users using debug knobs in our drivers to > >> disable display audio because ALSA defaults to that rather than other > >> audio. > > > > I guess one way to do this would be to register the card only when an > > audio-capable monitor is connected instead of doing this at probe > > time. I'm not sure how convenient it is for userspace though. > > Yeah, I have no idea how this is supposed to work, but pulseaudio keeps > doing reasonable things on my intel desktop so I'm wondering if we're > just missing some bit of the HDMI driver communicating to ALSA about the > state of the audio sink. We transport (either through the i915/snd-hda component or hw backdoors) both the "can this sink do audio" and the current eld describing the sinks audio capability to the alsa side. Afaiui the "can this sink do audio" even reflects whether the crtc is running or not (so the audio doesn't disappear into silence if you've dpms off'ed the screen). I think it's reflected into some alsa output sense/hotplug flag, that pulseaudio should take into account by default. -Daniel
On Tue, 05 Mar 2019 20:36:38 +0100, Ville Syrjälä wrote: > > On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote: > > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > > > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > > > > > need to ignore that the monitor alledgedly supports audio output and > > > > > > > > disable the audio output. > > > > > > > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > > > > > + > > > > > > > > /** > > > > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > > > > * @edid: EDID block to scan > > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > > > > > bool has_audio = false; > > > > > > > > int start_offset, end_offset; > > > > > > > > > > > > > > > > + if (ignore_edid_audio) > > > > > > > > + goto end; > > > > > > > > + > > > > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > > > > if (!edid_ext) > > > > > > > > goto end; > > > > > > > > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > > > > > "I've got a non-audio monitor, but the system comes up trying to play > > > > > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > > > > > to communicate to ALSA that this is not the right place to try to play > > > > > > > audio by default? > > > > > > > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > > > > disable display audio because ALSA defaults to that rather than other > > > > > > audio. > > > > > > > > > > I guess one way to do this would be to register the card only when an > > > > > audio-capable monitor is connected instead of doing this at probe > > > > > time. I'm not sure how convenient it is for userspace though. > > > > > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > > > > that stuff somehow to figure out whether to play audio over HDMI. > > > > But since I don't use pulseaudio myself I can't be 100% sure. > > > > > > > > Cc:ing Takashi/alsa folks for confirmation. > > > > > > I forgot that the .pin_eld_notify() stuff is i915 specific. But > > > I see some kind of hdmi_codec_ops thing used by some other drivers. > > > I guess that is supposed to achieve the same thing more or less? > > > I'm not immediately seeing any kind of drm->alsa notification > > > hook in there though. > > > > On AMD hw, the GPU has backdoor access to some of the audio state, so > > when stuff happens on the GPU side, it's reflected on the audio side > > automatically. > > Right. i915 has a similar thing (my theory is that it's basically > an industry wide hardware workaround for inflexible Windows driver > architecture). But that was problematic for some power management > related reasons (IIRC) so we added a software mechanism for it. > Though I believe we still write the ELD into the hardware buffer > as well. I'm late to the game as I've been off in the last week, and here is just the confirmation. The direct communication from drm to ALSA via component has been implemented currently only for i915. I had some patches to enable the feature for radeon and amdgpu, but the enablement on amdgpu DC is still missing, and the work is pending for now. For other drivers, we'd need more or less similar mechanism. We might want to choose a better infrastructure than the component binding, but it's a thing to be discussed. thanks, Takashi
Hi, On Wed, Mar 13, 2019 at 11:44:17AM +0100, Takashi Iwai wrote: > On Tue, 05 Mar 2019 20:36:38 +0100, > Ville Syrjälä wrote: > > > > On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote: > > > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > > > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote: > > > > > > > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > > > > > > > > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we > > > > > > > > > need to ignore that the monitor alledgedly supports audio output and > > > > > > > > > disable the audio output. > > > > > > > > > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/drm_edid.c | 8 ++++++++ > > > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > > > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > > > > > > > > > } > > > > > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > > > > > + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); > > > > > > > > > + > > > > > > > > > /** > > > > > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > > > > > * @edid: EDID block to scan > > > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > > > > > > > > > bool has_audio = false; > > > > > > > > > int start_offset, end_offset; > > > > > > > > > > > > > > > > > > + if (ignore_edid_audio) > > > > > > > > > + goto end; > > > > > > > > > + > > > > > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > > > > > if (!edid_ext) > > > > > > > > > goto end; > > > > > > > > > > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was > > > > > > > > "I've got a non-audio monitor, but the system comes up trying to play > > > > > > > > audio to HDMI instead of the analog jack". Do we have some way for DRM > > > > > > > > to communicate to ALSA that this is not the right place to try to play > > > > > > > > audio by default? > > > > > > > > > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > > > > > disable display audio because ALSA defaults to that rather than other > > > > > > > audio. > > > > > > > > > > > > I guess one way to do this would be to register the card only when an > > > > > > audio-capable monitor is connected instead of doing this at probe > > > > > > time. I'm not sure how convenient it is for userspace though. > > > > > > > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > > > > > that stuff somehow to figure out whether to play audio over HDMI. > > > > > But since I don't use pulseaudio myself I can't be 100% sure. > > > > > > > > > > Cc:ing Takashi/alsa folks for confirmation. > > > > > > > > I forgot that the .pin_eld_notify() stuff is i915 specific. But > > > > I see some kind of hdmi_codec_ops thing used by some other drivers. > > > > I guess that is supposed to achieve the same thing more or less? > > > > I'm not immediately seeing any kind of drm->alsa notification > > > > hook in there though. > > > > > > On AMD hw, the GPU has backdoor access to some of the audio state, so > > > when stuff happens on the GPU side, it's reflected on the audio side > > > automatically. > > > > Right. i915 has a similar thing (my theory is that it's basically > > an industry wide hardware workaround for inflexible Windows driver > > architecture). But that was problematic for some power management > > related reasons (IIRC) so we added a software mechanism for it. > > Though I believe we still write the ELD into the hardware buffer > > as well. > > I'm late to the game as I've been off in the last week, and here is > just the confirmation. > > The direct communication from drm to ALSA via component has been > implemented currently only for i915. I had some patches to enable the > feature for radeon and amdgpu, but the enablement on amdgpu DC is > still missing, and the work is pending for now. > > For other drivers, we'd need more or less similar mechanism. > We might want to choose a better infrastructure than the component > binding, but it's a thing to be discussed. I guess I would be interested in working on this. I've looked at the ELD situation on vc4, and even though the control is exposed for the vc4 driver, and that content seems to be ok, pulseaudio doesn't pick it up. My understanding is that pulseaudio waits on an ALSA event before looking at the ELD control, and only does so for the device that are considered HDMI, but I'm not quite sure what policy it has for that yet... Maxime
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 990b1909f9d7..c0258b011bb2 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid) } EXPORT_SYMBOL(drm_detect_hdmi_monitor); +static bool ignore_edid_audio = false; +module_param(ignore_edid_audio, bool, 0644); +MODULE_PARM_DESC(ignore_edid_audio, + "Ignore the EDID and always consider that a monitor doesn't have audio capabilities"); + /** * drm_detect_monitor_audio - check monitor audio capability * @edid: EDID block to scan @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid) bool has_audio = false; int start_offset, end_offset; + if (ignore_edid_audio) + goto end; + edid_ext = drm_find_cea_extension(edid); if (!edid_ext) goto end;
In some cases, in order to accomodate with displays with poor EDIDs, we need to ignore that the monitor alledgedly supports audio output and disable the audio output. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/drm_edid.c | 8 ++++++++ 1 file changed, 8 insertions(+)