Hi Jani, Actually this patch is no longer valid, I have included a differnt interface change with the link training patch series for calculating the max_sink_link_rate and max_sink_lane_count in the long pulse handler and not recompute it everytime when the caller needs common_rates and lane_count. These max values will be used in the computation of common_rates when caller requests it and same for lane count. Could you please review the latest patch: https://patchwork.freedesktop.org/patch/125744/ Also this simplifies the fallback logic on link train failure since I can update the max sink link rate and max sink lane count to match the fallback values thus lowering the max capabilities advertised by the sink. Manasi On Wed, Dec 07, 2016 at 11:25:29PM +0200, Jani Nikula wrote: > On Fri, 02 Dec 2016, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote: > > Supported link rate common to source and sink as well as the > > maximum supported lane count based on source and sink capabilities should > > be set only once on hotplug and then used anytime they are requested. > > This patch creates and array of common rates and max lane count as the > > intel_dp member. It gets calculated only once in the long pulse handler > > and then gets used when requested in compute_config and other functions. > > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++++---------------------- > > drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > > 2 files changed, 21 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 9dfbde4..de37807 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -274,8 +274,7 @@ static int intersect_rates(const int *source_rates, int source_len, > > return k; > > } > > > > -static int intel_dp_common_rates(struct intel_dp *intel_dp, > > - int *common_rates) > > +static int intel_dp_common_rates(struct intel_dp *intel_dp) > > This here is a bad interface change. After this, you can only use the > function to update intel_dp->common_rates. However, this doesn't finish > the job, and leaves it up to the caller to set intel_dp->common_len. The > sole remaining call site of intel_dp_common_rates() should set the alarm > bells ringing. > > Please keep it as a helper, passing in common_rates. There's follow-up > work to be done on top, but we can leave that for later. > > > { > > const int *source_rates, *sink_rates; > > int source_len, sink_len; > > @@ -285,7 +284,7 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > > > > return intersect_rates(source_rates, source_len, > > sink_rates, sink_len, > > - common_rates); > > + intel_dp->common_rates); > > } > > > > static enum drm_mode_status > > @@ -312,7 +311,7 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > > } > > > > max_link_clock = intel_dp_max_link_rate(intel_dp); > > - max_lanes = intel_dp_max_lane_count(intel_dp); > > + max_lanes = intel_dp->max_lane_count; > > > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > > mode_rate = intel_dp_link_required(target_clock, 18); > > @@ -1430,8 +1429,7 @@ static void snprintf_int_array(char *str, size_t len, > > static void intel_dp_print_rates(struct intel_dp *intel_dp) > > { > > const int *source_rates, *sink_rates; > > - int source_len, sink_len, common_len; > > - int common_rates[DP_MAX_SUPPORTED_RATES]; > > + int source_len, sink_len; > > char str[128]; /* FIXME: too big for stack? */ > > > > if ((drm_debug & DRM_UT_KMS) == 0) > > @@ -1445,8 +1443,8 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp) > > snprintf_int_array(str, sizeof(str), sink_rates, sink_len); > > DRM_DEBUG_KMS("sink rates: %s\n", str); > > > > - common_len = intel_dp_common_rates(intel_dp, common_rates); > > - snprintf_int_array(str, sizeof(str), common_rates, common_len); > > + snprintf_int_array(str, sizeof(str), intel_dp->common_rates, > > + intel_dp->common_len); > > DRM_DEBUG_KMS("common rates: %s\n", str); > > } > > > > @@ -1495,14 +1493,12 @@ static int rate_to_index(int find, const int *rates) > > int > > intel_dp_max_link_rate(struct intel_dp *intel_dp) > > { > > - int rates[DP_MAX_SUPPORTED_RATES] = {}; > > - int len; > > + int len = intel_dp->common_len; > > > > - len = intel_dp_common_rates(intel_dp, rates); > > if (WARN_ON(len <= 0)) > > return 162000; > > > > - return rates[len - 1]; > > + return intel_dp->common_rates[len - 1]; > > } > > > > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) > > @@ -1550,22 +1546,18 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > struct intel_connector *intel_connector = intel_dp->attached_connector; > > int lane_count, clock; > > int min_lane_count = 1; > > - int max_lane_count = intel_dp_max_lane_count(intel_dp); > > + int max_lane_count = intel_dp->max_lane_count; > > /* Conveniently, the link BW constants become indices with a shift...*/ > > int min_clock = 0; > > int max_clock; > > int bpp, mode_rate; > > int link_avail, link_clock; > > - int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > - int common_len; > > uint8_t link_bw, rate_select; > > > > - common_len = intel_dp_common_rates(intel_dp, common_rates); > > - > > /* No common link rates between source and sink */ > > - WARN_ON(common_len <= 0); > > + WARN_ON(intel_dp->common_len <= 0); > > This would be better right after setting intel_dp->common_len. > > > > > - max_clock = common_len - 1; > > + max_clock = intel_dp->common_len - 1; > > > > if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) > > pipe_config->has_pch_encoder = true; > > @@ -1597,7 +1589,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > > > DRM_DEBUG_KMS("DP link computation with max lane count %i " > > "max bw %d pixel clock %iKHz\n", > > - max_lane_count, common_rates[max_clock], > > + max_lane_count, intel_dp->common_rates[max_clock], > > adjusted_mode->crtc_clock); > > > > /* Walk through all bpp values. Luckily they're all nicely spaced with 2 > > @@ -1633,7 +1625,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > lane_count <= max_lane_count; > > lane_count <<= 1) { > > > > - link_clock = common_rates[clock]; > > + link_clock = intel_dp->common_rates[clock]; > > link_avail = intel_dp_max_data_rate(link_clock, > > lane_count); > > > > @@ -1663,7 +1655,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > pipe_config->lane_count = lane_count; > > > > pipe_config->pipe_bpp = bpp; > > - pipe_config->port_clock = common_rates[clock]; > > + pipe_config->port_clock = intel_dp->common_rates[clock]; > > > > intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > > &link_bw, &rate_select); > > @@ -4400,7 +4392,9 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv, > > yesno(intel_dp_source_supports_hbr2(intel_dp)), > > yesno(drm_dp_tps3_supported(intel_dp->dpcd))); > > > > + intel_dp->common_len = intel_dp_common_rates(intel_dp); > > See how this looks bad, as updating of the length and contents are > divided between caller and callee? > > > intel_dp_print_rates(intel_dp); > > + intel_dp->max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > intel_dp_read_desc(intel_dp); > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 1d126c2..7c8885e 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -906,6 +906,11 @@ struct intel_dp { > > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ > > uint8_t num_sink_rates; > > int sink_rates[DP_MAX_SUPPORTED_RATES]; > > + /* supported link rates common between source and sink */ > > + int common_rates[DP_MAX_SUPPORTED_RATES]; > > + int common_len; > > + /* Maximum supported lane count common between source and sink */ > > + uint8_t max_lane_count; > > /* sink or branch descriptor */ > > struct intel_dp_desc desc; > > struct drm_dp_aux aux; > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
