Re: [Intel-gfx] [PATCH] drm/i915: Flip guc_id allocation partition

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jan 13, 2022 at 03:18:14PM +0100, Michal Wajdeczko wrote:
> 
> 
> On 13.01.2022 00:26, Matthew Brost wrote:
> > On Thu, Jan 13, 2022 at 12:21:17AM +0100, Michal Wajdeczko wrote:
> >> On 11.01.2022 17:30, Matthew Brost wrote:
> 
> ...
> 
> >>> @@ -1863,6 +1861,33 @@ static void guc_submit_request(struct i915_request *rq)
> >>>  	spin_unlock_irqrestore(&sched_engine->lock, flags);
> >>>  }
> >>>  
> >>> +static int new_mlrc_guc_id(struct intel_guc *guc, struct intel_context *ce)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	GEM_BUG_ON(!intel_context_is_parent(ce));
> >>> +	GEM_BUG_ON(!guc->submission_state.guc_ids_bitmap);
> >>> +
> >>> +	ret =  bitmap_find_free_region(guc->submission_state.guc_ids_bitmap,
> >>> +				       NUMBER_MULTI_LRC_GUC_ID(guc),
> >>> +				       order_base_2(ce->parallel.number_children
> >>> +						    + 1));
> >>
> >> btw, is there any requirement (GuC ABI ?) that allocated ids need
> >> to be allocated with power of 2 alignment ? I don't think that we
> >> must optimize that hard and in some cases waste extra ids (as we might
> >> be limited on some configs)
> >>
> > 
> > No pow2 requirement in GuC ABI, bitmaps only work on pow2 alignment and
> > didn't optmize this.
> >
> 
> there is a slower variant of "find" function:
> 
> bitmap_find_next_zero_area - find a contiguous aligned zero area
> 
> that does not have this limitation
> 

Ah, wasn't aware of this. If this becomes an issue (running of multi-lrc
ids) for customers I suppose this is the first thing we can do to try to
address this. For now, I think we leave it as is.

> ..
> 
> 
> >>> @@ -1989,6 +2008,14 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce)
> >>>  
> >>>  	GEM_BUG_ON(atomic_read(&ce->guc_id.ref));
> >>>  
> >>> +	if (unlikely(intel_context_is_parent(ce) &&
> >>> +		     !guc->submission_state.guc_ids_bitmap)) {
> >>> +		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;
> >>> +	}
> >>
> >> maybe move this chunk to new_mlrc_guc_id() ?
> >> or we can't due to the spin_lock below ?
> >> but then how do you protect guc_ids_bitmap pointer itself ?
> >>
> > 
> > Can't use GFP_KERNEL inside a spin lock...
> > 
> 
> ok, but what if there will be two or more parallel calls to pin_guc_id()
> with all being first parent context? each will see NULL guc_ids_bitmap..
> or there is another layer of synchronization?
> 

Good catch. Yes, it techincally possible two multi-lrc contexts to try
to allocate at the same time. I guess I should just do this at driver
load time + allocate the maximum number of multi-lrc ids and possibly
waste a bit of memory on a PF or VF.

Matt

> -Michal