[Intel-gfx] [PATCH] drm/i915: Fix some NUM_RING iterators

[Chris Wilson]

On Fri, Jun 27, 2014 at 03:21:20PM -0700, Rodrigo Vivi wrote:
>    Reviewed-by: Rodrigo Vivi <[1]rodrigo.vivi at intel.com>
> 
>    On Fri, Jun 27, 2014 at 3:09 PM, Ben Widawsky
>    <[2]benjamin.widawsky at intel.com> wrote:
> 
>      There are some cases in the code where we need to know how many rings to
>      iterate over, but cannot use for_each_ring(). These are always error
>      cases
>      which happen either before ring setup, or after ring teardown (or
>      reset).
> 
>      Note, a NUM_RINGS issue exists in semaphores, but this is fixed by the
>      remaining semaphore patches which Rodrigo will resubmit shortly. I'd
>      rather see those patches for fixing the problem than fix it here.
> 
>      I found this initially for the BSD2 case where on the same platform we
>      can have differing rings. AFAICT however this effects many platforms.
> 
>      I'd CC stable on this, except I think all the issues have been around
>      for multiple releases without bug reports.
> 
>      Compile tested only for now.
> 
>      Signed-off-by: Ben Widawsky <[3]ben at bwidawsk.net>
>      ---
>       drivers/gpu/drm/i915/i915_gem_context.c | 6 +++---
>       drivers/gpu/drm/i915/i915_gpu_error.c   | 2 +-
>       drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
>       3 files changed, 6 insertions(+), 4 deletions(-)
> 
>      diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>      b/drivers/gpu/drm/i915/i915_gem_context.c
>      index b9bac25..0c044a9 100644
>      --- a/drivers/gpu/drm/i915/i915_gem_context.c
>      +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>      @@ -403,7 +403,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> 
>              /* Prevent the hardware from restoring the last context (which
>      hung) on
>               * the next switch */
>      -       for (i = 0; i < I915_NUM_RINGS; i++) {
>      +       for (i = 0; i < I915_ACTIVE_RINGS(dev); i++) {
>                      struct intel_engine_cs *ring = &dev_priv->ring[i];
>                      struct intel_context *dctx = ring->default_context;
> 
>      @@ -456,7 +456,7 @@ int i915_gem_context_init(struct drm_device *dev)
>              }
> 
>              /* NB: RCS will hold a ref for all rings */
>      -       for (i = 0; i < I915_NUM_RINGS; i++)
>      +       for (i = 0; i < I915_ACTIVE_RINGS(dev); i++)
>                      dev_priv->ring[i].default_context = ctx;
> 
>              DRM_DEBUG_DRIVER("%s context support initialized\n",
>      dev_priv->hw_context_size ? "HW" : "fake");
>      @@ -493,7 +493,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>                      i915_gem_object_ggtt_unpin(dctx->obj);
>              }
> 
>      -       for (i = 0; i < I915_NUM_RINGS; i++) {
>      +       for (i = 0; i < I915_ACTIVE_RINGS(dev); i++) {
>                      struct intel_engine_cs *ring = &dev_priv->ring[i];
> 
>                      if (ring->last_context)
>      diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
>      b/drivers/gpu/drm/i915/i915_gpu_error.c
>      index 86362de..6e5250d 100644
>      --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>      +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>      @@ -848,7 +848,7 @@ static uint32_t i915_error_generate_code(struct
>      drm_i915_private *dev_priv,
>               * synchronization commands which almost always appear in the
>      case
>               * strictly a client bug. Use instdone to differentiate those
>      some.
>               */
>      -       for (i = 0; i < I915_NUM_RINGS; i++) {
>      +       for (i = 0; i < I915_ACTIVE_RINGS(dev_priv->dev); i++) {
>                      if (error->ring[i].hangcheck_action == HANGCHECK_HUNG) {
>                              if (ring_id)
>                                      *ring_id = i;
>      diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>      b/drivers/gpu/drm/i915/intel_ringbuffer.h
>      index e72017b..67e2919 100644
>      --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>      +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>      @@ -90,6 +90,8 @@ struct  intel_engine_cs {
>              } id;
>       #define I915_NUM_RINGS 5
>       #define LAST_USER_RING (VECS + 1)
>      +#define I915_ACTIVE_RINGS(dev) hweight8(INTEL_INFO(dev)->ring_mask)

What does the popcount of the mask have to do with the validity of the
arrays being iterated over in this patch?
-Chris


-- 
Chris Wilson, Intel Open Source Technology Centre