Date: Wed, 2 Nov 2016 21:47:50 +0000
From: James Le Cuirot <chewi@gentoo.org>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/bridge: dw-hdmi: Set sink_is_hdmi and
 sink_has_audio in mode_set
Message-ID: <20161102214750.76d1e228@symphony.aura-online.co.uk>
In-Reply-To: <20161031163736.GL1041@n2100.armlinux.org.uk>
References: <20161030135625.11117-1-chewi@gentoo.org>
        <20161031163736.GL1041@n2100.armlinux.org.uk>
In-Reply-To: <20161030135625.11117-1-chewi@gentoo.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4779
Lines: 120

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,
> 
>