Subject: Re: [PATCH] drm/radeon: Retry DDC probing on DVI on failure if we got
 an HPD interrupt
To: <cpaul@redhat.com>, Alex Deucher <alexander.deucher@amd.com>,
        David Airlie <airlied@linux.ie>, <dri-devel@lists.freedesktop.org>,
        <linux-kernel@vger.kernel.org>
References: <1448034740-30193-1-git-send-email-cpaul@redhat.com>
CC: Jerome Glisse <jglisse@redhat.com>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
From: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <56507E0F.7030009@amd.com>
Date: Sat, 21 Nov 2015 15:22:07 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1448034740-30193-1-git-send-email-cpaul@redhat.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2015 14:22:24.0819
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR12MB0137
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 5898
Lines: 135

On 20.11.2015 16:52, cpaul@redhat.com wrote:
> From: Stephen Chandler Paul <cpaul@redhat.com>
>
> 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@redhat.com>

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.

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)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/