| Message ID | 1393001548-2883-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote: > From: 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: From Paulo > - Make it compile > - Drop the __i915_add_request chunk > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Tested-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> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > 2 files changed, 17 insertions(+) > > > Chris did not reply to my review comments yet, so I just went and implemented > them. We need at least an ACK form him here before merging. Didn't see them... Why have you altered the logic? -Chris
2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote: >> From: 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: From Paulo >> - Make it compile >> - Drop the __i915_add_request chunk >> >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Tested-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> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >> drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ >> 2 files changed, 17 insertions(+) >> >> >> Chris did not reply to my review comments yet, so I just went and implemented >> them. We need at least an ACK form him here before merging. > > Didn't see them... Why have you altered the logic? See the comment at the __i915_add_request chunk: http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html Maybe I just broke your patch :) If my review doesn't make sense, we can stick to your version, it should do the job, and I can retest everything easily. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote: > 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote: > >> From: 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: From Paulo > >> - Make it compile > >> - Drop the __i915_add_request chunk > >> > >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> Tested-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> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > >> drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > >> 2 files changed, 17 insertions(+) > >> > >> > >> Chris did not reply to my review comments yet, so I just went and implemented > >> them. We need at least an ACK form him here before merging. > > > > Didn't see them... Why have you altered the logic? > > See the comment at the __i915_add_request chunk: > > http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html Oh, I didn't look for comments inline. > > Maybe I just broke your patch :) > If my review doesn't make sense, we can stick to your version, it > should do the job, and I can retest everything easily. If there was a pending work item, the call to intel_mark_busy() would return false. So we can revamp the logic around there a little bit. The reason for the change should be self-evident - the previous code lost its way in the transition to multiple rings arguing over a global property. -Chris
2014-02-21 14:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote: >> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote: >> >> From: 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: From Paulo >> >> - Make it compile >> >> - Drop the __i915_add_request chunk >> >> >> >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Tested-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> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >> >> drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ >> >> 2 files changed, 17 insertions(+) >> >> >> >> >> >> Chris did not reply to my review comments yet, so I just went and implemented >> >> them. We need at least an ACK form him here before merging. >> > >> > Didn't see them... Why have you altered the logic? >> >> See the comment at the __i915_add_request chunk: >> >> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html > > Oh, I didn't look for comments inline. >> >> Maybe I just broke your patch :) >> If my review doesn't make sense, we can stick to your version, it >> should do the job, and I can retest everything easily. > > If there was a pending work item, the call to intel_mark_busy() would > return false. So we can revamp the logic around there a little bit. The > reason for the change should be self-evident - the previous code lost its > way in the transition to multiple rings arguing over a global property Just to avoid any possible confusions when/if we merge this series: Chris sent a new version of this patch on the original mail thread. Thanks, Paulo . > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Feb 21, 2014 at 04:34:29PM -0300, Paulo Zanoni wrote: > 2014-02-21 14:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote: > >> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > >> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote: > >> >> From: 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: From Paulo > >> >> - Make it compile > >> >> - Drop the __i915_add_request chunk > >> >> > >> >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> >> Tested-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> > >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> >> --- > >> >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > >> >> drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > >> >> 2 files changed, 17 insertions(+) > >> >> > >> >> > >> >> Chris did not reply to my review comments yet, so I just went and implemented > >> >> them. We need at least an ACK form him here before merging. > >> > > >> > Didn't see them... Why have you altered the logic? > >> > >> See the comment at the __i915_add_request chunk: > >> > >> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html > > > > Oh, I didn't look for comments inline. > >> > >> Maybe I just broke your patch :) > >> If my review doesn't make sense, we can stick to your version, it > >> should do the job, and I can retest everything easily. > > > > If there was a pending work item, the call to intel_mark_busy() would > > return false. So we can revamp the logic around there a little bit. The > > reason for the change should be self-evident - the previous code lost its > > way in the transition to multiple rings arguing over a global property > > Just to avoid any possible confusions when/if we merge this series: > Chris sent a new version of this patch on the original mail thread. Just to double check: Have I merged the right version? -Daniel
On Wed, Mar 05, 2014 at 02:13:15PM +0100, Daniel Vetter wrote:
> Just to double check: Have I merged the right version?
No conflicts or residual with my tree, so yes.
-Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8c64831..a5caa7e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1124,6 +1124,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/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f19e6ea..dd416f2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8192,8 +8192,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) @@ -8201,6 +8205,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)