* [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy @ 2014-02-18 18:46 Chris Wilson 2014-02-18 19:25 ` Mika Kuoppala 2014-02-21 17:38 ` Chris Wilson 0 siblings, 2 replies; 9+ messages in thread From: Chris Wilson @ 2014-02-18 18:46 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni We currently call intel_mark_idle() too often, as we do so as a side-effect of processing the request queue. However, we the calls to intel_mark_idle() are expected to be paired with a call to intel_mark_busy() (or else we try to idle the hardware by accessing registers that are already disabled). Make the idle/busy tracking explicit to prevent the multiple calls. Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ drivers/gpu/drm/i915/i915_gem.c | 4 +--- drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 +- 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 00222cc..8441c8a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1134,6 +1134,14 @@ struct i915_gem_mm { */ bool interruptible; + /** + * Is the GPU currently considered idle, or busy executing userspace + * requests? Whilst idle, we attempt to power down the hardware and + * display clocks. In order to reduce the effect on performance, there + * is a slight delay before we do so. + */ + bool busy; + /** Bit 6 swizzling required for X tiling */ uint32_t bit_6_swizzle_x; /** Bit 6 swizzling required for Y tiling */ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9a40ef5..4525dd7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2315,7 +2315,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, i915_gem_context_reference(request->ctx); request->emitted_jiffies = jiffies; - was_empty = list_empty(&ring->request_list); list_add_tail(&request->list, &ring->request_list); request->file_priv = NULL; @@ -2336,12 +2335,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, if (!dev_priv->ums.mm_suspended) { i915_queue_hangcheck(ring->dev); - if (was_empty) { + if (intel_mark_busy(dev_priv->dev)) { cancel_delayed_work_sync(&dev_priv->mm.idle_work); queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, round_jiffies_up_relative(HZ)); - intel_mark_busy(dev_priv->dev); } } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e127b23..bfd6396 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8220,8 +8220,14 @@ void intel_mark_busy(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (dev_priv->mm.busy) + return false; + hsw_package_c8_gpu_busy(dev_priv); i915_update_gfx_val(dev_priv); + dev_priv->mm.busy = true; + + return true; } void intel_mark_idle(struct drm_device *dev) @@ -8229,6 +8235,11 @@ void intel_mark_idle(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; + if (!dev_priv->mm.busy) + return; + + dev_priv->mm.busy = false; + hsw_package_c8_gpu_idle(dev_priv); if (!i915.powersave) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e5e1a5c..4c329e0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -656,7 +656,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, const char *intel_output_name(int output); bool intel_has_pending_fb_unpin(struct drm_device *dev); int intel_pch_rawclk(struct drm_device *dev); -void intel_mark_busy(struct drm_device *dev); +bool intel_mark_busy(struct drm_device *dev); void intel_mark_fb_busy(struct drm_i915_gem_object *obj, struct intel_ring_buffer *ring); void intel_mark_idle(struct drm_device *dev); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-18 18:46 [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy Chris Wilson @ 2014-02-18 19:25 ` Mika Kuoppala 2014-02-18 21:34 ` Paulo Zanoni 2014-02-21 17:38 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Mika Kuoppala @ 2014-02-18 19:25 UTC (permalink / raw) To: Chris Wilson, intel-gfx; +Cc: Paulo Zanoni Chris Wilson <chris@chris-wilson.co.uk> writes: > We currently call intel_mark_idle() too often, as we do so as a > side-effect of processing the request queue. However, we the calls to > intel_mark_idle() are expected to be paired with a call to > intel_mark_busy() (or else we try to idle the hardware by accessing > registers that are already disabled). Make the idle/busy tracking > explicit to prevent the multiple calls. > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > drivers/gpu/drm/i915/i915_gem.c | 4 +--- > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 4 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 00222cc..8441c8a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1134,6 +1134,14 @@ struct i915_gem_mm { > */ > bool interruptible; > > + /** > + * Is the GPU currently considered idle, or busy executing userspace > + * requests? Whilst idle, we attempt to power down the hardware and > + * display clocks. In order to reduce the effect on performance, there > + * is a slight delay before we do so. > + */ > + bool busy; > + > /** Bit 6 swizzling required for X tiling */ > uint32_t bit_6_swizzle_x; > /** Bit 6 swizzling required for Y tiling */ > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 9a40ef5..4525dd7 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2315,7 +2315,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, > i915_gem_context_reference(request->ctx); > > request->emitted_jiffies = jiffies; > - was_empty = list_empty(&ring->request_list); > list_add_tail(&request->list, &ring->request_list); > request->file_priv = NULL; > > @@ -2336,12 +2335,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, > if (!dev_priv->ums.mm_suspended) { > i915_queue_hangcheck(ring->dev); > > - if (was_empty) { > + if (intel_mark_busy(dev_priv->dev)) { > cancel_delayed_work_sync(&dev_priv->mm.idle_work); > queue_delayed_work(dev_priv->wq, > &dev_priv->mm.retire_work, > round_jiffies_up_relative(HZ)); > - intel_mark_busy(dev_priv->dev); > } > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e127b23..bfd6396 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8220,8 +8220,14 @@ void intel_mark_busy( bool intel_mark_busy(struct drm_device *dev) -Mika > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (dev_priv->mm.busy) > + return false; > + > hsw_package_c8_gpu_busy(dev_priv); > i915_update_gfx_val(dev_priv); > + dev_priv->mm.busy = true; > + > + return true; > } > > void intel_mark_idle(struct drm_device *dev) > @@ -8229,6 +8235,11 @@ void intel_mark_idle(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > + if (!dev_priv->mm.busy) > + return; > + > + dev_priv->mm.busy = false; > + > hsw_package_c8_gpu_idle(dev_priv); > > if (!i915.powersave) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e5e1a5c..4c329e0 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -656,7 +656,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > const char *intel_output_name(int output); > bool intel_has_pending_fb_unpin(struct drm_device *dev); > int intel_pch_rawclk(struct drm_device *dev); > -void intel_mark_busy(struct drm_device *dev); > +bool intel_mark_busy(struct drm_device *dev); > void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > struct intel_ring_buffer *ring); > void intel_mark_idle(struct drm_device *dev); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-18 19:25 ` Mika Kuoppala @ 2014-02-18 21:34 ` Paulo Zanoni 2014-02-18 22:23 ` Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2014-02-18 21:34 UTC (permalink / raw) To: Mika Kuoppala; +Cc: Intel Graphics Development, Paulo Zanoni 2014-02-18 16:25 GMT-03:00 Mika Kuoppala <mika.kuoppala@linux.intel.com>: > Chris Wilson <chris@chris-wilson.co.uk> writes: > >> We currently call intel_mark_idle() too often, as we do so as a >> side-effect of processing the request queue. However, we the calls to >> intel_mark_idle() are expected to be paired with a call to >> intel_mark_busy() (or else we try to idle the hardware by accessing >> registers that are already disabled). Make the idle/busy tracking >> explicit to prevent the multiple calls. >> >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Thanks! I tested it and this patch + another local patch I have fix the problem that can be reproduced by the "gem-idle" subtest of pm_pc8.c (I still did not commit the subtest, but will do it soon). Also, I guess this patch deprecates dev_priv->pc8.gpu_idle. I had plans to move it to dev_priv->pm.gpu_idle, but now I'll try to kill it. >> --- >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >> drivers/gpu/drm/i915/i915_gem.c | 4 +--- >> drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 2 +- >> 4 files changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 00222cc..8441c8a 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1134,6 +1134,14 @@ struct i915_gem_mm { >> */ >> bool interruptible; >> >> + /** >> + * Is the GPU currently considered idle, or busy executing userspace >> + * requests? Whilst idle, we attempt to power down the hardware and >> + * display clocks. In order to reduce the effect on performance, there >> + * is a slight delay before we do so. >> + */ >> + bool busy; >> + >> /** Bit 6 swizzling required for X tiling */ >> uint32_t bit_6_swizzle_x; >> /** Bit 6 swizzling required for Y tiling */ >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 9a40ef5..4525dd7 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2315,7 +2315,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, >> i915_gem_context_reference(request->ctx); >> >> request->emitted_jiffies = jiffies; >> - was_empty = list_empty(&ring->request_list); >> list_add_tail(&request->list, &ring->request_list); >> request->file_priv = NULL; >> >> @@ -2336,12 +2335,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, >> if (!dev_priv->ums.mm_suspended) { >> i915_queue_hangcheck(ring->dev); >> >> - if (was_empty) { >> + if (intel_mark_busy(dev_priv->dev)) { I'm new to this code, so forgive me if I'm way off. Now that we changed the relative order, isn't it possible that we run the code line above, marking the device as busy, and then just before the next line runs, the still-not-canceled idle_work function runs and marks the device as idle? That could be bad, right? Also, why do we need the change on this function? >> cancel_delayed_work_sync(&dev_priv->mm.idle_work); >> queue_delayed_work(dev_priv->wq, >> &dev_priv->mm.retire_work, >> round_jiffies_up_relative(HZ)); >> - intel_mark_busy(dev_priv->dev); >> } >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index e127b23..bfd6396 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -8220,8 +8220,14 @@ void intel_mark_busy( > > bool intel_mark_busy(struct drm_device *dev) > > -Mika Exactly. Thanks, Paulo > >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> + if (dev_priv->mm.busy) >> + return false; >> + >> hsw_package_c8_gpu_busy(dev_priv); >> i915_update_gfx_val(dev_priv); >> + dev_priv->mm.busy = true; >> + >> + return true; >> } >> >> void intel_mark_idle(struct drm_device *dev) >> @@ -8229,6 +8235,11 @@ void intel_mark_idle(struct drm_device *dev) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct drm_crtc *crtc; >> >> + if (!dev_priv->mm.busy) >> + return; >> + >> + dev_priv->mm.busy = false; >> + >> hsw_package_c8_gpu_idle(dev_priv); >> >> if (!i915.powersave) >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index e5e1a5c..4c329e0 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -656,7 +656,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >> const char *intel_output_name(int output); >> bool intel_has_pending_fb_unpin(struct drm_device *dev); >> int intel_pch_rawclk(struct drm_device *dev); >> -void intel_mark_busy(struct drm_device *dev); >> +bool intel_mark_busy(struct drm_device *dev); >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, >> struct intel_ring_buffer *ring); >> void intel_mark_idle(struct drm_device *dev); >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-18 21:34 ` Paulo Zanoni @ 2014-02-18 22:23 ` Paulo Zanoni 2014-02-19 10:37 ` Chris Wilson 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2014-02-18 22:23 UTC (permalink / raw) To: Mika Kuoppala; +Cc: Intel Graphics Development, Paulo Zanoni 2014-02-18 18:34 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > 2014-02-18 16:25 GMT-03:00 Mika Kuoppala <mika.kuoppala@linux.intel.com>: >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >>> We currently call intel_mark_idle() too often, as we do so as a >>> side-effect of processing the request queue. However, we the calls to >>> intel_mark_idle() are expected to be paired with a call to >>> intel_mark_busy() (or else we try to idle the hardware by accessing >>> registers that are already disabled). Make the idle/busy tracking >>> explicit to prevent the multiple calls. >>> >>> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Thanks! > > I tested it and this patch + another local patch I have fix the > problem that can be reproduced by the "gem-idle" subtest of pm_pc8.c > (I still did not commit the subtest, but will do it soon). > > Also, I guess this patch deprecates dev_priv->pc8.gpu_idle. I had > plans to move it to dev_priv->pm.gpu_idle, but now I'll try to kill > it. > >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >>> drivers/gpu/drm/i915/i915_gem.c | 4 +--- >>> drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ >>> drivers/gpu/drm/i915/intel_drv.h | 2 +- >>> 4 files changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 00222cc..8441c8a 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1134,6 +1134,14 @@ struct i915_gem_mm { >>> */ >>> bool interruptible; >>> >>> + /** >>> + * Is the GPU currently considered idle, or busy executing userspace >>> + * requests? Whilst idle, we attempt to power down the hardware and >>> + * display clocks. In order to reduce the effect on performance, there >>> + * is a slight delay before we do so. >>> + */ >>> + bool busy; Hi Also, don't we want to init this to true, since the first thing called seems to be intel_mark_idle? I get intel_mark_idle called at 6 seconds after booting, then intel_mark_busy is called only 19 seconds after booting. I found this while writing the patch to deprecate dev_priv->pc8.gpu_idle :) Thanks, Paulo >>> + >>> /** Bit 6 swizzling required for X tiling */ >>> uint32_t bit_6_swizzle_x; >>> /** Bit 6 swizzling required for Y tiling */ >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index 9a40ef5..4525dd7 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -2315,7 +2315,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, >>> i915_gem_context_reference(request->ctx); >>> >>> request->emitted_jiffies = jiffies; >>> - was_empty = list_empty(&ring->request_list); >>> list_add_tail(&request->list, &ring->request_list); >>> request->file_priv = NULL; >>> >>> @@ -2336,12 +2335,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, >>> if (!dev_priv->ums.mm_suspended) { >>> i915_queue_hangcheck(ring->dev); >>> >>> - if (was_empty) { >>> + if (intel_mark_busy(dev_priv->dev)) { > > I'm new to this code, so forgive me if I'm way off. Now that we > changed the relative order, isn't it possible that we run the code > line above, marking the device as busy, and then just before the next > line runs, the still-not-canceled idle_work function runs and marks > the device as idle? That could be bad, right? > > Also, why do we need the change on this function? > > >>> cancel_delayed_work_sync(&dev_priv->mm.idle_work); >>> queue_delayed_work(dev_priv->wq, >>> &dev_priv->mm.retire_work, >>> round_jiffies_up_relative(HZ)); >>> - intel_mark_busy(dev_priv->dev); >>> } >>> } >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index e127b23..bfd6396 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -8220,8 +8220,14 @@ void intel_mark_busy( >> >> bool intel_mark_busy(struct drm_device *dev) >> >> -Mika > > Exactly. > > Thanks, > Paulo > >> >>> { >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> >>> + if (dev_priv->mm.busy) >>> + return false; >>> + >>> hsw_package_c8_gpu_busy(dev_priv); >>> i915_update_gfx_val(dev_priv); >>> + dev_priv->mm.busy = true; >>> + >>> + return true; >>> } >>> >>> void intel_mark_idle(struct drm_device *dev) >>> @@ -8229,6 +8235,11 @@ void intel_mark_idle(struct drm_device *dev) >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> struct drm_crtc *crtc; >>> >>> + if (!dev_priv->mm.busy) >>> + return; >>> + >>> + dev_priv->mm.busy = false; >>> + >>> hsw_package_c8_gpu_idle(dev_priv); >>> >>> if (!i915.powersave) >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>> index e5e1a5c..4c329e0 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -656,7 +656,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >>> const char *intel_output_name(int output); >>> bool intel_has_pending_fb_unpin(struct drm_device *dev); >>> int intel_pch_rawclk(struct drm_device *dev); >>> -void intel_mark_busy(struct drm_device *dev); >>> +bool intel_mark_busy(struct drm_device *dev); >>> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, >>> struct intel_ring_buffer *ring); >>> void intel_mark_idle(struct drm_device *dev); >>> -- >>> 1.7.9.5 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-18 22:23 ` Paulo Zanoni @ 2014-02-19 10:37 ` Chris Wilson 0 siblings, 0 replies; 9+ messages in thread From: Chris Wilson @ 2014-02-19 10:37 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni On Tue, Feb 18, 2014 at 07:23:13PM -0300, Paulo Zanoni wrote: > 2014-02-18 18:34 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > > 2014-02-18 16:25 GMT-03:00 Mika Kuoppala <mika.kuoppala@linux.intel.com>: > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >>> We currently call intel_mark_idle() too often, as we do so as a > >>> side-effect of processing the request queue. However, we the calls to > >>> intel_mark_idle() are expected to be paired with a call to > >>> intel_mark_busy() (or else we try to idle the hardware by accessing > >>> registers that are already disabled). Make the idle/busy tracking > >>> explicit to prevent the multiple calls. > >>> > >>> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Thanks! > > > > I tested it and this patch + another local patch I have fix the > > problem that can be reproduced by the "gem-idle" subtest of pm_pc8.c > > (I still did not commit the subtest, but will do it soon). > > > > Also, I guess this patch deprecates dev_priv->pc8.gpu_idle. I had > > plans to move it to dev_priv->pm.gpu_idle, but now I'll try to kill > > it. > > > >>> --- > >>> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > >>> drivers/gpu/drm/i915/i915_gem.c | 4 +--- > >>> drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > >>> drivers/gpu/drm/i915/intel_drv.h | 2 +- > >>> 4 files changed, 21 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>> index 00222cc..8441c8a 100644 > >>> --- a/drivers/gpu/drm/i915/i915_drv.h > >>> +++ b/drivers/gpu/drm/i915/i915_drv.h > >>> @@ -1134,6 +1134,14 @@ struct i915_gem_mm { > >>> */ > >>> bool interruptible; > >>> > >>> + /** > >>> + * Is the GPU currently considered idle, or busy executing userspace > >>> + * requests? Whilst idle, we attempt to power down the hardware and > >>> + * display clocks. In order to reduce the effect on performance, there > >>> + * is a slight delay before we do so. > >>> + */ > >>> + bool busy; > > Hi > > Also, don't we want to init this to true, since the first thing called > seems to be intel_mark_idle? As far as the state being tracked, it is logically .busy = false on first load, until we process the first batch. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-18 18:46 [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy Chris Wilson 2014-02-18 19:25 ` Mika Kuoppala @ 2014-02-21 17:38 ` Chris Wilson 2014-02-21 17:55 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Chris Wilson @ 2014-02-21 17:38 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni We currently call intel_mark_idle() too often, as we do so as a side-effect of processing the request queue. However, we the calls to intel_mark_idle() are expected to be paired with a call to intel_mark_busy() (or else we try to idle the hardware by accessing registers that are already disabled). Make the idle/busy tracking explicit to prevent the multiple calls. v2: We can drop some of the complexity in __i915_add_request() as queue_delayed_work() already behaves as we want (not requeuing the item if it is already in the queue) and mark_busy/mark_idle imply that the idle task is inactive. Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ drivers/gpu/drm/i915/i915_gem.c | 13 ++++--------- drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 00222ccc0708..8441c8ac7cc8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1134,6 +1134,14 @@ struct i915_gem_mm { */ bool interruptible; + /** + * Is the GPU currently considered idle, or busy executing userspace + * requests? Whilst idle, we attempt to power down the hardware and + * display clocks. In order to reduce the effect on performance, there + * is a slight delay before we do so. + */ + bool busy; + /** Bit 6 swizzling required for X tiling */ uint32_t bit_6_swizzle_x; /** Bit 6 swizzling required for Y tiling */ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 979c6b5ac6a3..4f25c0398a49 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2255,7 +2255,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, drm_i915_private_t *dev_priv = ring->dev->dev_private; struct drm_i915_gem_request *request; u32 request_ring_position, request_start; - int was_empty; int ret; request_start = intel_ring_get_tail(ring); @@ -2306,7 +2305,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, i915_gem_context_reference(request->ctx); request->emitted_jiffies = jiffies; - was_empty = list_empty(&ring->request_list); list_add_tail(&request->list, &ring->request_list); request->file_priv = NULL; @@ -2327,13 +2325,10 @@ int __i915_add_request(struct intel_ring_buffer *ring, if (!dev_priv->ums.mm_suspended) { i915_queue_hangcheck(ring->dev); - if (was_empty) { - cancel_delayed_work_sync(&dev_priv->mm.idle_work); - queue_delayed_work(dev_priv->wq, - &dev_priv->mm.retire_work, - round_jiffies_up_relative(HZ)); - intel_mark_busy(dev_priv->dev); - } + queue_delayed_work(dev_priv->wq, + &dev_priv->mm.retire_work, + round_jiffies_up_relative(HZ)); + intel_mark_busy(dev_priv->dev); } if (out_seqno) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e127b23f0c31..b3b2576a32fc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8220,8 +8220,12 @@ void intel_mark_busy(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (dev_priv->mm.busy) + return; + hsw_package_c8_gpu_busy(dev_priv); i915_update_gfx_val(dev_priv); + dev_priv->mm.busy = true; } void intel_mark_idle(struct drm_device *dev) @@ -8229,6 +8233,11 @@ void intel_mark_idle(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; + if (!dev_priv->mm.busy) + return; + + dev_priv->mm.busy = false; + hsw_package_c8_gpu_idle(dev_priv); if (!i915.powersave) -- 1.9.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-21 17:38 ` Chris Wilson @ 2014-02-21 17:55 ` Chris Wilson 2014-02-21 19:33 ` Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2014-02-21 17:55 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni We currently call intel_mark_idle() too often, as we do so as a side-effect of processing the request queue. However, we the calls to intel_mark_idle() are expected to be paired with a call to intel_mark_busy() (or else we try to idle the hardware by accessing registers that are already disabled). Make the idle/busy tracking explicit to prevent the multiple calls. v2: We can drop some of the complexity in __i915_add_request() as queue_delayed_work() already behaves as we want (not requeuing the item if it is already in the queue) and mark_busy/mark_idle imply that the idle task is inactive. v3: We do still need to cancel the pending idle task so that it is sent again after the current busy load completes (not in the middle of it). Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ drivers/gpu/drm/i915/i915_gem.c | 14 +++++--------- drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 00222ccc0708..8441c8ac7cc8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1134,6 +1134,14 @@ struct i915_gem_mm { */ bool interruptible; + /** + * Is the GPU currently considered idle, or busy executing userspace + * requests? Whilst idle, we attempt to power down the hardware and + * display clocks. In order to reduce the effect on performance, there + * is a slight delay before we do so. + */ + bool busy; + /** Bit 6 swizzling required for X tiling */ uint32_t bit_6_swizzle_x; /** Bit 6 swizzling required for Y tiling */ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 979c6b5ac6a3..293018392ac1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2255,7 +2255,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, drm_i915_private_t *dev_priv = ring->dev->dev_private; struct drm_i915_gem_request *request; u32 request_ring_position, request_start; - int was_empty; int ret; request_start = intel_ring_get_tail(ring); @@ -2306,7 +2305,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, i915_gem_context_reference(request->ctx); request->emitted_jiffies = jiffies; - was_empty = list_empty(&ring->request_list); list_add_tail(&request->list, &ring->request_list); request->file_priv = NULL; @@ -2327,13 +2325,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, if (!dev_priv->ums.mm_suspended) { i915_queue_hangcheck(ring->dev); - if (was_empty) { - cancel_delayed_work_sync(&dev_priv->mm.idle_work); - queue_delayed_work(dev_priv->wq, - &dev_priv->mm.retire_work, - round_jiffies_up_relative(HZ)); - intel_mark_busy(dev_priv->dev); - } + cancel_delayed_work_sync(&dev_priv->mm.idle_work); + queue_delayed_work(dev_priv->wq, + &dev_priv->mm.retire_work, + round_jiffies_up_relative(HZ)); + intel_mark_busy(dev_priv->dev); } if (out_seqno) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e127b23f0c31..b3b2576a32fc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8220,8 +8220,12 @@ void intel_mark_busy(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (dev_priv->mm.busy) + return; + hsw_package_c8_gpu_busy(dev_priv); i915_update_gfx_val(dev_priv); + dev_priv->mm.busy = true; } void intel_mark_idle(struct drm_device *dev) @@ -8229,6 +8233,11 @@ void intel_mark_idle(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; + if (!dev_priv->mm.busy) + return; + + dev_priv->mm.busy = false; + hsw_package_c8_gpu_idle(dev_priv); if (!i915.powersave) -- 1.9.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-21 17:55 ` Chris Wilson @ 2014-02-21 19:33 ` Paulo Zanoni 2014-03-05 10:55 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2014-02-21 19:33 UTC (permalink / raw) To: Chris Wilson; +Cc: Intel Graphics Development, Paulo Zanoni 2014-02-21 14:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > We currently call intel_mark_idle() too often, as we do so as a > side-effect of processing the request queue. However, we the calls to > intel_mark_idle() are expected to be paired with a call to > intel_mark_busy() (or else we try to idle the hardware by accessing > registers that are already disabled). Make the idle/busy tracking > explicit to prevent the multiple calls. > > v2: We can drop some of the complexity in __i915_add_request() as > queue_delayed_work() already behaves as we want (not requeuing the item > if it is already in the queue) and mark_busy/mark_idle imply that the > idle task is inactive. > > v3: We do still need to cancel the pending idle task so that it is sent > again after the current busy load completes (not in the middle of it). > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (ran the pm_pc8 test suite on HSW) > --- > drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > drivers/gpu/drm/i915/i915_gem.c | 14 +++++--------- > drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > 3 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 00222ccc0708..8441c8ac7cc8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1134,6 +1134,14 @@ struct i915_gem_mm { > */ > bool interruptible; > > + /** > + * Is the GPU currently considered idle, or busy executing userspace > + * requests? Whilst idle, we attempt to power down the hardware and > + * display clocks. In order to reduce the effect on performance, there > + * is a slight delay before we do so. > + */ > + bool busy; > + > /** Bit 6 swizzling required for X tiling */ > uint32_t bit_6_swizzle_x; > /** Bit 6 swizzling required for Y tiling */ > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 979c6b5ac6a3..293018392ac1 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2255,7 +2255,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, > drm_i915_private_t *dev_priv = ring->dev->dev_private; > struct drm_i915_gem_request *request; > u32 request_ring_position, request_start; > - int was_empty; > int ret; > > request_start = intel_ring_get_tail(ring); > @@ -2306,7 +2305,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, > i915_gem_context_reference(request->ctx); > > request->emitted_jiffies = jiffies; > - was_empty = list_empty(&ring->request_list); > list_add_tail(&request->list, &ring->request_list); > request->file_priv = NULL; > > @@ -2327,13 +2325,11 @@ int __i915_add_request(struct intel_ring_buffer *ring, > if (!dev_priv->ums.mm_suspended) { > i915_queue_hangcheck(ring->dev); > > - if (was_empty) { > - cancel_delayed_work_sync(&dev_priv->mm.idle_work); > - queue_delayed_work(dev_priv->wq, > - &dev_priv->mm.retire_work, > - round_jiffies_up_relative(HZ)); > - intel_mark_busy(dev_priv->dev); > - } > + cancel_delayed_work_sync(&dev_priv->mm.idle_work); > + queue_delayed_work(dev_priv->wq, > + &dev_priv->mm.retire_work, > + round_jiffies_up_relative(HZ)); > + intel_mark_busy(dev_priv->dev); > } > > if (out_seqno) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e127b23f0c31..b3b2576a32fc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8220,8 +8220,12 @@ void intel_mark_busy(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (dev_priv->mm.busy) > + return; > + > hsw_package_c8_gpu_busy(dev_priv); > i915_update_gfx_val(dev_priv); > + dev_priv->mm.busy = true; > } > > void intel_mark_idle(struct drm_device *dev) > @@ -8229,6 +8233,11 @@ void intel_mark_idle(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > + if (!dev_priv->mm.busy) > + return; > + > + dev_priv->mm.busy = false; > + > hsw_package_c8_gpu_idle(dev_priv); > > if (!i915.powersave) > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy 2014-02-21 19:33 ` Paulo Zanoni @ 2014-03-05 10:55 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2014-03-05 10:55 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni On Fri, Feb 21, 2014 at 04:33:07PM -0300, Paulo Zanoni wrote: > 2014-02-21 14:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > We currently call intel_mark_idle() too often, as we do so as a > > side-effect of processing the request queue. However, we the calls to > > intel_mark_idle() are expected to be paired with a call to > > intel_mark_busy() (or else we try to idle the hardware by accessing > > registers that are already disabled). Make the idle/busy tracking > > explicit to prevent the multiple calls. > > > > v2: We can drop some of the complexity in __i915_add_request() as > > queue_delayed_work() already behaves as we want (not requeuing the item > > if it is already in the queue) and mark_busy/mark_idle imply that the > > idle task is inactive. > > > > v3: We do still need to cancel the pending idle task so that it is sent > > again after the current busy load completes (not in the middle of it). > > > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (ran the pm_pc8 > test suite on HSW) Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-03-05 10:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-18 18:46 [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy Chris Wilson 2014-02-18 19:25 ` Mika Kuoppala 2014-02-18 21:34 ` Paulo Zanoni 2014-02-18 22:23 ` Paulo Zanoni 2014-02-19 10:37 ` Chris Wilson 2014-02-21 17:38 ` Chris Wilson 2014-02-21 17:55 ` Chris Wilson 2014-02-21 19:33 ` Paulo Zanoni 2014-03-05 10:55 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.