| Message ID | 20180516080110.22770-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. > > Per the report, no matter what display mode you select with xrandr, > the > i915 driver will always select the alternate fixed mode. For the > reporter this means that the display will always run at 40Hz which is > quite annoying. This may be due to the mode comparison. > > But there are some other potential issues. The choice of > alt_fixed_mode > seems dubious. It's the first non-preferred mode, but there are no > guarantees that the only difference would be refresh rate. Similarly, > there may be more than one preferred mode in the probed modes list, > and > the commit changes the preferred mode selection to choose the last > one > on the list instead of the first. > > (Note that the probed modes list is the raw, unfiltered, unsorted > list > of modes from drm_add_edid_modes(), not the pretty result after a > drm_helper_probe_single_connector_modes() call.) > > Finally, we already have eerily similar code in place to find the > downclock mode for DRRS that seems like could be reused here. > > Back to the drawing board. > > Note: This is a hand-crafted revert due to conflicts. If it fails to > backport, please just try reverting the original commit directly. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > Reported-by: Rune Petersen <rune@megahurts.dk> > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for > eDP if available.") > Cc: Clint Taylor <clinton.a.taylor@intel.com> > Cc: David Weinehall <david.weinehall@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jim Bride <jim.bride@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v4.14+ > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------------- > ---------- > drivers/gpu/drm/i915/intel_drv.h | 2 -- > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > drivers/gpu/drm/i915/intel_dvo.c | 2 +- > drivers/gpu/drm/i915/intel_lvds.c | 3 +-- > drivers/gpu/drm/i915/intel_panel.c | 6 ------ > 6 files changed, 8 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index dde92e4af5d3..8320f0e8e3be 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct > intel_dp *intel_dp, > return bpp; > } > > -static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, > - struct drm_display_mode *m2) > -{ > - bool bres = false; > - > - if (m1 && m2) > - bres = (m1->hdisplay == m2->hdisplay && > - m1->hsync_start == m2->hsync_start && > - m1->hsync_end == m2->hsync_end && > - m1->htotal == m2->htotal && > - m1->vdisplay == m2->vdisplay && > - m1->vsync_start == m2->vsync_start && > - m1->vsync_end == m2->vsync_end && > - m1->vtotal == m2->vtotal); > - return bres; > -} > - > /* Adjust link config limits based on compliance test requests. */ > static void > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct intel_encoder > *encoder, > pipe_config->has_audio = intel_conn_state- > >force_audio == HDMI_AUDIO_ON; > > if (intel_dp_is_edp(intel_dp) && intel_connector- > >panel.fixed_mode) { > - struct drm_display_mode *panel_mode = > - intel_connector->panel.alt_fixed_mode; > - struct drm_display_mode *req_mode = &pipe_config- > >base.mode; > - > - if (!intel_edp_compare_alt_mode(req_mode, > panel_mode)) > - panel_mode = intel_connector- > >panel.fixed_mode; > - > - drm_mode_debug_printmodeline(panel_mode); > - > - intel_fixed_panel_mode(panel_mode, adjusted_mode); > + intel_fixed_panel_mode(intel_connector- > >panel.fixed_mode, > + adjusted_mode); > > if (INTEL_GEN(dev_priv) >= 9) { > int ret; > @@ -6159,7 +6134,6 @@ static bool intel_edp_init_connector(struct > intel_dp *intel_dp, > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_connector *connector = &intel_connector->base; > struct drm_display_mode *fixed_mode = NULL; > - struct drm_display_mode *alt_fixed_mode = NULL; > struct drm_display_mode *downclock_mode = NULL; > bool has_dpcd; > struct drm_display_mode *scan; > @@ -6214,14 +6188,13 @@ static bool intel_edp_init_connector(struct > intel_dp *intel_dp, > } > intel_connector->edid = edid; > > - /* prefer fixed mode from EDID if available, save an alt > mode also */ > + /* prefer fixed mode from EDID if available */ > list_for_each_entry(scan, &connector->probed_modes, head) { > if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { > fixed_mode = drm_mode_duplicate(dev, scan); > downclock_mode = intel_dp_drrs_init( > intel_connector, > fixed_mode); > - } else if (!alt_fixed_mode) { > - alt_fixed_mode = drm_mode_duplicate(dev, > scan); > + break; If multiple preferred modes are present, we'll now end up calling drrs_init() only for the first. I see that this is what the original code did but this revert does more than removing support for alternate modes. > } > } > > @@ -6258,8 +6231,7 @@ static bool intel_edp_init_connector(struct > intel_dp *intel_dp, > pipe_name(pipe)); > } > > - intel_panel_init(&intel_connector->panel, fixed_mode, > alt_fixed_mode, > - downclock_mode); > + intel_panel_init(&intel_connector->panel, fixed_mode, > downclock_mode); > intel_connector->panel.backlight.power = > intel_edp_backlight_power; > intel_panel_setup_backlight(connector, pipe); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index d7dbca1aabff..0361130500a6 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -277,7 +277,6 @@ struct intel_encoder { > > struct intel_panel { > struct drm_display_mode *fixed_mode; > - struct drm_display_mode *alt_fixed_mode; > struct drm_display_mode *downclock_mode; > > /* backlight */ > @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct > drm_i915_private *dev_priv); > /* intel_panel.c */ > int intel_panel_init(struct intel_panel *panel, > struct drm_display_mode *fixed_mode, > - struct drm_display_mode *alt_fixed_mode, > struct drm_display_mode *downclock_mode); > void intel_panel_fini(struct intel_panel *panel); > void intel_fixed_panel_mode(const struct drm_display_mode > *fixed_mode, > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > b/drivers/gpu/drm/i915/intel_dsi.c > index 51a1d6868b1e..cf39ca90d887 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct drm_i915_private > *dev_priv) > connector->display_info.width_mm = fixed_mode->width_mm; > connector->display_info.height_mm = fixed_mode->height_mm; > > - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, > NULL); > + intel_panel_init(&intel_connector->panel, fixed_mode, NULL); > intel_panel_setup_backlight(connector, INVALID_PIPE); > > intel_dsi_add_properties(intel_connector); > diff --git a/drivers/gpu/drm/i915/intel_dvo.c > b/drivers/gpu/drm/i915/intel_dvo.c > index eb0c559b2715..a70d767313aa 100644 > --- a/drivers/gpu/drm/i915/intel_dvo.c > +++ b/drivers/gpu/drm/i915/intel_dvo.c > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private > *dev_priv) > */ > intel_panel_init(&intel_connector->panel, > intel_dvo_get_current_mode( > intel_encoder), > - NULL, NULL); > + NULL); > intel_dvo->panel_wants_dither = true; > } > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > b/drivers/gpu/drm/i915/intel_lvds.c > index 8691c86f579c..d8ece907ff54 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct drm_i915_private > *dev_priv) > out: > mutex_unlock(&dev->mode_config.mutex); > > - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, > - downclock_mode); > + intel_panel_init(&intel_connector->panel, fixed_mode, > downclock_mode); > intel_panel_setup_backlight(connector, INVALID_PIPE); > > lvds_encoder->is_dual_link = > compute_is_dual_link_lvds(lvds_encoder); > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c > index 41d00b1603e3..b443278e569c 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct > intel_panel *panel) > > int intel_panel_init(struct intel_panel *panel, > struct drm_display_mode *fixed_mode, > - struct drm_display_mode *alt_fixed_mode, > struct drm_display_mode *downclock_mode) > { > intel_panel_init_backlight_funcs(panel); > > panel->fixed_mode = fixed_mode; > - panel->alt_fixed_mode = alt_fixed_mode; > panel->downclock_mode = downclock_mode; > > return 0; > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel > *panel) > if (panel->fixed_mode) > drm_mode_destroy(intel_connector->base.dev, panel- > >fixed_mode); > > - if (panel->alt_fixed_mode) > - drm_mode_destroy(intel_connector->base.dev, > - panel->alt_fixed_mode); > - > if (panel->downclock_mode) > drm_mode_destroy(intel_connector->base.dev, > panel->downclock_mode);
On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: >> This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. >> >> Per the report, no matter what display mode you select with xrandr, >> the >> i915 driver will always select the alternate fixed mode. For the >> reporter this means that the display will always run at 40Hz which is >> quite annoying. This may be due to the mode comparison. >> >> But there are some other potential issues. The choice of >> alt_fixed_mode >> seems dubious. It's the first non-preferred mode, but there are no >> guarantees that the only difference would be refresh rate. Similarly, >> there may be more than one preferred mode in the probed modes list, >> and >> the commit changes the preferred mode selection to choose the last >> one >> on the list instead of the first. >> >> (Note that the probed modes list is the raw, unfiltered, unsorted >> list >> of modes from drm_add_edid_modes(), not the pretty result after a >> drm_helper_probe_single_connector_modes() call.) >> >> Finally, we already have eerily similar code in place to find the >> downclock mode for DRRS that seems like could be reused here. >> >> Back to the drawing board. >> >> Note: This is a hand-crafted revert due to conflicts. If it fails to >> backport, please just try reverting the original commit directly. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 >> Reported-by: Rune Petersen <rune@megahurts.dk> >> Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> >> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for >> eDP if available.") >> Cc: Clint Taylor <clinton.a.taylor@intel.com> >> Cc: David Weinehall <david.weinehall@linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jim Bride <jim.bride@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Cc: <stable@vger.kernel.org> # v4.14+ >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------------- >> ---------- >> drivers/gpu/drm/i915/intel_drv.h | 2 -- >> drivers/gpu/drm/i915/intel_dsi.c | 2 +- >> drivers/gpu/drm/i915/intel_dvo.c | 2 +- >> drivers/gpu/drm/i915/intel_lvds.c | 3 +-- >> drivers/gpu/drm/i915/intel_panel.c | 6 ------ >> 6 files changed, 8 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c >> index dde92e4af5d3..8320f0e8e3be 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct >> intel_dp *intel_dp, >> return bpp; >> } >> >> -static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, >> - struct drm_display_mode *m2) >> -{ >> - bool bres = false; >> - >> - if (m1 && m2) >> - bres = (m1->hdisplay == m2->hdisplay && >> - m1->hsync_start == m2->hsync_start && >> - m1->hsync_end == m2->hsync_end && >> - m1->htotal == m2->htotal && >> - m1->vdisplay == m2->vdisplay && >> - m1->vsync_start == m2->vsync_start && >> - m1->vsync_end == m2->vsync_end && >> - m1->vtotal == m2->vtotal); >> - return bres; >> -} >> - >> /* Adjust link config limits based on compliance test requests. */ >> static void >> intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, >> @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct intel_encoder >> *encoder, >> pipe_config->has_audio = intel_conn_state- >> >force_audio == HDMI_AUDIO_ON; >> >> if (intel_dp_is_edp(intel_dp) && intel_connector- >> >panel.fixed_mode) { >> - struct drm_display_mode *panel_mode = >> - intel_connector->panel.alt_fixed_mode; >> - struct drm_display_mode *req_mode = &pipe_config- >> >base.mode; >> - >> - if (!intel_edp_compare_alt_mode(req_mode, >> panel_mode)) >> - panel_mode = intel_connector- >> >panel.fixed_mode; >> - >> - drm_mode_debug_printmodeline(panel_mode); >> - >> - intel_fixed_panel_mode(panel_mode, adjusted_mode); >> + intel_fixed_panel_mode(intel_connector- >> >panel.fixed_mode, >> + adjusted_mode); >> >> if (INTEL_GEN(dev_priv) >= 9) { >> int ret; >> @@ -6159,7 +6134,6 @@ static bool intel_edp_init_connector(struct >> intel_dp *intel_dp, >> struct drm_i915_private *dev_priv = to_i915(dev); >> struct drm_connector *connector = &intel_connector->base; >> struct drm_display_mode *fixed_mode = NULL; >> - struct drm_display_mode *alt_fixed_mode = NULL; >> struct drm_display_mode *downclock_mode = NULL; >> bool has_dpcd; >> struct drm_display_mode *scan; >> @@ -6214,14 +6188,13 @@ static bool intel_edp_init_connector(struct >> intel_dp *intel_dp, >> } >> intel_connector->edid = edid; >> >> - /* prefer fixed mode from EDID if available, save an alt >> mode also */ >> + /* prefer fixed mode from EDID if available */ >> list_for_each_entry(scan, &connector->probed_modes, head) { >> if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { >> fixed_mode = drm_mode_duplicate(dev, scan); >> downclock_mode = intel_dp_drrs_init( >> intel_connector, >> fixed_mode); >> - } else if (!alt_fixed_mode) { >> - alt_fixed_mode = drm_mode_duplicate(dev, >> scan); >> + break; > > If multiple preferred modes are present, we'll now end up calling > drrs_init() only for the first. I see that this is what the original > code did but this revert does more than removing support for alternate > modes. It boils down to which preferred mode is *the* preferred mode. I think the original code was, uh, preferrable. Note that drrs init scans the entire list of modes again to find the same size mode with the lowest refresh rate. BR, Jani. > >> } >> } >> >> @@ -6258,8 +6231,7 @@ static bool intel_edp_init_connector(struct >> intel_dp *intel_dp, >> pipe_name(pipe)); >> } >> >> - intel_panel_init(&intel_connector->panel, fixed_mode, >> alt_fixed_mode, >> - downclock_mode); >> + intel_panel_init(&intel_connector->panel, fixed_mode, >> downclock_mode); >> intel_connector->panel.backlight.power = >> intel_edp_backlight_power; >> intel_panel_setup_backlight(connector, pipe); >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index d7dbca1aabff..0361130500a6 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -277,7 +277,6 @@ struct intel_encoder { >> >> struct intel_panel { >> struct drm_display_mode *fixed_mode; >> - struct drm_display_mode *alt_fixed_mode; >> struct drm_display_mode *downclock_mode; >> >> /* backlight */ >> @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct >> drm_i915_private *dev_priv); >> /* intel_panel.c */ >> int intel_panel_init(struct intel_panel *panel, >> struct drm_display_mode *fixed_mode, >> - struct drm_display_mode *alt_fixed_mode, >> struct drm_display_mode *downclock_mode); >> void intel_panel_fini(struct intel_panel *panel); >> void intel_fixed_panel_mode(const struct drm_display_mode >> *fixed_mode, >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c >> b/drivers/gpu/drm/i915/intel_dsi.c >> index 51a1d6868b1e..cf39ca90d887 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct drm_i915_private >> *dev_priv) >> connector->display_info.width_mm = fixed_mode->width_mm; >> connector->display_info.height_mm = fixed_mode->height_mm; >> >> - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, >> NULL); >> + intel_panel_init(&intel_connector->panel, fixed_mode, NULL); >> intel_panel_setup_backlight(connector, INVALID_PIPE); >> >> intel_dsi_add_properties(intel_connector); >> diff --git a/drivers/gpu/drm/i915/intel_dvo.c >> b/drivers/gpu/drm/i915/intel_dvo.c >> index eb0c559b2715..a70d767313aa 100644 >> --- a/drivers/gpu/drm/i915/intel_dvo.c >> +++ b/drivers/gpu/drm/i915/intel_dvo.c >> @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private >> *dev_priv) >> */ >> intel_panel_init(&intel_connector->panel, >> intel_dvo_get_current_mode( >> intel_encoder), >> - NULL, NULL); >> + NULL); >> intel_dvo->panel_wants_dither = true; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_lvds.c >> b/drivers/gpu/drm/i915/intel_lvds.c >> index 8691c86f579c..d8ece907ff54 100644 >> --- a/drivers/gpu/drm/i915/intel_lvds.c >> +++ b/drivers/gpu/drm/i915/intel_lvds.c >> @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct drm_i915_private >> *dev_priv) >> out: >> mutex_unlock(&dev->mode_config.mutex); >> >> - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, >> - downclock_mode); >> + intel_panel_init(&intel_connector->panel, fixed_mode, >> downclock_mode); >> intel_panel_setup_backlight(connector, INVALID_PIPE); >> >> lvds_encoder->is_dual_link = >> compute_is_dual_link_lvds(lvds_encoder); >> diff --git a/drivers/gpu/drm/i915/intel_panel.c >> b/drivers/gpu/drm/i915/intel_panel.c >> index 41d00b1603e3..b443278e569c 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct >> intel_panel *panel) >> >> int intel_panel_init(struct intel_panel *panel, >> struct drm_display_mode *fixed_mode, >> - struct drm_display_mode *alt_fixed_mode, >> struct drm_display_mode *downclock_mode) >> { >> intel_panel_init_backlight_funcs(panel); >> >> panel->fixed_mode = fixed_mode; >> - panel->alt_fixed_mode = alt_fixed_mode; >> panel->downclock_mode = downclock_mode; >> >> return 0; >> @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel >> *panel) >> if (panel->fixed_mode) >> drm_mode_destroy(intel_connector->base.dev, panel- >> >fixed_mode); >> >> - if (panel->alt_fixed_mode) >> - drm_mode_destroy(intel_connector->base.dev, >> - panel->alt_fixed_mode); >> - >> if (panel->downclock_mode) >> drm_mode_destroy(intel_connector->base.dev, >> panel->downclock_mode);
On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: >> On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: >>> This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. >>> >>> Per the report, no matter what display mode you select with xrandr, >>> the >>> i915 driver will always select the alternate fixed mode. For the >>> reporter this means that the display will always run at 40Hz which is >>> quite annoying. This may be due to the mode comparison. >>> >>> But there are some other potential issues. The choice of >>> alt_fixed_mode >>> seems dubious. It's the first non-preferred mode, but there are no >>> guarantees that the only difference would be refresh rate. Similarly, >>> there may be more than one preferred mode in the probed modes list, >>> and >>> the commit changes the preferred mode selection to choose the last >>> one >>> on the list instead of the first. >>> >>> (Note that the probed modes list is the raw, unfiltered, unsorted >>> list >>> of modes from drm_add_edid_modes(), not the pretty result after a >>> drm_helper_probe_single_connector_modes() call.) >>> >>> Finally, we already have eerily similar code in place to find the >>> downclock mode for DRRS that seems like could be reused here. >>> >>> Back to the drawing board. >>> >>> Note: This is a hand-crafted revert due to conflicts. If it fails to >>> backport, please just try reverting the original commit directly. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 >>> Reported-by: Rune Petersen <rune@megahurts.dk> >>> Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> >>> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for >>> eDP if available.") >>> Cc: Clint Taylor <clinton.a.taylor@intel.com> >>> Cc: David Weinehall <david.weinehall@linux.intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Jim Bride <jim.bride@linux.intel.com> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: intel-gfx@lists.freedesktop.org >>> Cc: <stable@vger.kernel.org> # v4.14+ >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------------- >>> ---------- >>> drivers/gpu/drm/i915/intel_drv.h | 2 -- >>> drivers/gpu/drm/i915/intel_dsi.c | 2 +- >>> drivers/gpu/drm/i915/intel_dvo.c | 2 +- >>> drivers/gpu/drm/i915/intel_lvds.c | 3 +-- >>> drivers/gpu/drm/i915/intel_panel.c | 6 ------ >>> 6 files changed, 8 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index dde92e4af5d3..8320f0e8e3be 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct >>> intel_dp *intel_dp, >>> return bpp; >>> } >>> >>> -static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, >>> - struct drm_display_mode *m2) >>> -{ >>> - bool bres = false; >>> - >>> - if (m1 && m2) >>> - bres = (m1->hdisplay == m2->hdisplay && >>> - m1->hsync_start == m2->hsync_start && >>> - m1->hsync_end == m2->hsync_end && >>> - m1->htotal == m2->htotal && >>> - m1->vdisplay == m2->vdisplay && >>> - m1->vsync_start == m2->vsync_start && >>> - m1->vsync_end == m2->vsync_end && >>> - m1->vtotal == m2->vtotal); >>> - return bres; >>> -} >>> - >>> /* Adjust link config limits based on compliance test requests. */ >>> static void >>> intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, >>> @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct intel_encoder >>> *encoder, >>> pipe_config->has_audio = intel_conn_state- >>> >force_audio == HDMI_AUDIO_ON; >>> >>> if (intel_dp_is_edp(intel_dp) && intel_connector- >>> >panel.fixed_mode) { >>> - struct drm_display_mode *panel_mode = >>> - intel_connector->panel.alt_fixed_mode; >>> - struct drm_display_mode *req_mode = &pipe_config- >>> >base.mode; >>> - >>> - if (!intel_edp_compare_alt_mode(req_mode, >>> panel_mode)) >>> - panel_mode = intel_connector- >>> >panel.fixed_mode; >>> - >>> - drm_mode_debug_printmodeline(panel_mode); >>> - >>> - intel_fixed_panel_mode(panel_mode, adjusted_mode); >>> + intel_fixed_panel_mode(intel_connector- >>> >panel.fixed_mode, >>> + adjusted_mode); >>> >>> if (INTEL_GEN(dev_priv) >= 9) { >>> int ret; >>> @@ -6159,7 +6134,6 @@ static bool intel_edp_init_connector(struct >>> intel_dp *intel_dp, >>> struct drm_i915_private *dev_priv = to_i915(dev); >>> struct drm_connector *connector = &intel_connector->base; >>> struct drm_display_mode *fixed_mode = NULL; >>> - struct drm_display_mode *alt_fixed_mode = NULL; >>> struct drm_display_mode *downclock_mode = NULL; >>> bool has_dpcd; >>> struct drm_display_mode *scan; >>> @@ -6214,14 +6188,13 @@ static bool intel_edp_init_connector(struct >>> intel_dp *intel_dp, >>> } >>> intel_connector->edid = edid; >>> >>> - /* prefer fixed mode from EDID if available, save an alt >>> mode also */ >>> + /* prefer fixed mode from EDID if available */ >>> list_for_each_entry(scan, &connector->probed_modes, head) { >>> if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { >>> fixed_mode = drm_mode_duplicate(dev, scan); >>> downclock_mode = intel_dp_drrs_init( >>> intel_connector, >>> fixed_mode); >>> - } else if (!alt_fixed_mode) { >>> - alt_fixed_mode = drm_mode_duplicate(dev, >>> scan); >>> + break; >> >> If multiple preferred modes are present, we'll now end up calling >> drrs_init() only for the first. I see that this is what the original >> code did but this revert does more than removing support for alternate >> modes. > > It boils down to which preferred mode is *the* preferred mode. I think > the original code was, uh, preferrable. Note that drrs init scans the > entire list of modes again to find the same size mode with the lowest > refresh rate. Moreover, as you can see, the original alt mode commit had more subtle changes than catches the eye, it caused regressions, and I feel pretty strongly about getting back to the drawing board and starting over with a clean slate than trying to tweak it when we are quite frankly way overdue with the revert. If after that you think the drrs/downclock selection needs tweaking, let's do that. BR, Jani. > > BR, > Jani. > >> >>> } >>> } >>> >>> @@ -6258,8 +6231,7 @@ static bool intel_edp_init_connector(struct >>> intel_dp *intel_dp, >>> pipe_name(pipe)); >>> } >>> >>> - intel_panel_init(&intel_connector->panel, fixed_mode, >>> alt_fixed_mode, >>> - downclock_mode); >>> + intel_panel_init(&intel_connector->panel, fixed_mode, >>> downclock_mode); >>> intel_connector->panel.backlight.power = >>> intel_edp_backlight_power; >>> intel_panel_setup_backlight(connector, pipe); >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>> b/drivers/gpu/drm/i915/intel_drv.h >>> index d7dbca1aabff..0361130500a6 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -277,7 +277,6 @@ struct intel_encoder { >>> >>> struct intel_panel { >>> struct drm_display_mode *fixed_mode; >>> - struct drm_display_mode *alt_fixed_mode; >>> struct drm_display_mode *downclock_mode; >>> >>> /* backlight */ >>> @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct >>> drm_i915_private *dev_priv); >>> /* intel_panel.c */ >>> int intel_panel_init(struct intel_panel *panel, >>> struct drm_display_mode *fixed_mode, >>> - struct drm_display_mode *alt_fixed_mode, >>> struct drm_display_mode *downclock_mode); >>> void intel_panel_fini(struct intel_panel *panel); >>> void intel_fixed_panel_mode(const struct drm_display_mode >>> *fixed_mode, >>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c >>> b/drivers/gpu/drm/i915/intel_dsi.c >>> index 51a1d6868b1e..cf39ca90d887 100644 >>> --- a/drivers/gpu/drm/i915/intel_dsi.c >>> +++ b/drivers/gpu/drm/i915/intel_dsi.c >>> @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct drm_i915_private >>> *dev_priv) >>> connector->display_info.width_mm = fixed_mode->width_mm; >>> connector->display_info.height_mm = fixed_mode->height_mm; >>> >>> - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, >>> NULL); >>> + intel_panel_init(&intel_connector->panel, fixed_mode, NULL); >>> intel_panel_setup_backlight(connector, INVALID_PIPE); >>> >>> intel_dsi_add_properties(intel_connector); >>> diff --git a/drivers/gpu/drm/i915/intel_dvo.c >>> b/drivers/gpu/drm/i915/intel_dvo.c >>> index eb0c559b2715..a70d767313aa 100644 >>> --- a/drivers/gpu/drm/i915/intel_dvo.c >>> +++ b/drivers/gpu/drm/i915/intel_dvo.c >>> @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private >>> *dev_priv) >>> */ >>> intel_panel_init(&intel_connector->panel, >>> intel_dvo_get_current_mode( >>> intel_encoder), >>> - NULL, NULL); >>> + NULL); >>> intel_dvo->panel_wants_dither = true; >>> } >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lvds.c >>> b/drivers/gpu/drm/i915/intel_lvds.c >>> index 8691c86f579c..d8ece907ff54 100644 >>> --- a/drivers/gpu/drm/i915/intel_lvds.c >>> +++ b/drivers/gpu/drm/i915/intel_lvds.c >>> @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct drm_i915_private >>> *dev_priv) >>> out: >>> mutex_unlock(&dev->mode_config.mutex); >>> >>> - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, >>> - downclock_mode); >>> + intel_panel_init(&intel_connector->panel, fixed_mode, >>> downclock_mode); >>> intel_panel_setup_backlight(connector, INVALID_PIPE); >>> >>> lvds_encoder->is_dual_link = >>> compute_is_dual_link_lvds(lvds_encoder); >>> diff --git a/drivers/gpu/drm/i915/intel_panel.c >>> b/drivers/gpu/drm/i915/intel_panel.c >>> index 41d00b1603e3..b443278e569c 100644 >>> --- a/drivers/gpu/drm/i915/intel_panel.c >>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>> @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct >>> intel_panel *panel) >>> >>> int intel_panel_init(struct intel_panel *panel, >>> struct drm_display_mode *fixed_mode, >>> - struct drm_display_mode *alt_fixed_mode, >>> struct drm_display_mode *downclock_mode) >>> { >>> intel_panel_init_backlight_funcs(panel); >>> >>> panel->fixed_mode = fixed_mode; >>> - panel->alt_fixed_mode = alt_fixed_mode; >>> panel->downclock_mode = downclock_mode; >>> >>> return 0; >>> @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel >>> *panel) >>> if (panel->fixed_mode) >>> drm_mode_destroy(intel_connector->base.dev, panel- >>> >fixed_mode); >>> >>> - if (panel->alt_fixed_mode) >>> - drm_mode_destroy(intel_connector->base.dev, >>> - panel->alt_fixed_mode); >>> - >>> if (panel->downclock_mode) >>> drm_mode_destroy(intel_connector->base.dev, >>> panel->downclock_mode);
On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote: > On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > > > > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel > > .com> wrote: > > > > > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: > > > > > > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. > > > > > > > > Per the report, no matter what display mode you select with > > > > xrandr, > > > > the > > > > i915 driver will always select the alternate fixed mode. For > > > > the > > > > reporter this means that the display will always run at 40Hz > > > > which is > > > > quite annoying. This may be due to the mode comparison. > > > > > > > > But there are some other potential issues. The choice of > > > > alt_fixed_mode > > > > seems dubious. It's the first non-preferred mode, but there are > > > > no > > > > guarantees that the only difference would be refresh rate. > > > > Similarly, > > > > there may be more than one preferred mode in the probed modes > > > > list, > > > > and > > > > the commit changes the preferred mode selection to choose the > > > > last > > > > one > > > > on the list instead of the first. > > > > > > > > (Note that the probed modes list is the raw, unfiltered, > > > > unsorted > > > > list > > > > of modes from drm_add_edid_modes(), not the pretty result after > > > > a > > > > drm_helper_probe_single_connector_modes() call.) > > > > > > > > Finally, we already have eerily similar code in place to find > > > > the > > > > downclock mode for DRRS that seems like could be reused here. > > > > > > > > Back to the drawing board. > > > > > > > > Note: This is a hand-crafted revert due to conflicts. If it > > > > fails to > > > > backport, please just try reverting the original commit > > > > directly. > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > > > > Reported-by: Rune Petersen <rune@megahurts.dk> > > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> > > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode > > > > for > > > > eDP if available.") > > > > Cc: Clint Taylor <clinton.a.taylor@intel.com> > > > > Cc: David Weinehall <david.weinehall@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Jim Bride <jim.bride@linux.intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > Cc: intel-gfx@lists.freedesktop.org > > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------- > > > > ------ > > > > ---------- > > > > drivers/gpu/drm/i915/intel_drv.h | 2 -- > > > > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > > > > drivers/gpu/drm/i915/intel_dvo.c | 2 +- > > > > drivers/gpu/drm/i915/intel_lvds.c | 3 +-- > > > > drivers/gpu/drm/i915/intel_panel.c | 6 ------ > > > > 6 files changed, 8 insertions(+), 45 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index dde92e4af5d3..8320f0e8e3be 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct > > > > intel_dp *intel_dp, > > > > return bpp; > > > > } > > > > > > > > -static bool intel_edp_compare_alt_mode(struct drm_display_mode > > > > *m1, > > > > - struct drm_display_mode > > > > *m2) > > > > -{ > > > > - bool bres = false; > > > > - > > > > - if (m1 && m2) > > > > - bres = (m1->hdisplay == m2->hdisplay && > > > > - m1->hsync_start == m2->hsync_start && > > > > - m1->hsync_end == m2->hsync_end && > > > > - m1->htotal == m2->htotal && > > > > - m1->vdisplay == m2->vdisplay && > > > > - m1->vsync_start == m2->vsync_start && > > > > - m1->vsync_end == m2->vsync_end && > > > > - m1->vtotal == m2->vtotal); > > > > - return bres; > > > > -} > > > > - > > > > /* Adjust link config limits based on compliance test > > > > requests. */ > > > > static void > > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, > > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct > > > > intel_encoder > > > > *encoder, > > > > pipe_config->has_audio = intel_conn_state- > > > > > > > > > > force_audio == HDMI_AUDIO_ON; > > > > > > > > if (intel_dp_is_edp(intel_dp) && intel_connector- > > > > > > > > > > panel.fixed_mode) { > > > > - struct drm_display_mode *panel_mode = > > > > - intel_connector->panel.alt_fixed_mode; > > > > - struct drm_display_mode *req_mode = > > > > &pipe_config- > > > > > > > > > > base.mode; > > > > - > > > > - if (!intel_edp_compare_alt_mode(req_mode, > > > > panel_mode)) > > > > - panel_mode = intel_connector- > > > > > > > > > > panel.fixed_mode; > > > > - > > > > - drm_mode_debug_printmodeline(panel_mode); > > > > - > > > > - intel_fixed_panel_mode(panel_mode, > > > > adjusted_mode); > > > > + intel_fixed_panel_mode(intel_connector- > > > > > > > > > > panel.fixed_mode, > > > > + adjusted_mode); > > > > > > > > if (INTEL_GEN(dev_priv) >= 9) { > > > > int ret; > > > > @@ -6159,7 +6134,6 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > struct drm_connector *connector = &intel_connector- > > > > >base; > > > > struct drm_display_mode *fixed_mode = NULL; > > > > - struct drm_display_mode *alt_fixed_mode = NULL; > > > > struct drm_display_mode *downclock_mode = NULL; > > > > bool has_dpcd; > > > > struct drm_display_mode *scan; > > > > @@ -6214,14 +6188,13 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > } > > > > intel_connector->edid = edid; > > > > > > > > - /* prefer fixed mode from EDID if available, save an > > > > alt > > > > mode also */ > > > > + /* prefer fixed mode from EDID if available */ > > > > list_for_each_entry(scan, &connector->probed_modes, > > > > head) { > > > > if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { > > > > fixed_mode = drm_mode_duplicate(dev, > > > > scan); > > > > downclock_mode = intel_dp_drrs_init( > > > > intel_connecto > > > > r, > > > > fixed_mode); > > > > - } else if (!alt_fixed_mode) { > > > > - alt_fixed_mode = > > > > drm_mode_duplicate(dev, > > > > scan); > > > > + break; > > > If multiple preferred modes are present, we'll now end up calling > > > drrs_init() only for the first. I see that this is what the > > > original > > > code did but this revert does more than removing support for > > > alternate > > > modes. > > It boils down to which preferred mode is *the* preferred mode. I > > think > > the original code was, uh, preferrable. Note that drrs init scans > > the > > entire list of modes again to find the same size mode with the > > lowest > > refresh rate. > Moreover, as you can see, the original alt mode commit had more > subtle > changes than catches the eye, it caused regressions, and I feel > pretty > strongly about getting back to the drawing board and starting over > with > a clean slate than trying to tweak it when we are quite frankly way > overdue with the revert. If after that you think the drrs/downclock > selection needs tweaking, let's do that. Yeah, agreed on starting with a clean slate. The offending commit claims alternate mode was intended to be used for PSR testing. I don't see any psr_basic failures in BAT or any other PSR sub-tests failing on shards, must have been a non-CI machine then. The revert itself looks correct, Reviewed-by: Dhinaakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > BR, > Jani. > > > > > > > > BR, > > Jani. > > > > > > > > > > > > > > > > } > > > > } > > > > > > > > @@ -6258,8 +6231,7 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > pipe_name(pipe)); > > > > } > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > alt_fixed_mode, > > > > - downclock_mode); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > downclock_mode); > > > > intel_connector->panel.backlight.power = > > > > intel_edp_backlight_power; > > > > intel_panel_setup_backlight(connector, pipe); > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index d7dbca1aabff..0361130500a6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -277,7 +277,6 @@ struct intel_encoder { > > > > > > > > struct intel_panel { > > > > struct drm_display_mode *fixed_mode; > > > > - struct drm_display_mode *alt_fixed_mode; > > > > struct drm_display_mode *downclock_mode; > > > > > > > > /* backlight */ > > > > @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct > > > > drm_i915_private *dev_priv); > > > > /* intel_panel.c */ > > > > int intel_panel_init(struct intel_panel *panel, > > > > struct drm_display_mode *fixed_mode, > > > > - struct drm_display_mode *alt_fixed_mode, > > > > struct drm_display_mode *downclock_mode); > > > > void intel_panel_fini(struct intel_panel *panel); > > > > void intel_fixed_panel_mode(const struct drm_display_mode > > > > *fixed_mode, > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > > > > b/drivers/gpu/drm/i915/intel_dsi.c > > > > index 51a1d6868b1e..cf39ca90d887 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > > > @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct > > > > drm_i915_private > > > > *dev_priv) > > > > connector->display_info.width_mm = fixed_mode- > > > > >width_mm; > > > > connector->display_info.height_mm = fixed_mode- > > > > >height_mm; > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL, > > > > NULL); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL); > > > > intel_panel_setup_backlight(connector, INVALID_PIPE); > > > > > > > > intel_dsi_add_properties(intel_connector); > > > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c > > > > b/drivers/gpu/drm/i915/intel_dvo.c > > > > index eb0c559b2715..a70d767313aa 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dvo.c > > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c > > > > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private > > > > *dev_priv) > > > > */ > > > > intel_panel_init(&intel_connector- > > > > >panel, > > > > intel_dvo_get_current > > > > _mode( > > > > intel_encoder), > > > > - NULL, NULL); > > > > + NULL); > > > > intel_dvo->panel_wants_dither = true; > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > > > > b/drivers/gpu/drm/i915/intel_lvds.c > > > > index 8691c86f579c..d8ece907ff54 100644 > > > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > > > @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct > > > > drm_i915_private > > > > *dev_priv) > > > > out: > > > > mutex_unlock(&dev->mode_config.mutex); > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL, > > > > - downclock_mode); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > downclock_mode); > > > > intel_panel_setup_backlight(connector, INVALID_PIPE); > > > > > > > > lvds_encoder->is_dual_link = > > > > compute_is_dual_link_lvds(lvds_encoder); > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > index 41d00b1603e3..b443278e569c 100644 > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct > > > > intel_panel *panel) > > > > > > > > int intel_panel_init(struct intel_panel *panel, > > > > struct drm_display_mode *fixed_mode, > > > > - struct drm_display_mode *alt_fixed_mode, > > > > struct drm_display_mode *downclock_mode) > > > > { > > > > intel_panel_init_backlight_funcs(panel); > > > > > > > > panel->fixed_mode = fixed_mode; > > > > - panel->alt_fixed_mode = alt_fixed_mode; > > > > panel->downclock_mode = downclock_mode; > > > > > > > > return 0; > > > > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel > > > > *panel) > > > > if (panel->fixed_mode) > > > > drm_mode_destroy(intel_connector->base.dev, > > > > panel- > > > > > > > > > > fixed_mode); > > > > > > > > - if (panel->alt_fixed_mode) > > > > - drm_mode_destroy(intel_connector->base.dev, > > > > - panel->alt_fixed_mode); > > > > - > > > > if (panel->downclock_mode) > > > > drm_mode_destroy(intel_connector->base.dev, > > > > panel->downclock_mode);
On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote: > On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > > > > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel > > .com> wrote: > > > > > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: > > > > > > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. > > > > > > > > Per the report, no matter what display mode you select with > > > > xrandr, > > > > the > > > > i915 driver will always select the alternate fixed mode. For > > > > the > > > > reporter this means that the display will always run at 40Hz > > > > which is > > > > quite annoying. This may be due to the mode comparison. > > > > > > > > But there are some other potential issues. The choice of > > > > alt_fixed_mode > > > > seems dubious. It's the first non-preferred mode, but there are > > > > no > > > > guarantees that the only difference would be refresh rate. > > > > Similarly, > > > > there may be more than one preferred mode in the probed modes > > > > list, > > > > and > > > > the commit changes the preferred mode selection to choose the > > > > last > > > > one > > > > on the list instead of the first. > > > > > > > > (Note that the probed modes list is the raw, unfiltered, > > > > unsorted > > > > list > > > > of modes from drm_add_edid_modes(), not the pretty result after > > > > a > > > > drm_helper_probe_single_connector_modes() call.) > > > > > > > > Finally, we already have eerily similar code in place to find > > > > the > > > > downclock mode for DRRS that seems like could be reused here. > > > > > > > > Back to the drawing board. > > > > > > > > Note: This is a hand-crafted revert due to conflicts. If it > > > > fails to > > > > backport, please just try reverting the original commit > > > > directly. > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > > > > Reported-by: Rune Petersen <rune@megahurts.dk> > > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> > > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode > > > > for > > > > eDP if available.") > > > > Cc: Clint Taylor <clinton.a.taylor@intel.com> > > > > Cc: David Weinehall <david.weinehall@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Jim Bride <jim.bride@linux.intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > Cc: intel-gfx@lists.freedesktop.org > > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------- > > > > ------ > > > > ---------- > > > > drivers/gpu/drm/i915/intel_drv.h | 2 -- > > > > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > > > > drivers/gpu/drm/i915/intel_dvo.c | 2 +- > > > > drivers/gpu/drm/i915/intel_lvds.c | 3 +-- > > > > drivers/gpu/drm/i915/intel_panel.c | 6 ------ > > > > 6 files changed, 8 insertions(+), 45 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index dde92e4af5d3..8320f0e8e3be 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct > > > > intel_dp *intel_dp, > > > > return bpp; > > > > } > > > > > > > > -static bool intel_edp_compare_alt_mode(struct drm_display_mode > > > > *m1, > > > > - struct drm_display_mode > > > > *m2) > > > > -{ > > > > - bool bres = false; > > > > - > > > > - if (m1 && m2) > > > > - bres = (m1->hdisplay == m2->hdisplay && > > > > - m1->hsync_start == m2->hsync_start && > > > > - m1->hsync_end == m2->hsync_end && > > > > - m1->htotal == m2->htotal && > > > > - m1->vdisplay == m2->vdisplay && > > > > - m1->vsync_start == m2->vsync_start && > > > > - m1->vsync_end == m2->vsync_end && > > > > - m1->vtotal == m2->vtotal); > > > > - return bres; > > > > -} > > > > - > > > > /* Adjust link config limits based on compliance test > > > > requests. */ > > > > static void > > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, > > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct > > > > intel_encoder > > > > *encoder, > > > > pipe_config->has_audio = intel_conn_state- > > > > > > > > > > force_audio == HDMI_AUDIO_ON; > > > > > > > > if (intel_dp_is_edp(intel_dp) && intel_connector- > > > > > > > > > > panel.fixed_mode) { > > > > - struct drm_display_mode *panel_mode = > > > > - intel_connector->panel.alt_fixed_mode; > > > > - struct drm_display_mode *req_mode = > > > > &pipe_config- > > > > > > > > > > base.mode; > > > > - > > > > - if (!intel_edp_compare_alt_mode(req_mode, > > > > panel_mode)) > > > > - panel_mode = intel_connector- > > > > > > > > > > panel.fixed_mode; > > > > - > > > > - drm_mode_debug_printmodeline(panel_mode); > > > > - > > > > - intel_fixed_panel_mode(panel_mode, > > > > adjusted_mode); > > > > + intel_fixed_panel_mode(intel_connector- > > > > > > > > > > panel.fixed_mode, > > > > + adjusted_mode); > > > > > > > > if (INTEL_GEN(dev_priv) >= 9) { > > > > int ret; > > > > @@ -6159,7 +6134,6 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > struct drm_connector *connector = &intel_connector- > > > > >base; > > > > struct drm_display_mode *fixed_mode = NULL; > > > > - struct drm_display_mode *alt_fixed_mode = NULL; > > > > struct drm_display_mode *downclock_mode = NULL; > > > > bool has_dpcd; > > > > struct drm_display_mode *scan; > > > > @@ -6214,14 +6188,13 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > } > > > > intel_connector->edid = edid; > > > > > > > > - /* prefer fixed mode from EDID if available, save an > > > > alt > > > > mode also */ > > > > + /* prefer fixed mode from EDID if available */ > > > > list_for_each_entry(scan, &connector->probed_modes, > > > > head) { > > > > if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { > > > > fixed_mode = drm_mode_duplicate(dev, > > > > scan); > > > > downclock_mode = intel_dp_drrs_init( > > > > intel_connecto > > > > r, > > > > fixed_mode); > > > > - } else if (!alt_fixed_mode) { > > > > - alt_fixed_mode = > > > > drm_mode_duplicate(dev, > > > > scan); > > > > + break; > > > If multiple preferred modes are present, we'll now end up calling > > > drrs_init() only for the first. I see that this is what the > > > original > > > code did but this revert does more than removing support for > > > alternate > > > modes. > > It boils down to which preferred mode is *the* preferred mode. I > > think > > the original code was, uh, preferrable. Note that drrs init scans > > the > > entire list of modes again to find the same size mode with the > > lowest > > refresh rate. > Moreover, as you can see, the original alt mode commit had more > subtle > changes than catches the eye, it caused regressions, and I feel > pretty > strongly about getting back to the drawing board and starting over > with > a clean slate than trying to tweak it when we are quite frankly way > overdue with the revert. If after that you think the drrs/downclock > selection needs tweaking, let's do that. Yeah, agreed on starting with a clean slate. The offending commit claims alternate mode was intended to be used for PSR testing. I don't see any psr_basic failures in BAT or any other PSR sub-tests failing on shards, must have been a non-CI machine then. The revert itself looks correct, Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > BR, > Jani. > > > > > > > > BR, > > Jani. > > > > > > > > > > > > > > > > } > > > > } > > > > > > > > @@ -6258,8 +6231,7 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > pipe_name(pipe)); > > > > } > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > alt_fixed_mode, > > > > - downclock_mode); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > downclock_mode); > > > > intel_connector->panel.backlight.power = > > > > intel_edp_backlight_power; > > > > intel_panel_setup_backlight(connector, pipe); > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index d7dbca1aabff..0361130500a6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -277,7 +277,6 @@ struct intel_encoder { > > > > > > > > struct intel_panel { > > > > struct drm_display_mode *fixed_mode; > > > > - struct drm_display_mode *alt_fixed_mode; > > > > struct drm_display_mode *downclock_mode; > > > > > > > > /* backlight */ > > > > @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct > > > > drm_i915_private *dev_priv); > > > > /* intel_panel.c */ > > > > int intel_panel_init(struct intel_panel *panel, > > > > struct drm_display_mode *fixed_mode, > > > > - struct drm_display_mode *alt_fixed_mode, > > > > struct drm_display_mode *downclock_mode); > > > > void intel_panel_fini(struct intel_panel *panel); > > > > void intel_fixed_panel_mode(const struct drm_display_mode > > > > *fixed_mode, > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > > > > b/drivers/gpu/drm/i915/intel_dsi.c > > > > index 51a1d6868b1e..cf39ca90d887 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > > > @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct > > > > drm_i915_private > > > > *dev_priv) > > > > connector->display_info.width_mm = fixed_mode- > > > > >width_mm; > > > > connector->display_info.height_mm = fixed_mode- > > > > >height_mm; > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL, > > > > NULL); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL); > > > > intel_panel_setup_backlight(connector, INVALID_PIPE); > > > > > > > > intel_dsi_add_properties(intel_connector); > > > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c > > > > b/drivers/gpu/drm/i915/intel_dvo.c > > > > index eb0c559b2715..a70d767313aa 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dvo.c > > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c > > > > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private > > > > *dev_priv) > > > > */ > > > > intel_panel_init(&intel_connector- > > > > >panel, > > > > intel_dvo_get_current > > > > _mode( > > > > intel_encoder), > > > > - NULL, NULL); > > > > + NULL); > > > > intel_dvo->panel_wants_dither = true; > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > > > > b/drivers/gpu/drm/i915/intel_lvds.c > > > > index 8691c86f579c..d8ece907ff54 100644 > > > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > > > @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct > > > > drm_i915_private > > > > *dev_priv) > > > > out: > > > > mutex_unlock(&dev->mode_config.mutex); > > > > > > > > - intel_panel_init(&intel_connector->panel, fixed_mode, > > > > NULL, > > > > - downclock_mode); > > > > + intel_panel_init(&intel_connector->panel, fixed_mode, > > > > downclock_mode); > > > > intel_panel_setup_backlight(connector, INVALID_PIPE); > > > > > > > > lvds_encoder->is_dual_link = > > > > compute_is_dual_link_lvds(lvds_encoder); > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > > > b/drivers/gpu/drm/i915/intel_panel.c > > > > index 41d00b1603e3..b443278e569c 100644 > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct > > > > intel_panel *panel) > > > > > > > > int intel_panel_init(struct intel_panel *panel, > > > > struct drm_display_mode *fixed_mode, > > > > - struct drm_display_mode *alt_fixed_mode, > > > > struct drm_display_mode *downclock_mode) > > > > { > > > > intel_panel_init_backlight_funcs(panel); > > > > > > > > panel->fixed_mode = fixed_mode; > > > > - panel->alt_fixed_mode = alt_fixed_mode; > > > > panel->downclock_mode = downclock_mode; > > > > > > > > return 0; > > > > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel > > > > *panel) > > > > if (panel->fixed_mode) > > > > drm_mode_destroy(intel_connector->base.dev, > > > > panel- > > > > > > > > > > fixed_mode); > > > > > > > > - if (panel->alt_fixed_mode) > > > > - drm_mode_destroy(intel_connector->base.dev, > > > > - panel->alt_fixed_mode); > > > > - > > > > if (panel->downclock_mode) > > > > drm_mode_destroy(intel_connector->base.dev, > > > > panel->downclock_mode);
On Thursday, May 17, 2018 12:44:30 PM PDT Dhinakaran Pandiyan wrote: > On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote: > > On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > > > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel > > > > > > .com> wrote: > > > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote: > > > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. > > > > > > > > > > Per the report, no matter what display mode you select with > > > > > xrandr, > > > > > the > > > > > i915 driver will always select the alternate fixed mode. For > > > > > the > > > > > reporter this means that the display will always run at 40Hz > > > > > which is > > > > > quite annoying. This may be due to the mode comparison. > > > > > > > > > > But there are some other potential issues. The choice of > > > > > alt_fixed_mode > > > > > seems dubious. It's the first non-preferred mode, but there are > > > > > no > > > > > guarantees that the only difference would be refresh rate. > > > > > Similarly, > > > > > there may be more than one preferred mode in the probed modes > > > > > list, > > > > > and > > > > > the commit changes the preferred mode selection to choose the > > > > > last > > > > > one > > > > > on the list instead of the first. > > > > > > > > > > (Note that the probed modes list is the raw, unfiltered, > > > > > unsorted > > > > > list > > > > > of modes from drm_add_edid_modes(), not the pretty result after > > > > > a > > > > > drm_helper_probe_single_connector_modes() call.) > > > > > > > > > > Finally, we already have eerily similar code in place to find > > > > > the > > > > > downclock mode for DRRS that seems like could be reused here. > > > > > > > > > > Back to the drawing board. > > > > > > > > > > Note: This is a hand-crafted revert due to conflicts. If it > > > > > fails to > > > > > backport, please just try reverting the original commit > > > > > directly. > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > > > > > Reported-by: Rune Petersen <rune@megahurts.dk> > > > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> > > > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode > > > > > for > > > > > eDP if available.") > > > > > Cc: Clint Taylor <clinton.a.taylor@intel.com> > > > > > Cc: David Weinehall <david.weinehall@linux.intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Cc: Jim Bride <jim.bride@linux.intel.com> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > > Cc: intel-gfx@lists.freedesktop.org > > > > > Cc: <stable@vger.kernel.org> # v4.14+ > > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++----------------- > > > > > ------ > > > > > ---------- > > > > > drivers/gpu/drm/i915/intel_drv.h | 2 -- > > > > > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > > > > > drivers/gpu/drm/i915/intel_dvo.c | 2 +- > > > > > drivers/gpu/drm/i915/intel_lvds.c | 3 +-- > > > > > drivers/gpu/drm/i915/intel_panel.c | 6 ------ > > > > > 6 files changed, 8 insertions(+), 45 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index dde92e4af5d3..8320f0e8e3be 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct > > > > > intel_dp *intel_dp, > > > > > return bpp; > > > > > } > > > > > > > > > > -static bool intel_edp_compare_alt_mode(struct drm_display_mode > > > > > *m1, > > > > > - struct drm_display_mode > > > > > *m2) > > > > > -{ > > > > > - bool bres = false; > > > > > - > > > > > - if (m1 && m2) > > > > > - bres = (m1->hdisplay == m2->hdisplay && > > > > > - m1->hsync_start == m2->hsync_start && > > > > > - m1->hsync_end == m2->hsync_end && > > > > > - m1->htotal == m2->htotal && > > > > > - m1->vdisplay == m2->vdisplay && > > > > > - m1->vsync_start == m2->vsync_start && > > > > > - m1->vsync_end == m2->vsync_end && > > > > > - m1->vtotal == m2->vtotal); > > > > > - return bres; > > > > > -} > > > > > - > > > > > /* Adjust link config limits based on compliance test > > > > > requests. */ > > > > > static void > > > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, > > > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct > > > > > intel_encoder > > > > > *encoder, > > > > > pipe_config->has_audio = intel_conn_state- > > > > > > > > > > > force_audio == HDMI_AUDIO_ON; > > > > > > > > > > > > > > > if (intel_dp_is_edp(intel_dp) && intel_connector- > > > > > > > > > > > panel.fixed_mode) { > > > > > > > > > > - struct drm_display_mode *panel_mode = > > > > > - intel_connector->panel.alt_fixed_mode; > > > > > - struct drm_display_mode *req_mode = > > > > > &pipe_config- > > > > > > > > > > > base.mode; > > > > > > > > > > - > > > > > - if (!intel_edp_compare_alt_mode(req_mode, > > > > > panel_mode)) > > > > > - panel_mode = intel_connector- > > > > > > > > > > > panel.fixed_mode; > > > > > > > > > > - > > > > > - drm_mode_debug_printmodeline(panel_mode); > > > > > - > > > > > - intel_fixed_panel_mode(panel_mode, > > > > > adjusted_mode); > > > > > + intel_fixed_panel_mode(intel_connector- > > > > > > > > > > > panel.fixed_mode, > > > > > > > > > > + adjusted_mode); > > > > > > > > > > if (INTEL_GEN(dev_priv) >= 9) { > > > > > int ret; > > > > > @@ -6159,7 +6134,6 @@ static bool > > > > > intel_edp_init_connector(struct > > > > > intel_dp *intel_dp, > > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > > struct drm_connector *connector = &intel_connector- > > > > > > > > > > >base; > > > > > > > > > > struct drm_display_mode *fixed_mode = NULL; > > > > > - struct drm_display_mode *alt_fixed_mode = NULL; > > > > > struct drm_display_mode *downclock_mode = NULL; > > > > > bool has_dpcd; > > > > > struct drm_display_mode *scan; > > > > > @@ -6214,14 +6188,13 @@ static bool > > > > > intel_edp_init_connector(struct > > > > > intel_dp *intel_dp, > > > > > } > > > > > intel_connector->edid = edid; > > > > > > > > > > - /* prefer fixed mode from EDID if available, save an > > > > > alt > > > > > mode also */ > > > > > + /* prefer fixed mode from EDID if available */ > > > > > list_for_each_entry(scan, &connector->probed_modes, > > > > > head) { > > > > > if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { > > > > > fixed_mode = drm_mode_duplicate(dev, > > > > > scan); > > > > > downclock_mode = intel_dp_drrs_init( > > > > > intel_connecto > > > > > r, > > > > > fixed_mode); > > > > > - } else if (!alt_fixed_mode) { > > > > > - alt_fixed_mode = > > > > > drm_mode_duplicate(dev, > > > > > scan); > > > > > + break; > > > > > > > > If multiple preferred modes are present, we'll now end up calling > > > > drrs_init() only for the first. I see that this is what the > > > > original > > > > code did but this revert does more than removing support for > > > > alternate > > > > modes. > > > > > > It boils down to which preferred mode is *the* preferred mode. I > > > think > > > the original code was, uh, preferrable. Note that drrs init scans > > > the > > > entire list of modes again to find the same size mode with the > > > lowest > > > refresh rate. > > > > Moreover, as you can see, the original alt mode commit had more > > subtle > > changes than catches the eye, it caused regressions, and I feel > > pretty > > strongly about getting back to the drawing board and starting over > > with > > a clean slate than trying to tweak it when we are quite frankly way > > overdue with the revert. If after that you think the drrs/downclock > > selection needs tweaking, let's do that. > > Yeah, agreed on starting with a clean slate. > > The offending commit claims alternate mode was intended to be used for > PSR testing. I don't see any psr_basic failures in BAT or any other PSR > sub-tests failing on shards, must have been a non-CI machine then. > > The revert itself looks correct, > Reviewed-by: Dhinaakaran Pandiyan <dhinakaran.pandiyan@intel.com> Hit Send too early and Cancel too late. The signature was misspelt, hope it doesn't confuse dim.
On Thu, 17 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Yeah, agreed on starting with a clean slate. > > The offending commit claims alternate mode was intended to be used for > PSR testing. I don't see any psr_basic failures in BAT or any other PSR > sub-tests failing on shards, must have been a non-CI machine then. > > The revert itself looks correct, > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Thanks for the review, pushed to dinq. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index dde92e4af5d3..8320f0e8e3be 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1679,23 +1679,6 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, return bpp; } -static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, - struct drm_display_mode *m2) -{ - bool bres = false; - - if (m1 && m2) - bres = (m1->hdisplay == m2->hdisplay && - m1->hsync_start == m2->hsync_start && - m1->hsync_end == m2->hsync_end && - m1->htotal == m2->htotal && - m1->vdisplay == m2->vdisplay && - m1->vsync_start == m2->vsync_start && - m1->vsync_end == m2->vsync_end && - m1->vtotal == m2->vtotal); - return bres; -} - /* Adjust link config limits based on compliance test requests. */ static void intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON; if (intel_dp_is_edp(intel_dp) && intel_connector->panel.fixed_mode) { - struct drm_display_mode *panel_mode = - intel_connector->panel.alt_fixed_mode; - struct drm_display_mode *req_mode = &pipe_config->base.mode; - - if (!intel_edp_compare_alt_mode(req_mode, panel_mode)) - panel_mode = intel_connector->panel.fixed_mode; - - drm_mode_debug_printmodeline(panel_mode); - - intel_fixed_panel_mode(panel_mode, adjusted_mode); + intel_fixed_panel_mode(intel_connector->panel.fixed_mode, + adjusted_mode); if (INTEL_GEN(dev_priv) >= 9) { int ret; @@ -6159,7 +6134,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, struct drm_i915_private *dev_priv = to_i915(dev); struct drm_connector *connector = &intel_connector->base; struct drm_display_mode *fixed_mode = NULL; - struct drm_display_mode *alt_fixed_mode = NULL; struct drm_display_mode *downclock_mode = NULL; bool has_dpcd; struct drm_display_mode *scan; @@ -6214,14 +6188,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, } intel_connector->edid = edid; - /* prefer fixed mode from EDID if available, save an alt mode also */ + /* prefer fixed mode from EDID if available */ list_for_each_entry(scan, &connector->probed_modes, head) { if ((scan->type & DRM_MODE_TYPE_PREFERRED)) { fixed_mode = drm_mode_duplicate(dev, scan); downclock_mode = intel_dp_drrs_init( intel_connector, fixed_mode); - } else if (!alt_fixed_mode) { - alt_fixed_mode = drm_mode_duplicate(dev, scan); + break; } } @@ -6258,8 +6231,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, pipe_name(pipe)); } - intel_panel_init(&intel_connector->panel, fixed_mode, alt_fixed_mode, - downclock_mode); + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode); intel_connector->panel.backlight.power = intel_edp_backlight_power; intel_panel_setup_backlight(connector, pipe); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d7dbca1aabff..0361130500a6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -277,7 +277,6 @@ struct intel_encoder { struct intel_panel { struct drm_display_mode *fixed_mode; - struct drm_display_mode *alt_fixed_mode; struct drm_display_mode *downclock_mode; /* backlight */ @@ -1850,7 +1849,6 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv); /* intel_panel.c */ int intel_panel_init(struct intel_panel *panel, struct drm_display_mode *fixed_mode, - struct drm_display_mode *alt_fixed_mode, struct drm_display_mode *downclock_mode); void intel_panel_fini(struct intel_panel *panel); void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 51a1d6868b1e..cf39ca90d887 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -1846,7 +1846,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv) connector->display_info.width_mm = fixed_mode->width_mm; connector->display_info.height_mm = fixed_mode->height_mm; - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, NULL); + intel_panel_init(&intel_connector->panel, fixed_mode, NULL); intel_panel_setup_backlight(connector, INVALID_PIPE); intel_dsi_add_properties(intel_connector); diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index eb0c559b2715..a70d767313aa 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv) */ intel_panel_init(&intel_connector->panel, intel_dvo_get_current_mode(intel_encoder), - NULL, NULL); + NULL); intel_dvo->panel_wants_dither = true; } diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 8691c86f579c..d8ece907ff54 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -1140,8 +1140,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) out: mutex_unlock(&dev->mode_config.mutex); - intel_panel_init(&intel_connector->panel, fixed_mode, NULL, - downclock_mode); + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode); intel_panel_setup_backlight(connector, INVALID_PIPE); lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder); diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 41d00b1603e3..b443278e569c 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1928,13 +1928,11 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) int intel_panel_init(struct intel_panel *panel, struct drm_display_mode *fixed_mode, - struct drm_display_mode *alt_fixed_mode, struct drm_display_mode *downclock_mode) { intel_panel_init_backlight_funcs(panel); panel->fixed_mode = fixed_mode; - panel->alt_fixed_mode = alt_fixed_mode; panel->downclock_mode = downclock_mode; return 0; @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel *panel) if (panel->fixed_mode) drm_mode_destroy(intel_connector->base.dev, panel->fixed_mode); - if (panel->alt_fixed_mode) - drm_mode_destroy(intel_connector->base.dev, - panel->alt_fixed_mode); - if (panel->downclock_mode) drm_mode_destroy(intel_connector->base.dev, panel->downclock_mode);
This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e. Per the report, no matter what display mode you select with xrandr, the i915 driver will always select the alternate fixed mode. For the reporter this means that the display will always run at 40Hz which is quite annoying. This may be due to the mode comparison. But there are some other potential issues. The choice of alt_fixed_mode seems dubious. It's the first non-preferred mode, but there are no guarantees that the only difference would be refresh rate. Similarly, there may be more than one preferred mode in the probed modes list, and the commit changes the preferred mode selection to choose the last one on the list instead of the first. (Note that the probed modes list is the raw, unfiltered, unsorted list of modes from drm_add_edid_modes(), not the pretty result after a drm_helper_probe_single_connector_modes() call.) Finally, we already have eerily similar code in place to find the downclock mode for DRRS that seems like could be reused here. Back to the drawing board. Note: This is a hand-crafted revert due to conflicts. If it fails to backport, please just try reverting the original commit directly. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 Reported-by: Rune Petersen <rune@megahurts.dk> Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP if available.") Cc: Clint Taylor <clinton.a.taylor@intel.com> Cc: David Weinehall <david.weinehall@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jim Bride <jim.bride@linux.intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v4.14+ Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 38 +++++--------------------------------- drivers/gpu/drm/i915/intel_drv.h | 2 -- drivers/gpu/drm/i915/intel_dsi.c | 2 +- drivers/gpu/drm/i915/intel_dvo.c | 2 +- drivers/gpu/drm/i915/intel_lvds.c | 3 +-- drivers/gpu/drm/i915/intel_panel.c | 6 ------ 6 files changed, 8 insertions(+), 45 deletions(-)