Reviewed-by: Rodrigo Vivi <[email protected]>
On Wed, Nov 27, 2013 at 06:01:19PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <[email protected]> > > 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_enabled 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(). > v3: - Rename to intel_crtc_set_enabled > > Signed-off-by: Paulo Zanoni <[email protected]> > --- > 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 0eb7053..5adf540 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9129,6 +9129,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_enabled(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) > { > @@ -9152,7 +9170,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_enabled(intel_crtc, > + intel_crtc_in_use(&intel_crtc->base)); > } > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > @@ -10961,7 +10980,7 @@ static void intel_sanitize_crtc(struct intel_crtc > *crtc) > } > > WARN_ON(crtc->active); > - crtc->base.enabled = false; > + intel_crtc_set_enabled(crtc, false); > } > > if (dev_priv->quirks & QUIRK_PIPEA_FORCE && > @@ -10988,7 +11007,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_enabled(crtc, crtc->active); > > /* Because we only establish the connector -> encoder -> > * crtc links if something is active, this means the > @@ -11085,7 +11104,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_enabled(crtc, crtc->active); > crtc->primary_enabled = crtc->active; > > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
