Re: [PATCH v9 1/6] drm/i915: Add per context timelines for fence objects

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 01-06-16 om 19:07 schreef John.C.Harrison@Intel.com:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The purpose of this patch series is to convert the requst structure to
> use fence objects for the underlying completion tracking. The fence
> object requires a sequence number. The ultimate aim is to use the same
> sequence number as for the request itself (or rather, to remove the
> request's seqno field and just use the fence's value throughout the
> driver). However, this is not currently possible and so this patch
> introduces a separate numbering scheme as an intermediate step.
>
> A major advantage of using the fence object is that it can be passed
> outside of the i915 driver and used externally. The fence API allows
> for various operations such as combining multiple fences. This
> requires that fence seqnos within a single fence context be guaranteed
> in-order. The GPU scheduler that is coming can re-order request
> execution but not within a single GPU context. Thus the fence context
> must be tied to the i915 context (and the engine within the context as
> each engine runs asynchronously).
>
> On the other hand, the driver as a whole currently only works with
> request seqnos that are allocated from a global in-order timeline. It
> will require a fair chunk of re-work to allow multiple independent
> seqno timelines to be used. Hence the introduction of a temporary,
> fence specific timeline. Once the work to update the rest of the
> driver has been completed then the request can use the fence seqno
> instead.
>
> v2: New patch in series.
>
> v3: Renamed/retyped timeline structure fields after review comments by
> Tvrtko Ursulin.
>
> Added context information to the timeline's name string for better
> identification in debugfs output.
>
> v5: Line wrapping and other white space fixes to keep style checker
> happy.
>
> v7: Updated to newer nightly (lots of ring -> engine renaming).
>
> v8: Moved to earlier in patch series so no longer needs to remove the
> quick hack timeline that was being added before.
>
> v9: Updated to another newer nightly (changes to context structure
> naming). Also updated commit message to match previous changes.
>
> For: VIZ-5190
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 14 ++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c         | 40 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_context.c | 16 +++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c        |  8 +++++++
>  4 files changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a88a46..a5f8ad8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -831,6 +831,19 @@ struct i915_ctx_hang_stats {
>  	bool banned;
>  };
>  
> +struct i915_fence_timeline {
> +	char        name[32];
> +	unsigned    fence_context;
Should be a u64 now, since commit 76bf0db5543976ef50362db7071da367cb118532
> +	unsigned    next;
> +
> +	struct i915_gem_context *ctx;
> +	struct intel_engine_cs *engine;
> +};
> +
> +int i915_create_fence_timeline(struct drm_device *dev,
> +			       struct i915_gem_context *ctx,
> +			       struct intel_engine_cs *ring);
> +
>  /* This must match up with the value previously used for execbuf2.rsvd1. */
>  #define DEFAULT_CONTEXT_HANDLE 0
>  
> @@ -875,6 +888,7 @@ struct i915_gem_context {
>  		u64 lrc_desc;
>  		int pin_count;
>  		bool initialised;
> +		struct i915_fence_timeline fence_timeline;
>  	} engine[I915_NUM_ENGINES];
>  
>  	struct list_head link;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5ffc6fa..57d3593 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2743,6 +2743,46 @@ void i915_gem_request_free(struct kref *req_ref)
>  	kmem_cache_free(req->i915->requests, req);
>  }
>  
> +int i915_create_fence_timeline(struct drm_device *dev,
> +			       struct i915_gem_context *ctx,
> +			       struct intel_engine_cs *engine)
> +{
> +	struct i915_fence_timeline *timeline;
> +
> +	timeline = &ctx->engine[engine->id].fence_timeline;
> +
> +	if (timeline->engine)
> +		return 0;
Do you ever expect a reinit?
> +	timeline->fence_context = fence_context_alloc(1);
> +
> +	/*
> +	 * Start the timeline from seqno 0 as this is a special value
> +	 * that is reserved for invalid sync points.
> +	 */
> +	timeline->next       = 1;
> +	timeline->ctx        = ctx;
> +	timeline->engine     = engine;
> +
> +	snprintf(timeline->name, sizeof(timeline->name), "%d>%s:%d",
> +		 timeline->fence_context, engine->name, ctx->user_handle);
> +
> +	return 0;
> +}
> +
On top of the other comments, you might want to add a TODO comment that there should be only one timeline for each context,
with each engine having only a unique fence->context.

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