| Message ID | 20180313135235.13686-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > } > > 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
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. ~Maarten
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.
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; } 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);
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(-)