Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Split guc_lrc_desc_pin apart

Ceraolo Spurio, Daniele
2022-02-23T01:04:33Z

On 2/17/2022 3:52 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The LRC descriptor pool is going away. Further, the function that was
> populating it was also doing a bunch of logic about the context
> registration sequence. So, split that code apart into separate state
> setup and try to register functions. Note that some of those 'try to
> register' code paths actually undo the state setup and leave it to be
> redone again later (with potentially different values). This is
> inefficient. The next patch will correct this.
>
> Also, move a comment about ignoring return values to the place where
> the return values are actually ignored.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> ---
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 45 ++++++++++++-------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index ad784e8068c7..0ab2d1a24bf6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -634,7 +634,7 @@ int intel_guc_wait_for_idle(struct intel_guc *guc, long timeout)
> true, timeout);
> }
>
> -static int guc_lrc_desc_pin(struct intel_context *ce, bool loop);
> +static int try_context_registration(struct intel_context *ce, bool loop);
>
> static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
> {
> @@ -932,7 +932,7 @@ static int guc_dequeue_one_context(struct intel_guc *guc)
>
> if (unlikely(!ctx_id_mapped(guc, ce->guc_id.id) &&
> !intel_context_is_banned(ce))) {
> - ret = guc_lrc_desc_pin(ce, false);
> + ret = try_context_registration(ce, false);
> if (unlikely(ret == -EPIPE)) {
> goto deadlk;
> } else if (ret == -EBUSY) {
> @@ -2237,17 +2237,13 @@ static void guc_context_policy_init(struct intel_engine_cs *engine,
> desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> }
>
> -static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> +static void prepare_context_registration_info(struct intel_context *ce)
> {
> struct intel_engine_cs *engine = ce->engine;
> - struct intel_runtime_pm *runtime_pm = engine->uncore->rpm;
> struct intel_guc *guc = &engine->gt->uc.guc;
> u32 desc_idx = ce->guc_id.id;
> struct guc_lrc_desc *desc;
> - bool context_registered;
> - intel_wakeref_t wakeref;
> struct intel_context *child;
> - int ret = 0;
>
> GEM_BUG_ON(!engine->mask);
> GEM_BUG_ON(!sched_state_is_init(ce));
> @@ -2259,8 +2255,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=
> i915_gem_object_is_lmem(ce->ring->vma->obj));
>
> - context_registered = ctx_id_mapped(guc, desc_idx);
> -
> clr_ctx_id_mapping(guc, desc_idx);
> set_ctx_id_mapping(guc, desc_idx, ce);
>
> @@ -2308,6 +2302,21 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
>
> clear_children_join_go_memory(ce);
> }
> +}
> +
> +static int try_context_registration(struct intel_context *ce, bool loop)
> +{
> + struct intel_engine_cs *engine = ce->engine;
> + struct intel_runtime_pm *runtime_pm = engine->uncore->rpm;
> + struct intel_guc *guc = &engine->gt->uc.guc;
> + intel_wakeref_t wakeref;
> + u32 desc_idx = ce->guc_id.id;
> + bool context_registered;
> + int ret = 0;
> +
> + context_registered = ctx_id_mapped(guc, desc_idx);
> +
> + prepare_context_registration_info(ce);
>
> /*
> * The context_lookup xarray is used to determine if the hardware
> @@ -3145,7 +3154,7 @@ static int guc_request_alloc(struct i915_request *rq)
> if (unlikely(ret < 0))
> return ret;
> if (context_needs_register(ce, !!ret)) {
> - ret = guc_lrc_desc_pin(ce, true);
> + ret = try_context_registration(ce, true);
> if (unlikely(ret)) { /* unwind */
> if (ret == -EPIPE) {
> disable_submission(guc);
> @@ -3633,9 +3642,17 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
> static inline void guc_kernel_context_pin(struct intel_guc *guc,
> struct intel_context *ce)
> {
> + /*
> + * Note: we purposefully do not check the returns below because
> + * the registration can only fail if a reset is just starting.
> + * This is called at the end of reset so presumably another reset
> + * isn't happening and even it did this code would be run again.
> + */
> +
> if (context_guc_id_invalid(ce))
> pin_guc_id(guc, ce);
> - guc_lrc_desc_pin(ce, true);
> +
> + try_context_registration(ce, true);
> }
>
> static inline void guc_init_lrc_mapping(struct intel_guc *guc)
> @@ -3653,13 +3670,7 @@ static inline void guc_init_lrc_mapping(struct intel_guc *guc)
> * Also, after a reset the of the GuC we want to make sure that the
> * information shared with GuC is properly reset. The kernel LRCs are
> * not attached to the gem_context, so they need to be added separately.
> - *
> - * Note: we purposefully do not check the return of guc_lrc_desc_pin,
> - * because that function can only fail if a reset is just starting. This
> - * is at the end of reset so presumably another reset isn't happening
> - * and even it did this code would be run again.
> */
> -
> for_each_engine(engine, gt, id) {
> struct intel_context *ce;
>