Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Wed, 2022-12-07 at 13:46 +0000, Tvrtko Ursulin wrote:
> [Side note - your email client somehow manages to make a mess of line wraps so after a few replies it is super hard to follow the quote. Don't know how/what/why but I don't have this problem on the mailing list with other folks so at least I *think* it is something about your client or it's configuration.]
Alan: Yeah i apologize i've been struggling to find the wrong configuration - which is why i use a lot of [snip]s

Alan: [snip]
> Null check is fine, I was a bit bothered by the helpers operating on pxp. But lets put this aside for now and focus on the init path.
Alan: Okay we can come back to this on next rev if we think it deserves more scrutiny

Alan: [snip]
> > 
> IS_ENABLED is always optimized away, be it 1 or 0, since it is a compile time constant. So it doesn't increase the number of runtime conditionals.
Alan: Right - my mistake.

Alan: [snip]
> 
> > int intel_pxp_init(struct drm_i915_private *i915)
> > {
> > 	intel_gt *gt;
> > 	intel_pxp *pxp;
> > 	/*
> > 	 * NOTE: Get a suitable ctrl_gt before checking intel_pxp_is_supported
Alan: ...[snip]...
> > 	/*
> > 	 * If HuC is loaded by GSC but PXP is disabled, we can skip the init of
> > 	 * the full PXP session/object management and just init the tee channel.
> > 	 */
> > 	if (intel_pxp_is_supported(pxp)) {
> 
> How does the "full pxp init" branch handle the case of "have vdbox but huc fw is not available"? Presumably i915->pxp is not needed on that path too then? If so that could
> also be folded into pxp_find_suitable_ctrl_gt and then you wouldn't need the "else kfree" branch below.
Alan: No, on legacy platforms we do support PXP context/buffers without HuC loaded. So we can't roll that in. But i get the intent.
> > Essentially, can you cram more of the static checks into pxp_find_suitable_ctrl_gt and if that one returns a gt, then you definitely know i915->pxp needs to be allocated
> > and just decide on the flavour of initialisation?
> I am not entirely sure about has_pxp but would this work:
> 
> static struct intel_gt *pxp_find_suitable_ctrl_gt(struct drm_i915_private *i915)
> {
> ...
> 	if (!VDBOX_MASK(...))
> 		return NULL; /* Can't possibly need pxp */
Alan: For MTL, we will wrongly fail here if we check root gt (but must check root-gt for pre-MTL), so this check would need to go into the "for_each_gt" below to cover both.
> 	else if (!intel_uc_uses_huc(...))
> 		return NULL; /* Ditto? */
Alan: So we cant add this for the existing support legacy case as mentioned above.


Alan: [snip]

> int intel_pxp_init(struct drm_i915_private *i915)
> {
> ...
> 	if (IS_ENABLED(CONFIG_DRM_I915_PXP) && INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp)
> 		pxp_init_full(pxp);
> 	else if (intel_huc_is_loaded_by_gsc(...))
> 		intel_pxp_tee_component_init(pxp);
> 	else
> 		WARN(1, "oopsie");
Alan: i get your point, we want to delay the allocation until we know for sure so don't need to free it back. I'll think about this and get a rev-11 out with the existing
fixes and we can continue from there. I'm assuming keeping intel_pxp_init cleaner at the risk of rolling more of these quirky rules into helpers like find_required_gt is the
preference.

Alan: [snip]
> P.S. s/pxp_find_suitable_ctrl_gt/pxp_find_required_ctrl_gt/, to signify it may not required even if it exists?
Alan: sounds good.