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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> On Thu, 17 May 2018, Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > Reported-by: Mark Spencer <n7u4722r35@xxxxxxxxxxxxxxxxx>
> > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode
> > > > for
> > > > eDP if available.")
> > > > Cc: Clint Taylor <clinton.a.taylor@xxxxxxxxx>
> > > > Cc: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > > Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
> > > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxx>

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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux