diff mbox

drm/bridge: dw-hdmi: Set sink_is_hdmi and sink_has_audio in mode_set

Message ID 20161031163736.GL1041@n2100.armlinux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King (Oracle) Oct. 31, 2016, 4:37 p.m. UTC 
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(-)

Comments

James Le Cuirot Nov. 2, 2016, 9:47 p.m. UTC | #1 
On Mon, 31 Oct 2016 16:37:36 +0000
Russell King - ARM Linux <linux@armlinux.org.uk> 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,
> 
>
Russell King (Oracle) Nov. 2, 2016, 10:02 p.m. UTC | #2 
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 <linux@armlinux.org.uk> 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,
> > 
> >
diff mbox

Patch

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,