Re: [Intel-gfx] [PATCH 14/21] drm/i915/gem: Return an error ptr from context_lookup

On Thu, Apr 29, 2021 at 10:29:51AM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 8:27 AM Daniel Vetter daniel@ffwll.ch wrote:

...

On Fri, Apr 23, 2021 at 05:31:24PM -0500, Jason Ekstrand wrote:

...

We're about to start doing lazy context creation which means contexts get created in i915_gem_context_lookup and we may start having more errors than -ENOENT.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 ++++++------ drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_perf.c | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 3e883daab93bf..7929d5a8be449 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -2105,8 +2105,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, int ret = 0;

  ctx = i915_gem_context_lookup(file_priv, args->ctx_id);

• if (!ctx)
  

•         return -ENOENT;
  

• if (IS_ERR(ctx))
  

•         return PTR_ERR(ctx);
  
  switch (args->param) {
  case I915_CONTEXT_PARAM_GTT_SIZE:
  

@@ -2174,8 +2174,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, int ret;

  ctx = i915_gem_context_lookup(file_priv, args->ctx_id);

• if (!ctx)
  

•         return -ENOENT;
  

• if (IS_ERR(ctx))
  

•         return PTR_ERR(ctx);
  
  ret = ctx_setparam(file_priv, ctx, args);
  
  

@@ -2194,8 +2194,8 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, return -EINVAL;

  ctx = i915_gem_context_lookup(file->driver_priv, args->ctx_id);

• if (!ctx)
  

•         return -ENOENT;
  

• if (IS_ERR(ctx))
  

•         return PTR_ERR(ctx);
  
  /*
   * We opt for unserialised reads here. This may result in tearing
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 7024adcd5cf15..de14b26f3b2d5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -739,8 +739,8 @@ static int eb_select_context(struct i915_execbuffer *eb) struct i915_gem_context *ctx;

  ctx = i915_gem_context_lookup(eb->file->driver_priv, eb->args->rsvd1);

• if (unlikely(!ctx))
  

•         return -ENOENT;
  

• if (unlikely(IS_ERR(ctx)))
  

•         return PTR_ERR(ctx);
  
  eb->gem_context = ctx;
  if (rcu_access_pointer(ctx->vm))
  

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8571c5c1509a7..004ed0e59c999 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h

I just realized that I think __i915_gem_context_lookup_rcu doesn't have users anymore. Please make sure it's deleted.

I deleted it in "drm/i915: Stop manually RCU banging in reset_stats_ioctl"

Indeed that's the case, so looks all fine. The benefits of reviewing patches one-by-one :-/ -Daniel

...

...

@@ -1851,7 +1851,7 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) ctx = NULL; rcu_read_unlock();

• return ctx;
  

• return ctx ? ctx : ERR_PTR(-ENOENT);
  

}

/* i915_gem_evict.c */ diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 85ad62dbabfab..b86ed03f6a705 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -3414,10 +3414,10 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf, struct drm_i915_file_private *file_priv = file->driver_priv;

          specific_ctx = i915_gem_context_lookup(file_priv, ctx_handle);

•         if (!specific_ctx) {
  

•         if (IS_ERR(specific_ctx)) {
                  DRM_DEBUG("Failed to look up context with ID %u for opening perf stream\n",
                            ctx_handle);
  

•                 ret = -ENOENT;
  

•                 ret = PTR_ERR(specific_ctx);
  

Yeah this looks like a nice place to integrate this.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

One thing we need to make sure in the next patch or thereabouts is that lookup can only return ENOENT or ENOMEM, but never EINVAL. I'll drop some bikesheds on that :-)

I believe that is the case. All -EINVAL should be handled in the proto-context code.

--Jason

...

-Daniel

...

                  goto err;
          }
  }

-- 2.31.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch