This series moves the logic for wbvind_on_all_cpus to drm_cache. The logic changes a little here, if platform is not x86 then we throw out a warning for when wbvind_on_all_cpus is being called.
v2(Michael Cheng): Move and redo logic for wbvind_on_all_cpus. Also add drm_cache.h where the function is being called and remove uneeded header files.
v3(Michael Cheng): Updated commit messages.
Michael Cheng (3): drm_cache: Add logic for wbvind_on_all_cpus drm/i915/gem: Remove logic for wbinvd_on_all_cpus drm/i915/: Add drm_cache.h
drivers/gpu/drm/drm_cache.c | 2 -- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_pm.c | 7 +------ drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +- include/drm/drm_cache.h | 6 ++++++ 5 files changed, 9 insertions(+), 10 deletions(-)
Add logic for wbvind_on_all_cpus for non-x86 platforms.
v2(Michael Cheng): Change logic to if platform is not x86, then we add pr_warn for calling wbvind_on_all_cpus.
Signed-off-by: Michael Cheng michael.cheng@intel.com --- drivers/gpu/drm/drm_cache.c | 2 -- include/drm/drm_cache.h | 6 ++++++ 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c index 66597e411764..722e3931d68a 100644 --- a/drivers/gpu/drm/drm_cache.c +++ b/drivers/gpu/drm/drm_cache.c @@ -40,8 +40,6 @@ #define MEMCPY_BOUNCE_SIZE 128
#if defined(CONFIG_X86) -#include <asm/smp.h> - /* * clflushopt is an unordered instruction which needs fencing with mfence or * sfence to avoid ordering issues. For drm_clflush_page this fencing happens diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h index 22deb216b59c..24fcf6be1419 100644 --- a/include/drm/drm_cache.h +++ b/include/drm/drm_cache.h @@ -34,6 +34,12 @@ #define _DRM_CACHE_H_
#include <linux/scatterlist.h> +#include <asm/smp.h> + +#if !defined(CONFIG_x86) +#define wbinvd_on_all_cpus() \ + pr_warn("Missing cache flush in %s\n", __func__) +#endif
struct iosys_map;
Hi Michael,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm/drm-next drm-tip/drm-tip v5.17-rc5 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Move-define-wbvind_on... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: nios2-randconfig-r003-20220221 (https://download.01.org/0day-ci/archive/20220223/202202230507.uLh3qqTV-lkp@i...) compiler: nios2-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/3aaa40c95b16a78c9059a77536de70bb08ce... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Move-define-wbvind_on_all_cpus/20220223-012853 git checkout 3aaa40c95b16a78c9059a77536de70bb08ce05e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_cache.c:37:
include/drm/drm_cache.h:37:10: fatal error: asm/smp.h: No such file or directory
37 | #include <asm/smp.h> | ^~~~~~~~~~~ compilation terminated.
vim +37 include/drm/drm_cache.h
35 36 #include <linux/scatterlist.h>
37 #include <asm/smp.h>
38
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Michael,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm/drm-next drm-tip/drm-tip v5.17-rc5 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Move-define-wbvind_on... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: hexagon-randconfig-r041-20220221 (https://download.01.org/0day-ci/archive/20220223/202202230702.Ya7PkY7G-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/3aaa40c95b16a78c9059a77536de70bb08ce... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Move-define-wbvind_on_all_cpus/20220223-012853 git checkout 3aaa40c95b16a78c9059a77536de70bb08ce05e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_cache.c:37: In file included from include/drm/drm_cache.h:37:
arch/hexagon/include/asm/smp.h:13:9: warning: 'raw_smp_processor_id' macro redefined [-Wmacro-redefined]
#define raw_smp_processor_id() (current_thread_info()->cpu) ^ include/linux/smp.h:191:9: note: previous definition is here #define raw_smp_processor_id() 0 ^ 1 warning generated.
vim +/raw_smp_processor_id +13 arch/hexagon/include/asm/smp.h
43afdf50838634 Richard Kuo 2011-10-31 12 43afdf50838634 Richard Kuo 2011-10-31 @13 #define raw_smp_processor_id() (current_thread_info()->cpu) 43afdf50838634 Richard Kuo 2011-10-31 14
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Michael,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm/drm-next drm-tip/drm-tip v5.17-rc5 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Move-define-wbvind_on... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: openrisc-randconfig-r025-20220221 (https://download.01.org/0day-ci/archive/20220223/202202230701.l0i1vncs-lkp@i...) compiler: or1k-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/3aaa40c95b16a78c9059a77536de70bb08ce... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Move-define-wbvind_on_all_cpus/20220223-012853 git checkout 3aaa40c95b16a78c9059a77536de70bb08ce05e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/gpu/drm/vgem/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
In file included from include/drm/drm_cache.h:37, from drivers/gpu/drm/vgem/vgem_drv.h:33, from drivers/gpu/drm/vgem/vgem_fence.c:28:
arch/openrisc/include/asm/smp.h:15: warning: "raw_smp_processor_id" redefined
15 | #define raw_smp_processor_id() (current_thread_info()->cpu) | In file included from include/linux/lockdep.h:14, from include/linux/spinlock.h:62, from include/linux/vmalloc.h:5, from include/asm-generic/io.h:911, from arch/openrisc/include/asm/io.h:36, from include/linux/io.h:13, from include/linux/iosys-map.h:9, from include/linux/dma-buf.h:16, from drivers/gpu/drm/vgem/vgem_fence.c:23: include/linux/smp.h:191: note: this is the location of the previous definition 191 | #define raw_smp_processor_id() 0 |
vim +/raw_smp_processor_id +15 arch/openrisc/include/asm/smp.h
8e6d08e0a15e7d4 Stefan Kristiansson 2014-05-11 14 8e6d08e0a15e7d4 Stefan Kristiansson 2014-05-11 @15 #define raw_smp_processor_id() (current_thread_info()->cpu) 8e6d08e0a15e7d4 Stefan Kristiansson 2014-05-11 16 #define hard_smp_processor_id() mfspr(SPR_COREID) 8e6d08e0a15e7d4 Stefan Kristiansson 2014-05-11 17
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_pm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c index 00359ec9d58b..ee4783e4d135 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c @@ -13,12 +13,7 @@ #include "i915_driver.h" #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 +#include <drm/drm_cache.h>
void i915_gem_suspend(struct drm_i915_private *i915) {
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
Thanks,
/Thomas
Thanks for letting me know! I will try for a different solution.
On 2022-02-22 11:24 a.m., Thomas Hellström (Intel) wrote:
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
Thanks,
/Thomas
On Tue, Feb 22, 2022 at 08:24:31PM +0100, Thomas Hellström (Intel) wrote:
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
the warn is there to guarantee we don't forget a code path. However simply adding the warning is the real issue: we should rather guarantee we can't take that code path. I.e., as you said refactor the code to guarantee it works on discrete without that logic.
$ git grep wbinvd_on_all_cpus -- drivers/gpu/drm/ drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus())
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: * Currently we just do a heavy handed wbinvd_on_all_cpus() here since drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: wbinvd_on_all_cpus();
It looks like we actually go through this on other discrete graphics. Is this missing an update like s/IS_DG1/IS_DGFX/? Or should we be doing something else?
drivers/gpu/drm/i915/gem/i915_gem_pm.c:#define wbinvd_on_all_cpus() \ drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Those are for suspend. Revert ac05a22cd07a ("drm/i915/gem: Almagamate clflushes on suspend") or extract that part to a helper function and implement it differently for arches != x86?
drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Probably take a similar approach to the suspend case?
drivers/gpu/drm/i915/gt/intel_ggtt.c: wbinvd_on_all_cpus();
This one comes from 64b95df91f44 ("drm/i915: Assume exclusive access to objects inside resume") Shouldn't that be doing the invalidate if the write domain is I915_GEM_DOMAIN_CPU
In the end I think the warning would be ok if it was the cherry on top, to guarantee we don't take those paths. We should probably have a warn_once() to avoid spamming the console. But we also have to rework the code to guarantee we are the only ones who may eventually get that warning, and not the end user.
Lucas De Marchi
Thanks,
/Thomas
On 2022-03-08 10:58 a.m., Lucas De Marchi wrote:
On Tue, Feb 22, 2022 at 08:24:31PM +0100, Thomas Hellström (Intel) wrote:
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
the warn is there to guarantee we don't forget a code path. However simply adding the warning is the real issue: we should rather guarantee we can't take that code path. I.e., as you said refactor the code to guarantee it works on discrete without that logic.
$ git grep wbinvd_on_all_cpus -- drivers/gpu/drm/ drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus())
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: * Currently we just do a heavy handed wbinvd_on_all_cpus() here since drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: wbinvd_on_all_cpus();
It looks like we actually go through this on other discrete graphics. Is this missing an update like s/IS_DG1/IS_DGFX/? Or should we be doing something else?
drivers/gpu/drm/i915/gem/i915_gem_pm.c:#define wbinvd_on_all_cpus() \ drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Those are for suspend. Revert ac05a22cd07a ("drm/i915/gem: Almagamate clflushes on suspend") or extract that part to a helper function and implement it differently for arches != x86?
drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Probably take a similar approach to the suspend case?
drivers/gpu/drm/i915/gt/intel_ggtt.c: wbinvd_on_all_cpus();
For a helper function, I have a #define for all non x86 architecture that gives a warn on [1] within drm_cache.h Or would it be better to implement a helper function instead?
[1]. https://patchwork.freedesktop.org/patch/475750/?series=99991&rev=5
This one comes from 64b95df91f44 ("drm/i915: Assume exclusive access to objects inside resume") Shouldn't that be doing the invalidate if the write domain is I915_GEM_DOMAIN_CPU
In the end I think the warning would be ok if it was the cherry on top, to guarantee we don't take those paths. We should probably have a warn_once() to avoid spamming the console. But we also have to rework the code to guarantee we are the only ones who may eventually get that warning, and not the end user.
Could we first add the helper function/#define for now, and rework the code in a different patch series?
Lucas De Marchi
Thanks,
/Thomas
+Daniel for additional feedback!
On 2022-03-14 4:06 p.m., Michael Cheng wrote:
On 2022-03-08 10:58 a.m., Lucas De Marchi wrote:
On Tue, Feb 22, 2022 at 08:24:31PM +0100, Thomas Hellström (Intel) wrote:
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
the warn is there to guarantee we don't forget a code path. However simply adding the warning is the real issue: we should rather guarantee we can't take that code path. I.e., as you said refactor the code to guarantee it works on discrete without that logic.
$ git grep wbinvd_on_all_cpus -- drivers/gpu/drm/ drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus())
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: * Currently we just do a heavy handed wbinvd_on_all_cpus() here since drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: wbinvd_on_all_cpus();
It looks like we actually go through this on other discrete graphics. Is this missing an update like s/IS_DG1/IS_DGFX/? Or should we be doing something else?
drivers/gpu/drm/i915/gem/i915_gem_pm.c:#define wbinvd_on_all_cpus() \ drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Those are for suspend. Revert ac05a22cd07a ("drm/i915/gem: Almagamate clflushes on suspend") or extract that part to a helper function and implement it differently for arches != x86?
drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Probably take a similar approach to the suspend case?
drivers/gpu/drm/i915/gt/intel_ggtt.c: wbinvd_on_all_cpus();
For a helper function, I have a #define for all non x86 architecture that gives a warn on [1] within drm_cache.h Or would it be better to implement a helper function instead?
[1]. https://patchwork.freedesktop.org/patch/475750/?series=99991&rev=5
This one comes from 64b95df91f44 ("drm/i915: Assume exclusive access to objects inside resume") Shouldn't that be doing the invalidate if the write domain is I915_GEM_DOMAIN_CPU
In the end I think the warning would be ok if it was the cherry on top, to guarantee we don't take those paths. We should probably have a warn_once() to avoid spamming the console. But we also have to rework the code to guarantee we are the only ones who may eventually get that warning, and not the end user.
Could we first add the helper function/#define for now, and rework the code in a different patch series?
Lucas De Marchi
Thanks,
/Thomas
Hi, Michael, others.
This is the response from Linus last time we copied that already pre-existing wbinvd_on_all_cpus() macro to another place in the driver:
https://lists.freedesktop.org/archives/dri-devel/2021-November/330928.html
My first interpretation of this is that even if there currently are similar patterns in drm_cache.c, we shouldn't introduce more, encouraging other drivers to use incoherent IO.
Other than that I think we should move whatever wbinvds we can over to the ranged versions, unless that is proven to be a performance drop.
Finally for any wbinvds left in our driver, ensure that they are never executed for any gpu where we provide full coherency. That is all discrete gpus (and to be discussed integrated gpus moving forward).
Might be that drm maintainers want to chime in here with other views.
Thanks,
Thomas
On 3/15/22 17:59, Michael Cheng wrote:
+Daniel for additional feedback!
On 2022-03-14 4:06 p.m., Michael Cheng wrote:
On 2022-03-08 10:58 a.m., Lucas De Marchi wrote:
On Tue, Feb 22, 2022 at 08:24:31PM +0100, Thomas Hellström (Intel) wrote:
Hi, Michael,
On 2/22/22 18:26, Michael Cheng wrote:
This patch removes logic for wbinvd_on_all_cpus and brings in drm_cache.h. This header has the logic that outputs a warning when wbinvd_on_all_cpus when its being used on a non-x86 platform.
Signed-off-by: Michael Cheng michael.cheng@intel.com
Linus has been pretty clear that he won't accept patches that add macros that works on one arch and warns on others anymore in i915 and I figure even less so in drm code.
So we shouldn't try to move this out to drm. Instead we should restrict the wbinvd() inside our driver to integrated and X86 only. For discrete on all architectures we should be coherent and hence not be needing wbinvd().
the warn is there to guarantee we don't forget a code path. However simply adding the warning is the real issue: we should rather guarantee we can't take that code path. I.e., as you said refactor the code to guarantee it works on discrete without that logic.
$ git grep wbinvd_on_all_cpus -- drivers/gpu/drm/ drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus()) drivers/gpu/drm/drm_cache.c: if (wbinvd_on_all_cpus())
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: * Currently we just do a heavy handed wbinvd_on_all_cpus() here since drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: wbinvd_on_all_cpus();
It looks like we actually go through this on other discrete graphics. Is this missing an update like s/IS_DG1/IS_DGFX/? Or should we be doing something else?
drivers/gpu/drm/i915/gem/i915_gem_pm.c:#define wbinvd_on_all_cpus() \ drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Those are for suspend. Revert ac05a22cd07a ("drm/i915/gem: Almagamate clflushes on suspend") or extract that part to a helper function and implement it differently for arches != x86?
drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
Probably take a similar approach to the suspend case?
drivers/gpu/drm/i915/gt/intel_ggtt.c: wbinvd_on_all_cpus();
For a helper function, I have a #define for all non x86 architecture that gives a warn on [1] within drm_cache.h Or would it be better to implement a helper function instead?
[1]. https://patchwork.freedesktop.org/patch/475750/?series=99991&rev=5
This one comes from 64b95df91f44 ("drm/i915: Assume exclusive access to objects inside resume") Shouldn't that be doing the invalidate if the write domain is I915_GEM_DOMAIN_CPU
In the end I think the warning would be ok if it was the cherry on top, to guarantee we don't take those paths. We should probably have a warn_once() to avoid spamming the console. But we also have to rework the code to guarantee we are the only ones who may eventually get that warning, and not the end user.
Could we first add the helper function/#define for now, and rework the code in a different patch series?
Lucas De Marchi
Thanks,
/Thomas
Add drm_cache.h to additionals files that calls wbinvd_on_all_cpus and remove un-needed header files. This will prevent compile errors when building for non-x86 platforms, as well as output a warning when wbinvd_on_all_cpus is being called incorrectly.
Signed-off-by: Michael Cheng michael.cheng@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +- drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 13917231ae81..edb0ebbb089c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -9,7 +9,7 @@ #include <linux/dma-resv.h> #include <linux/module.h>
-#include <asm/smp.h> +#include <drm/drm_cache.h>
#include "gem/i915_gem_dmabuf.h" #include "i915_drv.h" diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 8850d4e0f9cc..dac62e3ba142 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -7,10 +7,10 @@ #include <linux/stop_machine.h>
#include <asm/set_memory.h> -#include <asm/smp.h>
#include <drm/i915_drm.h> #include <drm/intel-gtt.h> +#include <drm/drm_cache.h>
#include "gem/i915_gem_lmem.h"
dri-devel@lists.freedesktop.org
-
kernel test robot -
Lucas De Marchi -
Michael Cheng -
Thomas Hellström (Intel)