Re: [PATCH v3 3/8] drm/i915/skl: Add DC5 Trigger Sequence

[Damien Lespiau <damien.lespiau@intel.com>]

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx