| Message ID | 1479454189-28345-2-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 18 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > If link training fails, then we need to fallback to lower > link rate first and if link training fails at RBR, then > fallback to lower lane count. > This function finds the next lower link rate/lane count > value after link training failure. > > v4: > * Remove the redundant variable link_train_failed > v3: > * Remove fallback_link_rate_index variable, just obtain > that using the helper intel_dp_link_rate_index (Jani Nikula) > v2: > Squash the patch that returns the link rate index (Jani Nikula) > > Acked-by: Tony Cheng <tony.cheng@amd.com> > Acked-by: Harry Wentland <harry.wentland@amd.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > 2 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 90283ed..4fb89e1 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -288,6 +288,46 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > common_rates); > } > > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > + int *common_rates, int link_rate) > +{ > + int common_len; > + int index; > + > + common_len = intel_dp_common_rates(intel_dp, common_rates); > + for (index = 0; index < common_len; index++) { > + if (link_rate == common_rates[common_len - index - 1]) > + return common_len - index - 1; > + } > + > + return -1; > +} > + > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > + int link_rate, uint8_t lane_count) > +{ > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > + int common_len; > + int link_rate_index = -1; > + > + common_len = intel_dp_common_rates(intel_dp, common_rates); > + link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + link_rate); > + if (link_rate_index > 0) { > + intel_dp->fallback_link_rate = common_rates[link_rate_index - 1]; > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); So you first try lower and lower link rates, until you're at the lowest... > + } else if (lane_count > 1) { > + intel_dp->fallback_link_rate = common_rates[common_len - 1]; > + intel_dp->fallback_lane_count = lane_count >> 1; ...and then go to highest rate, double lane count, and go back back to reducing link rate. Rinse and repeat. Problem is, lane_count will always be > 1, and you'll keep doubling lane count without bounds if link training persistently fails, and I don't think you'll reach the below else branch. I regret that I haven't caught all of these issues all at once; it is in part testament to the fact that the state machine here is not easy to follow. BR, Jani. > + } else { > + DRM_ERROR("Link Training Unsuccessful\n"); > + return -1; > + } > + > + return 0; > +} > + > static enum drm_mode_status > intel_dp_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index cd132c2..e1c43a9 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -887,6 +887,8 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int fallback_link_rate; > + uint8_t fallback_lane_count; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1383,6 +1385,8 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > void intel_dp_set_link_params(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count, > bool link_mst); > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > + int link_rate, uint8_t lane_count); > void intel_dp_start_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
On Fri, Nov 18, 2016 at 03:22:49PM +0200, Jani Nikula wrote: > On Fri, 18 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > If link training fails, then we need to fallback to lower > > link rate first and if link training fails at RBR, then > > fallback to lower lane count. > > This function finds the next lower link rate/lane count > > value after link training failure. > > > > v4: > > * Remove the redundant variable link_train_failed > > v3: > > * Remove fallback_link_rate_index variable, just obtain > > that using the helper intel_dp_link_rate_index (Jani Nikula) > > v2: > > Squash the patch that returns the link rate index (Jani Nikula) > > > > Acked-by: Tony Cheng <tony.cheng@amd.com> > > Acked-by: Harry Wentland <harry.wentland@amd.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 90283ed..4fb89e1 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -288,6 +288,46 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > > common_rates); > > } > > > > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > + int *common_rates, int link_rate) > > +{ > > + int common_len; > > + int index; > > + > > + common_len = intel_dp_common_rates(intel_dp, common_rates); > > + for (index = 0; index < common_len; index++) { > > + if (link_rate == common_rates[common_len - index - 1]) > > + return common_len - index - 1; > > + } > > + > > + return -1; > > +} > > + > > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > + int link_rate, uint8_t lane_count) > > +{ > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > + int common_len; > > + int link_rate_index = -1; > > + > > + common_len = intel_dp_common_rates(intel_dp, common_rates); > > + link_rate_index = intel_dp_link_rate_index(intel_dp, > > + common_rates, > > + link_rate); > > + if (link_rate_index > 0) { > > + intel_dp->fallback_link_rate = common_rates[link_rate_index - 1]; > > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); > > So you first try lower and lower link rates, until you're at the > lowest... > Yes so first it tries to lower the link rate until it goes to the lowes i.e RBR. All this link rate reduction happens with lane count set to maximum. > > + } else if (lane_count > 1) { > > + intel_dp->fallback_link_rate = common_rates[common_len - 1]; > > + intel_dp->fallback_lane_count = lane_count >> 1; > > ...and then go to highest rate, double lane count, and go back back to > reducing link rate. Rinse and repeat. > So after link rate reaches RBR, it checks if lane count is > 1 and if it is then it jumps the link rate back to highest supported and reduces the link rate from 4 to 2 to 1. > Problem is, lane_count will always be > 1, and you'll keep doubling lane > count without bounds if link training persistently fails, and I don't > think you'll reach the below else branch. > So since I am reducing the lane count in each iteration, lane_count >> 1, at some point it will go to 0 and that point it will exit with DRM_ERROR that link train failed because at thatpoint we have exhausted trying all the link raterate and lane count combinations. I am never doubling the link rate for it to reach out of bounds, infact I am reducing the link rate to half each time (right shifting lane count) so at some point lane count will fall to less than 0 when it will fall into the last else part. Regards Manasi > I regret that I haven't caught all of these issues all at once; it is in > part testament to the fact that the state machine here is not easy to > follow. > > BR, > Jani. > > > + } else { > > + DRM_ERROR("Link Training Unsuccessful\n"); > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > static enum drm_mode_status > > intel_dp_mode_valid(struct drm_connector *connector, > > struct drm_display_mode *mode) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index cd132c2..e1c43a9 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -887,6 +887,8 @@ struct intel_dp { > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > + int fallback_link_rate; > > + uint8_t fallback_lane_count; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1383,6 +1385,8 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count, > > bool link_mst); > > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > + int link_rate, uint8_t lane_count); > > void intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, Nov 18, 2016 at 07:39:50AM -0800, Manasi Navare wrote: > On Fri, Nov 18, 2016 at 03:22:49PM +0200, Jani Nikula wrote: > > On Fri, 18 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > > If link training fails, then we need to fallback to lower > > > link rate first and if link training fails at RBR, then > > > fallback to lower lane count. > > > This function finds the next lower link rate/lane count > > > value after link training failure. > > > > > > v4: > > > * Remove the redundant variable link_train_failed > > > v3: > > > * Remove fallback_link_rate_index variable, just obtain > > > that using the helper intel_dp_link_rate_index (Jani Nikula) > > > v2: > > > Squash the patch that returns the link rate index (Jani Nikula) > > > > > > Acked-by: Tony Cheng <tony.cheng@amd.com> > > > Acked-by: Harry Wentland <harry.wentland@amd.com> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 90283ed..4fb89e1 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -288,6 +288,46 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > > > common_rates); > > > } > > > > > > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > > + int *common_rates, int link_rate) > > > +{ > > > + int common_len; > > > + int index; > > > + > > > + common_len = intel_dp_common_rates(intel_dp, common_rates); > > > + for (index = 0; index < common_len; index++) { > > > + if (link_rate == common_rates[common_len - index - 1]) > > > + return common_len - index - 1; > > > + } > > > + > > > + return -1; > > > +} > > > + > > > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > + int link_rate, uint8_t lane_count) > > > +{ > > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > > + int common_len; > > > + int link_rate_index = -1; > > > + > > > + common_len = intel_dp_common_rates(intel_dp, common_rates); > > > + link_rate_index = intel_dp_link_rate_index(intel_dp, > > > + common_rates, > > > + link_rate); > > > + if (link_rate_index > 0) { > > > + intel_dp->fallback_link_rate = common_rates[link_rate_index - 1]; > > > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); > > > > So you first try lower and lower link rates, until you're at the > > lowest... > > > > Yes so first it tries to lower the link rate until it goes to the lowes i.e RBR. > All this link rate reduction happens with lane count set to maximum. > > > > + } else if (lane_count > 1) { > > > + intel_dp->fallback_link_rate = common_rates[common_len - 1]; > > > + intel_dp->fallback_lane_count = lane_count >> 1; > > > > ...and then go to highest rate, double lane count, and go back back to > > reducing link rate. Rinse and repeat. > > > > So after link rate reaches RBR, it checks if lane count is > 1 and if it is then > it jumps the link rate back to highest supported and reduces the link rate from > 4 to 2 to 1. > > > Problem is, lane_count will always be > 1, and you'll keep doubling lane > > count without bounds if link training persistently fails, and I don't > > think you'll reach the below else branch. > > > > So since I am reducing the lane count in each iteration, lane_count >> 1, at some point > it will go to 0 and that point it will exit with DRM_ERROR that link train failed > because at thatpoint we have exhausted trying all the link raterate and lane count > combinations. > I am never doubling the link rate for it to reach out of bounds, infact I am reducing > the link rate to half each time (right shifting lane count) so at some point lane count will > fall to less than 0 when it will fall into the last else part. > > Regards > Manasi > > The only problem i found is that I am setting lane count to max lane count which is wrong in each iteration it should be set to lane count being passed in and it should start from that value and fallback. So for Ex: If the first modeset is trying to train link at 2.7 and 4 lanes and link training fails, Iteration 1 : 1.62 and 4 if this fails Iteration 2: max link rate 5.4 and 2 (reduced lane count is halved) Iteration 3: 2.7 and 2 Iteration 4: 1.62 and 2 Iteration 5: 5.4 and 1 iteration 6: 2.7 and 1 Iteration 7: 1.62 and 1 If this fails then it will return an error that Link training is failed. Manasi > > I regret that I haven't caught all of these issues all at once; it is in > > part testament to the fact that the state machine here is not easy to > > follow. > > > > BR, > > Jani. > > > > > > + } else { > > > + DRM_ERROR("Link Training Unsuccessful\n"); > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static enum drm_mode_status > > > intel_dp_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index cd132c2..e1c43a9 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -887,6 +887,8 @@ struct intel_dp { > > > uint32_t DP; > > > int link_rate; > > > uint8_t lane_count; > > > + int fallback_link_rate; > > > + uint8_t fallback_lane_count; > > > uint8_t sink_count; > > > bool link_mst; > > > bool has_audio; > > > @@ -1383,6 +1385,8 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > > int link_rate, uint8_t lane_count, > > > bool link_mst); > > > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > + int link_rate, uint8_t lane_count); > > > void intel_dp_start_link_train(struct intel_dp *intel_dp); > > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > > > -- > > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 90283ed..4fb89e1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -288,6 +288,46 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, common_rates); } +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, + int *common_rates, int link_rate) +{ + int common_len; + int index; + + common_len = intel_dp_common_rates(intel_dp, common_rates); + for (index = 0; index < common_len; index++) { + if (link_rate == common_rates[common_len - index - 1]) + return common_len - index - 1; + } + + return -1; +} + +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, + int link_rate, uint8_t lane_count) +{ + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; + int common_len; + int link_rate_index = -1; + + common_len = intel_dp_common_rates(intel_dp, common_rates); + link_rate_index = intel_dp_link_rate_index(intel_dp, + common_rates, + link_rate); + if (link_rate_index > 0) { + intel_dp->fallback_link_rate = common_rates[link_rate_index - 1]; + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); + } else if (lane_count > 1) { + intel_dp->fallback_link_rate = common_rates[common_len - 1]; + intel_dp->fallback_lane_count = lane_count >> 1; + } else { + DRM_ERROR("Link Training Unsuccessful\n"); + return -1; + } + + return 0; +} + static enum drm_mode_status intel_dp_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index cd132c2..e1c43a9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -887,6 +887,8 @@ struct intel_dp { uint32_t DP; int link_rate; uint8_t lane_count; + int fallback_link_rate; + uint8_t fallback_lane_count; uint8_t sink_count; bool link_mst; bool has_audio; @@ -1383,6 +1385,8 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, void intel_dp_set_link_params(struct intel_dp *intel_dp, int link_rate, uint8_t lane_count, bool link_mst); +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, + int link_rate, uint8_t lane_count); void intel_dp_start_link_train(struct intel_dp *intel_dp); void intel_dp_stop_link_train(struct intel_dp *intel_dp); void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);