On Mon, 31 Oct 2016 16:37:36 +0000
Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> 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,
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
