Re: [PATCH 2/4] drm/i915/hdmi: Read the HPD status before trying to read the EDID

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 12 Dec 2012, Damien Lespiau <damien.lespiau@gmail.com> wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> If you unplug the hdmi connector slowly enough, the hotplug interrupt
> fires but then the kernel code tries to read the EDID and succeeds
> (because the connector is still half connected, the HPD pin is shorter
> than the others, and DDC works). Since EDID succeeds it thinks the
> monitor is still connected.
>
> To prevent that, read the live HPD status in the hotplug handler before
> trying to read the EDID.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55372
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_hdmi.c    | 10 ++++++++--
>  3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 115bf62..117d9e6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -994,6 +994,39 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
>  	}
>  }
>  
> +/*
> + * intel_ironlake_digital_port_connected - is the specified port connected?
> + * @dev_priv: i915 private structure
> + * @port: the port to test
> + *
> + * Returns true if @port is connected, false otherwise.
> + */
> +bool intel_ironlake_digital_port_connected(struct drm_i915_private *dev_priv,
> +					   struct intel_digital_port *port)
> +{
> +	u32 bit;
> +
> +	/* XXX: IBX has different SDEISR bits */
> +	if (HAS_PCH_IBX(dev_priv->dev))
> +		return true;

They are:

#define SDE_PORTD_HOTPLUG       (1 << 10)
#define SDE_PORTC_HOTPLUG       (1 << 9)
#define SDE_PORTB_HOTPLUG       (1 << 8)

Any reason not to add those now?

> +
> +	switch(port->port) {
> +	case PORT_B:
> +		bit = SDE_PORTB_HOTPLUG_CPT;
> +		break;
> +	case PORT_C:
> +		bit = SDE_PORTC_HOTPLUG_CPT;
> +		break;
> +	case PORT_D:
> +		bit = SDE_PORTD_HOTPLUG_CPT;
> +		break;
> +	default:
> +		return true;
> +	}
> +
> +	return I915_READ(SDEISR) & bit;

The wording in the SDEISR register spec isn't exactly convincing, but
after reading it a few times along with the bit definitions, I think I
agree with you. I hope the hardware agrees with us too. :)

BR,
Jani.

> +}
> +
>  static const char *state_string(bool enabled)
>  {
>  	return enabled ? "on" : "off";
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 22728f2..0e069d8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -545,6 +545,9 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
>  	return container_of(intel_hdmi, struct intel_digital_port, hdmi);
>  }
>  
> +bool intel_ironlake_digital_port_connected(struct drm_i915_private *dev_priv,
> +					   struct intel_digital_port *port);
> +
>  extern void intel_connector_attach_encoder(struct intel_connector *connector,
>  					   struct intel_encoder *encoder);
>  extern struct drm_encoder *intel_best_encoder(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 53df0a8..966efef5 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -793,16 +793,22 @@ static bool g4x_hdmi_connected(struct intel_hdmi *intel_hdmi)
>  static enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
> +	struct drm_device *dev = connector->dev;
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  	struct intel_digital_port *intel_dig_port =
>  		hdmi_to_dig_port(intel_hdmi);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> -	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct edid *edid;
>  	enum drm_connector_status status = connector_status_disconnected;
>  
> -	if (IS_G4X(connector->dev) && !g4x_hdmi_connected(intel_hdmi))
> +
> +	if (IS_G4X(dev) && !g4x_hdmi_connected(intel_hdmi))
>  		return status;
> +	else if (HAS_PCH_SPLIT(dev) &&
> +		 !intel_ironlake_digital_port_connected(dev_priv,
> +							intel_dig_port))
> +		 return status;
>  
>  	intel_hdmi->has_hdmi_sink = false;
>  	intel_hdmi->has_audio = false;
> -- 
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx