On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
---
drivers/gpu/drm/i915/intel_crt.c | 2 -
drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++--------------------
drivers/gpu/drm/i915/intel_drv.h | 1 -
drivers/gpu/drm/i915/intel_dvo.c | 2 -
drivers/gpu/drm/i915/intel_sdvo.c | 2 -
5 files changed, 36 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5d78c1feec81..9eba3dd5b434 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector,
int mode)
intel_crtc_update_dpms(crtc);
}
- intel_modeset_check_state(connector->dev);
-
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 3ab0a8a8e702..ba0b68a4209d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,42 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder
*encoder, int mode)
* internal consistency). */
static void intel_connector_check_state(struct intel_connector *connector)
{
+ struct drm_crtc *crtc = connector->base.state->crtc;
+
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+ connector->base.base.id,
+ connector->base.name);
+
if (connector->get_hw_state(connector)) {
- struct intel_encoder *encoder = connector->encoder;
- struct drm_crtc *crtc;
- bool encoder_enabled;
- enum pipe pipe;
+ struct drm_encoder *encoder = &connector->encoder->base;
+ struct drm_connector_state *conn_state = connector->base.state;
- DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
- connector->base.base.id,
- connector->base.name);
+ I915_STATE_WARN(!crtc,
+ "connector enabled without attached crtc\n");
- /* there is no real hw state for MST connectors */
- if (connector->mst_port)
Should the above deletion be part of the previous patch?
> + if (!crtc)
return;
- I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
- "wrong connector dpms state\n");
- I915_STATE_WARN(connector->base.encoder != &encoder->base,
- "active connector not linked to encoder\n");
+ I915_STATE_WARN(!crtc->state->active,
+ "connector is active, but attached crtc isn't\n");
- if (encoder) {
- I915_STATE_WARN(!encoder->connectors_active,
- "encoder->connectors_active not set\n");
The converted version doesn't check connectors_active. I see it is
later removed in the patch series, so I assume it is left out on
purpose, but I think this is worth mentioning in the commit message.
> -
- encoder_enabled = encoder->get_hw_state(encoder, &pipe);
- I915_STATE_WARN(!encoder_enabled, "encoder not
enabled\n");
- if (I915_STATE_WARN_ON(!encoder->base.crtc))
- return;
These are also left out. It looks like check_encoder_state() performs
the same check, but I think the commit message should mention this if
that's the case.
> + I915_STATE_WARN(conn_state->best_encoder != encoder,
+ "atomic encoder doesn't match attached encoder\n");
- crtc = encoder->base.crtc;
-
- I915_STATE_WARN(!crtc->state->enable,
- "crtc not enabled\n");
- I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not
active\n");
- I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
- "encoder active on the wrong pipe\n");
- }
+ I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+ "attached encoder crtc differs from connector crtc\n");
+ } else {
+ I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+ "best encoder set without crtc!\n");
}
}
@@ -6444,8 +6435,6 @@ int intel_connector_dpms(struct drm_connector *connector,
int mode)
if (connector->encoder)
intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
- intel_modeset_check_state(connector->dev);
-
return 0;
}
@@ -12705,20 +12694,22 @@ static void check_wm_state(struct drm_device *dev)
}
static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev, struct drm_atomic_state *state)
This is called after a state swap, so I think the state variable should
be called old_state for clarity.
> {
- struct intel_connector *connector;
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ int i;
- for_each_intel_connector(dev, connector) {
- struct drm_encoder *encoder = connector->base.encoder;
- struct drm_connector_state *state = connector->base.state;
+ for_each_connector_in_state(state, connector, conn_state, i) {
+ struct drm_encoder *encoder = connector->encoder;
+ struct drm_connector_state *state = connector->state;
/* This also checks the encoder/connector hw state with the
* ->get_hw_state callbacks. */
- intel_connector_check_state(connector);
+ intel_connector_check_state(to_intel_connector(connector));
I915_STATE_WARN(state->best_encoder != encoder,
- "connector's staged encoder doesn't match current
encoder\n");
+ "connector's atomic encoder doesn't match legacy
encoder\n");
}
}
@@ -12904,11 +12895,12 @@ check_shared_dpll_state(struct drm_device *dev)
}
}
-void
-intel_modeset_check_state(struct drm_device *dev)
+static void
+intel_modeset_check_state(struct drm_device *dev,
+ struct drm_atomic_state *state)
old_state here too.
> {
check_wm_state(dev);
- check_connector_state(dev);
+ check_connector_state(dev, state);
check_encoder_state(dev);
check_crtc_state(dev);
check_shared_dpll_state(dev);
@@ -13294,10 +13286,11 @@ static int intel_atomic_commit(struct drm_device *dev,
drm_atomic_helper_wait_for_vblanks(dev, state);
drm_atomic_helper_cleanup_planes(dev, state);
- drm_atomic_state_free(state);
if (any_ms)
- intel_modeset_check_state(dev);
+ intel_modeset_check_state(dev, state);
+
+ drm_atomic_state_free(state);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6bd848..0da4236dc85a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
struct intel_connector *intel_connector_alloc(void);
int intel_connector_dpms(struct drm_connector *, int mode);
bool intel_connector_get_hw_state(struct intel_connector *connector);
-void intel_modeset_check_state(struct drm_device *dev);
bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
struct intel_digital_port *port);
void intel_connector_attach_encoder(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fd5e522abebb..600f7fb855d8 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector,
int mode)
intel_crtc_update_dpms(crtc);
}
- intel_modeset_check_state(connector->dev);
-
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
b/drivers/gpu/drm/i915/intel_sdvo.c
index 2c435a79d4da..8911e0e417ee 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector
*connector, int mode)
intel_sdvo_set_active_outputs(intel_sdvo,
intel_sdvo->attached_output);
}
- intel_modeset_check_state(connector->dev);
-
How does removing the checks from the _dpms() functions relate to
converting the connector checking to atomic? If I understood correctly,
the checks aren't necessary anymore because intel_crtc_control() is
atomic, and that already performs a check. But if that's right this
should be a separate patch. Or did I miss something?
Ander
> return 0;
}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
