From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com ([192.55.52.115]:13576 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752129AbeEQHak (ORCPT ); Thu, 17 May 2018 03:30:40 -0400 From: Jani Nikula To: dhinakaran.pandiyan@intel.com, intel-gfx@lists.freedesktop.org Cc: Paulo Zanoni , Rodrigo Vivi , "# v4 . 14+" Subject: Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available." In-Reply-To: <87y3gizr48.fsf@intel.com> References: <20180516080110.22770-1-jani.nikula@intel.com> <1526513406.17473.162.camel@intel.com> <87y3gizr48.fsf@intel.com> Date: Thu, 17 May 2018 10:33:10 +0300 Message-ID: <87po1uzqh5.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: stable-owner@vger.kernel.org List-ID: On Thu, 17 May 2018, Jani Nikula wrote: > On Wed, 16 May 2018, Dhinakaran Pandiyan 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_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); -- Jani Nikula, Intel Open Source Technology Center