On Thu, May 10, 2012 at 03:39:32PM -0700, Ben Widawsky wrote:
> On Thu, 10 May 2012 22:21:50 +0100
> Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> > In many places we wish to iterate over the rings associated with the
> > GPU, so refactor them to use a common macro.
> >
> > Along the way, there are a few code removals that should be side-effect
> > free and some rearrangement which should only have a cosmetic impact,
> > such as error-state.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net> (if last hunk is removed
> and commit messages is amended)
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 9 ++---
> > drivers/gpu/drm/i915/i915_drv.c | 10 ++---
> > drivers/gpu/drm/i915/i915_drv.h | 9 +++--
> > drivers/gpu/drm/i915/i915_gem.c | 33 +++++++---------
> > drivers/gpu/drm/i915/i915_gem_evict.c | 14 +++----
> > drivers/gpu/drm/i915/i915_irq.c | 69 +++++++++++++--------------------
> > drivers/gpu/drm/i915/intel_pm.c | 6 ++-
> > 7 files changed, 66 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 950f72a..eb2b3c2 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -699,6 +699,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
> > struct drm_device *dev = error_priv->dev;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > struct drm_i915_error_state *error = error_priv->error;
> > + struct intel_ring_buffer *ring;
> > int i, j, page, offset, elt;
> >
> > if (!error) {
> > @@ -706,7 +707,6 @@ static int i915_error_state(struct seq_file *m, void *unused)
> > return 0;
> > }
> >
> > -
> > seq_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
> > error->time.tv_usec);
> > seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
> > @@ -722,11 +722,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
> > seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
> > }
> >
> > - i915_ring_error_state(m, dev, error, RCS);
> > - if (HAS_BLT(dev))
> > - i915_ring_error_state(m, dev, error, BCS);
> > - if (HAS_BSD(dev))
> > - i915_ring_error_state(m, dev, error, VCS);
> > + for_each_ring(ring, dev_priv, i)
> > + i915_ring_error_state(m, dev, error, i);
> >
> > if (error->active_bo)
> > print_error_buffers(m, "Active",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 5898be9..2b7967d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -893,15 +893,15 @@ int i915_reset(struct drm_device *dev)
> > */
> > if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> > !dev_priv->mm.suspended) {
> > + struct intel_ring_buffer *ring;
> > + int i;
> > +
> > dev_priv->mm.suspended = 0;
> >
> > i915_gem_init_swizzling(dev);
> >
> > - dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
> > - if (HAS_BSD(dev))
> > - dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
> > - if (HAS_BLT(dev))
> > - dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
> > + for_each_ring(ring, dev_priv, i)
> > + ring->init(ring, ring->obj);
> >
> > i915_gem_init_ppgtt(dev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 83a557c..c2e0732 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -410,9 +410,7 @@ typedef struct drm_i915_private {
> > #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> > struct timer_list hangcheck_timer;
> > int hangcheck_count;
> > - uint32_t last_acthd;
> > - uint32_t last_acthd_bsd;
> > - uint32_t last_acthd_blt;
> > + uint32_t last_acthd[I915_NUM_RINGS];
> > uint32_t last_instdone;
> > uint32_t last_instdone1;
> >
> > @@ -820,6 +818,11 @@ typedef struct drm_i915_private {
> > struct drm_property *force_audio_property;
> > } drm_i915_private_t;
> >
> > +/* Iterate over initialised rings */
> > +#define for_each_ring(ring__, dev_priv__, i__) \
> > + for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> > + if (((ring__) = &(dev_priv__)->ring[(i__)])->obj)
> > +
> > enum hdmi_force_audio {
> > HDMI_AUDIO_OFF_DVI = -2, /* no aux data for HDMI-DVI converter */
> > HDMI_AUDIO_OFF, /* force turn off HDMI audio */
>
> I'm somewhat disappointed with using ring->obj here. But I do not have a
> better suggestion.
I think we could wrap the ring->obj checking into intel_ring_initialized
and adjust the if checke like
if (((ring_) = &(dev_priv__)->ring[(i__)]) && intel_ring_initialized (ring__))
The follow-on patch to use the ring->obj check more would then also be
self-documenting.
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 44a5f24..99961ce 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1655,10 +1655,11 @@ void i915_gem_reset(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj;
> > + struct intel_ring_buffer *ring;
> > int i;
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++)
> > - i915_gem_reset_ring_lists(dev_priv, &dev_priv->ring[i]);
> > + for_each_ring(ring, dev_priv, i)
> > + i915_gem_reset_ring_lists(dev_priv, ring);
> >
> > /* Remove anything from the flushing lists. The GPU cache is likely
> > * to be lost on reset along with the data, so simply move the
> > @@ -1763,10 +1764,11 @@ void
> > i915_gem_retire_requests(struct drm_device *dev)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > + struct intel_ring_buffer *ring;
> > int i;
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++)
> > - i915_gem_retire_requests_ring(&dev_priv->ring[i]);
> > + for_each_ring(ring, dev_priv, i)
> > + i915_gem_retire_requests_ring(ring);
> > }
> >
> > static void
> > @@ -1774,6 +1776,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
> > {
> > drm_i915_private_t *dev_priv;
> > struct drm_device *dev;
> > + struct intel_ring_buffer *ring;
> > bool idle;
> > int i;
> >
> > @@ -1793,9 +1796,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
> > * objects indefinitely.
> > */
> > idle = true;
> > - for (i = 0; i < I915_NUM_RINGS; i++) {
> > - struct intel_ring_buffer *ring = &dev_priv->ring[i];
> > -
> > + for_each_ring(ring, dev_priv, i) {
> > if (!list_empty(&ring->gpu_write_list)) {
> > struct drm_i915_gem_request *request;
> > int ret;
> > @@ -2137,10 +2138,11 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
> > int i915_gpu_idle(struct drm_device *dev)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > + struct intel_ring_buffer *ring;
> > int ret, i;
> >
> > /* Flush everything onto the inactive list. */
> > - for (i = 0; i < I915_NUM_RINGS; i++) {
> > + for_each_ring(ring, dev_priv, i) {
> > ret = i915_ring_idle(&dev_priv->ring[i]);
> > if (ret)
> > return ret;
> > @@ -3463,9 +3465,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
> > /* GFX_MODE is per-ring on gen7+ */
> > }
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++) {
> > - ring = &dev_priv->ring[i];
> > -
> > + for_each_ring(ring, dev_priv, i) {
> > if (INTEL_INFO(dev)->gen >= 7)
> > I915_WRITE(RING_MODE_GEN7(ring),
> > _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> > @@ -3581,10 +3581,11 @@ void
> > i915_gem_cleanup_ringbuffer(struct drm_device *dev)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > + struct intel_ring_buffer *ring;
> > int i;
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++)
> > - intel_cleanup_ring_buffer(&dev_priv->ring[i]);
> > + for_each_ring(ring, dev_priv, i)
> > + intel_cleanup_ring_buffer(ring);
> > }
> >
> > int
> > @@ -3592,7 +3593,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file_priv)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - int ret, i;
> > + int ret;
> >
> > if (drm_core_check_feature(dev, DRIVER_MODESET))
> > return 0;
> > @@ -3614,10 +3615,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> > BUG_ON(!list_empty(&dev_priv->mm.active_list));
> > BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
> > BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
> > - for (i = 0; i < I915_NUM_RINGS; i++) {
> > - BUG_ON(!list_empty(&dev_priv->ring[i].active_list));
> > - BUG_ON(!list_empty(&dev_priv->ring[i].request_list));
> > - }
> > mutex_unlock(&dev->struct_mutex);
> >
> > ret = drm_irq_install(dev);
>
> I'm not opposed to this change, but it would have probably been better
> to not remove the BUG_ONs.
Shouldn't the ring->obj check dtrt here and we could just convert that
BUG_ON loop to for_each_ring?
> > index 3bcf045..ae7c24e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj, *next;
> > bool lists_empty;
> > - int ret,i;
> > + int ret;
> >
> > lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
> > list_empty(&dev_priv->mm.flushing_list) &&
> > @@ -178,17 +178,13 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> >
> > trace_i915_gem_evict_everything(dev, purgeable_only);
> >
> > - ret = i915_gpu_idle(dev);
> > - if (ret)
> > - return ret;
> > -
> > /* The gpu_idle will flush everything in the write domain to the
> > * active list. Then we must move everything off the active list
> > * with retire requests.
> > */
> > - for (i = 0; i < I915_NUM_RINGS; i++)
> > - if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
> > - return -EBUSY;
> > + ret = i915_gpu_idle(dev);
> > + if (ret)
> > + return ret;
> >
> > i915_gem_retire_requests(dev);
> >
>
> Sneaky way to get rid of the warning you never liked... I won't forget
> this.
I concur that killing that warnings is sneaky ;-) Can't we just keep them
- the ring->obj check should do the right thing? Or what exactly do I
miss?
> > @@ -203,5 +199,5 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> > }
> > }
> >
> > - return ret;
> > + return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2a062d7..cc4a633 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1022,15 +1022,11 @@ static void i915_gem_record_rings(struct drm_device *dev,
> > struct drm_i915_error_state *error)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_ring_buffer *ring;
> > struct drm_i915_gem_request *request;
> > int i, count;
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++) {
> > - struct intel_ring_buffer *ring = &dev_priv->ring[i];
> > -
> > - if (ring->obj == NULL)
> > - continue;
> > -
> > + for_each_ring(ring, dev_priv, i) {
> > i915_record_ring_state(dev, error, ring);
> >
> > error->ring[i].batchbuffer =
> > @@ -1295,6 +1291,8 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> > void i915_handle_error(struct drm_device *dev, bool wedged)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_ring_buffer *ring;
> > + int i;
> >
> > i915_capture_error_state(dev);
> > i915_report_and_clear_eir(dev);
> > @@ -1306,11 +1304,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
> > /*
> > * Wakeup waiting processes so they don't hang
> > */
> > - wake_up_all(&dev_priv->ring[RCS].irq_queue);
> > - if (HAS_BSD(dev))
> > - wake_up_all(&dev_priv->ring[VCS].irq_queue);
> > - if (HAS_BLT(dev))
> > - wake_up_all(&dev_priv->ring[BCS].irq_queue);
> > + for_each_ring(ring, dev_priv, i)
> > + wake_up_all(&ring->irq_queue);
> > }
> >
> > queue_work(dev_priv->wq, &dev_priv->error_work);
> > @@ -1515,11 +1510,6 @@ ring_last_seqno(struct intel_ring_buffer *ring)
> >
> > static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
> > {
> > - /* We don't check whether the ring even exists before calling this
> > - * function. Hence check whether it's initialized. */
> > - if (ring->obj == NULL)
> > - return true;
> > -
> > if (list_empty(&ring->request_list) ||
> > i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
> > /* Issue a wake-up to catch stuck h/w. */
> > @@ -1553,26 +1543,25 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
> > drm_i915_private_t *dev_priv = dev->dev_private;
> >
> > if (dev_priv->hangcheck_count++ > 1) {
> > + bool hung = true;
> > +
> > DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
> > i915_handle_error(dev, true);
> >
> > if (!IS_GEN2(dev)) {
> > + struct intel_ring_buffer *ring;
> > + int i;
> > +
> > /* Is the chip hanging on a WAIT_FOR_EVENT?
> > * If so we can simply poke the RB_WAIT bit
> > * and break the hang. This should work on
> > * all but the second generation chipsets.
> > */
> > - if (kick_ring(&dev_priv->ring[RCS]))
> > - return false;
> > -
> > - if (HAS_BSD(dev) && kick_ring(&dev_priv->ring[VCS]))
> > - return false;
> > -
> > - if (HAS_BLT(dev) && kick_ring(&dev_priv->ring[BCS]))
> > - return false;
> > + for_each_ring(ring, dev_priv, i)
> > + hung &= !kick_ring(ring);
> > }
> >
> > - return true;
> > + return hung;
> > }
> >
> > return false;
>
> It's worth noting in the commit message that there is a functional
> change here. Before we'd exit out on the first ring kicked, now we
> enumerate over all rings regardless.
I agree that's noteworthy (and poking all rings makes more sense anyway).
> I'm still a fan of ripping out the kicks completely.
We can't, it helps to get us out of broken userspace on gen3 (where we
lack gpu reset support).
> > @@ -1588,16 +1577,23 @@ void i915_hangcheck_elapsed(unsigned long data)
> > {
> > struct drm_device *dev = (struct drm_device *)data;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - uint32_t acthd, instdone, instdone1, acthd_bsd, acthd_blt;
> > - bool err = false;
> > + uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
> > + struct intel_ring_buffer *ring;
> > + bool err = false, idle;
> > + int i;
> >
> > if (!i915_enable_hangcheck)
> > return;
> >
> > + memset(acthd, 0, sizeof(acthd));
> > + idle = true;
> > + for_each_ring(ring, dev_priv, i) {
> > + idle &= i915_hangcheck_ring_idle(ring, &err);
> > + acthd[i] = intel_ring_get_active_head(ring);
> > + }
> > +
> > /* If all work is done then ACTHD clearly hasn't advanced. */
> > - if (i915_hangcheck_ring_idle(&dev_priv->ring[RCS], &err) &&
> > - i915_hangcheck_ring_idle(&dev_priv->ring[VCS], &err) &&
> > - i915_hangcheck_ring_idle(&dev_priv->ring[BCS], &err)) {
> > + if (idle) {
> > if (err) {
> > if (i915_hangcheck_hung(dev))
> > return;
>
> Similar functional change as previous: we do all rings no matter what.
>
> > @@ -1616,15 +1612,8 @@ void i915_hangcheck_elapsed(unsigned long data)
> > instdone = I915_READ(INSTDONE_I965);
> > instdone1 = I915_READ(INSTDONE1);
> > }
> > - acthd = intel_ring_get_active_head(&dev_priv->ring[RCS]);
> > - acthd_bsd = HAS_BSD(dev) ?
> > - intel_ring_get_active_head(&dev_priv->ring[VCS]) : 0;
> > - acthd_blt = HAS_BLT(dev) ?
> > - intel_ring_get_active_head(&dev_priv->ring[BCS]) : 0;
> >
> > - if (dev_priv->last_acthd == acthd &&
> > - dev_priv->last_acthd_bsd == acthd_bsd &&
> > - dev_priv->last_acthd_blt == acthd_blt &&
> > + if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> > dev_priv->last_instdone == instdone &&
> > dev_priv->last_instdone1 == instdone1) {
> > if (i915_hangcheck_hung(dev))
> > @@ -1632,9 +1621,7 @@ void i915_hangcheck_elapsed(unsigned long data)
> > } else {
> > dev_priv->hangcheck_count = 0;
> >
> > - dev_priv->last_acthd = acthd;
> > - dev_priv->last_acthd_bsd = acthd_bsd;
> > - dev_priv->last_acthd_blt = acthd_blt;
> > + memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> > dev_priv->last_instdone = instdone;
> > dev_priv->last_instdone1 = instdone1;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8f8d1da..438ff29 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2326,6 +2326,7 @@ int intel_enable_rc6(const struct drm_device *dev)
> >
> > void gen6_enable_rps(struct drm_i915_private *dev_priv)
> > {
> > + struct intel_ring_buffer *ring;
> > u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> > u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> > u32 pcu_mbox, rc6_mask = 0;
> > @@ -2360,8 +2361,8 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
> > I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
> > I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
> >
> > - for (i = 0; i < I915_NUM_RINGS; i++)
> > - I915_WRITE(RING_MAX_IDLE(dev_priv->ring[i].mmio_base), 10);
> > + for_each_ring(ring, dev_priv, i)
> > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> >
> > I915_WRITE(GEN6_RC_SLEEP, 0);
> > I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> > @@ -3048,6 +3049,7 @@ EXPORT_SYMBOL_GPL(i915_gpu_lower);
> > bool i915_gpu_busy(void)
> > {
> > struct drm_i915_private *dev_priv;
> > + struct intel_ring_buffer *ring;
> > bool ret = false;
> >
> > spin_lock(&mchdev_lock);
>
>
> There is still a hunk fail here.
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
