| Message ID | 1479179603-28476-5-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jani/Ville , could you please review this patch? Jani, you had mentioned it looks good and we were only waiting for ACKs from DRM, so now from the DRM point of view all these patches are ACKed and it looks good. But I need r-b for these two i915 specific patches to get them merged. Regards Manasi On Mon, Nov 14, 2016 at 07:13:22PM -0800, Manasi Navare 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. > > 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 | 42 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a1b0181..e87b451 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -288,6 +288,48 @@ 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_index = link_rate_index - 1; > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); > + } else if (lane_count > 1) { > + intel_dp->fallback_link_rate_index = common_len - 1; > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; > + 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 75252ec..55ceb44 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -887,6 +887,10 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int fallback_link_rate; > + uint8_t fallback_lane_count; > + int fallback_link_rate_index; > + bool link_train_failed; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1383,6 +1387,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); > -- > 1.9.1 >
On Tue, 15 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. > > 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 | 42 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a1b0181..e87b451 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -288,6 +288,48 @@ 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_index = link_rate_index - 1; > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; fallback_link_rate_index and fallback_link_rate are two ways to express the same thing, and you can derive one from the other. When you have two, you run the risk of having them out of sync. I thought I'd mentioned this in earlier review. Please do not store fallback_link_rate_index in intel_dp. Only store fallback_link_rate. You can use your intel_dp_link_rate_index() function when you need to convert rate to index. Moreover, resetting the state becomes more clear when all can be set to 0. BR, Jani. > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); > + } else if (lane_count > 1) { > + intel_dp->fallback_link_rate_index = common_len - 1; > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; > + 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 75252ec..55ceb44 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -887,6 +887,10 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int fallback_link_rate; > + uint8_t fallback_lane_count; > + int fallback_link_rate_index; > + bool link_train_failed; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1383,6 +1387,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 Thu, Nov 17, 2016 at 02:58:46PM +0200, Jani Nikula wrote: > On Tue, 15 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. > > > > 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 | 42 ++++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 6 ++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index a1b0181..e87b451 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -288,6 +288,48 @@ 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_index = link_rate_index - 1; > > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; > > fallback_link_rate_index and fallback_link_rate are two ways to express > the same thing, and you can derive one from the other. When you have > two, you run the risk of having them out of sync. I thought I'd > mentioned this in earlier review. > > Please do not store fallback_link_rate_index in intel_dp. Only store > fallback_link_rate. You can use your intel_dp_link_rate_index() function > when you need to convert rate to index. > > Moreover, resetting the state becomes more clear when all can be set to > 0. > > BR, > Jani. > Thanks Jani. From the previous feedback, I removed the redundant link_train_failed variable stored in intel_dp, but retained the link_rate_index. I will look at this and see if this can be optimized. Manasi > > + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); > > + } else if (lane_count > 1) { > > + intel_dp->fallback_link_rate_index = common_len - 1; > > + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; > > + 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 75252ec..55ceb44 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -887,6 +887,10 @@ struct intel_dp { > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > + int fallback_link_rate; > > + uint8_t fallback_lane_count; > > + int fallback_link_rate_index; > > + bool link_train_failed; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1383,6 +1387,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
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a1b0181..e87b451 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -288,6 +288,48 @@ 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_index = link_rate_index - 1; + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; + intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp); + } else if (lane_count > 1) { + intel_dp->fallback_link_rate_index = common_len - 1; + intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index]; + 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 75252ec..55ceb44 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -887,6 +887,10 @@ struct intel_dp { uint32_t DP; int link_rate; uint8_t lane_count; + int fallback_link_rate; + uint8_t fallback_lane_count; + int fallback_link_rate_index; + bool link_train_failed; uint8_t sink_count; bool link_mst; bool has_audio; @@ -1383,6 +1387,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);