On Mon, Mar 27, 2017 at 08:21:21PM +0300, Ville Syrjälä wrote: > On Mon, Mar 27, 2017 at 09:53:36AM -0700, Manasi Navare wrote: > > On Mon, Mar 27, 2017 at 06:26:06PM +0300, Jani Nikula wrote: > > > On Mon, 27 Mar 2017, Ville Syrjälä <[email protected]> wrote: > > > > On Thu, Mar 23, 2017 at 04:11:32PM -0700, Manasi Navare wrote: > > > >> Currently intel_dp_check_link_status() tries to retrain the link if > > > >> Clock recovery or Channel EQ for any of the lanes indicated by > > > >> intel_dp->lane_count is not set. However these values cached in > > > >> intel_dp > > > >> structure can be stale if link training has failed for these values > > > >> during previous modeset. Or these values can get stale since we have > > > >> now re read the DPCD registers or it can be 0 in case of connected boot > > > >> case. > > > >> > > > >> This patch validates these values against the common_rates and max lane > > > >> count values. > > > >> > > > >> This is absolutely required incase the common_rates or max lane count > > > >> are now different due to link fallback. > > > >> > > > >> Cc: Ville Syrjala <[email protected]> > > > >> Cc: Jani Nikula <[email protected]> > > > >> Signed-off-by: Manasi Navare <[email protected]> > > > >> --- > > > >> drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++++++++++++--- > > > >> 1 file changed, 21 insertions(+), 3 deletions(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > >> b/drivers/gpu/drm/i915/intel_dp.c > > > >> index fd96a6c..51fa6b5 100644 > > > >> --- a/drivers/gpu/drm/i915/intel_dp.c > > > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > > > >> @@ -295,6 +295,23 @@ static int intel_dp_link_rate_index(struct > > > >> intel_dp *intel_dp, > > > >> return -1; > > > >> } > > > >> > > > >> +static bool intel_dp_link_params_is_valid(struct intel_dp *intel_dp) > > > >> +{ > > > >> + int common_rates[DP_MAX_SUPPORTED_RATES]; > > > >> + int link_rate_index; > > > >> + > > > >> + link_rate_index = intel_dp_link_rate_index(intel_dp, > > > >> + common_rates, > > > >> + intel_dp->link_rate); > > > > > > > > Hmm. I thought we started to store the common rates somewhere more > > > > permanent, or did I imagine that? > > > > > > That series got stalled waiting for Manasi's stuff to land... which > > > means it's now conflicting and pending rebase, and still waiting. :( > > > > > > J. > > > > > > > Hmm, yes but if you want I can work on rebasing it after these two patches > > land. That series is really required. > > > > > > > > > > > > > > >> + if (link_rate_index < 0) > > > >> + return false; > > > >> + if (!intel_dp->lane_count || > > > >> + (intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))) > > > > > > > > Needless parens. > > > > > > > > Ok, will remove that. > > > > > > > >> + return false; > > > >> + > > > >> + return true; > > > >> +} > > > >> + > > > >> int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > >> int link_rate, uint8_t > > > >> lane_count) > > > >> { > > > >> @@ -4224,9 +4241,10 @@ intel_dp_check_link_status(struct intel_dp > > > >> *intel_dp) > > > >> if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > >> return; > > > >> > > > >> - /* FIXME: we need to synchronize this sort of stuff with > > > >> hardware > > > >> - * readout. Currently fast link training doesn't work on > > > >> boot-up. */ > > > > > > > > You shouldn't remove the FIXME. It's still totally valid. > > > > > > > > But that check is now incorporated in validating the params. > > So why is separate check required? > > Or are you just saying leave the FIXME comment? > > Yes, leave the comment. Or better yet, move into > intel_dp_link_params_is_valid() and maybe reword a bit to something > like: > > "FIXME: we need to synchronize the current link parameters with hardware > readout ..." > > to make it it clear that it's actually referring to the current link > params and not something else. > >
Ok, I will move the comment into the function. And submit a new revision. Everything else looks good? Regards Manasi > > > > Manasi > > > > > > > >> - if (!intel_dp->lane_count) > > > >> + /* Validate the cached values of intel_dp->link_rate and > > > >> + * intel_dp->lane_count before attempting to retrain. > > > >> + */ > > > >> + if (!intel_dp_link_params_is_valid(intel_dp)) > > > >> return; > > > >> > > > >> /* Retrain if Channel EQ or CR not ok */ > > > >> -- > > > >> 2.1.4 > > > > > > -- > > > Jani Nikula, Intel Open Source Technology Center > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
