On Tue, 02 Oct 2018, José Roberto de Souza <jose.souza@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
