[Intel-gfx] [PATCH 09/17] drm/i915: Push the ring creation flags to the backend
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Sep 2 14:17:02 UTC 2019
On 30/07/2019 14:30, Chris Wilson wrote:
> Push the ring creation flags from the outer GEM context to the inner
> intel_cotnext to avoid an unsightly back-reference from inside the
typo
> backend.
No mention of the pointer overload trick.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 21 +++++++++++------
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 3 ---
> drivers/gpu/drm/i915/gt/intel_context.c | 1 +
> drivers/gpu/drm/i915/gt/intel_context.h | 5 ++++
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 ++
> drivers/gpu/drm/i915/gt/intel_lrc.c | 5 ++--
> drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 2 +-
> drivers/gpu/drm/i915/gt/mock_engine.c | 9 ++++++--
> drivers/gpu/drm/i915/i915_debugfs.c | 23 ++++++++++++-------
> 9 files changed, 47 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 1b3dc7258ef2..2e8cedce059f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -434,8 +434,6 @@ __create_context(struct drm_i915_private *i915)
> i915_gem_context_set_bannable(ctx);
> i915_gem_context_set_recoverable(ctx);
>
> - ctx->ring_size = 4 * PAGE_SIZE;
> -
> for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
> ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>
> @@ -565,8 +563,15 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> i915_gem_context_set_closed(ctx); /* not user accessible */
> i915_gem_context_clear_bannable(ctx);
> i915_gem_context_set_force_single_submission(ctx);
> - if (!USES_GUC_SUBMISSION(to_i915(dev)))
> - ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
> + if (!USES_GUC_SUBMISSION(to_i915(dev))) {
> + const unsigned long ring_size = 512 * SZ_4K; /* max */
> + struct i915_gem_engines_iter it;
> + struct intel_context *ce;
> +
> + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it)
> + ce->ring = __intel_context_ring_size(ring_size);
> + i915_gem_context_unlock_engines(ctx);
> + }
>
> GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> out:
> @@ -605,7 +610,6 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
>
> i915_gem_context_clear_bannable(ctx);
> ctx->sched.priority = I915_USER_PRIORITY(prio);
> - ctx->ring_size = PAGE_SIZE;
>
> GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
>
> @@ -1589,6 +1593,7 @@ set_engines(struct i915_gem_context *ctx,
> for (n = 0; n < num_engines; n++) {
> struct i915_engine_class_instance ci;
> struct intel_engine_cs *engine;
> + struct intel_context *ce;
>
> if (copy_from_user(&ci, &user->engines[n], sizeof(ci))) {
> __free_engines(set.engines, n);
> @@ -1611,11 +1616,13 @@ set_engines(struct i915_gem_context *ctx,
> return -ENOENT;
> }
>
> - set.engines->engines[n] = intel_context_create(ctx, engine);
> - if (!set.engines->engines[n]) {
> + ce = intel_context_create(ctx, engine);
> + if (!ce) {
> __free_engines(set.engines, n);
> return -ENOMEM;
> }
> +
> + set.engines->engines[n] = ce;
> }
> set.engines->num_engines = num_engines;
>
> 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 a02d98494078..260d59cc3de8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -169,9 +169,6 @@ struct i915_gem_context {
>
> struct i915_sched_attr sched;
>
> - /** ring_size: size for allocating the per-engine ring buffer */
> - u32 ring_size;
> -
> /** guilty_count: How many times this context has caused a GPU hang. */
> atomic_t guilty_count;
> /**
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 34c8e37a73b8..db9236570ff5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -214,6 +214,7 @@ intel_context_init(struct intel_context *ce,
> ce->engine = engine;
> ce->ops = engine->cops;
> ce->sseu = engine->sseu;
> + ce->ring = __intel_context_ring_size(SZ_16K);
>
> INIT_LIST_HEAD(&ce->signal_link);
> INIT_LIST_HEAD(&ce->signals);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 07f9924de48f..13f28dd316bc 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -136,4 +136,9 @@ int intel_context_prepare_remote_request(struct intel_context *ce,
>
> struct i915_request *intel_context_create_request(struct intel_context *ce);
>
> +static inline struct intel_ring *__intel_context_ring_size(u64 sz)
> +{
> + return u64_to_ptr(struct intel_ring, sz);
How does this make sense on 32-bit builds? No warnings about potential
truncation?
At least I hope compiler is smart not to grow the code for assignments
for which u64 is overkill.
> +}
> +
> #endif /* __INTEL_CONTEXT_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 65cbf1d9118d..97ce3589338e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -783,6 +783,8 @@ static int pin_context(struct i915_gem_context *ctx,
> if (IS_ERR(ce))
> return PTR_ERR(ce);
>
> + ce->ring = __intel_context_ring_size(SZ_4K);
> +
> err = intel_context_pin(ce);
> intel_context_put(ce);
> if (err)
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 232f40fcb490..5e113ddbe273 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -3108,9 +3108,8 @@ static int execlists_context_deferred_alloc(struct intel_context *ce,
> goto error_deref_obj;
> }
>
> - ring = intel_engine_create_ring(engine,
> - timeline,
> - ce->gem_context->ring_size);
> + ring = intel_engine_create_ring(engine, timeline,
> + (unsigned long)ce->ring);
Strictly speaking uintptr_t, no?
I'd be tempted to add an assert in __intel_context_ring_size against
asking for more than the intel_engine_create_ring can create. Or
actually an assert in the latter to protect against calling it with a
size smelling of pointers.
> intel_timeline_put(timeline);
> if (IS_ERR(ring)) {
> ret = PTR_ERR(ring);
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index 8d24a49e5139..ebda379f7bac 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -2342,7 +2342,7 @@ int intel_ring_submission_init(struct intel_engine_cs *engine)
> }
> GEM_BUG_ON(timeline->has_initial_breadcrumb);
>
> - ring = intel_engine_create_ring(engine, timeline, 32 * PAGE_SIZE);
> + ring = intel_engine_create_ring(engine, timeline, SZ_16K);
16K is less than 128k and commit message say nothing about it.
> intel_timeline_put(timeline);
> if (IS_ERR(ring)) {
> err = PTR_ERR(ring);
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index 10cb312462e5..bf2dc1142f3c 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -133,13 +133,18 @@ static void mock_context_unpin(struct intel_context *ce)
> mock_timeline_unpin(ce->ring->timeline);
> }
>
> +static bool has_ring(struct intel_context *ce)
> +{
> + return ce->ring > __intel_context_ring_size(SZ_16K);
No comments added in struct intel_context definition of here to leave
note of the trick for a future reader. If at least union was too much to
self-document at least partially.
> +}
> +
> static void mock_context_destroy(struct kref *ref)
> {
> struct intel_context *ce = container_of(ref, typeof(*ce), ref);
>
> GEM_BUG_ON(intel_context_is_pinned(ce));
>
> - if (ce->ring)
> + if (has_ring(ce))
> mock_ring_free(ce->ring);
>
> intel_context_fini(ce);
> @@ -150,7 +155,7 @@ static int mock_context_pin(struct intel_context *ce)
> {
> int ret;
>
> - if (!ce->ring) {
> + if (!has_ring(ce)) {
> ce->ring = mock_ring(ce->engine);
> if (!ce->ring)
> return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24787bb48c9f..0ff504f79779 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -328,10 +328,14 @@ static void print_context_stats(struct seq_file *m,
>
> for_each_gem_engine(ce,
> i915_gem_context_lock_engines(ctx), it) {
> - if (ce->state)
> - per_file_stats(0, ce->state->obj, &kstats);
> - if (ce->ring)
> + intel_context_lock_pinned(ce);
> + if (intel_context_is_pinned(ce)) {
> + if (ce->state)
> + per_file_stats(0,
> + ce->state->obj, &kstats);
> per_file_stats(0, ce->ring->vma->obj, &kstats);
> + }
> + intel_context_unlock_pinned(ce);
> }
> i915_gem_context_unlock_engines(ctx);
>
> @@ -1677,12 +1681,15 @@ static int i915_context_status(struct seq_file *m, void *unused)
>
> for_each_gem_engine(ce,
> i915_gem_context_lock_engines(ctx), it) {
> - seq_printf(m, "%s: ", ce->engine->name);
> - if (ce->state)
> - describe_obj(m, ce->state->obj);
> - if (ce->ring)
> + intel_context_lock_pinned(ce);
> + if (intel_context_is_pinned(ce)) {
> + seq_printf(m, "%s: ", ce->engine->name);
> + if (ce->state)
> + describe_obj(m, ce->state->obj);
> describe_ctx_ring(m, ce->ring);
> - seq_putc(m, '\n');
> + seq_putc(m, '\n');
> + }
> + intel_context_unlock_pinned(ce);
> }
> i915_gem_context_unlock_engines(ctx);
>
>
You can tell I am miffed you are just happy to ignore my complaints and
move forward. I would had at least used an union for not cost than few
extra lines of code, which should exist in forms of comments anyway.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list