| Message ID | 1392674540-10915-15-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Feb 2014 00:02:15 +0200 Imre Deak <imre.deak@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@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@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. It's a bit noisy, power wise, but ultimately it might make for clearer code and one less special case.
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@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@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@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. --Imre
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@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@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@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
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);
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@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)