Re: [PATCH] i915: Enable SDVO hotplug interrupts for HDMI and DVI

[Simon Farnsworth <simon.farnsworth@onelan.co.uk>]

Hello Keith,

I've not seen any replies to this patch in the last month, nor can I find it in 
your tree at http://cgit.freedesktop.org/~keithp/linux/ (I'd check kernel.org 
as well, but it's still down).

Is there anything I need to do to make this patch acceptable? Or is it just a 
case of waiting for you to catch up on post-XDC madness?

-- 
Simon

On Wednesday 17 August 2011, Simon Farnsworth <simon.farnsworth@onelan.co.uk> 
wrote:
> I was seeing a nasty 5 frame glitch every 10 seconds, caused by the
> poll for connection on DVI attached by SDVO.
> 
> As my SDVO DVI supports hotplug detect interrupts, the fix is to
> enable them, and hook them in to the various bits of driver
> infrastructure so that they work reliably.
> 
> With lots of help from Adam Jackson <ajax@redhat.com> on IRC.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> ---
> I'm not entirely confident that I'm treating the result of
> SDVO_CMD_GET_HOT_PLUG_SUPPORT correctly for multifunction SDVO
> devices. I've assumed it's a bitmask, but I have no documents to tell
> me that that's right. Review from someone who understands SDVO would
> be appreciated.
> 
> Note that I've only wired up hotplug fully for SDVO TMDS
> outputs. While we will get hotplug interrupts for other SDVO types, we
> won't do anything with them; if someone with another SDVO type wants
> to wire it up and test it, look at how intel_sdvo_dvi_init sets
> connector->polled.
> 
> However, this does work well for my hardware, which is all
> single-function SDVO DVI-D ports, both on Atom N270 and 965GME.
> 
>  drivers/gpu/drm/i915/intel_drv.h  |    3 --
>  drivers/gpu/drm/i915/intel_sdvo.c |   43
> ++++++++++++++++++------------------- 2 files changed, 21 insertions(+),
> 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h index 7b330e7..94bb596 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -336,9 +336,6 @@ extern void intel_release_load_detect_pipe(struct
> intel_encoder *intel_encoder, struct drm_connector *connector,
>  					   struct intel_load_detect_pipe *old);
> 
> -extern struct drm_connector* intel_sdvo_find(struct drm_device *dev, int
> sdvoB); -extern int intel_sdvo_supports_hotplug(struct drm_connector
> *connector); -extern void intel_sdvo_set_hotplug(struct drm_connector
> *connector, int enable); extern void intelfb_restore(void);
>  extern void intel_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16
> green, u16 blue, int regno);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> b/drivers/gpu/drm/i915/intel_sdvo.c index 30fe554..9404388 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1235,47 +1235,38 @@ struct drm_connector* intel_sdvo_find(struct
> drm_device *dev, int sdvoB)
> 
>  	return NULL;
>  }
> +#endif
> 
> -int intel_sdvo_supports_hotplug(struct drm_connector *connector)
> +static int intel_sdvo_supports_hotplug(struct intel_sdvo *intel_sdvo)
>  {
>  	u8 response[2];
> -	u8 status;
> -	struct intel_sdvo *intel_sdvo;
> -	DRM_DEBUG_KMS("\n");
> -
> -	if (!connector)
> -		return 0;
> -
> -	intel_sdvo = to_intel_sdvo(connector);
> 
>  	return intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HOT_PLUG_SUPPORT,
>  				    &response, 2) && response[0];
>  }
> 
> -void intel_sdvo_set_hotplug(struct drm_connector *connector, int on)
> +static void intel_sdvo_set_hotplug(struct intel_sdvo *intel_sdvo)
>  {
>  	u8 response[2];
>  	u8 status;
> -	struct intel_sdvo *intel_sdvo = to_intel_sdvo(connector);
> 
>  	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_ACTIVE_HOT_PLUG, NULL, 0);
>  	intel_sdvo_read_response(intel_sdvo, &response, 2);
> 
> -	if (on) {
> -		intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HOT_PLUG_SUPPORT, 
NULL,
> 0); -		status = intel_sdvo_read_response(intel_sdvo, &response, 2);
> +	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HOT_PLUG_SUPPORT, NULL, 0);
> +	status = intel_sdvo_read_response(intel_sdvo, &response, 2);
> 
> -		intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG,
> &response, 2); -	} else {
> -		response[0] = 0;
> -		response[1] = 0;
> -		intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG,
> &response, 2); -	}
> +	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG, &response,
> 2);
> 
>  	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_ACTIVE_HOT_PLUG, NULL, 0);
>  	intel_sdvo_read_response(intel_sdvo, &response, 2);
>  }
> -#endif
> +
> +void intel_sdvo_do_hotplug(struct intel_encoder *encoder)
> +{
> +	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
> +	intel_sdvo_set_hotplug(intel_sdvo);
> +}
> 
>  static bool
>  intel_sdvo_multifunc_encoder(struct intel_sdvo *intel_sdvo)
> @@ -2062,7 +2053,10 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo,
> int device)
> 
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> DRM_CONNECTOR_POLL_DISCONNECT; +	if
> (intel_sdvo_supports_hotplug(intel_sdvo) & (1 << device))
> +		connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	else
> +		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> DRM_CONNECTOR_POLL_DISCONNECT; encoder->encoder_type =
> DRM_MODE_ENCODER_TMDS;
>  	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
> 
> @@ -2587,6 +2581,11 @@ bool intel_sdvo_init(struct drm_device *dev, int
> sdvo_reg) &intel_sdvo->pixel_clock_max))
>  		goto err;
> 
> +        if (intel_sdvo_supports_hotplug(intel_sdvo)) {
> +		intel_encoder->hot_plug = intel_sdvo_do_hotplug;
> +		intel_sdvo_set_hotplug(intel_sdvo);
> +	}
> +
>  	DRM_DEBUG_KMS("%s device VID/DID: %02X:%02X.%02X, "
>  			"clock range %dMHz - %dMHz, "
>  			"input 1: %c, input 2: %c, "
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/