On Thu, Sep 16, 2021 at 02:06:56PM +0300, Jani Nikula wrote:
On Wed, 15 Sep 2021, Rodrigo Vivi rodrigo.vivi@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.ceraolospurio@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.
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