| Message ID | 1480984058-552-3-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jani, Could you please review this patch? This is the patch that calculates the max sink link rate and max sink lane count only once at hotplug and then anytime the max lane count and common rates are requested, the helper functions use these values. This simplifies the fallback logic since we can go ahead and update the max sink link rate and lane count to these fallback lower values since link training failed so thus capping the max values to fallback values. This is better than defining common_rates array in intel_dp, because its easier to update the max link rate vs going and updating and resizing the whole array. Also with this approach the helper functions do not change. Regards Manasi On Mon, Dec 05, 2016 at 04:27:36PM -0800, Manasi Navare wrote: > Sink's capabilities are advertised through DPCD registers and get > updated only on hotplug. So they should be computed only once in the > long pulse handler and saved off in intel_dp structure for the use > later. For this reason two new fields max_sink_lane_count and > max_sink_link_bw are added to intel_dp structure. > > This also simplifies the fallback link rate/lane count logic > to handle link training failure. In that case, the max_sink_link_bw > and max_sink_lane_count can be reccomputed to match the fallback > values lowering the sink capabilities due to link train failure. > > 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 | 10 ++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index db75bb9..434dc7d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > u8 source_max, sink_max; > > source_max = intel_dig_port->max_lanes; > - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > + sink_max = intel_dp->max_sink_lane_count; > > return min(source_max, sink_max); > } > @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > *sink_rates = default_rates; > > - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; > + return (intel_dp->max_sink_link_bw >> 3) + 1; > } > > static int > @@ -4395,6 +4395,12 @@ 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))); > > + /* 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); > + > intel_dp_print_rates(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 fd77a3b..b6526ad 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -906,6 +906,10 @@ struct intel_dp { > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ > uint8_t num_sink_rates; > int sink_rates[DP_MAX_SUPPORTED_RATES]; > + /* Max lane count for the sink as per DPCD registers */ > + uint8_t max_sink_lane_count; > + /* Max link BW for the sink as per DPCD registers */ > + int max_sink_link_bw; > /* sink or branch descriptor */ > struct intel_dp_desc desc; > struct drm_dp_aux aux; > -- > 1.9.1 >
On Tue, 06 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > Sink's capabilities are advertised through DPCD registers and get > updated only on hotplug. So they should be computed only once in the > long pulse handler and saved off in intel_dp structure for the use > later. For this reason two new fields max_sink_lane_count and > max_sink_link_bw are added to intel_dp structure. > > This also simplifies the fallback link rate/lane count logic > to handle link training failure. In that case, the max_sink_link_bw > and max_sink_lane_count can be reccomputed to match the fallback > values lowering the sink capabilities due to link train failure. > > 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> Reviewed-by: Jani Nikula <jani.nikula@intel.com> Eventually we may want to call the fields *link* rates, because that's what they'll effectively be. Transient values that don't reflect the sink or source capabilities, but the link capabilities. > --- > drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index db75bb9..434dc7d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > u8 source_max, sink_max; > > source_max = intel_dig_port->max_lanes; > - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > + sink_max = intel_dp->max_sink_lane_count; > > return min(source_max, sink_max); > } > @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > *sink_rates = default_rates; > > - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; > + return (intel_dp->max_sink_link_bw >> 3) + 1; > } > > static int > @@ -4395,6 +4395,12 @@ 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))); > > + /* 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); > + > intel_dp_print_rates(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 fd77a3b..b6526ad 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -906,6 +906,10 @@ struct intel_dp { > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ > uint8_t num_sink_rates; > int sink_rates[DP_MAX_SUPPORTED_RATES]; > + /* Max lane count for the sink as per DPCD registers */ > + uint8_t max_sink_lane_count; > + /* Max link BW for the sink as per DPCD registers */ > + int max_sink_link_bw; > /* sink or branch descriptor */ > struct intel_dp_desc desc; > struct drm_dp_aux aux;
On Thu, Dec 08, 2016 at 11:23:39PM +0200, Jani Nikula wrote: > On Tue, 06 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > Sink's capabilities are advertised through DPCD registers and get > > updated only on hotplug. So they should be computed only once in the > > long pulse handler and saved off in intel_dp structure for the use > > later. For this reason two new fields max_sink_lane_count and > > max_sink_link_bw are added to intel_dp structure. > > > > This also simplifies the fallback link rate/lane count logic > > to handle link training failure. In that case, the max_sink_link_bw > > and max_sink_lane_count can be reccomputed to match the fallback > > values lowering the sink capabilities due to link train failure. > > > > 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> > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Eventually we may want to call the fields *link* rates, because that's > what they'll effectively be. Transient values that don't reflect the > sink or source capabilities, but the link capabilities. > Thanks Jani for your r-b. Yes I agree, should I change the name to link_rate instead of sink_link_rate now? The only reason I kept it as sink_link_rate is because we go ahead and calculate the link capabilities later based on common link rate. Regards Manasi > > --- > > drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index db75bb9..434dc7d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > u8 source_max, sink_max; > > > > source_max = intel_dig_port->max_lanes; > > - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > + sink_max = intel_dp->max_sink_lane_count; > > > > return min(source_max, sink_max); > > } > > @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > > > *sink_rates = default_rates; > > > > - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; > > + return (intel_dp->max_sink_link_bw >> 3) + 1; > > } > > > > static int > > @@ -4395,6 +4395,12 @@ 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))); > > > > + /* 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); > > + > > intel_dp_print_rates(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 fd77a3b..b6526ad 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -906,6 +906,10 @@ struct intel_dp { > > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ > > uint8_t num_sink_rates; > > int sink_rates[DP_MAX_SUPPORTED_RATES]; > > + /* Max lane count for the sink as per DPCD registers */ > > + uint8_t max_sink_lane_count; > > + /* Max link BW for the sink as per DPCD registers */ > > + int max_sink_link_bw; > > /* sink or branch descriptor */ > > struct intel_dp_desc desc; > > struct drm_dp_aux aux; > > -- > Jani Nikula, Intel Open Source Technology Center
Daniel, can we merge this patch? It has no dependency on other link train patches, it is just a clean up for the existing driver code that uses max link rate and lane count values. Other link train patches have dependency on this thats why it was part of the series. But it would be great if this gets merged so that i dont have to rebase it tremendously after 3 weeks when I am back and plus its good for the driver to start using this clean up. This kind of clean up was long due. Regards Manasi On Thu, Dec 08, 2016 at 11:23:39PM +0200, Jani Nikula wrote: > On Tue, 06 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > Sink's capabilities are advertised through DPCD registers and get > > updated only on hotplug. So they should be computed only once in the > > long pulse handler and saved off in intel_dp structure for the use > > later. For this reason two new fields max_sink_lane_count and > > max_sink_link_bw are added to intel_dp structure. > > > > This also simplifies the fallback link rate/lane count logic > > to handle link training failure. In that case, the max_sink_link_bw > > and max_sink_lane_count can be reccomputed to match the fallback > > values lowering the sink capabilities due to link train failure. > > > > 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> > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Eventually we may want to call the fields *link* rates, because that's > what they'll effectively be. Transient values that don't reflect the > sink or source capabilities, but the link capabilities. > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index db75bb9..434dc7d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > u8 source_max, sink_max; > > > > source_max = intel_dig_port->max_lanes; > > - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > + sink_max = intel_dp->max_sink_lane_count; > > > > return min(source_max, sink_max); > > } > > @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) > > > > *sink_rates = default_rates; > > > > - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; > > + return (intel_dp->max_sink_link_bw >> 3) + 1; > > } > > > > static int > > @@ -4395,6 +4395,12 @@ 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))); > > > > + /* 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); > > + > > intel_dp_print_rates(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 fd77a3b..b6526ad 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -906,6 +906,10 @@ struct intel_dp { > > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ > > uint8_t num_sink_rates; > > int sink_rates[DP_MAX_SUPPORTED_RATES]; > > + /* Max lane count for the sink as per DPCD registers */ > > + uint8_t max_sink_lane_count; > > + /* Max link BW for the sink as per DPCD registers */ > > + int max_sink_link_bw; > > /* sink or branch descriptor */ > > struct intel_dp_desc desc; > > struct drm_dp_aux aux; > > -- > Jani Nikula, Intel Open Source Technology Center
On Thu, 08 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > Daniel, can we merge this patch? Pushed this one to dinq, thanks for the patch. BR, Jani. > It has no dependency on other link train patches, > it is just a clean up for the existing driver code that > uses max link rate and lane count values. > Other link train patches have dependency on this thats why > it was part of the series. > But it would be great if this gets merged so that i dont have to > rebase it tremendously after 3 weeks when I am back and plus > its good for the driver to start using this clean up. This kind > of clean up was long due. > > Regards > Manasi > > On Thu, Dec 08, 2016 at 11:23:39PM +0200, Jani Nikula wrote: >> On Tue, 06 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > Sink's capabilities are advertised through DPCD registers and get >> > updated only on hotplug. So they should be computed only once in the >> > long pulse handler and saved off in intel_dp structure for the use >> > later. For this reason two new fields max_sink_lane_count and >> > max_sink_link_bw are added to intel_dp structure. >> > >> > This also simplifies the fallback link rate/lane count logic >> > to handle link training failure. In that case, the max_sink_link_bw >> > and max_sink_lane_count can be reccomputed to match the fallback >> > values lowering the sink capabilities due to link train failure. >> > >> > 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> >> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com> >> >> Eventually we may want to call the fields *link* rates, because that's >> what they'll effectively be. Transient values that don't reflect the >> sink or source capabilities, but the link capabilities. >> >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++-- >> > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ >> > 2 files changed, 12 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index db75bb9..434dc7d 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) >> > u8 source_max, sink_max; >> > >> > source_max = intel_dig_port->max_lanes; >> > - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); >> > + sink_max = intel_dp->max_sink_lane_count; >> > >> > return min(source_max, sink_max); >> > } >> > @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) >> > >> > *sink_rates = default_rates; >> > >> > - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; >> > + return (intel_dp->max_sink_link_bw >> 3) + 1; >> > } >> > >> > static int >> > @@ -4395,6 +4395,12 @@ 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))); >> > >> > + /* 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); >> > + >> > intel_dp_print_rates(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 fd77a3b..b6526ad 100644 >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > @@ -906,6 +906,10 @@ struct intel_dp { >> > /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ >> > uint8_t num_sink_rates; >> > int sink_rates[DP_MAX_SUPPORTED_RATES]; >> > + /* Max lane count for the sink as per DPCD registers */ >> > + uint8_t max_sink_lane_count; >> > + /* Max link BW for the sink as per DPCD registers */ >> > + int max_sink_link_bw; >> > /* sink or branch descriptor */ >> > struct intel_dp_desc desc; >> > struct drm_dp_aux aux; >> >> -- >> 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 db75bb9..434dc7d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -156,7 +156,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) u8 source_max, sink_max; source_max = intel_dig_port->max_lanes; - sink_max = drm_dp_max_lane_count(intel_dp->dpcd); + sink_max = intel_dp->max_sink_lane_count; return min(source_max, sink_max); } @@ -213,7 +213,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) *sink_rates = default_rates; - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; + return (intel_dp->max_sink_link_bw >> 3) + 1; } static int @@ -4395,6 +4395,12 @@ 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))); + /* 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); + intel_dp_print_rates(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 fd77a3b..b6526ad 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -906,6 +906,10 @@ struct intel_dp { /* sink rates as reported by DP_SUPPORTED_LINK_RATES */ uint8_t num_sink_rates; int sink_rates[DP_MAX_SUPPORTED_RATES]; + /* Max lane count for the sink as per DPCD registers */ + uint8_t max_sink_lane_count; + /* Max link BW for the sink as per DPCD registers */ + int max_sink_link_bw; /* sink or branch descriptor */ struct intel_dp_desc desc; struct drm_dp_aux aux;
Sink's capabilities are advertised through DPCD registers and get updated only on hotplug. So they should be computed only once in the long pulse handler and saved off in intel_dp structure for the use later. For this reason two new fields max_sink_lane_count and max_sink_link_bw are added to intel_dp structure. This also simplifies the fallback link rate/lane count logic to handle link training failure. In that case, the max_sink_link_bw and max_sink_lane_count can be reccomputed to match the fallback values lowering the sink capabilities due to link train failure. 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 | 10 ++++++++-- drivers/gpu/drm/i915/intel_drv.h | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-)