| Message ID | 20170921185406.2353-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 11:54 AM, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Rewriting this code without the goto, I believe, makes it more readable. > One functional change that has been included is the handling of failed ESI > register reads. Instead of disabling MST only for the first failed read, we > now disable MST on subsequent failed reads too. A failed ESI read is > problematic irrespective of whether it is the first or not. > > v2: Don't ignore return from _mst_hpd_irq() (James) > > Cc: James Ausmus <james.ausmus@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Reviewed-by: James Ausmus <james.ausmus@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 78 ++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 98e7b96ca826..aa97bd825369 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4191,57 +4191,47 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > - bool bret; > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > - if (intel_dp->is_mst) { > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > - int ret = 0; > - int retry; > + if (!intel_dp->is_mst) > + return -EINVAL; > + > + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) { > + int ret, retry; > bool handled; > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > -go_again: > - if (bret == true) { > - > - /* check link status - esi[10] = 0x200c */ > - if (intel_dp->active_mst_links && > - !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > - DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); > - intel_dp_start_link_train(intel_dp); > - intel_dp_stop_link_train(intel_dp); > - } > > - DRM_DEBUG_KMS("got esi %3ph\n", esi); > - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > - > - if (handled) { > - for (retry = 0; retry < 3; retry++) { > - int wret; > - wret = drm_dp_dpcd_write(&intel_dp->aux, > - DP_SINK_COUNT_ESI+1, > - &esi[1], 3); > - if (wret == 3) { > - break; > - } > - } > + DRM_DEBUG_KMS("ESI %3ph\n", esi); > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > - if (bret == true) { > - DRM_DEBUG_KMS("got esi2 %3ph\n", esi); > - goto go_again; > - } > - } else > - ret = 0; > + /* check link status - esi[10] = 0x200c */ > + if (intel_dp->active_mst_links && > + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > > - return ret; > - } else { > - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); > - intel_dp->is_mst = false; > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > - /* send a hotplug event */ > - drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > + if (!handled) > + return 0; > + > + if (ret) > + DRM_DEBUG_KMS("error handling MST IRQ_HPD %d\n", ret); > + > + for (retry = 0; retry < 3; retry++) { > + int wret; > + > + wret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_SINK_COUNT_ESI + 1, &esi[1], > + 3); > + if (wret == 3) > + break; > } > } > + > + DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); > + intel_dp->is_mst = false; > + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > return -EINVAL; > } > > -- > 2.11.0 >
On Thu, 21 Sep 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Rewriting this code without the goto, I believe, makes it more readable. > One functional change that has been included is the handling of failed ESI > register reads. Instead of disabling MST only for the first failed read, we > now disable MST on subsequent failed reads too. A failed ESI read is > problematic irrespective of whether it is the first or not. > > v2: Don't ignore return from _mst_hpd_irq() (James) > > Cc: James Ausmus <james.ausmus@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> I pushed everything *except* this patch in the series, please repost in a new thread for clean CI results. Thanks, Jani. > --- > drivers/gpu/drm/i915/intel_dp.c | 78 ++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 98e7b96ca826..aa97bd825369 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4191,57 +4191,47 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > - bool bret; > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > - if (intel_dp->is_mst) { > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > - int ret = 0; > - int retry; > + if (!intel_dp->is_mst) > + return -EINVAL; > + > + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) { > + int ret, retry; > bool handled; > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > -go_again: > - if (bret == true) { > - > - /* check link status - esi[10] = 0x200c */ > - if (intel_dp->active_mst_links && > - !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > - DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); > - intel_dp_start_link_train(intel_dp); > - intel_dp_stop_link_train(intel_dp); > - } > > - DRM_DEBUG_KMS("got esi %3ph\n", esi); > - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > - > - if (handled) { > - for (retry = 0; retry < 3; retry++) { > - int wret; > - wret = drm_dp_dpcd_write(&intel_dp->aux, > - DP_SINK_COUNT_ESI+1, > - &esi[1], 3); > - if (wret == 3) { > - break; > - } > - } > + DRM_DEBUG_KMS("ESI %3ph\n", esi); > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > - if (bret == true) { > - DRM_DEBUG_KMS("got esi2 %3ph\n", esi); > - goto go_again; > - } > - } else > - ret = 0; > + /* check link status - esi[10] = 0x200c */ > + if (intel_dp->active_mst_links && > + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > > - return ret; > - } else { > - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); > - intel_dp->is_mst = false; > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > - /* send a hotplug event */ > - drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > + if (!handled) > + return 0; > + > + if (ret) > + DRM_DEBUG_KMS("error handling MST IRQ_HPD %d\n", ret); > + > + for (retry = 0; retry < 3; retry++) { > + int wret; > + > + wret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_SINK_COUNT_ESI + 1, &esi[1], > + 3); > + if (wret == 3) > + break; > } > } > + > + DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); > + intel_dp->is_mst = false; > + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > return -EINVAL; > }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 98e7b96ca826..aa97bd825369 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4191,57 +4191,47 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) static int intel_dp_check_mst_status(struct intel_dp *intel_dp) { - bool bret; + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - if (intel_dp->is_mst) { - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; - int ret = 0; - int retry; + if (!intel_dp->is_mst) + return -EINVAL; + + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) { + int ret, retry; bool handled; - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); -go_again: - if (bret == true) { - - /* check link status - esi[10] = 0x200c */ - if (intel_dp->active_mst_links && - !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { - DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); - intel_dp_start_link_train(intel_dp); - intel_dp_stop_link_train(intel_dp); - } - DRM_DEBUG_KMS("got esi %3ph\n", esi); - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); - - if (handled) { - for (retry = 0; retry < 3; retry++) { - int wret; - wret = drm_dp_dpcd_write(&intel_dp->aux, - DP_SINK_COUNT_ESI+1, - &esi[1], 3); - if (wret == 3) { - break; - } - } + DRM_DEBUG_KMS("ESI %3ph\n", esi); - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); - if (bret == true) { - DRM_DEBUG_KMS("got esi2 %3ph\n", esi); - goto go_again; - } - } else - ret = 0; + /* check link status - esi[10] = 0x200c */ + if (intel_dp->active_mst_links && + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { + intel_dp_start_link_train(intel_dp); + intel_dp_stop_link_train(intel_dp); + } - return ret; - } else { - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); - intel_dp->is_mst = false; - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); - /* send a hotplug event */ - drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); + if (!handled) + return 0; + + if (ret) + DRM_DEBUG_KMS("error handling MST IRQ_HPD %d\n", ret); + + for (retry = 0; retry < 3; retry++) { + int wret; + + wret = drm_dp_dpcd_write(&intel_dp->aux, + DP_SINK_COUNT_ESI + 1, &esi[1], + 3); + if (wret == 3) + break; } } + + DRM_DEBUG_KMS("failed to get ESI - device may have failed\n"); + intel_dp->is_mst = false; + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); return -EINVAL; }
Rewriting this code without the goto, I believe, makes it more readable. One functional change that has been included is the handling of failed ESI register reads. Instead of disabling MST only for the first failed read, we now disable MST on subsequent failed reads too. A failed ESI read is problematic irrespective of whether it is the first or not. v2: Don't ignore return from _mst_hpd_irq() (James) Cc: James Ausmus <james.ausmus@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 78 ++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 44 deletions(-)