| Message ID | 1486515251-23469-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, I implemented this based on our IRC discussion on resetting the link params only on HPD and not on every long pulse function call. This will avoid them getting reset during the fill_modes call which is exactly what is required duirng the link training fallback tow ork correctly. Could you please take a look at this when you get a chance and let me know if anything else is required. Regards Manasi On Tue, Feb 07, 2017 at 04:54:11PM -0800, Manasi Navare wrote: > The max link parameters should be set/reset only on HPD or > connected boot case or on system resume. > > Add a flag reset_link_params to intel_dp to decide when > to reset the max link parameters. This prevents the parameters > from getting reset/overwritten through all other > connector->funcs->detect() calls. This is important when link > training fails and the max link params are modified to the > lower fallback values. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 21924cf..ed28788b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4624,11 +4624,15 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > yesno(intel_dp_source_supports_hbr2(intel_dp)), > yesno(drm_dp_tps3_supported(intel_dp->dpcd))); > > - /* Set the max lane count for sink */ > - intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > + if (intel_dp->reset_link_params) { > + /* Set the max lane count for sink */ > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > - /* Set the max link BW for sink */ > - intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > + /* Set the max link BW for sink */ > + intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > + > + intel_dp->reset_link_params = false; > + } > > intel_dp_print_rates(intel_dp); > > @@ -5010,6 +5014,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > if (lspcon->active) > lspcon_resume(lspcon); > > + intel_dp->reset_link_params = true; > + > pps_lock(intel_dp); > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > @@ -5079,6 +5085,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > long_hpd ? "long" : "short"); > > if (long_hpd) { > + intel_dp->reset_link_params = true; > intel_dp->detect_done = false; > return IRQ_NONE; > } > @@ -5920,6 +5927,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dig_port->max_lanes, port_name(port))) > return false; > > + intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index fbe5c72..f579776 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -923,6 +923,7 @@ struct intel_dp { > bool has_audio; > bool detect_done; > bool channel_eq_status; > + bool reset_link_params; > enum hdmi_force_audio force_audio; > bool limited_color_range; > bool color_range_auto; > -- > 2.1.4 >
On Tue, Feb 07, 2017 at 04:54:11PM -0800, Manasi Navare wrote: > The max link parameters should be set/reset only on HPD or > connected boot case or on system resume. > > Add a flag reset_link_params to intel_dp to decide when > to reset the max link parameters. This prevents the parameters > from getting reset/overwritten through all other > connector->funcs->detect() calls. This is important when link > training fails and the max link params are modified to the > lower fallback values. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Looks all right to me. Though I suppose this conflicts with Jani's changes? Anyways Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 21924cf..ed28788b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4624,11 +4624,15 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > yesno(intel_dp_source_supports_hbr2(intel_dp)), > yesno(drm_dp_tps3_supported(intel_dp->dpcd))); > > - /* Set the max lane count for sink */ > - intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > + if (intel_dp->reset_link_params) { > + /* Set the max lane count for sink */ > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > - /* Set the max link BW for sink */ > - intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > + /* Set the max link BW for sink */ > + intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > + > + intel_dp->reset_link_params = false; > + } > > intel_dp_print_rates(intel_dp); > > @@ -5010,6 +5014,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > if (lspcon->active) > lspcon_resume(lspcon); > > + intel_dp->reset_link_params = true; > + > pps_lock(intel_dp); > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > @@ -5079,6 +5085,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > long_hpd ? "long" : "short"); > > if (long_hpd) { > + intel_dp->reset_link_params = true; > intel_dp->detect_done = false; > return IRQ_NONE; > } > @@ -5920,6 +5927,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dig_port->max_lanes, port_name(port))) > return false; > > + intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index fbe5c72..f579776 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -923,6 +923,7 @@ struct intel_dp { > bool has_audio; > bool detect_done; > bool channel_eq_status; > + bool reset_link_params; > enum hdmi_force_audio force_audio; > bool limited_color_range; > bool color_range_auto; > -- > 2.1.4
On Tue, Feb 14, 2017 at 09:08:25PM +0200, Ville Syrjälä wrote: > On Tue, Feb 07, 2017 at 04:54:11PM -0800, Manasi Navare wrote: > > The max link parameters should be set/reset only on HPD or > > connected boot case or on system resume. > > > > Add a flag reset_link_params to intel_dp to decide when > > to reset the max link parameters. This prevents the parameters > > from getting reset/overwritten through all other > > connector->funcs->detect() calls. This is important when link > > training fails and the max link params are modified to the > > lower fallback values. > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Looks all right to me. Though I suppose this conflicts with Jani's > changes? > > Anyways > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Thanks for the review. With Jani's changes only the name changes to max_link_rate and max_link_lane_count. But the logic remains the same. I have tested this on top of Jani's patches and it works great. If this gets merged, Jani's patch will have to be rebased on top of this else vice versa if Jani's patches get merged first. Regards Manasi > > --- > > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++---- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 2 files changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 21924cf..ed28788b 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4624,11 +4624,15 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > yesno(intel_dp_source_supports_hbr2(intel_dp)), > > yesno(drm_dp_tps3_supported(intel_dp->dpcd))); > > > > - /* Set the max lane count for sink */ > > - intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > + if (intel_dp->reset_link_params) { > > + /* Set the max lane count for sink */ > > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > > > - /* Set the max link BW for sink */ > > - intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > > + /* Set the max link BW for sink */ > > + intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); > > + > > + intel_dp->reset_link_params = false; > > + } > > > > intel_dp_print_rates(intel_dp); > > > > @@ -5010,6 +5014,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > > if (lspcon->active) > > lspcon_resume(lspcon); > > > > + intel_dp->reset_link_params = true; > > + > > pps_lock(intel_dp); > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > @@ -5079,6 +5085,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > long_hpd ? "long" : "short"); > > > > if (long_hpd) { > > + intel_dp->reset_link_params = true; > > intel_dp->detect_done = false; > > return IRQ_NONE; > > } > > @@ -5920,6 +5927,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > intel_dig_port->max_lanes, port_name(port))) > > return false; > > > > + intel_dp->reset_link_params = true; > > intel_dp->pps_pipe = INVALID_PIPE; > > intel_dp->active_pipe = INVALID_PIPE; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index fbe5c72..f579776 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -923,6 +923,7 @@ struct intel_dp { > > bool has_audio; > > bool detect_done; > > bool channel_eq_status; > > + bool reset_link_params; > > enum hdmi_force_audio force_audio; > > bool limited_color_range; > > bool color_range_auto; > > -- > > 2.1.4 > > -- > Ville Syrjälä > Intel OTC
On Tue, 14 Feb 2017, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Tue, Feb 14, 2017 at 09:08:25PM +0200, Ville Syrjälä wrote: >> On Tue, Feb 07, 2017 at 04:54:11PM -0800, Manasi Navare wrote: >> > The max link parameters should be set/reset only on HPD or >> > connected boot case or on system resume. >> > >> > Add a flag reset_link_params to intel_dp to decide when >> > to reset the max link parameters. This prevents the parameters >> > from getting reset/overwritten through all other >> > connector->funcs->detect() calls. This is important when link >> > training fails and the max link params are modified to the >> > lower fallback values. >> > >> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> >> Looks all right to me. Though I suppose this conflicts with Jani's >> changes? >> >> Anyways >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > Thanks for the review. > With Jani's changes only the name changes to max_link_rate and max_link_lane_count. > But the logic remains the same. > I have tested this on top of Jani's patches and it works great. > If this gets merged, Jani's patch will have to be rebased on top of this else > vice versa if Jani's patches get merged first. Pushed to drm-intel-next-queued, thanks for the patch and review. I'll need to rebase my stuff on top. BR, Jani. > > Regards > Manasi >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++---- >> > drivers/gpu/drm/i915/intel_drv.h | 1 + >> > 2 files changed, 13 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index 21924cf..ed28788b 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -4624,11 +4624,15 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) >> > yesno(intel_dp_source_supports_hbr2(intel_dp)), >> > yesno(drm_dp_tps3_supported(intel_dp->dpcd))); >> > >> > - /* Set the max lane count for sink */ >> > - intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); >> > + if (intel_dp->reset_link_params) { >> > + /* Set the max lane count for sink */ >> > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); >> > >> > - /* Set the max link BW for sink */ >> > - intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); >> > + /* Set the max link BW for sink */ >> > + intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); >> > + >> > + intel_dp->reset_link_params = false; >> > + } >> > >> > intel_dp_print_rates(intel_dp); >> > >> > @@ -5010,6 +5014,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) >> > if (lspcon->active) >> > lspcon_resume(lspcon); >> > >> > + intel_dp->reset_link_params = true; >> > + >> > pps_lock(intel_dp); >> > >> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) >> > @@ -5079,6 +5085,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >> > long_hpd ? "long" : "short"); >> > >> > if (long_hpd) { >> > + intel_dp->reset_link_params = true; >> > intel_dp->detect_done = false; >> > return IRQ_NONE; >> > } >> > @@ -5920,6 +5927,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> > intel_dig_port->max_lanes, port_name(port))) >> > return false; >> > >> > + intel_dp->reset_link_params = true; >> > intel_dp->pps_pipe = INVALID_PIPE; >> > intel_dp->active_pipe = INVALID_PIPE; >> > >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> > index fbe5c72..f579776 100644 >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > @@ -923,6 +923,7 @@ struct intel_dp { >> > bool has_audio; >> > bool detect_done; >> > bool channel_eq_status; >> > + bool reset_link_params; >> > enum hdmi_force_audio force_audio; >> > bool limited_color_range; >> > bool color_range_auto; >> > -- >> > 2.1.4 >> >> -- >> Ville Syrjälä >> Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 21924cf..ed28788b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4624,11 +4624,15 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) yesno(intel_dp_source_supports_hbr2(intel_dp)), yesno(drm_dp_tps3_supported(intel_dp->dpcd))); - /* Set the max lane count for sink */ - intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); + if (intel_dp->reset_link_params) { + /* Set the max lane count for sink */ + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); - /* Set the max link BW for sink */ - intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); + /* Set the max link BW for sink */ + intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp); + + intel_dp->reset_link_params = false; + } intel_dp_print_rates(intel_dp); @@ -5010,6 +5014,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) if (lspcon->active) lspcon_resume(lspcon); + intel_dp->reset_link_params = true; + pps_lock(intel_dp); if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) @@ -5079,6 +5085,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) long_hpd ? "long" : "short"); if (long_hpd) { + intel_dp->reset_link_params = true; intel_dp->detect_done = false; return IRQ_NONE; } @@ -5920,6 +5927,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_dig_port->max_lanes, port_name(port))) return false; + intel_dp->reset_link_params = true; intel_dp->pps_pipe = INVALID_PIPE; intel_dp->active_pipe = INVALID_PIPE; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index fbe5c72..f579776 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -923,6 +923,7 @@ struct intel_dp { bool has_audio; bool detect_done; bool channel_eq_status; + bool reset_link_params; enum hdmi_force_audio force_audio; bool limited_color_range; bool color_range_auto;
The max link parameters should be set/reset only on HPD or connected boot case or on system resume. Add a flag reset_link_params to intel_dp to decide when to reset the max link parameters. This prevents the parameters from getting reset/overwritten through all other connector->funcs->detect() calls. This is important when link training fails and the max link params are modified to the lower fallback values. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 13 insertions(+), 4 deletions(-)