[Intel-gfx] [PATCH v1 1/1] drm/i915/gt: Move wbvind_on_all_cpus #define

[Cheng, Michael]

Thanks for the feedback! Another idea  I had that kind of align to what you are suggesting is adding a wrapper for wbinvd_on_all_cpus within drm_cache.h, then having it be included in the three files that calls this function. Thoughts on that?
________________________________
From: Jani Nikula <jani.nikula at linux.intel.com>
Sent: Friday, February 11, 2022 5:33:37 AM
To: Cheng, Michael <michael.cheng at intel.com>; intel-gfx at lists.freedesktop.org <intel-gfx at lists.freedesktop.org>
Cc: tvrtko.ursulin at linux.intel.com <tvrtko.ursulin at linux.intel.com>; Cheng, Michael <michael.cheng at intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanandan at intel.com>; Boyer, Wayne <wayne.boyer at intel.com>; Bowman, Casey G <casey.g.bowman at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; dri-devel at lists.freedesktop.org <dri-devel at lists.freedesktop.org>
Subject: Re: [PATCH v1 1/1] drm/i915/gt: Move wbvind_on_all_cpus #define

On Thu, 10 Feb 2022, Michael Cheng <michael.cheng at intel.com> wrote:
> Move wbvind_on_all_cpus to intel_gt.h. This will allow other wbind_on_all_cpus
> calls to benefit from the #define logic, and prevent compiler errors
> when building for non-x86 architectures.
>
> Signed-off-by: Michael Cheng <michael.cheng at intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_pm.c | 7 -------
>  drivers/gpu/drm/i915/gt/intel_gt.h     | 7 +++++++
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 6da68b38f00f..ff7340ae5ac8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -12,13 +12,6 @@
>
>  #include "i915_drv.h"
>
> -#if defined(CONFIG_X86)
> -#include <asm/smp.h>
> -#else
> -#define wbinvd_on_all_cpus() \
> -     pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
> -#endif
> -
>  void i915_gem_suspend(struct drm_i915_private *i915)
>  {
>        GEM_TRACE("%s\n", dev_name(i915->drm.dev));
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 2dad46c3eff2..149e8c13e402 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -10,6 +10,13 @@
>  #include "intel_gt_types.h"
>  #include "intel_reset.h"
>
> +#if defined(CONFIG_X86)
> +#include <asm/smp.h>
> +#else
> +#define wbinvd_on_all_cpus() \
> +         pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
> +#endif

Don't include headers from headers if it can be avoided.

gt/intel_gt.h is included from 79 files. We don't want all of them to
include <asm/smp.h> when only 3 files actually need
wbinvd_on_all_cpus().

Also, gt/intel_gt.h has absolutely nothing to do with
wbinvd_on_all_cpus() or asm/smp.h. Please don't use topical headers as
dumping grounds for random things.

Maybe a better idea is to add a local wrapper for wbinvd_on_all_cpus()
that does the right thing. Or add the above in a dedicated header.


BR,
Jani.


> +
>  struct drm_i915_private;
>  struct drm_printer;

--
Jani Nikula, Intel Open Source Graphics Center
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20220211/837e2f9d/attachment.htm>