[Intel-gfx] [RFC PATCH 43/97] drm/i915/guc: Add lrc descriptor context lookup array

[Matthew Brost]

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 at intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> >  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.'

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

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