From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com ([192.55.52.43]:35809 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750938AbeEQTTG (ORCPT ); Thu, 17 May 2018 15:19:06 -0400 Message-ID: <1526586270.17473.176.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available." From: Dhinakaran Pandiyan Reply-To: dhinakaran.pandiyan@intel.com To: Jani Nikula , intel-gfx@lists.freedesktop.org Cc: Paulo Zanoni , "# v4 . 14+" , Rodrigo Vivi Date: Thu, 17 May 2018 12:44:30 -0700 In-Reply-To: <87po1uzqh5.fsf@intel.com> References: <20180516080110.22770-1-jani.nikula@intel.com> <1526513406.17473.162.camel@intel.com> <87y3gizr48.fsf@intel.com> <87po1uzqh5.fsf@intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote: > On Thu, 17 May 2018, Jani Nikula wrote: > > > > On Wed, 16 May 2018, Dhinakaran Pandiyan > .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 > > > > Reported-by: Mark Spencer > > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode > > > > for > > > > eDP if available.") > > > > Cc: Clint Taylor > > > > Cc: David Weinehall > > > > Cc: Rodrigo Vivi > > > > Cc: Paulo Zanoni > > > > Cc: Jani Nikula > > > > Cc: Chris Wilson > > > > Cc: Jim Bride > > > > Cc: Jani Nikula > > > > Cc: Joonas Lahtinen > > > > Cc: intel-gfx@lists.freedesktop.org > > > > Cc: # v4.14+ > > > > Signed-off-by: Jani Nikula > > > > --- > > > >  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 > > 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);