Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

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

On Mon, 2022-11-14 at 20:11 -0800, Ceraolo Spurio, Daniele wrote:
> 
> On 10/21/2022 10:39 AM, Alan Previn wrote:
> > @@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp)
> >   	return false;
> >   }
> >   
> > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp)
> 
> I'd rename this to intel_pxp_is_initialized, that way we don't have 2 
> almost identically named checkers that mean different things (and also 
> avoid the gtpxp prefix).
> 
I disagree - one is a wrapper around the other so i rather DO insist we have the same function-action name in the middle
with a different part of the function name being the qualifier for whether its a global level checker or a gt-level
checker. Perhaps as per last review reply, we can do "intel_pxp_is_enabled" as wrapper around "intel_gt_has_pxp_enabled"
- i think the "enabled" part SHOULD be consistent since one is a wrapper around the other else a new reader will even
more baffled about why "enabled" is different from "initialized" despite trying to get to the same anchor point, "pxp-
>ce".


> >   {
> >   	return pxp->ce;
> >   }
> >   
> > +static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915)
> 
> nit: why the "_" prefix? we usually don't use it for x_to_y functions. 
> Not a blocker.
I was assuming internal static functions dont have to obey such rules - i like to use _foo for all local static
functions (so that when reading from a caller's code, i know its a local static). Again, just another naming convention
preference thing that i feel seems to be happening here and there in the driver code base but not consistent across all
files / function types. Since its a nit, i won't change this.

> 
> > +{
> > +	struct intel_gt *gt = NULL;
> > +	int i = 0;
> > +
> > +	for_each_gt(gt, i915, i) {
> > +		/* There can be only one GT that supports PXP */
> 
> 
> 
> > +		if (gt && intel_gtpxp_is_supported(&gt->pxp))
> 
> for_each_gt already checks for gt not being NULL, no need to check again.
Got it - will fix this.

> 
> Daniele
> 
>