On Tue, May 11, 2021 at 10:01:28AM -0700, Matthew Brost wrote:
> On Tue, May 11, 2021 at 05:26:34PM +0200, Daniel Vetter wrote:
> > On Thu, May 06, 2021 at 12:13:57PM -0700, Matthew Brost wrote:
> > > Add lrc descriptor context lookup array which can resolve the
> > > intel_context from the lrc descriptor index. In addition to lookup, it
> > > can determine in the lrc descriptor context is currently registered with
> > > the GuC by checking if an entry for a descriptor index is present.
> > > Future patches in the series will make use of this array.
> > >
> > > Cc: John Harrison <john.c.harrison@xxxxxxxxx>
> > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 5 +++
> > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 32 +++++++++++++++++--
> > > 2 files changed, 35 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > index d84f37afb9d8..2eb6c497e43c 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > @@ -6,6 +6,8 @@
> > > #ifndef _INTEL_GUC_H_
> > > #define _INTEL_GUC_H_
> > >
> > > +#include "linux/xarray.h"
> > > +
> > > #include "intel_uncore.h"
> > > #include "intel_guc_fw.h"
> > > #include "intel_guc_fwif.h"
> > > @@ -47,6 +49,9 @@ struct intel_guc {
> > > struct i915_vma *lrc_desc_pool;
> > > void *lrc_desc_pool_vaddr;
> > >
> > > + /* guc_id to intel_context lookup */
> > > + struct xarray context_lookup;
> >
> > The current code sets a disastrous example, but for stuff like this it's
> > always good to explain the locking, and who's holding references and how
> > you're handling cycles. Since I guess the intel_context also holds the
> > guc_id alive somehow.
> >
>
> I think (?) I know what you mean by this comment. How about adding:
>
> 'If an entry in the the context_lookup is present, that means a context
> associated with the guc_id is registered with the GuC. We use this xarray as a
> lookup mechanism when the GuC communicate with the i915 about the context.'
So no idea how this works, but generally we put a "Protecte by
&struct.lock" or similar in here (so you get a nice link plus something
you can use as jump label in your ide too). Plus since intel_context has
some lifetime rules, explaining whether you're allowed to use the pointer
after you unlock, or whether you need to grab a reference or what exactly
is going on. Usually there's three options:
- No refcounting, you cannot access a pointer obtained through this after
you unluck.
- Weak reference, you upgrade to a full reference with
kref_get_unless_zero. If that fails it indicates a lookup failure, since
you raced with destruction. If it succeeds you can use the pointer after
unlock.
- Strong reference, you get your own reference that stays valid with
kref_get().
I'm just bringing this up because the current i915-gem code is full of
very tricky locking and lifetime rules, and explains roughly nothing of it
in the data structures. Minimally some hints about the locking/lifetime
rules of important structs should be there.
For locking rules it's good to double-down on them by adding
lockdep_assert_held to all relevant functions (where appropriate only
ofc).
What I generally don't think makes sense is to then also document the
locking in the kerneldoc for the functions. That tends to be one place too
many and ime just gets out of date and not useful at all.
> > Again holds for the entire series, where it makes sense (as in we don't
> > expect to rewrite the entire code anyway).
>
> Slightly out of order but one of the last patches in the series, 'Update GuC
> documentation' adds a big section of comments that attempts to clarify how all
> of this code works. I likely should add a section explaining the data structures
> as well.
Yeah that would be nice.
-Daniel
>
> Matt
>
> > -Daniel
> >
> > > +
> > > /* Control params for fw initialization */
> > > u32 params[GUC_CTL_MAX_DWORDS];
> > >
> > > 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 6acc1ef34f92..c2b6d27404b7 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -65,8 +65,6 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
> > > return rb_entry(rb, struct i915_priolist, node);
> > > }
> > >
> > > -/* Future patches will use this function */
> > > -__attribute__ ((unused))
> > > static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
> > > {
> > > struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;
> > > @@ -76,6 +74,15 @@ static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
> > > return &base[index];
> > > }
> > >
> > > +static inline struct intel_context *__get_context(struct intel_guc *guc, u32 id)
> > > +{
> > > + struct intel_context *ce = xa_load(&guc->context_lookup, id);
> > > +
> > > + GEM_BUG_ON(id >= GUC_MAX_LRC_DESCRIPTORS);
> > > +
> > > + return ce;
> > > +}
> > > +
> > > static int guc_lrc_desc_pool_create(struct intel_guc *guc)
> > > {
> > > u32 size;
> > > @@ -96,6 +103,25 @@ static void guc_lrc_desc_pool_destroy(struct intel_guc *guc)
> > > i915_vma_unpin_and_release(&guc->lrc_desc_pool, I915_VMA_RELEASE_MAP);
> > > }
> > >
> > > +static inline void reset_lrc_desc(struct intel_guc *guc, u32 id)
> > > +{
> > > + struct guc_lrc_desc *desc = __get_lrc_desc(guc, id);
> > > +
> > > + memset(desc, 0, sizeof(*desc));
> > > + xa_erase_irq(&guc->context_lookup, id);
> > > +}
> > > +
> > > +static inline bool lrc_desc_registered(struct intel_guc *guc, u32 id)
> > > +{
> > > + return __get_context(guc, id);
> > > +}
> > > +
> > > +static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id,
> > > + struct intel_context *ce)
> > > +{
> > > + xa_store_irq(&guc->context_lookup, id, ce, GFP_ATOMIC);
> > > +}
> > > +
> > > static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
> > > {
> > > /* Leaving stub as this function will be used in future patches */
> > > @@ -404,6 +430,8 @@ 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);
> > > +
> > > return 0;
> > > }
> > >
> > > --
> > > 2.28.0
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
