On Thu, 2021-09-16 at 09:59 -0400, Rodrigo Vivi wrote: > On Thu, Sep 16, 2021 at 02:06:56PM +0300, Jani Nikula wrote: > > On Wed, 15 Sep 2021, Rodrigo Vivi <rodrigo.v...@intel.com> wrote: > > > On Wed, Sep 15, 2021 at 04:53:35PM +0300, Jani Nikula wrote: > > > > On Fri, 10 Sep 2021, Daniele Ceraolo Spurio > > > > <daniele.ceraolospu...@intel.com> wrote: > > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > > > b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > > > new file mode 100644 > > > > > index 000000000000..e87550fb9821 > > > > > --- /dev/null > > > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > > > @@ -0,0 +1,35 @@ > > > > > +/* SPDX-License-Identifier: MIT */ > > > > > +/* > > > > > + * Copyright(c) 2020, Intel Corporation. All rights reserved. > > > > > + */ > > > > > + > > > > > +#ifndef __INTEL_PXP_H__ > > > > > +#define __INTEL_PXP_H__ > > > > > + > > > > > +#include "gt/intel_gt_types.h" > > > > > > > > I've been trying to promote the idea that we don't include headers from > > > > headers, unless really necessary. It helps with build times by reducing > > > > rebuilds due to changes, but more importantly, it helps with coming up > > > > with abstractions that don't need to look at the guts of other > > > > components. > > > > > > > > The above include line pulls in 67 other includes. And it has to look at > > > > the same files a *lot* more times to know not to include them again. > > > > > > > > Maybe we need to start being more aggressive about hiding the > > > > abstractions behind the interfaces and headers. Static inlines are > > > > nothing but micro-optimizations that leak abstractions. Do we need > > > > these? > > > > > > Yeap, we have a few cases where this is already happening... > > > > > > Should we start using the container_of more directly and avoid the > > > a_to_b() > > > helpers? > > > > > > Should we create the a_to_b() helpers only inside .c files like we have > > > in a few other cases? > > > > > > In this pxp case here it looks like using the container of directly is > > > everywhere is better... is this your recommendation? > > > > Either that, or make it a non-inline function that's actually > > abstracted. Yes, it leads to a function call, but I'm really starting to > > wonder about the costs of a function call vs. maintainability across the > > board. > > Alan, could you please address this? With that addressed and CI happy > we will merge to the already created topic/pxp and do the proper > pull requests. > > if your change only touches this patch don't need to resend the whole > series but just in-reply-to. But if changes everything to container-of > please resend it entirely. > > Thanks, > Rodrigo. >
Yes - as you are aware, I have something ready to publish but it touches multiple patches (as function get introduced and modified across multiple patches). The approach i am taking is to make them function prototypes (except when CONFG_PXP is off as they are can be inline stubs). I just have more clean ups + testing to do. > > Static inlines considered harmful. > > > > > > BR, > > Jani. > > > > > > > > > +#include "intel_pxp_types.h" > > > > > + > > > > > +static inline struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp) > > > > > +{ > > > > > + return container_of(pxp, struct intel_gt, pxp); > > > > > +} > > > > > > > > I think it's questionable to claim the parameter is const, when you can > > > > do: > > > > > > > > const struct intel_pxp *const_pxp = something; > > > > struct intel_pxp *pxp = &pxp_to_gt(const_pxp)->pxp; > > > > > > > > BR, > > > > Jani. > > > > > > > > > + > > > > > +static inline bool intel_pxp_is_enabled(const struct intel_pxp *pxp) > > > > > +{ > > > > > + return pxp->ce; > > > > > +} > > > > > + > > > > > +#ifdef CONFIG_DRM_I915_PXP > > > > > +void intel_pxp_init(struct intel_pxp *pxp); > > > > > +void intel_pxp_fini(struct intel_pxp *pxp); > > > > > +#else > > > > > +static inline void intel_pxp_init(struct intel_pxp *pxp) > > > > > +{ > > > > > +} > > > > > + > > > > > +static inline void intel_pxp_fini(struct intel_pxp *pxp) > > > > > +{ > > > > > +} > > > > > +#endif > > > > > + > > > > > +#endif /* __INTEL_PXP_H__ */ > > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > > > > > b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > > > > > new file mode 100644 > > > > > index 000000000000..bd12c520e60a > > > > > --- /dev/null > > > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > > > > > @@ -0,0 +1,15 @@ > > > > > +/* SPDX-License-Identifier: MIT */ > > > > > +/* > > > > > + * Copyright(c) 2020, Intel Corporation. All rights reserved. > > > > > + */ > > > > > + > > > > > +#ifndef __INTEL_PXP_TYPES_H__ > > > > > +#define __INTEL_PXP_TYPES_H__ > > > > > + > > > > > +struct intel_context; > > > > > + > > > > > +struct intel_pxp { > > > > > + struct intel_context *ce; > > > > > +}; > > > > > + > > > > > +#endif /* __INTEL_PXP_TYPES_H__ */ > > > > > > > > -- > > > > Jani Nikula, Intel Open Source Graphics Center > > > > -- > > Jani Nikula, Intel Open Source Graphics Center