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.nik...@linux.intel.com> Sent: Friday, February 11, 2022 5:33:37 AM To: Cheng, Michael <michael.ch...@intel.com>; intel-...@lists.freedesktop.org <intel-...@lists.freedesktop.org> Cc: tvrtko.ursu...@linux.intel.com <tvrtko.ursu...@linux.intel.com>; Cheng, Michael <michael.ch...@intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanan...@intel.com>; Boyer, Wayne <wayne.bo...@intel.com>; Bowman, Casey G <casey.g.bow...@intel.com>; De Marchi, Lucas <lucas.demar...@intel.com>; dri-devel@lists.freedesktop.org <dri-devel@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.ch...@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.ch...@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