[Intel-gfx] [PATCH 14/19] drm/i915: switch order of power domain init wrt. irq install

[Daniel Vetter]

On Mon, Feb 24, 2014 at 03:23:23PM +0200, Imre Deak wrote:
> On Thu, 2014-02-20 at 11:48 -0800, Jesse Barnes wrote:
> > On Tue, 18 Feb 2014 00:02:15 +0200
> > Imre Deak <imre.deak at intel.com> wrote:
> > 
> > > On VLV at least the display IRQ register access and functionality
> > > depends on its power well to be on, so move the power domain HW init
> > > before we install the IRQs.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 8177c17..f8f7a59 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1321,12 +1321,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > >  	if (ret)
> > >  		goto cleanup_vga_switcheroo;
> > >  
> > > +	intel_power_domains_init_hw(dev_priv);
> > > +
> > >  	ret = drm_irq_install(dev);
> > >  	if (ret)
> > >  		goto cleanup_gem_stolen;
> > >  
> > > -	intel_power_domains_init_hw(dev_priv);
> > > -
> > >  	/* Important: The output setup functions called by modeset_init need
> > >  	 * working irqs for e.g. gmbus and dp aux transfers. */
> > >  	intel_modeset_init(dev);
> > 
> > Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > 
> > That said, this was always one part of the PM code that confused me and
> > caused some refcounts to get messed up last time I worked on it.
> >
> > I think it would be better to not treat init specially, and let the
> > power wells get turned on and off through normal power well get/put
> > calls during init and resume.
> 
> I agree this is the ideal way and we should move towards that. Atm, we
> have intel_display_set_init_power() for init and resume which is not so
> nice, but it should do the right thing.
> 
> > It's a bit noisy, power wise, but ultimately it might make for clearer
> > code and one less special case.
> 
> Yep and also give some power saving.

I think we should have some more robust debug infrastructure before we
endeavour on this journey. E.g. some basic checks to compare register
ranges with power wells and their state like Jesse suggested earlier. Init
code is hilarious complicated and I'm pretty sure we'll miss something in
the transition to more explicitly managed power wells for init and
suspend/resume and all that code otherwise.

Merged this patch here to give the init reordering a bit of time to
settle. This stuff just blows up too often ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch