[Intel-gfx] [PATCH] drm/i915: propagate errors from intel_dp_init_connector

[Daniel Vetter]

On Mon, Jun 10, 2013 at 02:19:45PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> In case we detect a "ghost eDP", intel_dp_init_connector was freeing
> both the connector and encoder and then returning. On Haswell,
> intel_ddi_init was trying to use the freed encoder on the HDMI
> initialization path since the following commit:
> 
> commit 21a8e6a4853b2ed39fa4c5188a710f2cf1b92026
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Wed Apr 10 23:28:35 2013 +0200
>     drm/i915: don't setup hdmi for port D edp in ddi_init
> 
> So on this patch we change intel_dp_init_connector from void to bool,
> check its return value on its callers and leave the resource-freeing
> task to its callers (since the callers are the ones who allocate the
> resources). With this change it will be easier to see that
> intel_dp_init_connector can fail, so I hope we won't have similar
> regressions in the future.
> 
> Also include a small change to remove extra "{" and "}" on nearby
> code.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Oops, thanks for tracking this down and fixing it. Patch is queued for
-next.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++---
>  drivers/gpu/drm/i915/intel_dp.c  | 15 +++++++++------
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 224ce25..4e0cf37 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1356,14 +1356,18 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->cloneable = false;
>  	intel_encoder->hot_plug = intel_ddi_hot_plug;
>  
> -	intel_dp_init_connector(intel_dig_port, dp_connector);
> +	/* May fail if it's an eDP connector that's disconnected. */
> +	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
> +		intel_ddi_destroy(encoder);
> +		intel_dp_destroy(&dp_connector->base);
> +		return;
> +	}
>  
>  	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
>  		hdmi_connector = kzalloc(sizeof(struct intel_connector),
>  					 GFP_KERNEL);
> -		if (!hdmi_connector) {
> +		if (!hdmi_connector)
>  			return;
> -		}
>  
>  		intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
>  		intel_hdmi_init_connector(intel_dig_port, hdmi_connector);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0de82bb..ec23b3d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2687,7 +2687,7 @@ done:
>  	return 0;
>  }
>  
> -static void
> +void
>  intel_dp_destroy(struct drm_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -2961,7 +2961,7 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		      I915_READ(pp_div_reg));
>  }
>  
> -void
> +bool
>  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  			struct intel_connector *intel_connector)
>  {
> @@ -3095,9 +3095,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		} else {
>  			/* if this fails, presume the device is a ghost */
>  			DRM_INFO("failed to retrieve link info, disabling eDP\n");
> -			intel_dp_encoder_destroy(&intel_encoder->base);
> -			intel_dp_destroy(connector);
> -			return;
> +			return false;
>  		}
>  
>  		/* We now know it's not a ghost, init power sequence regs. */
> @@ -3152,6 +3150,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>  	}
> +
> +	return true;
>  }
>  
>  void
> @@ -3197,5 +3197,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	intel_encoder->cloneable = false;
>  	intel_encoder->hot_plug = intel_dp_hot_plug;
>  
> -	intel_dp_init_connector(intel_dig_port, intel_connector);
> +	if (!intel_dp_init_connector(intel_dig_port, intel_connector)) {
> +		intel_dp_encoder_destroy(encoder);
> +		intel_dp_destroy(&intel_connector->base);
> +	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b9e250c..8a7ad99 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -569,9 +569,10 @@ extern bool intel_lvds_init(struct drm_device *dev);
>  extern bool intel_is_dual_link_lvds(struct drm_device *dev);
>  extern void intel_dp_init(struct drm_device *dev, int output_reg,
>  			  enum port port);
> -extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> +extern bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  				    struct intel_connector *intel_connector);
>  extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
> +extern void intel_dp_destroy(struct drm_connector *connector);
>  extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
>  extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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