| Message ID | 20210319223856.2983244-5-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: uAPI clean-ups part 2 | expand |
On 19/03/2021 22:38, Jason Ekstrand wrote: > I'd love to delete the SINGLE_TIMELINE API because it leaks an > implementation detail of contexts through to the API and is something > that userspace can do itself, trivially. Unfortunately, it's used by > the media driver so we can't do that. We can, however, do the next-best > thing which is to embed a syncobj in the context and do exactly what > we'd expect from userspace internally. > > This has a couple of advantages. One is that we're no longer leaking a > detail of the current execlist scheduler which will be problematic when > we try to add GuC scheduling. Second is that, together with deleting Narrative needs to be corrected as with the previous patch. > the CLONE_CONTEXT API, we should now have a 1:1 mapping between > intel_context and intel_timeline which should make some of our locking > mess a bit easier. Mess or complexity? Could you expand with some details so it's easier to understand? (I am thinking what gets easier, how and why, if this is done.) > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 47 ++++--------------- > .../gpu/drm/i915/gem/i915_gem_context_types.h | 8 +++- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++ > 3 files changed, 32 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index f88bac19333ec..e094f4a1ca4cd 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -67,6 +67,8 @@ > #include <linux/log2.h> > #include <linux/nospec.h> > > +#include <drm/drm_syncobj.h> > + > #include "gt/gen6_ppgtt.h" > #include "gt/intel_context.h" > #include "gt/intel_engine_heartbeat.h" > @@ -224,10 +226,6 @@ static void intel_context_set_gem(struct intel_context *ce, > ce->vm = vm; > } > > - GEM_BUG_ON(ce->timeline); > - if (ctx->timeline) > - ce->timeline = intel_timeline_get(ctx->timeline); > - > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > intel_engine_has_timeslices(ce->engine)) > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > @@ -344,8 +342,8 @@ void i915_gem_context_release(struct kref *ref) > mutex_destroy(&ctx->engines_mutex); > mutex_destroy(&ctx->lut_mutex); > > - if (ctx->timeline) > - intel_timeline_put(ctx->timeline); > + if (ctx->syncobj) > + drm_syncobj_put(ctx->syncobj); > > put_pid(ctx->pid); > mutex_destroy(&ctx->mutex); > @@ -790,33 +788,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, > i915_vm_close(vm); > } > > -static void __set_timeline(struct intel_timeline **dst, > - struct intel_timeline *src) > -{ > - struct intel_timeline *old = *dst; > - > - *dst = src ? intel_timeline_get(src) : NULL; > - > - if (old) > - intel_timeline_put(old); > -} > - > -static void __apply_timeline(struct intel_context *ce, void *timeline) > -{ > - __set_timeline(&ce->timeline, timeline); > -} > - > -static void __assign_timeline(struct i915_gem_context *ctx, > - struct intel_timeline *timeline) > -{ > - __set_timeline(&ctx->timeline, timeline); > - context_apply_all(ctx, __apply_timeline, timeline); > -} > - > static struct i915_gem_context * > i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > { > struct i915_gem_context *ctx; > + int ret; > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && > !HAS_EXECLISTS(i915)) > @@ -845,16 +821,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > } > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { If removal works out I would suggest deprecating the flag starting from some future platform. Maybe for GT gen greater than 12 you could already start rejecting in order to future proof. > - struct intel_timeline *timeline; > - > - timeline = intel_timeline_create(&i915->gt); > - if (IS_ERR(timeline)) { > + ret = drm_syncobj_create(&ctx->syncobj, > + DRM_SYNCOBJ_CREATE_SIGNALED, > + NULL); > + if (ret) { > context_close(ctx); > - return ERR_CAST(timeline); > + return ERR_PTR(ret); > } > - > - __assign_timeline(ctx, timeline); > - intel_timeline_put(timeline); > } > > trace_i915_context_create(ctx); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > index 676592e27e7d2..8a5fdd163b79d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > @@ -83,7 +83,13 @@ struct i915_gem_context { > struct i915_gem_engines __rcu *engines; > struct mutex engines_mutex; /* guards writes to engines */ > > - struct intel_timeline *timeline; > + /** > + * @syncobj: Shared timeline syncobj > + * > + * When the SHARED_TIMELINE flag is set on context creation, this > + * provides automatic implicit synchronization across all engines. > + */ > + struct drm_syncobj *syncobj; > > /** > * @vm: unique address space (GTT) > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 96403130a373d..2c56796f6a71b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, > goto err_vma; > } > > + if (eb.gem_context->syncobj) { > + struct dma_fence *fence; > + > + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); Who drops this reference? > + err = i915_request_await_dma_fence(eb.request, fence); > + if (err) > + goto err_ext; > + } > + > if (in_fence) { > if (args->flags & I915_EXEC_FENCE_SUBMIT) > err = i915_request_await_execution(eb.request, > @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, > fput(out_fence->file); > } > } > + > + if (eb.gem_context->syncobj) { > + drm_syncobj_replace_fence(eb.gem_context->syncobj, > + &eb.request->fence); > + } > + > i915_request_put(eb.request); > > err_vma: > So essentially moving the synchronisation to top level which is extra work, but given limited and questionable usage of the uapi may be acceptable. Need full picture on motivation to understand. Semantics are also not 1:1 since dma fence context will be different. So not fully single timeline as so far, but just implicitly serialised execution. Again due limited usage this may not be a problem. Worth spelling out in the commit message though. Regards, Tvrtko
On Mon, Mar 22, 2021 at 7:28 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 19/03/2021 22:38, Jason Ekstrand wrote: > > I'd love to delete the SINGLE_TIMELINE API because it leaks an > > implementation detail of contexts through to the API and is something > > that userspace can do itself, trivially. Unfortunately, it's used by > > the media driver so we can't do that. We can, however, do the next-best > > thing which is to embed a syncobj in the context and do exactly what > > we'd expect from userspace internally. > > > > This has a couple of advantages. One is that we're no longer leaking a > > detail of the current execlist scheduler which will be problematic when > > we try to add GuC scheduling. Second is that, together with deleting > > Narrative needs to be corrected as with the previous patch. > > > the CLONE_CONTEXT API, we should now have a 1:1 mapping between > > intel_context and intel_timeline which should make some of our locking > > mess a bit easier. > > Mess or complexity? Could you expand with some details so it's easier to > understand? (I am thinking what gets easier, how and why, if this is done.) Both? I guess "complexity" is a less abrasive way of stating it. I've not dug into the actual refactor yet but we should be able to drop the whole RCU business on intel_timeline that we have right now just to facilitate sharing like this. Fewer objects that are shared deep inside i915 with their own locks and RCUs seems like an advantage to me. Especially when we already have nice generic infrastructure for this. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 47 ++++--------------- > > .../gpu/drm/i915/gem/i915_gem_context_types.h | 8 +++- > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++ > > 3 files changed, 32 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > index f88bac19333ec..e094f4a1ca4cd 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -67,6 +67,8 @@ > > #include <linux/log2.h> > > #include <linux/nospec.h> > > > > +#include <drm/drm_syncobj.h> > > + > > #include "gt/gen6_ppgtt.h" > > #include "gt/intel_context.h" > > #include "gt/intel_engine_heartbeat.h" > > @@ -224,10 +226,6 @@ static void intel_context_set_gem(struct intel_context *ce, > > ce->vm = vm; > > } > > > > - GEM_BUG_ON(ce->timeline); > > - if (ctx->timeline) > > - ce->timeline = intel_timeline_get(ctx->timeline); > > - > > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > > intel_engine_has_timeslices(ce->engine)) > > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > @@ -344,8 +342,8 @@ void i915_gem_context_release(struct kref *ref) > > mutex_destroy(&ctx->engines_mutex); > > mutex_destroy(&ctx->lut_mutex); > > > > - if (ctx->timeline) > > - intel_timeline_put(ctx->timeline); > > + if (ctx->syncobj) > > + drm_syncobj_put(ctx->syncobj); > > > > put_pid(ctx->pid); > > mutex_destroy(&ctx->mutex); > > @@ -790,33 +788,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, > > i915_vm_close(vm); > > } > > > > -static void __set_timeline(struct intel_timeline **dst, > > - struct intel_timeline *src) > > -{ > > - struct intel_timeline *old = *dst; > > - > > - *dst = src ? intel_timeline_get(src) : NULL; > > - > > - if (old) > > - intel_timeline_put(old); > > -} > > - > > -static void __apply_timeline(struct intel_context *ce, void *timeline) > > -{ > > - __set_timeline(&ce->timeline, timeline); > > -} > > - > > -static void __assign_timeline(struct i915_gem_context *ctx, > > - struct intel_timeline *timeline) > > -{ > > - __set_timeline(&ctx->timeline, timeline); > > - context_apply_all(ctx, __apply_timeline, timeline); > > -} > > - > > static struct i915_gem_context * > > i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > > { > > struct i915_gem_context *ctx; > > + int ret; > > > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && > > !HAS_EXECLISTS(i915)) > > @@ -845,16 +821,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > > } > > > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { > > If removal works out I would suggest deprecating the flag starting from > some future platform. Maybe for GT gen greater than 12 you could already > start rejecting in order to future proof. > > > - struct intel_timeline *timeline; > > - > > - timeline = intel_timeline_create(&i915->gt); > > - if (IS_ERR(timeline)) { > > + ret = drm_syncobj_create(&ctx->syncobj, > > + DRM_SYNCOBJ_CREATE_SIGNALED, > > + NULL); > > + if (ret) { > > context_close(ctx); > > - return ERR_CAST(timeline); > > + return ERR_PTR(ret); > > } > > - > > - __assign_timeline(ctx, timeline); > > - intel_timeline_put(timeline); > > } > > > > trace_i915_context_create(ctx); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > index 676592e27e7d2..8a5fdd163b79d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > @@ -83,7 +83,13 @@ struct i915_gem_context { > > struct i915_gem_engines __rcu *engines; > > struct mutex engines_mutex; /* guards writes to engines */ > > > > - struct intel_timeline *timeline; > > + /** > > + * @syncobj: Shared timeline syncobj > > + * > > + * When the SHARED_TIMELINE flag is set on context creation, this > > + * provides automatic implicit synchronization across all engines. > > + */ > > + struct drm_syncobj *syncobj; > > > > /** > > * @vm: unique address space (GTT) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 96403130a373d..2c56796f6a71b 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > goto err_vma; > > } > > > > + if (eb.gem_context->syncobj) { > > + struct dma_fence *fence; > > + > > + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); > > Who drops this reference? i915_request_await_dma_fence() below consumes a reference. > > + err = i915_request_await_dma_fence(eb.request, fence); > > + if (err) > > + goto err_ext; > > + } > > + > > if (in_fence) { > > if (args->flags & I915_EXEC_FENCE_SUBMIT) > > err = i915_request_await_execution(eb.request, > > @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > fput(out_fence->file); > > } > > } > > + > > + if (eb.gem_context->syncobj) { > > + drm_syncobj_replace_fence(eb.gem_context->syncobj, > > + &eb.request->fence); > > + } > > + > > i915_request_put(eb.request); > > > > err_vma: > > > > So essentially moving the synchronisation to top level which is extra > work, but given limited and questionable usage of the uapi may be > acceptable. Need full picture on motivation to understand. For one thing, the GuC scheduler doesn't natively have a concept of "timelines" which can be shared like this. To work with the GuC scheduler as currently proposed in DII, they've asked the media driver to stop using this flag in favor of passing a sync file from batch to batch. If we want to slide GuC scheduling in smoothly, we've got to keep it working. This means either making timelines a concept there or doing an emulation like this. > Semantics are also not 1:1 since dma fence context will be different. Could you elaborate? --Jason > So > not fully single timeline as so far, but just implicitly serialised > execution. Again due limited usage this may not be a problem. Worth > spelling out in the commit message though. > > Regards, > > Tvrtko
On Fri, Mar 19, 2021 at 05:38:56PM -0500, Jason Ekstrand wrote: > I'd love to delete the SINGLE_TIMELINE API because it leaks an > implementation detail of contexts through to the API and is something > that userspace can do itself, trivially. Unfortunately, it's used by > the media driver so we can't do that. We can, however, do the next-best > thing which is to embed a syncobj in the context and do exactly what > we'd expect from userspace internally. > > This has a couple of advantages. One is that we're no longer leaking a > detail of the current execlist scheduler which will be problematic when > we try to add GuC scheduling. Second is that, together with deleting > the CLONE_CONTEXT API, we should now have a 1:1 mapping between > intel_context and intel_timeline which should make some of our locking > mess a bit easier. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 47 ++++--------------- > .../gpu/drm/i915/gem/i915_gem_context_types.h | 8 +++- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++ > 3 files changed, 32 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index f88bac19333ec..e094f4a1ca4cd 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -67,6 +67,8 @@ > #include <linux/log2.h> > #include <linux/nospec.h> > > +#include <drm/drm_syncobj.h> > + > #include "gt/gen6_ppgtt.h" > #include "gt/intel_context.h" > #include "gt/intel_engine_heartbeat.h" > @@ -224,10 +226,6 @@ static void intel_context_set_gem(struct intel_context *ce, > ce->vm = vm; > } > > - GEM_BUG_ON(ce->timeline); > - if (ctx->timeline) > - ce->timeline = intel_timeline_get(ctx->timeline); > - > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > intel_engine_has_timeslices(ce->engine)) > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > @@ -344,8 +342,8 @@ void i915_gem_context_release(struct kref *ref) > mutex_destroy(&ctx->engines_mutex); > mutex_destroy(&ctx->lut_mutex); > > - if (ctx->timeline) > - intel_timeline_put(ctx->timeline); > + if (ctx->syncobj) > + drm_syncobj_put(ctx->syncobj); > > put_pid(ctx->pid); > mutex_destroy(&ctx->mutex); > @@ -790,33 +788,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, > i915_vm_close(vm); > } > > -static void __set_timeline(struct intel_timeline **dst, > - struct intel_timeline *src) > -{ > - struct intel_timeline *old = *dst; > - > - *dst = src ? intel_timeline_get(src) : NULL; > - > - if (old) > - intel_timeline_put(old); > -} > - > -static void __apply_timeline(struct intel_context *ce, void *timeline) > -{ > - __set_timeline(&ce->timeline, timeline); > -} > - > -static void __assign_timeline(struct i915_gem_context *ctx, > - struct intel_timeline *timeline) > -{ > - __set_timeline(&ctx->timeline, timeline); > - context_apply_all(ctx, __apply_timeline, timeline); > -} > - > static struct i915_gem_context * > i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > { > struct i915_gem_context *ctx; > + int ret; > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && > !HAS_EXECLISTS(i915)) > @@ -845,16 +821,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > } > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { > - struct intel_timeline *timeline; > - > - timeline = intel_timeline_create(&i915->gt); > - if (IS_ERR(timeline)) { > + ret = drm_syncobj_create(&ctx->syncobj, > + DRM_SYNCOBJ_CREATE_SIGNALED, > + NULL); > + if (ret) { > context_close(ctx); > - return ERR_CAST(timeline); > + return ERR_PTR(ret); > } > - > - __assign_timeline(ctx, timeline); > - intel_timeline_put(timeline); > } > > trace_i915_context_create(ctx); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > index 676592e27e7d2..8a5fdd163b79d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > @@ -83,7 +83,13 @@ struct i915_gem_context { > struct i915_gem_engines __rcu *engines; > struct mutex engines_mutex; /* guards writes to engines */ > > - struct intel_timeline *timeline; > + /** > + * @syncobj: Shared timeline syncobj > + * > + * When the SHARED_TIMELINE flag is set on context creation, this > + * provides automatic implicit synchronization across all engines. I think we should explain a bit more what this actually does, i.e. emulate it using a syncobj, but unlike the real timeline it does not make any total order guarantees. Since userspace doesn't expect that. > + */ > + struct drm_syncobj *syncobj; > > /** > * @vm: unique address space (GTT) > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 96403130a373d..2c56796f6a71b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, > goto err_vma; > } > > + if (eb.gem_context->syncobj) { I think would be good to wrap these in unlikely, least also because it's nice documentation. -Daniel > + struct dma_fence *fence; > + > + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); > + err = i915_request_await_dma_fence(eb.request, fence); > + if (err) > + goto err_ext; > + } > + > if (in_fence) { > if (args->flags & I915_EXEC_FENCE_SUBMIT) > err = i915_request_await_execution(eb.request, > @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, > fput(out_fence->file); > } > } > + > + if (eb.gem_context->syncobj) { > + drm_syncobj_replace_fence(eb.gem_context->syncobj, > + &eb.request->fence); > + } > + > i915_request_put(eb.request); > > err_vma: > -- > 2.29.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 22, 2021 at 11:59 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Mar 19, 2021 at 05:38:56PM -0500, Jason Ekstrand wrote: > > I'd love to delete the SINGLE_TIMELINE API because it leaks an > > implementation detail of contexts through to the API and is something > > that userspace can do itself, trivially. Unfortunately, it's used by > > the media driver so we can't do that. We can, however, do the next-best > > thing which is to embed a syncobj in the context and do exactly what > > we'd expect from userspace internally. > > > > This has a couple of advantages. One is that we're no longer leaking a > > detail of the current execlist scheduler which will be problematic when > > we try to add GuC scheduling. Second is that, together with deleting > > the CLONE_CONTEXT API, we should now have a 1:1 mapping between > > intel_context and intel_timeline which should make some of our locking > > mess a bit easier. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 47 ++++--------------- > > .../gpu/drm/i915/gem/i915_gem_context_types.h | 8 +++- > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++ > > 3 files changed, 32 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > index f88bac19333ec..e094f4a1ca4cd 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -67,6 +67,8 @@ > > #include <linux/log2.h> > > #include <linux/nospec.h> > > > > +#include <drm/drm_syncobj.h> > > + > > #include "gt/gen6_ppgtt.h" > > #include "gt/intel_context.h" > > #include "gt/intel_engine_heartbeat.h" > > @@ -224,10 +226,6 @@ static void intel_context_set_gem(struct intel_context *ce, > > ce->vm = vm; > > } > > > > - GEM_BUG_ON(ce->timeline); > > - if (ctx->timeline) > > - ce->timeline = intel_timeline_get(ctx->timeline); > > - > > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > > intel_engine_has_timeslices(ce->engine)) > > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > @@ -344,8 +342,8 @@ void i915_gem_context_release(struct kref *ref) > > mutex_destroy(&ctx->engines_mutex); > > mutex_destroy(&ctx->lut_mutex); > > > > - if (ctx->timeline) > > - intel_timeline_put(ctx->timeline); > > + if (ctx->syncobj) > > + drm_syncobj_put(ctx->syncobj); > > > > put_pid(ctx->pid); > > mutex_destroy(&ctx->mutex); > > @@ -790,33 +788,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, > > i915_vm_close(vm); > > } > > > > -static void __set_timeline(struct intel_timeline **dst, > > - struct intel_timeline *src) > > -{ > > - struct intel_timeline *old = *dst; > > - > > - *dst = src ? intel_timeline_get(src) : NULL; > > - > > - if (old) > > - intel_timeline_put(old); > > -} > > - > > -static void __apply_timeline(struct intel_context *ce, void *timeline) > > -{ > > - __set_timeline(&ce->timeline, timeline); > > -} > > - > > -static void __assign_timeline(struct i915_gem_context *ctx, > > - struct intel_timeline *timeline) > > -{ > > - __set_timeline(&ctx->timeline, timeline); > > - context_apply_all(ctx, __apply_timeline, timeline); > > -} > > - > > static struct i915_gem_context * > > i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > > { > > struct i915_gem_context *ctx; > > + int ret; > > > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && > > !HAS_EXECLISTS(i915)) > > @@ -845,16 +821,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) > > } > > > > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { > > - struct intel_timeline *timeline; > > - > > - timeline = intel_timeline_create(&i915->gt); > > - if (IS_ERR(timeline)) { > > + ret = drm_syncobj_create(&ctx->syncobj, > > + DRM_SYNCOBJ_CREATE_SIGNALED, > > + NULL); > > + if (ret) { > > context_close(ctx); > > - return ERR_CAST(timeline); > > + return ERR_PTR(ret); > > } > > - > > - __assign_timeline(ctx, timeline); > > - intel_timeline_put(timeline); > > } > > > > trace_i915_context_create(ctx); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > index 676592e27e7d2..8a5fdd163b79d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > > @@ -83,7 +83,13 @@ struct i915_gem_context { > > struct i915_gem_engines __rcu *engines; > > struct mutex engines_mutex; /* guards writes to engines */ > > > > - struct intel_timeline *timeline; > > + /** > > + * @syncobj: Shared timeline syncobj > > + * > > + * When the SHARED_TIMELINE flag is set on context creation, this > > + * provides automatic implicit synchronization across all engines. > > I think we should explain a bit more what this actually does, i.e. emulate > it using a syncobj, but unlike the real timeline it does not make any > total order guarantees. Since userspace doesn't expect that. New comment: /** * @syncobj: Shared timeline syncobj * * When the SHARED_TIMELINE flag is set on context creation, we * emulate a single timeline across all engines using this syncobj. * For every execbuffer2 call, this syncobj is used as both an in- * and out-fence. Unlike the real intel_timeline, this doesn't * provide perfect atomic in-order guarantees if the client races * with itself by calling execbuffer2 twice concurrently. However, * if userspace races with itself, that's not likely to yield well- * defined results anyway so we choose to not care. */ > > + */ > > + struct drm_syncobj *syncobj; > > > > /** > > * @vm: unique address space (GTT) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 96403130a373d..2c56796f6a71b 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > goto err_vma; > > } > > > > + if (eb.gem_context->syncobj) { > > I think would be good to wrap these in unlikely, least also because it's > nice documentation. Done. > -Daniel > > > > + struct dma_fence *fence; > > + > > + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); > > + err = i915_request_await_dma_fence(eb.request, fence); > > + if (err) > > + goto err_ext; > > + } > > + > > if (in_fence) { > > if (args->flags & I915_EXEC_FENCE_SUBMIT) > > err = i915_request_await_execution(eb.request, > > @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > fput(out_fence->file); > > } > > } > > + > > + if (eb.gem_context->syncobj) { > > + drm_syncobj_replace_fence(eb.gem_context->syncobj, > > + &eb.request->fence); > > + } > > + > > i915_request_put(eb.request); > > > > err_vma: > > -- > > 2.29.2 > > > > _______________________________________________ > > 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
On 22/03/2021 16:10, Jason Ekstrand wrote: > On Mon, Mar 22, 2021 at 7:28 AM Tvrtko Ursulin [snip] >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> index 96403130a373d..2c56796f6a71b 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, >>> goto err_vma; >>> } >>> >>> + if (eb.gem_context->syncobj) { >>> + struct dma_fence *fence; >>> + >>> + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); >> >> Who drops this reference? > > i915_request_await_dma_fence() below consumes a reference. Not sure, please check on difference wrt input fence handling. >>> + err = i915_request_await_dma_fence(eb.request, fence); >>> + if (err) >>> + goto err_ext; >>> + } >>> + >>> if (in_fence) { >>> if (args->flags & I915_EXEC_FENCE_SUBMIT) >>> err = i915_request_await_execution(eb.request, >>> @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, >>> fput(out_fence->file); >>> } >>> } >>> + >>> + if (eb.gem_context->syncobj) { >>> + drm_syncobj_replace_fence(eb.gem_context->syncobj, >>> + &eb.request->fence); >>> + } >>> + >>> i915_request_put(eb.request); >>> >>> err_vma: >>> >> >> So essentially moving the synchronisation to top level which is extra >> work, but given limited and questionable usage of the uapi may be >> acceptable. Need full picture on motivation to understand. > > For one thing, the GuC scheduler doesn't natively have a concept of > "timelines" which can be shared like this. To work with the GuC Confused - neither does execlists. It is handled in common layer in i915. GuC scheduler has the same concept of one hw context is one timeline because that is the hw concept and not backend specific. > scheduler as currently proposed in DII, they've asked the media driver > to stop using this flag in favor of passing a sync file from batch to > batch. If we want to slide GuC scheduling in smoothly, we've got to > keep it working. This means either making timelines a concept there > or doing an emulation like this. Hm not aware and don't see that GuC backend can't or doesn't implement this. Perhaps this would be best discussed once GuC patches are posted. >> Semantics are also not 1:1 since dma fence context will be different. > > Could you elaborate? Exported dma fence context as an "timeline" id will be single with the current patch and multiple contexts with this implementation. Daniel also raised another difference caused by lack of serialisation due multiple tl->mutex here. I don't think this is important, it was never part of a contract what happens with racing execbufs, but it is definitely required covering both topics in the commit message. Regards, Tvrtko
On Tue, Mar 23, 2021 at 4:35 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 22/03/2021 16:10, Jason Ekstrand wrote: > > On Mon, Mar 22, 2021 at 7:28 AM Tvrtko Ursulin > > [snip] > > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >>> index 96403130a373d..2c56796f6a71b 100644 > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >>> @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, > >>> goto err_vma; > >>> } > >>> > >>> + if (eb.gem_context->syncobj) { > >>> + struct dma_fence *fence; > >>> + > >>> + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); > >> > >> Who drops this reference? > > > > i915_request_await_dma_fence() below consumes a reference. > > Not sure, please check on difference wrt input fence handling. Gah. You were right. It takes a reference if it needs one. I thought I was being symmetric with the other syncobj usage but it was well hidden inside our confusing tear-down paths. > >>> + err = i915_request_await_dma_fence(eb.request, fence); > >>> + if (err) > >>> + goto err_ext; > >>> + } > >>> + > >>> if (in_fence) { > >>> if (args->flags & I915_EXEC_FENCE_SUBMIT) > >>> err = i915_request_await_execution(eb.request, > >>> @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, > >>> fput(out_fence->file); > >>> } > >>> } > >>> + > >>> + if (eb.gem_context->syncobj) { At Daniel's request, I've wrapped these in unlikely() > >>> + drm_syncobj_replace_fence(eb.gem_context->syncobj, > >>> + &eb.request->fence); > >>> + } > >>> + > >>> i915_request_put(eb.request); > >>> > >>> err_vma: > >>> > >> > >> So essentially moving the synchronisation to top level which is extra > >> work, but given limited and questionable usage of the uapi may be > >> acceptable. Need full picture on motivation to understand. > > > > For one thing, the GuC scheduler doesn't natively have a concept of > > "timelines" which can be shared like this. To work with the GuC > > Confused - neither does execlists. It is handled in common layer in > i915. GuC scheduler has the same concept of one hw context is one > timeline because that is the hw concept and not backend specific. > > > scheduler as currently proposed in DII, they've asked the media driver > > to stop using this flag in favor of passing a sync file from batch to > > batch. If we want to slide GuC scheduling in smoothly, we've got to > > keep it working. This means either making timelines a concept there > > or doing an emulation like this. > > Hm not aware and don't see that GuC backend can't or doesn't implement > this. Perhaps this would be best discussed once GuC patches are posted. > > >> Semantics are also not 1:1 since dma fence context will be different. > > > > Could you elaborate? > > Exported dma fence context as an "timeline" id will be single with the > current patch and multiple contexts with this implementation. > > Daniel also raised another difference caused by lack of serialisation > due multiple tl->mutex here. > > I don't think this is important, it was never part of a contract what > happens with racing execbufs, but it is definitely required covering > both topics in the commit message. I've updated the commit message as follows: drm/i915: Implement SINGLE_TIMELINE with a syncobj (v2) This API is entirely unnecessary and I'd love to get rid of it. If userspace wants a single timeline across multiple contexts, they can either use implicit synchronization or a syncobj, both of which existed at the time this feature landed. The justification given at the time was that it would help GL drivers which are inherently single-timeline. However, neither of our GL drivers actually wanted the feature. i965 was already in maintenance mode at the time and iris uses syncobj for everything. Unfortunately, as much as I'd love to get rid of it, it is used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This isn't an entirely identical implementation because it's no longer atomic if userspace races with itself by calling execbuffer2 twice simultaneously from different threads. It won't crash in that case; it just doesn't guarantee any ordering between those two submits. Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical advantages beyond mere annoyance. One is that intel_timeline is no longer an api-visible object and can remain entirely an implementation detail. This may be advantageous as we make scheduler changes going forward. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which may help us reduce locking. I hope that clears up some of the confusion and is less bothersome. --Jason
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index f88bac19333ec..e094f4a1ca4cd 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -67,6 +67,8 @@ #include <linux/log2.h> #include <linux/nospec.h> +#include <drm/drm_syncobj.h> + #include "gt/gen6_ppgtt.h" #include "gt/intel_context.h" #include "gt/intel_engine_heartbeat.h" @@ -224,10 +226,6 @@ static void intel_context_set_gem(struct intel_context *ce, ce->vm = vm; } - GEM_BUG_ON(ce->timeline); - if (ctx->timeline) - ce->timeline = intel_timeline_get(ctx->timeline); - if (ctx->sched.priority >= I915_PRIORITY_NORMAL && intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); @@ -344,8 +342,8 @@ void i915_gem_context_release(struct kref *ref) mutex_destroy(&ctx->engines_mutex); mutex_destroy(&ctx->lut_mutex); - if (ctx->timeline) - intel_timeline_put(ctx->timeline); + if (ctx->syncobj) + drm_syncobj_put(ctx->syncobj); put_pid(ctx->pid); mutex_destroy(&ctx->mutex); @@ -790,33 +788,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, i915_vm_close(vm); } -static void __set_timeline(struct intel_timeline **dst, - struct intel_timeline *src) -{ - struct intel_timeline *old = *dst; - - *dst = src ? intel_timeline_get(src) : NULL; - - if (old) - intel_timeline_put(old); -} - -static void __apply_timeline(struct intel_context *ce, void *timeline) -{ - __set_timeline(&ce->timeline, timeline); -} - -static void __assign_timeline(struct i915_gem_context *ctx, - struct intel_timeline *timeline) -{ - __set_timeline(&ctx->timeline, timeline); - context_apply_all(ctx, __apply_timeline, timeline); -} - static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) { struct i915_gem_context *ctx; + int ret; if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && !HAS_EXECLISTS(i915)) @@ -845,16 +821,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) } if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { - struct intel_timeline *timeline; - - timeline = intel_timeline_create(&i915->gt); - if (IS_ERR(timeline)) { + ret = drm_syncobj_create(&ctx->syncobj, + DRM_SYNCOBJ_CREATE_SIGNALED, + NULL); + if (ret) { context_close(ctx); - return ERR_CAST(timeline); + return ERR_PTR(ret); } - - __assign_timeline(ctx, timeline); - intel_timeline_put(timeline); } trace_i915_context_create(ctx); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 676592e27e7d2..8a5fdd163b79d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -83,7 +83,13 @@ struct i915_gem_context { struct i915_gem_engines __rcu *engines; struct mutex engines_mutex; /* guards writes to engines */ - struct intel_timeline *timeline; + /** + * @syncobj: Shared timeline syncobj + * + * When the SHARED_TIMELINE flag is set on context creation, this + * provides automatic implicit synchronization across all engines. + */ + struct drm_syncobj *syncobj; /** * @vm: unique address space (GTT) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 96403130a373d..2c56796f6a71b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, goto err_vma; } + if (eb.gem_context->syncobj) { + struct dma_fence *fence; + + fence = drm_syncobj_fence_get(eb.gem_context->syncobj); + err = i915_request_await_dma_fence(eb.request, fence); + if (err) + goto err_ext; + } + if (in_fence) { if (args->flags & I915_EXEC_FENCE_SUBMIT) err = i915_request_await_execution(eb.request, @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, fput(out_fence->file); } } + + if (eb.gem_context->syncobj) { + drm_syncobj_replace_fence(eb.gem_context->syncobj, + &eb.request->fence); + } + i915_request_put(eb.request); err_vma:
I'd love to delete the SINGLE_TIMELINE API because it leaks an implementation detail of contexts through to the API and is something that userspace can do itself, trivially. Unfortunately, it's used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This has a couple of advantages. One is that we're no longer leaking a detail of the current execlist scheduler which will be problematic when we try to add GuC scheduling. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which should make some of our locking mess a bit easier. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 47 ++++--------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 8 +++- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++ 3 files changed, 32 insertions(+), 38 deletions(-)