| Message ID | 20181002175054.15010-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors | expand |
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);
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);
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. 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(-)