* [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset.
@ 2016-11-08 12:55 Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
` (11 more replies)
0 siblings, 12 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
These are the remaining watermark fixes required before nonblocking
modeset can be enabled, plus the patches required to enable
nonblocking modeset support in i915.
Maarten Lankhorst (11):
drm/i915: Add a atomic evasion step to watermark programming, v3.
drm/i915/gen9+: Program watermarks as a separate step during evasion,
v3.
drm/i915/gen9+: Preserve old allocation from crtc_state.
drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
drm/i915/gen9+: Do not initialise active_crtcs for !modeset
drm/i915: Convert intel_hdmi to use atomic state
drm/i915: Pass atomic state to intel_audio_codec_enable, v2.
drm/edid: Remove drm_select_eld
drm/i915: Update atomic modeset state synchronously, v2.
drm/i915: Pass atomic state to verify_connector_state
drm/i915: Enable support for nonblocking modeset
drivers/gpu/drm/drm_edid.c | 26 -------
drivers/gpu/drm/i915/i915_drv.h | 16 ++---
drivers/gpu/drm/i915/intel_audio.c | 17 +++--
drivers/gpu/drm/i915/intel_ddi.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 131 +++++++++++++++++------------------
drivers/gpu/drm/i915/intel_dp.c | 11 +--
drivers/gpu/drm/i915/intel_drv.h | 22 ++----
drivers/gpu/drm/i915/intel_hdmi.c | 50 ++++++-------
drivers/gpu/drm/i915/intel_pm.c | 123 ++++++++++++++++----------------
drivers/gpu/drm/i915/intel_sprite.c | 18 -----
include/drm/drm_edid.h | 1 -
11 files changed, 173 insertions(+), 244 deletions(-)
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-10 0:14 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 02/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3 Maarten Lankhorst
` (10 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
Allow the driver to write watermarks during atomic evasion.
This will make it possible to write the watermarks in a cleaner
way on gen9+.
intel_atomic_state is not used here yet, but will be used when
we program all watermarks as a separate step during evasion.
This also writes linetime all the time, while before it was only
done during plane updates. This looks like this could be a bugfix,
but I'm not sure what it affects.
Changes since v1:
- Add comment about atomic evasion to commit message.
- Unwrap I915_WRITE call. (Lyude)
Changes since v2:
- Rename atomic_evade_watermarks to atomic_update_watermarks. (Ville)
- Add line wraps where appropriate, fix grammar in commit message. (Matt)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 9 +++++++--
drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++-------------
drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++--
3 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4735b4177100..00988d716d7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -487,6 +487,7 @@ struct sdvo_device_mapping {
struct intel_connector;
struct intel_encoder;
+struct intel_atomic_state;
struct intel_crtc_state;
struct intel_initial_plane_config;
struct intel_crtc;
@@ -500,8 +501,12 @@ struct drm_i915_display_funcs {
int (*compute_intermediate_wm)(struct drm_device *dev,
struct intel_crtc *intel_crtc,
struct intel_crtc_state *newstate);
- void (*initial_watermarks)(struct intel_crtc_state *cstate);
- void (*optimize_watermarks)(struct intel_crtc_state *cstate);
+ void (*initial_watermarks)(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate);
+ void (*atomic_update_watermarks)(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate);
+ void (*optimize_watermarks)(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate);
int (*compute_global_watermarks)(struct drm_atomic_state *state);
void (*update_wm)(struct intel_crtc *crtc);
int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92ab01f33208..66cf29ac9fe4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5169,7 +5169,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
* us to.
*/
if (dev_priv->display.initial_watermarks != NULL)
- dev_priv->display.initial_watermarks(pipe_config);
+ dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
else if (pipe_config->update_wm_pre)
intel_update_watermarks(crtc);
}
@@ -5383,7 +5383,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
intel_color_load_luts(&pipe_config->base);
if (dev_priv->display.initial_watermarks != NULL)
- dev_priv->display.initial_watermarks(intel_crtc->config);
+ dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
intel_enable_pipe(intel_crtc);
if (intel_crtc->config->has_pch_encoder)
@@ -5489,7 +5489,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
intel_ddi_enable_transcoder_func(crtc);
if (dev_priv->display.initial_watermarks != NULL)
- dev_priv->display.initial_watermarks(pipe_config);
+ dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
else
intel_update_watermarks(intel_crtc);
@@ -14474,7 +14474,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_cstate = to_intel_crtc_state(crtc->state);
if (dev_priv->display.optimize_watermarks)
- dev_priv->display.optimize_watermarks(intel_cstate);
+ dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
}
for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14909,10 +14909,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_crtc_state *intel_cstate =
to_intel_crtc_state(crtc->state);
- struct intel_crtc_state *old_intel_state =
+ struct intel_crtc_state *old_intel_cstate =
to_intel_crtc_state(old_crtc_state);
+ struct intel_atomic_state *old_intel_state =
+ to_intel_atomic_state(old_crtc_state->state);
bool modeset = needs_modeset(crtc->state);
- enum pipe pipe = intel_crtc->pipe;
/* Perform vblank evasion around commit operation */
intel_pipe_update_start(intel_crtc);
@@ -14925,14 +14926,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
intel_color_load_luts(crtc->state);
}
- if (intel_cstate->update_pipe) {
- intel_update_pipe_config(intel_crtc, old_intel_state);
- } else if (INTEL_GEN(dev_priv) >= 9) {
+ if (intel_cstate->update_pipe)
+ intel_update_pipe_config(intel_crtc, old_intel_cstate);
+ else if (INTEL_GEN(dev_priv) >= 9)
skl_detach_scalers(intel_crtc);
- I915_WRITE(PIPE_WM_LINETIME(pipe),
- intel_cstate->wm.skl.optimal.linetime);
- }
+ if (dev_priv->display.atomic_update_watermarks)
+ dev_priv->display.atomic_update_watermarks(old_intel_state, intel_cstate);
}
static void intel_finish_crtc_commit(struct drm_crtc *crtc,
@@ -16411,7 +16411,7 @@ static void sanitize_watermarks(struct drm_device *dev)
struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
cs->wm.need_postvbl_update = true;
- dev_priv->display.optimize_watermarks(cs);
+ dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
}
put_state:
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 88e28c989b9c..9e825a9651a4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4197,6 +4197,17 @@ skl_compute_wm(struct drm_atomic_state *state)
return 0;
}
+static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate)
+{
+ struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+ enum pipe pipe = crtc->pipe;
+
+ I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+}
+
static void skl_update_wm(struct intel_crtc *intel_crtc)
{
struct drm_device *dev = intel_crtc->base.dev;
@@ -4287,7 +4298,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
ilk_write_wm_values(dev_priv, &results);
}
-static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
+static void ilk_initial_watermarks(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate)
{
struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -4298,7 +4310,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
mutex_unlock(&dev_priv->wm.wm_mutex);
}
-static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+static void ilk_optimize_watermarks(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate)
{
struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -7695,6 +7708,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
if (INTEL_GEN(dev_priv) >= 9) {
skl_setup_wm_latency(dev_priv);
dev_priv->display.update_wm = skl_update_wm;
+ dev_priv->display.atomic_update_watermarks = skl_atomic_update_crtc_wm;
dev_priv->display.compute_global_watermarks = skl_compute_wm;
} else if (HAS_PCH_SPLIT(dev_priv)) {
ilk_setup_wm_latency(dev_priv);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 02/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
` (9 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
The watermark updates for SKL style watermarks are no longer done
in the plane callbacks, but are now called in a separate watermark
update function that's called during the same vblank evasion,
before the plane updates.
This also gets rid of the global skl_results, which was required for
keeping track of the current atomic commit.
Changes since v1:
- Move line unwrap to correct patch. (Lyude)
- Make sure we don't regress ILK watermarks. (Matt)
- Rephrase commit message. (Matt)
Changes since v2:
- Fix disable watermark check to use the correct way to determine single
step watermark support.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Lyude <cpaul@redhat.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 7 -------
drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++------------------------
drivers/gpu/drm/i915/intel_drv.h | 7 -------
drivers/gpu/drm/i915/intel_pm.c | 40 ++++++++++++++++--------------------
drivers/gpu/drm/i915/intel_sprite.c | 18 ----------------
5 files changed, 31 insertions(+), 81 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00988d716d7e..547a1a4f0225 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2058,13 +2058,6 @@ struct drm_i915_private {
*/
uint16_t skl_latency[8];
- /*
- * The skl_wm_values structure is a bit too big for stack
- * allocation, so we keep the staging struct where we store
- * intermediate results here instead.
- */
- struct skl_wm_values skl_results;
-
/* current hardware state */
union {
struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 66cf29ac9fe4..abfcaa07bbe3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3382,9 +3382,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
struct drm_framebuffer *fb = plane_state->base.fb;
- const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
- const struct skl_plane_wm *p_wm =
- &crtc_state->wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
u32 plane_ctl;
unsigned int rotation = plane_state->base.rotation;
@@ -3420,9 +3417,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
intel_crtc->adjusted_x = src_x;
intel_crtc->adjusted_y = src_y;
- if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
- skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
-
I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -3455,18 +3449,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
- const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
- /*
- * We only populate skl_results on watermark updates, and if the
- * plane's visiblity isn't actually changing neither is its watermarks.
- */
- if (!crtc->primary->state->visible)
- skl_write_plane_wm(intel_crtc, p_wm,
- &dev_priv->wm.skl_results.ddb, 0);
-
I915_WRITE(PLANE_CTL(pipe, 0), 0);
I915_WRITE(PLANE_SURF(pipe, 0), 0);
POSTING_READ(PLANE_SURF(pipe, 0));
@@ -10848,16 +10832,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
- const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
- const struct skl_plane_wm *p_wm =
- &cstate->wm.skl.optimal.planes[PLANE_CURSOR];
int pipe = intel_crtc->pipe;
uint32_t cntl = 0;
- if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
- skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
-
if (plane_state && plane_state->base.visible) {
cntl = MCURSOR_GAMMA_ENABLE;
switch (plane_state->base.crtc_w) {
@@ -14407,8 +14384,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_check_cpu_fifo_underruns(dev_priv);
intel_check_pch_fifo_underruns(dev_priv);
- if (!crtc->state->active)
- intel_update_watermarks(intel_crtc);
+ if (!crtc->state->active) {
+ /*
+ * Make sure we don't call initial_watermarks
+ * for ILK-style watermark updates.
+ */
+ if (dev_priv->display.atomic_update_watermarks)
+ dev_priv->display.initial_watermarks(intel_state,
+ to_intel_crtc_state(crtc->state));
+ else
+ intel_update_watermarks(intel_crtc);
+ }
}
}
@@ -14603,7 +14589,6 @@ static int intel_atomic_commit(struct drm_device *dev,
drm_atomic_helper_swap_state(state, true);
dev_priv->wm.distrust_bios_wm = false;
- dev_priv->wm.skl_results = intel_state->wm_results;
intel_shared_dpll_commit(state);
intel_atomic_track_fbs(state);
@@ -14919,7 +14904,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
intel_pipe_update_start(intel_crtc);
if (modeset)
- return;
+ goto out;
if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
intel_color_set_csc(crtc->state);
@@ -14931,6 +14916,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
else if (INTEL_GEN(dev_priv) >= 9)
skl_detach_scalers(intel_crtc);
+out:
if (dev_priv->display.atomic_update_watermarks)
dev_priv->display.atomic_update_watermarks(old_intel_state, intel_cstate);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 398195bf6dd1..d18444097e1c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1743,13 +1743,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
enum pipe pipe);
bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
struct intel_crtc *intel_crtc);
-void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
- const struct skl_plane_wm *wm,
- const struct skl_ddb_allocation *ddb);
-void skl_write_plane_wm(struct intel_crtc *intel_crtc,
- const struct skl_plane_wm *wm,
- const struct skl_ddb_allocation *ddb,
- int plane);
uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
bool ilk_disable_lp_wm(struct drm_device *dev);
int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9e825a9651a4..93e261300ef0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4203,19 +4203,29 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+ const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
enum pipe pipe = crtc->pipe;
+ int plane;
+
+ if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
+ return;
I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+
+ for_each_universal_plane(dev_priv, pipe, plane)
+ skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
+
+ skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
}
-static void skl_update_wm(struct intel_crtc *intel_crtc)
-{
+static void skl_initial_wm(struct intel_atomic_state *state,
+ struct intel_crtc_state *cstate)
+ {
+ struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- struct skl_wm_values *results = &dev_priv->wm.skl_results;
+ struct skl_wm_values *results = &state->wm_results;
struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
- struct intel_crtc_state *cstate = to_intel_crtc_state(intel_crtc->base.state);
- struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
enum pipe pipe = intel_crtc->pipe;
if ((results->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) == 0)
@@ -4223,22 +4233,8 @@ static void skl_update_wm(struct intel_crtc *intel_crtc)
mutex_lock(&dev_priv->wm.wm_mutex);
- /*
- * If this pipe isn't active already, we're going to be enabling it
- * very soon. Since it's safe to update a pipe's ddb allocation while
- * the pipe's shut off, just do so here. Already active pipes will have
- * their watermarks updated once we update their planes.
- */
- if (intel_crtc->base.state->active_changed) {
- int plane;
-
- for_each_universal_plane(dev_priv, pipe, plane)
- skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
- &results->ddb, plane);
-
- skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
- &results->ddb);
- }
+ if (cstate->base.active_changed)
+ skl_atomic_update_crtc_wm(state, cstate);
skl_copy_wm_for_pipe(hw_vals, results, pipe);
@@ -7707,7 +7703,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
/* For FIFO watermark updates */
if (INTEL_GEN(dev_priv) >= 9) {
skl_setup_wm_latency(dev_priv);
- dev_priv->display.update_wm = skl_update_wm;
+ dev_priv->display.initial_watermarks = skl_initial_wm;
dev_priv->display.atomic_update_watermarks = skl_atomic_update_crtc_wm;
dev_priv->display.compute_global_watermarks = skl_compute_wm;
} else if (HAS_PCH_SPLIT(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index df0fbb4b15a3..55563346bba7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_plane *intel_plane = to_intel_plane(drm_plane);
struct drm_framebuffer *fb = plane_state->base.fb;
- const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
- struct drm_crtc *crtc = crtc_state->base.crtc;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
const int pipe = intel_plane->pipe;
const int plane = intel_plane->plane + 1;
- const struct skl_plane_wm *p_wm =
- &crtc_state->wm.skl.optimal.planes[plane];
u32 plane_ctl;
const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
u32 surf_addr = plane_state->main.offset;
@@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
plane_ctl |= skl_plane_ctl_rotation(rotation);
- if (wm->dirty_pipes & drm_crtc_mask(crtc))
- skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
-
if (key->flags) {
I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
@@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
struct drm_device *dev = dplane->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_plane *intel_plane = to_intel_plane(dplane);
- struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
const int pipe = intel_plane->pipe;
const int plane = intel_plane->plane + 1;
- /*
- * We only populate skl_results on watermark updates, and if the
- * plane's visiblity isn't actually changing neither is its watermarks.
- */
- if (!dplane->state->visible)
- skl_write_plane_wm(to_intel_crtc(crtc),
- &cstate->wm.skl.optimal.planes[plane],
- &dev_priv->wm.skl_results.ddb, plane);
-
I915_WRITE(PLANE_CTL(pipe, plane), 0);
I915_WRITE(PLANE_SURF(pipe, plane), 0);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 02/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3 Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-10 0:16 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
` (8 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
This is the last bit required for making nonblocking modesets work
correctly. The state in intel_crtc->hw_ddb is not updated until
somewhere in atomic commit, while the previous crtc state should be
accurate if the ddb hasn't changed.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index abfcaa07bbe3..69f9addb29b3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14313,7 +14313,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
* new ddb allocation to take effect.
*/
if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
- &intel_crtc->hw_ddb) &&
+ &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
!crtc->state->active_changed &&
intel_state->wm_results.dirty_pipes != updated)
vbl_wait = true;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 93e261300ef0..bde6c68eb0db 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3118,7 +3118,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
* we currently hold.
*/
if (!intel_state->active_pipe_changes) {
- *alloc = to_intel_crtc(for_crtc)->hw_ddb;
+ /*
+ * alloc may be cleared by clear_intel_crtc_state,
+ * copy from old state to be sure
+ */
+ *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
return;
}
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (2 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-10 0:52 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset Maarten Lankhorst
` (7 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
This member is only used in skl_update_crtcs now. It's easy to remove it
by keeping track of which ddb entries in an array, and update them after
we update the crtc. This removes the last bits of SKL-style watermarks
kept outside of crtc_state.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
drivers/gpu/drm/i915/intel_drv.h | 11 +++--------
drivers/gpu/drm/i915/intel_pm.c | 25 +++++++------------------
3 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69f9addb29b3..e59adb03933e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
unsigned int updated = 0;
bool progress;
enum pipe pipe;
+ int i;
+
+ const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
+
+ for_each_crtc_in_state(state, crtc, old_crtc_state, i)
+ /* ignore allocations for crtc's that have been turned off. */
+ if (crtc->state->active)
+ entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
/*
* Whenever the number of active pipes changes, we need to make sure we
@@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
* cause pipe underruns and other bad stuff.
*/
do {
- int i;
progress = false;
for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
cstate = to_intel_crtc_state(crtc->state);
pipe = intel_crtc->pipe;
- if (updated & cmask || !crtc->state->active)
+ if (updated & cmask || !cstate->base.active)
continue;
- if (skl_ddb_allocation_overlaps(state, intel_crtc))
+
+ if (skl_ddb_allocation_overlaps(entries, &cstate->wm.skl.ddb, i))
continue;
updated |= cmask;
+ entries[i] = &cstate->wm.skl.ddb;
/*
* If this is an already active pipe, it's DDB changed,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d18444097e1c..815e8dae3904 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -728,9 +728,6 @@ struct intel_crtc {
bool cxsr_allowed;
} wm;
- /* gen9+: ddb allocation currently being used */
- struct skl_ddb_entry hw_ddb;
-
int scanline_offset;
struct {
@@ -1738,11 +1735,9 @@ int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
bool skl_wm_level_equals(const struct skl_wm_level *l1,
const struct skl_wm_level *l2);
-bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
- const struct skl_ddb_allocation *new,
- enum pipe pipe);
-bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
- struct intel_crtc *intel_crtc);
+bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
+ const struct skl_ddb_entry *ddb,
+ int ignore);
uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
bool ilk_disable_lp_wm(struct drm_device *dev);
int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bde6c68eb0db..02f52b52a03d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3914,25 +3914,16 @@ static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
return a->start < b->end && b->start < a->end;
}
-bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
- struct intel_crtc *intel_crtc)
-{
- struct drm_crtc *other_crtc;
- struct drm_crtc_state *other_cstate;
- struct intel_crtc *other_intel_crtc;
- const struct skl_ddb_entry *ddb =
- &to_intel_crtc_state(intel_crtc->base.state)->wm.skl.ddb;
+bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
+ const struct skl_ddb_entry *ddb,
+ int ignore)
+{
int i;
- for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
- other_intel_crtc = to_intel_crtc(other_crtc);
-
- if (other_intel_crtc == intel_crtc)
- continue;
-
- if (skl_ddb_entries_overlap(ddb, &other_intel_crtc->hw_ddb))
+ for (i = 0; i < I915_MAX_PIPES; i++)
+ if (i != ignore && entries[i] &&
+ skl_ddb_entries_overlap(ddb, entries[i]))
return true;
- }
return false;
}
@@ -4242,8 +4233,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
skl_copy_wm_for_pipe(hw_vals, results, pipe);
- intel_crtc->hw_ddb = cstate->wm.skl.ddb;
-
mutex_unlock(&dev_priv->wm.wm_mutex);
}
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (3 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 14:11 ` Ville Syrjälä
2016-11-08 12:55 ` [PATCH v3 06/11] drm/i915: Convert intel_hdmi to use atomic state Maarten Lankhorst
` (6 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
This is a hack and not needed. Use the right mask by checking
intel_state->modeset. This works for watermark sanitization too.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 02f52b52a03d..d38a46efcfed 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_crtc *for_crtc = cstate->base.crtc;
- unsigned int pipe_size, ddb_size;
+ unsigned int pipe_size, ddb_size, active_crtcs;
int nth_active_pipe;
+ if (intel_state->modeset)
+ active_crtcs = intel_state->active_crtcs;
+ else
+ active_crtcs = dev_priv->active_crtcs;
+
+ *num_active = hweight32(active_crtcs);
+
if (WARN_ON(!state) || !cstate->base.active) {
alloc->start = 0;
alloc->end = 0;
- *num_active = hweight32(dev_priv->active_crtcs);
return;
}
- if (intel_state->active_pipe_changes)
- *num_active = hweight32(intel_state->active_crtcs);
- else
- *num_active = hweight32(dev_priv->active_crtcs);
-
- ddb_size = INTEL_INFO(dev_priv)->ddb_size;
- WARN_ON(ddb_size == 0);
-
- ddb_size -= 4; /* 4 blocks for bypass path allocation */
-
/*
* If the state doesn't change the active CRTC's, then there's
* no need to recalculate; the existing pipe allocation limits
@@ -3126,9 +3122,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
return;
}
- nth_active_pipe = hweight32(intel_state->active_crtcs &
+ ddb_size = INTEL_INFO(dev_priv)->ddb_size;
+ WARN_ON(ddb_size == 0);
+
+ ddb_size -= 4; /* 4 blocks for bypass path allocation */
+
+ nth_active_pipe = hweight32(active_crtcs &
(drm_crtc_mask(for_crtc) - 1));
- pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
+ pipe_size = ddb_size / *num_active;
alloc->start = nth_active_pipe * ddb_size / *num_active;
alloc->end = alloc->start + pipe_size;
}
@@ -4021,15 +4022,6 @@ skl_compute_ddb(struct drm_atomic_state *state)
return ret;
intel_state->active_pipe_changes = ~0;
-
- /*
- * We usually only initialize intel_state->active_crtcs if we
- * we're doing a modeset; make sure this field is always
- * initialized during the sanitization process that happens
- * on the first commit too.
- */
- if (!intel_state->modeset)
- intel_state->active_crtcs = dev_priv->active_crtcs;
}
/*
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 06/11] drm/i915: Convert intel_hdmi to use atomic state
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (4 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2 Maarten Lankhorst
` (5 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
This is the last connector still looking at crtc->config. Fix this.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_hdmi.c | 48 +++++++++++++++++----------------------
1 file changed, 21 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 35ada4e1c6cf..2218b10fa22a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -975,7 +975,9 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
pipe_config->lane_count = 4;
}
-static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
+static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
+ struct intel_crtc_state *pipe_config,
+ struct drm_connector_state *conn_state)
{
struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
@@ -991,21 +993,20 @@ static void g4x_enable_hdmi(struct intel_encoder *encoder,
{
struct drm_device *dev = encoder->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
u32 temp;
temp = I915_READ(intel_hdmi->hdmi_reg);
temp |= SDVO_ENABLE;
- if (crtc->config->has_audio)
+ if (pipe_config->has_audio)
temp |= SDVO_AUDIO_ENABLE;
I915_WRITE(intel_hdmi->hdmi_reg, temp);
POSTING_READ(intel_hdmi->hdmi_reg);
- if (crtc->config->has_audio)
- intel_enable_hdmi_audio(encoder);
+ if (pipe_config->has_audio)
+ intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
}
static void ibx_enable_hdmi(struct intel_encoder *encoder,
@@ -1040,8 +1041,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
* FIXME: BSpec says this should be done at the end of
* of the modeset sequence, so not sure if this isn't too soon.
*/
- if (crtc->config->pipe_bpp > 24 &&
- crtc->config->pixel_multiplier > 1) {
+ if (pipe_config->pipe_bpp > 24 &&
+ pipe_config->pixel_multiplier > 1) {
I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
POSTING_READ(intel_hdmi->hdmi_reg);
@@ -1055,8 +1056,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
POSTING_READ(intel_hdmi->hdmi_reg);
}
- if (crtc->config->has_audio)
- intel_enable_hdmi_audio(encoder);
+ if (pipe_config->has_audio)
+ intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
}
static void cpt_enable_hdmi(struct intel_encoder *encoder,
@@ -1073,7 +1074,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
temp = I915_READ(intel_hdmi->hdmi_reg);
temp |= SDVO_ENABLE;
- if (crtc->config->has_audio)
+ if (pipe_config->has_audio)
temp |= SDVO_AUDIO_ENABLE;
/*
@@ -1086,7 +1087,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
* 4. enable HDMI clock gating
*/
- if (crtc->config->pipe_bpp > 24) {
+ if (pipe_config->pipe_bpp > 24) {
I915_WRITE(TRANS_CHICKEN1(pipe),
I915_READ(TRANS_CHICKEN1(pipe)) |
TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
@@ -1098,7 +1099,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
I915_WRITE(intel_hdmi->hdmi_reg, temp);
POSTING_READ(intel_hdmi->hdmi_reg);
- if (crtc->config->pipe_bpp > 24) {
+ if (pipe_config->pipe_bpp > 24) {
temp &= ~SDVO_COLOR_FORMAT_MASK;
temp |= HDMI_COLOR_FORMAT_12bpc;
@@ -1110,8 +1111,8 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
}
- if (crtc->config->has_audio)
- intel_enable_hdmi_audio(encoder);
+ if (pipe_config->has_audio)
+ intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
}
static void vlv_enable_hdmi(struct intel_encoder *encoder,
@@ -1178,9 +1179,7 @@ static void g4x_disable_hdmi(struct intel_encoder *encoder,
struct intel_crtc_state *old_crtc_state,
struct drm_connector_state *old_conn_state)
{
- struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-
- if (crtc->config->has_audio)
+ if (old_crtc_state->has_audio)
intel_audio_codec_disable(encoder);
intel_disable_hdmi(encoder, old_crtc_state, old_conn_state);
@@ -1190,9 +1189,7 @@ static void pch_disable_hdmi(struct intel_encoder *encoder,
struct intel_crtc_state *old_crtc_state,
struct drm_connector_state *old_conn_state)
{
- struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-
- if (crtc->config->has_audio)
+ if (old_crtc_state->has_audio)
intel_audio_codec_disable(encoder);
}
@@ -1645,13 +1642,12 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder,
struct drm_connector_state *conn_state)
{
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
- struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
- const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+ const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
intel_hdmi_prepare(encoder);
intel_hdmi->set_infoframes(&encoder->base,
- intel_crtc->config->has_hdmi_sink,
+ pipe_config->has_hdmi_sink,
adjusted_mode);
}
@@ -1663,9 +1659,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
struct intel_hdmi *intel_hdmi = &dport->hdmi;
struct drm_device *dev = encoder->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *intel_crtc =
- to_intel_crtc(encoder->base.crtc);
- const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+ const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
vlv_phy_pre_encoder_enable(encoder);
@@ -1674,7 +1668,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
0x2b247878);
intel_hdmi->set_infoframes(&encoder->base,
- intel_crtc->config->has_hdmi_sink,
+ pipe_config->has_hdmi_sink,
adjusted_mode);
g4x_enable_hdmi(encoder, pipe_config, conn_state);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (5 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 06/11] drm/i915: Convert intel_hdmi to use atomic state Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 14:07 ` Ville Syrjälä
2016-11-08 12:55 ` [PATCH v3 08/11] drm/edid: Remove drm_select_eld Maarten Lankhorst
` (4 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
drm_select_eld requires mode_config.mutex and connection_mutex
because it looks at the connector list and at the legacy encoders.
This is not required, because when we call audio_codec_enable we know
which connector it was called for, so pass the state.
This also removes having to look at crtc->config.
Changes since v1:
- Use intel_crtc->pipe instead of drm_crtc_index. (Ville)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
drivers/gpu/drm/i915/intel_ddi.c | 2 +-
drivers/gpu/drm/i915/intel_dp.c | 11 ++++++-----
drivers/gpu/drm/i915/intel_drv.h | 4 +++-
drivers/gpu/drm/i915/intel_hdmi.c | 2 +-
5 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 813fd74d9c8d..1c509f7410f5 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -574,23 +574,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @intel_encoder: encoder on which to enable audio
+ * @crtc_state: pointer to the current crtc state.
+ * @conn_state: pointer to the current connector state.
*
* The enable sequences may only be performed after enabling the transcoder and
* port, and after completed link training.
*/
-void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
+void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state)
{
struct drm_encoder *encoder = &intel_encoder->base;
- struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
- const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
+ const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
struct drm_connector *connector;
struct drm_i915_private *dev_priv = to_i915(encoder->dev);
struct i915_audio_component *acomp = dev_priv->audio_component;
enum port port = intel_encoder->port;
- enum pipe pipe = crtc->pipe;
+ enum pipe pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
- connector = drm_select_eld(encoder);
- if (!connector)
+ connector = conn_state->connector;
+ if (!connector || !connector->eld[0])
return;
DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
@@ -601,7 +604,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
/* ELD Conn_Type */
connector->eld[5] &= ~(3 << 2);
- if (intel_crtc_has_dp_encoder(crtc->config))
+ if (intel_crtc_has_dp_encoder(crtc_state))
connector->eld[5] |= (1 << 2);
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 938ac4dbcb45..0ad4e16a639f 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1866,7 +1866,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
if (intel_crtc->config->has_audio) {
intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
- intel_audio_codec_enable(intel_encoder);
+ intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
}
}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9df331b3305b..117a71450ec2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2735,7 +2735,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp,
}
static void intel_enable_dp(struct intel_encoder *encoder,
- struct intel_crtc_state *pipe_config)
+ struct intel_crtc_state *pipe_config,
+ struct drm_connector_state *conn_state)
{
struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
struct drm_device *dev = encoder->base.dev;
@@ -2777,7 +2778,7 @@ static void intel_enable_dp(struct intel_encoder *encoder,
if (pipe_config->has_audio) {
DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
pipe_name(pipe));
- intel_audio_codec_enable(encoder);
+ intel_audio_codec_enable(encoder, pipe_config, conn_state);
}
}
@@ -2787,7 +2788,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder,
{
struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
- intel_enable_dp(encoder, pipe_config);
+ intel_enable_dp(encoder, pipe_config, conn_state);
intel_edp_backlight_on(intel_dp);
}
@@ -2924,7 +2925,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder,
{
vlv_phy_pre_encoder_enable(encoder);
- intel_enable_dp(encoder, pipe_config);
+ intel_enable_dp(encoder, pipe_config, conn_state);
}
static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder,
@@ -2942,7 +2943,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder,
{
chv_phy_pre_encoder_enable(encoder);
- intel_enable_dp(encoder, pipe_config);
+ intel_enable_dp(encoder, pipe_config, conn_state);
/* Second common lane will stay alive on its own now */
chv_phy_release_cl2_override(encoder);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 815e8dae3904..3d27e31dcada 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1184,7 +1184,9 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
/* intel_audio.c */
void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
-void intel_audio_codec_enable(struct intel_encoder *encoder);
+void intel_audio_codec_enable(struct intel_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state);
void intel_audio_codec_disable(struct intel_encoder *encoder);
void i915_audio_component_init(struct drm_i915_private *dev_priv);
void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2218b10fa22a..fb88e32e25a3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -984,7 +984,7 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
WARN_ON(!crtc->config->has_hdmi_sink);
DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
pipe_name(crtc->pipe));
- intel_audio_codec_enable(encoder);
+ intel_audio_codec_enable(encoder, pipe_config, conn_state);
}
static void g4x_enable_hdmi(struct intel_encoder *encoder,
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 08/11] drm/edid: Remove drm_select_eld
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (6 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2 Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2 Maarten Lankhorst
` (3 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
The only user was i915, which is now gone.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Dave Airlie <airlied@redhat.com> #irc
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_edid.c | 26 --------------------------
include/drm/drm_edid.h | 1 -
2 files changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9506933b41cd..1801e9c0e41b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3611,32 +3611,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
EXPORT_SYMBOL(drm_av_sync_delay);
/**
- * drm_select_eld - select one ELD from multiple HDMI/DP sinks
- * @encoder: the encoder just changed display mode
- *
- * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
- * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
- *
- * Return: The connector associated with the first HDMI/DP sink that has ELD
- * attached to it.
- */
-struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
-{
- struct drm_connector *connector;
- struct drm_device *dev = encoder->dev;
-
- WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
- WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-
- drm_for_each_connector(connector, dev)
- if (connector->encoder == encoder && connector->eld[0])
- return connector;
-
- return NULL;
-}
-EXPORT_SYMBOL(drm_select_eld);
-
-/**
* drm_detect_hdmi_monitor - detect whether monitor is HDMI
* @edid: monitor EDID information
*
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c3a7d440bc11..38eabf65f19d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -330,7 +330,6 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb);
int drm_av_sync_delay(struct drm_connector *connector,
const struct drm_display_mode *mode);
-struct drm_connector *drm_select_eld(struct drm_encoder *encoder);
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
int drm_load_edid_firmware(struct drm_connector *connector);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (7 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 08/11] drm/edid: Remove drm_select_eld Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 14:08 ` Ville Syrjälä
2016-11-08 12:55 ` [PATCH v3 10/11] drm/i915: Pass atomic state to verify_connector_state Maarten Lankhorst
` (2 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
All of this state should be updated as soon as possible. It shouldn't be
done later because then future updates may not depend on it.
Changes since v1:
- Move the modeset update to before drm_atomic_state_get. (Ville)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e59adb03933e..d368afa199e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14353,14 +14353,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
drm_atomic_helper_wait_for_dependencies(state);
- if (intel_state->modeset) {
- memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
- sizeof(intel_state->min_pixclk));
- dev_priv->active_crtcs = intel_state->active_crtcs;
- dev_priv->atomic_cdclk_freq = intel_state->cdclk;
-
+ if (intel_state->modeset)
intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
- }
for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -14601,6 +14595,13 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_shared_dpll_commit(state);
intel_atomic_track_fbs(state);
+ if (intel_state->modeset) {
+ memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
+ sizeof(intel_state->min_pixclk));
+ dev_priv->active_crtcs = intel_state->active_crtcs;
+ dev_priv->atomic_cdclk_freq = intel_state->cdclk;
+ }
+
drm_atomic_state_get(state);
INIT_WORK(&state->commit_work,
nonblock ? intel_atomic_commit_work : NULL);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 10/11] drm/i915: Pass atomic state to verify_connector_state
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (8 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2 Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 11/11] drm/i915: Enable support for nonblocking modeset Maarten Lankhorst
2016-11-08 13:45 ` ✓ Fi.CI.BAT: success for Skylake watermark fixes and " Patchwork
11 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
This gets rid of a warning that the connectors are used without locking
when doing a nonblocking modeset.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d368afa199e3..ee8b6db0b425 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13543,11 +13543,15 @@ static void verify_wm_state(struct drm_crtc *crtc,
}
static void
-verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
+verify_connector_state(struct drm_device *dev,
+ struct drm_atomic_state *state,
+ struct drm_crtc *crtc)
{
struct drm_connector *connector;
+ struct drm_connector_state *old_conn_state;
+ int i;
- drm_for_each_connector(connector, dev) {
+ for_each_connector_in_state(state, connector, old_conn_state, i) {
struct drm_encoder *encoder = connector->encoder;
struct drm_connector_state *state = connector->state;
@@ -13755,15 +13759,16 @@ verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
static void
intel_modeset_verify_crtc(struct drm_crtc *crtc,
- struct drm_crtc_state *old_state,
- struct drm_crtc_state *new_state)
+ struct drm_atomic_state *state,
+ struct drm_crtc_state *old_state,
+ struct drm_crtc_state *new_state)
{
if (!needs_modeset(new_state) &&
!to_intel_crtc_state(new_state)->update_pipe)
return;
verify_wm_state(crtc, new_state);
- verify_connector_state(crtc->dev, crtc);
+ verify_connector_state(crtc->dev, state, crtc);
verify_crtc_state(crtc, old_state, new_state);
verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
}
@@ -13779,10 +13784,11 @@ verify_disabled_dpll_state(struct drm_device *dev)
}
static void
-intel_modeset_verify_disabled(struct drm_device *dev)
+intel_modeset_verify_disabled(struct drm_device *dev,
+ struct drm_atomic_state *state)
{
verify_encoder_state(dev);
- verify_connector_state(dev, NULL);
+ verify_connector_state(dev, state, NULL);
verify_disabled_dpll_state(dev);
}
@@ -14420,7 +14426,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
if (!intel_can_enable_sagv(state))
intel_disable_sagv(dev_priv);
- intel_modeset_verify_disabled(dev);
+ intel_modeset_verify_disabled(dev, state);
}
/* Complete the events for pipes that have now been disabled */
@@ -14472,7 +14478,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
if (put_domains[i])
modeset_put_power_domains(dev_priv, put_domains[i]);
- intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
+ intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
}
if (intel_state->modeset && intel_can_enable_sagv(state))
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 11/11] drm/i915: Enable support for nonblocking modeset
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (9 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 10/11] drm/i915: Pass atomic state to verify_connector_state Maarten Lankhorst
@ 2016-11-08 12:55 ` Maarten Lankhorst
2016-11-08 13:45 ` ✓ Fi.CI.BAT: success for Skylake watermark fixes and " Patchwork
11 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-08 12:55 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee8b6db0b425..759b1fdbb009 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14561,10 +14561,6 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
* This function commits a top-level state object that has been validated
* with drm_atomic_helper_check().
*
- * FIXME: Atomic modeset support for i915 is not yet complete. At the moment
- * nonblocking commits are only safe for pure plane updates. Everything else
- * should work though.
- *
* RETURNS
* Zero for success or -errno.
*/
@@ -14576,11 +14572,6 @@ static int intel_atomic_commit(struct drm_device *dev,
struct drm_i915_private *dev_priv = to_i915(dev);
int ret = 0;
- if (intel_state->modeset && nonblock) {
- DRM_DEBUG_KMS("nonblocking commit for modeset not yet implemented.\n");
- return -EINVAL;
- }
-
ret = drm_atomic_helper_setup_commit(state, nonblock);
if (ret)
return ret;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* ✓ Fi.CI.BAT: success for Skylake watermark fixes and nonblocking modeset.
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
` (10 preceding siblings ...)
2016-11-08 12:55 ` [PATCH v3 11/11] drm/i915: Enable support for nonblocking modeset Maarten Lankhorst
@ 2016-11-08 13:45 ` Patchwork
11 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2016-11-08 13:45 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: Skylake watermark fixes and nonblocking modeset.
URL : https://patchwork.freedesktop.org/series/14966/
State : success
== Summary ==
Series 14966v1 Skylake watermark fixes and nonblocking modeset.
https://patchwork.freedesktop.org/api/1.0/series/14966/revisions/1/mbox/
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-skl-6770hq)
fi-bdw-5557u total:244 pass:229 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:244 pass:204 dwarn:0 dfail:0 fail:0 skip:40
fi-bxt-t5700 total:244 pass:216 dwarn:0 dfail:0 fail:0 skip:28
fi-byt-j1900 total:244 pass:216 dwarn:0 dfail:0 fail:0 skip:28
fi-byt-n2820 total:244 pass:212 dwarn:0 dfail:0 fail:0 skip:32
fi-hsw-4770 total:244 pass:224 dwarn:0 dfail:0 fail:0 skip:20
fi-hsw-4770r total:244 pass:224 dwarn:0 dfail:0 fail:0 skip:20
fi-ilk-650 total:244 pass:191 dwarn:0 dfail:0 fail:0 skip:53
fi-ivb-3520m total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-ivb-3770 total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-kbl-7200u total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-skl-6260u total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:244 pass:223 dwarn:0 dfail:0 fail:0 skip:21
fi-skl-6700k total:244 pass:222 dwarn:1 dfail:0 fail:0 skip:21
fi-skl-6770hq total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14
fi-snb-2520m total:244 pass:212 dwarn:0 dfail:0 fail:0 skip:32
fi-snb-2600 total:244 pass:211 dwarn:0 dfail:0 fail:0 skip:33
aa74f4d7a58d239a7ff25c02eebf00c9ceb42c95 drm-intel-nightly: 2016y-11m-08d-11h-05m-41s UTC integration manifest
dc6cbdd drm/i915: Enable support for nonblocking modeset
9d7426c drm/i915: Pass atomic state to verify_connector_state
4e7ab3e drm/i915: Update atomic modeset state synchronously, v2.
31b91b5 drm/edid: Remove drm_select_eld
e185dd9 drm/i915: Pass atomic state to intel_audio_codec_enable, v2.
dd9fe0d drm/i915: Convert intel_hdmi to use atomic state
e485020 drm/i915/gen9+: Do not initialise active_crtcs for !modeset
c2129a1 drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
3f454fa drm/i915/gen9+: Preserve old allocation from crtc_state.
49193be drm/i915/gen9+: Program watermarks as a separate step during evasion, v3.
5bb427c drm/i915: Add a atomic evasion step to watermark programming, v3.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2931/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2.
2016-11-08 12:55 ` [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2 Maarten Lankhorst
@ 2016-11-08 14:07 ` Ville Syrjälä
0 siblings, 0 replies; 24+ messages in thread
From: Ville Syrjälä @ 2016-11-08 14:07 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Nov 08, 2016 at 01:55:38PM +0100, Maarten Lankhorst wrote:
> drm_select_eld requires mode_config.mutex and connection_mutex
> because it looks at the connector list and at the legacy encoders.
>
> This is not required, because when we call audio_codec_enable we know
> which connector it was called for, so pass the state.
>
> This also removes having to look at crtc->config.
>
> Changes since v1:
> - Use intel_crtc->pipe instead of drm_crtc_index. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
> drivers/gpu/drm/i915/intel_ddi.c | 2 +-
> drivers/gpu/drm/i915/intel_dp.c | 11 ++++++-----
> drivers/gpu/drm/i915/intel_drv.h | 4 +++-
> drivers/gpu/drm/i915/intel_hdmi.c | 2 +-
> 5 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 813fd74d9c8d..1c509f7410f5 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -574,23 +574,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
> /**
> * intel_audio_codec_enable - Enable the audio codec for HD audio
> * @intel_encoder: encoder on which to enable audio
> + * @crtc_state: pointer to the current crtc state.
> + * @conn_state: pointer to the current connector state.
> *
> * The enable sequences may only be performed after enabling the transcoder and
> * port, and after completed link training.
> */
> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> + const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state)
> {
> struct drm_encoder *encoder = &intel_encoder->base;
> - struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> - const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> + const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> struct drm_connector *connector;
> struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> struct i915_audio_component *acomp = dev_priv->audio_component;
> enum port port = intel_encoder->port;
> - enum pipe pipe = crtc->pipe;
> + enum pipe pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
>
> - connector = drm_select_eld(encoder);
> - if (!connector)
> + connector = conn_state->connector;
> + if (!connector || !connector->eld[0])
> return;
>
> DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> @@ -601,7 +604,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>
> /* ELD Conn_Type */
> connector->eld[5] &= ~(3 << 2);
> - if (intel_crtc_has_dp_encoder(crtc->config))
> + if (intel_crtc_has_dp_encoder(crtc_state))
> connector->eld[5] |= (1 << 2);
>
> connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 938ac4dbcb45..0ad4e16a639f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1866,7 +1866,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>
> if (intel_crtc->config->has_audio) {
> intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> - intel_audio_codec_enable(intel_encoder);
> + intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9df331b3305b..117a71450ec2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2735,7 +2735,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp,
> }
>
> static void intel_enable_dp(struct intel_encoder *encoder,
> - struct intel_crtc_state *pipe_config)
> + struct intel_crtc_state *pipe_config,
> + struct drm_connector_state *conn_state)
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> struct drm_device *dev = encoder->base.dev;
> @@ -2777,7 +2778,7 @@ static void intel_enable_dp(struct intel_encoder *encoder,
> if (pipe_config->has_audio) {
> DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> pipe_name(pipe));
> - intel_audio_codec_enable(encoder);
> + intel_audio_codec_enable(encoder, pipe_config, conn_state);
> }
> }
>
> @@ -2787,7 +2788,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder,
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>
> - intel_enable_dp(encoder, pipe_config);
> + intel_enable_dp(encoder, pipe_config, conn_state);
> intel_edp_backlight_on(intel_dp);
> }
>
> @@ -2924,7 +2925,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder,
> {
> vlv_phy_pre_encoder_enable(encoder);
>
> - intel_enable_dp(encoder, pipe_config);
> + intel_enable_dp(encoder, pipe_config, conn_state);
> }
>
> static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder,
> @@ -2942,7 +2943,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder,
> {
> chv_phy_pre_encoder_enable(encoder);
>
> - intel_enable_dp(encoder, pipe_config);
> + intel_enable_dp(encoder, pipe_config, conn_state);
>
> /* Second common lane will stay alive on its own now */
> chv_phy_release_cl2_override(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 815e8dae3904..3d27e31dcada 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1184,7 +1184,9 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
>
> /* intel_audio.c */
> void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
> -void intel_audio_codec_enable(struct intel_encoder *encoder);
> +void intel_audio_codec_enable(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state);
> void intel_audio_codec_disable(struct intel_encoder *encoder);
> void i915_audio_component_init(struct drm_i915_private *dev_priv);
> void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2218b10fa22a..fb88e32e25a3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -984,7 +984,7 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
> WARN_ON(!crtc->config->has_hdmi_sink);
> DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> pipe_name(crtc->pipe));
> - intel_audio_codec_enable(encoder);
> + intel_audio_codec_enable(encoder, pipe_config, conn_state);
> }
>
> static void g4x_enable_hdmi(struct intel_encoder *encoder,
> --
> 2.7.4
>
> _______________________________________________
> 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] 24+ messages in thread
* Re: [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2.
2016-11-08 12:55 ` [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2 Maarten Lankhorst
@ 2016-11-08 14:08 ` Ville Syrjälä
0 siblings, 0 replies; 24+ messages in thread
From: Ville Syrjälä @ 2016-11-08 14:08 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Nov 08, 2016 at 01:55:40PM +0100, Maarten Lankhorst wrote:
> All of this state should be updated as soon as possible. It shouldn't be
> done later because then future updates may not depend on it.
>
> Changes since v1:
> - Move the modeset update to before drm_atomic_state_get. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e59adb03933e..d368afa199e3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14353,14 +14353,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>
> drm_atomic_helper_wait_for_dependencies(state);
>
> - if (intel_state->modeset) {
> - memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> - sizeof(intel_state->min_pixclk));
> - dev_priv->active_crtcs = intel_state->active_crtcs;
> - dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> -
> + if (intel_state->modeset)
> intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> - }
>
> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -14601,6 +14595,13 @@ static int intel_atomic_commit(struct drm_device *dev,
> intel_shared_dpll_commit(state);
> intel_atomic_track_fbs(state);
>
> + if (intel_state->modeset) {
> + memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> + sizeof(intel_state->min_pixclk));
> + dev_priv->active_crtcs = intel_state->active_crtcs;
> + dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> + }
> +
> drm_atomic_state_get(state);
> INIT_WORK(&state->commit_work,
> nonblock ? intel_atomic_commit_work : NULL);
> --
> 2.7.4
>
> _______________________________________________
> 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] 24+ messages in thread
* Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset
2016-11-08 12:55 ` [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset Maarten Lankhorst
@ 2016-11-08 14:11 ` Ville Syrjälä
2016-11-09 12:45 ` Maarten Lankhorst
0 siblings, 1 reply; 24+ messages in thread
From: Ville Syrjälä @ 2016-11-08 14:11 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
> This is a hack and not needed. Use the right mask by checking
> intel_state->modeset. This works for watermark sanitization too.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 02f52b52a03d..d38a46efcfed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_crtc *for_crtc = cstate->base.crtc;
> - unsigned int pipe_size, ddb_size;
> + unsigned int pipe_size, ddb_size, active_crtcs;
> int nth_active_pipe;
>
> + if (intel_state->modeset)
> + active_crtcs = intel_state->active_crtcs;
> + else
> + active_crtcs = dev_priv->active_crtcs;
What's the story with the locking here?
> +
> + *num_active = hweight32(active_crtcs);
> +
> if (WARN_ON(!state) || !cstate->base.active) {
> alloc->start = 0;
> alloc->end = 0;
> - *num_active = hweight32(dev_priv->active_crtcs);
> return;
> }
>
> - if (intel_state->active_pipe_changes)
> - *num_active = hweight32(intel_state->active_crtcs);
> - else
> - *num_active = hweight32(dev_priv->active_crtcs);
> -
> - ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> - WARN_ON(ddb_size == 0);
> -
> - ddb_size -= 4; /* 4 blocks for bypass path allocation */
> -
> /*
> * If the state doesn't change the active CRTC's, then there's
> * no need to recalculate; the existing pipe allocation limits
> @@ -3126,9 +3122,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> return;
> }
>
> - nth_active_pipe = hweight32(intel_state->active_crtcs &
> + ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> + WARN_ON(ddb_size == 0);
> +
> + ddb_size -= 4; /* 4 blocks for bypass path allocation */
> +
> + nth_active_pipe = hweight32(active_crtcs &
> (drm_crtc_mask(for_crtc) - 1));
> - pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> + pipe_size = ddb_size / *num_active;
> alloc->start = nth_active_pipe * ddb_size / *num_active;
> alloc->end = alloc->start + pipe_size;
> }
> @@ -4021,15 +4022,6 @@ skl_compute_ddb(struct drm_atomic_state *state)
> return ret;
>
> intel_state->active_pipe_changes = ~0;
> -
> - /*
> - * We usually only initialize intel_state->active_crtcs if we
> - * we're doing a modeset; make sure this field is always
> - * initialized during the sanitization process that happens
> - * on the first commit too.
> - */
> - if (!intel_state->modeset)
> - intel_state->active_crtcs = dev_priv->active_crtcs;
> }
>
> /*
> --
> 2.7.4
>
> _______________________________________________
> 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] 24+ messages in thread
* Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset
2016-11-08 14:11 ` Ville Syrjälä
@ 2016-11-09 12:45 ` Maarten Lankhorst
2016-11-10 10:53 ` Ville Syrjälä
0 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-09 12:45 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 08-11-16 om 15:11 schreef Ville Syrjälä:
> On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
>> This is a hack and not needed. Use the right mask by checking
>> intel_state->modeset. This works for watermark sanitization too.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
>> 1 file changed, 15 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 02f52b52a03d..d38a46efcfed 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> struct drm_crtc *for_crtc = cstate->base.crtc;
>> - unsigned int pipe_size, ddb_size;
>> + unsigned int pipe_size, ddb_size, active_crtcs;
>> int nth_active_pipe;
>>
>> + if (intel_state->modeset)
>> + active_crtcs = intel_state->active_crtcs;
>> + else
>> + active_crtcs = dev_priv->active_crtcs;
> What's the story with the locking here?
if !modeset, 3 things can happen:
1. fastset, connection_mutex held, dev_priv->active_crtcs cannot change.
2. crtc disabled, active_crtcs is potentially garbage, but harmless since we don't write disabled wm's when the crtc is already disabled.
(same as what happens currently)
3. crtc enabled, dev_priv->active_crtcs is valid because ddb reallocation requires locking all active crtc's for reallocation,
which requires taking this lock for this crtc.
This wouldn't be valid for gen8-, but is valid because of skl reallocation requirements.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3.
2016-11-08 12:55 ` [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
@ 2016-11-10 0:14 ` Matt Roper
0 siblings, 0 replies; 24+ messages in thread
From: Matt Roper @ 2016-11-10 0:14 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, Paulo Zanoni
On Tue, Nov 08, 2016 at 01:55:32PM +0100, Maarten Lankhorst wrote:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
>
> intel_atomic_state is not used here yet, but will be used when
> we program all watermarks as a separate step during evasion.
>
> This also writes linetime all the time, while before it was only
> done during plane updates. This looks like this could be a bugfix,
> but I'm not sure what it affects.
>
> Changes since v1:
> - Add comment about atomic evasion to commit message.
> - Unwrap I915_WRITE call. (Lyude)
> Changes since v2:
> - Rename atomic_evade_watermarks to atomic_update_watermarks. (Ville)
> - Add line wraps where appropriate, fix grammar in commit message. (Matt)
I'm not sure if these minor changes actually got applied or not; the
headline still needs s/a atomic/an atomic/ and there are still several
long lines that could be wrapped (especially the callsites of
initial_watermarks()).
But otherwise the code looks good, so if you clean up the cosmetic
issues, then my r-b still stands.
Matt
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 9 +++++++--
> drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++-------------
> drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++--
> 3 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4735b4177100..00988d716d7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -487,6 +487,7 @@ struct sdvo_device_mapping {
>
> struct intel_connector;
> struct intel_encoder;
> +struct intel_atomic_state;
> struct intel_crtc_state;
> struct intel_initial_plane_config;
> struct intel_crtc;
> @@ -500,8 +501,12 @@ struct drm_i915_display_funcs {
> int (*compute_intermediate_wm)(struct drm_device *dev,
> struct intel_crtc *intel_crtc,
> struct intel_crtc_state *newstate);
> - void (*initial_watermarks)(struct intel_crtc_state *cstate);
> - void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> + void (*initial_watermarks)(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate);
> + void (*atomic_update_watermarks)(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate);
> + void (*optimize_watermarks)(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate);
> int (*compute_global_watermarks)(struct drm_atomic_state *state);
> void (*update_wm)(struct intel_crtc *crtc);
> int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92ab01f33208..66cf29ac9fe4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5169,7 +5169,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> * us to.
> */
> if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(pipe_config);
> + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
> else if (pipe_config->update_wm_pre)
> intel_update_watermarks(crtc);
> }
> @@ -5383,7 +5383,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> intel_color_load_luts(&pipe_config->base);
>
> if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(intel_crtc->config);
> + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
> intel_enable_pipe(intel_crtc);
>
> if (intel_crtc->config->has_pch_encoder)
> @@ -5489,7 +5489,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> intel_ddi_enable_transcoder_func(crtc);
>
> if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(pipe_config);
> + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
> else
> intel_update_watermarks(intel_crtc);
>
> @@ -14474,7 +14474,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> intel_cstate = to_intel_crtc_state(crtc->state);
>
> if (dev_priv->display.optimize_watermarks)
> - dev_priv->display.optimize_watermarks(intel_cstate);
> + dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
> }
>
> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14909,10 +14909,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *intel_cstate =
> to_intel_crtc_state(crtc->state);
> - struct intel_crtc_state *old_intel_state =
> + struct intel_crtc_state *old_intel_cstate =
> to_intel_crtc_state(old_crtc_state);
> + struct intel_atomic_state *old_intel_state =
> + to_intel_atomic_state(old_crtc_state->state);
> bool modeset = needs_modeset(crtc->state);
> - enum pipe pipe = intel_crtc->pipe;
>
> /* Perform vblank evasion around commit operation */
> intel_pipe_update_start(intel_crtc);
> @@ -14925,14 +14926,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> intel_color_load_luts(crtc->state);
> }
>
> - if (intel_cstate->update_pipe) {
> - intel_update_pipe_config(intel_crtc, old_intel_state);
> - } else if (INTEL_GEN(dev_priv) >= 9) {
> + if (intel_cstate->update_pipe)
> + intel_update_pipe_config(intel_crtc, old_intel_cstate);
> + else if (INTEL_GEN(dev_priv) >= 9)
> skl_detach_scalers(intel_crtc);
>
> - I915_WRITE(PIPE_WM_LINETIME(pipe),
> - intel_cstate->wm.skl.optimal.linetime);
> - }
> + if (dev_priv->display.atomic_update_watermarks)
> + dev_priv->display.atomic_update_watermarks(old_intel_state, intel_cstate);
> }
>
> static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> @@ -16411,7 +16411,7 @@ static void sanitize_watermarks(struct drm_device *dev)
> struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>
> cs->wm.need_postvbl_update = true;
> - dev_priv->display.optimize_watermarks(cs);
> + dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
> }
>
> put_state:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 88e28c989b9c..9e825a9651a4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4197,6 +4197,17 @@ skl_compute_wm(struct drm_atomic_state *state)
> return 0;
> }
>
> +static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> + enum pipe pipe = crtc->pipe;
> +
> + I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +}
> +
> static void skl_update_wm(struct intel_crtc *intel_crtc)
> {
> struct drm_device *dev = intel_crtc->base.dev;
> @@ -4287,7 +4298,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> ilk_write_wm_values(dev_priv, &results);
> }
>
> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_initial_watermarks(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate)
> {
> struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -4298,7 +4310,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> mutex_unlock(&dev_priv->wm.wm_mutex);
> }
>
> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_optimize_watermarks(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate)
> {
> struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -7695,6 +7708,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
> if (INTEL_GEN(dev_priv) >= 9) {
> skl_setup_wm_latency(dev_priv);
> dev_priv->display.update_wm = skl_update_wm;
> + dev_priv->display.atomic_update_watermarks = skl_atomic_update_crtc_wm;
> dev_priv->display.compute_global_watermarks = skl_compute_wm;
> } else if (HAS_PCH_SPLIT(dev_priv)) {
> ilk_setup_wm_latency(dev_priv);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state.
2016-11-08 12:55 ` [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
@ 2016-11-10 0:16 ` Matt Roper
0 siblings, 0 replies; 24+ messages in thread
From: Matt Roper @ 2016-11-10 0:16 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Nov 08, 2016 at 01:55:34PM +0100, Maarten Lankhorst wrote:
> This is the last bit required for making nonblocking modesets work
> correctly. The state in intel_crtc->hw_ddb is not updated until
> somewhere in atomic commit, while the previous crtc state should be
> accurate if the ddb hasn't changed.
The "somewhere in atomic commit" part of the commit message here is a
little bit confusing given that the change to skl_update_crtcs is
happening "somewhere in atomic commit." The key being that your
comparison happens earlier in the commit phase than the hw_ddb update.
I'm not sure exactly how best to word that, but if you can convey that
in the commit message, then the changes here look fine so
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index abfcaa07bbe3..69f9addb29b3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14313,7 +14313,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> * new ddb allocation to take effect.
> */
> if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> - &intel_crtc->hw_ddb) &&
> + &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> !crtc->state->active_changed &&
> intel_state->wm_results.dirty_pipes != updated)
> vbl_wait = true;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 93e261300ef0..bde6c68eb0db 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3118,7 +3118,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> * we currently hold.
> */
> if (!intel_state->active_pipe_changes) {
> - *alloc = to_intel_crtc(for_crtc)->hw_ddb;
> + /*
> + * alloc may be cleared by clear_intel_crtc_state,
> + * copy from old state to be sure
> + */
> + *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
> return;
> }
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
2016-11-08 12:55 ` [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
@ 2016-11-10 0:52 ` Matt Roper
2016-11-10 6:59 ` Maarten Lankhorst
0 siblings, 1 reply; 24+ messages in thread
From: Matt Roper @ 2016-11-10 0:52 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Nov 08, 2016 at 01:55:35PM +0100, Maarten Lankhorst wrote:
> This member is only used in skl_update_crtcs now. It's easy to remove it
> by keeping track of which ddb entries in an array, and update them after
I'm having trouble parsing this line...not sure if you have an extra
word or are missing a word. But I think what you meant is that you're
snapshotting the DDB values at the beginning of this function so that
you'll have a copy of what the 'old' values that were already in the
hardware , then you update that snapshot as you write the DDB for pipe
to the hardware?
> we update the crtc. This removes the last bits of SKL-style watermarks
> kept outside of crtc_state.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 11 +++--------
> drivers/gpu/drm/i915/intel_pm.c | 25 +++++++------------------
> 3 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69f9addb29b3..e59adb03933e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> unsigned int updated = 0;
> bool progress;
> enum pipe pipe;
> + int i;
> +
> + const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
Do we want this to be const?
> +
> + for_each_crtc_in_state(state, crtc, old_crtc_state, i)
> + /* ignore allocations for crtc's that have been turned off. */
> + if (crtc->state->active)
> + entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>
> /*
> * Whenever the number of active pipes changes, we need to make sure we
> @@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> * cause pipe underruns and other bad stuff.
> */
> do {
> - int i;
> progress = false;
>
> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> cstate = to_intel_crtc_state(crtc->state);
> pipe = intel_crtc->pipe;
>
> - if (updated & cmask || !crtc->state->active)
> + if (updated & cmask || !cstate->base.active)
This change seems unrelated/unnecessary? cstate was just set a couple
lines above, so this is effectively just replacing crtc->state with
to_intel_crtc_state(crtc->state)->base.
Matt
> continue;
> - if (skl_ddb_allocation_overlaps(state, intel_crtc))
> +
> + if (skl_ddb_allocation_overlaps(entries, &cstate->wm.skl.ddb, i))
> continue;
>
> updated |= cmask;
> + entries[i] = &cstate->wm.skl.ddb;
>
> /*
> * If this is an already active pipe, it's DDB changed,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d18444097e1c..815e8dae3904 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -728,9 +728,6 @@ struct intel_crtc {
> bool cxsr_allowed;
> } wm;
>
> - /* gen9+: ddb allocation currently being used */
> - struct skl_ddb_entry hw_ddb;
> -
> int scanline_offset;
>
> struct {
> @@ -1738,11 +1735,9 @@ int intel_enable_sagv(struct drm_i915_private *dev_priv);
> int intel_disable_sagv(struct drm_i915_private *dev_priv);
> bool skl_wm_level_equals(const struct skl_wm_level *l1,
> const struct skl_wm_level *l2);
> -bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> - const struct skl_ddb_allocation *new,
> - enum pipe pipe);
> -bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> - struct intel_crtc *intel_crtc);
> +bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
> + const struct skl_ddb_entry *ddb,
> + int ignore);
> uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> bool ilk_disable_lp_wm(struct drm_device *dev);
> int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bde6c68eb0db..02f52b52a03d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3914,25 +3914,16 @@ static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
> return a->start < b->end && b->start < a->end;
> }
>
> -bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> - struct intel_crtc *intel_crtc)
> -{
> - struct drm_crtc *other_crtc;
> - struct drm_crtc_state *other_cstate;
> - struct intel_crtc *other_intel_crtc;
> - const struct skl_ddb_entry *ddb =
> - &to_intel_crtc_state(intel_crtc->base.state)->wm.skl.ddb;
> +bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
> + const struct skl_ddb_entry *ddb,
> + int ignore)
> +{
> int i;
>
> - for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
> - other_intel_crtc = to_intel_crtc(other_crtc);
> -
> - if (other_intel_crtc == intel_crtc)
> - continue;
> -
> - if (skl_ddb_entries_overlap(ddb, &other_intel_crtc->hw_ddb))
> + for (i = 0; i < I915_MAX_PIPES; i++)
> + if (i != ignore && entries[i] &&
> + skl_ddb_entries_overlap(ddb, entries[i]))
> return true;
> - }
>
> return false;
> }
> @@ -4242,8 +4233,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
>
> skl_copy_wm_for_pipe(hw_vals, results, pipe);
>
> - intel_crtc->hw_ddb = cstate->wm.skl.ddb;
> -
> mutex_unlock(&dev_priv->wm.wm_mutex);
> }
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
2016-11-10 0:52 ` Matt Roper
@ 2016-11-10 6:59 ` Maarten Lankhorst
2016-11-10 23:08 ` Matt Roper
0 siblings, 1 reply; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-10 6:59 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
Op 10-11-16 om 01:52 schreef Matt Roper:
> On Tue, Nov 08, 2016 at 01:55:35PM +0100, Maarten Lankhorst wrote:
>> This member is only used in skl_update_crtcs now. It's easy to remove it
>> by keeping track of which ddb entries in an array, and update them after
> I'm having trouble parsing this line...not sure if you have an extra
> word or are missing a word. But I think what you meant is that you're
> snapshotting the DDB values at the beginning of this function so that
> you'll have a copy of what the 'old' values that were already in the
> hardware , then you update that snapshot as you write the DDB for pipe
> to the hardware?
>
>> we update the crtc. This removes the last bits of SKL-style watermarks
>> kept outside of crtc_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
>> drivers/gpu/drm/i915/intel_drv.h | 11 +++--------
>> drivers/gpu/drm/i915/intel_pm.c | 25 +++++++------------------
>> 3 files changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69f9addb29b3..e59adb03933e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>> unsigned int updated = 0;
>> bool progress;
>> enum pipe pipe;
>> + int i;
>> +
>> + const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
> Do we want this to be const?
What is intended here is that the struct skl_ddb_entry is const. The pointers can be reassigned,
and point to either before state or after state, but the values are unmodified. :)
>> +
>> + for_each_crtc_in_state(state, crtc, old_crtc_state, i)
>> + /* ignore allocations for crtc's that have been turned off. */
>> + if (crtc->state->active)
>> + entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>>
>> /*
>> * Whenever the number of active pipes changes, we need to make sure we
>> @@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>> * cause pipe underruns and other bad stuff.
>> */
>> do {
>> - int i;
>> progress = false;
>>
>> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> @@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>> cstate = to_intel_crtc_state(crtc->state);
>> pipe = intel_crtc->pipe;
>>
>> - if (updated & cmask || !crtc->state->active)
>> + if (updated & cmask || !cstate->base.active)
> This change seems unrelated/unnecessary? cstate was just set a couple
> lines above, so this is effectively just replacing crtc->state with
> to_intel_crtc_state(crtc->state)->base.
I'm planning to replace all iterations over state with a new macro that has old and new state,
in which case dereferencing crtc->state directly becomes unneeded and dangerous if we ever
implement queue depth > 1.
I also plan to add some new macros that can pull the new obj->state from drm_atomic_state, or
(with locking verification) from the current state. This should kill all direct obj->state
dereferences.
It's the same conversion we did with dev -> dev_priv cleanups, I try to sneak those conversions
in where it makes sense. :)
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset
2016-11-09 12:45 ` Maarten Lankhorst
@ 2016-11-10 10:53 ` Ville Syrjälä
2016-11-10 14:32 ` Maarten Lankhorst
0 siblings, 1 reply; 24+ messages in thread
From: Ville Syrjälä @ 2016-11-10 10:53 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Wed, Nov 09, 2016 at 01:45:20PM +0100, Maarten Lankhorst wrote:
> Op 08-11-16 om 15:11 schreef Ville Syrjälä:
> > On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
> >> This is a hack and not needed. Use the right mask by checking
> >> intel_state->modeset. This works for watermark sanitization too.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
> >> 1 file changed, 15 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 02f52b52a03d..d38a46efcfed 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> >> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >> struct drm_i915_private *dev_priv = to_i915(dev);
> >> struct drm_crtc *for_crtc = cstate->base.crtc;
> >> - unsigned int pipe_size, ddb_size;
> >> + unsigned int pipe_size, ddb_size, active_crtcs;
> >> int nth_active_pipe;
> >>
> >> + if (intel_state->modeset)
> >> + active_crtcs = intel_state->active_crtcs;
> >> + else
> >> + active_crtcs = dev_priv->active_crtcs;
> > What's the story with the locking here?
> if !modeset, 3 things can happen:
> 1. fastset, connection_mutex held, dev_priv->active_crtcs cannot change.
> 2. crtc disabled, active_crtcs is potentially garbage, but harmless since we don't write disabled wm's when the crtc is already disabled.
> (same as what happens currently)
But we still compute them? Can the computation fail on account of
that? I guess it shouldn't because when things are disabled all wms
should come out as 0?
> 3. crtc enabled, dev_priv->active_crtcs is valid because ddb reallocation requires locking all active crtc's for reallocation,
> which requires taking this lock for this crtc.
Case 3 at least looks a little suspect to me. What the code does is:
for_each_intel_crtc_in_mask(realloc_pipes) {
intel_atomic_get_crtc_state();
skl_allocate_pipe_ddb() {
skl_ddb_get_pipe_allocation_limits();
}
...
}
Since it starts to compute this stuff already before it's necessarily
locked all the crtcs, I'm having a hard time convincing myself that
the state will always be up to date. I guess it might since
realloc_pipes should only have the one pipe unless active_crtcs is going
to change, but I don't think it's obvious at all when looking at the
code. Might want to change the way this is done for clarity if nothing
else.
I think we need to start documenting the locking rules for this kind
of device wide state better, and maybe try to sprinkle more locking
asserts around.
--
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] 24+ messages in thread
* Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset
2016-11-10 10:53 ` Ville Syrjälä
@ 2016-11-10 14:32 ` Maarten Lankhorst
0 siblings, 0 replies; 24+ messages in thread
From: Maarten Lankhorst @ 2016-11-10 14:32 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 10-11-16 om 11:53 schreef Ville Syrjälä:
> On Wed, Nov 09, 2016 at 01:45:20PM +0100, Maarten Lankhorst wrote:
>> Op 08-11-16 om 15:11 schreef Ville Syrjälä:
>>> On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
>>>> This is a hack and not needed. Use the right mask by checking
>>>> intel_state->modeset. This works for watermark sanitization too.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
>>>> 1 file changed, 15 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 02f52b52a03d..d38a46efcfed 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>>> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>>> struct drm_crtc *for_crtc = cstate->base.crtc;
>>>> - unsigned int pipe_size, ddb_size;
>>>> + unsigned int pipe_size, ddb_size, active_crtcs;
>>>> int nth_active_pipe;
>>>>
>>>> + if (intel_state->modeset)
>>>> + active_crtcs = intel_state->active_crtcs;
>>>> + else
>>>> + active_crtcs = dev_priv->active_crtcs;
>>> What's the story with the locking here?
>> if !modeset, 3 things can happen:
>> 1. fastset, connection_mutex held, dev_priv->active_crtcs cannot change.
>> 2. crtc disabled, active_crtcs is potentially garbage, but harmless since we don't write disabled wm's when the crtc is already disabled.
>> (same as what happens currently)
> But we still compute them? Can the computation fail on account of
> that? I guess it shouldn't because when things are disabled all wms
> should come out as 0?
Indeed, they come out as zero so it can't fail, regardless of the value of active_crtcs. :)
>> 3. crtc enabled, dev_priv->active_crtcs is valid because ddb reallocation requires locking all active crtc's for reallocation,
>> which requires taking this lock for this crtc.
> Case 3 at least looks a little suspect to me. What the code does is:
>
> for_each_intel_crtc_in_mask(realloc_pipes) {
> intel_atomic_get_crtc_state();
> skl_allocate_pipe_ddb() {
> skl_ddb_get_pipe_allocation_limits();
> }
> ...
> }
>
> Since it starts to compute this stuff already before it's necessarily
> locked all the crtcs, I'm having a hard time convincing myself that
> the state will always be up to date. I guess it might since
> realloc_pipes should only have the one pipe unless active_crtcs is going
> to change, but I don't think it's obvious at all when looking at the
> code. Might want to change the way this is done for clarity if nothing
> else.
>
> I think we need to start documenting the locking rules for this kind
> of device wide state better, and maybe try to sprinkle more locking
> asserts around.
Parallel enables are handled by connection_mutex, so only 1 can happen at the same time.
It can race with a parallel crtc update to another crtc, but either way the ddb value is the same,
so it's harmless as long as it grabs the crtc_state at some point before committing.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
2016-11-10 6:59 ` Maarten Lankhorst
@ 2016-11-10 23:08 ` Matt Roper
0 siblings, 0 replies; 24+ messages in thread
From: Matt Roper @ 2016-11-10 23:08 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Nov 10, 2016 at 07:59:05AM +0100, Maarten Lankhorst wrote:
> Op 10-11-16 om 01:52 schreef Matt Roper:
> > On Tue, Nov 08, 2016 at 01:55:35PM +0100, Maarten Lankhorst wrote:
> >> This member is only used in skl_update_crtcs now. It's easy to remove it
> >> by keeping track of which ddb entries in an array, and update them after
> > I'm having trouble parsing this line...not sure if you have an extra
> > word or are missing a word. But I think what you meant is that you're
> > snapshotting the DDB values at the beginning of this function so that
> > you'll have a copy of what the 'old' values that were already in the
> > hardware , then you update that snapshot as you write the DDB for pipe
> > to the hardware?
> >
> >> we update the crtc. This removes the last bits of SKL-style watermarks
> >> kept outside of crtc_state.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
> >> drivers/gpu/drm/i915/intel_drv.h | 11 +++--------
> >> drivers/gpu/drm/i915/intel_pm.c | 25 +++++++------------------
> >> 3 files changed, 22 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 69f9addb29b3..e59adb03933e 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> unsigned int updated = 0;
> >> bool progress;
> >> enum pipe pipe;
> >> + int i;
> >> +
> >> + const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
> > Do we want this to be const?
> What is intended here is that the struct skl_ddb_entry is const. The pointers can be reassigned,
> and point to either before state or after state, but the values are unmodified. :)
Ah, okay, that makes sense.
> >> +
> >> + for_each_crtc_in_state(state, crtc, old_crtc_state, i)
> >> + /* ignore allocations for crtc's that have been turned off. */
> >> + if (crtc->state->active)
> >> + entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
> >>
> >> /*
> >> * Whenever the number of active pipes changes, we need to make sure we
> >> @@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> * cause pipe underruns and other bad stuff.
> >> */
> >> do {
> >> - int i;
> >> progress = false;
> >>
> >> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> >> @@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> cstate = to_intel_crtc_state(crtc->state);
> >> pipe = intel_crtc->pipe;
> >>
> >> - if (updated & cmask || !crtc->state->active)
> >> + if (updated & cmask || !cstate->base.active)
> > This change seems unrelated/unnecessary? cstate was just set a couple
> > lines above, so this is effectively just replacing crtc->state with
> > to_intel_crtc_state(crtc->state)->base.
>
> I'm planning to replace all iterations over state with a new macro that has old and new state,
> in which case dereferencing crtc->state directly becomes unneeded and dangerous if we ever
> implement queue depth > 1.
>
> I also plan to add some new macros that can pull the new obj->state from drm_atomic_state, or
> (with locking verification) from the current state. This should kill all direct obj->state
> dereferences.
>
> It's the same conversion we did with dev -> dev_priv cleanups, I try to sneak those conversions
> in where it makes sense. :)
Okay, fair enough. Just wanted to make sure it wasn't something you'd
mis-squashed into this patch without realizing it.
If you update the confusing wording in the commit message that I pointed
out above, you can consider this
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> ~Maarten
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-11-10 23:08 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-08 12:55 [PATCH v3 00/11] Skylake watermark fixes and nonblocking modeset Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
2016-11-10 0:14 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 02/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3 Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 03/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
2016-11-10 0:16 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
2016-11-10 0:52 ` Matt Roper
2016-11-10 6:59 ` Maarten Lankhorst
2016-11-10 23:08 ` Matt Roper
2016-11-08 12:55 ` [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset Maarten Lankhorst
2016-11-08 14:11 ` Ville Syrjälä
2016-11-09 12:45 ` Maarten Lankhorst
2016-11-10 10:53 ` Ville Syrjälä
2016-11-10 14:32 ` Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 06/11] drm/i915: Convert intel_hdmi to use atomic state Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 07/11] drm/i915: Pass atomic state to intel_audio_codec_enable, v2 Maarten Lankhorst
2016-11-08 14:07 ` Ville Syrjälä
2016-11-08 12:55 ` [PATCH v3 08/11] drm/edid: Remove drm_select_eld Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 09/11] drm/i915: Update atomic modeset state synchronously, v2 Maarten Lankhorst
2016-11-08 14:08 ` Ville Syrjälä
2016-11-08 12:55 ` [PATCH v3 10/11] drm/i915: Pass atomic state to verify_connector_state Maarten Lankhorst
2016-11-08 12:55 ` [PATCH v3 11/11] drm/i915: Enable support for nonblocking modeset Maarten Lankhorst
2016-11-08 13:45 ` ✓ Fi.CI.BAT: success for Skylake watermark fixes and " 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.