Re: [RFC PATCH] drm/i915/skl: Add DC6 disabling as a power well

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 17, 2015 at 02:45:34PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > and here we need 
> > > disable pw2 -> enable dc6
> > > 
> > > That's symmetric which is great for using power wells here since we walk
> > > the power wells array forward when enabling, and backwards when
> > > disabling.
> > > 
> > > I'm thinking that we could also move the dc5 stuff into a power well.
> > > Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> > > what the requirements wrt. dc5 are. If they are the same as for dc6,
> > > then a single dc power well would do, otherwise we would need two, each
> > > with its own domains.
> > 
> > BTW after a bit more look, I think we could probably simplify things
> > quite a bit with this move. We could perhaps then set power_well->data
> > to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
> > convert the enable/disable dc5/6 into somehting like:
> > 
> > gen9_enable_dc_state(dev_priv, uint32_t state)
> > {
> > 	// csr wait and the debugmask thing could go here
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > 	val |= state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > gen9_disable_dc_state(dev_priv, uint32_t val)
> > {
> > 	uint32_t val;
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > We could even put these directly in the power well hooks, and share
> > those for DC5 and DC6. But that would perhaps mean losing the
> > can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
> > But perhaps it would be cleaners to have separate power well ops for dc5
> > and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
> > functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
> > macros which I supposed we'd need to check in the power well hooks.
> > 
> > But this unification could be a separate patch. First we can just
> > introduce the new power wells using the existing dc5/dc6 enable/disable
> > functions we have.
> > 
> > I didn't look at the dc9 stuff yet, so not sure if that could be handled
> > in a similar fashion.
> > 
> > Also I think currently we just disable runtime PM when the firmware
> > hasn't yet been loaded. But I think we would need to change that to hold
> > a DC5/DC5 references. I guess to do this properly we should a new power
> > domain for this purpose, but I'm not sure that's really worth it for a
> > single user use case.
> 
> I just had the idea that POWER_DOMAIN_INIT might be a nice fit for this,
> but that would also keep the DDI power wells on even though we don't
> need the firmware for those.

POWER_DOMAIN_INIT is what I'm using in my csr cleanup patches to fix up
the current mess.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx