Return-Path: Received: from mga09.intel.com ([134.134.136.24]:53010 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752163AbeCMO1X (ORCPT ); Tue, 13 Mar 2018 10:27:23 -0400 Date: Tue, 13 Mar 2018 16:27:17 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maarten Lankhorst Cc: intel-gfx@lists.freedesktop.org, David Weinehall , Rodrigo Vivi , Paulo Zanoni , Jani Nikula , Chris Wilson , Jim Bride , Joonas Lahtinen , stable@vger.kernel.org Subject: Re: [PATCH] drm/i915/dp: Fix alt mode handling. Message-ID: <20180313142717.GX5453@intel.com> References: <20180313131143.GV5453@intel.com> <20180313135235.13686-1-maarten.lankhorst@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180313135235.13686-1-maarten.lankhorst@linux.intel.com> Sender: stable-owner@vger.kernel.org List-ID: 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 > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP if available.") > Cc: David Weinehall > Cc: Rodrigo Vivi > Cc: Paulo Zanoni > Cc: Jani Nikula > Cc: Chris Wilson > Cc: Jim Bride > Cc: Joonas Lahtinen > Cc: # 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. > } > > bool > @@ -1734,12 +1738,11 @@ 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; > + const struct drm_display_mode *panel_mode; > > - if (!intel_edp_compare_alt_mode(req_mode, panel_mode)) > - panel_mode = intel_connector->panel.fixed_mode; > + panel_mode = intel_edp_choose_mode(&pipe_config->base.mode, > + intel_connector->panel.fixed_mode, > + intel_connector->panel.alt_fixed_mode); > > drm_mode_debug_printmodeline(panel_mode); > > -- > 2.16.2 -- Ville Syrj�l� Intel OTC