Re: [PATCH 3/4] drm/i915: Find fallback link rate/lane count

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 06 Dec 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 and limits the max
> link_rate and lane_count values to these fallback values.
>
> v6:
> * Cap the max link rate and lane count to the max
> values obtained during fallback link training (Daniel Vetter)
> v5:
> * Start the fallback at the lane count value passed not
> the max lane count (Jani Nikula)
> 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 |  2 ++
>  2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 434dc7d..b5c7526f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -278,6 +278,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;

Probably somewhere in the history of the patch series there was a time
when it was necessary to search for the rates in reverse order. What
possible benefit could that offer at this point?

> +	}
> +
> +	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] = {};

No need to initialize because you initialize it a couple of lines later.

> +	int common_len;
> +	int link_rate_index = -1;

No need to initialize because you initialize it a couple of lines later.

> +
> +	common_len = intel_dp_common_rates(intel_dp, common_rates);
> +	link_rate_index = intel_dp_link_rate_index(intel_dp,
> +						   common_rates,
> +						   link_rate);

Please stop and think, and don't rush each new iteration of the patches.

What's wrong with the above lines? Please think about it. Answer at the
end of the mail (*).

> +	if (link_rate_index > 0) {
> +		intel_dp->max_sink_link_bw = drm_dp_link_rate_to_bw_code(common_rates[link_rate_index - 1]);
> +		intel_dp->max_sink_lane_count = lane_count;
> +	} else if (lane_count > 1) {
> +		intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp);
> +		intel_dp->max_sink_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 b6526ad..47e3671 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1400,6 +1400,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);


(*) You do intel_dp_common_rates(intel_dp, common_rates) twice, for no
reason at all.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx