* [PATCH v2 0/3] drm/i915: Enable fastboot, v3!
@ 2017-11-20 12:23 Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2 Maarten Lankhorst
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-20 12:23 UTC (permalink / raw)
To: intel-gfx
Updates to IPS fixes, and then flipping the switch. :)
Maarten Lankhorst (3):
drm/i915: Make ips_enabled a property depending on whether IPS is
enabled, v2.
drm/i915: Enable IPS with only sprite plane visible too, v2.
drm/i915: Re-enable fastboot by default
drivers/gpu/drm/i915/i915_params.h | 2 +-
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++++++----------------
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
5 files changed, 63 insertions(+), 58 deletions(-)
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2.
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
@ 2017-11-20 12:23 ` Maarten Lankhorst
2017-11-20 14:01 ` Ville Syrjälä
2017-11-20 12:23 ` [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2 Maarten Lankhorst
` (6 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-20 12:23 UTC (permalink / raw)
To: intel-gfx
ips_enabled was used as a variable of whether IPS can be enabled or not,
but should be used to test whether IPS is actually enabled.
Changes since v1:
- Call needs_modeset on new crtc state. (Ville)
- IPS can be enabled with sprite plane enabled too. (Ville)
- Fix CDCLK vs IPS workaround. (Ville)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 119 +++++++++++++++++++---------------
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
4 files changed, 67 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index e8884c2ade98..30affa7903d7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
- if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
+ if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
/* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b3dec1e6640..0b9ba415839a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- if (!crtc->config->ips_enabled)
+ if (!crtc_state->ips_enabled)
return;
/*
@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_enable_ips(new_crtc_state);
-
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
* So don't enable underrun reporting before at least some planes
@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
intel_check_pch_fifo_underruns(dev_priv);
}
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
- const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
- * So diasble underrun reporting before all the planes get disabled.
- * FIXME: Need to fix the logic to work when we turn off all planes
- * but leave the pipe running.
+ * So disable underrun reporting before all the planes get disabled.
*/
if (IS_GEN2(dev_priv))
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- int pipe = intel_crtc->pipe;
-
- intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+ hsw_disable_ips(to_intel_crtc_state(crtc->state));
/*
* Vblank time updates from the shadow to live plane control register
@@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
if (pipe_config->update_wm_post && pipe_config->base.active)
intel_update_watermarks(crtc);
+ if (!old_crtc_state->ips_enabled ||
+ needs_modeset(&pipe_config->base) ||
+ pipe_config->update_pipe)
+ hsw_enable_ips(pipe_config);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
@@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
struct intel_atomic_state *old_intel_state =
to_intel_atomic_state(old_state);
+ if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+ hsw_disable_ips(old_crtc_state);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(old_intel_state,
@@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
to_intel_plane_state(old_pri_state);
intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
- if (old_primary_state->base.visible &&
+ /*
+ * Gen2 reports pipe underruns whenever all planes are disabled.
+ * So disable underrun reporting before all the planes get disabled.
+ */
+ if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
(modeset || !primary_state->base.visible))
- intel_pre_disable_primary(&crtc->base, old_crtc_state);
+ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
}
/*
@@ -6228,15 +6210,42 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
return ret;
}
-static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
- struct intel_crtc_state *pipe_config)
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
{
- if (pipe_config->ips_force_disable)
+ struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
+
+ /* IPS only exists on ULT machines and is tied to pipe A. */
+ if (!hsw_crtc_supports_ips(crtc))
+ return false;
+
+ if (!i915_modparams.enable_ips)
return false;
if (pipe_config->pipe_bpp > 24)
return false;
+ return true;
+}
+
+static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
+{
+ struct drm_i915_private *dev_priv = to_i915(pipe_config->base.crtc->dev);
+
+ if (!hsw_pipe_config_ips_capable(pipe_config))
+ return false;
+
+ if (pipe_config->ips_force_disable)
+ return false;
+
+ /*
+ * FIXME IPS should be fine as long as one plane is
+ * enabled, but in practice it seems to have problems
+ * when going from primary only to sprite only and vice
+ * versa.
+ */
+ if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ return false;
+
/* HSW can handle pixel rate up to cdclk? */
if (IS_HASWELL(dev_priv))
return true;
@@ -6252,17 +6261,6 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
dev_priv->max_cdclk_freq * 95 / 100;
}
-static void hsw_compute_ips_config(struct intel_crtc *crtc,
- struct intel_crtc_state *pipe_config)
-{
- struct drm_device *dev = crtc->base.dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
-
- pipe_config->ips_enabled = i915_modparams.enable_ips &&
- hsw_crtc_supports_ips(crtc) &&
- pipe_config_supports_ips(dev_priv, pipe_config);
-}
-
static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
{
const struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -6378,9 +6376,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
intel_crtc_compute_pixel_rate(pipe_config);
- if (HAS_IPS(dev_priv))
- hsw_compute_ips_config(crtc, pipe_config);
-
if (pipe_config->has_pch_encoder)
return ironlake_fdi_compute_config(crtc, pipe_config);
@@ -9275,6 +9270,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
ironlake_get_pfit_config(crtc, pipe_config);
}
+ if (hsw_crtc_supports_ips(crtc)) {
+ if (IS_HASWELL(dev_priv))
+ pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
+ else {
+ /*
+ * We cannot readout IPS state on broadwell, set to
+ * true so we can set it to a defined state on first
+ * commit.
+ */
+ pipe_config->ips_enabled = true;
+ }
+ }
+
if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
pipe_config->pixel_multiplier =
@@ -10488,6 +10496,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
pipe_config);
}
+ if (HAS_IPS(dev_priv))
+ pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
+
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 69aab324aaa1..40417b20844e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
bool intel_crtc_active(struct intel_crtc *crtc);
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
enum intel_display_power_domain intel_port_to_power_domain(enum port port);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 61641d479b93..1f5cd572a7ff 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
* completely disable it.
*/
pipe_config->ips_force_disable = enable;
- if (pipe_config->ips_enabled == enable)
- pipe_config->base.connectors_changed = true;
}
if (IS_HASWELL(dev_priv)) {
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2.
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2 Maarten Lankhorst
@ 2017-11-20 12:23 ` Maarten Lankhorst
2017-11-20 13:27 ` Ville Syrjälä
2017-11-20 12:23 ` [PATCH (resend) 3/3] drm/i915: Re-enable fastboot by default Maarten Lankhorst
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-20 12:23 UTC (permalink / raw)
To: intel-gfx
This comment predates atomic, and I think with the way we currently
track IPS, it's safe to enable this for the case we switch too.
Changes since v1:
- Keep IPS enabled when switching planes.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b9ba415839a..09817bc6f9a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6237,13 +6237,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
if (pipe_config->ips_force_disable)
return false;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ /* IPS should be fine as long as one plane is enabled. */
+ if (hweight32(pipe_config->active_planes & ~BIT(PLANE_CURSOR)) != 1)
return false;
/* HSW can handle pixel rate up to cdclk? */
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH (resend) 3/3] drm/i915: Re-enable fastboot by default
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2 Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2 Maarten Lankhorst
@ 2017-11-20 12:23 ` Maarten Lankhorst
2017-11-20 13:05 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! Patchwork
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-20 12:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter
This fix was originally reverted because it left a chromebook pixel
black, and no immediate fix was available. This has been fixed in the
meantime.
Rather than trying to remove the parameter, set it to default to true
for now, so we can always back out if required.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Testcase: kms_panel_fitting
---
drivers/gpu/drm/i915/i915_params.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c7292268ed43..b99cb58801e6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -57,7 +57,7 @@
param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \
param(bool, enable_cmd_parser, true) \
param(bool, enable_hangcheck, true) \
- param(bool, fastboot, false) \
+ param(bool, fastboot, true) \
param(bool, prefault_disable, false) \
param(bool, load_detect_test, false) \
param(bool, force_reset_modeset_test, false) \
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3!
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
` (2 preceding siblings ...)
2017-11-20 12:23 ` [PATCH (resend) 3/3] drm/i915: Re-enable fastboot by default Maarten Lankhorst
@ 2017-11-20 13:05 ` Patchwork
2017-11-20 14:15 ` ✓ Fi.CI.IGT: " Patchwork
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2017-11-20 13:05 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable fastboot, v3!
URL : https://patchwork.freedesktop.org/series/34098/
State : success
== Summary ==
Series 34098v1 drm/i915: Enable fastboot, v3!
https://patchwork.freedesktop.org/api/1.0/series/34098/revisions/1/mbox/
Test kms_busy:
Subgroup basic-flip-a:
fail -> PASS (fi-gdg-551) fdo#102654
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
fail -> PASS (fi-gdg-551) fdo#102618
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:441s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:453s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:382s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:545s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:279s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:507s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:508s
fi-byt-j1900 total:289 pass:254 dwarn:0 dfail:0 fail:0 skip:35 time:496s
fi-cfl-s2 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:604s
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:433s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:268s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:541s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:429s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:439s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:434s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:492s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:458s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:484s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:536s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:474s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:535s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:584s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:447s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:544s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:522s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:497s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:465s
fi-snb-2520m total:246 pass:212 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:417s
Blacklisted hosts:
fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:503s
4b69b21e93c02a8b45781c490077dc1a35adc608 drm-tip: 2017y-11m-20d-10h-20m-58s UTC integration manifest
b3b673ef83f2 drm/i915: Re-enable fastboot by default
0df89b40577b drm/i915: Enable IPS with only sprite plane visible too, v2.
a0a4a79f52db drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7202/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2.
2017-11-20 12:23 ` [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2 Maarten Lankhorst
@ 2017-11-20 13:27 ` Ville Syrjälä
2017-11-21 9:27 ` Maarten Lankhorst
2017-11-22 15:08 ` [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3 Maarten Lankhorst
0 siblings, 2 replies; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-20 13:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Nov 20, 2017 at 01:23:36PM +0100, Maarten Lankhorst wrote:
> This comment predates atomic, and I think with the way we currently
> track IPS, it's safe to enable this for the case we switch too.
>
> Changes since v1:
> - Keep IPS enabled when switching planes.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0b9ba415839a..09817bc6f9a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6237,13 +6237,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> if (pipe_config->ips_force_disable)
> return false;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + /* IPS should be fine as long as one plane is enabled. */
Maybe "... at least one plane is enabled." to make it less vague?
> + if (hweight32(pipe_config->active_planes & ~BIT(PLANE_CURSOR)) != 1)
I keep wondering if popcnt is cheaper than is_power_of_2()? If it is,
maybe someone should actually make is_power_of_2() use popcnt...
> return false;
>
> /* HSW can handle pixel rate up to cdclk? */
> --
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2.
2017-11-20 12:23 ` [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2 Maarten Lankhorst
@ 2017-11-20 14:01 ` Ville Syrjälä
2017-11-20 16:21 ` Maarten Lankhorst
2017-11-22 15:07 ` [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3 Maarten Lankhorst
0 siblings, 2 replies; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-20 14:01 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Nov 20, 2017 at 01:23:35PM +0100, Maarten Lankhorst wrote:
> ips_enabled was used as a variable of whether IPS can be enabled or not,
> but should be used to test whether IPS is actually enabled.
>
> Changes since v1:
> - Call needs_modeset on new crtc state. (Ville)
> - IPS can be enabled with sprite plane enabled too. (Ville)
> - Fix CDCLK vs IPS workaround. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 119 +++++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
> 4 files changed, 67 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index e8884c2ade98..30affa7903d7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> + if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
> min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
I think either we should drop this entirely and make the
ips_compute_config() check logical.cdclk instead of max_cdclk,
or we stick to the max_cdclk based check but move it into
hsw_pipe_config_ips_capable(). Otherwise we would end up
rejecting any mode whose pixel rate exceed the 95% cdclk
limit.
>
> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b3dec1e6640..0b9ba415839a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (!crtc->config->ips_enabled)
> + if (!crtc_state->ips_enabled)
> return;
>
> /*
> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_enable_ips(new_crtc_state);
> -
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> * So don't enable underrun reporting before at least some planes
> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> intel_check_pch_fifo_underruns(dev_priv);
> }
>
> -/* FIXME move all this to pre_plane_update() with proper state tracking */
> +/* FIXME get rid of this and use pre_plane_update */
> static void
> -intel_pre_disable_primary(struct drm_crtc *crtc,
> - const struct intel_crtc_state *old_crtc_state)
> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> - * So diasble underrun reporting before all the planes get disabled.
> - * FIXME: Need to fix the logic to work when we turn off all planes
> - * but leave the pipe running.
> + * So disable underrun reporting before all the planes get disabled.
> */
> if (IS_GEN2(dev_priv))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_disable_ips(old_crtc_state);
> -}
> -
> -/* FIXME get rid of this and use pre_plane_update */
> -static void
> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> -
> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> + hsw_disable_ips(to_intel_crtc_state(crtc->state));
>
> /*
> * Vblank time updates from the shadow to live plane control register
> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> if (pipe_config->update_wm_post && pipe_config->base.active)
> intel_update_watermarks(crtc);
>
> + if (!old_crtc_state->ips_enabled ||
> + needs_modeset(&pipe_config->base) ||
> + pipe_config->update_pipe)
Hmm. So thinking more about this I guess I figured out why you put the
update_pipe check here. On BDW the initial old_crtc_state will have
ips_enabled=true since the readout now always makes it so. Thus if we
skip the initial modeset we would not turn on IPS even if a suitable
set of planes is enabled.
I think it would be more clear to check for the INHERITED thing
specifically. Also that would avoid paying the cost of the pcode
access every time we have update_pipe==true and ips_enabled==true.
> + hsw_enable_ips(pipe_config);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> struct intel_atomic_state *old_intel_state =
> to_intel_atomic_state(old_state);
>
> + if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> + hsw_disable_ips(old_crtc_state);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(old_intel_state,
> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> to_intel_plane_state(old_pri_state);
>
> intel_fbc_pre_update(crtc, pipe_config, primary_state);
> -
> - if (old_primary_state->base.visible &&
> + /*
> + * Gen2 reports pipe underruns whenever all planes are disabled.
> + * So disable underrun reporting before all the planes get disabled.
> + */
> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
> (modeset || !primary_state->base.visible))
> - intel_pre_disable_primary(&crtc->base, old_crtc_state);
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> }
>
> /*
> @@ -6228,15 +6210,42 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> return ret;
> }
>
> -static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> - struct intel_crtc_state *pipe_config)
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
> {
> - if (pipe_config->ips_force_disable)
> + struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> +
> + /* IPS only exists on ULT machines and is tied to pipe A. */
> + if (!hsw_crtc_supports_ips(crtc))
> + return false;
> +
> + if (!i915_modparams.enable_ips)
> return false;
>
> if (pipe_config->pipe_bpp > 24)
> return false;
>
> + return true;
> +}
> +
> +static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> +{
> + struct drm_i915_private *dev_priv = to_i915(pipe_config->base.crtc->dev);
> +
> + if (!hsw_pipe_config_ips_capable(pipe_config))
> + return false;
> +
> + if (pipe_config->ips_force_disable)
> + return false;
> +
> + /*
> + * FIXME IPS should be fine as long as one plane is
> + * enabled, but in practice it seems to have problems
> + * when going from primary only to sprite only and vice
> + * versa.
> + */
> + if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + return false;
> +
> /* HSW can handle pixel rate up to cdclk? */
> if (IS_HASWELL(dev_priv))
> return true;
> @@ -6252,17 +6261,6 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> dev_priv->max_cdclk_freq * 95 / 100;
> }
>
> -static void hsw_compute_ips_config(struct intel_crtc *crtc,
> - struct intel_crtc_state *pipe_config)
> -{
> - struct drm_device *dev = crtc->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> -
> - pipe_config->ips_enabled = i915_modparams.enable_ips &&
> - hsw_crtc_supports_ips(crtc) &&
> - pipe_config_supports_ips(dev_priv, pipe_config);
> -}
> -
> static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> {
> const struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -6378,9 +6376,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>
> intel_crtc_compute_pixel_rate(pipe_config);
>
> - if (HAS_IPS(dev_priv))
> - hsw_compute_ips_config(crtc, pipe_config);
> -
> if (pipe_config->has_pch_encoder)
> return ironlake_fdi_compute_config(crtc, pipe_config);
>
> @@ -9275,6 +9270,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> ironlake_get_pfit_config(crtc, pipe_config);
> }
>
> + if (hsw_crtc_supports_ips(crtc)) {
> + if (IS_HASWELL(dev_priv))
> + pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
> + else {
> + /*
> + * We cannot readout IPS state on broadwell, set to
> + * true so we can set it to a defined state on first
> + * commit.
> + */
> + pipe_config->ips_enabled = true;
> + }
> + }
> +
> if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> pipe_config->pixel_multiplier =
> @@ -10488,6 +10496,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> pipe_config);
> }
>
> + if (HAS_IPS(dev_priv))
> + pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 69aab324aaa1..40417b20844e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
>
> bool intel_crtc_active(struct intel_crtc *crtc);
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
> void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
> void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
> enum intel_display_power_domain intel_port_to_power_domain(enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 61641d479b93..1f5cd572a7ff 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> * completely disable it.
> */
> pipe_config->ips_force_disable = enable;
> - if (pipe_config->ips_enabled == enable)
> - pipe_config->base.connectors_changed = true;
> }
>
> if (IS_HASWELL(dev_priv)) {
> --
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Enable fastboot, v3!
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
` (3 preceding siblings ...)
2017-11-20 13:05 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! Patchwork
@ 2017-11-20 14:15 ` Patchwork
2017-11-22 15:38 ` ✗ Fi.CI.BAT: failure for drm/i915: Enable fastboot, v3! (rev3) Patchwork
` (2 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2017-11-20 14:15 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable fastboot, v3!
URL : https://patchwork.freedesktop.org/series/34098/
State : success
== Summary ==
Test drv_module_reload:
Subgroup basic-no-display:
pass -> DMESG-WARN (shard-snb) fdo#102707
Test gem_eio:
Subgroup in-flight-suspend:
fail -> PASS (shard-hsw) fdo#103375
Test kms_cursor_legacy:
Subgroup long-nonblocking-modeset-vs-cursor-atomic:
pass -> SKIP (shard-hsw) fdo#103181
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#103181 https://bugs.freedesktop.org/show_bug.cgi?id=103181
shard-hsw total:2585 pass:1473 dwarn:1 dfail:1 fail:10 skip:1100 time:9504s
shard-snb total:2585 pass:1259 dwarn:2 dfail:1 fail:12 skip:1311 time:8015s
Blacklisted hosts:
shard-apl total:2565 pass:1603 dwarn:3 dfail:0 fail:22 skip:936 time:12910s
shard-kbl total:2534 pass:1675 dwarn:8 dfail:0 fail:26 skip:823 time:10169s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7202/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2.
2017-11-20 14:01 ` Ville Syrjälä
@ 2017-11-20 16:21 ` Maarten Lankhorst
2017-11-22 15:07 ` [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3 Maarten Lankhorst
1 sibling, 0 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-20 16:21 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 20-11-17 om 15:01 schreef Ville Syrjälä:
> On Mon, Nov 20, 2017 at 01:23:35PM +0100, Maarten Lankhorst wrote:
>> ips_enabled was used as a variable of whether IPS can be enabled or not,
>> but should be used to test whether IPS is actually enabled.
>>
>> Changes since v1:
>> - Call needs_modeset on new crtc state. (Ville)
>> - IPS can be enabled with sprite plane enabled too. (Ville)
>> - Fix CDCLK vs IPS workaround. (Ville)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 119 +++++++++++++++++++---------------
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
>> 4 files changed, 67 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>> index e8884c2ade98..30affa7903d7 100644
>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>> @@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>> min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
>>
>> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
>> + if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
>> min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
> I think either we should drop this entirely and make the
> ips_compute_config() check logical.cdclk instead of max_cdclk,
> or we stick to the max_cdclk based check but move it into
> hsw_pipe_config_ips_capable(). Otherwise we would end up
> rejecting any mode whose pixel rate exceed the 95% cdclk
> limit.
>
>>
>> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3b3dec1e6640..0b9ba415839a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>>
>> - if (!crtc->config->ips_enabled)
>> + if (!crtc_state->ips_enabled)
>> return;
>>
>> /*
>> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> int pipe = intel_crtc->pipe;
>>
>> - /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> - */
>> - hsw_enable_ips(new_crtc_state);
>> -
>> /*
>> * Gen2 reports pipe underruns whenever all planes are disabled.
>> * So don't enable underrun reporting before at least some planes
>> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>> intel_check_pch_fifo_underruns(dev_priv);
>> }
>>
>> -/* FIXME move all this to pre_plane_update() with proper state tracking */
>> +/* FIXME get rid of this and use pre_plane_update */
>> static void
>> -intel_pre_disable_primary(struct drm_crtc *crtc,
>> - const struct intel_crtc_state *old_crtc_state)
>> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>>
>> /*
>> * Gen2 reports pipe underruns whenever all planes are disabled.
>> - * So diasble underrun reporting before all the planes get disabled.
>> - * FIXME: Need to fix the logic to work when we turn off all planes
>> - * but leave the pipe running.
>> + * So disable underrun reporting before all the planes get disabled.
>> */
>> if (IS_GEN2(dev_priv))
>> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>>
>> - /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> - */
>> - hsw_disable_ips(old_crtc_state);
>> -}
>> -
>> -/* FIXME get rid of this and use pre_plane_update */
>> -static void
>> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> -{
>> - struct drm_device *dev = crtc->dev;
>> - struct drm_i915_private *dev_priv = to_i915(dev);
>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> - int pipe = intel_crtc->pipe;
>> -
>> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>> + hsw_disable_ips(to_intel_crtc_state(crtc->state));
>>
>> /*
>> * Vblank time updates from the shadow to live plane control register
>> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>> if (pipe_config->update_wm_post && pipe_config->base.active)
>> intel_update_watermarks(crtc);
>>
>> + if (!old_crtc_state->ips_enabled ||
>> + needs_modeset(&pipe_config->base) ||
>> + pipe_config->update_pipe)
> Hmm. So thinking more about this I guess I figured out why you put the
> update_pipe check here. On BDW the initial old_crtc_state will have
> ips_enabled=true since the readout now always makes it so. Thus if we
> skip the initial modeset we would not turn on IPS even if a suitable
> set of planes is enabled.
>
> I think it would be more clear to check for the INHERITED thing
> specifically. Also that would avoid paying the cost of the pcode
> access every time we have update_pipe==true and ips_enabled==true.
In case of update_pipe = true we already avoided a modeset, which is an order more expensive. I'm not too worried about it. :p
Maybe hsw_ips_pre/post_update with the relevant checks?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2.
2017-11-20 13:27 ` Ville Syrjälä
@ 2017-11-21 9:27 ` Maarten Lankhorst
2017-11-22 15:08 ` [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3 Maarten Lankhorst
1 sibling, 0 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-21 9:27 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 20-11-17 om 14:27 schreef Ville Syrjälä:
> On Mon, Nov 20, 2017 at 01:23:36PM +0100, Maarten Lankhorst wrote:
>> This comment predates atomic, and I think with the way we currently
>> track IPS, it's safe to enable this for the case we switch too.
>>
>> Changes since v1:
>> - Keep IPS enabled when switching planes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0b9ba415839a..09817bc6f9a8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6237,13 +6237,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
>> if (pipe_config->ips_force_disable)
>> return false;
>>
>> - /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> - */
>> - if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
>> + /* IPS should be fine as long as one plane is enabled. */
> Maybe "... at least one plane is enabled." to make it less vague?
>
>> + if (hweight32(pipe_config->active_planes & ~BIT(PLANE_CURSOR)) != 1)
> I keep wondering if popcnt is cheaper than is_power_of_2()? If it is,
> maybe someone should actually make is_power_of_2() use popcnt...
Crap, should be
if (pipe_config->active_planes & ~BIT(PLANE_CURSOR))
>
>> return false;
>>
>> /* HSW can handle pixel rate up to cdclk? */
>> --
>> 2.15.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
2017-11-20 14:01 ` Ville Syrjälä
2017-11-20 16:21 ` Maarten Lankhorst
@ 2017-11-22 15:07 ` Maarten Lankhorst
2017-11-22 15:34 ` Ville Syrjälä
1 sibling, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-22 15:07 UTC (permalink / raw)
To: intel-gfx
ips_enabled was used as a variable of whether IPS can be enabled or not,
but should be used to test whether IPS is actually enabled.
Changes since v1:
- Call needs_modeset on new crtc state. (Ville)
- IPS can be enabled with sprite plane enabled too. (Ville)
- Fix CDCLK vs IPS workaround. (Ville)
Changes since v2:
- Only re-enable fastset when inheriting mode. (Ville)
- Put the conditions for enabling and disabling IPS in a helper.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 151 +++++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
4 files changed, 98 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index e8884c2ade98..30affa7903d7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
- if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
+ if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
/* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2007c69468b9..b62cb6e90669 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -489,7 +489,7 @@ static const struct intel_limit intel_limits_bxt = {
};
static bool
-needs_modeset(struct drm_crtc_state *state)
+needs_modeset(const struct drm_crtc_state *state)
{
return drm_atomic_crtc_needs_modeset(state);
}
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- if (!crtc->config->ips_enabled)
+ if (!crtc_state->ips_enabled)
return;
/*
@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_enable_ips(new_crtc_state);
-
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
* So don't enable underrun reporting before at least some planes
@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
intel_check_pch_fifo_underruns(dev_priv);
}
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
- const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
- * So diasble underrun reporting before all the planes get disabled.
- * FIXME: Need to fix the logic to work when we turn off all planes
- * but leave the pipe running.
+ * So disable underrun reporting before all the planes get disabled.
*/
if (IS_GEN2(dev_priv))
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- int pipe = intel_crtc->pipe;
-
- intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+ hsw_disable_ips(to_intel_crtc_state(crtc->state));
/*
* Vblank time updates from the shadow to live plane control register
@@ -5042,6 +5013,38 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
intel_wait_for_vblank(dev_priv, pipe);
}
+static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+ if (!old_crtc_state->ips_enabled)
+ return false;
+
+ if (needs_modeset(&new_crtc_state->base))
+ return true;
+
+ return !new_crtc_state->ips_enabled;
+}
+
+static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+ if (!new_crtc_state->ips_enabled)
+ return false;
+
+ if (needs_modeset(&new_crtc_state->base))
+ return true;
+
+ /*
+ * We can't read out IPS on broadwell, assume the worst and
+ * forcibly enable IPS on the first fastset.
+ */
+ if (new_crtc_state->update_pipe &&
+ old_crtc_state->base.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
+ return true;
+
+ return !old_crtc_state->ips_enabled;
+}
+
static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
@@ -5058,6 +5061,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
if (pipe_config->update_wm_post && pipe_config->base.active)
intel_update_watermarks(crtc);
+ if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
+ hsw_enable_ips(pipe_config);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
@@ -5088,6 +5094,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
struct intel_atomic_state *old_intel_state =
to_intel_atomic_state(old_state);
+ if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
+ hsw_disable_ips(old_crtc_state);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(old_intel_state,
@@ -5096,10 +5105,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
to_intel_plane_state(old_pri_state);
intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
- if (old_primary_state->base.visible &&
+ /*
+ * Gen2 reports pipe underruns whenever all planes are disabled.
+ * So disable underrun reporting before all the planes get disabled.
+ */
+ if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
(modeset || !primary_state->base.visible))
- intel_pre_disable_primary(&crtc->base, old_crtc_state);
+ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
}
/*
@@ -6228,15 +6240,42 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
return ret;
}
-static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
- struct intel_crtc_state *pipe_config)
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
{
- if (pipe_config->ips_force_disable)
+ struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
+
+ /* IPS only exists on ULT machines and is tied to pipe A. */
+ if (!hsw_crtc_supports_ips(crtc))
+ return false;
+
+ if (!i915_modparams.enable_ips)
return false;
if (pipe_config->pipe_bpp > 24)
return false;
+ return true;
+}
+
+static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
+{
+ struct drm_i915_private *dev_priv = to_i915(pipe_config->base.crtc->dev);
+
+ if (!hsw_pipe_config_ips_capable(pipe_config))
+ return false;
+
+ if (pipe_config->ips_force_disable)
+ return false;
+
+ /*
+ * FIXME IPS should be fine as long as one plane is
+ * enabled, but in practice it seems to have problems
+ * when going from primary only to sprite only and vice
+ * versa.
+ */
+ if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ return false;
+
/* HSW can handle pixel rate up to cdclk? */
if (IS_HASWELL(dev_priv))
return true;
@@ -6252,17 +6291,6 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
dev_priv->max_cdclk_freq * 95 / 100;
}
-static void hsw_compute_ips_config(struct intel_crtc *crtc,
- struct intel_crtc_state *pipe_config)
-{
- struct drm_device *dev = crtc->base.dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
-
- pipe_config->ips_enabled = i915_modparams.enable_ips &&
- hsw_crtc_supports_ips(crtc) &&
- pipe_config_supports_ips(dev_priv, pipe_config);
-}
-
static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
{
const struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -6378,9 +6406,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
intel_crtc_compute_pixel_rate(pipe_config);
- if (HAS_IPS(dev_priv))
- hsw_compute_ips_config(crtc, pipe_config);
-
if (pipe_config->has_pch_encoder)
return ironlake_fdi_compute_config(crtc, pipe_config);
@@ -9275,6 +9300,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
ironlake_get_pfit_config(crtc, pipe_config);
}
+ if (hsw_crtc_supports_ips(crtc)) {
+ if (IS_HASWELL(dev_priv))
+ pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
+ else {
+ /*
+ * We cannot readout IPS state on broadwell, set to
+ * true so we can set it to a defined state on first
+ * commit.
+ */
+ pipe_config->ips_enabled = true;
+ }
+ }
+
if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
pipe_config->pixel_multiplier =
@@ -10489,6 +10527,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
pipe_config);
}
+ if (HAS_IPS(dev_priv))
+ pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
+
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 69aab324aaa1..40417b20844e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
bool intel_crtc_active(struct intel_crtc *crtc);
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
enum intel_display_power_domain intel_port_to_power_domain(enum port port);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 61641d479b93..1f5cd572a7ff 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
* completely disable it.
*/
pipe_config->ips_force_disable = enable;
- if (pipe_config->ips_enabled == enable)
- pipe_config->base.connectors_changed = true;
}
if (IS_HASWELL(dev_priv)) {
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3.
2017-11-20 13:27 ` Ville Syrjälä
2017-11-21 9:27 ` Maarten Lankhorst
@ 2017-11-22 15:08 ` Maarten Lankhorst
2017-11-22 15:28 ` Ville Syrjälä
1 sibling, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-22 15:08 UTC (permalink / raw)
To: intel-gfx
This comment predates atomic, and I think with the way we currently
track IPS, it's safe to enable this for the case we switch too.
Changes since v1:
- Keep IPS enabled when switching planes.
Changes since v2:
- Enable IPS when at least one plane is enabled. (Ville)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b62cb6e90669..77778bc0f0ff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6267,13 +6267,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
if (pipe_config->ips_force_disable)
return false;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ /* IPS should be fine as long as at least one plane is enabled. */
+ if (pipe_config->active_planes & ~BIT(PLANE_CURSOR))
return false;
/* HSW can handle pixel rate up to cdclk? */
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3.
2017-11-22 15:08 ` [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3 Maarten Lankhorst
@ 2017-11-22 15:28 ` Ville Syrjälä
2017-11-22 18:39 ` [PATCH 2/2] drm/i915: Enable IPS with only sprite plane visible too, v4 Maarten Lankhorst
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-22 15:28 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Wed, Nov 22, 2017 at 04:08:03PM +0100, Maarten Lankhorst wrote:
> This comment predates atomic, and I think with the way we currently
> track IPS, it's safe to enable this for the case we switch too.
>
> Changes since v1:
> - Keep IPS enabled when switching planes.
> Changes since v2:
> - Enable IPS when at least one plane is enabled. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b62cb6e90669..77778bc0f0ff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6267,13 +6267,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> if (pipe_config->ips_force_disable)
> return false;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + /* IPS should be fine as long as at least one plane is enabled. */
> + if (pipe_config->active_planes & ~BIT(PLANE_CURSOR))
== 0 missing
> return false;
>
> /* HSW can handle pixel rate up to cdclk? */
> --
> 2.15.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
2017-11-22 15:07 ` [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3 Maarten Lankhorst
@ 2017-11-22 15:34 ` Ville Syrjälä
2017-11-22 18:39 ` [PATCH 1/2] " Maarten Lankhorst
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-22 15:34 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Wed, Nov 22, 2017 at 04:07:17PM +0100, Maarten Lankhorst wrote:
> ips_enabled was used as a variable of whether IPS can be enabled or not,
> but should be used to test whether IPS is actually enabled.
>
> Changes since v1:
> - Call needs_modeset on new crtc state. (Ville)
> - IPS can be enabled with sprite plane enabled too. (Ville)
> - Fix CDCLK vs IPS workaround. (Ville)
> Changes since v2:
> - Only re-enable fastset when inheriting mode. (Ville)
> - Put the conditions for enabling and disabling IPS in a helper.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 151 +++++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
> 4 files changed, 98 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index e8884c2ade98..30affa7903d7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> + if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
> min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
>
> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2007c69468b9..b62cb6e90669 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -489,7 +489,7 @@ static const struct intel_limit intel_limits_bxt = {
> };
>
> static bool
> -needs_modeset(struct drm_crtc_state *state)
> +needs_modeset(const struct drm_crtc_state *state)
> {
> return drm_atomic_crtc_needs_modeset(state);
> }
> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (!crtc->config->ips_enabled)
> + if (!crtc_state->ips_enabled)
> return;
>
> /*
> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_enable_ips(new_crtc_state);
> -
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> * So don't enable underrun reporting before at least some planes
> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> intel_check_pch_fifo_underruns(dev_priv);
> }
>
> -/* FIXME move all this to pre_plane_update() with proper state tracking */
> +/* FIXME get rid of this and use pre_plane_update */
> static void
> -intel_pre_disable_primary(struct drm_crtc *crtc,
> - const struct intel_crtc_state *old_crtc_state)
> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> - * So diasble underrun reporting before all the planes get disabled.
> - * FIXME: Need to fix the logic to work when we turn off all planes
> - * but leave the pipe running.
> + * So disable underrun reporting before all the planes get disabled.
> */
> if (IS_GEN2(dev_priv))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_disable_ips(old_crtc_state);
> -}
> -
> -/* FIXME get rid of this and use pre_plane_update */
> -static void
> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> -
> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> + hsw_disable_ips(to_intel_crtc_state(crtc->state));
>
> /*
> * Vblank time updates from the shadow to live plane control register
> @@ -5042,6 +5013,38 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> intel_wait_for_vblank(dev_priv, pipe);
> }
>
> +static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state)
> +{
> + if (!old_crtc_state->ips_enabled)
> + return false;
> +
> + if (needs_modeset(&new_crtc_state->base))
> + return true;
> +
> + return !new_crtc_state->ips_enabled;
> +}
> +
> +static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state)
> +{
> + if (!new_crtc_state->ips_enabled)
> + return false;
> +
> + if (needs_modeset(&new_crtc_state->base))
> + return true;
> +
> + /*
> + * We can't read out IPS on broadwell, assume the worst and
> + * forcibly enable IPS on the first fastset.
> + */
> + if (new_crtc_state->update_pipe &&
> + old_crtc_state->base.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
> + return true;
> +
> + return !old_crtc_state->ips_enabled;
> +}
> +
> static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> @@ -5058,6 +5061,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> if (pipe_config->update_wm_post && pipe_config->base.active)
> intel_update_watermarks(crtc);
>
> + if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
> + hsw_enable_ips(pipe_config);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
> @@ -5088,6 +5094,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> struct intel_atomic_state *old_intel_state =
> to_intel_atomic_state(old_state);
>
> + if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
> + hsw_disable_ips(old_crtc_state);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(old_intel_state,
> @@ -5096,10 +5105,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> to_intel_plane_state(old_pri_state);
>
> intel_fbc_pre_update(crtc, pipe_config, primary_state);
> -
> - if (old_primary_state->base.visible &&
> + /*
> + * Gen2 reports pipe underruns whenever all planes are disabled.
> + * So disable underrun reporting before all the planes get disabled.
> + */
> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
> (modeset || !primary_state->base.visible))
> - intel_pre_disable_primary(&crtc->base, old_crtc_state);
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> }
>
> /*
> @@ -6228,15 +6240,42 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> return ret;
> }
>
> -static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> - struct intel_crtc_state *pipe_config)
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
> {
> - if (pipe_config->ips_force_disable)
> + struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> +
> + /* IPS only exists on ULT machines and is tied to pipe A. */
> + if (!hsw_crtc_supports_ips(crtc))
> + return false;
> +
> + if (!i915_modparams.enable_ips)
> return false;
>
> if (pipe_config->pipe_bpp > 24)
> return false;
>
> + return true;
The max_cdclk check should be here.
Otherwise this looks quite nice.
Maybe also re-add the ips_enabled vs. hw state assert on HSW?
I guess intel_verify_planes() migth be a nice place for it, in
case we ever decide to start doing these checks for pure plane
updates as well...
> +}
> +
> +static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> +{
> + struct drm_i915_private *dev_priv = to_i915(pipe_config->base.crtc->dev);
> +
> + if (!hsw_pipe_config_ips_capable(pipe_config))
> + return false;
> +
> + if (pipe_config->ips_force_disable)
> + return false;
> +
> + /*
> + * FIXME IPS should be fine as long as one plane is
> + * enabled, but in practice it seems to have problems
> + * when going from primary only to sprite only and vice
> + * versa.
> + */
> + if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + return false;
> +
> /* HSW can handle pixel rate up to cdclk? */
> if (IS_HASWELL(dev_priv))
> return true;
> @@ -6252,17 +6291,6 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> dev_priv->max_cdclk_freq * 95 / 100;
> }
>
> -static void hsw_compute_ips_config(struct intel_crtc *crtc,
> - struct intel_crtc_state *pipe_config)
> -{
> - struct drm_device *dev = crtc->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> -
> - pipe_config->ips_enabled = i915_modparams.enable_ips &&
> - hsw_crtc_supports_ips(crtc) &&
> - pipe_config_supports_ips(dev_priv, pipe_config);
> -}
> -
> static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> {
> const struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -6378,9 +6406,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>
> intel_crtc_compute_pixel_rate(pipe_config);
>
> - if (HAS_IPS(dev_priv))
> - hsw_compute_ips_config(crtc, pipe_config);
> -
> if (pipe_config->has_pch_encoder)
> return ironlake_fdi_compute_config(crtc, pipe_config);
>
> @@ -9275,6 +9300,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> ironlake_get_pfit_config(crtc, pipe_config);
> }
>
> + if (hsw_crtc_supports_ips(crtc)) {
> + if (IS_HASWELL(dev_priv))
> + pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
> + else {
> + /*
> + * We cannot readout IPS state on broadwell, set to
> + * true so we can set it to a defined state on first
> + * commit.
> + */
> + pipe_config->ips_enabled = true;
> + }
> + }
> +
> if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> pipe_config->pixel_multiplier =
> @@ -10489,6 +10527,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> pipe_config);
> }
>
> + if (HAS_IPS(dev_priv))
> + pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 69aab324aaa1..40417b20844e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
>
> bool intel_crtc_active(struct intel_crtc *crtc);
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
> void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
> void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
> enum intel_display_power_domain intel_port_to_power_domain(enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 61641d479b93..1f5cd572a7ff 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> * completely disable it.
> */
> pipe_config->ips_force_disable = enable;
> - if (pipe_config->ips_enabled == enable)
> - pipe_config->base.connectors_changed = true;
> }
>
> if (IS_HASWELL(dev_priv)) {
> --
> 2.15.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Enable fastboot, v3! (rev3)
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
` (4 preceding siblings ...)
2017-11-20 14:15 ` ✓ Fi.CI.IGT: " Patchwork
@ 2017-11-22 15:38 ` Patchwork
2017-11-22 19:27 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! (rev5) Patchwork
2017-11-22 21:11 ` ✓ Fi.CI.IGT: " Patchwork
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2017-11-22 15:38 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable fastboot, v3! (rev3)
URL : https://patchwork.freedesktop.org/series/34098/
State : failure
== Summary ==
Series 34098v3 drm/i915: Enable fastboot, v3!
https://patchwork.freedesktop.org/api/1.0/series/34098/revisions/3/mbox/
Test debugfs_test:
Subgroup read_all_entries:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test gem_ctx_create:
Subgroup basic-files:
pass -> FAIL (fi-bsw-n3050) fdo#103706
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test gem_ringfill:
Subgroup basic-default-hang:
dmesg-warn -> PASS (fi-blb-e6850) fdo#101600
Test kms_busy:
Subgroup basic-flip-a:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-b:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-c:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-busy-flip-before-cursor-legacy:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-after-cursor-atomic:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-after-cursor-legacy:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-after-cursor-varying-size:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-before-cursor-atomic:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-before-cursor-legacy:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-before-cursor-varying-size:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-vs-modeset:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-flip-vs-wf_vblank:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup basic-plain-flip:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test kms_frontbuffer_tracking:
Subgroup basic:
pass -> DMESG-WARN (fi-bdw-5557u) fdo#102473
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup hang-read-crc-pipe-b:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup hang-read-crc-pipe-c:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
pass -> FAIL (fi-cfl-s2)
Subgroup nonblocking-crc-pipe-a:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup nonblocking-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup nonblocking-crc-pipe-b:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup nonblocking-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup nonblocking-crc-pipe-c:
pass -> DMESG-WARN (fi-bdw-5557u)
pass -> DMESG-WARN (fi-bdw-gvtdvm)
Subgroup nonblocking-crc-pipe-c-frame-sequence:
pass -> DMESG-WARN (fi-bdw-5557u)
WARNING: Long output truncated
fi-pnv-d510 failed to collect. IGT log at Patchwork_7235/fi-pnv-d510/igt.log
368de251cb3d8b5a69ea4e2731a2b53d329c8485 drm-tip: 2017y-11m-22d-11h-27m-26s UTC integration manifest
543608acf2b4 drm/i915: Re-enable fastboot by default
243c803c4365 drm/i915: Enable IPS with only sprite plane visible too, v3.
3d127369b401 drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7235/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
2017-11-22 15:34 ` Ville Syrjälä
@ 2017-11-22 18:39 ` Maarten Lankhorst
2017-11-23 14:36 ` Ville Syrjälä
0 siblings, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-22 18:39 UTC (permalink / raw)
To: intel-gfx
ips_enabled was used as a variable of whether IPS can be enabled or not,
but should be used to test whether IPS is actually enabled.
Changes since v1:
- Call needs_modeset on new crtc state. (Ville)
- IPS can be enabled with sprite plane enabled too. (Ville)
- Fix CDCLK vs IPS workaround. (Ville)
Changes since v2:
- Only re-enable fastset when inheriting mode. (Ville)
- Put the conditions for enabling and disabling IPS in a helper.
Changes since v3:
- Keep the max_cdclk workaround working. (Ville)
- Also check logical cdclk out of paranoia.
- Remove planes check from IPS disable function for initial disable.
- Remove assert_plane_enabled/disabled checks and use
crtc_state->active_planes for hsw_enable_ips only, always allow
calling hsw_disable_ips to disable it initially in hw.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 168 ++++++++++++++++++++++------------
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
4 files changed, 109 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index e8884c2ade98..30affa7903d7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
- if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
+ if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
/* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60d0a2d8534c..95df5c2128b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -489,7 +489,7 @@ static const struct intel_limit intel_limits_bxt = {
};
static bool
-needs_modeset(struct drm_crtc_state *state)
+needs_modeset(const struct drm_crtc_state *state)
{
return drm_atomic_crtc_needs_modeset(state);
}
@@ -4833,7 +4833,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- if (!crtc->config->ips_enabled)
+ if (!crtc_state->ips_enabled)
return;
/*
@@ -4841,8 +4841,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
* This function is called from post_plane_update, which is run after
* a vblank wait.
*/
-
- assert_plane_enabled(to_intel_plane(crtc->base.primary));
+ WARN_ON(!(crtc_state->active_planes & ~BIT(PLANE_CURSOR)));
if (IS_BROADWELL(dev_priv)) {
mutex_lock(&dev_priv->pcu_lock);
@@ -4877,8 +4876,6 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
if (!crtc_state->ips_enabled)
return;
- assert_plane_enabled(to_intel_plane(crtc->base.primary));
-
if (IS_BROADWELL(dev_priv)) {
mutex_lock(&dev_priv->pcu_lock);
WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
@@ -4931,14 +4928,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_enable_ips(new_crtc_state);
-
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
* So don't enable underrun reporting before at least some planes
@@ -4954,10 +4943,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
intel_check_pch_fifo_underruns(dev_priv);
}
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
- const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4966,32 +4954,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
- * So diasble underrun reporting before all the planes get disabled.
- * FIXME: Need to fix the logic to work when we turn off all planes
- * but leave the pipe running.
+ * So disable underrun reporting before all the planes get disabled.
*/
if (IS_GEN2(dev_priv))
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- int pipe = intel_crtc->pipe;
-
- intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+ hsw_disable_ips(to_intel_crtc_state(crtc->state));
/*
* Vblank time updates from the shadow to live plane control register
@@ -5007,6 +4975,38 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
intel_wait_for_vblank(dev_priv, pipe);
}
+static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+ if (!old_crtc_state->ips_enabled)
+ return false;
+
+ if (needs_modeset(&new_crtc_state->base))
+ return true;
+
+ return !new_crtc_state->ips_enabled;
+}
+
+static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+ if (!new_crtc_state->ips_enabled)
+ return false;
+
+ if (needs_modeset(&new_crtc_state->base))
+ return true;
+
+ /*
+ * We can't read out IPS on broadwell, assume the worst and
+ * forcibly enable IPS on the first fastset.
+ */
+ if (new_crtc_state->update_pipe &&
+ old_crtc_state->base.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
+ return true;
+
+ return !old_crtc_state->ips_enabled;
+}
+
static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
@@ -5023,6 +5023,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
if (pipe_config->update_wm_post && pipe_config->base.active)
intel_update_watermarks(crtc);
+ if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
+ hsw_enable_ips(pipe_config);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
@@ -5053,6 +5056,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
struct intel_atomic_state *old_intel_state =
to_intel_atomic_state(old_state);
+ if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
+ hsw_disable_ips(old_crtc_state);
+
if (old_pri_state) {
struct intel_plane_state *primary_state =
intel_atomic_get_new_plane_state(old_intel_state,
@@ -5061,10 +5067,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
to_intel_plane_state(old_pri_state);
intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
- if (old_primary_state->base.visible &&
+ /*
+ * Gen2 reports pipe underruns whenever all planes are disabled.
+ * So disable underrun reporting before all the planes get disabled.
+ */
+ if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
(modeset || !primary_state->base.visible))
- intel_pre_disable_primary(&crtc->base, old_crtc_state);
+ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
}
/*
@@ -6195,18 +6204,20 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
return ret;
}
-static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
- struct intel_crtc_state *pipe_config)
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
{
- if (pipe_config->ips_force_disable)
+ struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+ /* IPS only exists on ULT machines and is tied to pipe A. */
+ if (!hsw_crtc_supports_ips(crtc))
return false;
- if (pipe_config->pipe_bpp > 24)
+ if (!i915_modparams.enable_ips)
return false;
- /* HSW can handle pixel rate up to cdclk? */
- if (IS_HASWELL(dev_priv))
- return true;
+ if (pipe_config->pipe_bpp > 24)
+ return false;
/*
* We compare against max which means we must take
@@ -6215,19 +6226,41 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
*
* Should measure whether using a lower cdclk w/o IPS
*/
- return pipe_config->pixel_rate <=
- dev_priv->max_cdclk_freq * 95 / 100;
+ if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
+ dev_priv->max_cdclk_freq * 95 / 100)
+ return false;
+
+ return true;
}
-static void hsw_compute_ips_config(struct intel_crtc *crtc,
- struct intel_crtc_state *pipe_config)
+static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
{
- struct drm_device *dev = crtc->base.dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_i915_private *dev_priv =
+ to_i915(pipe_config->base.crtc->dev);
+ struct intel_atomic_state *intel_state =
+ to_intel_atomic_state(pipe_config->base.state);
+
+ if (!hsw_pipe_config_ips_capable(pipe_config))
+ return false;
+
+ if (pipe_config->ips_force_disable)
+ return false;
+
+ /*
+ * FIXME IPS should be fine as long as one plane is
+ * enabled, but in practice it seems to have problems
+ * when going from primary only to sprite only and vice
+ * versa.
+ */
+ if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ return false;
- pipe_config->ips_enabled = i915_modparams.enable_ips &&
- hsw_crtc_supports_ips(crtc) &&
- pipe_config_supports_ips(dev_priv, pipe_config);
+ /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
+ if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
+ intel_state->cdclk.logical.cdclk * 95 / 100)
+ return false;
+
+ return true;
}
static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
@@ -6345,9 +6378,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
intel_crtc_compute_pixel_rate(pipe_config);
- if (HAS_IPS(dev_priv))
- hsw_compute_ips_config(crtc, pipe_config);
-
if (pipe_config->has_pch_encoder)
return ironlake_fdi_compute_config(crtc, pipe_config);
@@ -9183,6 +9213,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
ironlake_get_pfit_config(crtc, pipe_config);
}
+ if (hsw_crtc_supports_ips(crtc)) {
+ if (IS_HASWELL(dev_priv))
+ pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
+ else {
+ /*
+ * We cannot readout IPS state on broadwell, set to
+ * true so we can set it to a defined state on first
+ * commit.
+ */
+ pipe_config->ips_enabled = true;
+ }
+ }
+
if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
pipe_config->pixel_multiplier =
@@ -10436,6 +10479,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
pipe_config);
}
+ if (HAS_IPS(dev_priv))
+ pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
+
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 635a96fcd788..4f4cf5128e0c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
bool intel_crtc_active(struct intel_crtc *crtc);
+bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
enum intel_display_power_domain intel_port_to_power_domain(enum port port);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 61641d479b93..1f5cd572a7ff 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
* completely disable it.
*/
pipe_config->ips_force_disable = enable;
- if (pipe_config->ips_enabled == enable)
- pipe_config->base.connectors_changed = true;
}
if (IS_HASWELL(dev_priv)) {
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] drm/i915: Enable IPS with only sprite plane visible too, v4.
2017-11-22 15:28 ` Ville Syrjälä
@ 2017-11-22 18:39 ` Maarten Lankhorst
2017-11-23 14:37 ` Ville Syrjälä
0 siblings, 1 reply; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-22 18:39 UTC (permalink / raw)
To: intel-gfx
This comment predates atomic, and I think with the way we currently
track IPS, it's safe to enable this for the case we switch too.
Changes since v1:
- Keep IPS enabled when switching planes.
Changes since v2:
- Enable IPS when at least one plane is enabled. (Ville)
Changes since v3:
- Actually do what was advertised in v3, sigh! (Ville, CI)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 95df5c2128b4..3b4ae922be3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6246,13 +6246,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
if (pipe_config->ips_force_disable)
return false;
- /*
- * FIXME IPS should be fine as long as one plane is
- * enabled, but in practice it seems to have problems
- * when going from primary only to sprite only and vice
- * versa.
- */
- if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
+ /* IPS should be fine as long as at least one plane is enabled. */
+ if (!(pipe_config->active_planes & ~BIT(PLANE_CURSOR)))
return false;
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! (rev5)
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
` (5 preceding siblings ...)
2017-11-22 15:38 ` ✗ Fi.CI.BAT: failure for drm/i915: Enable fastboot, v3! (rev3) Patchwork
@ 2017-11-22 19:27 ` Patchwork
2017-11-22 21:11 ` ✓ Fi.CI.IGT: " Patchwork
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2017-11-22 19:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable fastboot, v3! (rev5)
URL : https://patchwork.freedesktop.org/series/34098/
State : success
== Summary ==
Series 34098v5 drm/i915: Enable fastboot, v3!
https://patchwork.freedesktop.org/api/1.0/series/34098/revisions/5/mbox/
Test gem_exec_reloc:
Subgroup basic-cpu-active:
pass -> FAIL (fi-gdg-551) fdo#102582 +4
fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:449s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:463s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:380s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:545s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:278s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:506s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:513s
fi-byt-j1900 total:289 pass:254 dwarn:0 dfail:0 fail:0 skip:35 time:499s
fi-byt-n2820 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:495s
fi-cfl-s2 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:620s
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:437s
fi-gdg-551 total:289 pass:173 dwarn:1 dfail:0 fail:6 skip:109 time:266s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:546s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:429s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:438s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:430s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:477s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:463s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:482s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:533s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:477s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:536s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:581s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:545s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:561s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:520s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:504s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:459s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:568s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:428s
Blacklisted hosts:
fi-cnl-y total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:554s
fi-glk-dsi total:22 pass:21 dwarn:0 dfail:0 fail:0 skip:0
a3f080ee939e8befca924f1ab83deeabfa7ff98e drm-tip: 2017y-11m-22d-18h-37m-24s UTC integration manifest
fb4472672320 drm/i915: Re-enable fastboot by default
ccd6b721bda1 drm/i915: Enable IPS with only sprite plane visible too, v4.
17c63ce32a99 drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7239/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Enable fastboot, v3! (rev5)
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
` (6 preceding siblings ...)
2017-11-22 19:27 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! (rev5) Patchwork
@ 2017-11-22 21:11 ` Patchwork
7 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2017-11-22 21:11 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable fastboot, v3! (rev5)
URL : https://patchwork.freedesktop.org/series/34098/
State : success
== Summary ==
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
fail -> PASS (shard-snb) fdo#101623
Subgroup fbc-1p-pri-indfb-multidraw:
incomplete -> PASS (shard-hsw) fdo#103167
Test kms_cursor_crc:
Subgroup cursor-128x128-suspend:
pass -> SKIP (shard-hsw) fdo#103375
Test drv_module_reload:
Subgroup basic-no-display:
pass -> DMESG-WARN (shard-hsw) fdo#102707
Test gem_busy:
Subgroup close-race:
fail -> PASS (shard-snb) fdo#103829
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#103829 https://bugs.freedesktop.org/show_bug.cgi?id=103829
shard-hsw total:2667 pass:1525 dwarn:2 dfail:0 fail:17 skip:1123 time:9411s
shard-snb total:2646 pass:1289 dwarn:1 dfail:0 fail:18 skip:1337 time:7953s
Blacklisted hosts:
shard-apl total:2573 pass:1618 dwarn:4 dfail:0 fail:21 skip:927 time:12654s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7239/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
2017-11-22 18:39 ` [PATCH 1/2] " Maarten Lankhorst
@ 2017-11-23 14:36 ` Ville Syrjälä
2017-11-30 15:54 ` Maarten Lankhorst
0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-23 14:36 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Wed, Nov 22, 2017 at 07:39:01PM +0100, Maarten Lankhorst wrote:
> ips_enabled was used as a variable of whether IPS can be enabled or not,
> but should be used to test whether IPS is actually enabled.
>
> Changes since v1:
> - Call needs_modeset on new crtc state. (Ville)
> - IPS can be enabled with sprite plane enabled too. (Ville)
> - Fix CDCLK vs IPS workaround. (Ville)
> Changes since v2:
> - Only re-enable fastset when inheriting mode. (Ville)
> - Put the conditions for enabling and disabling IPS in a helper.
> Changes since v3:
> - Keep the max_cdclk workaround working. (Ville)
> - Also check logical cdclk out of paranoia.
> - Remove planes check from IPS disable function for initial disable.
> - Remove assert_plane_enabled/disabled checks and use
> crtc_state->active_planes for hsw_enable_ips only, always allow
> calling hsw_disable_ips to disable it initially in hw.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 168 ++++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
> 4 files changed, 109 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index e8884c2ade98..30affa7903d7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> + if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
> min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
>
> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 60d0a2d8534c..95df5c2128b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -489,7 +489,7 @@ static const struct intel_limit intel_limits_bxt = {
> };
>
> static bool
> -needs_modeset(struct drm_crtc_state *state)
> +needs_modeset(const struct drm_crtc_state *state)
> {
> return drm_atomic_crtc_needs_modeset(state);
> }
> @@ -4833,7 +4833,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (!crtc->config->ips_enabled)
> + if (!crtc_state->ips_enabled)
> return;
>
> /*
> @@ -4841,8 +4841,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> * This function is called from post_plane_update, which is run after
> * a vblank wait.
> */
> -
> - assert_plane_enabled(to_intel_plane(crtc->base.primary));
> + WARN_ON(!(crtc_state->active_planes & ~BIT(PLANE_CURSOR)));
>
> if (IS_BROADWELL(dev_priv)) {
> mutex_lock(&dev_priv->pcu_lock);
> @@ -4877,8 +4876,6 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
> if (!crtc_state->ips_enabled)
> return;
>
> - assert_plane_enabled(to_intel_plane(crtc->base.primary));
> -
> if (IS_BROADWELL(dev_priv)) {
> mutex_lock(&dev_priv->pcu_lock);
> WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
> @@ -4931,14 +4928,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_enable_ips(new_crtc_state);
> -
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> * So don't enable underrun reporting before at least some planes
> @@ -4954,10 +4943,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> intel_check_pch_fifo_underruns(dev_priv);
> }
>
> -/* FIXME move all this to pre_plane_update() with proper state tracking */
> +/* FIXME get rid of this and use pre_plane_update */
> static void
> -intel_pre_disable_primary(struct drm_crtc *crtc,
> - const struct intel_crtc_state *old_crtc_state)
> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -4966,32 +4954,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>
> /*
> * Gen2 reports pipe underruns whenever all planes are disabled.
> - * So diasble underrun reporting before all the planes get disabled.
> - * FIXME: Need to fix the logic to work when we turn off all planes
> - * but leave the pipe running.
> + * So disable underrun reporting before all the planes get disabled.
> */
> if (IS_GEN2(dev_priv))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - hsw_disable_ips(old_crtc_state);
> -}
> -
> -/* FIXME get rid of this and use pre_plane_update */
> -static void
> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> -
> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> + hsw_disable_ips(to_intel_crtc_state(crtc->state));
>
> /*
> * Vblank time updates from the shadow to live plane control register
> @@ -5007,6 +4975,38 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> intel_wait_for_vblank(dev_priv, pipe);
> }
>
> +static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state)
> +{
> + if (!old_crtc_state->ips_enabled)
> + return false;
> +
> + if (needs_modeset(&new_crtc_state->base))
> + return true;
> +
> + return !new_crtc_state->ips_enabled;
> +}
> +
> +static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state)
> +{
> + if (!new_crtc_state->ips_enabled)
> + return false;
> +
> + if (needs_modeset(&new_crtc_state->base))
> + return true;
> +
> + /*
> + * We can't read out IPS on broadwell, assume the worst and
> + * forcibly enable IPS on the first fastset.
> + */
> + if (new_crtc_state->update_pipe &&
Do we even need the update_pipe check now? Ie. why would we skip this if the
initial update happens to update just the planes?
> + old_crtc_state->base.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
> + return true;
> +
> + return !old_crtc_state->ips_enabled;
> +}
> +
> static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> @@ -5023,6 +5023,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> if (pipe_config->update_wm_post && pipe_config->base.active)
> intel_update_watermarks(crtc);
>
> + if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
> + hsw_enable_ips(pipe_config);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
> @@ -5053,6 +5056,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> struct intel_atomic_state *old_intel_state =
> to_intel_atomic_state(old_state);
>
> + if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
> + hsw_disable_ips(old_crtc_state);
> +
> if (old_pri_state) {
> struct intel_plane_state *primary_state =
> intel_atomic_get_new_plane_state(old_intel_state,
> @@ -5061,10 +5067,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> to_intel_plane_state(old_pri_state);
>
> intel_fbc_pre_update(crtc, pipe_config, primary_state);
> -
> - if (old_primary_state->base.visible &&
> + /*
> + * Gen2 reports pipe underruns whenever all planes are disabled.
> + * So disable underrun reporting before all the planes get disabled.
> + */
> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
> (modeset || !primary_state->base.visible))
> - intel_pre_disable_primary(&crtc->base, old_crtc_state);
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> }
>
> /*
> @@ -6195,18 +6204,20 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> return ret;
> }
>
> -static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> - struct intel_crtc_state *pipe_config)
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
Since you're touching pretty much everything, how about
s/pipe_config/crtc_state/ while at it?
> {
> - if (pipe_config->ips_force_disable)
> + struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> + /* IPS only exists on ULT machines and is tied to pipe A. */
> + if (!hsw_crtc_supports_ips(crtc))
> return false;
>
> - if (pipe_config->pipe_bpp > 24)
> + if (!i915_modparams.enable_ips)
> return false;
>
> - /* HSW can handle pixel rate up to cdclk? */
> - if (IS_HASWELL(dev_priv))
> - return true;
> + if (pipe_config->pipe_bpp > 24)
> + return false;
>
> /*
> * We compare against max which means we must take
> @@ -6215,19 +6226,41 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> *
> * Should measure whether using a lower cdclk w/o IPS
> */
> - return pipe_config->pixel_rate <=
> - dev_priv->max_cdclk_freq * 95 / 100;
> + if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
> + dev_priv->max_cdclk_freq * 95 / 100)
Splitting the line like that makes it pretty hard to read.
if (IS_BROADWELL(...) &&
pipe_config->pixel_rate > dev_priv->max_cdclk_freq * 95 / 100)
would look much better IMO.
> + return false;
> +
> + return true;
> }
>
> -static void hsw_compute_ips_config(struct intel_crtc *crtc,
> - struct intel_crtc_state *pipe_config)
> +static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
s/pipe_config/crtc_state/ again
> {
> - struct drm_device *dev = crtc->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_i915_private *dev_priv =
> + to_i915(pipe_config->base.crtc->dev);
> + struct intel_atomic_state *intel_state =
> + to_intel_atomic_state(pipe_config->base.state);
> +
> + if (!hsw_pipe_config_ips_capable(pipe_config))
> + return false;
> +
> + if (pipe_config->ips_force_disable)
> + return false;
> +
> + /*
> + * FIXME IPS should be fine as long as one plane is
> + * enabled, but in practice it seems to have problems
> + * when going from primary only to sprite only and vice
> + * versa.
> + */
> + if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + return false;
>
> - pipe_config->ips_enabled = i915_modparams.enable_ips &&
> - hsw_crtc_supports_ips(crtc) &&
> - pipe_config_supports_ips(dev_priv, pipe_config);
> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> + if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
> + intel_state->cdclk.logical.cdclk * 95 / 100)
> + return false;
Another hard to read split.
Apart from those nits, this lgtm so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> +
> + return true;
> }
>
> static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> @@ -6345,9 +6378,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>
> intel_crtc_compute_pixel_rate(pipe_config);
>
> - if (HAS_IPS(dev_priv))
> - hsw_compute_ips_config(crtc, pipe_config);
> -
> if (pipe_config->has_pch_encoder)
> return ironlake_fdi_compute_config(crtc, pipe_config);
>
> @@ -9183,6 +9213,19 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> ironlake_get_pfit_config(crtc, pipe_config);
> }
>
> + if (hsw_crtc_supports_ips(crtc)) {
> + if (IS_HASWELL(dev_priv))
> + pipe_config->ips_enabled = I915_READ(IPS_CTL) & IPS_ENABLE;
> + else {
> + /*
> + * We cannot readout IPS state on broadwell, set to
> + * true so we can set it to a defined state on first
> + * commit.
> + */
> + pipe_config->ips_enabled = true;
> + }
> + }
> +
> if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> pipe_config->pixel_multiplier =
> @@ -10436,6 +10479,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> pipe_config);
> }
>
> + if (HAS_IPS(dev_priv))
> + pipe_config->ips_enabled = hsw_compute_ips_config(pipe_config);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 635a96fcd788..4f4cf5128e0c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1485,6 +1485,7 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
>
> bool intel_crtc_active(struct intel_crtc *crtc);
> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config);
> void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
> void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
> enum intel_display_power_domain intel_port_to_power_domain(enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 61641d479b93..1f5cd572a7ff 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> * completely disable it.
> */
> pipe_config->ips_force_disable = enable;
> - if (pipe_config->ips_enabled == enable)
> - pipe_config->base.connectors_changed = true;
> }
>
> if (IS_HASWELL(dev_priv)) {
> --
> 2.15.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable IPS with only sprite plane visible too, v4.
2017-11-22 18:39 ` [PATCH 2/2] drm/i915: Enable IPS with only sprite plane visible too, v4 Maarten Lankhorst
@ 2017-11-23 14:37 ` Ville Syrjälä
0 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2017-11-23 14:37 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Wed, Nov 22, 2017 at 07:39:06PM +0100, Maarten Lankhorst wrote:
> This comment predates atomic, and I think with the way we currently
> track IPS, it's safe to enable this for the case we switch too.
>
> Changes since v1:
> - Keep IPS enabled when switching planes.
> Changes since v2:
> - Enable IPS when at least one plane is enabled. (Ville)
> Changes since v3:
> - Actually do what was advertised in v3, sigh! (Ville, CI)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 95df5c2128b4..3b4ae922be3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6246,13 +6246,8 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> if (pipe_config->ips_force_disable)
> return false;
>
> - /*
> - * FIXME IPS should be fine as long as one plane is
> - * enabled, but in practice it seems to have problems
> - * when going from primary only to sprite only and vice
> - * versa.
> - */
> - if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
> + /* IPS should be fine as long as at least one plane is enabled. */
> + if (!(pipe_config->active_planes & ~BIT(PLANE_CURSOR)))
> return false;
Yep, now it makes sense ;)
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> --
> 2.15.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3.
2017-11-23 14:36 ` Ville Syrjälä
@ 2017-11-30 15:54 ` Maarten Lankhorst
0 siblings, 0 replies; 22+ messages in thread
From: Maarten Lankhorst @ 2017-11-30 15:54 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 23-11-17 om 15:36 schreef Ville Syrjälä:
> On Wed, Nov 22, 2017 at 07:39:01PM +0100, Maarten Lankhorst wrote:
>> ips_enabled was used as a variable of whether IPS can be enabled or not,
>> but should be used to test whether IPS is actually enabled.
>>
>> Changes since v1:
>> - Call needs_modeset on new crtc state. (Ville)
>> - IPS can be enabled with sprite plane enabled too. (Ville)
>> - Fix CDCLK vs IPS workaround. (Ville)
>> Changes since v2:
>> - Only re-enable fastset when inheriting mode. (Ville)
>> - Put the conditions for enabling and disabling IPS in a helper.
>> Changes since v3:
>> - Keep the max_cdclk workaround working. (Ville)
>> - Also check logical cdclk out of paranoia.
>> - Remove planes check from IPS disable function for initial disable.
>> - Remove assert_plane_enabled/disabled checks and use
>> crtc_state->active_planes for hsw_enable_ips only, always allow
>> calling hsw_disable_ips to disable it initially in hw.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 168 ++++++++++++++++++++++------------
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 -
>> 4 files changed, 109 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>> index e8884c2ade98..30affa7903d7 100644
>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>> @@ -1896,7 +1896,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>> min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
>>
>> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
>> + if (IS_BROADWELL(dev_priv) && hsw_pipe_config_ips_capable(crtc_state))
>> min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
>>
>> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 60d0a2d8534c..95df5c2128b4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -489,7 +489,7 @@ static const struct intel_limit intel_limits_bxt = {
>> };
>>
>> static bool
>> -needs_modeset(struct drm_crtc_state *state)
>> +needs_modeset(const struct drm_crtc_state *state)
>> {
>> return drm_atomic_crtc_needs_modeset(state);
>> }
>> @@ -4833,7 +4833,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>>
>> - if (!crtc->config->ips_enabled)
>> + if (!crtc_state->ips_enabled)
>> return;
>>
>> /*
>> @@ -4841,8 +4841,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>> * This function is called from post_plane_update, which is run after
>> * a vblank wait.
>> */
>> -
>> - assert_plane_enabled(to_intel_plane(crtc->base.primary));
>> + WARN_ON(!(crtc_state->active_planes & ~BIT(PLANE_CURSOR)));
>>
>> if (IS_BROADWELL(dev_priv)) {
>> mutex_lock(&dev_priv->pcu_lock);
>> @@ -4877,8 +4876,6 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
>> if (!crtc_state->ips_enabled)
>> return;
>>
>> - assert_plane_enabled(to_intel_plane(crtc->base.primary));
>> -
>> if (IS_BROADWELL(dev_priv)) {
>> mutex_lock(&dev_priv->pcu_lock);
>> WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
>> @@ -4931,14 +4928,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> int pipe = intel_crtc->pipe;
>>
>> - /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> - */
>> - hsw_enable_ips(new_crtc_state);
>> -
>> /*
>> * Gen2 reports pipe underruns whenever all planes are disabled.
>> * So don't enable underrun reporting before at least some planes
>> @@ -4954,10 +4943,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>> intel_check_pch_fifo_underruns(dev_priv);
>> }
>>
>> -/* FIXME move all this to pre_plane_update() with proper state tracking */
>> +/* FIXME get rid of this and use pre_plane_update */
>> static void
>> -intel_pre_disable_primary(struct drm_crtc *crtc,
>> - const struct intel_crtc_state *old_crtc_state)
>> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -4966,32 +4954,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>>
>> /*
>> * Gen2 reports pipe underruns whenever all planes are disabled.
>> - * So diasble underrun reporting before all the planes get disabled.
>> - * FIXME: Need to fix the logic to work when we turn off all planes
>> - * but leave the pipe running.
>> + * So disable underrun reporting before all the planes get disabled.
>> */
>> if (IS_GEN2(dev_priv))
>> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>>
>> - /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> - */
>> - hsw_disable_ips(old_crtc_state);
>> -}
>> -
>> -/* FIXME get rid of this and use pre_plane_update */
>> -static void
>> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> -{
>> - struct drm_device *dev = crtc->dev;
>> - struct drm_i915_private *dev_priv = to_i915(dev);
>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> - int pipe = intel_crtc->pipe;
>> -
>> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>> + hsw_disable_ips(to_intel_crtc_state(crtc->state));
>>
>> /*
>> * Vblank time updates from the shadow to live plane control register
>> @@ -5007,6 +4975,38 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> intel_wait_for_vblank(dev_priv, pipe);
>> }
>>
>> +static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
>> + const struct intel_crtc_state *new_crtc_state)
>> +{
>> + if (!old_crtc_state->ips_enabled)
>> + return false;
>> +
>> + if (needs_modeset(&new_crtc_state->base))
>> + return true;
>> +
>> + return !new_crtc_state->ips_enabled;
>> +}
>> +
>> +static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
>> + const struct intel_crtc_state *new_crtc_state)
>> +{
>> + if (!new_crtc_state->ips_enabled)
>> + return false;
>> +
>> + if (needs_modeset(&new_crtc_state->base))
>> + return true;
>> +
>> + /*
>> + * We can't read out IPS on broadwell, assume the worst and
>> + * forcibly enable IPS on the first fastset.
>> + */
>> + if (new_crtc_state->update_pipe &&
> Do we even need the update_pipe check now? Ie. why would we skip this if the
> initial update happens to update just the planes?
The first update may update planes, but that keeps the I915_MODE_FLAG_INHERITED flag,
we can either choose to keep updating IPS until first update_pipe is called, or only call
ips_enabled after first update_pipe.
>> + old_crtc_state->base.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
>> + return true;
>> +
>> + return !old_crtc_state->ips_enabled;
>> +}
>> +
>> static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>> {
>> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>> @@ -5023,6 +5023,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>> if (pipe_config->update_wm_post && pipe_config->base.active)
>> intel_update_watermarks(crtc);
>>
>> + if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
>> + hsw_enable_ips(pipe_config);
>> +
>> if (old_pri_state) {
>> struct intel_plane_state *primary_state =
>> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
>> @@ -5053,6 +5056,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>> struct intel_atomic_state *old_intel_state =
>> to_intel_atomic_state(old_state);
>>
>> + if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
>> + hsw_disable_ips(old_crtc_state);
>> +
>> if (old_pri_state) {
>> struct intel_plane_state *primary_state =
>> intel_atomic_get_new_plane_state(old_intel_state,
>> @@ -5061,10 +5067,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>> to_intel_plane_state(old_pri_state);
>>
>> intel_fbc_pre_update(crtc, pipe_config, primary_state);
>> -
>> - if (old_primary_state->base.visible &&
>> + /*
>> + * Gen2 reports pipe underruns whenever all planes are disabled.
>> + * So disable underrun reporting before all the planes get disabled.
>> + */
>> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
>> (modeset || !primary_state->base.visible))
>> - intel_pre_disable_primary(&crtc->base, old_crtc_state);
>> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>> }
>>
>> /*
>> @@ -6195,18 +6204,20 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>> return ret;
>> }
>>
>> -static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>> - struct intel_crtc_state *pipe_config)
>> +bool hsw_pipe_config_ips_capable(const struct intel_crtc_state *pipe_config)
> Since you're touching pretty much everything, how about
> s/pipe_config/crtc_state/ while at it?
>
>> {
>> - if (pipe_config->ips_force_disable)
>> + struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +
>> + /* IPS only exists on ULT machines and is tied to pipe A. */
>> + if (!hsw_crtc_supports_ips(crtc))
>> return false;
>>
>> - if (pipe_config->pipe_bpp > 24)
>> + if (!i915_modparams.enable_ips)
>> return false;
>>
>> - /* HSW can handle pixel rate up to cdclk? */
>> - if (IS_HASWELL(dev_priv))
>> - return true;
>> + if (pipe_config->pipe_bpp > 24)
>> + return false;
>>
>> /*
>> * We compare against max which means we must take
>> @@ -6215,19 +6226,41 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>> *
>> * Should measure whether using a lower cdclk w/o IPS
>> */
>> - return pipe_config->pixel_rate <=
>> - dev_priv->max_cdclk_freq * 95 / 100;
>> + if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
>> + dev_priv->max_cdclk_freq * 95 / 100)
> Splitting the line like that makes it pretty hard to read.
>
> if (IS_BROADWELL(...) &&
> pipe_config->pixel_rate > dev_priv->max_cdclk_freq * 95 / 100)
>
> would look much better IMO.
>
>> + return false;
>> +
>> + return true;
>> }
>>
>> -static void hsw_compute_ips_config(struct intel_crtc *crtc,
>> - struct intel_crtc_state *pipe_config)
>> +static bool hsw_compute_ips_config(struct intel_crtc_state *pipe_config)
> s/pipe_config/crtc_state/ again
>
>> {
>> - struct drm_device *dev = crtc->base.dev;
>> - struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_i915_private *dev_priv =
>> + to_i915(pipe_config->base.crtc->dev);
>> + struct intel_atomic_state *intel_state =
>> + to_intel_atomic_state(pipe_config->base.state);
>> +
>> + if (!hsw_pipe_config_ips_capable(pipe_config))
>> + return false;
>> +
>> + if (pipe_config->ips_force_disable)
>> + return false;
>> +
>> + /*
>> + * FIXME IPS should be fine as long as one plane is
>> + * enabled, but in practice it seems to have problems
>> + * when going from primary only to sprite only and vice
>> + * versa.
>> + */
>> + if (!(pipe_config->active_planes & BIT(PLANE_PRIMARY)))
>> + return false;
>>
>> - pipe_config->ips_enabled = i915_modparams.enable_ips &&
>> - hsw_crtc_supports_ips(crtc) &&
>> - pipe_config_supports_ips(dev_priv, pipe_config);
>> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> + if (IS_BROADWELL(dev_priv) && pipe_config->pixel_rate >
>> + intel_state->cdclk.logical.cdclk * 95 / 100)
>> + return false;
> Another hard to read split.
>
> Apart from those nits, this lgtm so
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Thanks, moved the split and did s/pipe_config/crtc_state/ (including on function name).
Pushed.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-11-30 15:54 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-20 12:23 [PATCH v2 0/3] drm/i915: Enable fastboot, v3! Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v2 Maarten Lankhorst
2017-11-20 14:01 ` Ville Syrjälä
2017-11-20 16:21 ` Maarten Lankhorst
2017-11-22 15:07 ` [PATCH] drm/i915: Make ips_enabled a property depending on whether IPS is enabled, v3 Maarten Lankhorst
2017-11-22 15:34 ` Ville Syrjälä
2017-11-22 18:39 ` [PATCH 1/2] " Maarten Lankhorst
2017-11-23 14:36 ` Ville Syrjälä
2017-11-30 15:54 ` Maarten Lankhorst
2017-11-20 12:23 ` [PATCH v2 2/3] drm/i915: Enable IPS with only sprite plane visible too, v2 Maarten Lankhorst
2017-11-20 13:27 ` Ville Syrjälä
2017-11-21 9:27 ` Maarten Lankhorst
2017-11-22 15:08 ` [PATCH] drm/i915: Enable IPS with only sprite plane visible too, v3 Maarten Lankhorst
2017-11-22 15:28 ` Ville Syrjälä
2017-11-22 18:39 ` [PATCH 2/2] drm/i915: Enable IPS with only sprite plane visible too, v4 Maarten Lankhorst
2017-11-23 14:37 ` Ville Syrjälä
2017-11-20 12:23 ` [PATCH (resend) 3/3] drm/i915: Re-enable fastboot by default Maarten Lankhorst
2017-11-20 13:05 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! Patchwork
2017-11-20 14:15 ` ✓ Fi.CI.IGT: " Patchwork
2017-11-22 15:38 ` ✗ Fi.CI.BAT: failure for drm/i915: Enable fastboot, v3! (rev3) Patchwork
2017-11-22 19:27 ` ✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v3! (rev5) Patchwork
2017-11-22 21:11 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.