These were previously set in dw_hdmi_connector_get_modes but this
isn't called when the EDID is overridden. This can be seen in
drm_helper_probe_single_connector_modes. Using the
drm_kms_helper.edid_firmware parameter therefore always resulted in
silence, even when providing the very same EDID that had previously
been read from /sys.
I saw that other drivers set similar properties in mode_set rather
than get_modes. radeon_audio_hdmi_mode_set is one such example. It
calls radeon_connector_edid to retrieve the EDID so I drew inspiration
from this for the fix.
I have tested this with a Utilite Pro on 4.9-rc3. I tried overriding
the EDID with my own, not overriding the EDID, hotplugging the display
after booting, and overriding the EDID with 1920x1080.bin. The latter
has no audio parameters so no sound was heard as expected.
Signed-off-by: James Le Cuirot <[email protected]>
---
Notes:
I do have some questions.
I don't know the significance of the mutex lock. I put my code inside
it because I am modifying the hdmi properties. Is this necessary?
Should it go before or after the lock instead?
I'm also wondering whether I should initially set both properties to
false in case the EDID is missing but the driver didn't do this
previously. Perhaps it should have?
I have only contributed one line to the kernel before, many years ago,
so please be gentle. :)
drivers/gpu/drm/bridge/dw-hdmi.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index ab7023e..a30b691 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1383,12 +1383,23 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *mode)
{
struct dw_hdmi *hdmi = bridge->driver_private;
+ struct drm_property_blob *edid_blob;
+ struct edid *edid;
mutex_lock(&hdmi->mutex);
/* Store the display mode for plugin/DKMS poweron events */
memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
+ edid_blob = hdmi->connector.edid_blob_ptr;
+ if (edid_blob) {
+ edid = (struct edid *) edid_blob->data;
+ if (edid) {
+ hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+ hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
+ }
+ }
+
mutex_unlock(&hdmi->mutex);
}
@@ -1445,8 +1456,6 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
edid->width_cm, edid->height_cm);
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
- hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
drm_mode_connector_update_edid_property(connector, edid);
ret = drm_add_edid_modes(connector, edid);
/* Store the ELD */
--
2.10.1
On Sun, Oct 30, 2016 at 01:56:25PM +0000, James Le Cuirot wrote:
> These were previously set in dw_hdmi_connector_get_modes but this
> isn't called when the EDID is overridden. This can be seen in
> drm_helper_probe_single_connector_modes. Using the
> drm_kms_helper.edid_firmware parameter therefore always resulted in
> silence, even when providing the very same EDID that had previously
> been read from /sys.
>
> I saw that other drivers set similar properties in mode_set rather
> than get_modes. radeon_audio_hdmi_mode_set is one such example. It
> calls radeon_connector_edid to retrieve the EDID so I drew inspiration
> from this for the fix.
>
> I have tested this with a Utilite Pro on 4.9-rc3. I tried overriding
> the EDID with my own, not overriding the EDID, hotplugging the display
> after booting, and overriding the EDID with 1920x1080.bin. The latter
> has no audio parameters so no sound was heard as expected.
I'm not sure I particularly like this approach - the issue seems to
be that drm_helper_probe_single_connector_modes() can avoid calling
->get_modes(), at which point our ideas about the EDID-based
capabilities become stale.
I think it would be better to provide our own ->fill_modes implementation
which calls drm_helper_probe_single_connector_modes(), and then parses
the resulting EDID, rather than re-parsing it each time we set a mode.
We also need to apply this to the ELD as well - and several other
drivers are similarly buggy, and are going to need similar fixes (thanks
for pointing this problem out!)
> Notes:
> I do have some questions.
>
> I don't know the significance of the mutex lock. I put my code inside
> it because I am modifying the hdmi properties. Is this necessary?
> Should it go before or after the lock instead?
It's there to ensure that ->previous_mode, ->disabled, and the power
management all operate atomically.
> I'm also wondering whether I should initially set both properties to
> false in case the EDID is missing but the driver didn't do this
> previously. Perhaps it should have?
The driver's private data is initially zero-ed, so that should be
unnecessary.
So maybe something like this instead - can you test please?
drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 77ab47341658..878568af2d41 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
mutex_unlock(&hdmi->mutex);
}
+static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
+ uint32_t maxX, uint32_t maxY)
+{
+ struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
+ connector);
+ struct edid *edid;
+ int ret;
+
+ ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+ edid = connector->edid_blob_ptr;
+ if (edid) {
+ hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+ hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
+ /* Store the ELD */
+ drm_edid_to_eld(connector, edid);
+ } else {
+ hdmi->sink_is_hdmi = false;
+ hdmi->sink_has_audio = false;
+ }
+
+ return ret;
+}
+
static enum drm_connector_status
dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
{
@@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
edid->width_cm, edid->height_cm);
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
- hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
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");
@@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
.dpms = drm_atomic_helper_connector_dpms,
- .fill_modes = drm_helper_probe_single_connector_modes,
+ .fill_modes = dw_hdmi_connector_fill_modes,
.detect = dw_hdmi_connector_detect,
.destroy = dw_hdmi_connector_destroy,
.force = dw_hdmi_connector_force,
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Mon, 31 Oct 2016 16:37:36 +0000
Russell King - ARM Linux <[email protected]> wrote:
> On Sun, Oct 30, 2016 at 01:56:25PM +0000, James Le Cuirot wrote:
> > These were previously set in dw_hdmi_connector_get_modes but this
> > isn't called when the EDID is overridden. This can be seen in
> > drm_helper_probe_single_connector_modes. Using the
> > drm_kms_helper.edid_firmware parameter therefore always resulted in
> > silence, even when providing the very same EDID that had previously
> > been read from /sys.
> >
> > I saw that other drivers set similar properties in mode_set rather
> > than get_modes. radeon_audio_hdmi_mode_set is one such example. It
> > calls radeon_connector_edid to retrieve the EDID so I drew
> > inspiration from this for the fix.
>
> I'm not sure I particularly like this approach - the issue seems to
> be that drm_helper_probe_single_connector_modes() can avoid calling
> ->get_modes(), at which point our ideas about the EDID-based
> capabilities become stale.
>
> I think it would be better to provide our own ->fill_modes
> implementation which calls drm_helper_probe_single_connector_modes(),
> and then parses the resulting EDID, rather than re-parsing it each
> time we set a mode.
I also thought it was a little odd to do it via mode_set but figured it
was like that for a reason. I can't really comment on which approach is
better so some input from others would be appreciated.
> We also need to apply this to the ELD as well - and several other
> drivers are similarly buggy, and are going to need similar fixes
> (thanks for pointing this problem out!)
Are you sure that is necessary? If you take a look at
drm_load_edid_firmware, you'll see that it already calls
drm_edid_to_eld when the drm_kms_helper.edid_firmware parameter is
given.
> > Notes:
> > I do have some questions.
> >
> > I'm also wondering whether I should initially set both
> > properties to false in case the EDID is missing but the driver
> > didn't do this previously. Perhaps it should have?
>
> The driver's private data is initially zero-ed, so that should be
> unnecessary.
Right, but what if you hotplug from a display that has a readable EDID
to one that doesn't? You did this in your patch anyway.
> So maybe something like this instead - can you test please?
I briefly tested while giving drm_kms_helper.edid_firmware and it
works, though I did have to assign edid via edid_blob->data like I did
in my own patch. edid_blob_ptr is not an edid struct.
> drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 77ab47341658..878568af2d41 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
> mutex_unlock(&hdmi->mutex);
> }
>
> +static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
> + uint32_t maxX, uint32_t maxY)
> +{
> + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> + connector);
> + struct edid *edid;
> + int ret;
> +
> + ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
> +
> + edid = connector->edid_blob_ptr;
> + if (edid) {
> + hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> + hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
> + /* Store the ELD */
> + drm_edid_to_eld(connector, edid);
> + } else {
> + hdmi->sink_is_hdmi = false;
> + hdmi->sink_has_audio = false;
> + }
> +
> + return ret;
> +}
> +
> static enum drm_connector_status
> dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> {
> @@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
> edid->width_cm, edid->height_cm);
>
> - hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> - hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
> 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");
> @@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>
> static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
> .dpms = drm_atomic_helper_connector_dpms,
> - .fill_modes = drm_helper_probe_single_connector_modes,
> + .fill_modes = dw_hdmi_connector_fill_modes,
> .detect = dw_hdmi_connector_detect,
> .destroy = dw_hdmi_connector_destroy,
> .force = dw_hdmi_connector_force,
>
>
On Wed, Nov 02, 2016 at 09:47:50PM +0000, James Le Cuirot wrote:
> On Mon, 31 Oct 2016 16:37:36 +0000
> Russell King - ARM Linux <[email protected]> wrote:
> > We also need to apply this to the ELD as well - and several other
> > drivers are similarly buggy, and are going to need similar fixes
> > (thanks for pointing this problem out!)
>
> Are you sure that is necessary? If you take a look at
> drm_load_edid_firmware, you'll see that it already calls
> drm_edid_to_eld when the drm_kms_helper.edid_firmware parameter is
> given.
Hmm, thanks for pointing that out - you are correct that we only need
to call drm_edid_to_eld() in get_modes().
I've also been digging more into the CEA861B spec, and it seems that
keying the infoframes off the result of drm_detect_hdmi_monitor() is
not entirely correct - CEA861B allows version 2 AVI infoframes to be
sent if the EDID version is 3, and audio (along with audio infoframes)
where audio is reported as supported.
I've been fixing this in TDA998x primarily at the moment, because that
works. I've been avoiding iMX6 stuff (where dw-hdmi is used) while
iMX stabilises after the merge window - the autobuilers have been
reporting boot failures on those platforms, although it looks like rc3
has solved them all now.
> I briefly tested while giving drm_kms_helper.edid_firmware and it
> works, though I did have to assign edid via edid_blob->data like I did
> in my own patch. edid_blob_ptr is not an edid struct.
Yes, found that with applying a similar approach to TDA998x, I'll fix
this dw-hdmi patch up for that once I've finished with TDA998x.
Nevertheless, your report is good news.
> > drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++-----
> > 1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> > index 77ab47341658..878568af2d41 100644
> > --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> > @@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
> > mutex_unlock(&hdmi->mutex);
> > }
> >
> > +static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
> > + uint32_t maxX, uint32_t maxY)
> > +{
> > + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> > + connector);
> > + struct edid *edid;
> > + int ret;
> > +
> > + ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
> > +
> > + edid = connector->edid_blob_ptr;
> > + if (edid) {
> > + hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> > + hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
> > + /* Store the ELD */
> > + drm_edid_to_eld(connector, edid);
> > + } else {
> > + hdmi->sink_is_hdmi = false;
> > + hdmi->sink_has_audio = false;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static enum drm_connector_status
> > dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> > {
> > @@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> > dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
> > edid->width_cm, edid->height_cm);
> >
> > - hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> > - hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
> > 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");
> > @@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
> >
> > static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
> > .dpms = drm_atomic_helper_connector_dpms,
> > - .fill_modes = drm_helper_probe_single_connector_modes,
> > + .fill_modes = dw_hdmi_connector_fill_modes,
> > .detect = dw_hdmi_connector_detect,
> > .destroy = dw_hdmi_connector_destroy,
> > .force = dw_hdmi_connector_force,
> >
> >
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.