| Message ID | 1469034967-15840-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2016 at 06:16:07PM +0100, Dave Gordon wrote: > 'ring' is an old deprecated term for a GPU engine, so we're trying to > phase out all such terminology. eb_select_ring() not only has 'ring' > (meaning engine) in its name, but it has an ugly calling convention > whereby it returns an errno and stores a pointer-to-engine indirectly > through an output parameter. As there is only one error it ever returns > (-EINVAL), we can make it return the pointer directly, and have the > caller pass back the error code -EINVAL if the pointer result is NULL. > > Thus we can replace > - ret = eb_select_ring(dev_priv, file, args, &engine); > - if (ret) > - return ret; > with > + engine = eb_select_engine(dev_priv, file, args); > + if (!engine) > + return -EINVAL; > for increased clarity and maybe save a few cycles too. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Thanks. I feel foolish for missing that easy transform before. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> There's a bsd_ring buried beneath here as well... -Chris
On 20/07/16 18:31, Chris Wilson wrote: > On Wed, Jul 20, 2016 at 06:16:07PM +0100, Dave Gordon wrote: >> 'ring' is an old deprecated term for a GPU engine, so we're trying to >> phase out all such terminology. eb_select_ring() not only has 'ring' >> (meaning engine) in its name, but it has an ugly calling convention >> whereby it returns an errno and stores a pointer-to-engine indirectly >> through an output parameter. As there is only one error it ever returns >> (-EINVAL), we can make it return the pointer directly, and have the >> caller pass back the error code -EINVAL if the pointer result is NULL. >> >> Thus we can replace >> - ret = eb_select_ring(dev_priv, file, args, &engine); >> - if (ret) >> - return ret; >> with >> + engine = eb_select_engine(dev_priv, file, args); >> + if (!engine) >> + return -EINVAL; >> for increased clarity and maybe save a few cycles too. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > Thanks. I feel foolish for missing that easy transform before. > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > There's a bsd_ring buried beneath here as well... The ugly calling convention was my doing in: commit de1add360522c876c25ef2bbbbab1c94bdb509ab Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Date: Fri Jan 15 15:12:50 2016 +0000 drm/i915: Decouple execbuf uAPI from internal implementation And the reason for it was to avoid growing the text size of i915_gem_do_execbuffer while at the same time extracting all the engine selection logic into a separate function. For some reason GCC most liked it like that. Or maybe I was only trying the ERR_PTR route, not the NULL/ptr return. Don't remember now. Anyway, I don't mind, just providing reasoning for the "ugly" calling convention. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6482ec2..f8d8ae3 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1380,24 +1380,24 @@ static bool only_mappable_for_reloc(unsigned int flags) [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) +static struct intel_engine_cs * +eb_select_engine(struct drm_i915_private *dev_priv, + struct drm_file *file, + struct drm_i915_gem_execbuffer2 *args) { unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; + struct intel_engine_cs *engine; if (user_ring_id > I915_USER_RINGS) { DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id); - return -EINVAL; + return NULL; } 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; + return NULL; } if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { @@ -1412,20 +1412,20 @@ static bool only_mappable_for_reloc(unsigned int flags) } else { DRM_DEBUG("execbuf with unknown bsd ring: %u\n", bsd_idx); - return -EINVAL; + return NULL; } - *ring = &dev_priv->engine[_VCS(bsd_idx)]; + engine = &dev_priv->engine[_VCS(bsd_idx)]; } else { - *ring = &dev_priv->engine[user_ring_map[user_ring_id]]; + engine = &dev_priv->engine[user_ring_map[user_ring_id]]; } - if (!intel_engine_initialized(*ring)) { + if (!intel_engine_initialized(engine)) { DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id); - return -EINVAL; + return NULL; } - return 0; + return engine; } static int @@ -1467,9 +1467,9 @@ static bool only_mappable_for_reloc(unsigned int flags) if (args->flags & I915_EXEC_IS_PINNED) dispatch_flags |= I915_DISPATCH_PINNED; - ret = eb_select_ring(dev_priv, file, args, &engine); - if (ret) - return ret; + engine = eb_select_engine(dev_priv, file, args); + if (!engine) + return -EINVAL; if (args->buffer_count < 1) { DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
'ring' is an old deprecated term for a GPU engine, so we're trying to phase out all such terminology. eb_select_ring() not only has 'ring' (meaning engine) in its name, but it has an ugly calling convention whereby it returns an errno and stores a pointer-to-engine indirectly through an output parameter. As there is only one error it ever returns (-EINVAL), we can make it return the pointer directly, and have the caller pass back the error code -EINVAL if the pointer result is NULL. Thus we can replace - ret = eb_select_ring(dev_priv, file, args, &engine); - if (ret) - return ret; with + engine = eb_select_engine(dev_priv, file, args); + if (!engine) + return -EINVAL; for increased clarity and maybe save a few cycles too. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-)