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(>->pxp))
>
> for_each_gt already checks for gt not being NULL, no need to check again.
Got it - will fix this.
>
> Daniele
>
>
