| Message ID | 20170912235730.3492-5-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 12 Sep 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > There is just only one caller now, which already checks for > intel_dp->is_mst. So, remove this and fix some braces while at it. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++--------------------- > 1 file changed, 42 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b3d95cb7546b..3f2ca10ccbcd 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4192,57 +4192,56 @@ static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > bool bret; > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > + int ret = 0; > + int retry; > + bool handled; > > - if (intel_dp->is_mst) { > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > - int ret = 0; > - int retry; > - bool handled; > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > + bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > go_again: > - if (bret == true) { > - > - /* check link status - esi[10] = 0x200c */ Drive-by observation unrelated to this patch: 0x200c isn't even defined in drm_dp_helper.h... care to add that, please? BR, Jani. > - 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); > - } > + 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("got esi %3ph\n", esi); > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > > - 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; > + if (handled) { > + for (retry = 0; retry < 3; retry++) { > + int wret; > > - return ret; > + wret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_SINK_COUNT_ESI+1, > + &esi[1], 3); > + if (wret == 3) > + break; > + } > + > + 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 { > - 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 = 0; > } > + } 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); > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > + ret = -EINVAL; > } > - return -EINVAL; > + return ret; > } > > static void
On Tue, Sep 12, 2017 at 04:57:26PM -0700, Dhinakaran Pandiyan wrote: > There is just only one caller now, which already checks for > intel_dp->is_mst. So, remove this and fix some braces while at it. Hmm. So this depends on the subtle detail that drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst prior to calling it. That fact isn't obvious at all, so I would suggest noting that in the commit message at least (maybe we should even have a comment in the code?) I'm assuming there are not other changes beyond just dropping the top level 'if (mst) { }' thing? This is one reason I prefer flat code. If the function had been flat to begin with the diff would have actually turned out legible. IIRC I even posted a flattening patch at some point but I guess it didn't go in. I would suggest that you take the opportunity to flatten out the rest of this function while you in the neighbourhood. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++--------------------- > 1 file changed, 42 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b3d95cb7546b..3f2ca10ccbcd 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4192,57 +4192,56 @@ static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > bool bret; > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > + int ret = 0; > + int retry; > + bool handled; > > - if (intel_dp->is_mst) { > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > - int ret = 0; > - int retry; > - bool handled; > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > + 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); > - } > + 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("got esi %3ph\n", esi); > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > > - 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; > + if (handled) { > + for (retry = 0; retry < 3; retry++) { > + int wret; > > - return ret; > + wret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_SINK_COUNT_ESI+1, > + &esi[1], 3); > + if (wret == 3) > + break; > + } > + > + 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 { > - 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 = 0; > } > + } 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); > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > + ret = -EINVAL; > } > - return -EINVAL; > + return ret; > } > > static void > -- > 2.11.0
On Wed, 2017-09-13 at 12:32 +0300, Jani Nikula wrote: > On Tue, 12 Sep 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > There is just only one caller now, which already checks for > > intel_dp->is_mst. So, remove this and fix some braces while at it. > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++--------------------- > > 1 file changed, 42 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b3d95cb7546b..3f2ca10ccbcd 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4192,57 +4192,56 @@ static int > > intel_dp_check_mst_status(struct intel_dp *intel_dp) > > { > > bool bret; > > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > > + int ret = 0; > > + int retry; > > + bool handled; > > > > - if (intel_dp->is_mst) { > > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > > - int ret = 0; > > - int retry; > > - bool handled; > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > + bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > go_again: > > - if (bret == true) { > > - > > - /* check link status - esi[10] = 0x200c */ > > Drive-by observation unrelated to this patch: 0x200c isn't even defined > in drm_dp_helper.h... care to add that, please? Sure. > > BR, > Jani. > > > - 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); > > - } > > + 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("got esi %3ph\n", esi); > > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > > > > - 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; > > + if (handled) { > > + for (retry = 0; retry < 3; retry++) { > > + int wret; > > > > - return ret; > > + wret = drm_dp_dpcd_write(&intel_dp->aux, > > + DP_SINK_COUNT_ESI+1, > > + &esi[1], 3); > > + if (wret == 3) > > + break; > > + } > > + > > + 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 { > > - 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 = 0; > > } > > + } 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); > > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > > + ret = -EINVAL; > > } > > - return -EINVAL; > > + return ret; > > } > > > > static void >
On Wed, 2017-09-13 at 16:31 +0300, Ville Syrjälä wrote: > On Tue, Sep 12, 2017 at 04:57:26PM -0700, Dhinakaran Pandiyan wrote: > > There is just only one caller now, which already checks for > > intel_dp->is_mst. So, remove this and fix some braces while at it. > > Hmm. So this depends on the subtle detail that > drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst > prior to calling it. That fact isn't obvious at all, so I would > suggest noting that in the commit message at least (maybe we should > even have a comment in the code?) > > I'm assuming there are not other changes beyond just dropping the > top level 'if (mst) { }' thing? This is one reason I prefer flat code. > If the function had been flat to begin with the diff would have > actually turned out legible. IIRC I even posted a flattening patch at > some point but I guess it didn't go in. I would suggest that you take > the opportunity to flatten out the rest of this function while you > in the neighbourhood. That was the goal :) Then I decided to simplify the function a little bit in patches 4, 5, 6 and 7 before working on modifying it's structure. > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++--------------------- > > 1 file changed, 42 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b3d95cb7546b..3f2ca10ccbcd 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4192,57 +4192,56 @@ static int > > intel_dp_check_mst_status(struct intel_dp *intel_dp) > > { > > bool bret; > > + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > > + int ret = 0; > > + int retry; > > + bool handled; > > > > - if (intel_dp->is_mst) { > > - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; > > - int ret = 0; > > - int retry; > > - bool handled; > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > + 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); > > - } > > + 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("got esi %3ph\n", esi); > > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > > > > - 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; > > + if (handled) { > > + for (retry = 0; retry < 3; retry++) { > > + int wret; > > > > - return ret; > > + wret = drm_dp_dpcd_write(&intel_dp->aux, > > + DP_SINK_COUNT_ESI+1, > > + &esi[1], 3); > > + if (wret == 3) > > + break; > > + } > > + > > + 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 { > > - 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 = 0; > > } > > + } 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); > > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); > > + ret = -EINVAL; > > } > > - return -EINVAL; > > + return ret; > > } > > > > static void > > -- > > 2.11.0 >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b3d95cb7546b..3f2ca10ccbcd 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4192,57 +4192,56 @@ static int intel_dp_check_mst_status(struct intel_dp *intel_dp) { bool bret; + u8 esi[DP_DPRX_ESI_LEN] = { 0 }; + int ret = 0; + int retry; + bool handled; - if (intel_dp->is_mst) { - u8 esi[DP_DPRX_ESI_LEN] = { 0 }; - int ret = 0; - int retry; - bool handled; - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); + 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); - } + 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("got esi %3ph\n", esi); + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); - 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; + if (handled) { + for (retry = 0; retry < 3; retry++) { + int wret; - return ret; + wret = drm_dp_dpcd_write(&intel_dp->aux, + DP_SINK_COUNT_ESI+1, + &esi[1], 3); + if (wret == 3) + break; + } + + 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 { - 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 = 0; } + } 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); + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev); + ret = -EINVAL; } - return -EINVAL; + return ret; } static void
There is just only one caller now, which already checks for intel_dp->is_mst. So, remove this and fix some braces while at it. Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 43 deletions(-)