This series moves the #define wbvind_on_all_cpus logic to intel_gt.h. This way all calls to wbvind_on_all_cpus benefit from the logic, and fixes compile errors on non-x86 platforms.
Michael Cheng (1): drm/i915/gt: Move wbvind_on_all_cpus #define
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(-)
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@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 + struct drm_i915_private; struct drm_printer;
On Thu, 10 Feb 2022, Michael Cheng michael.cheng@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@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;
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@linux.intel.com Sent: Friday, February 11, 2022 5:33:37 AM To: Cheng, Michael michael.cheng@intel.com; intel-gfx@lists.freedesktop.org intel-gfx@lists.freedesktop.org Cc: tvrtko.ursulin@linux.intel.com tvrtko.ursulin@linux.intel.com; Cheng, Michael michael.cheng@intel.com; Vivekanandan, Balasubramani balasubramani.vivekanandan@intel.com; Boyer, Wayne wayne.boyer@intel.com; Bowman, Casey G casey.g.bowman@intel.com; De Marchi, Lucas lucas.demarchi@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.cheng@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@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
On 11/02/2022 13:33, Jani Nikula wrote:
On Thu, 10 Feb 2022, Michael Cheng michael.cheng@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@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.
+1, noting the naming angle:
WBINVD — Write Back and Invalidate Cache
Is an x86 instruction. Also interesting comment:
* XXX: Consider doing a vmap flush or something, where possible. * Currently we just do a heavy handed wbinvd_on_all_cpus() here since * the underlying sg_table might not even point to struct pages, so we * can't just call drm_clflush_sg or similar, like we do elsewhere in * the driver. */ if (i915_gem_object_can_bypass_llc(obj) || (!HAS_LLC(i915) && !IS_DG1(i915))) wbinvd_on_all_cpus();
The two together to me sound like the fix is to either find an equivalent existing platform agnostic API in the kernel, or if it does not exist create one and name it generically.
Either per-platform i915, if we go for my proposal, or I guess drm_cache.c if we don't.
Regards,
Tvrtko
dri-devel@lists.freedesktop.org
-
Cheng, Michael -
Jani Nikula -
Michael Cheng
-
Tvrtko Ursulin