Re: [PATCH] drm/bridge: dw-hdmi: Set sink_is_hdmi and sink_has_audio in mode_set

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,