* [RFC PATCH 1/7] drm/i915: Do not update pfit state when toggling crtc enabled.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2 Maarten Lankhorst
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
This must be done in advance, and during crtc_disable all scalers
can be force disabled.
This means intel_atomic_setup_scalers is only called in 1 place now,
during crtc_check.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 14 ++------
drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++--------------
drivers/gpu/drm/i915/intel_dp.c | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 2 +-
4 files changed, 47 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 0aeced82201e..429677a111d5 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -272,17 +272,12 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
struct drm_plane *plane = NULL;
struct intel_plane *intel_plane;
struct intel_plane_state *plane_state = NULL;
- struct intel_crtc_scaler_state *scaler_state;
- struct drm_atomic_state *drm_state;
+ struct intel_crtc_scaler_state *scaler_state =
+ &crtc_state->scaler_state;
+ struct drm_atomic_state *drm_state = crtc_state->base.state;
int num_scalers_need;
int i, j;
- if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
- return 0;
-
- scaler_state = &crtc_state->scaler_state;
- drm_state = crtc_state->base.state;
-
num_scalers_need = hweight32(scaler_state->scaler_users);
DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
crtc_state, num_scalers_need, intel_crtc->num_scalers,
@@ -327,9 +322,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
name = "PLANE";
idx = plane->base.id;
- if (!drm_state)
- continue;
-
/* plane scaler case: assign as a plane scaler */
/* find the plane that set the bit as scaler_user */
plane = drm_state->planes[i];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 59430e4c361f..b3d8d1a30a04 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2898,29 +2898,32 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
return i915_gem_obj_ggtt_offset_view(obj, view);
}
+static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
+{
+ struct drm_device *dev = intel_crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, id), 0);
+ I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, id), 0);
+ I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, id), 0);
+ DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
+ intel_crtc->base.base.id, intel_crtc->pipe, id);
+}
+
/*
* This function detaches (aka. unbinds) unused scalers in hardware
*/
static void skl_detach_scalers(struct intel_crtc *intel_crtc)
{
- struct drm_device *dev;
- struct drm_i915_private *dev_priv;
struct intel_crtc_scaler_state *scaler_state;
int i;
- dev = intel_crtc->base.dev;
- dev_priv = dev->dev_private;
scaler_state = &intel_crtc->config->scaler_state;
/* loop through and disable scalers that aren't in use */
for (i = 0; i < intel_crtc->num_scalers; i++) {
- if (!scaler_state->scalers[i].in_use) {
- I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0);
- I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0);
- I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0);
- DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
- intel_crtc->base.base.id, intel_crtc->pipe, i);
- }
+ if (!scaler_state->scalers[i].in_use)
+ skl_detach_scaler(intel_crtc, i);
}
}
@@ -4351,13 +4354,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
* skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
*
* @state: crtc's scaler state
- * @force_detach: whether to forcibly disable scaler
*
* Return
* 0 - scaler_usage updated successfully
* error - requested scaling cannot be supported or other error condition
*/
-int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
+int skl_update_scaler_crtc(struct intel_crtc_state *state)
{
struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
struct drm_display_mode *adjusted_mode =
@@ -4366,7 +4368,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n",
intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
- return skl_update_scaler(state, force_detach, SKL_CRTC_INDEX,
+ return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
&state->scaler_state.scaler_id, DRM_ROTATE_0,
state->pipe_src_w, state->pipe_src_h,
adjusted_mode->hdisplay, adjusted_mode->hdisplay);
@@ -4440,7 +4442,15 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
return 0;
}
-static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
+static void skylake_scaler_disable(struct intel_crtc *crtc)
+{
+ int i;
+
+ for (i = 0; i < crtc->num_scalers; i++)
+ skl_detach_scaler(crtc, i);
+}
+
+static void skylake_pfit_enable(struct intel_crtc *crtc)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4450,13 +4460,6 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
- /* To update pfit, first update scaler state */
- skl_update_scaler_crtc(crtc->config, !enable);
- intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
- skl_detach_scalers(crtc);
- if (!enable)
- return;
-
if (crtc->config->pch_pfit.enabled) {
int id;
@@ -4934,7 +4937,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
intel_ddi_enable_pipe_clock(intel_crtc);
if (INTEL_INFO(dev)->gen == 9)
- skylake_pfit_update(intel_crtc, 1);
+ skylake_pfit_enable(intel_crtc);
else if (INTEL_INFO(dev)->gen < 9)
ironlake_pfit_enable(intel_crtc);
else
@@ -5068,7 +5071,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
if (INTEL_INFO(dev)->gen == 9)
- skylake_pfit_update(intel_crtc, 0);
+ skylake_scaler_disable(intel_crtc);
else if (INTEL_INFO(dev)->gen < 9)
ironlake_pfit_disable(intel_crtc);
else
@@ -11846,7 +11849,16 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
return ret;
}
- return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
+ ret = 0;
+ if (INTEL_INFO(dev)->gen >= 9) {
+ if (mode_changed)
+ ret = skl_update_scaler_crtc(pipe_config);
+
+ if (!ret)
+ ret = intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
+ }
+
+ return ret;
}
static const struct drm_crtc_helper_funcs intel_helper_funcs = {
@@ -15360,6 +15372,11 @@ static void readout_plane_state(struct intel_crtc *crtc,
continue;
drm_plane_state = p->base.state;
+
+ /* Plane scaler state is not touched here. The first atomic
+ * commit will restore all plane scalers to its old state.
+ */
+
if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
visible = primary_get_hw_state(crtc);
to_intel_plane_state(drm_plane_state)->visible = visible;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6141a702ba36..daf47b1de76c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1377,7 +1377,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
if (INTEL_INFO(dev)->gen >= 9) {
int ret;
- ret = skl_update_scaler_crtc(pipe_config, 0);
+ ret = skl_update_scaler_crtc(pipe_config);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 853bfd98ef72..4b185564aeea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1149,7 +1149,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
-int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
+int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 1/7] drm/i915: Do not update pfit state when toggling crtc enabled Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-19 9:38 ` Daniel Stone
2015-06-22 15:31 ` Daniel Vetter
2015-06-19 8:02 ` [RFC PATCH 3/7] All changes from try2 Maarten Lankhorst
` (5 subsequent siblings)
7 siblings, 2 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
To make this work we load the new hardware state into the
atomic_state, then swap it with the sw state.
This lets us change the force restore path in setup_hw_state()
to use a single call to intel_mode_set() to restore all the
previous state.
As a nice bonus this kills off encoder->new_encoder,
connector->new_enabled and crtc->new_enabled. They were used only
to restore the state after a modeset.
Change since try1:
- Fix the regressions caused by not updating drm core sw state.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 381 ++++++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_drv.h | 14 +-
3 files changed, 243 insertions(+), 154 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..4d87574a2032 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -398,7 +398,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
return 0;
}
-static void
+void
intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
struct intel_shared_dpll_config *shared_dpll)
{
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b3d8d1a30a04..41193dab58c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10231,7 +10231,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
retry:
ret = drm_modeset_lock(&config->connection_mutex, ctx);
if (ret)
- goto fail_unlock;
+ goto fail;
/*
* Algorithm gets a little messy:
@@ -10249,10 +10249,10 @@ retry:
ret = drm_modeset_lock(&crtc->mutex, ctx);
if (ret)
- goto fail_unlock;
+ goto fail;
ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
if (ret)
- goto fail_unlock;
+ goto fail;
old->dpms_mode = connector->dpms;
old->load_detect_temp = false;
@@ -10271,9 +10271,6 @@ retry:
continue;
if (possible_crtc->state->enable)
continue;
- /* This can occur when applying the pipe A quirk on resume. */
- if (to_intel_crtc(possible_crtc)->new_enabled)
- continue;
crtc = possible_crtc;
break;
@@ -10284,20 +10281,17 @@ retry:
*/
if (!crtc) {
DRM_DEBUG_KMS("no pipe available for load-detect\n");
- goto fail_unlock;
+ goto fail;
}
ret = drm_modeset_lock(&crtc->mutex, ctx);
if (ret)
- goto fail_unlock;
+ goto fail;
ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
if (ret)
- goto fail_unlock;
- intel_encoder->new_crtc = to_intel_crtc(crtc);
- to_intel_connector(connector)->new_encoder = intel_encoder;
+ goto fail;
intel_crtc = to_intel_crtc(crtc);
- intel_crtc->new_enabled = true;
old->dpms_mode = connector->dpms;
old->load_detect_temp = true;
old->release_fb = NULL;
@@ -10365,9 +10359,7 @@ retry:
intel_wait_for_vblank(dev, intel_crtc->pipe);
return true;
- fail:
- intel_crtc->new_enabled = crtc->state->enable;
-fail_unlock:
+fail:
drm_atomic_state_free(state);
state = NULL;
@@ -10413,10 +10405,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
if (IS_ERR(crtc_state))
goto fail;
- to_intel_connector(connector)->new_encoder = NULL;
- intel_encoder->new_crtc = NULL;
- intel_crtc->new_enabled = false;
-
connector_state->best_encoder = NULL;
connector_state->crtc = NULL;
@@ -11869,33 +11857,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
.atomic_check = intel_crtc_atomic_check,
};
-/**
- * intel_modeset_update_staged_output_state
- *
- * Updates the staged output configuration state, e.g. after we've read out the
- * current hw state.
- */
-static void intel_modeset_update_staged_output_state(struct drm_device *dev)
-{
- struct intel_crtc *crtc;
- struct intel_encoder *encoder;
- struct intel_connector *connector;
-
- for_each_intel_connector(dev, connector) {
- connector->new_encoder =
- to_intel_encoder(connector->base.encoder);
- }
-
- for_each_intel_encoder(dev, encoder) {
- encoder->new_crtc =
- to_intel_crtc(encoder->base.crtc);
- }
-
- for_each_intel_crtc(dev, crtc) {
- crtc->new_enabled = crtc->base.state->enable;
- }
-}
-
/* Transitional helper to copy current connector/encoder state to
* connector->state. This is needed so that code that is partially
* converted to atomic does the right thing.
@@ -12349,7 +12310,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
}
drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
- intel_modeset_update_staged_output_state(state->dev);
/* Double check state. */
for_each_crtc(dev, crtc) {
@@ -12659,11 +12619,14 @@ check_connector_state(struct drm_device *dev)
struct intel_connector *connector;
for_each_intel_connector(dev, connector) {
+ struct drm_encoder *encoder = connector->base.encoder;
+ struct drm_connector_state *state = connector->base.state;
+
/* This also checks the encoder/connector hw state with the
* ->get_hw_state callbacks. */
intel_connector_check_state(connector);
- I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
+ I915_STATE_WARN(state->best_encoder != encoder,
"connector's staged encoder doesn't match current encoder\n");
}
}
@@ -12683,8 +12646,6 @@ check_encoder_state(struct drm_device *dev)
encoder->base.base.id,
encoder->base.name);
- I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
- "encoder's stage crtc doesn't match current crtc\n");
I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
"encoder's active_connectors set, but no crtc\n");
@@ -12694,6 +12655,9 @@ check_encoder_state(struct drm_device *dev)
enabled = true;
if (connector->base.dpms != DRM_MODE_DPMS_OFF)
active = true;
+
+ I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
+ "encoder's stage crtc doesn't match current crtc\n");
}
/*
* for MST connectors if we unplug the connector is gone
@@ -13255,11 +13219,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
* need to copy the staged config to the atomic state, otherwise the
* mode set will just reapply the state the HW is already in. */
for_each_intel_encoder(dev, encoder) {
- if (&encoder->new_crtc->base != crtc)
+ if (encoder->base.crtc != crtc)
continue;
for_each_intel_connector(dev, connector) {
- if (connector->new_encoder != encoder)
+ if (connector->base.state->best_encoder != &encoder->base)
continue;
connector_state = drm_atomic_get_connector_state(state, &connector->base);
@@ -13272,14 +13236,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
}
connector_state->crtc = crtc;
- connector_state->best_encoder = &encoder->base;
}
}
for_each_intel_crtc(dev, intel_crtc) {
- if (intel_crtc->new_enabled == intel_crtc->base.enabled)
- continue;
-
crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
if (IS_ERR(crtc_state)) {
DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
@@ -13288,9 +13248,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
continue;
}
- crtc_state->base.active = crtc_state->base.enable =
- intel_crtc->new_enabled;
-
if (&intel_crtc->base == crtc)
drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
}
@@ -15181,8 +15138,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
/* restore vblank interrupts to correct state */
drm_crtc_vblank_reset(&crtc->base);
- if (crtc->active) {
+ if (crtc->base.state->active) {
update_scanline_offset(crtc);
+ drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
drm_crtc_vblank_on(&crtc->base);
}
@@ -15350,12 +15308,14 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
}
-static void readout_plane_state(struct intel_crtc *crtc,
+static int readout_plane_state(struct drm_atomic_state *state,
+ struct intel_crtc *crtc,
struct intel_crtc_state *crtc_state)
{
struct intel_plane *p;
struct drm_plane_state *drm_plane_state;
bool active = crtc_state->base.active;
+ int ret = 0;
if (active) {
crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
@@ -15371,7 +15331,11 @@ static void readout_plane_state(struct intel_crtc *crtc,
if (crtc->pipe != p->pipe)
continue;
- drm_plane_state = p->base.state;
+ drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
+ if (IS_ERR(drm_plane_state)) {
+ ret = PTR_ERR(drm_plane_state);
+ break;
+ }
/* Plane scaler state is not touched here. The first atomic
* commit will restore all plane scalers to its old state.
@@ -15388,101 +15352,215 @@ static void readout_plane_state(struct intel_crtc *crtc,
to_intel_plane_state(drm_plane_state)->visible = false;
}
+ p->base.crtc = drm_plane_state->crtc;
if (visible) {
crtc_state->base.plane_mask |=
1 << drm_plane_index(&p->base);
- } else if (crtc_state->base.state) {
- /* Make this unconditional for atomic hw readout. */
+ } else {
crtc_state->base.plane_mask &=
~(1 << drm_plane_index(&p->base));
}
}
+
+ return ret;
}
-static void intel_modeset_readout_hw_state(struct drm_device *dev)
+static int readout_hw_crtc_state(struct drm_atomic_state *state,
+ struct intel_crtc *crtc)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- enum pipe pipe;
- struct intel_crtc *crtc;
- struct intel_encoder *encoder;
- struct intel_connector *connector;
- int i;
+ struct drm_i915_private *dev_priv = to_i915(state->dev);
+ struct intel_crtc_state *crtc_state;
+ int ret;
- for_each_intel_crtc(dev, crtc) {
- memset(crtc->config, 0, sizeof(*crtc->config));
- crtc->config->base.crtc = &crtc->base;
+ crtc_state = intel_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
- crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
+ ret = drm_atomic_add_affected_planes(state, &crtc->base);
+ if (ret)
+ return ret;
- crtc->active = dev_priv->display.get_pipe_config(crtc,
- crtc->config);
+ memset(crtc_state, 0, sizeof(*crtc_state));
+ crtc_state->base.crtc = &crtc->base;
+ crtc_state->base.state = state;
- crtc->base.state->enable = crtc->active;
- crtc->base.state->active = crtc->active;
- crtc->base.enabled = crtc->active;
- crtc->base.hwmode = crtc->config->base.adjusted_mode;
+ crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
- readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
+ crtc_state->base.enable = crtc_state->base.active =
+ crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
- DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
- crtc->base.base.id,
- crtc->active ? "enabled" : "disabled");
- }
+ /* update transitional state */
+ crtc->active = crtc_state->base.active;
+ crtc->config = crtc_state;
+
+ DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
+ crtc->base.base.id,
+ crtc_state->base.active ? "enabled" : "disabled");
+
+ return readout_plane_state(state, crtc, crtc_state);
+}
+static int readout_hw_pll_state(struct drm_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->dev);
+ struct intel_shared_dpll_config *shared_dpll;
+ struct intel_crtc *crtc;
+ struct intel_crtc_state *crtc_state;
+ int i;
+
+ shared_dpll = intel_atomic_get_shared_dpll_state(state);
for (i = 0; i < dev_priv->num_shared_dpll; i++) {
struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
pll->on = pll->get_hw_state(dev_priv, pll,
- &pll->config.hw_state);
+ &shared_dpll[i].hw_state);
+
pll->active = 0;
- pll->config.crtc_mask = 0;
- for_each_intel_crtc(dev, crtc) {
- if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
+ shared_dpll[i].crtc_mask = 0;
+
+ for_each_intel_crtc(state->dev, crtc) {
+ crtc_state = intel_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ if (crtc_state->base.active &&
+ crtc_state->shared_dpll == i) {
pll->active++;
- pll->config.crtc_mask |= 1 << crtc->pipe;
+ shared_dpll[i].crtc_mask |=
+ 1 << crtc->pipe;
}
}
DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
- pll->name, pll->config.crtc_mask, pll->on);
+ pll->name, shared_dpll[i].crtc_mask,
+ pll->on);
- if (pll->config.crtc_mask)
+ if (shared_dpll[i].crtc_mask)
intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
}
- for_each_intel_encoder(dev, encoder) {
- pipe = 0;
+ return 0;
+}
- if (encoder->get_hw_state(encoder, &pipe)) {
- crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
- encoder->base.crtc = &crtc->base;
- encoder->get_config(encoder, crtc->config);
- } else {
- encoder->base.crtc = NULL;
- }
+static struct drm_connector_state *
+get_connector_state_for_encoder(struct drm_atomic_state *state,
+ struct intel_encoder *encoder)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *connector_state;
+ int i;
- encoder->connectors_active = false;
- DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
- encoder->base.base.id,
- encoder->base.name,
- encoder->base.crtc ? "enabled" : "disabled",
- pipe_name(pipe));
- }
+ for_each_connector_in_state(state, connector, connector_state, i)
+ if (connector_state->best_encoder == &encoder->base)
+ return connector_state;
+
+ return NULL;
+}
+
+static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
+{
+ struct drm_device *dev = state->dev;
+ struct drm_i915_private *dev_priv = to_i915(state->dev);
+ struct intel_crtc *crtc;
+ struct drm_crtc_state *drm_crtc_state;
+ struct intel_crtc_state *crtc_state;
+ struct intel_encoder *encoder;
+ struct intel_connector *connector;
+ struct drm_connector_state *connector_state;
+ enum pipe pipe;
for_each_intel_connector(dev, connector) {
+ connector_state =
+ drm_atomic_get_connector_state(state, &connector->base);
+ if (IS_ERR(connector_state))
+ return PTR_ERR(connector_state);
+
if (connector->get_hw_state(connector)) {
connector->base.dpms = DRM_MODE_DPMS_ON;
- connector->encoder->connectors_active = true;
connector->base.encoder = &connector->encoder->base;
} else {
connector->base.dpms = DRM_MODE_DPMS_OFF;
connector->base.encoder = NULL;
}
+
+ /* We'll update the crtc field when reading encoder state */
+ connector_state->crtc = NULL;
+ connector_state->best_encoder = connector->base.encoder;
+
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
connector->base.base.id,
connector->base.name,
connector->base.encoder ? "enabled" : "disabled");
}
+
+ for_each_intel_encoder(dev, encoder) {
+ pipe = 0;
+
+ connector_state =
+ get_connector_state_for_encoder(state, encoder);
+
+ encoder->connectors_active = !!connector_state;
+
+ if (encoder->get_hw_state(encoder, &pipe)) {
+ encoder->base.crtc =
+ dev_priv->pipe_to_crtc_mapping[pipe];
+ crtc = to_intel_crtc(encoder->base.crtc);
+
+ drm_crtc_state =
+ state->crtc_states[drm_crtc_index(&crtc->base)];
+ crtc_state = to_intel_crtc_state(drm_crtc_state);
+
+ encoder->get_config(encoder, crtc_state);
+
+ if (connector_state)
+ connector_state->crtc = &crtc->base;
+ } else {
+ encoder->base.crtc = NULL;
+ }
+
+ DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
+ encoder->base.base.id,
+ encoder->base.name,
+ encoder->base.crtc ? "enabled" : "disabled",
+ pipe_name(pipe));
+ }
+
+ return 0;
+}
+
+static struct drm_atomic_state *
+intel_modeset_readout_hw_state(struct drm_device *dev)
+{
+ struct intel_crtc *crtc;
+ int ret = 0;
+
+ struct drm_atomic_state *state;
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state)
+ return ERR_PTR(-ENOMEM);
+
+ state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+ for_each_intel_crtc(dev, crtc) {
+ ret = readout_hw_crtc_state(state, crtc);
+ if (ret)
+ goto err_free;
+ }
+
+ ret = readout_hw_pll_state(state);
+ if (ret)
+ goto err_free;
+
+ ret = readout_hw_connector_encoder_state(state);
+ if (ret)
+ goto err_free;
+
+ return state;
+
+err_free:
+ drm_atomic_state_free(state);
+ return ERR_PTR(ret);
}
/* Scan out the current hw modeset state, sanitizes it and maps it into the drm
@@ -15491,37 +15569,61 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
bool force_restore)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- enum pipe pipe;
- struct intel_crtc *crtc;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
struct intel_encoder *encoder;
+ struct drm_atomic_state *state;
+ struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
int i;
- intel_modeset_readout_hw_state(dev);
-
- /*
- * Now that we have the config, copy it to each CRTC struct
- * Note that this could go away if we move to using crtc_config
- * checking everywhere.
- */
- for_each_intel_crtc(dev, crtc) {
- if (crtc->active && i915.fastboot) {
- intel_mode_from_pipe_config(&crtc->base.mode,
- crtc->config);
- DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
- crtc->base.base.id);
- drm_mode_debug_printmodeline(&crtc->base.mode);
- }
+ state = intel_modeset_readout_hw_state(dev);
+ if (IS_ERR(state)) {
+ DRM_ERROR("Failed to read out hw state\n");
+ return;
}
+ drm_atomic_helper_swap_state(dev, state);
+
+ /* swap sw/hw dpll state */
+ intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
+ intel_shared_dpll_commit(state);
+ memcpy(to_intel_atomic_state(state)->shared_dpll,
+ shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
+
/* HW state is read out, now we need to sanitize this mess. */
for_each_intel_encoder(dev, encoder) {
intel_sanitize_encoder(encoder);
}
- for_each_pipe(dev_priv, pipe) {
- crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
- intel_sanitize_crtc(crtc);
- intel_dump_pipe_config(crtc, crtc->config,
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+ /* prevent unnneeded restores with force_restore */
+ crtc_state->active_changed =
+ crtc_state->mode_changed =
+ crtc_state->planes_changed = false;
+
+ if (crtc->state->enable) {
+ intel_mode_from_pipe_config(&crtc->mode,
+ to_intel_crtc_state(crtc->state));
+ intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
+ to_intel_crtc_state(crtc->state));
+
+ if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode) < 0)
+ drm_mode_copy(&crtc->state->mode, &crtc->mode);
+
+ drm_mode_copy(&crtc->hwmode,
+ &crtc->state->adjusted_mode);
+ }
+
+ intel_sanitize_crtc(intel_crtc);
+
+ /*
+ * sanitize_crtc may have forced an update of crtc->state,
+ * so reload in intel_dump_pipe_config
+ */
+ intel_dump_pipe_config(intel_crtc,
+ to_intel_crtc_state(crtc->state),
"[setup_hw_state]");
}
@@ -15545,20 +15647,17 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
ilk_wm_get_hw_state(dev);
if (force_restore) {
- i915_redisable_vga(dev);
+ int ret;
- /*
- * We need to use raw interfaces for restoring state to avoid
- * checking (bogus) intermediate states.
- */
- for_each_pipe(dev_priv, pipe) {
- struct drm_crtc *crtc =
- dev_priv->pipe_to_crtc_mapping[pipe];
+ i915_redisable_vga(dev);
- intel_crtc_restore_mode(crtc);
+ ret = intel_set_mode(state);
+ if (ret) {
+ DRM_ERROR("Failed to restore previous mode\n");
+ drm_atomic_state_free(state);
}
} else {
- intel_modeset_update_staged_output_state(dev);
+ drm_atomic_state_free(state);
}
intel_modeset_check_state(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4b185564aeea..bc7d0a003c8e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -130,11 +130,6 @@ struct intel_fbdev {
struct intel_encoder {
struct drm_encoder base;
- /*
- * The new crtc this encoder will be driven from. Only differs from
- * base->crtc while a modeset is in progress.
- */
- struct intel_crtc *new_crtc;
enum intel_output_type type;
unsigned int cloneable;
@@ -195,12 +190,6 @@ struct intel_connector {
*/
struct intel_encoder *encoder;
- /*
- * The new encoder this connector will be driven. Only differs from
- * encoder while a modeset is in progress.
- */
- struct intel_encoder *new_encoder;
-
/* Reads out the current hw, returning true if the connector is enabled
* and active (i.e. dpms ON state). */
bool (*get_hw_state)(struct intel_connector *);
@@ -541,7 +530,6 @@ struct intel_crtc {
struct intel_initial_plane_config plane_config;
struct intel_crtc_state *config;
- bool new_enabled;
/* reset counter value when the last flip was submitted */
unsigned int reset_counter;
@@ -1418,6 +1406,8 @@ struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
void intel_atomic_state_clear(struct drm_atomic_state *);
struct intel_shared_dpll_config *
intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
+void intel_atomic_duplicate_dpll_state(struct drm_i915_private *,
+ struct intel_shared_dpll_config *);
static inline struct intel_crtc_state *
intel_atomic_get_crtc_state(struct drm_atomic_state *state,
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.
2015-06-19 8:02 ` [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2 Maarten Lankhorst
@ 2015-06-19 9:38 ` Daniel Stone
2015-06-22 15:31 ` Daniel Vetter
1 sibling, 0 replies; 17+ messages in thread
From: Daniel Stone @ 2015-06-19 9:38 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
Hi,
On 19 June 2015 at 09:02, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> + if (crtc->state->enable) {
> + intel_mode_from_pipe_config(&crtc->mode,
> + to_intel_crtc_state(crtc->state));
> + intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> + to_intel_crtc_state(crtc->state));
> +
> + if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode) < 0)
> + drm_mode_copy(&crtc->state->mode, &crtc->mode);
I've never really understood why this is here. There are only two ways
in which set_mode_for_crtc can fail: either the mode fails validation
(fixed in the patch I sent you the other day, which it seems like
you've now pulled in), or -ENOMEM. The first one we shouldn't really
be papering over - and I haven't seen it happen since that patch - and
the second one isn't really recoverable anyway. Plus I would argue
that breaking the state like that is pretty bad.
Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.
2015-06-19 8:02 ` [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2 Maarten Lankhorst
2015-06-19 9:38 ` Daniel Stone
@ 2015-06-22 15:31 ` Daniel Vetter
2015-06-23 10:49 ` Maarten Lankhorst
1 sibling, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2015-06-22 15:31 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
> /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> @@ -15491,37 +15569,61 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> bool force_restore)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - enum pipe pipe;
> - struct intel_crtc *crtc;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> struct intel_encoder *encoder;
> + struct drm_atomic_state *state;
> + struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
> int i;
>
> - intel_modeset_readout_hw_state(dev);
> -
> - /*
> - * Now that we have the config, copy it to each CRTC struct
> - * Note that this could go away if we move to using crtc_config
> - * checking everywhere.
> - */
> - for_each_intel_crtc(dev, crtc) {
> - if (crtc->active && i915.fastboot) {
> - intel_mode_from_pipe_config(&crtc->base.mode,
> - crtc->config);
> - DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> - crtc->base.base.id);
> - drm_mode_debug_printmodeline(&crtc->base.mode);
> - }
> + state = intel_modeset_readout_hw_state(dev);
> + if (IS_ERR(state)) {
> + DRM_ERROR("Failed to read out hw state\n");
> + return;
> }
>
> + drm_atomic_helper_swap_state(dev, state);
> +
> + /* swap sw/hw dpll state */
> + intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
> + intel_shared_dpll_commit(state);
> + memcpy(to_intel_atomic_state(state)->shared_dpll,
> + shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
This is imo way too much open-coding of stuff that's already there. What I
had in mind for force_restore was a generic helper to duplicate all the
state objects into a drm_atomic_state, without calling any of the
->atomic_check functions or anything. Then the sequence for restoring
state would be.
1. saved_state = drm_atomic_helper_duplicate_state()
2. intel_readout_hw_sate() <- this would just update ->state and sanitize
it.
3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
always works.
Ofc we still need the ww-mutex deadlock avoidance wrapped around this, but
since atomic_commit will first compute all derived state there's no need
to duplicate all the i915-internal state too. That should all work like
with any other normal modeset.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.
2015-06-22 15:31 ` Daniel Vetter
@ 2015-06-23 10:49 ` Maarten Lankhorst
2015-06-23 11:40 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-23 10:49 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 22-06-15 om 17:31 schreef Daniel Vetter:
> On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
>> /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
>> @@ -15491,37 +15569,61 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>> bool force_restore)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - enum pipe pipe;
>> - struct intel_crtc *crtc;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *crtc_state;
>> struct intel_encoder *encoder;
>> + struct drm_atomic_state *state;
>> + struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
>> int i;
>>
>> - intel_modeset_readout_hw_state(dev);
>> -
>> - /*
>> - * Now that we have the config, copy it to each CRTC struct
>> - * Note that this could go away if we move to using crtc_config
>> - * checking everywhere.
>> - */
>> - for_each_intel_crtc(dev, crtc) {
>> - if (crtc->active && i915.fastboot) {
>> - intel_mode_from_pipe_config(&crtc->base.mode,
>> - crtc->config);
>> - DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
>> - crtc->base.base.id);
>> - drm_mode_debug_printmodeline(&crtc->base.mode);
>> - }
>> + state = intel_modeset_readout_hw_state(dev);
>> + if (IS_ERR(state)) {
>> + DRM_ERROR("Failed to read out hw state\n");
>> + return;
>> }
>>
>> + drm_atomic_helper_swap_state(dev, state);
>> +
>> + /* swap sw/hw dpll state */
>> + intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
>> + intel_shared_dpll_commit(state);
>> + memcpy(to_intel_atomic_state(state)->shared_dpll,
>> + shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
> This is imo way too much open-coding of stuff that's already there. What I
> had in mind for force_restore was a generic helper to duplicate all the
> state objects into a drm_atomic_state, without calling any of the
> ->atomic_check functions or anything. Then the sequence for restoring
> state would be.
>
> 1. saved_state = drm_atomic_helper_duplicate_state()
> 2. intel_readout_hw_sate() <- this would just update ->state and sanitize
> it.
> 3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
> always works.
Well, for !force_restore we want to do an initial sanitizing modeset too, but with the full disabled state as reference point.
I'm not sure dupe state followed by a swap, is different from save old state, and read out to current state.
> Ofc we still need the ww-mutex deadlock avoidance wrapped around this, but
> since atomic_commit will first compute all derived state there's no need
> to duplicate all the i915-internal state too. That should all work like
> with any other normal modeset.
No need to wrap, readout touches everything so lock_all is fine.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.
2015-06-23 10:49 ` Maarten Lankhorst
@ 2015-06-23 11:40 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2015-06-23 11:40 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Jun 23, 2015 at 12:49:00PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:31 schreef Daniel Vetter:
> > On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
> >> /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> >> @@ -15491,37 +15569,61 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >> bool force_restore)
> >> {
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> - enum pipe pipe;
> >> - struct intel_crtc *crtc;
> >> + struct drm_crtc *crtc;
> >> + struct drm_crtc_state *crtc_state;
> >> struct intel_encoder *encoder;
> >> + struct drm_atomic_state *state;
> >> + struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
> >> int i;
> >>
> >> - intel_modeset_readout_hw_state(dev);
> >> -
> >> - /*
> >> - * Now that we have the config, copy it to each CRTC struct
> >> - * Note that this could go away if we move to using crtc_config
> >> - * checking everywhere.
> >> - */
> >> - for_each_intel_crtc(dev, crtc) {
> >> - if (crtc->active && i915.fastboot) {
> >> - intel_mode_from_pipe_config(&crtc->base.mode,
> >> - crtc->config);
> >> - DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> >> - crtc->base.base.id);
> >> - drm_mode_debug_printmodeline(&crtc->base.mode);
> >> - }
> >> + state = intel_modeset_readout_hw_state(dev);
> >> + if (IS_ERR(state)) {
> >> + DRM_ERROR("Failed to read out hw state\n");
> >> + return;
> >> }
> >>
> >> + drm_atomic_helper_swap_state(dev, state);
> >> +
> >> + /* swap sw/hw dpll state */
> >> + intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
> >> + intel_shared_dpll_commit(state);
> >> + memcpy(to_intel_atomic_state(state)->shared_dpll,
> >> + shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
> > This is imo way too much open-coding of stuff that's already there. What I
> > had in mind for force_restore was a generic helper to duplicate all the
> > state objects into a drm_atomic_state, without calling any of the
> > ->atomic_check functions or anything. Then the sequence for restoring
> > state would be.
> >
> > 1. saved_state = drm_atomic_helper_duplicate_state()
> > 2. intel_readout_hw_sate() <- this would just update ->state and sanitize
> > it.
> > 3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
> > always works.
> Well, for !force_restore we want to do an initial sanitizing modeset too, but with the full disabled state as reference point.
> I'm not sure dupe state followed by a swap, is different from save old state, and read out to current state.
You also dupe internal state like dplls, that's what a helper-level
duplicat state would avoid. And imo that's cleaner. ->atomic_check should
then grab all the internal states needed, if any are necessary. The goal
really is to avoid another special case with too much knowledge of i915
internals, which atomic really should allow us to do.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFC PATCH 3/7] All changes from try2.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 1/7] drm/i915: Do not update pfit state when toggling crtc enabled Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2 Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 4/7] drm/i915: enable fastboot for everyone Maarten Lankhorst
` (4 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
Always read out plane state, and do an initial modeset in modeset_gem_init.
---
drivers/gpu/drm/i915/i915_dma.c | 12 +-
drivers/gpu/drm/i915/i915_drv.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 7 +-
drivers/gpu/drm/i915/intel_atomic.c | 7 -
drivers/gpu/drm/i915/intel_display.c | 501 ++++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_drv.h | 3 +-
drivers/gpu/drm/i915/intel_fbdev.c | 12 +-
drivers/gpu/drm/i915/intel_lvds.c | 2 +-
8 files changed, 280 insertions(+), 266 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 88795d2f1819..ede07f6fe7f7 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -393,6 +393,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
static int i915_load_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_atomic_state *state;
int ret;
ret = intel_parse_bios(dev);
@@ -431,13 +432,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
/* Important: The output setup functions called by modeset_init need
* working irqs for e.g. gmbus and dp aux transfers. */
- intel_modeset_init(dev);
+ state = intel_modeset_init(dev);
ret = i915_gem_init(dev);
- if (ret)
+ if (ret) {
+ if (state) {
+ drm_atomic_state_free(state);
+ drm_modeset_unlock_all(dev);
+ }
goto cleanup_irq;
+ }
- intel_modeset_gem_init(dev);
+ intel_modeset_gem_init(dev, state);
/* Always safe in the mode setting case. */
/* FIXME: do pre/post-mode set stuff in core KMS code */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 78ef0bb53c36..a29b24bca25e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -758,7 +758,7 @@ static int i915_drm_resume(struct drm_device *dev)
spin_unlock_irq(&dev_priv->irq_lock);
drm_modeset_lock_all(dev);
- intel_modeset_setup_hw_state(dev, true);
+ intel_display_resume(dev);
drm_modeset_unlock_all(dev);
intel_dp_mst_resume(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d9410bd4ab59..e67d2f1e3ab8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3233,13 +3233,12 @@ static inline void intel_unregister_dsm_handler(void) { return; }
/* modesetting */
extern void intel_modeset_init_hw(struct drm_device *dev);
-extern void intel_modeset_init(struct drm_device *dev);
-extern void intel_modeset_gem_init(struct drm_device *dev);
+extern struct drm_atomic_state *intel_modeset_init(struct drm_device *dev);
+extern void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *);
extern void intel_modeset_cleanup(struct drm_device *dev);
extern void intel_connector_unregister(struct intel_connector *);
extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
-extern void intel_modeset_setup_hw_state(struct drm_device *dev,
- bool force_restore);
+extern void intel_display_resume(struct drm_device *dev);
extern void i915_redisable_vga(struct drm_device *dev);
extern void i915_redisable_vga_power_on(struct drm_device *dev);
extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4d87574a2032..f36fcc7ceecb 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
return -EINVAL;
}
- if (crtc_state &&
- crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
- ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
- if (ret)
- return ret;
- }
-
ret = drm_atomic_helper_check_planes(dev, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41193dab58c5..9b2acc7b4e29 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,9 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static struct drm_atomic_state *
+intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
{
@@ -2567,40 +2570,41 @@ update_state_fb(struct drm_plane *plane)
static void
intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
- struct intel_initial_plane_config *plane_config)
+ struct intel_initial_plane_config *plane_config,
+ struct intel_plane_state *plane_state)
{
+ struct drm_atomic_state *state = plane_state->base.state;
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_crtc *c;
- struct intel_crtc *i;
struct drm_i915_gem_object *obj;
- struct drm_plane *primary = intel_crtc->base.primary;
struct drm_framebuffer *fb;
+ struct drm_plane *plane = plane_state->base.plane, *drm_plane;
+ struct drm_plane_state *drm_plane_state;
+ int i, ret;
if (!plane_config->fb)
return;
if (intel_alloc_initial_plane_obj(intel_crtc, plane_config)) {
- fb = &plane_config->fb->base;
- goto valid_fb;
- }
+ int ret;
- kfree(plane_config->fb);
+ fb = &plane_config->fb->base;
+ ret = intel_pin_and_fence_fb_obj(plane, fb, &plane_state->base, NULL);
+ if (!ret)
+ goto valid_fb;
+ drm_framebuffer_unreference(fb);
+ } else
+ kfree(plane_config->fb);
/*
* Failed to alloc the obj, check to see if we should share
* an fb with another CRTC instead
*/
- for_each_crtc(dev, c) {
- i = to_intel_crtc(c);
-
- if (c == &intel_crtc->base)
+ for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
+ if (drm_plane->type != DRM_PLANE_TYPE_PRIMARY)
continue;
- if (!i->active)
- continue;
-
- fb = c->primary->fb;
+ fb = drm_plane_state->fb;
if (!fb)
continue;
@@ -2611,17 +2615,22 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
}
}
+ intel_pre_disable_primary(&intel_crtc->base);
+ to_intel_plane(plane)->disable_plane(plane, &intel_crtc->base);
+
return;
valid_fb:
+ drm_atomic_set_fb_for_plane(&plane_state->base, fb);
+ ret = drm_atomic_set_crtc_for_plane(&plane_state->base, &intel_crtc->base);
+ WARN_ON(ret);
+ plane->fb = fb;
+
+ plane_state->visible = true;
obj = intel_fb_obj(fb);
if (obj->tiling_mode != I915_TILING_NONE)
dev_priv->preserve_bios_swizzle = true;
- primary->fb = fb;
- primary->crtc = primary->state->crtc = &intel_crtc->base;
- update_state_fb(primary);
- intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
}
@@ -3229,7 +3238,7 @@ void intel_finish_reset(struct drm_device *dev)
dev_priv->display.hpd_irq_setup(dev);
spin_unlock_irq(&dev_priv->irq_lock);
- intel_modeset_setup_hw_state(dev, true);
+ intel_display_resume(dev);
intel_hpd_init(dev_priv);
@@ -6190,6 +6199,11 @@ void intel_display_suspend(struct drm_device *dev)
intel_crtc_disable_noatomic(crtc);
}
+void intel_display_resume(struct drm_device *dev)
+{
+ intel_modeset_setup_hw_state(dev, true);
+}
+
/* Master function to enable/disable CRTC and corresponding power wells */
int intel_crtc_control(struct drm_crtc *crtc, bool enable)
{
@@ -7700,9 +7714,14 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
mode->vsync_end = pipe_config->base.adjusted_mode.crtc_vsync_end;
mode->flags = pipe_config->base.adjusted_mode.flags;
+ mode->type = DRM_MODE_TYPE_DRIVER;
mode->clock = pipe_config->base.adjusted_mode.crtc_clock;
mode->flags |= pipe_config->base.adjusted_mode.flags;
+
+ mode->hsync = drm_mode_hsync(mode);
+ mode->vrefresh = drm_mode_vrefresh(mode);
+ drm_mode_set_name(mode);
}
static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
@@ -11772,34 +11791,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
return true;
}
-static void intel_crtc_check_initial_planes(struct drm_crtc *crtc,
- struct drm_crtc_state *crtc_state)
-{
- struct intel_crtc_state *pipe_config =
- to_intel_crtc_state(crtc_state);
- struct drm_plane *p;
- unsigned visible_mask = 0;
-
- drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) {
- struct drm_plane_state *plane_state =
- drm_atomic_get_existing_plane_state(crtc_state->state, p);
-
- if (WARN_ON(!plane_state))
- continue;
-
- if (!plane_state->fb)
- crtc_state->plane_mask &=
- ~(1 << drm_plane_index(p));
- else if (to_intel_plane_state(plane_state)->visible)
- visible_mask |= 1 << drm_plane_index(p);
- }
-
- if (!visible_mask)
- return;
-
- pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-}
-
static int intel_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state)
{
@@ -11821,10 +11812,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
idx, crtc->state->active, intel_crtc->active);
- /* plane mask is fixed up after all initial planes are calculated */
- if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
- intel_crtc_check_initial_planes(crtc, crtc_state);
-
if (mode_changed)
intel_crtc->atomic.update_wm = !crtc_state->active;
@@ -12374,24 +12361,35 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
static bool
intel_pipe_config_compare(struct drm_device *dev,
struct intel_crtc_state *current_config,
- struct intel_crtc_state *pipe_config)
+ struct intel_crtc_state *pipe_config,
+ bool check_only)
{
+ bool ret = true;
+
+#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
+ do { \
+ if (!check_only) \
+ DRM_ERROR(fmt, ##__VA_ARGS__); \
+ else \
+ DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
+ } while (0)
+
#define PIPE_CONF_CHECK_X(name) \
if (current_config->name != pipe_config->name) { \
- DRM_ERROR("mismatch in " #name " " \
+ INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
"(expected 0x%08x, found 0x%08x)\n", \
current_config->name, \
pipe_config->name); \
- return false; \
+ ret = false; \
}
#define PIPE_CONF_CHECK_I(name) \
if (current_config->name != pipe_config->name) { \
- DRM_ERROR("mismatch in " #name " " \
+ INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
"(expected %i, found %i)\n", \
current_config->name, \
pipe_config->name); \
- return false; \
+ ret = false; \
}
/* This is required for BDW+ where there is only one set of registers for
@@ -12402,30 +12400,30 @@ intel_pipe_config_compare(struct drm_device *dev,
#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
if ((current_config->name != pipe_config->name) && \
(current_config->alt_name != pipe_config->name)) { \
- DRM_ERROR("mismatch in " #name " " \
+ INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
"(expected %i or %i, found %i)\n", \
current_config->name, \
current_config->alt_name, \
pipe_config->name); \
- return false; \
+ ret = false; \
}
#define PIPE_CONF_CHECK_FLAGS(name, mask) \
if ((current_config->name ^ pipe_config->name) & (mask)) { \
- DRM_ERROR("mismatch in " #name "(" #mask ") " \
+ INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") " \
"(expected %i, found %i)\n", \
current_config->name & (mask), \
pipe_config->name & (mask)); \
- return false; \
+ ret = false; \
}
#define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
- DRM_ERROR("mismatch in " #name " " \
+ INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
"(expected %i, found %i)\n", \
current_config->name, \
pipe_config->name); \
- return false; \
+ ret = false; \
}
#define PIPE_CONF_QUIRK(quirk) \
@@ -12559,8 +12557,9 @@ intel_pipe_config_compare(struct drm_device *dev,
#undef PIPE_CONF_CHECK_FLAGS
#undef PIPE_CONF_CHECK_CLOCK_FUZZY
#undef PIPE_CONF_QUIRK
+#undef INTEL_ERR_OR_DBG_KMS
- return true;
+ return ret;
}
static void check_wm_state(struct drm_device *dev)
@@ -12757,7 +12756,7 @@ check_crtc_state(struct drm_device *dev)
"(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
if (active &&
- !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
+ !intel_pipe_config_compare(dev, crtc->config, &pipe_config, false)) {
I915_STATE_WARN(1, "pipe state doesn't match!\n");
intel_dump_pipe_config(crtc, &pipe_config,
"[hw state]");
@@ -13064,20 +13063,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
continue;
}
- if (to_intel_crtc_state(crtc_state)->quirks &
- PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
- ret = drm_atomic_add_affected_planes(state, crtc);
- if (ret)
- return ret;
-
- /*
- * We ought to handle i915.fastboot here.
- * If no modeset is required and the primary plane has
- * a fb, update the members of crtc_state as needed,
- * and run the necessary updates during vblank evasion.
- */
- }
-
if (!needs_modeset(crtc_state)) {
ret = drm_atomic_add_affected_connectors(state, crtc);
if (ret)
@@ -13140,6 +13125,12 @@ static int __intel_set_mode(struct drm_atomic_state *state)
intel_crtc->active = false;
intel_disable_shared_dpll(intel_crtc);
}
+
+ /* FIXME: Move this to i9xx_crtc_disable when it gets a pointer
+ * to the old crtc_state. */
+ if (to_intel_crtc_state(crtc_state)->quirks &
+ PIPE_CONFIG_QUIRK_WRONG_PLANE)
+ intel_crtc->plane = !intel_crtc->plane;
}
/* Only after disabling all output pipelines that will be changed can we
@@ -14975,12 +14966,12 @@ void intel_modeset_init_hw(struct drm_device *dev)
intel_enable_gt_powersave(dev);
}
-void intel_modeset_init(struct drm_device *dev)
+struct drm_atomic_state *intel_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_atomic_state *state;
int sprite, ret;
enum pipe pipe;
- struct intel_crtc *crtc;
drm_mode_config_init(dev);
@@ -14999,7 +14990,7 @@ void intel_modeset_init(struct drm_device *dev)
intel_init_pm(dev);
if (INTEL_INFO(dev)->num_pipes == 0)
- return;
+ return NULL;
intel_init_display(dev);
intel_init_audio(dev);
@@ -15054,30 +15045,11 @@ void intel_modeset_init(struct drm_device *dev)
intel_fbc_disable(dev);
drm_modeset_lock_all(dev);
- intel_modeset_setup_hw_state(dev, false);
- drm_modeset_unlock_all(dev);
-
- for_each_intel_crtc(dev, crtc) {
- if (!crtc->active)
- continue;
+ state = intel_modeset_setup_hw_state(dev, false);
+ if (!state)
+ drm_modeset_unlock_all(dev);
- /*
- * Note that reserving the BIOS fb up front prevents us
- * from stuffing other stolen allocations like the ring
- * on top. This prevents some ugliness at boot time, and
- * can even allow for smooth boot transitions if the BIOS
- * fb is large enough for the active pipe configuration.
- */
- if (dev_priv->display.get_initial_plane_config) {
- dev_priv->display.get_initial_plane_config(crtc,
- &crtc->plane_config);
- /*
- * If the fb is shared between multiple heads, we'll
- * just get the first one.
- */
- intel_find_initial_plane_obj(crtc, &crtc->plane_config);
- }
- }
+ return state;
}
static void intel_enable_pipe_a(struct drm_device *dev)
@@ -15124,13 +15096,17 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
return true;
}
-static void intel_sanitize_crtc(struct intel_crtc *crtc)
+static void intel_sanitize_crtc(struct intel_crtc *crtc,
+ struct intel_crtc_state *pipe_config,
+ bool force_restore)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_encoder *encoder;
+ struct drm_atomic_state *state = pipe_config->base.state;
+ struct intel_crtc_state *hw_state =
+ to_intel_crtc_state(crtc->base.state);
u32 reg;
- bool enable;
/* Clear any frame start delays used for debugging left by the BIOS */
reg = PIPECONF(crtc->config->cpu_transcoder);
@@ -15144,27 +15120,55 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
drm_crtc_vblank_on(&crtc->base);
}
- /* We need to sanitize the plane -> pipe mapping first because this will
- * disable the crtc (and hence change the state) if it is wrong. Note
- * that gen4+ has a fixed plane -> pipe mapping. */
- if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
- bool plane;
+ /* set up current state */
+ if (!force_restore && hw_state->base.active) {
+ bool enable;
- DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
- crtc->base.base.id);
+ memcpy(pipe_config, hw_state, sizeof(*pipe_config));
+ __drm_atomic_helper_crtc_duplicate_state(&crtc->base, &pipe_config->base);
+ pipe_config->base.state = state;
- /* Pipe has the wrong plane attached and the plane is active.
- * Temporarily change the plane mapping and disable everything
- * ... */
- plane = crtc->plane;
- to_intel_plane_state(crtc->base.primary->state)->visible = true;
- crtc->plane = !plane;
- intel_crtc_disable_noatomic(&crtc->base);
- crtc->plane = plane;
+ enable = false;
+ for_each_encoder_on_crtc(dev, &crtc->base, encoder)
+ enable |= encoder->connectors_active;
+
+ pipe_config->base.active = !!enable;
+ }
+
+ if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
+ if (force_restore || i915.fastboot)
+ pipe_config->base.mode_changed = true;
+ else
+ pipe_config->base.active = false;
}
- if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
- crtc->pipe == PIPE_A && !crtc->active) {
+ /* XXX: This is needs to be modified for making fastboot possible
+ * by default without requiring a modeset.
+ */
+ if (hw_state->base.active && pipe_config->base.active) {
+ struct intel_crtc_state sw_state;
+
+ memset(&sw_state, 0, sizeof(sw_state));
+ sw_state.base = pipe_config->base;
+ sw_state.scaler_state = pipe_config->scaler_state;
+ sw_state.shared_dpll = pipe_config->shared_dpll;
+ sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
+ sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
+
+ intel_modeset_pipe_config(&crtc->base, &sw_state);
+
+ /* Check if we need to force a modeset */
+ if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
+ DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
+ pipe_config->base.mode_changed = true;
+ } else {
+ *pipe_config = sw_state;
+ }
+ }
+
+
+ if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
+ crtc->pipe == PIPE_A && !pipe_config->base.active) {
/* BIOS forgot to enable pipe A, this mostly happens after
* resume. Force-enable the pipe to fix this, the update_dpms
* call below we restore the pipe to the right state, but leave
@@ -15172,44 +15176,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
intel_enable_pipe_a(dev);
}
- /* Adjust the state of the output pipe according to whether we
- * have active connectors/encoders. */
- enable = false;
- for_each_encoder_on_crtc(dev, &crtc->base, encoder)
- enable |= encoder->connectors_active;
-
- if (!enable)
- intel_crtc_disable_noatomic(&crtc->base);
-
- if (crtc->active != crtc->base.state->active) {
-
- /* This can happen either due to bugs in the get_hw_state
- * functions or because of calls to intel_crtc_disable_noatomic,
- * or because the pipe is force-enabled due to the
- * pipe A quirk. */
- DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
- crtc->base.base.id,
- crtc->base.state->enable ? "enabled" : "disabled",
- crtc->active ? "enabled" : "disabled");
-
- crtc->base.state->enable = crtc->active;
- crtc->base.state->active = crtc->active;
- crtc->base.enabled = crtc->active;
-
- /* Because we only establish the connector -> encoder ->
- * crtc links if something is active, this means the
- * crtc is now deactivated. Break the links. connector
- * -> encoder links are only establish when things are
- * actually up, hence no need to break them. */
- WARN_ON(crtc->active);
-
- for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
- WARN_ON(encoder->connectors_active);
- encoder->base.crtc = NULL;
- }
- }
-
- if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
+ if (hw_state->base.active || HAS_GMCH_DISPLAY(dev)) {
/*
* We start out with underrun reporting disabled to avoid races.
* For correct bookkeeping mark this on active crtcs.
@@ -15265,6 +15232,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
for_each_intel_connector(dev, connector) {
if (connector->encoder != encoder)
continue;
+
connector->base.dpms = DRM_MODE_DPMS_OFF;
connector->base.encoder = NULL;
}
@@ -15301,74 +15269,54 @@ void i915_redisable_vga(struct drm_device *dev)
i915_redisable_vga_power_on(dev);
}
-static bool primary_get_hw_state(struct intel_crtc *crtc)
-{
- struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-
- return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
-}
-
static int readout_plane_state(struct drm_atomic_state *state,
struct intel_crtc *crtc,
struct intel_crtc_state *crtc_state)
{
- struct intel_plane *p;
+ struct intel_initial_plane_config plane_config = {};
struct drm_plane_state *drm_plane_state;
+ struct intel_plane *p;
bool active = crtc_state->base.active;
- int ret = 0;
-
- if (active) {
- crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-
- /* apply to previous sw state too */
- to_intel_crtc_state(crtc->base.state)->quirks |=
- PIPE_CONFIG_QUIRK_INITIAL_PLANES;
- }
for_each_intel_plane(crtc->base.dev, p) {
- bool visible = active;
+ struct intel_plane_state *plane_state;
if (crtc->pipe != p->pipe)
continue;
drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
- if (IS_ERR(drm_plane_state)) {
- ret = PTR_ERR(drm_plane_state);
- break;
- }
+ if (IS_ERR(drm_plane_state))
+ return PTR_ERR(drm_plane_state);
+ plane_state = to_intel_plane_state(drm_plane_state);
/* Plane scaler state is not touched here. The first atomic
* commit will restore all plane scalers to its old state.
*/
+ plane_state->visible = false;
if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
- visible = primary_get_hw_state(crtc);
- to_intel_plane_state(drm_plane_state)->visible = visible;
- } else {
- /*
- * unknown state, assume it's off to force a transition
- * to on when calculating state changes.
- */
- to_intel_plane_state(drm_plane_state)->visible = false;
- }
+ to_i915(state->dev)->display.get_initial_plane_config(crtc, &plane_config);
+ intel_find_initial_plane_obj(crtc, &plane_config, plane_state);
+ } else if (active)
+ p->disable_plane(&p->base, &crtc->base);
- p->base.crtc = drm_plane_state->crtc;
- if (visible) {
- crtc_state->base.plane_mask |=
- 1 << drm_plane_index(&p->base);
- } else {
- crtc_state->base.plane_mask &=
- ~(1 << drm_plane_index(&p->base));
+ if (!plane_state->visible) {
+ int ret;
+ ret = drm_atomic_set_crtc_for_plane(drm_plane_state, NULL);
+ WARN_ON(ret);
+ drm_atomic_set_fb_for_plane(drm_plane_state, NULL);
}
+ p->base.crtc = drm_plane_state->crtc;
}
- return ret;
+ return 0;
}
static int readout_hw_crtc_state(struct drm_atomic_state *state,
struct intel_crtc *crtc)
{
- struct drm_i915_private *dev_priv = to_i915(state->dev);
+ struct drm_device *dev = state->dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc_state *crtc_state;
int ret;
@@ -15380,6 +15328,7 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
if (ret)
return ret;
+ __drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base);
memset(crtc_state, 0, sizeof(*crtc_state));
crtc_state->base.crtc = &crtc->base;
crtc_state->base.state = state;
@@ -15397,6 +15346,22 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
crtc->base.base.id,
crtc_state->base.active ? "enabled" : "disabled");
+ /* We need to sanitize the plane -> pipe mapping first because this will
+ * disable the crtc (and hence change the state) if it is wrong. Note
+ * that gen4+ has a fixed plane -> pipe mapping. */
+ if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
+ DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
+ crtc->base.base.id);
+
+ /* Pipe has the wrong plane attached and the plane is active.
+ * Temporarily change the plane mapping and disable everything
+ * ... */
+ crtc_state->quirks |=
+ PIPE_CONFIG_QUIRK_WRONG_PLANE;
+
+ crtc->plane = !crtc->plane;
+ }
+
return readout_plane_state(state, crtc, crtc_state);
}
@@ -15512,7 +15477,7 @@ static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
encoder->get_config(encoder, crtc_state);
- if (connector_state)
+ if (!WARN_ON(!connector_state))
connector_state->crtc = &crtc->base;
} else {
encoder->base.crtc = NULL;
@@ -15565,8 +15530,8 @@ err_free:
/* Scan out the current hw modeset state, sanitizes it and maps it into the drm
* and i915 state tracking structures. */
-void intel_modeset_setup_hw_state(struct drm_device *dev,
- bool force_restore)
+static struct drm_atomic_state *
+intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_crtc *crtc;
@@ -15574,27 +15539,42 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
struct intel_encoder *encoder;
struct drm_atomic_state *state;
struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
- int i;
+ int i, ret;
state = intel_modeset_readout_hw_state(dev);
if (IS_ERR(state)) {
DRM_ERROR("Failed to read out hw state\n");
- return;
+ return NULL;
}
drm_atomic_helper_swap_state(dev, state);
- /* swap sw/hw dpll state */
- intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
- intel_shared_dpll_commit(state);
- memcpy(to_intel_atomic_state(state)->shared_dpll,
- shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
+ if (force_restore) {
+ /* swap sw/hw dpll state */
+ intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
+ intel_shared_dpll_commit(state);
+ memcpy(to_intel_atomic_state(state)->shared_dpll,
+ shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
+ } else {
+ intel_shared_dpll_commit(state);
+ to_intel_atomic_state(state)->dpll_set = false;
+ }
/* HW state is read out, now we need to sanitize this mess. */
for_each_intel_encoder(dev, encoder) {
intel_sanitize_encoder(encoder);
}
+ if (!force_restore) {
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+
+ for_each_connector_in_state(state, connector, conn_state, i) {
+ conn_state->best_encoder = connector->state->best_encoder;
+ conn_state->crtc = connector->state->crtc;
+ }
+ }
+
for_each_crtc_in_state(state, crtc, crtc_state, i) {
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -15616,7 +15596,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
&crtc->state->adjusted_mode);
}
- intel_sanitize_crtc(intel_crtc);
+ intel_sanitize_crtc(intel_crtc, to_intel_crtc_state(crtc_state), force_restore);
/*
* sanitize_crtc may have forced an update of crtc->state,
@@ -15646,29 +15626,28 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
else if (HAS_PCH_SPLIT(dev))
ilk_wm_get_hw_state(dev);
- if (force_restore) {
- int ret;
+ if (!force_restore)
+ return state;
- i915_redisable_vga(dev);
+ i915_redisable_vga(dev);
- ret = intel_set_mode(state);
- if (ret) {
- DRM_ERROR("Failed to restore previous mode\n");
- drm_atomic_state_free(state);
- }
- } else {
+ ret = intel_set_mode(state);
+ if (ret) {
+ DRM_ERROR("Failed to restore previous mode\n");
drm_atomic_state_free(state);
}
intel_modeset_check_state(dev);
+ return NULL;
}
-void intel_modeset_gem_init(struct drm_device *dev)
+void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_crtc *c;
struct drm_i915_gem_object *obj;
- int ret;
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *c;
+ int ret, i;
mutex_lock(&dev->struct_mutex);
intel_init_gt_powersave(dev);
@@ -15693,27 +15672,67 @@ void intel_modeset_gem_init(struct drm_device *dev)
* pinned & fenced. When we do the allocation it's too early
* for this.
*/
- for_each_crtc(dev, c) {
- obj = intel_fb_obj(c->primary->fb);
+ for_each_crtc_in_state(state, c, crtc_state, i) {
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ int j;
+ struct drm_plane *primary = c->primary;
+
+ obj = intel_fb_obj(primary->state->fb);
if (obj == NULL)
- continue;
+ goto disable;
mutex_lock(&dev->struct_mutex);
- ret = intel_pin_and_fence_fb_obj(c->primary,
- c->primary->fb,
- c->primary->state,
- NULL);
+ ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
+ primary->state, NULL);
mutex_unlock(&dev->struct_mutex);
- if (ret) {
- DRM_ERROR("failed to pin boot fb on pipe %d\n",
- to_intel_crtc(c)->pipe);
- drm_framebuffer_unreference(c->primary->fb);
- c->primary->fb = NULL;
- c->primary->crtc = c->primary->state->crtc = NULL;
- update_state_fb(c->primary);
- c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
+ if (!ret && !crtc_state->active)
+ goto disable;
+
+ if (!ret) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_existing_plane_state(state, primary);
+
+ ret = drm_atomic_set_crtc_for_plane(plane_state, c);
+ WARN_ON(ret);
+ drm_atomic_set_fb_for_plane(plane_state, primary->state->fb);
+ continue;
+ }
+
+
+ obj->frontbuffer_bits &=
+ ~INTEL_FRONTBUFFER_PRIMARY(to_intel_crtc(c)->pipe);
+
+ DRM_ERROR("failed to pin boot fb on pipe %d: %i\n",
+ to_intel_crtc(c)->pipe, ret);
+
+ drm_framebuffer_unreference(primary->state->fb);
+ drm_framebuffer_unreference(primary->fb);
+ primary->fb = primary->state->fb = NULL;
+ primary->crtc = primary->state->crtc = NULL;
+ /* Do not update crtc_state->plane_mask, this forces
+ * the disabling of the primary plane during modeset.
+ */
+
+disable:
+ crtc_state->active = crtc_state->enable = false;
+
+ ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+ WARN_ON(ret);
+
+ for_each_connector_in_state(state, connector, conn_state, j) {
+ if (conn_state->crtc != c)
+ continue;
+
+ ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+ WARN_ON(ret);
}
+
}
+ ret = intel_set_mode(state);
+ if (ret)
+ DRM_ERROR("Initial modeset failed with %i\n", ret);
+ drm_modeset_unlock_all(dev);
intel_backlight_register(dev);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bc7d0a003c8e..ea380b83eb5d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -330,7 +330,7 @@ struct intel_crtc_state {
*/
#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */
#define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from firmware */
-#define PIPE_CONFIG_QUIRK_INITIAL_PLANES (1<<2) /* planes are in unknown state */
+#define PIPE_CONFIG_QUIRK_WRONG_PLANE (1<<2) /* intel_crtc->plane is wrong */
unsigned long quirks;
/* Pipe source size (ie. panel fitter input size)
@@ -528,7 +528,6 @@ struct intel_crtc {
uint32_t cursor_size;
uint32_t cursor_base;
- struct intel_initial_plane_config plane_config;
struct intel_crtc_state *config;
/* reset counter value when the last flip was submitted */
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6372cfc7d053..6ac4990a0c11 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -582,7 +582,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
struct intel_framebuffer *fb = NULL;
struct drm_crtc *crtc;
struct intel_crtc *intel_crtc;
- struct intel_initial_plane_config *plane_config = NULL;
unsigned int max_size = 0;
if (!i915.fastboot)
@@ -590,20 +589,21 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
/* Find the largest fb */
for_each_crtc(dev, crtc) {
+ struct intel_framebuffer *plane_fb =
+ to_intel_framebuffer(crtc->primary->state->fb);
intel_crtc = to_intel_crtc(crtc);
- if (!intel_crtc->active || !crtc->primary->fb) {
+ if (!intel_crtc->active || !plane_fb) {
DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
pipe_name(intel_crtc->pipe));
continue;
}
- if (intel_crtc->plane_config.size > max_size) {
+ if (plane_fb->obj->base.size > max_size) {
DRM_DEBUG_KMS("found possible fb from plane %c\n",
pipe_name(intel_crtc->pipe));
- plane_config = &intel_crtc->plane_config;
fb = to_intel_framebuffer(crtc->primary->fb);
- max_size = plane_config->size;
+ max_size = fb->obj->base.size;
}
}
@@ -638,7 +638,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
DRM_DEBUG_KMS("fb not wide enough for plane %c (%d vs %d)\n",
pipe_name(intel_crtc->pipe),
cur_size, fb->base.pitches[0]);
- plane_config = NULL;
fb = NULL;
break;
}
@@ -659,7 +658,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
DRM_DEBUG_KMS("fb not big enough for plane %c (%d vs %d)\n",
pipe_name(intel_crtc->pipe),
cur_size, max_size);
- plane_config = NULL;
fb = NULL;
break;
}
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 161ab26f81fb..78d86ae01621 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -452,7 +452,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
*/
if (!HAS_PCH_SPLIT(dev)) {
drm_modeset_lock_all(dev);
- intel_modeset_setup_hw_state(dev, true);
+ intel_display_resume(dev);
drm_modeset_unlock_all(dev);
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
` (2 preceding siblings ...)
2015-06-19 8:02 ` [RFC PATCH 3/7] All changes from try2 Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-22 15:21 ` Daniel Vetter
2015-06-19 8:02 ` [RFC PATCH 5/7] drm/i915: Update power domains only on affected crtc's Maarten Lankhorst
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
Now that we read out the full atomic state we can force fastboot without hacks.
The only thing that we worry about is preventing a modeset. This can be easily
done by calculating if sw state matches hw state, with exception for pfit and
pipe size. Since the original fastboot code only touched pipe size and panel
fitter we keep those, and compare full sw state against hw state.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/i915_params.c | 5 -
drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_fbdev.c | 10 +-
3 files changed, 169 insertions(+), 117 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8ac5a1b29ac0..9b344a956ba6 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -41,7 +41,6 @@ struct i915_params i915 __read_mostly = {
.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
.disable_power_well = 1,
.enable_ips = 1,
- .fastboot = 0,
.prefault_disable = 0,
.load_detect_test = 0,
.reset = true,
@@ -137,10 +136,6 @@ MODULE_PARM_DESC(disable_power_well,
module_param_named(enable_ips, i915.enable_ips, int, 0600);
MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
-module_param_named(fastboot, i915.fastboot, bool, 0600);
-MODULE_PARM_DESC(fastboot,
- "Try to skip unnecessary mode sets at boot time (default: false)");
-
module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
MODULE_PARM_DESC(prefault_disable,
"Disable page prefaulting for pread/pwrite/reloc (default:false). "
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b2acc7b4e29..de975ef09e23 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static void skylake_pfit_enable(struct intel_crtc *crtc);
static struct drm_atomic_state *
intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
@@ -3289,14 +3290,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
return pending;
}
-static void intel_update_pipe_size(struct intel_crtc *crtc)
+static void intel_update_fastboot(struct intel_crtc *crtc,
+ struct intel_crtc_state *old_crtc_state)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- const struct drm_display_mode *adjusted_mode;
+ struct intel_crtc_state *pipe_config =
+ to_intel_crtc_state(crtc->base.state);
- if (!i915.fastboot)
- return;
+ DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
+ old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
+ pipe_config->pipe_src_w, pipe_config->pipe_src_h);
/*
* Update pipe size and adjust fitter if needed: the reason for this is
@@ -3305,27 +3309,27 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
* fastboot case, we'll flip, but if we don't update the pipesrc and
* pfit state, we'll end up with a big fb scanned out into the wrong
* sized surface.
- *
- * To fix this properly, we need to hoist the checks up into
- * compute_mode_changes (or above), check the actual pfit state and
- * whether the platform allows pfit disable with pipe active, and only
- * then update the pipesrc and pfit state, even on the flip path.
*/
- adjusted_mode = &crtc->config->base.adjusted_mode;
-
I915_WRITE(PIPESRC(crtc->pipe),
- ((adjusted_mode->crtc_hdisplay - 1) << 16) |
- (adjusted_mode->crtc_vdisplay - 1));
- if (!crtc->config->pch_pfit.enabled &&
- (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
- intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
+ ((pipe_config->pipe_src_w - 1) << 16) |
+ (pipe_config->pipe_src_h - 1));
+
+ /* on skylake this is done by detaching scalers */
+ if (INTEL_INFO(dev)->gen == 9) {
+ skl_detach_scalers(crtc);
+
+ if (pipe_config->pch_pfit.enabled)
+ skylake_pfit_enable(crtc);
+ }
+ else if (!pipe_config->pch_pfit.enabled &&
+ old_crtc_state->pch_pfit.enabled) {
+ DRM_DEBUG_KMS("Disabling panel fitter\n");
+
I915_WRITE(PF_CTL(crtc->pipe), 0);
I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
}
- crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
- crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
}
static void intel_fdi_normal_train(struct drm_crtc *crtc)
@@ -12244,19 +12248,6 @@ encoder_retry:
DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
- /* Check if we need to force a modeset */
- if (pipe_config->has_audio !=
- to_intel_crtc_state(crtc->state)->has_audio) {
- pipe_config->base.mode_changed = true;
- ret = drm_atomic_add_affected_planes(state, crtc);
- }
-
- /*
- * Note we have an issue here with infoframes: current code
- * only updates them on the full mode set path per hw
- * requirements. So here we should be checking for any
- * required changes and forcing a mode set.
- */
fail:
return ret;
}
@@ -12500,29 +12491,21 @@ intel_pipe_config_compare(struct drm_device *dev,
DRM_MODE_FLAG_NVSYNC);
}
- PIPE_CONF_CHECK_I(pipe_src_w);
- PIPE_CONF_CHECK_I(pipe_src_h);
-
- /*
- * FIXME: BIOS likes to set up a cloned config with lvds+external
- * screen. Since we don't yet re-compute the pipe config when moving
- * just the lvds port away to another pipe the sw tracking won't match.
- *
- * Proper atomic modesets with recomputed global state will fix this.
- * Until then just don't check gmch state for inherited modes.
- */
if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
- PIPE_CONF_CHECK_I(gmch_pfit.control);
- /* pfit ratios are autocomputed by the hw on gen4+ */
- if (INTEL_INFO(dev)->gen < 4)
- PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
- PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
- }
+ PIPE_CONF_CHECK_I(pipe_src_w);
+ PIPE_CONF_CHECK_I(pipe_src_h);
+
+ PIPE_CONF_CHECK_I(pch_pfit.enabled);
+ if (current_config->pch_pfit.enabled) {
+ PIPE_CONF_CHECK_I(pch_pfit.pos);
+ PIPE_CONF_CHECK_I(pch_pfit.size);
- PIPE_CONF_CHECK_I(pch_pfit.enabled);
- if (current_config->pch_pfit.enabled) {
- PIPE_CONF_CHECK_I(pch_pfit.pos);
- PIPE_CONF_CHECK_I(pch_pfit.size);
+ PIPE_CONF_CHECK_I(gmch_pfit.control);
+ /* pfit ratios are autocomputed by the hw on gen4+ */
+ if (INTEL_INFO(dev)->gen < 4)
+ PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
+ PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
+ }
}
PIPE_CONF_CHECK_I(scaler_state.scaler_id);
@@ -13057,26 +13040,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
return ret;
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- if (!crtc_state->enable) {
- if (needs_modeset(crtc_state))
- any_ms = true;
+ if (!needs_modeset(crtc_state))
continue;
- }
- if (!needs_modeset(crtc_state)) {
- ret = drm_atomic_add_affected_connectors(state, crtc);
- if (ret)
- return ret;
- }
+ any_ms = true;
+ if (!crtc_state->enable)
+ continue;
ret = intel_modeset_pipe_config(crtc,
to_intel_crtc_state(crtc_state));
if (ret)
return ret;
- if (needs_modeset(crtc_state))
- any_ms = true;
-
intel_dump_pipe_config(to_intel_crtc(crtc),
to_intel_crtc_state(crtc_state),
"[modeset]");
@@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
if (needs_modeset(crtc->state) && crtc->state->active) {
update_scanline_offset(to_intel_crtc(crtc));
dev_priv->display.crtc_enable(crtc);
- }
+ } else if (to_intel_crtc_state(crtc_state)->quirks &
+ PIPE_CONFIG_QUIRK_INHERITED_MODE)
+ /* force hw readout check */
+ any_ms = true;
drm_atomic_helper_commit_planes_on_crtc(crtc_state);
}
@@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
if (ret)
goto out;
- intel_update_pipe_size(to_intel_crtc(set->crtc));
-
ret = intel_set_mode_checked(state);
if (ret) {
DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
@@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
if (!crtc->state->active)
return;
- if (state->visible)
- /* FIXME: kill this fastboot hack */
- intel_update_pipe_size(intel_crtc);
-
dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
}
@@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc_state *old_intel_state =
+ to_intel_crtc_state(old_crtc_state);
if (!needs_modeset(crtc->state))
intel_pre_plane_update(intel_crtc);
@@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
intel_pipe_update_start(intel_crtc,
&intel_crtc->atomic.start_vbl_count);
- if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
+ if (!needs_modeset(crtc->state) && crtc->state->active &&
+ old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
+ intel_update_fastboot(intel_crtc, old_intel_state);
+ else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
skl_detach_scalers(intel_crtc);
}
@@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
return true;
}
+static bool
+intel_compare_m_n(unsigned int m, unsigned int n,
+ unsigned int m2, unsigned int n2)
+{
+ if (m == m2 && n == n2)
+ return true;
+
+ if (!m || !n || !m2 || !n2)
+ return false;
+
+ if (m > m2) {
+ while (m > m2) {
+ m >>= 1;
+ n >>= 1;
+ }
+ } else if (m < m2) {
+ while (m < m2) {
+ m2 >>= 1;
+ n2 >>= 1;
+ }
+ }
+
+ return m == m2 && n == n2;
+}
+
+static bool
+intel_compare_link_m_n(const struct intel_link_m_n *m_n,
+ const struct intel_link_m_n *m2_n2)
+{
+ if (m_n->tu == m2_n2->tu &&
+ intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
+ m2_n2->gmch_m, m2_n2->gmch_n) &&
+ intel_compare_m_n(m_n->link_m, m_n->link_n,
+ m2_n2->link_m, m2_n2->link_n))
+ return true;
+
+ return false;
+}
+
+static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
+ struct intel_crtc_state *hw_state,
+ struct intel_crtc_state *pipe_config)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct intel_crtc_state sw_state;
+
+ if (needs_modeset(&pipe_config->base)) {
+ DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
+ return;
+ }
+
+ memset(&sw_state, 0, sizeof(sw_state));
+ sw_state.base = pipe_config->base;
+ sw_state.scaler_state = pipe_config->scaler_state;
+ sw_state.shared_dpll = pipe_config->shared_dpll;
+ sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
+ sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
+ intel_modeset_pipe_config(&crtc->base, &sw_state);
+ sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
+
+ if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
+ sw_state.fdi_m_n = hw_state->fdi_m_n;
+
+ if (INTEL_INFO(dev)->gen < 8) {
+ if (intel_compare_link_m_n(&sw_state.dp_m_n,
+ &hw_state->dp_m_n))
+ sw_state.dp_m_n = hw_state->dp_m_n;
+
+ if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
+ &hw_state->dp_m2_n2))
+ sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
+ } else {
+ if (intel_compare_link_m_n(&sw_state.dp_m_n,
+ &hw_state->dp_m_n)) {
+ sw_state.dp_m_n = hw_state->dp_m_n;
+ hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
+ } else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
+ &hw_state->dp_m_n)) {
+ sw_state.dp_m2_n2 = hw_state->dp_m_n;
+
+ hw_state->dp_m_n = sw_state.dp_m_n;
+ hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
+ }
+ }
+
+ /* Check if we need to force a modeset */
+ if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
+ DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
+ crtc->base.base.id);
+ pipe_config->base.mode_changed = true;
+ } else {
+ DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
+ crtc->base.base.id);
+
+ *pipe_config = sw_state;
+ intel_dump_pipe_config(crtc, pipe_config,
+ "[new fastboot state]");
+
+ /* prevent a modeset by patching up mode struct */
+
+ hw_state->base.mode.type = pipe_config->base.mode.type =
+ hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
+
+ /* clock readout can be approximate, just copy it. */
+ hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
+ }
+}
+
static void intel_sanitize_crtc(struct intel_crtc *crtc,
struct intel_crtc_state *pipe_config,
bool force_restore)
@@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
pipe_config->base.active = !!enable;
}
- if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
- if (force_restore || i915.fastboot)
- pipe_config->base.mode_changed = true;
- else
- pipe_config->base.active = false;
- }
-
- /* XXX: This is needs to be modified for making fastboot possible
- * by default without requiring a modeset.
- */
- if (hw_state->base.active && pipe_config->base.active) {
- struct intel_crtc_state sw_state;
-
- memset(&sw_state, 0, sizeof(sw_state));
- sw_state.base = pipe_config->base;
- sw_state.scaler_state = pipe_config->scaler_state;
- sw_state.shared_dpll = pipe_config->shared_dpll;
- sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
- sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
-
- intel_modeset_pipe_config(&crtc->base, &sw_state);
-
- /* Check if we need to force a modeset */
- if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
- DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
- pipe_config->base.mode_changed = true;
- } else {
- *pipe_config = sw_state;
- }
- }
+ if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
+ pipe_config->base.mode_changed = true;
+ else if (hw_state->base.active && pipe_config->base.active)
+ intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
crtc->pipe == PIPE_A && !pipe_config->base.active) {
@@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
crtc_state->planes_changed = false;
if (crtc->state->enable) {
+ memset(&crtc->mode, 0, sizeof(crtc->mode));
intel_mode_from_pipe_config(&crtc->mode,
to_intel_crtc_state(crtc->state));
intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
@@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
int j;
struct drm_plane *primary = c->primary;
+ if (!c->state->active)
+ continue;
+
obj = intel_fb_obj(primary->state->fb);
if (obj == NULL)
- goto disable;
+ continue;
mutex_lock(&dev->struct_mutex);
ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
@@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
*/
disable:
+ DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
+ c->base.id);
crtc_state->active = crtc_state->enable = false;
ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6ac4990a0c11..2c494d78652a 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -486,15 +486,10 @@ retry:
* config, not the input mode, which is what crtc->mode
* usually contains. But since our current fastboot
* code puts a mode derived from the post-pfit timings
- * into crtc->mode this works out correctly. We don't
- * use hwmode anywhere right now, so use it for this
- * since the fb helper layer wants a pointer to
- * something we own.
+ * into crtc->mode this works out correctly.
*/
DRM_DEBUG_KMS("looking for current mode on connector %s\n",
connector->name);
- intel_mode_from_pipe_config(&encoder->crtc->hwmode,
- to_intel_crtc(encoder->crtc)->config);
modes[i] = &encoder->crtc->hwmode;
}
crtcs[i] = new_crtc;
@@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
struct intel_crtc *intel_crtc;
unsigned int max_size = 0;
- if (!i915.fastboot)
- return false;
-
/* Find the largest fb */
for_each_crtc(dev, crtc) {
struct intel_framebuffer *plane_fb =
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
2015-06-19 8:02 ` [RFC PATCH 4/7] drm/i915: enable fastboot for everyone Maarten Lankhorst
@ 2015-06-22 15:21 ` Daniel Vetter
2015-06-23 10:38 ` Maarten Lankhorst
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2015-06-22 15:21 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Jun 19, 2015 at 10:02:27AM +0200, Maarten Lankhorst wrote:
> Now that we read out the full atomic state we can force fastboot without hacks.
> The only thing that we worry about is preventing a modeset. This can be easily
> done by calculating if sw state matches hw state, with exception for pfit and
> pipe size. Since the original fastboot code only touched pipe size and panel
> fitter we keep those, and compare full sw state against hw state.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_params.c | 5 -
> drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_fbdev.c | 10 +-
> 3 files changed, 169 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8ac5a1b29ac0..9b344a956ba6 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -41,7 +41,6 @@ struct i915_params i915 __read_mostly = {
> .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> .disable_power_well = 1,
> .enable_ips = 1,
> - .fastboot = 0,
> .prefault_disable = 0,
> .load_detect_test = 0,
> .reset = true,
> @@ -137,10 +136,6 @@ MODULE_PARM_DESC(disable_power_well,
> module_param_named(enable_ips, i915.enable_ips, int, 0600);
> MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>
> -module_param_named(fastboot, i915.fastboot, bool, 0600);
> -MODULE_PARM_DESC(fastboot,
> - "Try to skip unnecessary mode sets at boot time (default: false)");
> -
> module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
> MODULE_PARM_DESC(prefault_disable,
> "Disable page prefaulting for pread/pwrite/reloc (default:false). "
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9b2acc7b4e29..de975ef09e23 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> int num_connectors);
> static void intel_pre_disable_primary(struct drm_crtc *crtc);
> +static void skylake_pfit_enable(struct intel_crtc *crtc);
> static struct drm_atomic_state *
> intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
>
> @@ -3289,14 +3290,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> return pending;
> }
>
> -static void intel_update_pipe_size(struct intel_crtc *crtc)
> +static void intel_update_fastboot(struct intel_crtc *crtc,
> + struct intel_crtc_state *old_crtc_state)
That's not a useful rename imo - updating pfit/pipe_size clearly tells us
what's going on, fastboot is much more a marketing thing really. And I
expect that eventually we'll have to grow a lot more update code to fix up
fastboot.
> {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - const struct drm_display_mode *adjusted_mode;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc->base.state);
>
> - if (!i915.fastboot)
> - return;
> + DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
> + old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
> + pipe_config->pipe_src_w, pipe_config->pipe_src_h);
>
> /*
> * Update pipe size and adjust fitter if needed: the reason for this is
> @@ -3305,27 +3309,27 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
> * fastboot case, we'll flip, but if we don't update the pipesrc and
> * pfit state, we'll end up with a big fb scanned out into the wrong
> * sized surface.
> - *
> - * To fix this properly, we need to hoist the checks up into
> - * compute_mode_changes (or above), check the actual pfit state and
> - * whether the platform allows pfit disable with pipe active, and only
> - * then update the pipesrc and pfit state, even on the flip path.
> */
>
> - adjusted_mode = &crtc->config->base.adjusted_mode;
> -
> I915_WRITE(PIPESRC(crtc->pipe),
> - ((adjusted_mode->crtc_hdisplay - 1) << 16) |
> - (adjusted_mode->crtc_vdisplay - 1));
> - if (!crtc->config->pch_pfit.enabled &&
> - (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
> - intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> + ((pipe_config->pipe_src_w - 1) << 16) |
> + (pipe_config->pipe_src_h - 1));
> +
> + /* on skylake this is done by detaching scalers */
> + if (INTEL_INFO(dev)->gen == 9) {
> + skl_detach_scalers(crtc);
> +
> + if (pipe_config->pch_pfit.enabled)
> + skylake_pfit_enable(crtc);
> + }
> + else if (!pipe_config->pch_pfit.enabled &&
> + old_crtc_state->pch_pfit.enabled) {
> + DRM_DEBUG_KMS("Disabling panel fitter\n");
> +
> I915_WRITE(PF_CTL(crtc->pipe), 0);
> I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
> I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
> }
> - crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
> - crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
> }
>
> static void intel_fdi_normal_train(struct drm_crtc *crtc)
> @@ -12244,19 +12248,6 @@ encoder_retry:
> DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>
> - /* Check if we need to force a modeset */
> - if (pipe_config->has_audio !=
> - to_intel_crtc_state(crtc->state)->has_audio) {
> - pipe_config->base.mode_changed = true;
> - ret = drm_atomic_add_affected_planes(state, crtc);
> - }
> -
> - /*
> - * Note we have an issue here with infoframes: current code
> - * only updates them on the full mode set path per hw
> - * requirements. So here we should be checking for any
> - * required changes and forcing a mode set.
> - */
> fail:
> return ret;
> }
> @@ -12500,29 +12491,21 @@ intel_pipe_config_compare(struct drm_device *dev,
> DRM_MODE_FLAG_NVSYNC);
> }
>
> - PIPE_CONF_CHECK_I(pipe_src_w);
> - PIPE_CONF_CHECK_I(pipe_src_h);
> -
> - /*
> - * FIXME: BIOS likes to set up a cloned config with lvds+external
> - * screen. Since we don't yet re-compute the pipe config when moving
> - * just the lvds port away to another pipe the sw tracking won't match.
> - *
> - * Proper atomic modesets with recomputed global state will fix this.
> - * Until then just don't check gmch state for inherited modes.
> - */
> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
This seems like a separate change - with recomputing state on all pipes
unconditionally we should be able to drop this?
> - PIPE_CONF_CHECK_I(gmch_pfit.control);
> - /* pfit ratios are autocomputed by the hw on gen4+ */
> - if (INTEL_INFO(dev)->gen < 4)
> - PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
> - PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
> - }
> + PIPE_CONF_CHECK_I(pipe_src_w);
> + PIPE_CONF_CHECK_I(pipe_src_h);
> +
> + PIPE_CONF_CHECK_I(pch_pfit.enabled);
> + if (current_config->pch_pfit.enabled) {
> + PIPE_CONF_CHECK_I(pch_pfit.pos);
> + PIPE_CONF_CHECK_I(pch_pfit.size);
>
> - PIPE_CONF_CHECK_I(pch_pfit.enabled);
> - if (current_config->pch_pfit.enabled) {
> - PIPE_CONF_CHECK_I(pch_pfit.pos);
> - PIPE_CONF_CHECK_I(pch_pfit.size);
> + PIPE_CONF_CHECK_I(gmch_pfit.control);
> + /* pfit ratios are autocomputed by the hw on gen4+ */
> + if (INTEL_INFO(dev)->gen < 4)
> + PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
> + PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
> + }
> }
>
> PIPE_CONF_CHECK_I(scaler_state.scaler_id);
> @@ -13057,26 +13040,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
> return ret;
>
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - if (!crtc_state->enable) {
> - if (needs_modeset(crtc_state))
> - any_ms = true;
> + if (!needs_modeset(crtc_state))
> continue;
> - }
>
> - if (!needs_modeset(crtc_state)) {
> - ret = drm_atomic_add_affected_connectors(state, crtc);
> - if (ret)
> - return ret;
> - }
> + any_ms = true;
> + if (!crtc_state->enable)
> + continue;
>
> ret = intel_modeset_pipe_config(crtc,
> to_intel_crtc_state(crtc_state));
> if (ret)
> return ret;
>
> - if (needs_modeset(crtc_state))
> - any_ms = true;
> -
> intel_dump_pipe_config(to_intel_crtc(crtc),
> to_intel_crtc_state(crtc_state),
> "[modeset]");
> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
> if (needs_modeset(crtc->state) && crtc->state->active) {
> update_scanline_offset(to_intel_crtc(crtc));
> dev_priv->display.crtc_enable(crtc);
> - }
> + } else if (to_intel_crtc_state(crtc_state)->quirks &
> + PIPE_CONFIG_QUIRK_INHERITED_MODE)
> + /* force hw readout check */
> + any_ms = true;
Imo this should be a separate patch since forcing a modeset check after
the initial takeover is a good thing. Also maybe do a
s/any_ms/do_modeset_checks/ for clarity of what this does.
>
> drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> }
> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> if (ret)
> goto out;
>
> - intel_update_pipe_size(to_intel_crtc(set->crtc));
> -
> ret = intel_set_mode_checked(state);
> if (ret) {
> DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> if (!crtc->state->active)
> return;
>
> - if (state->visible)
> - /* FIXME: kill this fastboot hack */
> - intel_update_pipe_size(intel_crtc);
> -
> dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> }
>
> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc_state *old_intel_state =
> + to_intel_crtc_state(old_crtc_state);
>
> if (!needs_modeset(crtc->state))
> intel_pre_plane_update(intel_crtc);
> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> intel_pipe_update_start(intel_crtc,
> &intel_crtc->atomic.start_vbl_count);
>
> - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> + if (!needs_modeset(crtc->state) && crtc->state->active &&
> + old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
> + intel_update_fastboot(intel_crtc, old_intel_state);
Nack, fastboot only for the initial mode is not what I want. We should be
able to speed up _any_ modeset where we just disable the pfit. Allowing
fast pfit removal unconditionally will also allow us to easily test this
part of fastboot with an igt, which I'd also like to see.
> + else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> skl_detach_scalers(intel_crtc);
Also I think it'd be clearer if we have state flags (like for the pageflip
stuff) in crtc_state to enable these fixup functions on an as-needed
basis.
> }
>
> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
> return true;
> }
>
> +static bool
> +intel_compare_m_n(unsigned int m, unsigned int n,
> + unsigned int m2, unsigned int n2)
> +{
> + if (m == m2 && n == n2)
> + return true;
> +
> + if (!m || !n || !m2 || !n2)
> + return false;
> +
> + if (m > m2) {
> + while (m > m2) {
> + m >>= 1;
> + n >>= 1;
> + }
> + } else if (m < m2) {
> + while (m < m2) {
> + m2 >>= 1;
> + n2 >>= 1;
> + }
> + }
> +
> + return m == m2 && n == n2;
> +}
> +
> +static bool
> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> + const struct intel_link_m_n *m2_n2)
> +{
> + if (m_n->tu == m2_n2->tu &&
> + intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> + m2_n2->gmch_m, m2_n2->gmch_n) &&
> + intel_compare_m_n(m_n->link_m, m_n->link_n,
> + m2_n2->link_m, m2_n2->link_n))
> + return true;
> +
> + return false;
> +}
> +
> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
> + struct intel_crtc_state *hw_state,
> + struct intel_crtc_state *pipe_config)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct intel_crtc_state sw_state;
> +
> + if (needs_modeset(&pipe_config->base)) {
> + DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
> + return;
> + }
> +
> + memset(&sw_state, 0, sizeof(sw_state));
> + sw_state.base = pipe_config->base;
> + sw_state.scaler_state = pipe_config->scaler_state;
> + sw_state.shared_dpll = pipe_config->shared_dpll;
> + sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> + sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> + intel_modeset_pipe_config(&crtc->base, &sw_state);
> + sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
> +
> + if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
> + sw_state.fdi_m_n = hw_state->fdi_m_n;
> +
> + if (INTEL_INFO(dev)->gen < 8) {
> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> + &hw_state->dp_m_n))
> + sw_state.dp_m_n = hw_state->dp_m_n;
> +
> + if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> + &hw_state->dp_m2_n2))
> + sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
> + } else {
> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> + &hw_state->dp_m_n)) {
> + sw_state.dp_m_n = hw_state->dp_m_n;
> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> + } else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> + &hw_state->dp_m_n)) {
> + sw_state.dp_m2_n2 = hw_state->dp_m_n;
> +
> + hw_state->dp_m_n = sw_state.dp_m_n;
> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> + }
> + }
Not too happy with splitting up the modeset state compare code like this.
If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
could tie things together well and have all the pipe config code grouped
in one place. But then the _compare() is a bit misleading since it'd also
update the sw_state, so maybe better to call it compare_and_ajust.
The macros would then either copy the hw_state to sw_state (if it's some
intermediate state we don't care about) or compare it (with maybe some
adjustments made). But I'd really like to have all that compare/adjust
logic in one place if possible.
> +
> + /* Check if we need to force a modeset */
> + if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
> + DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
> + crtc->base.base.id);
> + pipe_config->base.mode_changed = true;
> + } else {
> + DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
> + crtc->base.base.id);
> +
> + *pipe_config = sw_state;
> + intel_dump_pipe_config(crtc, pipe_config,
> + "[new fastboot state]");
> +
> + /* prevent a modeset by patching up mode struct */
> +
> + hw_state->base.mode.type = pipe_config->base.mode.type =
> + hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
> +
> + /* clock readout can be approximate, just copy it. */
> + hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
Why do we still need this? If we use intel_pipe_config_compare to compute
mode_changed then we hopefully don't need to hack around things any more.
I guess this is where we need your plan to split out connector_changed
from mode_changed and then replace the mode_changed computed by the
helpers by what intel_pipe_config_compare things is justified.
> + }
> +}
> +
> static void intel_sanitize_crtc(struct intel_crtc *crtc,
> struct intel_crtc_state *pipe_config,
> bool force_restore)
> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> pipe_config->base.active = !!enable;
> }
>
> - if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
> - if (force_restore || i915.fastboot)
> - pipe_config->base.mode_changed = true;
> - else
> - pipe_config->base.active = false;
> - }
> -
> - /* XXX: This is needs to be modified for making fastboot possible
> - * by default without requiring a modeset.
> - */
> - if (hw_state->base.active && pipe_config->base.active) {
> - struct intel_crtc_state sw_state;
> -
> - memset(&sw_state, 0, sizeof(sw_state));
> - sw_state.base = pipe_config->base;
> - sw_state.scaler_state = pipe_config->scaler_state;
> - sw_state.shared_dpll = pipe_config->shared_dpll;
> - sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> - sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> -
> - intel_modeset_pipe_config(&crtc->base, &sw_state);
> -
> - /* Check if we need to force a modeset */
> - if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
> - DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
> - pipe_config->base.mode_changed = true;
> - } else {
> - *pipe_config = sw_state;
> - }
> - }
> + if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
> + pipe_config->base.mode_changed = true;
>
> + else if (hw_state->base.active && pipe_config->base.active)
> + intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
>
> if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> crtc->pipe == PIPE_A && !pipe_config->base.active) {
> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
> crtc_state->planes_changed = false;
>
> if (crtc->state->enable) {
> + memset(&crtc->mode, 0, sizeof(crtc->mode));
> intel_mode_from_pipe_config(&crtc->mode,
> to_intel_crtc_state(crtc->state));
> intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> int j;
> struct drm_plane *primary = c->primary;
>
> + if (!c->state->active)
> + continue;
> +
> obj = intel_fb_obj(primary->state->fb);
> if (obj == NULL)
> - goto disable;
> + continue;
>
> mutex_lock(&dev->struct_mutex);
> ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> */
>
> disable:
> + DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
> + c->base.id);
> crtc_state->active = crtc_state->enable = false;
>
> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6ac4990a0c11..2c494d78652a 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -486,15 +486,10 @@ retry:
> * config, not the input mode, which is what crtc->mode
> * usually contains. But since our current fastboot
> * code puts a mode derived from the post-pfit timings
> - * into crtc->mode this works out correctly. We don't
> - * use hwmode anywhere right now, so use it for this
> - * since the fb helper layer wants a pointer to
> - * something we own.
> + * into crtc->mode this works out correctly.
> */
> DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> connector->name);
> - intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> - to_intel_crtc(encoder->crtc)->config);
> modes[i] = &encoder->crtc->hwmode;
> }
> crtcs[i] = new_crtc;
> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> struct intel_crtc *intel_crtc;
> unsigned int max_size = 0;
>
> - if (!i915.fastboot)
> - return false;
I think it'd be useful to split the fbdev part of the fastboot removal out
into a separate patch. Or do I miss a depency here? Assuming ofc that we
enable the pfit trick for modesets in general.
-Daniel
> -
> /* Find the largest fb */
> for_each_crtc(dev, crtc) {
> struct intel_framebuffer *plane_fb =
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
2015-06-22 15:21 ` Daniel Vetter
@ 2015-06-23 10:38 ` Maarten Lankhorst
2015-06-23 11:50 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-23 10:38 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 22-06-15 om 17:21 schreef Daniel Vetter:
> On Fri, Jun 19, 2015 at 10:02:27AM +0200, Maarten Lankhorst wrote:
>> Now that we read out the full atomic state we can force fastboot without hacks.
>> The only thing that we worry about is preventing a modeset. This can be easily
>> done by calculating if sw state matches hw state, with exception for pfit and
>> pipe size. Since the original fastboot code only touched pipe size and panel
>> fitter we keep those, and compare full sw state against hw state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_params.c | 5 -
>> drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
>> drivers/gpu/drm/i915/intel_fbdev.c | 10 +-
>> 3 files changed, 169 insertions(+), 117 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 8ac5a1b29ac0..9b344a956ba6 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -41,7 +41,6 @@ struct i915_params i915 __read_mostly = {
>> .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>> .disable_power_well = 1,
>> .enable_ips = 1,
>> - .fastboot = 0,
>> .prefault_disable = 0,
>> .load_detect_test = 0,
>> .reset = true,
>> @@ -137,10 +136,6 @@ MODULE_PARM_DESC(disable_power_well,
>> module_param_named(enable_ips, i915.enable_ips, int, 0600);
>> MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>>
>> -module_param_named(fastboot, i915.fastboot, bool, 0600);
>> -MODULE_PARM_DESC(fastboot,
>> - "Try to skip unnecessary mode sets at boot time (default: false)");
>> -
>> module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
>> MODULE_PARM_DESC(prefault_disable,
>> "Disable page prefaulting for pread/pwrite/reloc (default:false). "
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9b2acc7b4e29..de975ef09e23 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>> int num_connectors);
>> static void intel_pre_disable_primary(struct drm_crtc *crtc);
>> +static void skylake_pfit_enable(struct intel_crtc *crtc);
>> static struct drm_atomic_state *
>> intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
>>
>> @@ -3289,14 +3290,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>> return pending;
>> }
>>
>> -static void intel_update_pipe_size(struct intel_crtc *crtc)
>> +static void intel_update_fastboot(struct intel_crtc *crtc,
>> + struct intel_crtc_state *old_crtc_state)
> That's not a useful rename imo - updating pfit/pipe_size clearly tells us
> what's going on, fastboot is much more a marketing thing really. And I
> expect that eventually we'll have to grow a lot more update code to fix up
> fastboot.
>
>> {
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - const struct drm_display_mode *adjusted_mode;
>> + struct intel_crtc_state *pipe_config =
>> + to_intel_crtc_state(crtc->base.state);
>>
>> - if (!i915.fastboot)
>> - return;
>> + DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
>> + old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
>> + pipe_config->pipe_src_w, pipe_config->pipe_src_h);
>>
>> /*
>> * Update pipe size and adjust fitter if needed: the reason for this is
>> @@ -3305,27 +3309,27 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>> * fastboot case, we'll flip, but if we don't update the pipesrc and
>> * pfit state, we'll end up with a big fb scanned out into the wrong
>> * sized surface.
>> - *
>> - * To fix this properly, we need to hoist the checks up into
>> - * compute_mode_changes (or above), check the actual pfit state and
>> - * whether the platform allows pfit disable with pipe active, and only
>> - * then update the pipesrc and pfit state, even on the flip path.
>> */
>>
>> - adjusted_mode = &crtc->config->base.adjusted_mode;
>> -
>> I915_WRITE(PIPESRC(crtc->pipe),
>> - ((adjusted_mode->crtc_hdisplay - 1) << 16) |
>> - (adjusted_mode->crtc_vdisplay - 1));
>> - if (!crtc->config->pch_pfit.enabled &&
>> - (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
>> - intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
>> + ((pipe_config->pipe_src_w - 1) << 16) |
>> + (pipe_config->pipe_src_h - 1));
>> +
>> + /* on skylake this is done by detaching scalers */
>> + if (INTEL_INFO(dev)->gen == 9) {
>> + skl_detach_scalers(crtc);
>> +
>> + if (pipe_config->pch_pfit.enabled)
>> + skylake_pfit_enable(crtc);
>> + }
>> + else if (!pipe_config->pch_pfit.enabled &&
>> + old_crtc_state->pch_pfit.enabled) {
>> + DRM_DEBUG_KMS("Disabling panel fitter\n");
>> +
>> I915_WRITE(PF_CTL(crtc->pipe), 0);
>> I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
>> I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
>> }
>> - crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
>> - crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
>> }
>>
>> static void intel_fdi_normal_train(struct drm_crtc *crtc)
>> @@ -12244,19 +12248,6 @@ encoder_retry:
>> DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>> base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>>
>> - /* Check if we need to force a modeset */
>> - if (pipe_config->has_audio !=
>> - to_intel_crtc_state(crtc->state)->has_audio) {
>> - pipe_config->base.mode_changed = true;
>> - ret = drm_atomic_add_affected_planes(state, crtc);
>> - }
>> -
>> - /*
>> - * Note we have an issue here with infoframes: current code
>> - * only updates them on the full mode set path per hw
>> - * requirements. So here we should be checking for any
>> - * required changes and forcing a mode set.
>> - */
>> fail:
>> return ret;
>> }
>> @@ -12500,29 +12491,21 @@ intel_pipe_config_compare(struct drm_device *dev,
>> DRM_MODE_FLAG_NVSYNC);
>> }
>>
>> - PIPE_CONF_CHECK_I(pipe_src_w);
>> - PIPE_CONF_CHECK_I(pipe_src_h);
>> -
>> - /*
>> - * FIXME: BIOS likes to set up a cloned config with lvds+external
>> - * screen. Since we don't yet re-compute the pipe config when moving
>> - * just the lvds port away to another pipe the sw tracking won't match.
>> - *
>> - * Proper atomic modesets with recomputed global state will fix this.
>> - * Until then just don't check gmch state for inherited modes.
>> - */
>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
> This seems like a separate change - with recomputing state on all pipes
> unconditionally we should be able to drop this?
We repurpose inherited mode here to skip some checks on first boot that will be patched up on first atomic commit.
>> - PIPE_CONF_CHECK_I(gmch_pfit.control);
>> - /* pfit ratios are autocomputed by the hw on gen4+ */
>> - if (INTEL_INFO(dev)->gen < 4)
>> - PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
>> - PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
>> - }
>> + PIPE_CONF_CHECK_I(pipe_src_w);
>> + PIPE_CONF_CHECK_I(pipe_src_h);
>> +
>> + PIPE_CONF_CHECK_I(pch_pfit.enabled);
>> + if (current_config->pch_pfit.enabled) {
>> + PIPE_CONF_CHECK_I(pch_pfit.pos);
>> + PIPE_CONF_CHECK_I(pch_pfit.size);
>>
>> - PIPE_CONF_CHECK_I(pch_pfit.enabled);
>> - if (current_config->pch_pfit.enabled) {
>> - PIPE_CONF_CHECK_I(pch_pfit.pos);
>> - PIPE_CONF_CHECK_I(pch_pfit.size);
>> + PIPE_CONF_CHECK_I(gmch_pfit.control);
>> + /* pfit ratios are autocomputed by the hw on gen4+ */
>> + if (INTEL_INFO(dev)->gen < 4)
>> + PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
>> + PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
>> + }
>> }
>>
>> PIPE_CONF_CHECK_I(scaler_state.scaler_id);
>> @@ -13057,26 +13040,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>> return ret;
>>
>> for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> - if (!crtc_state->enable) {
>> - if (needs_modeset(crtc_state))
>> - any_ms = true;
>> + if (!needs_modeset(crtc_state))
>> continue;
>> - }
>>
>> - if (!needs_modeset(crtc_state)) {
>> - ret = drm_atomic_add_affected_connectors(state, crtc);
>> - if (ret)
>> - return ret;
>> - }
>> + any_ms = true;
>> + if (!crtc_state->enable)
>> + continue;
>>
>> ret = intel_modeset_pipe_config(crtc,
>> to_intel_crtc_state(crtc_state));
>> if (ret)
>> return ret;
>>
>> - if (needs_modeset(crtc_state))
>> - any_ms = true;
>> -
>> intel_dump_pipe_config(to_intel_crtc(crtc),
>> to_intel_crtc_state(crtc_state),
>> "[modeset]");
>> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>> if (needs_modeset(crtc->state) && crtc->state->active) {
>> update_scanline_offset(to_intel_crtc(crtc));
>> dev_priv->display.crtc_enable(crtc);
>> - }
>> + } else if (to_intel_crtc_state(crtc_state)->quirks &
>> + PIPE_CONFIG_QUIRK_INHERITED_MODE)
>> + /* force hw readout check */
>> + any_ms = true;
> Imo this should be a separate patch since forcing a modeset check after
> the initial takeover is a good thing. Also maybe do a
> s/any_ms/do_modeset_checks/ for clarity of what this does.
It's used for other reasons too, like updating power domains when crtc states change. So do_modeset_checks would be misleading.
>>
>> drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>> }
>> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>> if (ret)
>> goto out;
>>
>> - intel_update_pipe_size(to_intel_crtc(set->crtc));
>> -
>> ret = intel_set_mode_checked(state);
>> if (ret) {
>> DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>> if (!crtc->state->active)
>> return;
>>
>> - if (state->visible)
>> - /* FIXME: kill this fastboot hack */
>> - intel_update_pipe_size(intel_crtc);
>> -
>> dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
>> }
>>
>> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>> struct drm_device *dev = crtc->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> + struct intel_crtc_state *old_intel_state =
>> + to_intel_crtc_state(old_crtc_state);
>>
>> if (!needs_modeset(crtc->state))
>> intel_pre_plane_update(intel_crtc);
>> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>> intel_pipe_update_start(intel_crtc,
>> &intel_crtc->atomic.start_vbl_count);
>>
>> - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> + if (!needs_modeset(crtc->state) && crtc->state->active &&
>> + old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
>> + intel_update_fastboot(intel_crtc, old_intel_state);
> Nack, fastboot only for the initial mode is not what I want. We should be
> able to speed up _any_ modeset where we just disable the pfit. Allowing
> fast pfit removal unconditionally will also allow us to easily test this
> part of fastboot with an igt, which I'd also like to see.
You're right, but I think because this needs more infrastructure. I only want to convert fastboot at first.
For MST we might also need a variant which only performs a pipe modeset, rather than a full modeset.
Still there are cases where we will need to force a modeset regardless, for example when device frequency
changes.
In either case this probably requires a lot of code may differ on each platform. :(
>> + else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> skl_detach_scalers(intel_crtc);
> Also I think it'd be clearer if we have state flags (like for the pageflip
> stuff) in crtc_state to enable these fixup functions on an as-needed
> basis.
I think this call should be moved to the plane commit function instead, with a single
detach_scaler to disable the relevant scaler, if the scaler's not used afterwards.
fastboot updates would disable it for the crtc, eliminating the need to call it separately.
>> }
>>
>> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>> return true;
>> }
>>
>> +static bool
>> +intel_compare_m_n(unsigned int m, unsigned int n,
>> + unsigned int m2, unsigned int n2)
>> +{
>> + if (m == m2 && n == n2)
>> + return true;
>> +
>> + if (!m || !n || !m2 || !n2)
>> + return false;
>> +
>> + if (m > m2) {
>> + while (m > m2) {
>> + m >>= 1;
>> + n >>= 1;
>> + }
>> + } else if (m < m2) {
>> + while (m < m2) {
>> + m2 >>= 1;
>> + n2 >>= 1;
>> + }
>> + }
>> +
>> + return m == m2 && n == n2;
>> +}
>> +
>> +static bool
>> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
>> + const struct intel_link_m_n *m2_n2)
>> +{
>> + if (m_n->tu == m2_n2->tu &&
>> + intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
>> + m2_n2->gmch_m, m2_n2->gmch_n) &&
>> + intel_compare_m_n(m_n->link_m, m_n->link_n,
>> + m2_n2->link_m, m2_n2->link_n))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
>> + struct intel_crtc_state *hw_state,
>> + struct intel_crtc_state *pipe_config)
>> +{
>> + struct drm_device *dev = crtc->base.dev;
>> + struct intel_crtc_state sw_state;
>> +
>> + if (needs_modeset(&pipe_config->base)) {
>> + DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
>> + return;
>> + }
>> +
>> + memset(&sw_state, 0, sizeof(sw_state));
>> + sw_state.base = pipe_config->base;
>> + sw_state.scaler_state = pipe_config->scaler_state;
>> + sw_state.shared_dpll = pipe_config->shared_dpll;
>> + sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
>> + sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
>> + intel_modeset_pipe_config(&crtc->base, &sw_state);
>> + sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
>> +
>> + if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
>> + sw_state.fdi_m_n = hw_state->fdi_m_n;
>> +
>> + if (INTEL_INFO(dev)->gen < 8) {
>> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
>> + &hw_state->dp_m_n))
>> + sw_state.dp_m_n = hw_state->dp_m_n;
>> +
>> + if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
>> + &hw_state->dp_m2_n2))
>> + sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
>> + } else {
>> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
>> + &hw_state->dp_m_n)) {
>> + sw_state.dp_m_n = hw_state->dp_m_n;
>> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
>> + } else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
>> + &hw_state->dp_m_n)) {
>> + sw_state.dp_m2_n2 = hw_state->dp_m_n;
>> +
>> + hw_state->dp_m_n = sw_state.dp_m_n;
>> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
>> + }
>> + }
> Not too happy with splitting up the modeset state compare code like this.
> If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
> could tie things together well and have all the pipe config code grouped
> in one place. But then the _compare() is a bit misleading since it'd also
> update the sw_state, so maybe better to call it compare_and_ajust.
Or pass a bool adjust, since checking state after a modeset needs to match exactly,
and no adjustment can be done.
> The macros would then either copy the hw_state to sw_state (if it's some
> intermediate state we don't care about) or compare it (with maybe some
> adjustments made). But I'd really like to have all that compare/adjust
> logic in one place if possible.
>
>> +
>> + /* Check if we need to force a modeset */
>> + if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
>> + DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
>> + crtc->base.base.id);
>> + pipe_config->base.mode_changed = true;
>> + } else {
>> + DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
>> + crtc->base.base.id);
>> +
>> + *pipe_config = sw_state;
>> + intel_dump_pipe_config(crtc, pipe_config,
>> + "[new fastboot state]");
>> +
>> + /* prevent a modeset by patching up mode struct */
>> +
>> + hw_state->base.mode.type = pipe_config->base.mode.type =
>> + hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
>> +
>> + /* clock readout can be approximate, just copy it. */
>> + hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
> Why do we still need this? If we use intel_pipe_config_compare to compute
> mode_changed then we hopefully don't need to hack around things any more.
> I guess this is where we need your plan to split out connector_changed
> from mode_changed and then replace the mode_changed computed by the
> helpers by what intel_pipe_config_compare things is justified.
My display has a slightly different clock and mode->type in edid. Patching this up prevented the modeset.
There can also be other reasons for modeset, like CDCLK changes.
>> + }
>> +}
>> +
>> static void intel_sanitize_crtc(struct intel_crtc *crtc,
>> struct intel_crtc_state *pipe_config,
>> bool force_restore)
>> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>> pipe_config->base.active = !!enable;
>> }
>>
>> - if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
>> - if (force_restore || i915.fastboot)
>> - pipe_config->base.mode_changed = true;
>> - else
>> - pipe_config->base.active = false;
>> - }
>> -
>> - /* XXX: This is needs to be modified for making fastboot possible
>> - * by default without requiring a modeset.
>> - */
>> - if (hw_state->base.active && pipe_config->base.active) {
>> - struct intel_crtc_state sw_state;
>> -
>> - memset(&sw_state, 0, sizeof(sw_state));
>> - sw_state.base = pipe_config->base;
>> - sw_state.scaler_state = pipe_config->scaler_state;
>> - sw_state.shared_dpll = pipe_config->shared_dpll;
>> - sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
>> - sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
>> -
>> - intel_modeset_pipe_config(&crtc->base, &sw_state);
>> -
>> - /* Check if we need to force a modeset */
>> - if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
>> - DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
>> - pipe_config->base.mode_changed = true;
>> - } else {
>> - *pipe_config = sw_state;
>> - }
>> - }
>> + if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
>> + pipe_config->base.mode_changed = true;
>>
>> + else if (hw_state->base.active && pipe_config->base.active)
>> + intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
>>
>> if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
>> crtc->pipe == PIPE_A && !pipe_config->base.active) {
>> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
>> crtc_state->planes_changed = false;
>>
>> if (crtc->state->enable) {
>> + memset(&crtc->mode, 0, sizeof(crtc->mode));
>> intel_mode_from_pipe_config(&crtc->mode,
>> to_intel_crtc_state(crtc->state));
>> intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
>> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>> int j;
>> struct drm_plane *primary = c->primary;
>>
>> + if (!c->state->active)
>> + continue;
>> +
>> obj = intel_fb_obj(primary->state->fb);
>> if (obj == NULL)
>> - goto disable;
>> + continue;
>>
>> mutex_lock(&dev->struct_mutex);
>> ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
>> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>> */
>>
>> disable:
>> + DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
>> + c->base.id);
>> crtc_state->active = crtc_state->enable = false;
>>
>> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 6ac4990a0c11..2c494d78652a 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -486,15 +486,10 @@ retry:
>> * config, not the input mode, which is what crtc->mode
>> * usually contains. But since our current fastboot
>> * code puts a mode derived from the post-pfit timings
>> - * into crtc->mode this works out correctly. We don't
>> - * use hwmode anywhere right now, so use it for this
>> - * since the fb helper layer wants a pointer to
>> - * something we own.
>> + * into crtc->mode this works out correctly.
>> */
>> DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>> connector->name);
>> - intel_mode_from_pipe_config(&encoder->crtc->hwmode,
>> - to_intel_crtc(encoder->crtc)->config);
>> modes[i] = &encoder->crtc->hwmode;
>> }
>> crtcs[i] = new_crtc;
>> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>> struct intel_crtc *intel_crtc;
>> unsigned int max_size = 0;
>>
>> - if (!i915.fastboot)
>> - return false;
> I think it'd be useful to split the fbdev part of the fastboot removal out
> into a separate patch. Or do I miss a depency here? Assuming ofc that we
> enable the pfit trick for modesets in general.
>
I think you did miss a dependency. Not competely sure though..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
2015-06-23 10:38 ` Maarten Lankhorst
@ 2015-06-23 11:50 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2015-06-23 11:50 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Tue, Jun 23, 2015 at 12:38:50PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:21 schreef Daniel Vetter:
> >> - /*
> >> - * FIXME: BIOS likes to set up a cloned config with lvds+external
> >> - * screen. Since we don't yet re-compute the pipe config when moving
> >> - * just the lvds port away to another pipe the sw tracking won't match.
> >> - *
> >> - * Proper atomic modesets with recomputed global state will fix this.
> >> - * Until then just don't check gmch state for inherited modes.
> >> - */
> >> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
> > This seems like a separate change - with recomputing state on all pipes
> > unconditionally we should be able to drop this?
> We repurpose inherited mode here to skip some checks on first boot that
> will be patched up on first atomic commit.
Yeah, but since you delete the FIXME comment (I agree that's adressed)
there shouldn't be any reason any more for this. If there still is then we
need a new comment imo. And we do read out pfit state correctly, so this
shouldn't cause any pipe config mismatches on the initial state.
> >> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
> >> if (needs_modeset(crtc->state) && crtc->state->active) {
> >> update_scanline_offset(to_intel_crtc(crtc));
> >> dev_priv->display.crtc_enable(crtc);
> >> - }
> >> + } else if (to_intel_crtc_state(crtc_state)->quirks &
> >> + PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> + /* force hw readout check */
> >> + any_ms = true;
> > Imo this should be a separate patch since forcing a modeset check after
> > the initial takeover is a good thing. Also maybe do a
> > s/any_ms/do_modeset_checks/ for clarity of what this does.
> It's used for other reasons too, like updating power domains when crtc
> states change. So do_modeset_checks would be misleading.
Yeah I got confused reading code since it looks so different. any_ms
totally does not what I thought it does, and the name is good as-is.
> >>
> >> drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> >> }
> >> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >> if (ret)
> >> goto out;
> >>
> >> - intel_update_pipe_size(to_intel_crtc(set->crtc));
> >> -
> >> ret = intel_set_mode_checked(state);
> >> if (ret) {
> >> DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >> if (!crtc->state->active)
> >> return;
> >>
> >> - if (state->visible)
> >> - /* FIXME: kill this fastboot hack */
> >> - intel_update_pipe_size(intel_crtc);
> >> -
> >> dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> >> }
> >>
> >> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >> struct drm_device *dev = crtc->dev;
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> + struct intel_crtc_state *old_intel_state =
> >> + to_intel_crtc_state(old_crtc_state);
> >>
> >> if (!needs_modeset(crtc->state))
> >> intel_pre_plane_update(intel_crtc);
> >> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >> intel_pipe_update_start(intel_crtc,
> >> &intel_crtc->atomic.start_vbl_count);
> >>
> >> - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >> + if (!needs_modeset(crtc->state) && crtc->state->active &&
> >> + old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> + intel_update_fastboot(intel_crtc, old_intel_state);
> > Nack, fastboot only for the initial mode is not what I want. We should be
> > able to speed up _any_ modeset where we just disable the pfit. Allowing
> > fast pfit removal unconditionally will also allow us to easily test this
> > part of fastboot with an igt, which I'd also like to see.
> You're right, but I think because this needs more infrastructure. I only want to convert fastboot at first.
> For MST we might also need a variant which only performs a pipe modeset, rather than a full modeset.
> Still there are cases where we will need to force a modeset regardless, for example when device frequency
> changes.
>
> In either case this probably requires a lot of code may differ on each platform. :(
This isn't about anything else but fastboot: I just want the pfit-disable
trick to work on _any_ modeset, since if we don't do that then we can't
test it with igt. And that's bad. pfit-disabling really should be
orthogonal to any inherited state cleanup, if it's not the good isn't yet
ready for prime time imo.
> >> + else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >> skl_detach_scalers(intel_crtc);
> > Also I think it'd be clearer if we have state flags (like for the pageflip
> > stuff) in crtc_state to enable these fixup functions on an as-needed
> > basis.
> I think this call should be moved to the plane commit function instead, with a single
> detach_scaler to disable the relevant scaler, if the scaler's not used afterwards.
>
> fastboot updates would disable it for the crtc, eliminating the need to call it separately.
Yeah that makes sense.
>
> >> }
> >>
> >> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
> >> return true;
> >> }
> >>
> >> +static bool
> >> +intel_compare_m_n(unsigned int m, unsigned int n,
> >> + unsigned int m2, unsigned int n2)
> >> +{
> >> + if (m == m2 && n == n2)
> >> + return true;
> >> +
> >> + if (!m || !n || !m2 || !n2)
> >> + return false;
> >> +
> >> + if (m > m2) {
> >> + while (m > m2) {
> >> + m >>= 1;
> >> + n >>= 1;
> >> + }
> >> + } else if (m < m2) {
> >> + while (m < m2) {
> >> + m2 >>= 1;
> >> + n2 >>= 1;
> >> + }
> >> + }
> >> +
> >> + return m == m2 && n == n2;
> >> +}
> >> +
> >> +static bool
> >> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> >> + const struct intel_link_m_n *m2_n2)
> >> +{
> >> + if (m_n->tu == m2_n2->tu &&
> >> + intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> >> + m2_n2->gmch_m, m2_n2->gmch_n) &&
> >> + intel_compare_m_n(m_n->link_m, m_n->link_n,
> >> + m2_n2->link_m, m2_n2->link_n))
> >> + return true;
> >> +
> >> + return false;
> >> +}
> >> +
> >> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
> >> + struct intel_crtc_state *hw_state,
> >> + struct intel_crtc_state *pipe_config)
> >> +{
> >> + struct drm_device *dev = crtc->base.dev;
> >> + struct intel_crtc_state sw_state;
> >> +
> >> + if (needs_modeset(&pipe_config->base)) {
> >> + DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
> >> + return;
> >> + }
> >> +
> >> + memset(&sw_state, 0, sizeof(sw_state));
> >> + sw_state.base = pipe_config->base;
> >> + sw_state.scaler_state = pipe_config->scaler_state;
> >> + sw_state.shared_dpll = pipe_config->shared_dpll;
> >> + sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> >> + sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> >> + intel_modeset_pipe_config(&crtc->base, &sw_state);
> >> + sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
> >> +
> >> + if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
> >> + sw_state.fdi_m_n = hw_state->fdi_m_n;
> >> +
> >> + if (INTEL_INFO(dev)->gen < 8) {
> >> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> + &hw_state->dp_m_n))
> >> + sw_state.dp_m_n = hw_state->dp_m_n;
> >> +
> >> + if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> + &hw_state->dp_m2_n2))
> >> + sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
> >> + } else {
> >> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> + &hw_state->dp_m_n)) {
> >> + sw_state.dp_m_n = hw_state->dp_m_n;
> >> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> + } else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> + &hw_state->dp_m_n)) {
> >> + sw_state.dp_m2_n2 = hw_state->dp_m_n;
> >> +
> >> + hw_state->dp_m_n = sw_state.dp_m_n;
> >> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> + }
> >> + }
> > Not too happy with splitting up the modeset state compare code like this.
> > If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
> > could tie things together well and have all the pipe config code grouped
> > in one place. But then the _compare() is a bit misleading since it'd also
> > update the sw_state, so maybe better to call it compare_and_ajust.
> Or pass a bool adjust, since checking state after a modeset needs to match exactly,
> and no adjustment can be done.
Yeah adjust is a better name. The point behind the enum is that if you
have a pile of bool arguments it's hard to know without looking at the
function what's going on. With enums you can go with self-descriptive
names. We're not doing this yet everywhere, but as a rule I'd like to see
it if there's more than one bool in the arg list.
> > The macros would then either copy the hw_state to sw_state (if it's some
> > intermediate state we don't care about) or compare it (with maybe some
> > adjustments made). But I'd really like to have all that compare/adjust
> > logic in one place if possible.
> >
> >> +
> >> + /* Check if we need to force a modeset */
> >> + if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
> >> + DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
> >> + crtc->base.base.id);
> >> + pipe_config->base.mode_changed = true;
> >> + } else {
> >> + DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
> >> + crtc->base.base.id);
> >> +
> >> + *pipe_config = sw_state;
> >> + intel_dump_pipe_config(crtc, pipe_config,
> >> + "[new fastboot state]");
> >> +
> >> + /* prevent a modeset by patching up mode struct */
> >> +
> >> + hw_state->base.mode.type = pipe_config->base.mode.type =
> >> + hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
> >> +
> >> + /* clock readout can be approximate, just copy it. */
> >> + hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
> > Why do we still need this? If we use intel_pipe_config_compare to compute
> > mode_changed then we hopefully don't need to hack around things any more.
> > I guess this is where we need your plan to split out connector_changed
> > from mode_changed and then replace the mode_changed computed by the
> > helpers by what intel_pipe_config_compare things is justified.
> My display has a slightly different clock and mode->type in edid. Patching this up prevented the modeset.
> There can also be other reasons for modeset, like CDCLK changes.
Yeah I know we need to patch this up, but we should patch up the requested
mode. Instead allowing some slight mismatches in the adjusted mode (and
copying over the dpll state if that's the case) should be all that's
needed.
Maybe helpers should also look at adjusted modes instead of requested
modes? Would be a big change, but I think it makes sense in general -
adjusted_mode is the one the crtc is supposed to output. That would also
help with handling pfit changes correctly in i915.
>
> >> + }
> >> +}
> >> +
> >> static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >> struct intel_crtc_state *pipe_config,
> >> bool force_restore)
> >> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >> pipe_config->base.active = !!enable;
> >> }
> >>
> >> - if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
> >> - if (force_restore || i915.fastboot)
> >> - pipe_config->base.mode_changed = true;
> >> - else
> >> - pipe_config->base.active = false;
> >> - }
> >> -
> >> - /* XXX: This is needs to be modified for making fastboot possible
> >> - * by default without requiring a modeset.
> >> - */
> >> - if (hw_state->base.active && pipe_config->base.active) {
> >> - struct intel_crtc_state sw_state;
> >> -
> >> - memset(&sw_state, 0, sizeof(sw_state));
> >> - sw_state.base = pipe_config->base;
> >> - sw_state.scaler_state = pipe_config->scaler_state;
> >> - sw_state.shared_dpll = pipe_config->shared_dpll;
> >> - sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> >> - sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> >> -
> >> - intel_modeset_pipe_config(&crtc->base, &sw_state);
> >> -
> >> - /* Check if we need to force a modeset */
> >> - if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
> >> - DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
> >> - pipe_config->base.mode_changed = true;
> >> - } else {
> >> - *pipe_config = sw_state;
> >> - }
> >> - }
> >> + if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
> >> + pipe_config->base.mode_changed = true;
> >>
> >> + else if (hw_state->base.active && pipe_config->base.active)
> >> + intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
> >>
> >> if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> >> crtc->pipe == PIPE_A && !pipe_config->base.active) {
> >> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
> >> crtc_state->planes_changed = false;
> >>
> >> if (crtc->state->enable) {
> >> + memset(&crtc->mode, 0, sizeof(crtc->mode));
> >> intel_mode_from_pipe_config(&crtc->mode,
> >> to_intel_crtc_state(crtc->state));
> >> intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> >> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >> int j;
> >> struct drm_plane *primary = c->primary;
> >>
> >> + if (!c->state->active)
> >> + continue;
> >> +
> >> obj = intel_fb_obj(primary->state->fb);
> >> if (obj == NULL)
> >> - goto disable;
> >> + continue;
> >>
> >> mutex_lock(&dev->struct_mutex);
> >> ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
> >> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >> */
> >>
> >> disable:
> >> + DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
> >> + c->base.id);
> >> crtc_state->active = crtc_state->enable = false;
> >>
> >> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >> index 6ac4990a0c11..2c494d78652a 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >> @@ -486,15 +486,10 @@ retry:
> >> * config, not the input mode, which is what crtc->mode
> >> * usually contains. But since our current fastboot
> >> * code puts a mode derived from the post-pfit timings
> >> - * into crtc->mode this works out correctly. We don't
> >> - * use hwmode anywhere right now, so use it for this
> >> - * since the fb helper layer wants a pointer to
> >> - * something we own.
> >> + * into crtc->mode this works out correctly.
> >> */
> >> DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> >> connector->name);
> >> - intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> >> - to_intel_crtc(encoder->crtc)->config);
> >> modes[i] = &encoder->crtc->hwmode;
> >> }
> >> crtcs[i] = new_crtc;
> >> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> >> struct intel_crtc *intel_crtc;
> >> unsigned int max_size = 0;
> >>
> >> - if (!i915.fastboot)
> >> - return false;
> > I think it'd be useful to split the fbdev part of the fastboot removal out
> > into a separate patch. Or do I miss a depency here? Assuming ofc that we
> > enable the pfit trick for modesets in general.
> >
> I think you did miss a dependency. Not competely sure though..
Well yeah that's likely, but since I didn't see it you need to help me out
;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFC PATCH 5/7] drm/i915: Update power domains only on affected crtc's.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
` (3 preceding siblings ...)
2015-06-19 8:02 ` [RFC PATCH 4/7] drm/i915: enable fastboot for everyone Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-19 8:02 ` [RFC PATCH 6/7] drm/i915: Always reset in intel_crtc_restore_mode Maarten Lankhorst
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
Use for_each_crtc_state to only touch affected crtc's.
In order to make sure that the initial power is still set
correctly we make sure modeset_update_crtc_power_domains is called
during the initial modeset.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index de975ef09e23..942d25e7490a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5197,30 +5197,35 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
return mask;
}
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
+static void modeset_update_crtc_power_domains(struct drm_atomic_state *state,
+ bool power_only)
{
struct drm_device *dev = state->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
- struct intel_crtc *crtc;
+ unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }, domains;
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *crtc;
+ int i;
/*
* First get all needed power domains, then put all unneeded, to avoid
* any unnecessary toggling of the power wells.
*/
- for_each_intel_crtc(dev, crtc) {
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
enum intel_display_power_domain domain;
+ enum pipe pipe = to_intel_crtc(crtc)->pipe;
- if (!crtc->base.state->enable)
+ if (!crtc->state->active)
continue;
- pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
+ domains = pipe_domains[pipe] = get_crtc_power_domains(crtc);
+ domains &= ~to_intel_crtc(crtc)->enabled_power_domains;
- for_each_power_domain(domain, pipe_domains[crtc->pipe])
+ for_each_power_domain(domain, domains)
intel_display_power_get(dev_priv, domain);
}
- if (dev_priv->display.modeset_commit_cdclk) {
+ if (!power_only && dev_priv->display.modeset_commit_cdclk) {
unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
if (cdclk != dev_priv->cdclk_freq &&
@@ -5228,16 +5233,19 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
dev_priv->display.modeset_commit_cdclk(state);
}
- for_each_intel_crtc(dev, crtc) {
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum pipe pipe = intel_crtc->pipe;
enum intel_display_power_domain domain;
- for_each_power_domain(domain, crtc->enabled_power_domains)
- intel_display_power_put(dev_priv, domain);
+ domains = intel_crtc->enabled_power_domains;
+ domains &= ~pipe_domains[pipe];
- crtc->enabled_power_domains = pipe_domains[crtc->pipe];
- }
+ intel_crtc->enabled_power_domains = pipe_domains[pipe];
- intel_display_set_init_power(dev_priv, false);
+ for_each_power_domain(domain, domains)
+ intel_display_power_put(dev_priv, domain);
+ }
}
static void intel_update_max_cdclk(struct drm_device *dev)
@@ -13115,7 +13123,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
/* The state has been swaped above, so state actually contains the
* old state now. */
if (any_ms)
- modeset_update_crtc_power_domains(state);
+ modeset_update_crtc_power_domains(state, false);
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -15606,6 +15614,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
return NULL;
}
+ /* swap sw/hw state */
drm_atomic_helper_swap_state(dev, state);
if (force_restore) {
@@ -15619,6 +15628,9 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
to_intel_atomic_state(state)->dpll_set = false;
}
+ /* update power state to match hw state */
+ modeset_update_crtc_power_domains(state, true);
+
/* HW state is read out, now we need to sanitize this mess. */
for_each_intel_encoder(dev, encoder) {
intel_sanitize_encoder(encoder);
@@ -15697,6 +15709,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
drm_atomic_state_free(state);
}
+ intel_display_set_init_power(dev_priv, false);
+
intel_modeset_check_state(dev);
return NULL;
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFC PATCH 6/7] drm/i915: Always reset in intel_crtc_restore_mode
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
` (4 preceding siblings ...)
2015-06-19 8:02 ` [RFC PATCH 5/7] drm/i915: Update power domains only on affected crtc's Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-22 15:25 ` Daniel Vetter
2015-06-19 8:02 ` [RFC PATCH 7/7] drm/i915: Make intel_display_suspend atomic, try 2 Maarten Lankhorst
2015-06-22 14:54 ` [RFC PATCH 0/7] Convert to atomic, part 4 Daniel Vetter
7 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
And get rid of things that are no longer true. This function is only
used for forcing a modeset when encoder properties are changed.
All the existing state is fine in this case, only setting mode_changed
will force a full recalculation here, and take all the state needed.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 62 +++++++++++-------------------------
1 file changed, 18 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 942d25e7490a..796d08c4606e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13174,66 +13174,40 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct drm_atomic_state *state;
- struct intel_crtc *intel_crtc;
- struct intel_encoder *encoder;
- struct intel_connector *connector;
- struct drm_connector_state *connector_state;
- struct intel_crtc_state *crtc_state;
+ struct drm_crtc_state *crtc_state;
int ret;
state = drm_atomic_state_alloc(dev);
if (!state) {
- DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
+ DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
crtc->base.id);
return;
}
- state->acquire_ctx = dev->mode_config.acquire_ctx;
-
- /* The force restore path in the HW readout code relies on the staged
- * config still keeping the user requested config while the actual
- * state has been overwritten by the configuration read from HW. We
- * need to copy the staged config to the atomic state, otherwise the
- * mode set will just reapply the state the HW is already in. */
- for_each_intel_encoder(dev, encoder) {
- if (encoder->base.crtc != crtc)
- continue;
+ state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
- for_each_intel_connector(dev, connector) {
- if (connector->base.state->best_encoder != &encoder->base)
- continue;
-
- connector_state = drm_atomic_get_connector_state(state, &connector->base);
- if (IS_ERR(connector_state)) {
- DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
- connector->base.base.id,
- connector->base.name,
- PTR_ERR(connector_state));
- continue;
- }
+retry:
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ ret = PTR_ERR_OR_ZERO(crtc_state);
+ if (!ret) {
+ if (!crtc_state->active)
+ goto out;
- connector_state->crtc = crtc;
- }
+ crtc_state->mode_changed = true;
+ ret = intel_modeset_compute_config(state);
}
- for_each_intel_crtc(dev, intel_crtc) {
- crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
- if (IS_ERR(crtc_state)) {
- DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
- intel_crtc->base.base.id,
- PTR_ERR(crtc_state));
- continue;
- }
+ if (!ret)
+ ret = intel_set_mode_checked(state);
- if (&intel_crtc->base == crtc)
- drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
+ if (ret == -EDEADLK) {
+ drm_atomic_state_clear(state);
+ drm_modeset_backoff(state->acquire_ctx);
+ goto retry;
}
- intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
- crtc->primary->fb, crtc->x, crtc->y);
-
- ret = intel_set_mode(state);
if (ret)
+out:
drm_atomic_state_free(state);
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFC PATCH 6/7] drm/i915: Always reset in intel_crtc_restore_mode
2015-06-19 8:02 ` [RFC PATCH 6/7] drm/i915: Always reset in intel_crtc_restore_mode Maarten Lankhorst
@ 2015-06-22 15:25 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2015-06-22 15:25 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Jun 19, 2015 at 10:02:29AM +0200, Maarten Lankhorst wrote:
> And get rid of things that are no longer true. This function is only
> used for forcing a modeset when encoder properties are changed.
>
> All the existing state is fine in this case, only setting mode_changed
> will force a full recalculation here, and take all the state needed.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Just to make sure we're aligned here on future work: When we start
converting over connector props we simply need to push the ->mode_changed
(or well perhaps better crtc_state->connector_changed) update if there's
anything to do into the connector's atomic_set_prop callback.
But I guess for a lot of state it's actually better to not do that, and
rely upon intel_pipe_config_compare to figure things out for us. That
means we must run pretty much all the modeset compute_config code
unconditionally, but I think that has a clear advantage of reducing
duplicated state comparisons.
I don't expect we'll ever get rid of this one since of Old Crap
encoders/connectores (dvo, sdvo, tv, ...) we'll likely never convert the
props to atomic properly.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 62 +++++++++++-------------------------
> 1 file changed, 18 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 942d25e7490a..796d08c4606e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13174,66 +13174,40 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_atomic_state *state;
> - struct intel_crtc *intel_crtc;
> - struct intel_encoder *encoder;
> - struct intel_connector *connector;
> - struct drm_connector_state *connector_state;
> - struct intel_crtc_state *crtc_state;
> + struct drm_crtc_state *crtc_state;
> int ret;
>
> state = drm_atomic_state_alloc(dev);
> if (!state) {
> - DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
> + DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
> crtc->base.id);
> return;
> }
>
> - state->acquire_ctx = dev->mode_config.acquire_ctx;
> -
> - /* The force restore path in the HW readout code relies on the staged
> - * config still keeping the user requested config while the actual
> - * state has been overwritten by the configuration read from HW. We
> - * need to copy the staged config to the atomic state, otherwise the
> - * mode set will just reapply the state the HW is already in. */
> - for_each_intel_encoder(dev, encoder) {
> - if (encoder->base.crtc != crtc)
> - continue;
> + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
>
> - for_each_intel_connector(dev, connector) {
> - if (connector->base.state->best_encoder != &encoder->base)
> - continue;
> -
> - connector_state = drm_atomic_get_connector_state(state, &connector->base);
> - if (IS_ERR(connector_state)) {
> - DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
> - connector->base.base.id,
> - connector->base.name,
> - PTR_ERR(connector_state));
> - continue;
> - }
> +retry:
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + ret = PTR_ERR_OR_ZERO(crtc_state);
> + if (!ret) {
> + if (!crtc_state->active)
> + goto out;
>
> - connector_state->crtc = crtc;
> - }
> + crtc_state->mode_changed = true;
> + ret = intel_modeset_compute_config(state);
> }
>
> - for_each_intel_crtc(dev, intel_crtc) {
> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> - if (IS_ERR(crtc_state)) {
> - DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
> - intel_crtc->base.base.id,
> - PTR_ERR(crtc_state));
> - continue;
> - }
> + if (!ret)
> + ret = intel_set_mode_checked(state);
>
> - if (&intel_crtc->base == crtc)
> - drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> + if (ret == -EDEADLK) {
> + drm_atomic_state_clear(state);
> + drm_modeset_backoff(state->acquire_ctx);
> + goto retry;
> }
>
> - intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
> - crtc->primary->fb, crtc->x, crtc->y);
> -
> - ret = intel_set_mode(state);
> if (ret)
> +out:
> drm_atomic_state_free(state);
> }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFC PATCH 7/7] drm/i915: Make intel_display_suspend atomic, try 2.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
` (5 preceding siblings ...)
2015-06-19 8:02 ` [RFC PATCH 6/7] drm/i915: Always reset in intel_crtc_restore_mode Maarten Lankhorst
@ 2015-06-19 8:02 ` Maarten Lankhorst
2015-06-22 14:54 ` [RFC PATCH 0/7] Convert to atomic, part 4 Daniel Vetter
7 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2015-06-19 8:02 UTC (permalink / raw)
To: intel-gfx
Calculate all state using a normal transition, but afterwards fudge
crtc->state->active back to its old value. This should still allow
state restore in setup_hw_state to work properly.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_drv.h | 2 +-
2 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 796d08c4606e..9c25b080260c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6175,40 +6175,62 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
}
-static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
+/*
+ * turn all crtc's off, but do not adjust state
+ * This has to be paired with a call to intel_modeset_setup_hw_state.
+ */
+int intel_display_suspend(struct drm_device *dev)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_i915_private *dev_priv = to_i915(crtc->dev);
- enum intel_display_power_domain domain;
- unsigned long domains;
+ struct drm_mode_config *config = &dev->mode_config;
+ struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+ struct drm_atomic_state *state;
+ struct drm_crtc *crtc;
+ unsigned crtc_mask = 0;
+ int ret = 0;
- if (!intel_crtc->active)
- return;
+ if (WARN_ON(!ctx))
+ return 0;
- if (to_intel_plane_state(crtc->primary->state)->visible) {
- intel_crtc_wait_for_pending_flips(crtc);
- intel_pre_disable_primary(crtc);
+ lockdep_assert_held(&ctx->ww_ctx);
+ state = drm_atomic_state_alloc(dev);
+ if (WARN_ON(!state))
+ return -ENOMEM;
+
+ state->acquire_ctx = ctx;
+ state->allow_modeset = true;
+
+ for_each_crtc(dev, crtc) {
+ struct drm_crtc_state *crtc_state =
+ drm_atomic_get_crtc_state(state, crtc);
+
+ ret = PTR_ERR_OR_ZERO(crtc_state);
+ if (ret)
+ goto free;
+
+ if (!crtc_state->active)
+ continue;
+
+ crtc_state->active = false;
+ crtc_mask |= 1 << drm_crtc_index(crtc);
}
- intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
- dev_priv->display.crtc_disable(crtc);
+ if (crtc_mask) {
+ ret = intel_set_mode(state);
- domains = intel_crtc->enabled_power_domains;
- for_each_power_domain(domain, domains)
- intel_display_power_put(dev_priv, domain);
- intel_crtc->enabled_power_domains = 0;
-}
+ if (!ret) {
+ for_each_crtc(dev, crtc)
+ if (crtc_mask & (1 << drm_crtc_index(crtc)))
+ crtc->state->active = true;
-/*
- * turn all crtc's off, but do not adjust state
- * This has to be paired with a call to intel_modeset_setup_hw_state.
- */
-void intel_display_suspend(struct drm_device *dev)
-{
- struct drm_crtc *crtc;
+ return ret;
+ }
+ }
- for_each_crtc(dev, crtc)
- intel_crtc_disable_noatomic(crtc);
+free:
+ if (ret)
+ DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+ drm_atomic_state_free(state);
+ return ret;
}
void intel_display_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea380b83eb5d..fb6c88ade842 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -992,7 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev);
void intel_mark_busy(struct drm_device *dev);
void intel_mark_idle(struct drm_device *dev);
void intel_crtc_restore_mode(struct drm_crtc *crtc);
-void intel_display_suspend(struct drm_device *dev);
+int intel_display_suspend(struct drm_device *dev);
int intel_crtc_control(struct drm_crtc *crtc, bool enable);
void intel_crtc_update_dpms(struct drm_crtc *crtc);
void intel_encoder_destroy(struct drm_encoder *encoder);
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFC PATCH 0/7] Convert to atomic, part 4.
2015-06-19 8:02 [RFC PATCH 0/7] Convert to atomic, part 4 Maarten Lankhorst
` (6 preceding siblings ...)
2015-06-19 8:02 ` [RFC PATCH 7/7] drm/i915: Make intel_display_suspend atomic, try 2 Maarten Lankhorst
@ 2015-06-22 14:54 ` Daniel Vetter
7 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2015-06-22 14:54 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Jun 19, 2015 at 10:02:23AM +0200, Maarten Lankhorst wrote:
> The only thing missing after part 3 is restoring atomic readout and making
> suspend/restore. With fixes for all identified causes of regressions from
> atomic suspend being done in convert to atomic, part 3 it's time to worry
> about getting atomic suspend/restore working again.
>
> I do this in a few steps, first I re-introduce the old commit for hw state
> readout, with fixes. After that I convert the atomic readout to a full initial
> modeset, and finally I try to re-enable fastboot optimizations again by default,
> in a less hacky way.
>
> This is a RFC because of the following possible issues:
> - mode->clock may not be read out correctly. It can differ slightly from the
> actually set value, which forces a modeset anyway when it could be avoided.
We shouldn't look at mode->clock but instead at the adjusted mode. And I
guess for that one we should reshuffle the code to put the computed clock
with the actual dp hw settings in there. Plus add it to the hw state
readout logic and require an exact match.
Or we need to allow a slight mismatch (really just slight) when deciding
whether to modeset or not.
> - Not tested with DRRS support on DP, might be broken with fastboot?
There's hw state readout and cross-checking for this, you should get
yelled at if this happens. I guess we'll eventually grow a pretty big list
of fixup code that we need to run after the crtc is enabled.
-Daniel
>
> Maarten Lankhorst (7):
> drm/i915: Do not update pfit state when toggling crtc enabled.
> drm/i915: Read hw state into an atomic state struct, try 2.
> All changes from try2.
> drm/i915: enable fastboot for everyone
> drm/i915: Update power domains only on affected crtc's.
> drm/i915: Always reset in intel_crtc_restore_mode
> drm/i915: Make intel_display_suspend atomic, try 2.
>
> drivers/gpu/drm/i915/i915_dma.c | 12 +-
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 7 +-
> drivers/gpu/drm/i915/i915_params.c | 5 -
> drivers/gpu/drm/i915/intel_atomic.c | 23 +-
> drivers/gpu/drm/i915/intel_display.c | 1302 ++++++++++++++++++++--------------
> drivers/gpu/drm/i915/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 21 +-
> drivers/gpu/drm/i915/intel_fbdev.c | 22 +-
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 10 files changed, 786 insertions(+), 612 deletions(-)
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread