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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com>]

On Thursday, May 17, 2018 12:44:30 PM PDT Dhinakaran Pandiyan wrote:
> On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> > 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_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 <dhinakaran.pandiyan@intel.com>

Hit Send too early and Cancel too late. The signature was misspelt, hope it 
doesn't confuse dim.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx