Re: [Intel-gfx] [PATCH 13/46] drm/i915: Add logical engine mapping

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Aug 09, 2021 at 06:28:58PM +0000, Matthew Brost wrote:
> On Mon, Aug 09, 2021 at 04:28:04PM +0200, Daniel Vetter wrote:
> > On Tue, Aug 03, 2021 at 03:29:10PM -0700, Matthew Brost wrote:
> > > Add logical engine mapping. This is required for split-frame, as
> > > workloads need to be placed on engines in a logically contiguous manner.
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_engine_cs.c     | 60 ++++++++++++++++---
> > >  drivers/gpu/drm/i915/gt/intel_engine_types.h  |  1 +
> > >  .../drm/i915/gt/intel_execlists_submission.c  |  1 +
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    |  2 +-
> > >  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 21 +------
> > >  5 files changed, 56 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index 0d9105a31d84..4d790f9a65dd 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -290,7 +290,8 @@ static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir)
> > >  	GEM_DEBUG_WARN_ON(iir);
> > >  }
> > >  
> > > -static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> > > +static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
> > > +			      u8 logical_instance)
> > >  {
> > >  	const struct engine_info *info = &intel_engines[id];
> > >  	struct drm_i915_private *i915 = gt->i915;
> > > @@ -334,6 +335,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> > >  
> > >  	engine->class = info->class;
> > >  	engine->instance = info->instance;
> > > +	engine->logical_mask = BIT(logical_instance);
> > >  	__sprint_engine_name(engine);
> > >  
> > >  	engine->props.heartbeat_interval_ms =
> > > @@ -572,6 +574,37 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
> > >  	return info->engine_mask;
> > >  }
> > >  
> > > +static void populate_logical_ids(struct intel_gt *gt, u8 *logical_ids,
> > > +				 u8 class, const u8 *map, u8 num_instances)
> > > +{
> > > +	int i, j;
> > > +	u8 current_logical_id = 0;
> > > +
> > > +	for (j = 0; j < num_instances; ++j) {
> > > +		for (i = 0; i < ARRAY_SIZE(intel_engines); ++i) {
> > > +			if (!HAS_ENGINE(gt, i) ||
> > > +			    intel_engines[i].class != class)
> > > +				continue;
> > > +
> > > +			if (intel_engines[i].instance == map[j]) {
> > > +				logical_ids[intel_engines[i].instance] =
> > > +					current_logical_id++;
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void setup_logical_ids(struct intel_gt *gt, u8 *logical_ids, u8 class)
> > > +{
> > > +	int i;
> > > +	u8 map[MAX_ENGINE_INSTANCE + 1];
> > > +
> > > +	for (i = 0; i < MAX_ENGINE_INSTANCE + 1; ++i)
> > > +		map[i] = i;
> > > +	populate_logical_ids(gt, logical_ids, class, map, ARRAY_SIZE(map));
> > > +}
> > > +
> > >  /**
> > >   * intel_engines_init_mmio() - allocate and prepare the Engine Command Streamers
> > >   * @gt: pointer to struct intel_gt
> > > @@ -583,7 +616,8 @@ int intel_engines_init_mmio(struct intel_gt *gt)
> > >  	struct drm_i915_private *i915 = gt->i915;
> > >  	const unsigned int engine_mask = init_engine_mask(gt);
> > >  	unsigned int mask = 0;
> > > -	unsigned int i;
> > > +	unsigned int i, class;
> > > +	u8 logical_ids[MAX_ENGINE_INSTANCE + 1];
> > >  	int err;
> > >  
> > >  	drm_WARN_ON(&i915->drm, engine_mask == 0);
> > > @@ -593,15 +627,23 @@ int intel_engines_init_mmio(struct intel_gt *gt)
> > >  	if (i915_inject_probe_failure(i915))
> > >  		return -ENODEV;
> > >  
> > > -	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
> > > -		if (!HAS_ENGINE(gt, i))
> > > -			continue;
> > > +	for (class = 0; class < MAX_ENGINE_CLASS + 1; ++class) {
> > > +		setup_logical_ids(gt, logical_ids, class);
> > >  
> > > -		err = intel_engine_setup(gt, i);
> > > -		if (err)
> > > -			goto cleanup;
> > > +		for (i = 0; i < ARRAY_SIZE(intel_engines); ++i) {
> > > +			u8 instance = intel_engines[i].instance;
> > > +
> > > +			if (intel_engines[i].class != class ||
> > > +			    !HAS_ENGINE(gt, i))
> > > +				continue;
> > >  
> > > -		mask |= BIT(i);
> > > +			err = intel_engine_setup(gt, i,
> > > +						 logical_ids[instance]);
> > > +			if (err)
> > > +				goto cleanup;
> > > +
> > > +			mask |= BIT(i);
> > > +		}
> > >  	}
> > >  
> > >  	/*
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > index ed91bcff20eb..85e5c9a9e502 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -266,6 +266,7 @@ struct intel_engine_cs {
> > >  	unsigned int guc_id;
> > >  
> > >  	intel_engine_mask_t mask;
> > > +	intel_engine_mask_t logical_mask;
> > 
> > Kerneldoc at least for new stuff. Bonus points if you get the
> > struct/header file up to speed (with dummy/fixme comments if need be) so
> 
> Sure can add Kerneldoc for new variables. Def don't have time to get all
> structs kerneldoc up to speed at moment as by backlog is about a mile
> long. Perhaps after we get all of GuC submission upstream I can take
> sometime to go through all the structures and update the DoC.

The idea isn't to add comments that are actually meaningful to all of
them, but just enough to be able to pull in the header without warnings.
Once you have that then any new addition will cause a warning in the doc
build, which CI iirc checks. And that's pretty good baseline to have, and
hence why I think it'd be good to quickly go through the motions to add
these.

Really fixing this is likely a few years of work across all the structs,
because in many cases the locking/coherency design is somewhere between
very tricky and outright broken. Doing that in one go makes no sense.
-Daniel

> 
> Matt
> 
> > we can include it into our overall html hierarchy).
> > -Daniel
> > 
> > >  
> > >  	u8 class;
> > >  	u8 instance;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > index de5f9c86b9a4..baa1797af1c8 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > @@ -3879,6 +3879,7 @@ execlists_create_virtual(struct intel_engine_cs **siblings, unsigned int count)
> > >  
> > >  		ve->siblings[ve->num_siblings++] = sibling;
> > >  		ve->base.mask |= sibling->mask;
> > > +		ve->base.logical_mask |= sibling->logical_mask;
> > >  
> > >  		/*
> > >  		 * All physical engines must be compatible for their emission
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > index 6926919bcac6..9f5f43a16182 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > @@ -176,7 +176,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
> > >  	for_each_engine(engine, gt, id) {
> > >  		u8 guc_class = engine_class_to_guc_class(engine->class);
> > >  
> > > -		system_info->mapping_table[guc_class][engine->instance] =
> > > +		system_info->mapping_table[guc_class][ilog2(engine->logical_mask)] =
> > >  			engine->instance;
> > >  	}
> > >  }
> > > 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 310116f40509..dec757d319a2 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -1795,23 +1795,6 @@ static int deregister_context(struct intel_context *ce, u32 guc_id, bool loop)
> > >  	return __guc_action_deregister_context(guc, guc_id, loop);
> > >  }
> > >  
> > > -static intel_engine_mask_t adjust_engine_mask(u8 class, intel_engine_mask_t mask)
> > > -{
> > > -	switch (class) {
> > > -	case RENDER_CLASS:
> > > -		return mask >> RCS0;
> > > -	case VIDEO_ENHANCEMENT_CLASS:
> > > -		return mask >> VECS0;
> > > -	case VIDEO_DECODE_CLASS:
> > > -		return mask >> VCS0;
> > > -	case COPY_ENGINE_CLASS:
> > > -		return mask >> BCS0;
> > > -	default:
> > > -		MISSING_CASE(class);
> > > -		return 0;
> > > -	}
> > > -}
> > > -
> > >  static void guc_context_policy_init(struct intel_engine_cs *engine,
> > >  				    struct guc_lrc_desc *desc)
> > >  {
> > > @@ -1952,8 +1935,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> > >  
> > >  	desc = __get_lrc_desc(guc, ce->guc_lrcd_reg_idx);
> > >  	desc->engine_class = engine_class_to_guc_class(engine->class);
> > > -	desc->engine_submit_mask = adjust_engine_mask(engine->class,
> > > -						      engine->mask);
> > > +	desc->engine_submit_mask = engine->logical_mask;
> > >  	desc->hw_context_desc = ce->lrc.lrca;
> > >  	ce->guc_prio = map_i915_prio_to_guc_prio(prio);
> > >  	desc->priority = ce->guc_prio;
> > > @@ -3978,6 +3960,7 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count)
> > >  		}
> > >  
> > >  		ve->base.mask |= sibling->mask;
> > > +		ve->base.logical_mask |= sibling->logical_mask;
> > >  
> > >  		if (n != 0 && ve->base.class != sibling->class) {
> > >  			DRM_DEBUG("invalid mixing of engine class, sibling %d, already %d\n",
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch