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(>->uc.guc) &&
> > + intel_guc_is_ready(>->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(>->uc.guc) &&
intel_guc_is_fw_running(>->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
