On Wed, 2018-05-09 at 08:09 -0700, Matt Atwood wrote: > On Wed, 2018-05-09 at 05:21 -0700, Rodrigo Vivi wrote: > > On Wed, May 09, 2018 at 10:13:21AM +0300, Jani Nikula wrote: > > > We've opted to use the maximum link rate and lane count for eDP > > > panels, > > > because typically the maximum supported configuration reported by > > > the > > > panel has matched the native resolution requirements of the > > > panel, > > > and > > > optimizing the link has lead to problems. > > > > > > With eDP 1.4 rate select method and DSC features, this is > > > decreasingly > > > the case. There's a need to optimize the link parameters. > > > Moreover, > > > already eDP 1.3 states fast link with fewer lanes is preferred > > > over > > > the > > > wide and slow. (Wide and slow should still be more reliable for > > > longer > > > cable lengths.) > > > > > > Additionally, there have been reports of panels failing on > > > arbitrary > > > link configurations, although arguably all configurations they > > > claim to > > > support should work. > > > > > > Optimize eDP 1.4+ link config fast and narrow. > > > > > > Side note: The implementation has a near duplicate of the link > > > config > > > function, with just the two inner for loops turned inside out. > > > Perhaps > > > there'd be a way to make this, say, more table driven to reduce > > > the > > > duplication, but seems like that would lead to duplication in the > > > table > > > generation. We'll also have to see how the link config > > > optimization > > > for > > > DSC turns out. > > > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > Cc: Matt Atwood <matthew.s.atwood@xxxxxxxxx> > > > > I believe Matt is interested on this and know who could test this > > for > > us. > > I'm actually in the process of working with my counterpart at Google > to > actually quantify what power is saved. With both chromeos- > 4.14/chromeos-4.4 patches to do so across multiple boards and > multiple > systems. > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 > > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> > > > > This matches my understand of the eDP 1.4 spec I believe this is > > the > > way to go, so > > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > but probably better to get a proper review and wait for someone > > to test... > > > > > > > > --- > > > > > > Untested. It's possible this helps the referenced bug. The > > > downside > > > is > > > that this patch has a bunch of dependencies that are too much to > > > backport to stable kernels. If the patch works, we may need to > > > consider > > > hacking together an uglier backport. > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 73 > > > ++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 62 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index dde92e4af5d3..1ec62965ece3 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1768,6 +1768,42 @@ intel_dp_compute_link_config_wide(struct > > > intel_dp *intel_dp, > > > return false; > > > } > > > > > > +/* Optimize link config in order: max bpp, min lanes, min clock > > > */ > > > +static bool > > > +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp, > > > + struct intel_crtc_state > > > *pipe_config, > > > + const struct > > > link_config_limits > > > *limits) > > I personally called this intel_dp_compute_link_config_narrow as a > counterpart to the "wide" implementation. > > > +{ > > > + struct drm_display_mode *adjusted_mode = &pipe_config- > > > > base.adjusted_mode; > > > > > > + int bpp, clock, lane_count; > > > + int mode_rate, link_clock, link_avail; > > > + > > > + for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp > > > -= > > > 2 * 3) { > > > + mode_rate = > > > intel_dp_link_required(adjusted_mode- > > > > crtc_clock, > > > > > > + bpp); > > > + > > > + for (lane_count = limits->min_lane_count; > > > + lane_count <= limits->max_lane_count; > > > + lane_count <<= 1) { > > > + for (clock = limits->min_clock; clock <= > > > limits->max_clock; clock++) { > > > + link_clock = intel_dp- > > > > common_rates[clock]; > > > > > > + link_avail = > > > intel_dp_max_data_rate(link_clock, > > > + > > > > > > lane_count); > > > + > > > + if (mode_rate <= link_avail) { > > > + pipe_config->lane_count > > > = > > > lane_count; > > > + pipe_config->pipe_bpp = > > > bpp; > > > + pipe_config->port_clock > > > = > > > link_clock; > > > + > > > + return true; > > > + } > > > + } > > > + } > > > + } > > > + > > > + return false; > > > +} > > > + > > > static bool > > > intel_dp_compute_link_config(struct intel_encoder *encoder, > > > struct intel_crtc_state > > > *pipe_config) > > > @@ -1792,13 +1828,15 @@ intel_dp_compute_link_config(struct > > > intel_encoder *encoder, > > > limits.min_bpp = 6 * 3; > > > limits.max_bpp = intel_dp_compute_bpp(intel_dp, > > > pipe_config); > > > > > > - if (intel_dp_is_edp(intel_dp)) { > > > + if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < > > > DP_EDP_14) { > > > /* > > > * Use the maximum clock and number of lanes the > > > eDP panel > > > - * advertizes being capable of. The panels are > > > generally > > > - * designed to support only a single clock and > > > lane > > > - * configuration, and typically these values > > > correspond to the > > > - * native resolution of the panel. > > > + * advertizes being capable of. The eDP 1.3 and > > > earlier panels > > > + * are generally designed to support only a > > > single > > > clock and > > > + * lane configuration, and typically these > > > values > > > correspond to > > > + * the native resolution of the panel. With eDP > > > 1.4 rate select > > > + * and DSC, this is decreasingly the case, and > > > we > > > need to be > > > + * able to select less than maximum link config. > > > */ > > > limits.min_lane_count = limits.max_lane_count; > > > limits.min_clock = limits.max_clock; > > > @@ -1812,12 +1850,25 @@ intel_dp_compute_link_config(struct > > > intel_encoder *encoder, > > > intel_dp->common_rates[limits.max_clock], > > > limits.max_bpp, adjusted_mode- > > > >crtc_clock); > > > > > > - /* > > > - * Optimize for slow and wide. This is the place to add > > > alternative > > > - * optimization policy. > > > - */ > > > - if (!intel_dp_compute_link_config_wide(intel_dp, > > > pipe_config, &limits)) > > > - return false; > > > + if (intel_dp_is_edp(intel_dp)) { > > > + /* > > > + * Optimize for fast and narrow. eDP 1.3 section > > > 3.3 and eDP 1.4 > > > + * section A.1: "It is recommended that the > > > minimum number of > > > + * lanes be used, using the minimum link rate > > > allowed for that > > > + * lane configuration." > > > + * > > > + * Note that we use the max clock and lane count > > > for eDP 1.3 and > > > + * earlier, and fast vs. wide is irrelevant. > > > + */ > > This is where I got hung up on *many* eDP panels. This will break > many > pre edp 1.4 panels. I wrapped mine on a DPCD compare to DP_DPCD_REV. disregard this statement missed the second conditional > > > + if (!intel_dp_compute_link_config_fast(intel_dp, > > > pipe_config, > > > + &limits)) > > > + return false; > > > + } else { > > > + /* Optimize for slow and wide. */ > > > + if (!intel_dp_compute_link_config_wide(intel_dp, > > > pipe_config, > > > + &limits)) > > > + return false; > > > + } > > > > > > DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", > > > pipe_config->lane_count, pipe_config- > > > > port_clock, > > > > > > -- > > > 2.11.0 > > > > > I'm honestly glad that someone else cares I know in the case of the > wide optimization on a product I was working one we saved 200 uW, I'm > hoping for a bigger share with the narrow/fast implementation. > > -Matt _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
