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?
+#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