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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 03/10/2023 17:41, Andi Shyti wrote:
> 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.

1)

I didn't specifically have an opinion on whether it should be a device 
info flag or in the guc struct or whatever. More knowledge of GuC state 
transitions than I have would be required for an informed decision. To 
me it just felt it raises the question if it can be simplified by 
splitting the invariant from variant and eliminating the redundant. All 
upper case macro name was also IMO wrong since we normally use those for 
static things.

I'll have a look in the latest version how it looks.

2)

Only for MTL - really? I didn't see the patch do that. Why is that?

Regards,

Tvrtko

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