[Intel-gfx] [PATCH v3 1/4] drm/i915: Define and use GuC and CTB TLB invalidation routines

[Andi Shyti]

Hi,

[...]

> > +static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> > +{
> > +	struct drm_i915_private *i915 = ggtt->vm.i915;
> > +	struct intel_gt *gt;
> > +
> > +	if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> > +		gen8_ggtt_invalidate(ggtt);
> > +
> > +	list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) {
> > +		if (INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&gt->uc.guc) &&
> > +		    intel_guc_is_ready(&gt->uc.guc)) {
> 
> The condition here expands to a relatively heavy one:
> 
> +#define INTEL_GUC_SUPPORTS_TLB_INVALIDATION(guc) \
> +	((intel_guc_ct_enabled(&(guc)->ct)) && \
> +	 (intel_guc_submission_is_used(guc)) && \
> +	 (GRAPHICS_VER(guc_to_gt((guc))->i915) >= 12))
> 
> 
> &&
> 
> static inline bool intel_guc_is_ready(struct intel_guc *guc)
> {
> 	return intel_guc_is_fw_running(guc) && intel_guc_ct_enabled(&guc->ct);
> }
> 
> intel_guc_ct_enabled is even duplicated.

Maybe this is a smaller set?

	if (INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&gt->uc.guc) &&
	    intel_guc_is_fw_running(&gt->uc.guc))

The last condition includes intel_guc_submission_is_used() from
the macro.

> Is there scope to consolidate the parts which are platform invariant, or even runtime invariant, or at least guaranteed not to transition back and forth but one way only?
> 
> In other words, if we know during init we will want it, mark it as a flag in intel_guc or somewhere, and then at runtime do only those conditions which can transition back and forth due driver flows.
> 
> I am not saying this is performance sensitive, but in terms of elegance, readability and self-documentation the proposed version looks a bit sub-optimal to me.

Are you suggesting some PCI flag? This is actually applying only
for MTL.

> > +			guc_ggtt_ct_invalidate(gt);
> > +		} else if (GRAPHICS_VER(i915) >= 12) {
> > +			intel_uncore_write(gt->uncore,
> > +					   GEN12_GUC_TLB_INV_CR,
> > +					   GEN12_GUC_TLB_INV_CR_INVALIDATE);
> > +		} else {
> > +			intel_uncore_write(gt->uncore,
> > +					   GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> > +		}

[...]

> > -		mmio_invalidate_full(gt);
> > +		if (INTEL_GUC_SUPPORTS_TLB_INVALIDATION(guc)) {
> > +			if (intel_guc_is_ready(guc))
> > +				intel_guc_invalidate_tlb_full(guc);
> > +		} else {
> > +			/*
> > +			 * Fall back to old path if GuC is disabled.
> > +			 * This is safe because GuC is not enabled and not writing to MMIO.
> > +			 */
> 
> It is safe for intel_guc_is_ready() transitioning from false to true during GuC init? No way for some path to start issuing invalidations as that is happening?
> 
> > +			mmio_invalidate_full(gt);
> > +		}

supernitpick: as we are at this, brackets are not required.

Andi