Re: [PATCH] drm/i915/dp: Fix alt mode handling.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 13, 2018 at 03:37:05PM +0100, Maarten Lankhorst wrote:
> Op 13-03-18 om 15:27 schreef Ville Syrj�l�:
> > On Tue, Mar 13, 2018 at 02:52:35PM +0100, Maarten Lankhorst wrote:
> >> On fi-cnl-y3 we have 2 modes that differ only by crtc_clock. This means
> >> that if we request the normal mode, we automatically get the downclocked
> >> mode.
> >>
> >> This can be seen during boot:
> >> [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:101:eDP-1] probed modes :
> >> [drm:drm_mode_debug_printmodeline] Modeline 102:"3840x2160" 60 533250 3840 3888 3920 4000 2160 2163 2168 2222 0x48 0xa
> >> [drm:drm_mode_debug_printmodeline] Modeline 103:"3840x2160" 48 426600 3840 3888 3920 4000 2160 2163 2168 2222 0x40 0xa
> >> ...
> >> [drm:intel_dump_pipe_config [i915]] [CRTC:51:pipe A][modeset]
> >> [drm:intel_dump_pipe_config [i915]] output_types: EDP (0x100)
> >> [drm:intel_dump_pipe_config [i915]] cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> >> [drm:intel_dump_pipe_config [i915]] dp m_n: lanes: 4; gmch_m: 4970250, gmch_n: 8388608, link_m: 828375, link_n: 1048576, tu: 64
> >> [drm:intel_dump_pipe_config [i915]] dp m2_n2: lanes: 4; gmch_m: 4970250, gmch_n: 8388608, link_m: 828375, link_n: 1048576, tu: 64
> >> [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0
> >> [drm:intel_dump_pipe_config [i915]] requested mode:
> >> [drm:drm_mode_debug_printmodeline] Modeline 0:"3840x2160" 60 533250 3840 3888 3920 4000 2160 2163 2168 2222 0x48 0xa
> >> [drm:intel_dump_pipe_config [i915]] adjusted mode:
> >> [drm:drm_mode_debug_printmodeline] Modeline 0:"3840x2160" 48 426600 3840 3888 3920 4000 2160 2163 2168 2222 0x40 0xa
> >>
> >> Add a intel_edp_choose_mode helper that picks the mode.
> >>
> >> If there is no alt mode, it always picks the default mode.
> >> If the alt mode is identical to the default mode, it picks the mode
> >> based on what mode is closest to the request clock.
> >> If the alt mode differs, it only picks the alt mode on exact match.
> >>
> >> Testcase: kms_panel_fitting.atomic-fastset
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP if available.")
> >> 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: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: <stable@vger.kernel.org> # v4.14+
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=105456
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 39 +++++++++++++++++++++------------------
> >>  1 file changed, 21 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 9a4a51e79fa1..075981842a7f 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1671,21 +1671,25 @@ 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)
> >> +static const struct drm_display_mode *
> >> +intel_edp_choose_mode(const struct drm_display_mode *req,
> >> +		      const struct drm_display_mode *def,
> >> +		      const struct drm_display_mode *alt)
> >>  {
> >> -	bool bres = false;
> >> +	if (!alt)
> >> +		return def;
> >>  
> >> -	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;
> >> +	if (drm_mode_equal_no_clocks(def, alt)) {
> >> +		int clock;
> >> +
> >> +		if (!drm_mode_equal_no_clocks(req, def))
> >> +			return def;
> >> +
> >> +		clock = def->clock / 2 + alt->clock / 2;
> >> +
> >> +		return req->clock > clock ? def : alt;
> >> +	} else
> >> +		return drm_mode_equal_no_clocks(req, alt) ? alt : def;
> > Seems a bit convoluted to me.
> >
> > return user_mode->vrefresh > alt_mode->vrefresh ?
> > 	fixed_mode : alt_mode;
> >
> > maybe?
> >
> > Though we don't seem to try very hard to make vrefresh accurate so not
> > 100% this is going to work perfectly.
> >
> > I guess we could go with the "pick the closest" vrefresh approach
> > instead. I'm thinking we should actually be rejecting the user mode
> > entirely if it isn't closeto either refresh rate.
> There's no guarantee vrefresh is set to something sane, afaik. I don't think we can use it.
> If it was always set, then maybe. But I think nothing prevents it from being garbage.

Yeah. I just sent a patch that might fix that.

-- 
Ville Syrj�l�
Intel OTC