2013/10/30 Paulo Zanoni <przanoni@xxxxxxxxx>:
> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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 this will grab the PC8 lock, which will
> wait for the PC8-enable task to finish.
>
> 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.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
This specific patch introduces a locking problem. Please ignore it for
now. The previous 3 patches are fine.
> ---
> drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c226f4d..e841cd7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6409,6 +6409,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>
> DRM_DEBUG_KMS("Enabling package C8+\n");
>
> + mutex_lock(&dev_priv->pc8.lock);
> dev_priv->pc8.enabled = true;
>
> if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> @@ -6420,6 +6421,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
> lpt_disable_clkout_dp(dev);
> hsw_pc8_disable_interrupts(dev);
> hsw_disable_lcpll(dev_priv, true, true);
> + mutex_unlock(&dev_priv->pc8.lock);
> }
>
> static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> @@ -8880,6 +8882,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)
> {
> @@ -8903,7 +8923,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) {
> @@ -10695,7 +10716,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 &&
> @@ -10722,7 +10743,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
> @@ -10819,7 +10840,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
>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
