| Message ID | 1463922152-2709-7-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/05/16 14:02, Chris Wilson wrote: > Rather than have every context ask "am I owned by the kernel? pin!", > move that logic into the creator of the kernel context, in order to > improve code comprehension. Makes sense. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 52 +++++++++++++++------------------ > 1 file changed, 23 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a4c6377eea32..51e83a79ab36 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -303,9 +303,7 @@ static struct i915_gem_context * > i915_gem_create_context(struct drm_device *dev, > struct drm_i915_file_private *file_priv) > { > - const bool is_global_default_ctx = file_priv == NULL; > struct i915_gem_context *ctx; > - int ret = 0; > > BUG_ON(!mutex_is_locked(&dev->struct_mutex)); > > @@ -313,30 +311,14 @@ i915_gem_create_context(struct drm_device *dev, > if (IS_ERR(ctx)) > return ctx; > > - if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) { > - /* We may need to do things with the shrinker which > - * require us to immediately switch back to the default > - * context. This can cause a problem as pinning the > - * default context also requires GTT space which may not > - * be available. To avoid this we always pin the default > - * context. > - */ > - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > - get_context_alignment(to_i915(dev)), 0); > - if (ret) { > - DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); > - goto err_destroy; > - } > - } > - > if (USES_FULL_PPGTT(dev)) { > struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); > > - if (IS_ERR_OR_NULL(ppgtt)) { > + if (IS_ERR(ppgtt)) { > DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > PTR_ERR(ppgtt)); > - ret = PTR_ERR(ppgtt); > - goto err_unpin; > + i915_gem_context_put(ctx); > + return ERR_CAST(ppgtt); > } > > ctx->ppgtt = ppgtt; > @@ -345,14 +327,6 @@ i915_gem_create_context(struct drm_device *dev, > trace_i915_context_create(ctx); > > return ctx; > - > -err_unpin: > - if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) > - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > -err_destroy: > - idr_remove(&file_priv->context_idr, ctx->user_handle); Isn't idr_remove still required in the error path above? > - i915_gem_context_put(ctx); > - return ERR_PTR(ret); > } > > static void i915_gem_context_unpin(struct i915_gem_context *ctx, > @@ -426,6 +400,26 @@ int i915_gem_context_init(struct drm_device *dev) > return PTR_ERR(ctx); > } > > + if (ctx->legacy_hw_ctx.rcs_state) { > + int ret; > + > + /* We may need to do things with the shrinker which > + * require us to immediately switch back to the default > + * context. This can cause a problem as pinning the > + * default context also requires GTT space which may not > + * be available. To avoid this we always pin the default > + * context. > + */ > + ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > + get_context_alignment(dev_priv), 0); > + if (ret) { > + DRM_ERROR("Failed to pinned default global context (error %d)\n", > + ret); > + i915_gem_context_put(ctx); > + return ret; > + } > + } > + > dev_priv->kernel_context = ctx; > > DRM_DEBUG_DRIVER("%s context support initialized\n", > Regards, Tvrtko
On Mon, May 23, 2016 at 10:33:24AM +0100, Tvrtko Ursulin wrote: > >@@ -313,30 +311,14 @@ i915_gem_create_context(struct drm_device *dev, > > if (IS_ERR(ctx)) > > return ctx; > > > >- if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) { > >- /* We may need to do things with the shrinker which > >- * require us to immediately switch back to the default > >- * context. This can cause a problem as pinning the > >- * default context also requires GTT space which may not > >- * be available. To avoid this we always pin the default > >- * context. > >- */ > >- ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > >- get_context_alignment(to_i915(dev)), 0); > >- if (ret) { > >- DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); > >- goto err_destroy; > >- } > >- } > >- > > if (USES_FULL_PPGTT(dev)) { > > struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); > > > >- if (IS_ERR_OR_NULL(ppgtt)) { > >+ if (IS_ERR(ppgtt)) { > > DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > > PTR_ERR(ppgtt)); > >- ret = PTR_ERR(ppgtt); > >- goto err_unpin; > >+ i915_gem_context_put(ctx); > >+ return ERR_CAST(ppgtt); > > } > > > > ctx->ppgtt = ppgtt; > >@@ -345,14 +327,6 @@ i915_gem_create_context(struct drm_device *dev, > > trace_i915_context_create(ctx); > > > > return ctx; > >- > >-err_unpin: > >- if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) > >- i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > >-err_destroy: > >- idr_remove(&file_priv->context_idr, ctx->user_handle); > > Isn't idr_remove still required in the error path above? Yes. I can blame a rebase error here since in the kernel it was extracted upon we call context_close() here instead which does the idr removal as well. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index a4c6377eea32..51e83a79ab36 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -303,9 +303,7 @@ static struct i915_gem_context * i915_gem_create_context(struct drm_device *dev, struct drm_i915_file_private *file_priv) { - const bool is_global_default_ctx = file_priv == NULL; struct i915_gem_context *ctx; - int ret = 0; BUG_ON(!mutex_is_locked(&dev->struct_mutex)); @@ -313,30 +311,14 @@ i915_gem_create_context(struct drm_device *dev, if (IS_ERR(ctx)) return ctx; - if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) { - /* We may need to do things with the shrinker which - * require us to immediately switch back to the default - * context. This can cause a problem as pinning the - * default context also requires GTT space which may not - * be available. To avoid this we always pin the default - * context. - */ - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, - get_context_alignment(to_i915(dev)), 0); - if (ret) { - DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); - goto err_destroy; - } - } - if (USES_FULL_PPGTT(dev)) { struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); - if (IS_ERR_OR_NULL(ppgtt)) { + if (IS_ERR(ppgtt)) { DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", PTR_ERR(ppgtt)); - ret = PTR_ERR(ppgtt); - goto err_unpin; + i915_gem_context_put(ctx); + return ERR_CAST(ppgtt); } ctx->ppgtt = ppgtt; @@ -345,14 +327,6 @@ i915_gem_create_context(struct drm_device *dev, trace_i915_context_create(ctx); return ctx; - -err_unpin: - if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); -err_destroy: - idr_remove(&file_priv->context_idr, ctx->user_handle); - i915_gem_context_put(ctx); - return ERR_PTR(ret); } static void i915_gem_context_unpin(struct i915_gem_context *ctx, @@ -426,6 +400,26 @@ int i915_gem_context_init(struct drm_device *dev) return PTR_ERR(ctx); } + if (ctx->legacy_hw_ctx.rcs_state) { + int ret; + + /* We may need to do things with the shrinker which + * require us to immediately switch back to the default + * context. This can cause a problem as pinning the + * default context also requires GTT space which may not + * be available. To avoid this we always pin the default + * context. + */ + ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, + get_context_alignment(dev_priv), 0); + if (ret) { + DRM_ERROR("Failed to pinned default global context (error %d)\n", + ret); + i915_gem_context_put(ctx); + return ret; + } + } + dev_priv->kernel_context = ctx; DRM_DEBUG_DRIVER("%s context support initialized\n",
Rather than have every context ask "am I owned by the kernel? pin!", move that logic into the creator of the kernel context, in order to improve code comprehension. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 52 +++++++++++++++------------------ 1 file changed, 23 insertions(+), 29 deletions(-)