| Message ID | 1423828140-10653-48-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > Updated ironlake_enable_rc6() to do explicit request creation and submission. If you merged the context here with the common context switching code, we don't even need to touch the ring here. -Chris
On 13/02/2015 12:19, Chris Wilson wrote: > On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> Updated ironlake_enable_rc6() to do explicit request creation and submission. > If you merged the context here with the common context switching code, > we don't even need to touch the ring here. > -Chris > It would certainly be preferable to not have any ring commands written from deep within the power management code. However, I didn't want to change anything I didn't really need to, especially in code that I'm not at all sure about. Plus I don't have an ironlake to test any significant change on.
On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: > On 13/02/2015 12:19, Chris Wilson wrote: > >On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > >>From: John Harrison <John.C.Harrison@Intel.com> > >> > >>Updated ironlake_enable_rc6() to do explicit request creation and submission. > >If you merged the context here with the common context switching code, > >we don't even need to touch the ring here. > >-Chris > > > It would certainly be preferable to not have any ring commands > written from deep within the power management code. However, I > didn't want to change anything I didn't really need to, especially > in code that I'm not at all sure about. Plus I don't have an > ironlake to test any significant change on. I did and tested extensively. -Chris
On 13/02/2015 17:03, Chris Wilson wrote: > On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: >> On 13/02/2015 12:19, Chris Wilson wrote: >>> On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: >>>> From: John Harrison <John.C.Harrison@Intel.com> >>>> >>>> Updated ironlake_enable_rc6() to do explicit request creation and submission. >>> If you merged the context here with the common context switching code, >>> we don't even need to touch the ring here. >>> -Chris >>> >> It would certainly be preferable to not have any ring commands >> written from deep within the power management code. However, I >> didn't want to change anything I didn't really need to, especially >> in code that I'm not at all sure about. Plus I don't have an >> ironlake to test any significant change on. > I did and tested extensively. > -Chris Do you have a patch that just does the move of this from PM code to context switch code? Something that I can drop into this series would be great. If not, exactly where about in the context switch code should it go? Should it be in the start of day initialisation, in the per context intialisation, every context switch, only the first switch after a resume, ...? Tracing back to where/when this code is currently executed seems to be quite complicated. The _enable_rc6() function is called during ring reset but only for Gen6+ because Ironlake is broken according to the comment. It is also called by a system power management callback but it is unclear when that would occur. Finally, it is also called from the display code in intel_modeset_init_hw(). Thanks, John.
On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote: > On 13/02/2015 17:03, Chris Wilson wrote: > >On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: > >>On 13/02/2015 12:19, Chris Wilson wrote: > >>>On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > >>>>From: John Harrison <John.C.Harrison@Intel.com> > >>>> > >>>>Updated ironlake_enable_rc6() to do explicit request creation and submission. > >>>If you merged the context here with the common context switching code, > >>>we don't even need to touch the ring here. > >>>-Chris > >>> > >>It would certainly be preferable to not have any ring commands > >>written from deep within the power management code. However, I > >>didn't want to change anything I didn't really need to, especially > >>in code that I'm not at all sure about. Plus I don't have an > >>ironlake to test any significant change on. > >I did and tested extensively. > >-Chris > > Do you have a patch that just does the move of this from PM code to context > switch code? Something that I can drop into this series would be great. If > not, exactly where about in the context switch code should it go? Should it > be in the start of day initialisation, in the per context intialisation, > every context switch, only the first switch after a resume, ...? > > Tracing back to where/when this code is currently executed seems to be quite > complicated. The _enable_rc6() function is called during ring reset but only > for Gen6+ because Ironlake is broken according to the comment. It is also > called by a system power management callback but it is unclear when that > would occur. Finally, it is also called from the display code in > intel_modeset_init_hw(). ilk rc6 is disabled by default because it crashes machines hard and doesn't seem to be all that useful really. Compared to gen6+ rc6 which has a massive impact on idle power consumption, more with each generation. I've also never heard of anyone managing to make this work reliably. We could also just rip the entire code out I think, at least I wouldn't be surprised if it has bitrot completely by now. -Daniel
On 25/02/2015 21:31, Daniel Vetter wrote: > On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote: >> On 13/02/2015 17:03, Chris Wilson wrote: >>> On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: >>>> On 13/02/2015 12:19, Chris Wilson wrote: >>>>> On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: >>>>>> From: John Harrison <John.C.Harrison@Intel.com> >>>>>> >>>>>> Updated ironlake_enable_rc6() to do explicit request creation and submission. >>>>> If you merged the context here with the common context switching code, >>>>> we don't even need to touch the ring here. >>>>> -Chris >>>>> >>>> It would certainly be preferable to not have any ring commands >>>> written from deep within the power management code. However, I >>>> didn't want to change anything I didn't really need to, especially >>>> in code that I'm not at all sure about. Plus I don't have an >>>> ironlake to test any significant change on. >>> I did and tested extensively. >>> -Chris >> Do you have a patch that just does the move of this from PM code to context >> switch code? Something that I can drop into this series would be great. If >> not, exactly where about in the context switch code should it go? Should it >> be in the start of day initialisation, in the per context intialisation, >> every context switch, only the first switch after a resume, ...? >> >> Tracing back to where/when this code is currently executed seems to be quite >> complicated. The _enable_rc6() function is called during ring reset but only >> for Gen6+ because Ironlake is broken according to the comment. It is also >> called by a system power management callback but it is unclear when that >> would occur. Finally, it is also called from the display code in >> intel_modeset_init_hw(). > ilk rc6 is disabled by default because it crashes machines hard and > doesn't seem to be all that useful really. Compared to gen6+ rc6 which has > a massive impact on idle power consumption, more with each generation. > > I've also never heard of anyone managing to make this work reliably. We > could also just rip the entire code out I think, at least I wouldn't be > surprised if it has bitrot completely by now. > -Daniel So just remove the ironlake_enable_rc6() function completely? Or keep it as a stub that just says '/* this has never worked so has been removed */'.
On Wed, Feb 25, 2015 at 10:31:13PM +0100, Daniel Vetter wrote: > On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote: > > On 13/02/2015 17:03, Chris Wilson wrote: > > >On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: > > >>On 13/02/2015 12:19, Chris Wilson wrote: > > >>>On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > > >>>>From: John Harrison <John.C.Harrison@Intel.com> > > >>>> > > >>>>Updated ironlake_enable_rc6() to do explicit request creation and submission. > > >>>If you merged the context here with the common context switching code, > > >>>we don't even need to touch the ring here. > > >>>-Chris > > >>> > > >>It would certainly be preferable to not have any ring commands > > >>written from deep within the power management code. However, I > > >>didn't want to change anything I didn't really need to, especially > > >>in code that I'm not at all sure about. Plus I don't have an > > >>ironlake to test any significant change on. > > >I did and tested extensively. > > >-Chris > > > > Do you have a patch that just does the move of this from PM code to context > > switch code? Something that I can drop into this series would be great. If > > not, exactly where about in the context switch code should it go? Should it > > be in the start of day initialisation, in the per context intialisation, > > every context switch, only the first switch after a resume, ...? > > > > Tracing back to where/when this code is currently executed seems to be quite > > complicated. The _enable_rc6() function is called during ring reset but only > > for Gen6+ because Ironlake is broken according to the comment. It is also > > called by a system power management callback but it is unclear when that > > would occur. Finally, it is also called from the display code in > > intel_modeset_init_hw(). > > ilk rc6 is disabled by default because it crashes machines hard and > doesn't seem to be all that useful really. Compared to gen6+ rc6 which has > a massive impact on idle power consumption, more with each generation. > > I've also never heard of anyone managing to make this work reliably. We > could also just rip the entire code out I think, at least I wouldn't be > surprised if it has bitrot completely by now. I was running with it when Ben had those ILK context+rc6 patches flying around. But I admit that was some time ago.
On Fri, Feb 27, 2015 at 03:03:22PM +0200, Ville Syrjälä wrote: > On Wed, Feb 25, 2015 at 10:31:13PM +0100, Daniel Vetter wrote: > > On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote: > > > On 13/02/2015 17:03, Chris Wilson wrote: > > > >On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: > > > >>On 13/02/2015 12:19, Chris Wilson wrote: > > > >>>On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > > > >>>>From: John Harrison <John.C.Harrison@Intel.com> > > > >>>> > > > >>>>Updated ironlake_enable_rc6() to do explicit request creation and submission. > > > >>>If you merged the context here with the common context switching code, > > > >>>we don't even need to touch the ring here. > > > >>>-Chris > > > >>> > > > >>It would certainly be preferable to not have any ring commands > > > >>written from deep within the power management code. However, I > > > >>didn't want to change anything I didn't really need to, especially > > > >>in code that I'm not at all sure about. Plus I don't have an > > > >>ironlake to test any significant change on. > > > >I did and tested extensively. > > > >-Chris > > > > > > Do you have a patch that just does the move of this from PM code to context > > > switch code? Something that I can drop into this series would be great. If > > > not, exactly where about in the context switch code should it go? Should it > > > be in the start of day initialisation, in the per context intialisation, > > > every context switch, only the first switch after a resume, ...? > > > > > > Tracing back to where/when this code is currently executed seems to be quite > > > complicated. The _enable_rc6() function is called during ring reset but only > > > for Gen6+ because Ironlake is broken according to the comment. It is also > > > called by a system power management callback but it is unclear when that > > > would occur. Finally, it is also called from the display code in > > > intel_modeset_init_hw(). > > > > ilk rc6 is disabled by default because it crashes machines hard and > > doesn't seem to be all that useful really. Compared to gen6+ rc6 which has > > a massive impact on idle power consumption, more with each generation. > > > > I've also never heard of anyone managing to make this work reliably. We > > could also just rip the entire code out I think, at least I wouldn't be > > surprised if it has bitrot completely by now. > > I was running with it when Ben had those ILK context+rc6 patches flying > around. But I admit that was some time ago. Iirc we've enabled it for a bit with those, it fell to pieces too. And it seems to be rare enough that testing on a developers machine doesn't cut it (tried myself to enable it a few times and had to revert again). -Daniel
On Fri, Feb 27, 2015 at 12:49:11PM +0000, John Harrison wrote: > On 25/02/2015 21:31, Daniel Vetter wrote: > >On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote: > >>On 13/02/2015 17:03, Chris Wilson wrote: > >>>On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote: > >>>>On 13/02/2015 12:19, Chris Wilson wrote: > >>>>>On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote: > >>>>>>From: John Harrison <John.C.Harrison@Intel.com> > >>>>>> > >>>>>>Updated ironlake_enable_rc6() to do explicit request creation and submission. > >>>>>If you merged the context here with the common context switching code, > >>>>>we don't even need to touch the ring here. > >>>>>-Chris > >>>>> > >>>>It would certainly be preferable to not have any ring commands > >>>>written from deep within the power management code. However, I > >>>>didn't want to change anything I didn't really need to, especially > >>>>in code that I'm not at all sure about. Plus I don't have an > >>>>ironlake to test any significant change on. > >>>I did and tested extensively. > >>>-Chris > >>Do you have a patch that just does the move of this from PM code to context > >>switch code? Something that I can drop into this series would be great. If > >>not, exactly where about in the context switch code should it go? Should it > >>be in the start of day initialisation, in the per context intialisation, > >>every context switch, only the first switch after a resume, ...? > >> > >>Tracing back to where/when this code is currently executed seems to be quite > >>complicated. The _enable_rc6() function is called during ring reset but only > >>for Gen6+ because Ironlake is broken according to the comment. It is also > >>called by a system power management callback but it is unclear when that > >>would occur. Finally, it is also called from the display code in > >>intel_modeset_init_hw(). > >ilk rc6 is disabled by default because it crashes machines hard and > >doesn't seem to be all that useful really. Compared to gen6+ rc6 which has > >a massive impact on idle power consumption, more with each generation. > > > >I've also never heard of anyone managing to make this work reliably. We > >could also just rip the entire code out I think, at least I wouldn't be > >surprised if it has bitrot completely by now. > >-Daniel > > So just remove the ironlake_enable_rc6() function completely? Or keep it as > a stub that just says '/* this has never worked so has been removed */'. Just remove it entirely, together with all the other ironlake rc6 stuff plus functions only called by it. All the ironlake_*_rc6 stuff essentially. ironlake_*_drps is the turbo support, that part needs to stay. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 6ece663..0844166 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4959,6 +4959,7 @@ static void ironlake_enable_rc6(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_engine_cs *ring = &dev_priv->ring[RCS]; + struct drm_i915_gem_request *req = NULL; bool was_interruptible; int ret; @@ -4977,16 +4978,17 @@ static void ironlake_enable_rc6(struct drm_device *dev) was_interruptible = dev_priv->mm.interruptible; dev_priv->mm.interruptible = false; + ret = dev_priv->gt.alloc_request(ring, NULL, &req); + if (ret) + goto err; + /* * GPU can automatically power down the render unit if given a page * to save state. */ ret = intel_ring_begin(ring, 6); - if (ret) { - ironlake_teardown_rc6(dev); - dev_priv->mm.interruptible = was_interruptible; - return; - } + if (ret) + goto err; intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN); intel_ring_emit(ring, MI_SET_CONTEXT); @@ -5000,6 +5002,11 @@ static void ironlake_enable_rc6(struct drm_device *dev) intel_ring_emit(ring, MI_FLUSH); intel_ring_advance(ring); + ret = i915_add_request_no_flush(req); + if (ret) + goto err; + req = NULL; + /* * Wait for the command parser to advance past MI_SET_CONTEXT. The HW * does an implicit flush, combined with MI_FLUSH above, it should be @@ -5007,16 +5014,20 @@ static void ironlake_enable_rc6(struct drm_device *dev) */ ret = intel_ring_idle(ring); dev_priv->mm.interruptible = was_interruptible; - if (ret) { - DRM_ERROR("failed to enable ironlake power savings\n"); - ironlake_teardown_rc6(dev); - return; - } + if (ret) + goto err; I915_WRITE(PWRCTXA, i915_gem_obj_ggtt_offset(dev_priv->ips.pwrctx) | PWRCTX_EN); I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT); intel_print_rc6_info(dev, GEN6_RC_CTL_RC6_ENABLE); + +err: + DRM_ERROR("failed to enable ironlake power savings\n"); + ironlake_teardown_rc6(dev); + dev_priv->mm.interruptible = was_interruptible; + if (req) + i915_gem_request_unreference(req); } static unsigned long intel_pxfreq(u32 vidfreq)