Re: [PATCH 02/10] drm/i915: Make intel_dp_detect() more clear to read

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

On Tue, 02 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> Moving all the error handling to the end of the function and ealier
> returning for connected MST ports make this function way more easy to
> read.
> No functional changed is intended here.

Honestly, I disagree that the end result would be more clear to
read. I'm all for improving clarity, but there does have to be a clear
improvement to justify changing this.

I'm especially suspicious of duplicating the intel_display_power_put()
calls and the returns. It sets a precedent to duplicate them again, and
someone's bound to forget to put.

BR,
Jani,



>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 55 +++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 15a981ef5966..5a84a929bc7d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5077,28 +5077,20 @@ intel_dp_detect(struct drm_connector *connector,
>  
>  	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
>  
> -	/* Can't disconnect eDP */
> +	/* Is port connected? eDP can't be disconnected */
> +	if (!intel_dp_is_edp(intel_dp) &&
> +	    !intel_digital_port_connected(encoder)) {
> +		status = connector_status_disconnected;
> +		goto port_disconnected;
> +	}
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
> -	else if (intel_digital_port_connected(encoder))
> -		status = intel_dp_detect_dpcd(intel_dp);
>  	else
> -		status = connector_status_disconnected;
> -
> -	if (status == connector_status_disconnected) {
> -		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> -
> -		if (intel_dp->is_mst) {
> -			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> -				      intel_dp->is_mst,
> -				      intel_dp->mst_mgr.mst_state);
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -							intel_dp->is_mst);
> -		}
> +		status = intel_dp_detect_dpcd(intel_dp);
>  
> -		goto out;
> -	}
> +	if (status == connector_status_disconnected)
> +		goto port_not_detected;
>  
>  	if (intel_dp->reset_link_params) {
>  		/* Initial max link lane count */
> @@ -5123,8 +5115,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * won't appear connected or have anything
>  		 * with EDID on it
>  		 */
> -		status = connector_status_disconnected;
> -		goto out;
> +		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		return connector_status_disconnected;
>  	}
>  
>  	/*
> @@ -5151,14 +5143,29 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp->aux.i2c_defer_count = 0;
>  
>  	intel_dp_set_edid(intel_dp);
> -	if (intel_dp_is_edp(intel_dp) ||
> -	    to_intel_connector(connector)->detect_edid)
> +	/*
> +	 * intel_dp_detect_dpcd() will return connector_status_unknown for some
> +	 * type of DP devices, if edid can be read set status as connected
> +	 */
> +	if (to_intel_connector(connector)->detect_edid)
>  		status = connector_status_connected;
>  
>  	intel_dp_check_service_irq(intel_dp);
>  
> -out:
> -	if (status != connector_status_connected && !intel_dp->is_mst)
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	return status;
> +
> +port_not_detected:
> +port_disconnected:
> +	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> +
> +	if (intel_dp->is_mst) {
> +		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> +			      intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> +		intel_dp->is_mst = false;
> +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +						intel_dp->is_mst);
> +	} else
>  		intel_dp_unset_edid(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx