| Message ID | 20170918222141.4674-4-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 18, 2017 at 3:21 PM, 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. > > 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 | 75 +++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 98e7b96ca826..cc129aa444ac 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4191,57 +4191,44 @@ 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)) { It looks like if the underlying drm_dp_dpcd_read fails and returns -EIO, for instance, you'll get true back from intel_dp_get_sink_irq_esi, and you'll still go in to the while, but with a potentially invalid esi. Granted, this is a problem in the original code as well, but it seems like something that should be fixed during the refactoring. > + 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); You're no longer using the value returned by drm_dp_mst_hpd_irq > + if (!handled) > + return 0; > + > + 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 Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote: > On Mon, Sep 18, 2017 at 3:21 PM, 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. > > > > 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 | 75 +++++++++++++++++------------------------ > > 1 file changed, 31 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 98e7b96ca826..cc129aa444ac 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4191,57 +4191,44 @@ 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)) { > > It looks like if the underlying drm_dp_dpcd_read fails and returns > -EIO, for instance, you'll get true back from > intel_dp_get_sink_irq_esi, Wait, anything other than 14 from that dpcd read is a false, isn't it? > and you'll still go in to the while, but > with a potentially invalid esi. Granted, this is a problem in the > original code as well, but it seems like something that should be > fixed during the refactoring. > > > > + 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); > > You're no longer using the value returned by drm_dp_mst_hpd_irq The way the code was originally written, the return from drm_dp_mst_hpd_irq() was a) changed to 0 when handled == false b) discarded and a new return value was obtained if handled == true and intel_dp_get_sink_irq_esi() is true the second time. So the only case when the return value was returned back to the caller is when handled == true and the second intel_dp_get_sink_irq_esi() returned false. But this does not make sense. If the second intel_dp_get_sink_irq_esi() is false, then we should still have to disable MST. This is the functional change I noted in the commit message. > > > + if (!handled) > > + return 0; > > + > > + 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 Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> wrote: > On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote: >> On Mon, Sep 18, 2017 at 3:21 PM, 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. >> > >> > 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 | 75 +++++++++++++++++------------------------ >> > 1 file changed, 31 insertions(+), 44 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index 98e7b96ca826..cc129aa444ac 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -4191,57 +4191,44 @@ 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)) { >> >> It looks like if the underlying drm_dp_dpcd_read fails and returns >> -EIO, for instance, you'll get true back from >> intel_dp_get_sink_irq_esi, > > Wait, anything other than 14 from that dpcd read is a false, isn't it? D'oh! You're right - I completely glossed over the whole " == DP_DPRX_ESI_LEN" bit - sorry for the noise... > >> and you'll still go in to the while, but >> with a potentially invalid esi. Granted, this is a problem in the >> original code as well, but it seems like something that should be >> fixed during the refactoring. >> >> >> > + 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); >> >> You're no longer using the value returned by drm_dp_mst_hpd_irq > > The way the code was originally written, the return from > drm_dp_mst_hpd_irq() was > a) changed to 0 when handled == false > b) discarded and a new return value was obtained if handled == true and > intel_dp_get_sink_irq_esi() is true the second time. > > > So the only case when the return value was returned back to the caller > is when handled == true and the second intel_dp_get_sink_irq_esi() > returned false. > > But this does not make sense. If the second intel_dp_get_sink_irq_esi() > is false, then we should still have to disable MST. This is the > functional change I noted in the commit message. > Certainly, but you aren't actually using ret for anything anymore, so the variable can be dropped > >> >> > + if (!handled) >> > + return 0; >> > + >> > + 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 Wed, 2017-09-20 at 13:02 -0700, Ausmus, James wrote: > On Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > > On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote: > >> On Mon, Sep 18, 2017 at 3:21 PM, 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. > >> > > >> > 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 | 75 +++++++++++++++++------------------------ > >> > 1 file changed, 31 insertions(+), 44 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> > index 98e7b96ca826..cc129aa444ac 100644 > >> > --- a/drivers/gpu/drm/i915/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/intel_dp.c > >> > @@ -4191,57 +4191,44 @@ 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)) { > >> > >> It looks like if the underlying drm_dp_dpcd_read fails and returns > >> -EIO, for instance, you'll get true back from > >> intel_dp_get_sink_irq_esi, > > > > Wait, anything other than 14 from that dpcd read is a false, isn't it? > > D'oh! You're right - I completely glossed over the whole " == > DP_DPRX_ESI_LEN" bit - sorry for the noise... > > > > >> and you'll still go in to the while, but > >> with a potentially invalid esi. Granted, this is a problem in the > >> original code as well, but it seems like something that should be > >> fixed during the refactoring. > >> > >> > >> > + 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); > >> > >> You're no longer using the value returned by drm_dp_mst_hpd_irq > > > > The way the code was originally written, the return from > > drm_dp_mst_hpd_irq() was > > a) changed to 0 when handled == false > > b) discarded and a new return value was obtained if handled == true and > > intel_dp_get_sink_irq_esi() is true the second time. > > > > > > So the only case when the return value was returned back to the caller > > is when handled == true and the second intel_dp_get_sink_irq_esi() > > returned false. > > > > But this does not make sense. If the second intel_dp_get_sink_irq_esi() > > is false, then we should still have to disable MST. This is the > > functional change I noted in the commit message. > > > > Certainly, but you aren't actually using ret for anything anymore, so > the variable can be dropped > > > > > >> > >> > + if (!handled) > >> > + return 0; > >> > + if (ret) DRM_DEBUG_KMS("MST irq_hpd handling failed %d\n", ret); I was thinking of adding something like this, but then the drm helper functions _handle_down_rep() and _handle_up_req() only ever return 0. I also don't want to get rid of 'ret' because that may make it seem like the underlying helpers don't return anything. So, we should either change the helpers to return something useful or modify their signatures. Both options need a bit more thought. For now, I guess we could add just the debug message. Let me know what you think. -DK > >> > + 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 Wed, Sep 20, 2017 at 3:47 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> wrote: > On Wed, 2017-09-20 at 13:02 -0700, Ausmus, James wrote: >> On Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran >> <dhinakaran.pandiyan@intel.com> wrote: >> > On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote: >> >> On Mon, Sep 18, 2017 at 3:21 PM, 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. >> >> > >> >> > 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 | 75 +++++++++++++++++------------------------ >> >> > 1 file changed, 31 insertions(+), 44 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> >> > index 98e7b96ca826..cc129aa444ac 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> >> > @@ -4191,57 +4191,44 @@ 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)) { >> >> >> >> It looks like if the underlying drm_dp_dpcd_read fails and returns >> >> -EIO, for instance, you'll get true back from >> >> intel_dp_get_sink_irq_esi, >> > >> > Wait, anything other than 14 from that dpcd read is a false, isn't it? >> >> D'oh! You're right - I completely glossed over the whole " == >> DP_DPRX_ESI_LEN" bit - sorry for the noise... >> >> > >> >> and you'll still go in to the while, but >> >> with a potentially invalid esi. Granted, this is a problem in the >> >> original code as well, but it seems like something that should be >> >> fixed during the refactoring. >> >> >> >> >> >> > + 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); >> >> >> >> You're no longer using the value returned by drm_dp_mst_hpd_irq >> > >> > The way the code was originally written, the return from >> > drm_dp_mst_hpd_irq() was >> > a) changed to 0 when handled == false >> > b) discarded and a new return value was obtained if handled == true and >> > intel_dp_get_sink_irq_esi() is true the second time. >> > >> > >> > So the only case when the return value was returned back to the caller >> > is when handled == true and the second intel_dp_get_sink_irq_esi() >> > returned false. >> > >> > But this does not make sense. If the second intel_dp_get_sink_irq_esi() >> > is false, then we should still have to disable MST. This is the >> > functional change I noted in the commit message. >> > >> >> Certainly, but you aren't actually using ret for anything anymore, so >> the variable can be dropped >> >> >> > >> >> >> >> > + if (!handled) >> >> > + return 0; >> >> > + > if (ret) > DRM_DEBUG_KMS("MST irq_hpd handling failed %d\n", ret); > > > I was thinking of adding something like this, but then the drm helper > functions _handle_down_rep() and _handle_up_req() only ever return 0. I > also don't want to get rid of 'ret' because that may make it seem like > the underlying helpers don't return anything. So, we should either > change the helpers to return something useful or modify their > signatures. Both options need a bit more thought. > > For now, I guess we could add just the debug message. Let me know what > you think. A debug message is better than just ignoring the value - I think that's a good option until the underlying helpers start returning anything useful... > > -DK > >> >> > + 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 >> >> > >> >> >> >> >> >> >> >> >>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 98e7b96ca826..cc129aa444ac 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4191,57 +4191,44 @@ 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; + + 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. 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 | 75 +++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 44 deletions(-)