On Mon, Apr 13, 2015 at 02:33:50PM +0300, Imre Deak wrote: > > @@ -424,6 +434,25 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask); > > POSTING_READ(HSW_PWR_WELL_DRIVER); > > DRM_DEBUG_KMS("Disabling %s\n", power_well->name); > > + > > + if (GEN9_ENABLE_DC5(dev) && > > + power_well->data == SKL_DISP_PW_2) { > > + if (dev_priv->csr.states <= FW_LOADING) { > > + /* > > + * TODO: wait for a completion event or > > + * similar here instead of busy > > + * waiting using wait_for function. > > + */ > > + if (wait_for( > > + intel_csr_load_status_get( > > + dev_priv), 1000)) > > + DRM_ERROR("Timed out waiting for CSR to be loaded!"); > > This waits until the state is set to FW_LOADING or FW_FAILED, whereas it > should wait until it's FW_LOADED. I think the above block becomes > clearer by returning the state from the helper: > > if (GEN9_ENABLE_DC5(dev) && power_well->data == SKL_DISP_PW_2) { > enum csr_state state; > > wait_for((state = intel_csr_state(dev_priv)) != FW_UNINITIALIZED, 1000); > if (state != FW_LOADED) > DRM_ERROR("CSR firmware not ready (%d)\n", state); > else > gen9_enable_dc5(dev_priv); > } Also, this level of indentation is becoming unmanageable. Maybe we should move this code to skl_power_well_post_enable()? -- Damien _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx