[PATCH] drm: don't loose HPD events

[Daniel Vetter]

On Tue, Jan 20, 2015 at 01:03:51PM -0500, Rob Clark wrote:
> Since drm_helper_probe_single_connector_modes_merge_bits() and other
> places also update connector->status, the output_poll_execute() and/or
> drm_helper_hpd_irq_event() logic can get confused and forget to send
> a HPD event.
> 
> There are two possible solutions: (1) keep track of state at last HPD
> check separately, or (2) send events in more places.  The latter
> approach isn't so convenient to avoid per-connector HPD events (in
> case multiple connectors change connection state at the same time) so
> I went for the first option.

Imo the 2nd option is better, since with that
a) we don't need to track the last-seen status, which might get out of
date
b) it might take an awful long time for a poll/hpd to happen and notice
the change (current code tries hard not to do superflous work)

Also it's more in line with the current logic in the poll worker and hpd
handler which both locally cache the old_status and then compare
afterwards.

Oh and my google fu is working again, I've written this ages ago:

http://lists.freedesktop.org/archives/dri-devel/2012-October/029399.html

Revised version with a 2nd patch to tackle some ping-pong issues around
status == unkown:

http://lists.freedesktop.org/archives/dri-devel/2013-June/039507.html

I'll rebase and resend quickly.
-Daniel

> 
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc.c         | 1 +
>  drivers/gpu/drm/drm_probe_helper.c | 9 ++++++---
>  include/drm/drm_crtc.h             | 9 +++++++++
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7c1786d..615edee 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -865,6 +865,7 @@ int drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->modes);
>  	connector->edid_blob_ptr = NULL;
>  	connector->status = connector_status_unknown;
> +	connector->hpd_status = connector_status_unknown;
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 2fbdcca..c227285 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -293,14 +293,16 @@ static void output_poll_execute(struct work_struct *work)
>  
>  		repoll = true;
>  
> -		old_status = connector->status;
>  		/* if we are connected and don't want to poll for disconnect
>  		   skip it */
> -		if (old_status == connector_status_connected &&
> +		if (connector->status == connector_status_connected &&
>  		    !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT))
>  			continue;
>  
> +		old_status = connector->hpd_status;
> +
>  		connector->status = connector->funcs->detect(connector, false);
> +		connector->hpd_status = connector->status;
>  		if (old_status != connector->status) {
>  			const char *old, *new;
>  
> @@ -450,9 +452,10 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
>  		if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
>  			continue;
>  
> -		old_status = connector->status;
> +		old_status = connector->hpd_status;
>  
>  		connector->status = connector->funcs->detect(connector, false);
> +		connector->hpd_status = connector->status;
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
>  			      connector->base.id,
>  			      connector->name,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f444263..cb1899b 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -619,6 +619,7 @@ struct drm_encoder {
>   * @stereo_allowed: can this connector handle stereo modes?
>   * @modes: modes available on this connector (from fill_modes() + user)
>   * @status: one of the drm_connector_status enums (connected, not, or unknown)
> + * @hpd_status: status as of last hpd/poll
>   * @probed_modes: list of modes derived directly from the display
>   * @display_info: information about attached display (e.g. from EDID)
>   * @funcs: connector control functions
> @@ -676,6 +677,14 @@ struct drm_connector {
>  
>  	enum drm_connector_status status;
>  
> +	/* since connector->status can change in various other places
> +	 * (other than hotplug poll/irq), we need to separately keep
> +	 * track of the connection status at last should-I-send-an-
> +	 * hpd-event check, to avoid forgetting to send an hpd event
> +	 * to userspace
> +	 */
> +	enum drm_connector_status hpd_status;
> +
>  	/* these are modes added by probing with DDC or the BIOS */
>  	struct list_head probed_modes;
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch