Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> From: Shashank Sharma <contactshashanksharma@gmail.com>
> 
> This patch makes sure that the HDMI detect function
> reads EDID only when its forced to do it. All the other
> times, it uses the connector->detect_edid which was cached
> during hotplug handling in the hdmi_probe() function. As the
> probe function gets called before detect in the interrupt handler
> and handles the EDID cacheing part, its absolutely safe to assume
> that presence of EDID reflects monitor connected and viceversa.
> 
> This will save us from many race conditions between hotplug/unplug
> detect call handler threads and userspace calls for the same.
> The previous patch in this patch series explains this in detail.
> 
> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 064ddd8..1fb6919 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1362,19 +1362,33 @@ static enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
>  	enum drm_connector_status status;
> +	struct intel_connector *intel_connector =
> +				to_intel_connector(connector);
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
> +	/*
> +	 * There are many userspace calls which probe EDID from
> +	 * detect path. In case on multiple hotplug/unplug, these
> +	 * can cause race conditions while probing EDID. Also its
> +	 * waste of CPU cycles to read the EDID again and again
> +	 * unless there is a real hotplug.
> +	 * So until we are forced, check connector status
> +	 * based on availability of cached EDID. This will avoid many of
> +	 * these race conditions and timing problems.
> +	 */
> +	if (force)

Userspace always sets force. Are you sure this actually improves anything?
Also the goal should be to keep things cache for a few calls from
userspace (since often it pokes a few times in a row unfortuantely), for
which we need a proper timeout to clear the edid again.
-Daniel

> +		intel_hdmi_probe(intel_connector->encoder);
>  
> -	intel_hdmi_unset_edid(connector);
> -
> -	if (intel_hdmi_set_edid(connector)) {
> +	if (intel_connector->detect_edid) {
>  		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> -
> -		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
>  		status = connector_status_connected;
> -	} else
> +		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> +		DRM_DEBUG_DRIVER("hdmi status = connected\n");
> +	} else {
>  		status = connector_status_disconnected;
> +		DRM_DEBUG_DRIVER("hdmi status = disconnected\n");
> +	}
>  
>  	return status;
>  }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx