[PATCH 04/19] drm/i915: get/put PC8 when we get/put a CRTC

Paulo Zanoni
2013-11-21T15:47:56Z

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Currently, PC8 is enabled at modeset_global_resources, which is called
after intel_modeset_update_state. Due to this, there's a small race
condition on the case where we start enabling PC8, then do a modeset
while PC8 is still being enabled. The racing condition triggers a WARN
because intel_modeset_update_state will mark the CRTC as enabled, then
the thread that's still enabling PC8 might look at the data structure
and think that PC8 is being enabled while a pipe is enabled. Despite
the WARN, this is not really a bug since we'll wait for the
PC8-enabling thread to finish when we call modeset_global_resources.

So this patch makes sure we get/put PC8 before we update
drm_crtc->enabled, because if a get() call triggers a PC8 disable,
we'll call cancel_delayed_work_sync(), which will wait for the thread
that's enabling PC8, then, after this, we'll disable PC8.

The side-effect benefit of this patch is that we have a nice place to
track enabled/disabled CRTCs, so we may want to move some code from
modeset_global_resources to intel_crtc_set_state in the future.

The problem fixed by this patch can be reproduced by the
modeset-lpsp-stress-no-wait subtest from the pc8 test of
intel-gpu-tools.

v2: - No need for pc8.lock since we already have
cancel_delayed_work_sync().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ea32a8..846f2de 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9124,6 +9124,24 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
return false;
}

+/* Sets crtc->base.enabled and gets/puts whatever resources are needed by the
+ * CRTC. */
+static void intel_crtc_set_state(struct intel_crtc *crtc, bool enabled)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (enabled == crtc->base.enabled)
+ return;
+
+ if (enabled)
+ hsw_disable_package_c8(dev_priv);
+ else
+ hsw_enable_package_c8(dev_priv);
+
+ crtc->base.enabled = enabled;
+}
+
static void
intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
{
@@ -9147,7 +9165,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
/* Update computed state. */
list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
base.head) {
- intel_crtc->base.enabled = intel_crtc_in_use(&intel_crtc->base);
+ intel_crtc_set_state(intel_crtc,
+ intel_crtc_in_use(&intel_crtc->base));
}

list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -10956,7 +10975,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
}

WARN_ON(crtc->active);
- crtc->base.enabled = false;
+ intel_crtc_set_state(crtc, false);
}

if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
@@ -10983,7 +11002,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
crtc->base.enabled ? "enabled" : "disabled",
crtc->active ? "enabled" : "disabled");

- crtc->base.enabled = crtc->active;
+ intel_crtc_set_state(crtc, crtc->active);

/* Because we only establish the connector -> encoder ->
* crtc links if something is active, this means the
@@ -11080,7 +11099,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
crtc->active = dev_priv->display.get_pipe_config(crtc,
&crtc->config);

- crtc->base.enabled = crtc->active;
+ intel_crtc_set_state(crtc, crtc->active);
crtc->primary_enabled = crtc->active;

DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
--
1.8.3.1