Re: [Intel-gfx] [PATCH] drm/i915/guc: Initialize GuC submission locks and queues early

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 15/02/2022 16:39, Ceraolo Spurio, Daniele wrote:
> On 2/15/2022 1:09 AM, Tvrtko Ursulin wrote:
> > On 15/02/2022 01:11, Daniele Ceraolo Spurio wrote:
> > > Move initialization of submission-related spinlock, lists and workers to
> > > init_early. This fixes an issue where if the GuC init fails we might
> > > still try to get the lock in the context cleanup code. Note that it is
> >
> > What's the worst case impact on non-debug builds aka is Fixes: required?
>
> There is no lock contention in this scenario and nothing is done within 
> the locked section (because submission is not initialized and all 
> contexts are marked as invalid), so no problems from the fact that the 
> lock doesn't work. Is that enough to avoid a fixes tag?

If the "lock doesn't work" is benign due no chance of touching 
uninitialised memory and then deadlocking then it's good. I just 
couldn't read that part from the commit message and did not have time to 
go dig into the code.

Regards,

Tvrtko

> Daniele
>
> > Regards,
> >
> > Tvrtko
> >
> > > safe to call the GuC context cleanup code even if the init failed
> > > because all contexts are initialized with an invalid GuC ID, which will
> > > cause the GuC side of the cleanup to be skipped, so it is easier to just
> > > make sure the variables are initialized than to special case the cleanup
> > > to handle the case when they're not.
> > >
> > > References: https://gitlab.freedesktop.org/drm/intel/-/issues/4932
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> > > ---
> > >   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 27 ++++++++++---------
> > >   1 file changed, 14 insertions(+), 13 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 b3a429a92c0da..2160da2c83cbf 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -1818,24 +1818,11 @@ int intel_guc_submission_init(struct 
> > > intel_guc *guc)
> > >        */
> > >       GEM_BUG_ON(!guc->lrc_desc_pool);
> > >   -    xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
> > > -
> > > -    spin_lock_init(&guc->submission_state.lock);
> > > -    INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
> > > -    ida_init(&guc->submission_state.guc_ids);
> > > - INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> > > -    INIT_WORK(&guc->submission_state.destroyed_worker,
> > > -          destroyed_worker_func);
> > > -    INIT_WORK(&guc->submission_state.reset_fail_worker,
> > > -          reset_fail_worker_func);
> > > -
> > >       guc->submission_state.guc_ids_bitmap =
> > >           bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
> > >       if (!guc->submission_state.guc_ids_bitmap)
> > >           return -ENOMEM;
> > >   -    spin_lock_init(&guc->timestamp.lock);
> > > -    INIT_DELAYED_WORK(&guc->timestamp.work, guc_timestamp_ping);
> > >       guc->timestamp.ping_delay = (POLL_TIME_CLKS / 
> > > gt->clock_frequency + 1) * HZ;
> > >       guc->timestamp.shift = gpm_timestamp_shift(gt);
> > >   @@ -3831,6 +3818,20 @@ static bool __guc_submission_selected(struct 
> > > intel_guc *guc)
> > >     void intel_guc_submission_init_early(struct intel_guc *guc)
> > >   {
> > > +    xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
> > > +
> > > +    spin_lock_init(&guc->submission_state.lock);
> > > +    INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
> > > +    ida_init(&guc->submission_state.guc_ids);
> > > + INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> > > +    INIT_WORK(&guc->submission_state.destroyed_worker,
> > > +          destroyed_worker_func);
> > > +    INIT_WORK(&guc->submission_state.reset_fail_worker,
> > > +          reset_fail_worker_func);
> > > +
> > > +    spin_lock_init(&guc->timestamp.lock);
> > > +    INIT_DELAYED_WORK(&guc->timestamp.work, guc_timestamp_ping);
> > > +
> > >       guc->submission_state.num_guc_ids = GUC_MAX_LRC_DESCRIPTORS;
> > >       guc->submission_supported = __guc_submission_supported(guc);
> > >       guc->submission_selected = __guc_submission_selected(guc);