Re: [PATCH 09/17] drm/i915: Push the ring creation flags to the backend

Tvrtko Ursulin
2019-09-02T14:17:04Z

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@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