| Message ID | 1491512412-30016-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 02:00:12PM -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 max link rate and max lane > count values. > > This is absolutely required incase the common_rates or max lane count > are now different due to link fallback. > > v2: > * Include the FIXME commnet inside the function (Ville Syrjala) > * Remove the redundant parenthesis (Ville Syrjala) > > v3 by Jani: > * rebase on the DP refactoring series > * rename intel_dp_link_params_is_valid to intel_dp_link_params_valid > * minor stylistic changes > > v4: > * Compare the link rate against max link rate not the > common_rates since common_rates does not account for the > lowered fallback link rate value. (Ville Syrjala) > > v5: > * Fixed a warning for unused variable (Manasi) > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 3c5c80d..36485be 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, > return 0; > } > > +static bool intel_dp_link_params_valid(struct intel_dp *intel_dp) > +{ > + /* > + * FIXME: we need to synchronize the current link parameters with > + * hardware readout. Currently fast link training doesn't work on > + * boot-up. > + */ > + if (intel_dp->link_rate == 0 || > + intel_dp->link_rate > intel_dp->max_link_rate) > + return false; > + > + if (intel_dp->lane_count == 0 || > + intel_dp->lane_count > intel_dp_max_lane_count(intel_dp)) > + return false; > + > + return true; > +} This lgtm Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count) > { > @@ -4253,9 +4271,11 @@ 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. */ > - 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_valid(intel_dp)) > return; > > /* Retrain if Channel EQ or CR not ok */ > -- > 2.1.4
On Wed, 12 Apr 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Apr 06, 2017 at 02:00:12PM -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 max link rate and max lane >> count values. >> >> This is absolutely required incase the common_rates or max lane count >> are now different due to link fallback. >> >> v2: >> * Include the FIXME commnet inside the function (Ville Syrjala) >> * Remove the redundant parenthesis (Ville Syrjala) >> >> v3 by Jani: >> * rebase on the DP refactoring series >> * rename intel_dp_link_params_is_valid to intel_dp_link_params_valid >> * minor stylistic changes >> >> v4: >> * Compare the link rate against max link rate not the >> common_rates since common_rates does not account for the >> lowered fallback link rate value. (Ville Syrjala) >> >> v5: >> * Fixed a warning for unused variable (Manasi) >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 3c5c80d..36485be 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, >> return 0; >> } >> >> +static bool intel_dp_link_params_valid(struct intel_dp *intel_dp) >> +{ >> + /* >> + * FIXME: we need to synchronize the current link parameters with >> + * hardware readout. Currently fast link training doesn't work on >> + * boot-up. >> + */ >> + if (intel_dp->link_rate == 0 || >> + intel_dp->link_rate > intel_dp->max_link_rate) >> + return false; >> + >> + if (intel_dp->lane_count == 0 || >> + intel_dp->lane_count > intel_dp_max_lane_count(intel_dp)) >> + return false; >> + >> + return true; >> +} > > This lgtm > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Pushed to drm-intel-next-queued, thanks for the patch and review. BR, Jani. > >> + >> int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, >> int link_rate, uint8_t lane_count) >> { >> @@ -4253,9 +4271,11 @@ 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. */ >> - 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_valid(intel_dp)) >> return; >> >> /* Retrain if Channel EQ or CR not ok */ >> -- >> 2.1.4
Thanks Ville for the review and thanks Jani for pushing this patch. Now we are down to 1 patch to get merged! Regards Manasi On Wed, 12 Apr 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Apr 06, 2017 at 02:00:12PM -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 max link rate and max >> lane count values. >> >> This is absolutely required incase the common_rates or max lane count >> are now different due to link fallback. >> >> v2: >> * Include the FIXME commnet inside the function (Ville Syrjala) >> * Remove the redundant parenthesis (Ville Syrjala) >> >> v3 by Jani: >> * rebase on the DP refactoring series >> * rename intel_dp_link_params_is_valid to intel_dp_link_params_valid >> * minor stylistic changes >> >> v4: >> * Compare the link rate against max link rate not the common_rates >> since common_rates does not account for the lowered fallback link >> rate value. (Ville Syrjala) >> >> v5: >> * Fixed a warning for unused variable (Manasi) >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c index 3c5c80d..36485be 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, >> return 0; >> } >> >> +static bool intel_dp_link_params_valid(struct intel_dp *intel_dp) { >> + /* >> + * FIXME: we need to synchronize the current link parameters with >> + * hardware readout. Currently fast link training doesn't work on >> + * boot-up. >> + */ >> + if (intel_dp->link_rate == 0 || >> + intel_dp->link_rate > intel_dp->max_link_rate) >> + return false; >> + >> + if (intel_dp->lane_count == 0 || >> + intel_dp->lane_count > intel_dp_max_lane_count(intel_dp)) >> + return false; >> + >> + return true; >> +} > > This lgtm > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Pushed to drm-intel-next-queued, thanks for the patch and review. BR, Jani. > >> + >> int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, >> int link_rate, uint8_t lane_count) { @@ -4253,9 +4271,11 >> @@ 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. */ >> - 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_valid(intel_dp)) >> return; >> >> /* Retrain if Channel EQ or CR not ok */ >> -- >> 2.1.4 -- 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 3c5c80d..36485be 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, return 0; } +static bool intel_dp_link_params_valid(struct intel_dp *intel_dp) +{ + /* + * FIXME: we need to synchronize the current link parameters with + * hardware readout. Currently fast link training doesn't work on + * boot-up. + */ + if (intel_dp->link_rate == 0 || + intel_dp->link_rate > intel_dp->max_link_rate) + return false; + + if (intel_dp->lane_count == 0 || + intel_dp->lane_count > intel_dp_max_lane_count(intel_dp)) + return false; + + return true; +} + int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, int link_rate, uint8_t lane_count) { @@ -4253,9 +4271,11 @@ 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. */ - 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_valid(intel_dp)) return; /* Retrain if Channel EQ or CR not ok */