Re: [Intel-gfx] [PATCH v2 1/7] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

From: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>
To: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2 1/7] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT
Date: Mon, 17 Oct 2022 17:01:20 +0000	[thread overview]
Message-ID: <ca3929d15d31d18f5aa95b8612a7dc5e62932e08.camel@intel.com> (raw)
In-Reply-To: <a43a158e-8772-729d-2e2f-bcc8dba1a8d0@intel.com>

On Thu, 2022-10-13 at 13:48 -0700, Ceraolo Spurio, Daniele wrote:
> 
> On 10/5/2022 9:38 PM, Alan Previn wrote:
> > In preparation for future MTL-PXP feature support, PXP control
> > context should only valid on the correct gt tile. Depending on the
> > device-info this mat not necessarily be the root GT tile and
> > depends on which tile owns the VEBOX and KCR.
> > 
Alan:[snip]
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index b367cfff48d5..e61f6c5ed440 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -850,6 +850,10 @@ int intel_gt_probe_all(struct drm_i915_private *i915)
> >   	gt->name = "Primary GT";
> >   	gt->info.engine_mask = RUNTIME_INFO(i915)->platform_engine_mask;
> >   
> > +	/* device config determines which GT owns the global pxp-tee context */
> > +	if (VDBOX_MASK(gt) && !INTEL_INFO(i915)->has_nonroot_pxpgt)
> > +		gt->pxptee_iface_owner = true;
> > +
> 
> I'm not convinced that we need dedicated has_nonroot_pxpgt and 
> pxptee_iface_owner flags. MTL moves the GSC inside a GT and the owner of 
> PXP is the GT where the GSC engine resides. So we could have a checker like:
> 
> bool intel_pxp_supported(struct intel_gt *gt)
> {
>          /* we only support HECI PXP from the root GT */
>          if (HAS_HECI_PXP(gt->i915))
>                  return gt_is_root(gt);
> 
>          return HAS_ENGINE(gt, GSC);
> }
> 
> I'm aware that the GSC engine code is still not available, but we can 
> special case for MTL for now and then replace it when the GSC code lands:
> 
> bool intel_pxp_supported(struct intel_gt *gt)
> {
>          /* we only support HECI PXP from the root GT */
>          if (HAS_HECI_PXP(gt->i915))
>                  return gt_is_root(gt);
> 
>          /* TODO: replace with GSC check */
>          return IS_METEORLAKE(gt->i915) && !gt_is_root(gt);
> }
> 
> Then we can use intel_pxp_supported(gt) instead of 
> gt->pxptee_iface_owner and we can drop has_nonroot_pxpgt. Might also be 
> worth merging this with HAS_PXP for a unified check, but that can come 
> later.
> 
> Daniele

As per offline conversations, we know above combination may not work for the DG2 case, but i'll go ahead and re-rev this
after i look for another way to avoid creating another device info variable- i'll try to get a karnaugh map going to
ensure we have a good combination of existing device-config info that are reliable for all current and MTL usages else
we may need a new device-config after all (maybe a better named one if needed).