Re: [Intel-gfx] [PATCH v9 04/17] drm/i915/pxp: allocate a vcs context for pxp usage

[Jani Nikula <jani.nikula@linux.intel.com>]

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?

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