Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."

[Jani Nikula <jani.nikula@intel.com>]

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);

-- 
Jani Nikula, Intel Open Source Technology Center