Re: [PATCH] drm/radeon: Retry DDC probing on DVI on failure if we got an HPD interrupt

[Daniel Vetter]

On Sat, Nov 21, 2015 at 03:22:07PM +0100, Christian König wrote:
> On 20.11.2015 16:52, cpaul@xxxxxxxxxx wrote:
> >From: Stephen Chandler Paul <cpaul@xxxxxxxxxx>
> >
> >HPD signals on DVI ports can be fired off before the pins required for
> >DDC probing actually make contact, due to the pins for HPD making
> >contact first. This results in a HPD signal being asserted but DDC
> >probing failing, resulting in hotplugging occasionally failing.
> >
> >This is somewhat rare on most cards (depending on what angle you plug
> >the DVI connector in), but on some cards it happens constantly. The
> >Radeon R5 on the machine used for testing this patch for instance, runs
> >into this issue just about every time I try to hotplug a DVI monitor and
> >as a result hotplugging almost never works.
> >
> >Rescheduling the hotplug work for a second when we run into an HPD
> >signal with a failing DDC probe usually gives enough time for the rest
> >of the connector's pins to make contact, and fixes this issue.
> >
> >Signed-off-by: Stephen Chandler Paul <cpaul@xxxxxxxxxx>
> 
> Yeah, that's something I always wondered a about bit as well.
> 
> Debouncing is something very common done in electronics, but as far as I
> know the HPD pins don't necessary have an RC circuit so we might need to
> handle this case in software here.
> 
> A delay of something between 10-30ms between the last HPD interrupt and
> further processing of the signal doesn't sounds like such a bad idea.
> 
> Retrying on the other hand doesn't necessarily improve the situation cause
> the delay introduced by this might not be enough.
> 
> So I would rather vote for a fixed delay between an HPD interrupt and
> actually starting to process anything.

Might be interesting to compare what we do in i915 in latest kernels:

- in the hpd handler we check the hpd status bits before probing the edid.
  This will take care of slow unplug case where we get the hpd
  interrupt/status change, but i2c is still working.

- but because crap screens (or whatever it is) if hpd is _not_ set we
  retest the status for 30ms (and bail out as soon as hpd status is
  asserted). This seems to be enough time to settle all screens.

Note that this is in intel_hdmi.c since native dvi ports on intel hw also
always supported hdmi features. Not sure whether we could extract this
into some kind of helper and share.
-Daniel


> 
> Regards,
> Christian.
> 
> >---
> >So this one has kind of been a tough sell with Jerome, mostly because it's
> >somewhat of a hack. Unfortunately however I've managed to find machines where
> >DVI hotplugging literally doesn't work without a patch like this. We've already
> >tried a couple of ways of handling the situation of retriggering ddc probes:
> >
> >* Trying the DDC probe in the radeon_dvi_detect() function multiple times.
> >* Trying to reschedule the hotplug_work task whenever DDC probing fails on DVI
> >   but we got a hpd signal (this ended up being a much more complicated patch
> >   then anticipated)
> >* Doing what we do right now, which is just triggering userspace to rescan all
> >   the ports when the hpd signal is asserted by the DVI port but there's no DDC
> >   probe, and repeating until at least a second passes.
> >
> >All of these actually work, but I guess it's a question of which one is less of
> >a hack. If anyone here can think of a cleaner way of handling this feel free to
> >let me know.
> >
> >  drivers/gpu/drm/radeon/radeon.h            |  3 +++
> >  drivers/gpu/drm/radeon/radeon_connectors.c | 20 +++++++++++++++++---
> >  drivers/gpu/drm/radeon/radeon_irq_kms.c    |  2 ++
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> >index b6cbd81..d63f0fe 100644
> >--- a/drivers/gpu/drm/radeon/radeon.h
> >+++ b/drivers/gpu/drm/radeon/radeon.h
> >@@ -2460,6 +2460,9 @@ struct radeon_device {
> >  	/* amdkfd interface */
> >  	struct kfd_dev		*kfd;
> >+	/* last time we received an hpd signal */
> >+	unsigned long hpd_time;
> >+
> >  	struct mutex	mn_lock;
> >  	DECLARE_HASHTABLE(mn_hash, 7);
> >  };
> >diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> >index 5a2cafb..4ee9440 100644
> >--- a/drivers/gpu/drm/radeon/radeon_connectors.c
> >+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> >@@ -1228,19 +1228,33 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> >  	const struct drm_encoder_helper_funcs *encoder_funcs;
> >  	int i, r;
> >  	enum drm_connector_status ret = connector_status_disconnected;
> >-	bool dret = false, broken_edid = false;
> >+	bool dret = false, broken_edid = false, hpd_unchanged;
> >  	r = pm_runtime_get_sync(connector->dev->dev);
> >  	if (r < 0)
> >  		return connector_status_disconnected;
> >-	if (!force && radeon_check_hpd_status_unchanged(connector)) {
> >+	hpd_unchanged = radeon_check_hpd_status_unchanged(connector);
> >+	if (!force && hpd_unchanged) {
> >  		ret = connector->status;
> >  		goto exit;
> >  	}
> >-	if (radeon_connector->ddc_bus)
> >+	if (radeon_connector->ddc_bus) {
> >  		dret = radeon_ddc_probe(radeon_connector, false);
> >+
> >+		/* Sometimes the pins required for the DDC probe on DVI
> >+		 * connectors don't make contact at the same time that the ones
> >+		 * for HPD do. If the DDC probe fails even though we had an HPD
> >+		 * signal, signal userspace to try again */
> >+		if (!dret && !hpd_unchanged &&
> >+		    connector->status != connector_status_connected &&
> >+		    time_before(jiffies, rdev->hpd_time + msecs_to_jiffies(1000))) {
> >+			DRM_DEBUG_KMS("%s: hpd asserted but ddc probe failed, retrying\n",
> >+				      connector->name);
> >+			drm_sysfs_hotplug_event(dev);
> >+		}
> >+	}
> >  	if (dret) {
> >  		radeon_connector->detected_by_load = false;
> >  		radeon_connector_free_edid(connector);
> >diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> >index 171d3e4..579c22c 100644
> >--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> >+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> >@@ -79,6 +79,8 @@ static void radeon_hotplug_work_func(struct work_struct *work)
> >  	struct drm_mode_config *mode_config = &dev->mode_config;
> >  	struct drm_connector *connector;
> >+	rdev->hpd_time = jiffies;
> >+
> >  	/* we can race here at startup, some boards seem to trigger
> >  	 * hotplug irqs when they shouldn't. */
> >  	if (!rdev->mode_info.mode_config_initialized)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/