On Thu, 17 May 2018, Jani Nikula <jani.nik...@intel.com> wrote:
> On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandi...@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 <r...@megahurts.dk>
>>> Reported-by: Mark Spencer <n7u4722...@ynzlx.anonbox.net>
>>> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for
>>> eDP if available.")
>>> Cc: Clint Taylor <clinton.a.tay...@intel.com>
>>> Cc: David Weinehall <david.weineh...@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
>>> Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
>>> Cc: Jani Nikula <jani.nik...@intel.com>
>>> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
>>> Cc: Jim Bride <jim.br...@linux.intel.com>
>>> Cc: Jani Nikula <jani.nik...@linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: <sta...@vger.kernel.org> # v4.14+
>>> Signed-off-by: Jani Nikula <jani.nik...@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);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to