| Message ID | 20180905095321.13843-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: optimize eDP 1.4+ link config fast and narrow | expand |
On Wed, Sep 05, 2018 at 12:53:21PM +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. In case of DSC, currently we only compute DSC params and enable DSC if intel_dp_compute_link_config() returns a false. So since the fast_narrow link calculation is embedded within the intel_dp_compute_link_config(), DSC can still be enabled if link config returns false with either fast_narrow or wide approach. However ideally, for power savings for eDP we could eventually enable DSC first if supported by the panel, find the mode rate with compressed bpp and then optimize the link config with that compressed bpp. So reverse the order of dsc_compute_config and intel_dp_compute_link_config() Everything else in the patch looks good to me. Acked-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Matt Atwood <matthew.s.atwood@intel.com> > Cc: "Lee, Shawn C" <shawn.c.lee@intel.com> > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com > > 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 436c22de33b6..bf7b91832c8a 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1921,6 +1921,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) > +{ > + 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) > @@ -1945,13 +1981,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; > @@ -1965,12 +2003,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. > + */ > + 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 >
On Wed, Sep 05, 2018 at 12:53:21PM +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@linux.intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Matt Atwood <matthew.s.atwood@intel.com> > Cc: "Lee, Shawn C" <shawn.c.lee@intel.com> > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com > > 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 436c22de33b6..bf7b91832c8a 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1921,6 +1921,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) > +{ > + 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) > @@ -1945,13 +1981,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; > @@ -1965,12 +2003,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)) { Nitpick below: Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below: if (intel_dp->edp_dpcd[0] < DP_EDP_14) { limits.min_lane_count = limits.max_lane_count; limits.min_clock = limits.max_clock; } With that, Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > + /* > + * 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. > + */ > + 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 >
On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Wed, Sep 05, 2018 at 12:53:21PM +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@linux.intel.com> >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Matt Atwood <matthew.s.atwood@intel.com> >> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com> >> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com >> >> 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 436c22de33b6..bf7b91832c8a 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1921,6 +1921,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) >> +{ >> + 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) >> @@ -1945,13 +1981,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; >> @@ -1965,12 +2003,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)) { > > Nitpick below: > Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below: > > if (intel_dp->edp_dpcd[0] < DP_EDP_14) { > limits.min_lane_count = limits.max_lane_count; > limits.min_clock = limits.max_clock; > } Doing that would require duplicating the debug logging to both branches of the if (intel_dp_is_edp(intel_dp)) condition. BR, Jani. > > With that, > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > > Manasi > >> + /* >> + * 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. >> + */ >> + 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 >>
On Thu, Sep 27, 2018 at 02:15:26PM +0300, Jani Nikula wrote: > On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: > > On Wed, Sep 05, 2018 at 12:53:21PM +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@linux.intel.com> > >> Cc: Manasi Navare <manasi.d.navare@intel.com> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Cc: Matt Atwood <matthew.s.atwood@intel.com> > >> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com> > >> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > >> --- > >> > >> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com > >> > >> 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 436c22de33b6..bf7b91832c8a 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -1921,6 +1921,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) > >> +{ > >> + 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) > >> @@ -1945,13 +1981,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; > >> @@ -1965,12 +2003,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)) { > > > > Nitpick below: > > Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below: > > > > if (intel_dp->edp_dpcd[0] < DP_EDP_14) { > > limits.min_lane_count = limits.max_lane_count; > > limits.min_clock = limits.max_clock; > > } > > Doing that would require duplicating the debug logging to both branches > of the if (intel_dp_is_edp(intel_dp)) condition. > > BR, > Jani. > Ok yes makes sense, now its common to both eDP and DP cases. Manasi > > > > With that, > > > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > > > > Manasi > > > >> + /* > >> + * 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. > >> + */ > >> + 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 > >> > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Thu, Sep 27, 2018 at 02:15:26PM +0300, Jani Nikula wrote: >> On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > On Wed, Sep 05, 2018 at 12:53:21PM +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@linux.intel.com> >> >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> >> Cc: Matt Atwood <matthew.s.atwood@intel.com> >> >> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com> >> >> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267 >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Thanks for the review, pushed to dinq. BR, Jani. > > >> >> --- >> >> >> >> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com >> >> >> >> 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 436c22de33b6..bf7b91832c8a 100644 >> >> --- a/drivers/gpu/drm/i915/intel_dp.c >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> >> @@ -1921,6 +1921,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) >> >> +{ >> >> + 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) >> >> @@ -1945,13 +1981,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; >> >> @@ -1965,12 +2003,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)) { >> > >> > Nitpick below: >> > Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below: >> > >> > if (intel_dp->edp_dpcd[0] < DP_EDP_14) { >> > limits.min_lane_count = limits.max_lane_count; >> > limits.min_clock = limits.max_clock; >> > } >> >> Doing that would require duplicating the debug logging to both branches >> of the if (intel_dp_is_edp(intel_dp)) condition. >> >> BR, >> Jani. >> > > Ok yes makes sense, now its common to both eDP and DP cases. > > Manasi > >> > >> > With that, >> > >> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> > >> > Manasi >> > >> >> + /* >> >> + * 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. >> >> + */ >> >> + 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 >> >> >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 436c22de33b6..bf7b91832c8a 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1921,6 +1921,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) +{ + 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) @@ -1945,13 +1981,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; @@ -1965,12 +2003,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. + */ + 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,