| Message ID | 1452870770-13981-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote: > +static int > +eb_select_ring(struct drm_i915_private *dev_priv, > + struct drm_file *file, > + struct drm_i915_gem_execbuffer2 *args, > + struct intel_engine_cs **ring) > +{ > + unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; > + > + if (user_ring_id > I915_USER_RINGS) { > + DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id); > + return -EINVAL; > + } > + > + if ((user_ring_id != I915_EXEC_BSD) && > + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { > + DRM_DEBUG("execbuf with non bsd ring but with invalid " > + "bsd dispatch flags: %d\n", (int)(args->flags)); Not your bug, but we should limit the flags reported here to (int)(args->flags & I915_EXEC_BSD_MASK). Though actually just nuke the test. At the moment, we complain for !BSD, then allow them even if we don't have BSD2 (and ignore the setting). A little inconsistent. If we just document that these flags only provide extra selection criteria for the I915_EXEC_BSD ring, we would be done. I'll pretend that it is adequately documented... Looks good and we completed our review of ABI impact for reordering the ring ids, so Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > At the moment execbuf ring selection is fully coupled to > internal ring ids which is not a good thing on its own. > > This dependency is also spread between two source files and > not spelled out at either side which makes it hidden and > fragile. > > This patch decouples this dependency by introducing an explicit > translation table of execbuf uAPI to ring id close to the only > call site (i915_gem_do_execbuffer). > > This way we are free to change driver internal implementation > details without breaking userspace. All state relating to the > uAPI is now contained in, or next to, i915_gem_do_execbuffer. > > As a side benefit, this patch decreases the compiled size > of i915_gem_do_execbuffer. > > v2: Extract ring selection into eb_select_ring. (Chris Wilson) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Yeah, this decoupling is nice, after all the point of include/uapi was to make the uapi vs. internal headers clear. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/i915_gem.c | 2 + > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 139 +++++++++++++++-------------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +-- > 4 files changed, 81 insertions(+), 74 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index eb7bb97f7316..35d5d6099a44 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -334,7 +334,7 @@ struct drm_i915_file_private { > unsigned boosts; > } rps; > > - struct intel_engine_cs *bsd_ring; > + unsigned int bsd_ring; > }; > > enum intel_dpll_id { > @@ -1300,7 +1300,7 @@ struct i915_gem_mm { > bool busy; > > /* the indicator for dispatch video commands on two BSD rings */ > - int bsd_ring_dispatch_index; > + unsigned int bsd_ring_dispatch_index; > > /** Bit 6 swizzling required for X tiling */ > uint32_t bit_6_swizzle_x; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ddc21d4b388d..26e6842f2df3 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5112,6 +5112,8 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file) > spin_lock_init(&file_priv->mm.lock); > INIT_LIST_HEAD(&file_priv->mm.request_list); > > + file_priv->bsd_ring = -1; > + > ret = i915_gem_context_open(dev, file); > if (ret) > kfree(file_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index d469c4779ff5..497a2f87836c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1328,33 +1328,23 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > > /** > * Find one BSD ring to dispatch the corresponding BSD command. > - * The Ring ID is returned. > + * The ring index is returned. > */ > -static int gen8_dispatch_bsd_ring(struct drm_device *dev, > - struct drm_file *file) > +static unsigned int > +gen8_dispatch_bsd_ring(struct drm_i915_private *dev_priv, struct drm_file *file) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_file_private *file_priv = file->driver_priv; > > - /* Check whether the file_priv is using one ring */ > - if (file_priv->bsd_ring) > - return file_priv->bsd_ring->id; > - else { > - /* If no, use the ping-pong mechanism to select one ring */ > - int ring_id; > - > - mutex_lock(&dev->struct_mutex); > - if (dev_priv->mm.bsd_ring_dispatch_index == 0) { > - ring_id = VCS; > - dev_priv->mm.bsd_ring_dispatch_index = 1; > - } else { > - ring_id = VCS2; > - dev_priv->mm.bsd_ring_dispatch_index = 0; > - } > - file_priv->bsd_ring = &dev_priv->ring[ring_id]; > - mutex_unlock(&dev->struct_mutex); > - return ring_id; > + /* Check whether the file_priv has already selected one ring. */ > + if ((int)file_priv->bsd_ring < 0) { > + /* If not, use the ping-pong mechanism to select one. */ > + mutex_lock(&dev_priv->dev->struct_mutex); > + file_priv->bsd_ring = dev_priv->mm.bsd_ring_dispatch_index; > + dev_priv->mm.bsd_ring_dispatch_index ^= 1; > + mutex_unlock(&dev_priv->dev->struct_mutex); > } > + > + return file_priv->bsd_ring; > } > > static struct drm_i915_gem_object * > @@ -1377,6 +1367,63 @@ eb_get_batch(struct eb_vmas *eb) > return vma->obj; > } > > +#define I915_USER_RINGS (4) > + > +static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = { > + [I915_EXEC_DEFAULT] = RCS, > + [I915_EXEC_RENDER] = RCS, > + [I915_EXEC_BLT] = BCS, > + [I915_EXEC_BSD] = VCS, > + [I915_EXEC_VEBOX] = VECS > +}; > + > +static int > +eb_select_ring(struct drm_i915_private *dev_priv, > + struct drm_file *file, > + struct drm_i915_gem_execbuffer2 *args, > + struct intel_engine_cs **ring) > +{ > + unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; > + > + if (user_ring_id > I915_USER_RINGS) { > + DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id); > + return -EINVAL; > + } > + > + if ((user_ring_id != I915_EXEC_BSD) && > + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { > + DRM_DEBUG("execbuf with non bsd ring but with invalid " > + "bsd dispatch flags: %d\n", (int)(args->flags)); > + return -EINVAL; > + } > + > + if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { > + unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > + > + if (bsd_idx == I915_EXEC_BSD_DEFAULT) { > + bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file); > + } else if (bsd_idx >= I915_EXEC_BSD_RING1 && > + bsd_idx <= I915_EXEC_BSD_RING2) { > + bsd_idx--; > + } else { > + DRM_DEBUG("execbuf with unknown bsd ring: %u\n", > + bsd_idx); > + return -EINVAL; > + } > + > + *ring = &dev_priv->ring[_VCS(bsd_idx)]; > + } else { > + *ring = &dev_priv->ring[user_ring_map[user_ring_id]]; > + } > + > + if (!intel_ring_initialized(*ring)) { > + DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int > i915_gem_do_execbuffer(struct drm_device *dev, void *data, > struct drm_file *file, > @@ -1414,51 +1461,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > if (args->flags & I915_EXEC_IS_PINNED) > dispatch_flags |= I915_DISPATCH_PINNED; > > - if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) { > - DRM_DEBUG("execbuf with unknown ring: %d\n", > - (int)(args->flags & I915_EXEC_RING_MASK)); > - return -EINVAL; > - } > - > - if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) && > - ((args->flags & I915_EXEC_BSD_MASK) != 0)) { > - DRM_DEBUG("execbuf with non bsd ring but with invalid " > - "bsd dispatch flags: %d\n", (int)(args->flags)); > - return -EINVAL; > - } > - > - if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT) > - ring = &dev_priv->ring[RCS]; > - else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) { > - if (HAS_BSD2(dev)) { > - int ring_id; > - > - switch (args->flags & I915_EXEC_BSD_MASK) { > - case I915_EXEC_BSD_DEFAULT: > - ring_id = gen8_dispatch_bsd_ring(dev, file); > - ring = &dev_priv->ring[ring_id]; > - break; > - case I915_EXEC_BSD_RING1: > - ring = &dev_priv->ring[VCS]; > - break; > - case I915_EXEC_BSD_RING2: > - ring = &dev_priv->ring[VCS2]; > - break; > - default: > - DRM_DEBUG("execbuf with unknown bsd ring: %d\n", > - (int)(args->flags & I915_EXEC_BSD_MASK)); > - return -EINVAL; > - } > - } else > - ring = &dev_priv->ring[VCS]; > - } else > - ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1]; > - > - if (!intel_ring_initialized(ring)) { > - DRM_DEBUG("execbuf with invalid ring: %d\n", > - (int)(args->flags & I915_EXEC_RING_MASK)); > - return -EINVAL; > - } > + ret = eb_select_ring(dev_priv, file, args, &ring); > + if (ret) > + return ret; > > if (args->buffer_count < 1) { > DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 7349d9258191..fdc220fc2b18 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -148,14 +148,14 @@ struct i915_ctx_workarounds { > struct intel_engine_cs { > const char *name; > enum intel_ring_id { > - RCS = 0x0, > - VCS, > + RCS = 0, > BCS, > - VECS, > - VCS2 > + VCS, > + VCS2, /* Keep instances of the same type engine together. */ > + VECS > } id; > #define I915_NUM_RINGS 5 > -#define LAST_USER_RING (VECS + 1) > +#define _VCS(n) (VCS + (n)) > u32 mmio_base; > struct drm_device *dev; > struct intel_ringbuffer *buffer; > -- > 1.9.1 >
> On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > + if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { > > + unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > > + > > + if (bsd_idx == I915_EXEC_BSD_DEFAULT) { > > + bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file); > > + } else if (bsd_idx >= I915_EXEC_BSD_RING1 && > > + bsd_idx <= I915_EXEC_BSD_RING2) { > > + bsd_idx--; There is one regression here, I915_EXEC_BSD_RING1/2 is left-shifted 13 bits, so we should add something like bsd_idx >>= 13; Otherwise the kernel panics.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index eb7bb97f7316..35d5d6099a44 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -334,7 +334,7 @@ struct drm_i915_file_private { unsigned boosts; } rps; - struct intel_engine_cs *bsd_ring; + unsigned int bsd_ring; }; enum intel_dpll_id { @@ -1300,7 +1300,7 @@ struct i915_gem_mm { bool busy; /* the indicator for dispatch video commands on two BSD rings */ - int bsd_ring_dispatch_index; + unsigned int bsd_ring_dispatch_index; /** Bit 6 swizzling required for X tiling */ uint32_t bit_6_swizzle_x; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ddc21d4b388d..26e6842f2df3 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5112,6 +5112,8 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file) spin_lock_init(&file_priv->mm.lock); INIT_LIST_HEAD(&file_priv->mm.request_list); + file_priv->bsd_ring = -1; + ret = i915_gem_context_open(dev, file); if (ret) kfree(file_priv); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index d469c4779ff5..497a2f87836c 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1328,33 +1328,23 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, /** * Find one BSD ring to dispatch the corresponding BSD command. - * The Ring ID is returned. + * The ring index is returned. */ -static int gen8_dispatch_bsd_ring(struct drm_device *dev, - struct drm_file *file) +static unsigned int +gen8_dispatch_bsd_ring(struct drm_i915_private *dev_priv, struct drm_file *file) { - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_file_private *file_priv = file->driver_priv; - /* Check whether the file_priv is using one ring */ - if (file_priv->bsd_ring) - return file_priv->bsd_ring->id; - else { - /* If no, use the ping-pong mechanism to select one ring */ - int ring_id; - - mutex_lock(&dev->struct_mutex); - if (dev_priv->mm.bsd_ring_dispatch_index == 0) { - ring_id = VCS; - dev_priv->mm.bsd_ring_dispatch_index = 1; - } else { - ring_id = VCS2; - dev_priv->mm.bsd_ring_dispatch_index = 0; - } - file_priv->bsd_ring = &dev_priv->ring[ring_id]; - mutex_unlock(&dev->struct_mutex); - return ring_id; + /* Check whether the file_priv has already selected one ring. */ + if ((int)file_priv->bsd_ring < 0) { + /* If not, use the ping-pong mechanism to select one. */ + mutex_lock(&dev_priv->dev->struct_mutex); + file_priv->bsd_ring = dev_priv->mm.bsd_ring_dispatch_index; + dev_priv->mm.bsd_ring_dispatch_index ^= 1; + mutex_unlock(&dev_priv->dev->struct_mutex); } + + return file_priv->bsd_ring; } static struct drm_i915_gem_object * @@ -1377,6 +1367,63 @@ eb_get_batch(struct eb_vmas *eb) return vma->obj; } +#define I915_USER_RINGS (4) + +static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = { + [I915_EXEC_DEFAULT] = RCS, + [I915_EXEC_RENDER] = RCS, + [I915_EXEC_BLT] = BCS, + [I915_EXEC_BSD] = VCS, + [I915_EXEC_VEBOX] = VECS +}; + +static int +eb_select_ring(struct drm_i915_private *dev_priv, + struct drm_file *file, + struct drm_i915_gem_execbuffer2 *args, + struct intel_engine_cs **ring) +{ + unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; + + if (user_ring_id > I915_USER_RINGS) { + DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id); + return -EINVAL; + } + + if ((user_ring_id != I915_EXEC_BSD) && + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { + DRM_DEBUG("execbuf with non bsd ring but with invalid " + "bsd dispatch flags: %d\n", (int)(args->flags)); + return -EINVAL; + } + + if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { + unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; + + if (bsd_idx == I915_EXEC_BSD_DEFAULT) { + bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file); + } else if (bsd_idx >= I915_EXEC_BSD_RING1 && + bsd_idx <= I915_EXEC_BSD_RING2) { + bsd_idx--; + } else { + DRM_DEBUG("execbuf with unknown bsd ring: %u\n", + bsd_idx); + return -EINVAL; + } + + *ring = &dev_priv->ring[_VCS(bsd_idx)]; + } else { + *ring = &dev_priv->ring[user_ring_map[user_ring_id]]; + } + + if (!intel_ring_initialized(*ring)) { + DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id); + return -EINVAL; + } + + return 0; +} + static int i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_file *file, @@ -1414,51 +1461,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (args->flags & I915_EXEC_IS_PINNED) dispatch_flags |= I915_DISPATCH_PINNED; - if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) { - DRM_DEBUG("execbuf with unknown ring: %d\n", - (int)(args->flags & I915_EXEC_RING_MASK)); - return -EINVAL; - } - - if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) && - ((args->flags & I915_EXEC_BSD_MASK) != 0)) { - DRM_DEBUG("execbuf with non bsd ring but with invalid " - "bsd dispatch flags: %d\n", (int)(args->flags)); - return -EINVAL; - } - - if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT) - ring = &dev_priv->ring[RCS]; - else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) { - if (HAS_BSD2(dev)) { - int ring_id; - - switch (args->flags & I915_EXEC_BSD_MASK) { - case I915_EXEC_BSD_DEFAULT: - ring_id = gen8_dispatch_bsd_ring(dev, file); - ring = &dev_priv->ring[ring_id]; - break; - case I915_EXEC_BSD_RING1: - ring = &dev_priv->ring[VCS]; - break; - case I915_EXEC_BSD_RING2: - ring = &dev_priv->ring[VCS2]; - break; - default: - DRM_DEBUG("execbuf with unknown bsd ring: %d\n", - (int)(args->flags & I915_EXEC_BSD_MASK)); - return -EINVAL; - } - } else - ring = &dev_priv->ring[VCS]; - } else - ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1]; - - if (!intel_ring_initialized(ring)) { - DRM_DEBUG("execbuf with invalid ring: %d\n", - (int)(args->flags & I915_EXEC_RING_MASK)); - return -EINVAL; - } + ret = eb_select_ring(dev_priv, file, args, &ring); + if (ret) + return ret; if (args->buffer_count < 1) { DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 7349d9258191..fdc220fc2b18 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -148,14 +148,14 @@ struct i915_ctx_workarounds { struct intel_engine_cs { const char *name; enum intel_ring_id { - RCS = 0x0, - VCS, + RCS = 0, BCS, - VECS, - VCS2 + VCS, + VCS2, /* Keep instances of the same type engine together. */ + VECS } id; #define I915_NUM_RINGS 5 -#define LAST_USER_RING (VECS + 1) +#define _VCS(n) (VCS + (n)) u32 mmio_base; struct drm_device *dev; struct intel_ringbuffer *buffer;