On Thu, Nov 17, 2016 at 02:58:46PM +0200, Jani Nikula wrote:
> On Tue, 15 Nov 2016, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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@xxxxxxx>
> > Acked-by: Harry Wentland <harry.wentland@xxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
