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