[Intel-gfx] [RFC 1/5] drm/i915: introduce logical_ring and lr_context naming
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Dec 11 22:04:48 UTC 2019
On 12/11/19 1:20 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-12-11 21:12:40)
>> Ahead of splitting out the code specific to execlists submission to its
>> own file, we can re-organize the code within the intel_lrc file to make
>> that separation clearer. To achieve this, a number of functions have
>> been split/renamed using the "logical_ring" and "lr_context" naming,
>> respectively for engine-related setup and lrc management.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_lrc.c | 154 ++++++++++++++-----------
>> drivers/gpu/drm/i915/gt/selftest_lrc.c | 12 +-
>> 2 files changed, 93 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> index 929f6bae4eba..6d6148e11fd0 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> @@ -228,17 +228,17 @@ static struct virtual_engine *to_virtual_engine(struct intel_engine_cs *engine)
>> return container_of(engine, struct virtual_engine, base);
>> }
>>
>> -static int __execlists_context_alloc(struct intel_context *ce,
>> - struct intel_engine_cs *engine);
>> -
>> -static void execlists_init_reg_state(u32 *reg_state,
>> - const struct intel_context *ce,
>> - const struct intel_engine_cs *engine,
>> - const struct intel_ring *ring,
>> - bool close);
>> +static int lr_context_alloc(struct intel_context *ce,
>> + struct intel_engine_cs *engine);
>
> Execlists.
>
>> +
>> +static void lr_context_init_reg_state(u32 *reg_state,
>> + const struct intel_context *ce,
>> + const struct intel_engine_cs *engine,
>> + const struct intel_ring *ring,
>> + bool close);
>
> lrc. lrc should just be the register offsets and default context image.
>
I've used "lrc" for anything that was related to managing the context
object (creation, population, pin, etc) and "execlists" for anything
related to executing the context on the HW, with the aim of having the
GuC code call only into lrc functions and not into execlists ones.
If you prefer keeping the execlists naming for anything non related to
the context of the context image, should we go for execlist_submission_*
for anything that's specific to the execlist submission, and avoid those
from the GuC side?
Daniele
>> static void
>> -__execlists_update_reg_state(const struct intel_context *ce,
>> - const struct intel_engine_cs *engine);
>> +lr_context_update_reg_state(const struct intel_context *ce,
>> + const struct intel_engine_cs *engine);
>
> lrc.
>
>>
>> static void mark_eio(struct i915_request *rq)
>> {
>> @@ -1035,8 +1035,8 @@ execlists_check_context(const struct intel_context *ce,
>> WARN_ONCE(!valid, "Invalid lrc state found before submission\n");
>> }
>>
>> -static void restore_default_state(struct intel_context *ce,
>> - struct intel_engine_cs *engine)
>> +static void lr_context_restore_default_state(struct intel_context *ce,
>> + struct intel_engine_cs *engine)
>> {
>> u32 *regs = ce->lrc_reg_state;
>>
>> @@ -1045,7 +1045,7 @@ static void restore_default_state(struct intel_context *ce,
>> engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
>> engine->context_size - PAGE_SIZE);
>>
>> - execlists_init_reg_state(regs, ce, engine, ce->ring, false);
>> + lr_context_init_reg_state(regs, ce, engine, ce->ring, false);
>> }
>>
>> static void reset_active(struct i915_request *rq,
>> @@ -1081,8 +1081,8 @@ static void reset_active(struct i915_request *rq,
>> intel_ring_update_space(ce->ring);
>>
>> /* Scrub the context image to prevent replaying the previous batch */
>> - restore_default_state(ce, engine);
>> - __execlists_update_reg_state(ce, engine);
>> + lr_context_restore_default_state(ce, engine);
>> + lr_context_update_reg_state(ce, engine);
>>
>> /* We've switched away, so this should be a no-op, but intent matters */
>> ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
>> @@ -2378,7 +2378,7 @@ static void execlists_submit_request(struct i915_request *request)
>> spin_unlock_irqrestore(&engine->active.lock, flags);
>> }
>>
>> -static void __execlists_context_fini(struct intel_context *ce)
>> +static void lr_context_fini(struct intel_context *ce)
>
> execlists.
>
>> {
>> intel_ring_put(ce->ring);
>> i915_vma_put(ce->state);
>> @@ -2392,7 +2392,7 @@ static void execlists_context_destroy(struct kref *kref)
>> GEM_BUG_ON(intel_context_is_pinned(ce));
>>
>> if (ce->state)
>> - __execlists_context_fini(ce);
>> + lr_context_fini(ce);
>>
>> intel_context_fini(ce);
>> intel_context_free(ce);
>> @@ -2423,7 +2423,7 @@ check_redzone(const void *vaddr, const struct intel_engine_cs *engine)
>> engine->name);
>> }
>>
>> -static void execlists_context_unpin(struct intel_context *ce)
>> +static void intel_lr_context_unpin(struct intel_context *ce)
>
> execlists.
>
>> {
>> check_redzone((void *)ce->lrc_reg_state - LRC_STATE_PN * PAGE_SIZE,
>> ce->engine);
>> @@ -2433,8 +2433,8 @@ static void execlists_context_unpin(struct intel_context *ce)
>> }
>>
>> static void
>> -__execlists_update_reg_state(const struct intel_context *ce,
>> - const struct intel_engine_cs *engine)
>> +lr_context_update_reg_state(const struct intel_context *ce,
>> + const struct intel_engine_cs *engine)
>
> lrc.
>
>> {
>> struct intel_ring *ring = ce->ring;
>> u32 *regs = ce->lrc_reg_state;
>> @@ -2456,8 +2456,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
>> }
>>
>> static int
>> -__execlists_context_pin(struct intel_context *ce,
>> - struct intel_engine_cs *engine)
>> +lr_context_pin(struct intel_context *ce, struct intel_engine_cs *engine)
>
> execlists.
>
>> {
>> void *vaddr;
>> int ret;
>> @@ -2479,7 +2478,7 @@ __execlists_context_pin(struct intel_context *ce,
>>
>> ce->lrc_desc = lrc_descriptor(ce, engine);
>> ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>> - __execlists_update_reg_state(ce, engine);
>> + lr_context_update_reg_state(ce, engine);
>>
>> return 0;
>>
>> @@ -2491,12 +2490,12 @@ __execlists_context_pin(struct intel_context *ce,
>>
>> static int execlists_context_pin(struct intel_context *ce)
>> {
>> - return __execlists_context_pin(ce, ce->engine);
>> + return lr_context_pin(ce, ce->engine);
>> }
>>
>> static int execlists_context_alloc(struct intel_context *ce)
>> {
>> - return __execlists_context_alloc(ce, ce->engine);
>> + return lr_context_alloc(ce, ce->engine);
>> }
>>
>> static void execlists_context_reset(struct intel_context *ce)
>> @@ -2518,14 +2517,14 @@ static void execlists_context_reset(struct intel_context *ce)
>> * simplicity, we just zero everything out.
>> */
>> intel_ring_reset(ce->ring, 0);
>> - __execlists_update_reg_state(ce, ce->engine);
>> + lr_context_update_reg_state(ce, ce->engine);
>> }
>>
>> static const struct intel_context_ops execlists_context_ops = {
>> .alloc = execlists_context_alloc,
>>
>> .pin = execlists_context_pin,
>> - .unpin = execlists_context_unpin,
>> + .unpin = intel_lr_context_unpin,
>
> execlists.
>
>>
>> .enter = intel_context_enter_engine,
>> .exit = intel_context_exit_engine,
>> @@ -2912,7 +2911,33 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>> return ret;
>> }
>>
>> -static void enable_execlists(struct intel_engine_cs *engine)
>> +static int logical_ring_init(struct intel_engine_cs *engine)
>> +{
>> + int ret;
>> +
>> + ret = intel_engine_init_common(engine);
>> + if (ret)
>> + return ret;
>> +
>> + if (intel_init_workaround_bb(engine))
>> + /*
>> + * We continue even if we fail to initialize WA batch
>> + * because we only expect rare glitches but nothing
>> + * critical to prevent us from using GPU
>> + */
>> + DRM_ERROR("WA batch buffer initialization failed\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void logical_ring_destroy(struct intel_engine_cs *engine)
>> +{
>> + intel_engine_cleanup_common(engine);
>> + lrc_destroy_wa_ctx(engine);
>> + kfree(engine);
>
>> +}
>> +
>> +static void logical_ring_enable(struct intel_engine_cs *engine)
>> {
>> u32 mode;
>>
>> @@ -2946,7 +2971,7 @@ static bool unexpected_starting_state(struct intel_engine_cs *engine)
>> return unexpected;
>> }
>>
>> -static int execlists_resume(struct intel_engine_cs *engine)
>> +static int logical_ring_resume(struct intel_engine_cs *engine)
>
> execlists.
>
>> {
>> intel_engine_apply_workarounds(engine);
>> intel_engine_apply_whitelist(engine);
>> @@ -2961,7 +2986,7 @@ static int execlists_resume(struct intel_engine_cs *engine)
>> intel_engine_dump(engine, &p, NULL);
>> }
>>
>> - enable_execlists(engine);
>> + logical_ring_enable(engine);
>>
>> return 0;
>> }
>> @@ -3037,8 +3062,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
>> &execlists->csb_status[reset_value]);
>> }
>>
>> -static void __execlists_reset_reg_state(const struct intel_context *ce,
>> - const struct intel_engine_cs *engine)
>> +static void lr_context_reset_reg_state(const struct intel_context *ce,
>> + const struct intel_engine_cs *engine)
>
> lrc.
>
>> {
>> u32 *regs = ce->lrc_reg_state;
>> int x;
>> @@ -3131,14 +3156,14 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>> * to recreate its own state.
>> */
>> GEM_BUG_ON(!intel_context_is_pinned(ce));
>> - restore_default_state(ce, engine);
>> + lr_context_restore_default_state(ce, engine);
>>
>> out_replay:
>> GEM_TRACE("%s replay {head:%04x, tail:%04x}\n",
>> engine->name, ce->ring->head, ce->ring->tail);
>> intel_ring_update_space(ce->ring);
>> - __execlists_reset_reg_state(ce, engine);
>> - __execlists_update_reg_state(ce, engine);
>> + lr_context_reset_reg_state(ce, engine);
>> + lr_context_update_reg_state(ce, engine);
>> ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; /* paranoid: GPU was reset! */
>>
>> unwind:
>> @@ -3788,9 +3813,7 @@ static void execlists_destroy(struct intel_engine_cs *engine)
>> {
>> execlists_shutdown(engine);
>>
>> - intel_engine_cleanup_common(engine);
>> - lrc_destroy_wa_ctx(engine);
>> - kfree(engine);
>> + logical_ring_destroy(engine);
>> }
>>
>> static void
>> @@ -3799,7 +3822,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>> /* Default vfuncs which can be overriden by each engine. */
>>
>> engine->destroy = execlists_destroy;
>> - engine->resume = execlists_resume;
>> + engine->resume = logical_ring_resume;
>>
>> engine->reset.prepare = execlists_reset_prepare;
>> engine->reset.reset = execlists_reset;
>> @@ -3872,6 +3895,15 @@ static void rcs_submission_override(struct intel_engine_cs *engine)
>> }
>> }
>>
>> +static void logical_ring_setup(struct intel_engine_cs *engine)
>> +{
>> + logical_ring_default_vfuncs(engine);
>> + logical_ring_default_irqs(engine);
>> +
>> + if (engine->class == RENDER_CLASS)
>> + rcs_submission_override(engine);
>> +}
>> +
>> int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>> {
>> tasklet_init(&engine->execlists.tasklet,
>> @@ -3879,11 +3911,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>> timer_setup(&engine->execlists.timer, execlists_timeslice, 0);
>> timer_setup(&engine->execlists.preempt, execlists_preempt, 0);
>>
>> - logical_ring_default_vfuncs(engine);
>> - logical_ring_default_irqs(engine);
>> -
>> - if (engine->class == RENDER_CLASS)
>> - rcs_submission_override(engine);
>> + logical_ring_setup(engine);
>>
>> return 0;
>> }
>> @@ -3896,18 +3924,10 @@ int intel_execlists_submission_init(struct intel_engine_cs *engine)
>> u32 base = engine->mmio_base;
>> int ret;
>>
>> - ret = intel_engine_init_common(engine);
>> + ret = logical_ring_init(engine);
>> if (ret)
>> return ret;
>>
>> - if (intel_init_workaround_bb(engine))
>> - /*
>> - * We continue even if we fail to initialize WA batch
>> - * because we only expect rare glitches but nothing
>> - * critical to prevent us from using GPU
>> - */
>> - DRM_ERROR("WA batch buffer initialization failed\n");
>> -
>> if (HAS_LOGICAL_RING_ELSQ(i915)) {
>> execlists->submit_reg = uncore->regs +
>> i915_mmio_reg_offset(RING_EXECLIST_SQ_CONTENTS(base));
>> @@ -4033,11 +4053,11 @@ static struct i915_ppgtt *vm_alias(struct i915_address_space *vm)
>> return i915_vm_to_ppgtt(vm);
>> }
>>
>> -static void execlists_init_reg_state(u32 *regs,
>> - const struct intel_context *ce,
>> - const struct intel_engine_cs *engine,
>> - const struct intel_ring *ring,
>> - bool close)
>> +static void lr_context_init_reg_state(u32 *regs,
>> + const struct intel_context *ce,
>> + const struct intel_engine_cs *engine,
>> + const struct intel_ring *ring,
>> + bool close)
>> {
>> /*
>> * A context is actually a big batch buffer with several
>> @@ -4105,7 +4125,7 @@ populate_lr_context(struct intel_context *ce,
>> /* The second page of the context object contains some fields which must
>> * be set up prior to the first execution. */
>> regs = vaddr + LRC_STATE_PN * PAGE_SIZE;
>> - execlists_init_reg_state(regs, ce, engine, ring, inhibit);
>> + lr_context_init_reg_state(regs, ce, engine, ring, inhibit);
>> if (inhibit)
>> regs[CTX_CONTEXT_CONTROL] |=
>> _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
More information about the Intel-gfx
mailing list