[Intel-gfx] [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()

[Intel-gfx] [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Hi list,

Sorry for a long delay since v1 [1]. This patchset is based on 197b6b6
(Linux 6.3-rc4).

Welcome and thanks for your review and comments!


# Purpose of this patchset

The purpose of this pacthset is to replace all uses of kmap_atomic() in
i915 with kmap_local_page() because the use of kmap_atomic() is being
deprecated in favor of kmap_local_page()[1]. And 92b64bd (mm/highmem:
add notes about conversions from kmap{,_atomic}()) has declared the
deprecation of kmap_atomic().


# Motivation for deprecating kmap_atomic() and using kmap_local_page()

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.


# Patch summary

Patch 1, 4-6 and 8-9 replace kamp_atomic()/kunmap_atomic() with
        kmap_local_page()/kunmap_local() directly. With thses local
        mappings, the page faults and preemption are allowed.

Patch 2 and 7 use memcpy_from_page() and memcpy_to_page() to replace
        kamp_atomic()/kunmap_atomic(). These two variants of memcpy()
        are based on the local mapping, so page faults and preemption
        are also allowed in these two interfaces.

Patch 3 replaces kamp_atomic()/kunmap_atomic() with kmap_local_page()/
        kunmap_local() and also diable page fault since the for special
        handling (pls see the commit message).


# Changes since v1

* Dropped hot plug related description in commit message since it has
  nothing to do with kmap_local_page().
* Emphasized the motivation for using kmap_local_page() in commit
  message.
* Rebased patch 1 on f47e630 (drm/i915/gem: Typecheck page lookups) to
  keep the "idx" variable of type pgoff_t here.
* Used memcpy_from_page() and memcpy_to_page() to replace
  kmap_local_page() + memcpy() in patch 2.


# Reference

[1]: https://lore.kernel.org/lkml/20221017093726.2070674-1-zhao1.liu@linux.intel.com/
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
---
Zhao Liu (9):
  drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
  drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
  drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
  drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
  drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
  drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
  drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
  drm/i915: Use kmap_local_page() in i915_cmd_parser.c
  drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c       | 10 +++++-----
 drivers/gpu/drm/i915/gem/i915_gem_object.c           |  8 +++-----
 drivers/gpu/drm/i915/gem/i915_gem_phys.c             | 10 ++--------
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c            |  6 ++++--
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c      |  6 +++---
 .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c  | 12 ++++--------
 .../gpu/drm/i915/gem/selftests/i915_gem_context.c    |  8 ++++----
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             |  5 +----
 drivers/gpu/drm/i915/i915_cmd_parser.c               |  4 ++--
 9 files changed, 28 insertions(+), 41 deletions(-)

-- 
2.34.1

[Intel-gfx] [PATCH v2 1/9] drm/i915: Use kmap_local_page() in gem/i915_gem_object.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.

There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
need to disable pagefaults and preemption for mapping:

1. The flush operation is safe. In drm/i915/gem/i915_gem_object.c,
i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range() to
use CLFLUSHOPT or WBINVD to flush. Since CLFLUSHOPT is global on x86
and WBINVD is called on each cpu in drm_clflush_virt_range(), the flush
operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, i915_gem_object_read_from_page_kmap() is a function where
the use of kmap_local_page() in place of kmap_atomic() is correctly
suited.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

And remove the redundant variable that stores the address of the mapped
page since kunmap_local() can accept any pointer within the page.

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* Rebased on f47e630 (drm/i915/gem: Typecheck page lookups) to keep
  the "idx" variable of type pgoff_t here.
* Added description of the motivation of using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index e6d4efde4fc5..c0bfdd7784f7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -428,17 +428,15 @@ static void
 i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64 offset, void *dst, int size)
 {
 	pgoff_t idx = offset >> PAGE_SHIFT;
-	void *src_map;
 	void *src_ptr;
 
-	src_map = kmap_atomic(i915_gem_object_get_page(obj, idx));
-
-	src_ptr = src_map + offset_in_page(offset);
+	src_ptr = kmap_local_page(i915_gem_object_get_page(obj, idx))
+	          + offset_in_page(offset);
 	if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
 		drm_clflush_virt_range(src_ptr, size);
 	memcpy(dst, src_ptr, size);
 
-	kunmap_atomic(src_map);
+	kunmap_local(src_ptr);
 }
 
 static void
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 1/9] drm/i915: Use kmap_local_page() in gem/i915_gem_object.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> need to disable pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. In drm/i915/gem/i915_gem_object.c,
> i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range() to
> use CLFLUSHOPT or WBINVD to flush. Since CLFLUSHOPT is global on x86
> and WBINVD is called on each cpu in drm_clflush_virt_range(), the flush
> operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, i915_gem_object_read_from_page_kmap() is a function where
> the use of kmap_local_page() in place of kmap_atomic() is correctly
> suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> And remove the redundant variable that stores the address of the mapped
> page since kunmap_local() can accept any pointer within the page.
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * Rebased on f47e630 (drm/i915/gem: Typecheck page lookups) to keep
>   the "idx" variable of type pgoff_t here.
> * Added description of the motivation of using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[Intel-gfx] [PATCH v2 2/9] drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1],  and this patch converts the call from
kmap_atomic() + memcpy() to memcpy_[from/to]_page(), which use
kmap_local_page() to build local mapping and then do memcpy().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.

In drm/i915/gem/i915_gem_phys.c, the functions
i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys()
don't need to disable pagefaults and preemption for mapping because of
2 reasons:

1. The flush operation is safe. In drm/i915/gem/i915_gem_object.c,
i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys()
calls drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush.
Since CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
drm_clflush_virt_range(), the flush operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, i915_gem_object_get_pages_phys() and
i915_gem_object_put_pages_phys() are two functions where the uses of
local mappings in place of atomic mappings are correctly suited.

Convert the calls of kmap_atomic() / kunmap_atomic() + memcpy() to
memcpy_from_page() and memcpy_to_page().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Used memcpy_from_page() and memcpy_to_page() to replace
  kmap_local_page() + memcpy().
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* Added description of the motivation of using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred. Also based on
         his suggestion to use memcpy_[from/to]_page() directly.
---
 drivers/gpu/drm/i915/gem/i915_gem_phys.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 76efe98eaa14..4c6d3f07260a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -64,16 +64,13 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	dst = vaddr;
 	for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
 		struct page *page;
-		void *src;
 
 		page = shmem_read_mapping_page(mapping, i);
 		if (IS_ERR(page))
 			goto err_st;
 
-		src = kmap_atomic(page);
-		memcpy(dst, src, PAGE_SIZE);
+		memcpy_from_page(dst, page, 0, PAGE_SIZE);
 		drm_clflush_virt_range(dst, PAGE_SIZE);
-		kunmap_atomic(src);
 
 		put_page(page);
 		dst += PAGE_SIZE;
@@ -112,16 +109,13 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
 
 		for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
 			struct page *page;
-			char *dst;
 
 			page = shmem_read_mapping_page(mapping, i);
 			if (IS_ERR(page))
 				continue;
 
-			dst = kmap_atomic(page);
 			drm_clflush_virt_range(src, PAGE_SIZE);
-			memcpy(dst, src, PAGE_SIZE);
-			kunmap_atomic(dst);
+			memcpy_to_page(page, 0, src, PAGE_SIZE);
 
 			set_page_dirty(page);
 			if (obj->mm.madv == I915_MADV_WILLNEED)
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 2/9] drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1],  and this patch converts the call from
> kmap_atomic() + memcpy() to memcpy_[from/to]_page(), which use
> kmap_local_page() to build local mapping and then do memcpy().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> In drm/i915/gem/i915_gem_phys.c, the functions
> i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys()
> don't need to disable pagefaults and preemption for mapping because of
> 2 reasons:
> 
> 1. The flush operation is safe. In drm/i915/gem/i915_gem_object.c,
> i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys()
> calls drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush.
> Since CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
> drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, i915_gem_object_get_pages_phys() and
> i915_gem_object_put_pages_phys() are two functions where the uses of
> local mappings in place of atomic mappings are correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() + memcpy() to
> memcpy_from_page() and memcpy_to_page().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Used memcpy_from_page() and memcpy_to_page() to replace
>   kmap_local_page() + memcpy().
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * Added description of the motivation of using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[Intel-gfx] [PATCH v2 3/9] drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1].

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults or preemption disables.

In drm/i915/gem/i915_gem_shmem.c, the function shmem_pwrite() need to
disable pagefault to eliminate the potential recursion fault[2]. But
here __copy_from_user_inatomic() doesn't need to disable preemption and
local mapping is valid for sched in/out.

So it can use kmap_local_page() / kunmap_local() with
pagefault_disable() / pagefault_enable() to replace atomic mapping.

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
[2]: https://patchwork.freedesktop.org/patch/295840/

v2: No code change since v1, and added description of the motivation of
    using kmap_local_page().

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Ira: Referred to his suggestions about keeping pagefault_disable().
  Fabio: Referred to his description about why kmap_local_page() should
         be preferred.
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 37d1efcd3ca6..ad69a79c8b31 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -475,11 +475,13 @@ shmem_pwrite(struct drm_i915_gem_object *obj,
 		if (err < 0)
 			return err;
 
-		vaddr = kmap_atomic(page);
+		vaddr = kmap_local_page(page);
+		pagefault_disable();
 		unwritten = __copy_from_user_inatomic(vaddr + pg,
 						      user_data,
 						      len);
-		kunmap_atomic(vaddr);
+		pagefault_enable();
+		kunmap_local(vaddr);
 
 		err = aops->write_end(obj->base.filp, mapping, offset, len,
 				      len - unwritten, page, data);
-- 
2.34.1

[Intel-gfx] [PATCH v2 4/9] drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults or preemption disables.

In drm/i915/gem/selftests/huge_pages.c, function __cpu_check_shmem()
mainly uses mapping to flush cache and check the value. There're
2 reasons why __cpu_check_shmem() doesn't need to disable pagefaults
and preemption for mapping:

1. The flush operation is safe. Function __cpu_check_shmem() calls
drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
drm_clflush_virt_range(), the flush operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, __cpu_check_shmem() is a function where the use of
kmap_local_page() in place of kmap_atomic() is correctly suited.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* No code change since v1, and added description of the motivation of
  using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index defece0bcb81..3f9ea48a48d0 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -1026,7 +1026,7 @@ __cpu_check_shmem(struct drm_i915_gem_object *obj, u32 dword, u32 val)
 		goto err_unlock;
 
 	for (n = 0; n < obj->base.size >> PAGE_SHIFT; ++n) {
-		u32 *ptr = kmap_atomic(i915_gem_object_get_page(obj, n));
+		u32 *ptr = kmap_local_page(i915_gem_object_get_page(obj, n));
 
 		if (needs_flush & CLFLUSH_BEFORE)
 			drm_clflush_virt_range(ptr, PAGE_SIZE);
@@ -1034,12 +1034,12 @@ __cpu_check_shmem(struct drm_i915_gem_object *obj, u32 dword, u32 val)
 		if (ptr[dword] != val) {
 			pr_err("n=%lu ptr[%u]=%u, val=%u\n",
 			       n, dword, ptr[dword], val);
-			kunmap_atomic(ptr);
+			kunmap_local(ptr);
 			err = -EINVAL;
 			break;
 		}
 
-		kunmap_atomic(ptr);
+		kunmap_local(ptr);
 	}
 
 	i915_gem_object_finish_access(obj);
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 4/9] drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults or preemption disables.
> 
> In drm/i915/gem/selftests/huge_pages.c, function __cpu_check_shmem()
> mainly uses mapping to flush cache and check the value. There're
> 2 reasons why __cpu_check_shmem() doesn't need to disable pagefaults
> and preemption for mapping:
> 
> 1. The flush operation is safe. Function __cpu_check_shmem() calls
> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
> CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
> drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, __cpu_check_shmem() is a function where the use of
> kmap_local_page() in place of kmap_atomic() is correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * No code change since v1, and added description of the motivation of
>   using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[Intel-gfx] [PATCH v2 5/9] drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration)..

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults or preemption disables.

In drm/i915/gem/selftests/i915_gem_coherency.c, functions cpu_set()
and cpu_get() mainly uses mapping to flush cache and assign the value.
There're 2 reasons why cpu_set() and cpu_get() don't need to disable
pagefaults and preemption for mapping:

1. The flush operation is safe. cpu_set() and cpu_get() call
drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
drm_clflush_virt_range(), the flush operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, cpu_set() and cpu_get() are functions where the use of
kmap_local_page() in place of kmap_atomic() is correctly suited.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* No code change since v1, and added description of the motivation of
  using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c  | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
index 3bef1beec7cb..beeb3e12eccc 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
@@ -24,7 +24,6 @@ static int cpu_set(struct context *ctx, unsigned long offset, u32 v)
 {
 	unsigned int needs_clflush;
 	struct page *page;
-	void *map;
 	u32 *cpu;
 	int err;
 
@@ -34,8 +33,7 @@ static int cpu_set(struct context *ctx, unsigned long offset, u32 v)
 		goto out;
 
 	page = i915_gem_object_get_page(ctx->obj, offset >> PAGE_SHIFT);
-	map = kmap_atomic(page);
-	cpu = map + offset_in_page(offset);
+	cpu = kmap_local_page(page) + offset_in_page(offset);
 
 	if (needs_clflush & CLFLUSH_BEFORE)
 		drm_clflush_virt_range(cpu, sizeof(*cpu));
@@ -45,7 +43,7 @@ static int cpu_set(struct context *ctx, unsigned long offset, u32 v)
 	if (needs_clflush & CLFLUSH_AFTER)
 		drm_clflush_virt_range(cpu, sizeof(*cpu));
 
-	kunmap_atomic(map);
+	kunmap_local(cpu);
 	i915_gem_object_finish_access(ctx->obj);
 
 out:
@@ -57,7 +55,6 @@ static int cpu_get(struct context *ctx, unsigned long offset, u32 *v)
 {
 	unsigned int needs_clflush;
 	struct page *page;
-	void *map;
 	u32 *cpu;
 	int err;
 
@@ -67,15 +64,14 @@ static int cpu_get(struct context *ctx, unsigned long offset, u32 *v)
 		goto out;
 
 	page = i915_gem_object_get_page(ctx->obj, offset >> PAGE_SHIFT);
-	map = kmap_atomic(page);
-	cpu = map + offset_in_page(offset);
+	cpu = kmap_local_page(page) + offset_in_page(offset);
 
 	if (needs_clflush & CLFLUSH_BEFORE)
 		drm_clflush_virt_range(cpu, sizeof(*cpu));
 
 	*v = *cpu;
 
-	kunmap_atomic(map);
+	kunmap_local(cpu);
 	i915_gem_object_finish_access(ctx->obj);
 
 out:
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 5/9] drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration)..
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults or preemption disables.
> 
> In drm/i915/gem/selftests/i915_gem_coherency.c, functions cpu_set()
> and cpu_get() mainly uses mapping to flush cache and assign the value.
> There're 2 reasons why cpu_set() and cpu_get() don't need to disable
> pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. cpu_set() and cpu_get() call
> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
> CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
> drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, cpu_set() and cpu_get() are functions where the use of
> kmap_local_page() in place of kmap_atomic() is correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * No code change since v1, and added description of the motivation of
>   using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[Intel-gfx] [PATCH v2 6/9] drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption.

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults or preemption disables.

In drm/i915/gem/selftests/i915_gem_context.c, functions cpu_fill() and
cpu_check() mainly uses mapping to flush cache and check/assign the
value.

There're 2 reasons why cpu_fill() and cpu_check() don't need to disable
pagefaults and preemption for mapping:

1. The flush operation is safe. cpu_fill() and cpu_check() call
drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
drm_clflush_virt_range(), the flush operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, cpu_fill() and cpu_check() are functions where the use of
kmap_local_page() in place of kmap_atomic() is correctly suited.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* No code change since v1, and added description of the motivation of
  using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index a81fa6a20f5a..dcbc0b8e3323 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -481,12 +481,12 @@ static int cpu_fill(struct drm_i915_gem_object *obj, u32 value)
 	for (n = 0; n < real_page_count(obj); n++) {
 		u32 *map;
 
-		map = kmap_atomic(i915_gem_object_get_page(obj, n));
+		map = kmap_local_page(i915_gem_object_get_page(obj, n));
 		for (m = 0; m < DW_PER_PAGE; m++)
 			map[m] = value;
 		if (!has_llc)
 			drm_clflush_virt_range(map, PAGE_SIZE);
-		kunmap_atomic(map);
+		kunmap_local(map);
 	}
 
 	i915_gem_object_finish_access(obj);
@@ -512,7 +512,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
 	for (n = 0; n < real_page_count(obj); n++) {
 		u32 *map, m;
 
-		map = kmap_atomic(i915_gem_object_get_page(obj, n));
+		map = kmap_local_page(i915_gem_object_get_page(obj, n));
 		if (needs_flush & CLFLUSH_BEFORE)
 			drm_clflush_virt_range(map, PAGE_SIZE);
 
@@ -538,7 +538,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
 		}
 
 out_unmap:
-		kunmap_atomic(map);
+		kunmap_local(map);
 		if (err)
 			break;
 	}
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption.
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults or preemption disables.
> 
> In drm/i915/gem/selftests/i915_gem_context.c, functions cpu_fill() and
> cpu_check() mainly uses mapping to flush cache and check/assign the
> value.
> 
> There're 2 reasons why cpu_fill() and cpu_check() don't need to disable
> pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. cpu_fill() and cpu_check() call
> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
> CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
> drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, cpu_fill() and cpu_check() are functions where the use of
> kmap_local_page() in place of kmap_atomic() is correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * No code change since v1, and added description of the motivation of
>   using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

First off I think this is fine.

But as I looked at this final selftests patch I began to wonder how the
memory being mapped here and in the previous selftests patches are
allocated.  Does highmem need to be considered at all?  Unfortunately, I
could not determine where the memory in the SG list of this test gem
object was allocated.

AFAICS cpu_fill() is only called in create_test_object().  Digging into
huge_gem_object() did not reveal where these pages were allocated from.

I wonder if these kmap_local_page() calls could be removed entirely based
on knowing that the pages were allocated from low mem?  Removing yet
another user of highmem altogether would be best if possible.

Do you know how these test objects are created?  Do the pages come from
user space somehow?

Regardless this is still a step in the right direction so:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Suggested by credits:
>   Dave: Referred to his explanation about cache flush.
>   Ira: Referred to his task document, review comments and explanation
>        about cache flush.
>   Fabio: Referred to his boiler plate commit message and his description
>          about why kmap_local_page() should be preferred.
> ---
>  drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index a81fa6a20f5a..dcbc0b8e3323 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -481,12 +481,12 @@ static int cpu_fill(struct drm_i915_gem_object *obj, u32 value)
>  	for (n = 0; n < real_page_count(obj); n++) {
>  		u32 *map;
>  
> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>  		for (m = 0; m < DW_PER_PAGE; m++)
>  			map[m] = value;
>  		if (!has_llc)
>  			drm_clflush_virt_range(map, PAGE_SIZE);
> -		kunmap_atomic(map);
> +		kunmap_local(map);
>  	}
>  
>  	i915_gem_object_finish_access(obj);
> @@ -512,7 +512,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>  	for (n = 0; n < real_page_count(obj); n++) {
>  		u32 *map, m;
>  
> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>  		if (needs_flush & CLFLUSH_BEFORE)
>  			drm_clflush_virt_range(map, PAGE_SIZE);
>  
> @@ -538,7 +538,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>  		}
>  
>  out_unmap:
> -		kunmap_atomic(map);
> +		kunmap_local(map);
>  		if (err)
>  			break;
>  	}
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c

[Tvrtko Ursulin]

On 31/03/2023 04:33, Ira Weiny wrote:
> Zhao Liu wrote:
>> From: Zhao Liu <zhao1.liu@intel.com>
>>
>> The use of kmap_atomic() is being deprecated in favor of
>> kmap_local_page()[1], and this patch converts the call from
>> kmap_atomic() to kmap_local_page().
>>
>> The main difference between atomic and local mappings is that local
>> mappings doesn't disable page faults or preemption.
>>
>> With kmap_local_page(), we can avoid the often unwanted side effect of
>> unnecessary page faults or preemption disables.
>>
>> In drm/i915/gem/selftests/i915_gem_context.c, functions cpu_fill() and
>> cpu_check() mainly uses mapping to flush cache and check/assign the
>> value.
>>
>> There're 2 reasons why cpu_fill() and cpu_check() don't need to disable
>> pagefaults and preemption for mapping:
>>
>> 1. The flush operation is safe. cpu_fill() and cpu_check() call
>> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
>> CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
>> drm_clflush_virt_range(), the flush operation is global.
>>
>> 2. Any context switch caused by preemption or page faults (page fault
>> may cause sleep) doesn't affect the validity of local mapping.
>>
>> Therefore, cpu_fill() and cpu_check() are functions where the use of
>> kmap_local_page() in place of kmap_atomic() is correctly suited.
>>
>> Convert the calls of kmap_atomic() / kunmap_atomic() to
>> kmap_local_page() / kunmap_local().
>>
>> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
>>
>> v2:
>> * Dropped hot plug related description since it has nothing to do with
>>    kmap_local_page().
>> * No code change since v1, and added description of the motivation of
>>    using kmap_local_page().
>>
>> Suggested-by: Dave Hansen <dave.hansen@intel.com>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> 
> First off I think this is fine.
> 
> But as I looked at this final selftests patch I began to wonder how the
> memory being mapped here and in the previous selftests patches are
> allocated.  Does highmem need to be considered at all?  Unfortunately, I
> could not determine where the memory in the SG list of this test gem
> object was allocated.
> 
> AFAICS cpu_fill() is only called in create_test_object().  Digging into
> huge_gem_object() did not reveal where these pages were allocated from.
> 
> I wonder if these kmap_local_page() calls could be removed entirely based
> on knowing that the pages were allocated from low mem?  Removing yet
> another user of highmem altogether would be best if possible.
> 
> Do you know how these test objects are created?  Do the pages come from
> user space somehow?

FWIW

create_test_object
  -> huge_gem_object
      -> i915_gem_object_init(obj, &huge_ops, &lock_class, 0);

Which is:

static const struct drm_i915_gem_object_ops huge_ops = {
	.name = "huge-gem",
	.get_pages = huge_get_pages,
	.put_pages = huge_put_pages,
};

And:

huge_get_pages()
...
#define GFP (GFP_KERNEL | __GFP_NOWARN | __GFP_RETRY_MAYFAIL)
...
		page = alloc_page(GFP | __GFP_HIGHMEM);

> 
> Regardless this is still a step in the right direction so:
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Yeah LGTM.

FYI I am yet to read through the rest of the series, but I don't think 
there will be anything problematic and it passed our CI so likely is 
good to pull in.

Regards,

Tvrtko

> 
>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> ---
>> Suggested by credits:
>>    Dave: Referred to his explanation about cache flush.
>>    Ira: Referred to his task document, review comments and explanation
>>         about cache flush.
>>    Fabio: Referred to his boiler plate commit message and his description
>>           about why kmap_local_page() should be preferred.
>> ---
>>   drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> index a81fa6a20f5a..dcbc0b8e3323 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> @@ -481,12 +481,12 @@ static int cpu_fill(struct drm_i915_gem_object *obj, u32 value)
>>   	for (n = 0; n < real_page_count(obj); n++) {
>>   		u32 *map;
>>   
>> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
>> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>>   		for (m = 0; m < DW_PER_PAGE; m++)
>>   			map[m] = value;
>>   		if (!has_llc)
>>   			drm_clflush_virt_range(map, PAGE_SIZE);
>> -		kunmap_atomic(map);
>> +		kunmap_local(map);
>>   	}
>>   
>>   	i915_gem_object_finish_access(obj);
>> @@ -512,7 +512,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>>   	for (n = 0; n < real_page_count(obj); n++) {
>>   		u32 *map, m;
>>   
>> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
>> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>>   		if (needs_flush & CLFLUSH_BEFORE)
>>   			drm_clflush_virt_range(map, PAGE_SIZE);
>>   
>> @@ -538,7 +538,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>>   		}
>>   
>>   out_unmap:
>> -		kunmap_atomic(map);
>> +		kunmap_local(map);
>>   		if (err)
>>   			break;
>>   	}
>> -- 
>> 2.34.1
>>
> 
> 

[Intel-gfx] [PATCH v2 7/9] drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption  (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults or preemption disables.

In drm/i915/gt/uc/intel_us_fw.c, the function intel_uc_fw_copy_rsa()
just use the mapping to do memory copy so it doesn't need to disable
pagefaults and preemption for mapping. Thus the local mapping without
atomic context (not disable pagefaults / preemption) is enough.

Therefore, intel_uc_fw_copy_rsa() is a function where the use of
memcpy_from_page() with kmap_local_page() in place of memcpy() with
kmap_atomic() is correctly suited.

Convert the calls of memcpy() with kmap_atomic() / kunmap_atomic() to
memcpy_from_page() which uses local mapping to copy.

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com/T/#u

v2: No code change since v1, and added description of the motivation of
    using kmap_local_page().

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Ira: Referred to his task document and suggestions about using
       memcpy_from_page() directly.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 65672ff82605..5bbde4abd565 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -1152,16 +1152,13 @@ size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len)
 
 		for_each_sgt_page(page, iter, uc_fw->obj->mm.pages) {
 			u32 len = min_t(u32, size, PAGE_SIZE - offset);
-			void *vaddr;
 
 			if (idx > 0) {
 				idx--;
 				continue;
 			}
 
-			vaddr = kmap_atomic(page);
-			memcpy(dst, vaddr + offset, len);
-			kunmap_atomic(vaddr);
+			memcpy_from_page(dst, page, offset, len);
 
 			offset = 0;
 			dst += len;
-- 
2.34.1

[Intel-gfx] [PATCH v2 8/9] drm/i915: Use kmap_local_page() in i915_cmd_parser.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the call from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.

There're 2 reasons why function copy_batch() doesn't need to disable
pagefaults and preemption for mapping:

1. The flush operation is safe. In i915_cmd_parser.c, copy_batch() calls
drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush.
Since CLFLUSHOPT is global on x86 and WBINVD is called on each cpu
in drm_clflush_virt_range(), the flush operation is global.

2. Any context switch caused by preemption or page faults (page fault
may cause sleep) doesn't affect the validity of local mapping.

Therefore, copy_batch() is a function where the use of
kmap_local_page() in place of kmap_atomic() is correctly suited.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2:
* Dropped hot plug related description since it has nothing to do with
  kmap_local_page().
* No code change since v1, and added description of the motivation of
  using kmap_local_page().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Dave: Referred to his explanation about cache flush.
  Ira: Referred to his task document, review comments and explanation
       about cache flush.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index ddf49c2dbb91..2905df83e180 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1211,11 +1211,11 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
 		for (n = offset >> PAGE_SHIFT; remain; n++) {
 			int len = min(remain, PAGE_SIZE - x);
 
-			src = kmap_atomic(i915_gem_object_get_page(src_obj, n));
+			src = kmap_local_page(i915_gem_object_get_page(src_obj, n));
 			if (src_needs_clflush)
 				drm_clflush_virt_range(src + x, len);
 			memcpy(ptr, src + x, len);
-			kunmap_atomic(src);
+			kunmap_local(src);
 
 			ptr += len;
 			remain -= len;
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 8/9] drm/i915: Use kmap_local_page() in i915_cmd_parser.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> There're 2 reasons why function copy_batch() doesn't need to disable
> pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. In i915_cmd_parser.c, copy_batch() calls
> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush.
> Since CLFLUSHOPT is global on x86 and WBINVD is called on each cpu
> in drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, copy_batch() is a function where the use of
> kmap_local_page() in place of kmap_atomic() is correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * No code change since v1, and added description of the motivation of
>   using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the calls from
kmap_atomic() to kmap_local_page().

The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.

In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
kmap_atomic() in eb_relocate_entry(), and is unmapped by
kunmap_atomic() in reloc_cache_reset().

And this mapping/unmapping occurs in two places: one is in
eb_relocate_vma(), and another is in eb_relocate_vma_slow().

The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
need to disable pagefaults and preemption during the above mapping/
unmapping.

So it can simply use kmap_local_page() / kunmap_local() that can
instead do the mapping / unmapping regardless of the context.

Convert the calls of kmap_atomic() / kunmap_atomic() to
kmap_local_page() / kunmap_local().

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

v2: No code change since v1. Added description of the motivation of
    using kmap_local_page() and "Suggested-by" tag of Fabio.

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Ira: Referred to his task document, review comments.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 9dce2957b4e5..805565edd148 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1151,7 +1151,7 @@ static void reloc_cache_unmap(struct reloc_cache *cache)
 
 	vaddr = unmask_page(cache->vaddr);
 	if (cache->vaddr & KMAP)
-		kunmap_atomic(vaddr);
+		kunmap_local(vaddr);
 	else
 		io_mapping_unmap_atomic((void __iomem *)vaddr);
 }
@@ -1167,7 +1167,7 @@ static void reloc_cache_remap(struct reloc_cache *cache,
 	if (cache->vaddr & KMAP) {
 		struct page *page = i915_gem_object_get_page(obj, cache->page);
 
-		vaddr = kmap_atomic(page);
+		vaddr = kmap_local_page(page);
 		cache->vaddr = unmask_flags(cache->vaddr) |
 			(unsigned long)vaddr;
 	} else {
@@ -1197,7 +1197,7 @@ static void reloc_cache_reset(struct reloc_cache *cache, struct i915_execbuffer
 		if (cache->vaddr & CLFLUSH_AFTER)
 			mb();
 
-		kunmap_atomic(vaddr);
+		kunmap_local(vaddr);
 		i915_gem_object_finish_access(obj);
 	} else {
 		struct i915_ggtt *ggtt = cache_to_ggtt(cache);
@@ -1229,7 +1229,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
 	struct page *page;
 
 	if (cache->vaddr) {
-		kunmap_atomic(unmask_page(cache->vaddr));
+		kunmap_local(unmask_page(cache->vaddr));
 	} else {
 		unsigned int flushes;
 		int err;
@@ -1251,7 +1251,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
 	if (!obj->mm.dirty)
 		set_page_dirty(page);
 
-	vaddr = kmap_atomic(page);
+	vaddr = kmap_local_page(page);
 	cache->vaddr = unmask_flags(cache->vaddr) | (unsigned long)vaddr;
 	cache->page = pageno;
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Ira Weiny]

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the calls from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
> kmap_atomic() in eb_relocate_entry(), and is unmapped by
> kunmap_atomic() in reloc_cache_reset().

First off thanks for the series and sticking with this.  That said this
patch kind of threw me for a loop because tracing the map/unmap calls did
not make sense to me.  See below.

> 
> And this mapping/unmapping occurs in two places: one is in
> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
> 
> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
> need to disable pagefaults and preemption during the above mapping/
> unmapping.
> 
> So it can simply use kmap_local_page() / kunmap_local() that can
> instead do the mapping / unmapping regardless of the context.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2: No code change since v1. Added description of the motivation of
>     using kmap_local_page() and "Suggested-by" tag of Fabio.
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Suggested by credits:
>   Ira: Referred to his task document, review comments.
>   Fabio: Referred to his boiler plate commit message and his description
>          about why kmap_local_page() should be preferred.
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 9dce2957b4e5..805565edd148 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1151,7 +1151,7 @@ static void reloc_cache_unmap(struct reloc_cache *cache)
>  
>  	vaddr = unmask_page(cache->vaddr);
>  	if (cache->vaddr & KMAP)
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);

In the cover letter you don't mention this unmap path.  Rather you mention
only reloc_cache_reset().

After digging into this and considering these are kmap_atomic() calls I
_think_ what you have is ok.  But I think I'd like to see the call paths
documented a bit more clearly.  Or perhaps cleaned up a lot.

For example I see the following call possibility from a user ioctl.  In
this trace I see 2 examples where something is unmapped first.  I don't
understand why that is required?  I would assume reloc_cache_unmap() and
reloc_kmap() are helpers called from somewhere else requiring a remapping
of the cache but I don't see it.

i915_gem_execbuffer2_ioctl()
eb_relocate_parse()
eb_relocate_parse_slow()
eb_relocate_vma_slow()
	eb_relocate_entry()
		reloc_cache_unmap()
			kunmap_atomic()  <=== HERE!
		reloc_cache_remap()
			kmap_atomic()
		relocate_entry()
			reloc_vaddr()
				reloc_kmap()
					kunmap_atomic() <== HERE!
					kmap_atomic()

	reloc_cache_reset()
		kunmap_atomic()

Could these mappings be cleaned up a lot more?  Perhaps by removing some
of the helper functions which AFAICT are left over from older versions of
the code?

Also as an aside I think it is really bad that eb_relocate_entry() returns
negative errors in a u64.  Better to get the types right IMO.

Thanks for the series!
Ira

>  	else
>  		io_mapping_unmap_atomic((void __iomem *)vaddr);
>  }
> @@ -1167,7 +1167,7 @@ static void reloc_cache_remap(struct reloc_cache *cache,
>  	if (cache->vaddr & KMAP) {
>  		struct page *page = i915_gem_object_get_page(obj, cache->page);
>  
> -		vaddr = kmap_atomic(page);
> +		vaddr = kmap_local_page(page);
>  		cache->vaddr = unmask_flags(cache->vaddr) |
>  			(unsigned long)vaddr;
>  	} else {
> @@ -1197,7 +1197,7 @@ static void reloc_cache_reset(struct reloc_cache *cache, struct i915_execbuffer
>  		if (cache->vaddr & CLFLUSH_AFTER)
>  			mb();
>  
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);
>  		i915_gem_object_finish_access(obj);
>  	} else {
>  		struct i915_ggtt *ggtt = cache_to_ggtt(cache);
> @@ -1229,7 +1229,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>  	struct page *page;
>  
>  	if (cache->vaddr) {
> -		kunmap_atomic(unmask_page(cache->vaddr));
> +		kunmap_local(unmask_page(cache->vaddr));
>  	} else {
>  		unsigned int flushes;
>  		int err;
> @@ -1251,7 +1251,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>  	if (!obj->mm.dirty)
>  		set_page_dirty(page);
>  
> -	vaddr = kmap_atomic(page);
> +	vaddr = kmap_local_page(page);
>  	cache->vaddr = unmask_flags(cache->vaddr) | (unsigned long)vaddr;
>  	cache->page = pageno;
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Tvrtko Ursulin]

On 31/03/2023 05:18, Ira Weiny wrote:
> Zhao Liu wrote:
>> From: Zhao Liu <zhao1.liu@intel.com>
>>
>> The use of kmap_atomic() is being deprecated in favor of
>> kmap_local_page()[1], and this patch converts the calls from
>> kmap_atomic() to kmap_local_page().
>>
>> The main difference between atomic and local mappings is that local
>> mappings doesn't disable page faults or preemption (the preemption is
>> disabled for !PREEMPT_RT case, otherwise it only disables migration).
>>
>> With kmap_local_page(), we can avoid the often unwanted side effect of
>> unnecessary page faults and preemption disables.
>>
>> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
>> kmap_atomic() in eb_relocate_entry(), and is unmapped by
>> kunmap_atomic() in reloc_cache_reset().
> 
> First off thanks for the series and sticking with this.  That said this
> patch kind of threw me for a loop because tracing the map/unmap calls did
> not make sense to me.  See below.
> 
>>
>> And this mapping/unmapping occurs in two places: one is in
>> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
>>
>> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
>> need to disable pagefaults and preemption during the above mapping/
>> unmapping.
>>
>> So it can simply use kmap_local_page() / kunmap_local() that can
>> instead do the mapping / unmapping regardless of the context.
>>
>> Convert the calls of kmap_atomic() / kunmap_atomic() to
>> kmap_local_page() / kunmap_local().
>>
>> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
>>
>> v2: No code change since v1. Added description of the motivation of
>>      using kmap_local_page() and "Suggested-by" tag of Fabio.
>>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> ---
>> Suggested by credits:
>>    Ira: Referred to his task document, review comments.
>>    Fabio: Referred to his boiler plate commit message and his description
>>           about why kmap_local_page() should be preferred.
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 9dce2957b4e5..805565edd148 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -1151,7 +1151,7 @@ static void reloc_cache_unmap(struct reloc_cache *cache)
>>   
>>   	vaddr = unmask_page(cache->vaddr);
>>   	if (cache->vaddr & KMAP)
>> -		kunmap_atomic(vaddr);
>> +		kunmap_local(vaddr);
> 
> In the cover letter you don't mention this unmap path.  Rather you mention
> only reloc_cache_reset().
> 
> After digging into this and considering these are kmap_atomic() calls I
> _think_ what you have is ok.  But I think I'd like to see the call paths
> documented a bit more clearly.  Or perhaps cleaned up a lot.
> 
> For example I see the following call possibility from a user ioctl.  In
> this trace I see 2 examples where something is unmapped first.  I don't
> understand why that is required?  I would assume reloc_cache_unmap() and
> reloc_kmap() are helpers called from somewhere else requiring a remapping
> of the cache but I don't see it.

Reloc_cache_unmap is called from eb_relocate_entry.

The confusing part unmap appears first is just because reloc_cache is a 
stateful setup. The previous mapping is kept around until reset (callers 
moves to a different parent object), and unampped/remapped once moved to 
a different page within that object.

However I am unsure if disabling pagefaulting is needed or not. Thomas, 
Matt, being the last to touch this area, perhaps you could have a look? 
Because I notice we have a fallback iomap path which still uses 
io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is 
safe, does the iomap side also needs converting to 
io_mapping_map_local_wc? Or they have separate requirements?

Regards,

Tvrtko

> 
> i915_gem_execbuffer2_ioctl()
> eb_relocate_parse()
> eb_relocate_parse_slow()
> eb_relocate_vma_slow()
> 	eb_relocate_entry()
> 		reloc_cache_unmap()
> 			kunmap_atomic()  <=== HERE!
> 		reloc_cache_remap()
> 			kmap_atomic()
> 		relocate_entry()
> 			reloc_vaddr()
> 				reloc_kmap()
> 					kunmap_atomic() <== HERE!
> 					kmap_atomic()
> 
> 	reloc_cache_reset()
> 		kunmap_atomic()
> 
> Could these mappings be cleaned up a lot more?  Perhaps by removing some
> of the helper functions which AFAICT are left over from older versions of
> the code?
> 
> Also as an aside I think it is really bad that eb_relocate_entry() returns
> negative errors in a u64.  Better to get the types right IMO.
> 
> Thanks for the series!
> Ira
> 
>>   	else
>>   		io_mapping_unmap_atomic((void __iomem *)vaddr);
>>   }
>> @@ -1167,7 +1167,7 @@ static void reloc_cache_remap(struct reloc_cache *cache,
>>   	if (cache->vaddr & KMAP) {
>>   		struct page *page = i915_gem_object_get_page(obj, cache->page);
>>   
>> -		vaddr = kmap_atomic(page);
>> +		vaddr = kmap_local_page(page);
>>   		cache->vaddr = unmask_flags(cache->vaddr) |
>>   			(unsigned long)vaddr;
>>   	} else {
>> @@ -1197,7 +1197,7 @@ static void reloc_cache_reset(struct reloc_cache *cache, struct i915_execbuffer
>>   		if (cache->vaddr & CLFLUSH_AFTER)
>>   			mb();
>>   
>> -		kunmap_atomic(vaddr);
>> +		kunmap_local(vaddr);
>>   		i915_gem_object_finish_access(obj);
>>   	} else {
>>   		struct i915_ggtt *ggtt = cache_to_ggtt(cache);
>> @@ -1229,7 +1229,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>>   	struct page *page;
>>   
>>   	if (cache->vaddr) {
>> -		kunmap_atomic(unmask_page(cache->vaddr));
>> +		kunmap_local(unmask_page(cache->vaddr));
>>   	} else {
>>   		unsigned int flushes;
>>   		int err;
>> @@ -1251,7 +1251,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>>   	if (!obj->mm.dirty)
>>   		set_page_dirty(page);
>>   
>> -	vaddr = kmap_atomic(page);
>> +	vaddr = kmap_local_page(page);
>>   	cache->vaddr = unmask_flags(cache->vaddr) | (unsigned long)vaddr;
>>   	cache->page = pageno;
>>   
>> -- 
>> 2.34.1
>>
> 
> 

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Fabio M. De Francesco]

On venerdì 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
> On 31/03/2023 05:18, Ira Weiny wrote:
> > Zhao Liu wrote:
> >> From: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> The use of kmap_atomic() is being deprecated in favor of
> >> kmap_local_page()[1], and this patch converts the calls from
> >> kmap_atomic() to kmap_local_page().
> >> 
> >> The main difference between atomic and local mappings is that local
> >> mappings doesn't disable page faults or preemption (the preemption is
> >> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> >> 
> >> With kmap_local_page(), we can avoid the often unwanted side effect of
> >> unnecessary page faults and preemption disables.
> >> 
> >> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
> >> kmap_atomic() in eb_relocate_entry(), and is unmapped by
> >> kunmap_atomic() in reloc_cache_reset().
> > 
> > First off thanks for the series and sticking with this.  That said this
> > patch kind of threw me for a loop because tracing the map/unmap calls did
> > not make sense to me.  See below.
> > 
> >> And this mapping/unmapping occurs in two places: one is in
> >> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
> >> 
> >> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
> >> need to disable pagefaults and preemption during the above mapping/
> >> unmapping.
> >> 
> >> So it can simply use kmap_local_page() / kunmap_local() that can
> >> instead do the mapping / unmapping regardless of the context.
> >> 
> >> Convert the calls of kmap_atomic() / kunmap_atomic() to
> >> kmap_local_page() / kunmap_local().
> >> 
> >> [1]:
> >> https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> >> 
> >> v2: No code change since v1. Added description of the motivation of
> >> 
> >>      using kmap_local_page() and "Suggested-by" tag of Fabio.
> >> 
> >> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> >> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> >> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> ---
> >> 
> >> Suggested by credits:
> >>    Ira: Referred to his task document, review comments.
> >>    Fabio: Referred to his boiler plate commit message and his description
> >>    
> >>           about why kmap_local_page() should be preferred.
> >> 
> >> ---
> >> 
> >>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >> 

[snip]
 
> However I am unsure if disabling pagefaulting is needed or not. Thomas,
> Matt, being the last to touch this area, perhaps you could have a look?
> Because I notice we have a fallback iomap path which still uses
> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> safe, does the iomap side also needs converting to
> io_mapping_map_local_wc? Or they have separate requirements?

AFAIK, the requirements for io_mapping_map_local_wc() are the same as for 
kmap_local_page(): the kernel virtual address is _only_ valid in the caller 
context, and map/unmap nesting must be done in stack-based ordering (LIFO).

I think a follow up patch could safely switch to io_mapping_map_local_wc() / 
io_mapping_unmap_local_wc since the address is local to context.

However, not being an expert, reading your note now I suspect that I'm missing 
something. Can I ask why you think that page-faults disabling might be 
necessary? 

Thanks,

Fabio

> Regards,
> 
> Tvrtko

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Zhao Liu]

Thanks all for your review!

On Fri, Mar 31, 2023 at 05:32:17PM +0200, Fabio M. De Francesco wrote:
> Date: Fri, 31 Mar 2023 17:32:17 +0200
> From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>  gem/i915_gem_execbuffer.c
> 
> On venerd? 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
> > On 31/03/2023 05:18, Ira Weiny wrote:
> 

[snip]

>  
> > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > Matt, being the last to touch this area, perhaps you could have a look?
> > Because I notice we have a fallback iomap path which still uses
> > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > safe, does the iomap side also needs converting to
> > io_mapping_map_local_wc? Or they have separate requirements?
> 
> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for 
> kmap_local_page(): the kernel virtual address is _only_ valid in the caller 
> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> 
> I think a follow up patch could safely switch to io_mapping_map_local_wc() / 
> io_mapping_unmap_local_wc since the address is local to context.
> 
> However, not being an expert, reading your note now I suspect that I'm missing 
> something. Can I ask why you think that page-faults disabling might be 
> necessary? 


About the disabling of pagefault here, could you please talk more about
it? :-)

From previous discussions and commit history, I didn't find relevant
information and I lack background knowledge about it...

If we have the reason to diable pagefault, I will fix and refresh the new
version.

Thanks,
Zhao

> 
> Thanks,
> 
> Fabio
> 
> > Regards,
> > 
> > Tvrtko
> 
> 
> 

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Tvrtko Ursulin]

On 31/03/2023 16:32, Fabio M. De Francesco wrote:
> On venerdì 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
>> On 31/03/2023 05:18, Ira Weiny wrote:
>>> Zhao Liu wrote:
>>>> From: Zhao Liu <zhao1.liu@intel.com>
>>>>
>>>> The use of kmap_atomic() is being deprecated in favor of
>>>> kmap_local_page()[1], and this patch converts the calls from
>>>> kmap_atomic() to kmap_local_page().
>>>>
>>>> The main difference between atomic and local mappings is that local
>>>> mappings doesn't disable page faults or preemption (the preemption is
>>>> disabled for !PREEMPT_RT case, otherwise it only disables migration).
>>>>
>>>> With kmap_local_page(), we can avoid the often unwanted side effect of
>>>> unnecessary page faults and preemption disables.
>>>>
>>>> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
>>>> kmap_atomic() in eb_relocate_entry(), and is unmapped by
>>>> kunmap_atomic() in reloc_cache_reset().
>>>
>>> First off thanks for the series and sticking with this.  That said this
>>> patch kind of threw me for a loop because tracing the map/unmap calls did
>>> not make sense to me.  See below.
>>>
>>>> And this mapping/unmapping occurs in two places: one is in
>>>> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
>>>>
>>>> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
>>>> need to disable pagefaults and preemption during the above mapping/
>>>> unmapping.
>>>>
>>>> So it can simply use kmap_local_page() / kunmap_local() that can
>>>> instead do the mapping / unmapping regardless of the context.
>>>>
>>>> Convert the calls of kmap_atomic() / kunmap_atomic() to
>>>> kmap_local_page() / kunmap_local().
>>>>
>>>> [1]:
>>>> https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
>>>>
>>>> v2: No code change since v1. Added description of the motivation of
>>>>
>>>>       using kmap_local_page() and "Suggested-by" tag of Fabio.
>>>>
>>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>>> ---
>>>>
>>>> Suggested by credits:
>>>>     Ira: Referred to his task document, review comments.
>>>>     Fabio: Referred to his boiler plate commit message and his description
>>>>     
>>>>            about why kmap_local_page() should be preferred.
>>>>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
> 
> [snip]
>   
>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>> Matt, being the last to touch this area, perhaps you could have a look?
>> Because I notice we have a fallback iomap path which still uses
>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>> safe, does the iomap side also needs converting to
>> io_mapping_map_local_wc? Or they have separate requirements?
> 
> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> 
> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> io_mapping_unmap_local_wc since the address is local to context.
> 
> However, not being an expert, reading your note now I suspect that I'm missing
> something. Can I ask why you think that page-faults disabling might be
> necessary?

I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.

I guess it will work since the copying is done like this anyway:

		/*
		 * This is the fast path and we cannot handle a pagefault
		 * whilst holding the struct mutex lest the user pass in the
		 * relocations contained within a mmaped bo. For in such a case
		 * we, the page fault handler would call i915_gem_fault() and
		 * we would try to acquire the struct mutex again. Obviously
		 * this is bad and so lockdep complains vehemently.
		 */
		pagefault_disable();
		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
		pagefault_enable();
		if (unlikely(copied)) {
			remain = -EFAULT;
			goto out;
		}

Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.

Patch to convert the io_mapping_map_atomic_wc can indeed come later.

In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Zhao Liu]

Hi Tvrtko,

On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:

[snip]

> > 
> > [snip]
> > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > Matt, being the last to touch this area, perhaps you could have a look?
> > > Because I notice we have a fallback iomap path which still uses
> > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > safe, does the iomap side also needs converting to
> > > io_mapping_map_local_wc? Or they have separate requirements?
> > 
> > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > 
> > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > io_mapping_unmap_local_wc since the address is local to context.
> > 
> > However, not being an expert, reading your note now I suspect that I'm missing
> > something. Can I ask why you think that page-faults disabling might be
> > necessary?
> 
> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> 
> I guess it will work since the copying is done like this anyway:
> 
> 		/*
> 		 * This is the fast path and we cannot handle a pagefault
> 		 * whilst holding the struct mutex lest the user pass in the
> 		 * relocations contained within a mmaped bo. For in such a case
> 		 * we, the page fault handler would call i915_gem_fault() and
> 		 * we would try to acquire the struct mutex again. Obviously
> 		 * this is bad and so lockdep complains vehemently.
> 		 */
> 		pagefault_disable();
> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> 		pagefault_enable();
> 		if (unlikely(copied)) {
> 			remain = -EFAULT;
> 			goto out;
> 		}
> 
> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.

Thanks for your explanation!

> 
> Patch to convert the io_mapping_map_atomic_wc can indeed come later.

Okay, I will also look at this.

> 
> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?

Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?

Thanks,
Zhao

> 
> Regards,
> 
> Tvrtko

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Tvrtko Ursulin]

On 14/04/2023 11:45, Zhao Liu wrote:
> Hi Tvrtko,
> 
> On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> 
> [snip]
> 
>>>
>>> [snip]
>>>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>>>> Matt, being the last to touch this area, perhaps you could have a look?
>>>> Because I notice we have a fallback iomap path which still uses
>>>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>>>> safe, does the iomap side also needs converting to
>>>> io_mapping_map_local_wc? Or they have separate requirements?
>>>
>>> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
>>> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
>>> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
>>>
>>> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
>>> io_mapping_unmap_local_wc since the address is local to context.
>>>
>>> However, not being an expert, reading your note now I suspect that I'm missing
>>> something. Can I ask why you think that page-faults disabling might be
>>> necessary?
>>
>> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
>>
>> I guess it will work since the copying is done like this anyway:
>>
>> 		/*
>> 		 * This is the fast path and we cannot handle a pagefault
>> 		 * whilst holding the struct mutex lest the user pass in the
>> 		 * relocations contained within a mmaped bo. For in such a case
>> 		 * we, the page fault handler would call i915_gem_fault() and
>> 		 * we would try to acquire the struct mutex again. Obviously
>> 		 * this is bad and so lockdep complains vehemently.
>> 		 */
>> 		pagefault_disable();
>> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
>> 		pagefault_enable();
>> 		if (unlikely(copied)) {
>> 			remain = -EFAULT;
>> 			goto out;
>> 		}
>>
>> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> 
> Thanks for your explanation!
> 
>>
>> Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> 
> Okay, I will also look at this.
> 
>>
>> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> 
> Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?

Yes, but just because we failed to review and merge in time, not because 
you did not provide patches in time.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Rodrigo Vivi]

On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/04/2023 11:45, Zhao Liu wrote:
> > Hi Tvrtko,
> > 
> > On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> > 
> > [snip]
> > 
> > > > 
> > > > [snip]
> > > > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > > > Matt, being the last to touch this area, perhaps you could have a look?
> > > > > Because I notice we have a fallback iomap path which still uses
> > > > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > > > safe, does the iomap side also needs converting to
> > > > > io_mapping_map_local_wc? Or they have separate requirements?
> > > > 
> > > > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > > > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > > > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > > > 
> > > > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > > > io_mapping_unmap_local_wc since the address is local to context.
> > > > 
> > > > However, not being an expert, reading your note now I suspect that I'm missing
> > > > something. Can I ask why you think that page-faults disabling might be
> > > > necessary?
> > > 
> > > I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> > > 
> > > I guess it will work since the copying is done like this anyway:
> > > 
> > > 		/*
> > > 		 * This is the fast path and we cannot handle a pagefault
> > > 		 * whilst holding the struct mutex lest the user pass in the
> > > 		 * relocations contained within a mmaped bo. For in such a case
> > > 		 * we, the page fault handler would call i915_gem_fault() and
> > > 		 * we would try to acquire the struct mutex again. Obviously
> > > 		 * this is bad and so lockdep complains vehemently.
> > > 		 */
> > > 		pagefault_disable();
> > > 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> > > 		pagefault_enable();
> > > 		if (unlikely(copied)) {
> > > 			remain = -EFAULT;
> > > 			goto out;
> > > 		}
> > > 
> > > Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> > 
> > Thanks for your explanation!
> > 
> > > 
> > > Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> > 
> > Okay, I will also look at this.
> > 
> > > 
> > > In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> > 
> > Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
> 
> Yes, but just because we failed to review and merge in time, not because you
> did not provide patches in time.

It is worth mentioning that under drm we close the merge window earlier.
Around -rc5.

So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
is going to be sent there is already closed.

> 
> Regards,
> 
> Tvrtko
> 

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Zhao Liu]

Hi Rodrigo and Tvrtko,

It seems this series is missed in v6.5.
This work should not be forgotten. Let me rebase and refresh the version.

Regards,
Zhao

On Mon, Apr 17, 2023 at 10:53:28AM -0400, Rodrigo Vivi wrote:
> Date: Mon, 17 Apr 2023 10:53:28 -0400
> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>  gem/i915_gem_execbuffer.c
> 
> On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 14/04/2023 11:45, Zhao Liu wrote:
> > > Hi Tvrtko,
> > > 
> > > On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > [snip]
> > > 
> > > > > 
> > > > > [snip]
> > > > > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > > > > Matt, being the last to touch this area, perhaps you could have a look?
> > > > > > Because I notice we have a fallback iomap path which still uses
> > > > > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > > > > safe, does the iomap side also needs converting to
> > > > > > io_mapping_map_local_wc? Or they have separate requirements?
> > > > > 
> > > > > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > > > > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > > > > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > > > > 
> > > > > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > > > > io_mapping_unmap_local_wc since the address is local to context.
> > > > > 
> > > > > However, not being an expert, reading your note now I suspect that I'm missing
> > > > > something. Can I ask why you think that page-faults disabling might be
> > > > > necessary?
> > > > 
> > > > I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> > > > 
> > > > I guess it will work since the copying is done like this anyway:
> > > > 
> > > > 		/*
> > > > 		 * This is the fast path and we cannot handle a pagefault
> > > > 		 * whilst holding the struct mutex lest the user pass in the
> > > > 		 * relocations contained within a mmaped bo. For in such a case
> > > > 		 * we, the page fault handler would call i915_gem_fault() and
> > > > 		 * we would try to acquire the struct mutex again. Obviously
> > > > 		 * this is bad and so lockdep complains vehemently.
> > > > 		 */
> > > > 		pagefault_disable();
> > > > 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> > > > 		pagefault_enable();
> > > > 		if (unlikely(copied)) {
> > > > 			remain = -EFAULT;
> > > > 			goto out;
> > > > 		}
> > > > 
> > > > Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> > > 
> > > Thanks for your explanation!
> > > 
> > > > 
> > > > Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> > > 
> > > Okay, I will also look at this.
> > > 
> > > > 
> > > > In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> > > 
> > > Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
> > 
> > Yes, but just because we failed to review and merge in time, not because you
> > did not provide patches in time.
> 
> It is worth mentioning that under drm we close the merge window earlier.
> Around -rc5.
> 
> So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
> is going to be sent there is already closed.
> 
> > 
> > Regards,
> > 
> > Tvrtko
> > 

Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Tvrtko Ursulin]

Hi,

On 18/10/2023 17:19, Zhao Liu wrote:
> Hi Rodrigo and Tvrtko,
> 
> It seems this series is missed in v6.5.
> This work should not be forgotten. Let me rebase and refresh the version.

Right it seems we did not manage to social engineer any reviews. Please 
do respin and we will try again.

Regards,

Tvrtko

> 
> Regards,
> Zhao
> 
> On Mon, Apr 17, 2023 at 10:53:28AM -0400, Rodrigo Vivi wrote:
>> Date: Mon, 17 Apr 2023 10:53:28 -0400
>> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>>   gem/i915_gem_execbuffer.c
>>
>> On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
>>>
>>> On 14/04/2023 11:45, Zhao Liu wrote:
>>>> Hi Tvrtko,
>>>>
>>>> On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> [snip]
>>>>
>>>>>>
>>>>>> [snip]
>>>>>>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>>>>>>> Matt, being the last to touch this area, perhaps you could have a look?
>>>>>>> Because I notice we have a fallback iomap path which still uses
>>>>>>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>>>>>>> safe, does the iomap side also needs converting to
>>>>>>> io_mapping_map_local_wc? Or they have separate requirements?
>>>>>>
>>>>>> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
>>>>>> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
>>>>>> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
>>>>>>
>>>>>> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
>>>>>> io_mapping_unmap_local_wc since the address is local to context.
>>>>>>
>>>>>> However, not being an expert, reading your note now I suspect that I'm missing
>>>>>> something. Can I ask why you think that page-faults disabling might be
>>>>>> necessary?
>>>>>
>>>>> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
>>>>>
>>>>> I guess it will work since the copying is done like this anyway:
>>>>>
>>>>> 		/*
>>>>> 		 * This is the fast path and we cannot handle a pagefault
>>>>> 		 * whilst holding the struct mutex lest the user pass in the
>>>>> 		 * relocations contained within a mmaped bo. For in such a case
>>>>> 		 * we, the page fault handler would call i915_gem_fault() and
>>>>> 		 * we would try to acquire the struct mutex again. Obviously
>>>>> 		 * this is bad and so lockdep complains vehemently.
>>>>> 		 */
>>>>> 		pagefault_disable();
>>>>> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
>>>>> 		pagefault_enable();
>>>>> 		if (unlikely(copied)) {
>>>>> 			remain = -EFAULT;
>>>>> 			goto out;
>>>>> 		}
>>>>>
>>>>> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
>>>>
>>>> Thanks for your explanation!
>>>>
>>>>>
>>>>> Patch to convert the io_mapping_map_atomic_wc can indeed come later.
>>>>
>>>> Okay, I will also look at this.
>>>>
>>>>>
>>>>> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
>>>>
>>>> Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
>>>
>>> Yes, but just because we failed to review and merge in time, not because you
>>> did not provide patches in time.
>>
>> It is worth mentioning that under drm we close the merge window earlier.
>> Around -rc5.
>>
>> So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
>> is going to be sent there is already closed.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Replace kmap_atomic() with kmap_local_page()

[Patchwork]

== Series Details ==

Series: drm/i915: Replace kmap_atomic() with kmap_local_page()
URL   : https://patchwork.freedesktop.org/series/115769/
State : warning

== Summary ==

Error: dim checkpatch failed
9bed0825b968 drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
-:39: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#39: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

-:68: ERROR:CODE_INDENT: code indent should use tabs where possible
#68: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.c:434:
+^I          + offset_in_page(offset);$

total: 1 errors, 1 warnings, 0 checks, 20 lines checked
d3723eb0a5a9 drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
-:39: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#39: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 34 lines checked
a6ad1bedd102 drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
-:24: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#24: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 15 lines checked
140ac9e589f6 drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
-:36: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#36: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 22 lines checked
8ca8fe8829f9 drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
-:37: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#37: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 48 lines checked
3fe0f41f6073 drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
-:38: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#38: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 30 lines checked
1a1713ebe97f drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
-:29: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#29: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com/T/#u

total: 0 errors, 1 warnings, 0 checks, 17 lines checked
9cc672fadd3c drm/i915: Use kmap_local_page() in i915_cmd_parser.c
-:34: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#34: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 13 lines checked
69cab3363bd2 drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
-:34: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#34: 
[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com

total: 0 errors, 1 warnings, 0 checks, 40 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Replace kmap_atomic() with kmap_local_page()

[Patchwork]

== Series Details ==

Series: drm/i915: Replace kmap_atomic() with kmap_local_page()
URL   : https://patchwork.freedesktop.org/series/115769/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Replace kmap_atomic() with kmap_local_page()

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6123 bytes --]

== Series Details ==

Series: drm/i915: Replace kmap_atomic() with kmap_local_page()
URL   : https://patchwork.freedesktop.org/series/115769/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12931 -> Patchwork_115769v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/index.html

Participating hosts (36 -> 37)
------------------------------

  Additional (1): fi-kbl-soraka 

Known issues
------------

  Here are the changes found in Patchwork_115769v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-skl-guc:         NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#4613]) +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-skl-guc/igt@gem_lmem_swapping@basic.html
    - fi-kbl-soraka:      NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) +3 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_pm_rpm@module-reload:
    - bat-jsl-1:          [PASS][4] -> [INCOMPLETE][5] ([i915#6866])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/bat-jsl-1/igt@i915_pm_rpm@module-reload.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/bat-jsl-1/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][6] ([i915#1886])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [PASS][7] -> [ABORT][8] ([i915#7911] / [i915#7982])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/bat-rpls-1/igt@i915_selftest@live@requests.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         [PASS][9] -> [ABORT][10] ([i915#4983] / [i915#7913])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/bat-rpls-2/igt@i915_selftest@live@reset.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/bat-rpls-2/igt@i915_selftest@live@reset.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][11] ([fdo#109271]) +16 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][12] ([i915#5354]) +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][13] -> [FAIL][14] ([i915#7932]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-skl-guc:         NOTRUN -> [SKIP][15] ([fdo#109271]) +19 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-skl-guc/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_module_load@load:
    - fi-skl-guc:         [ABORT][16] ([i915#8189]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/fi-skl-guc/igt@i915_module_load@load.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/fi-skl-guc/igt@i915_module_load@load.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6866]: https://gitlab.freedesktop.org/drm/intel/issues/6866
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7982]: https://gitlab.freedesktop.org/drm/intel/issues/7982
  [i915#8189]: https://gitlab.freedesktop.org/drm/intel/issues/8189


Build changes
-------------

  * Linux: CI_DRM_12931 -> Patchwork_115769v1

  CI-20190529: 20190529
  CI_DRM_12931: e1b8055e62c6f94ef94db3e7f125704ac0fab0b5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7223: 2cbfa210fc95d126edf9a60ae6ab4e96cf4fca7f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115769v1: e1b8055e62c6f94ef94db3e7f125704ac0fab0b5 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

84593c4aab99 drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
a3896445fd7e drm/i915: Use kmap_local_page() in i915_cmd_parser.c
80657d64dab1 drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
543115809263 drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
b763f964facf drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
81a8cd576dc2 drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
e3fdcab4f639 drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
84a5e524797e drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
b9d1e6d4b37d drm/i915: Use kmap_local_page() in gem/i915_gem_object.c

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/index.html

[-- Attachment #2: Type: text/html, Size: 7382 bytes --]

Re: [Intel-gfx] [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()

[Fabio M. De Francesco]

On mercoledì 29 marzo 2023 09:32:11 CEST Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Hi list,
> 
> Sorry for a long delay since v1 [1]. This patchset is based on 197b6b6
> (Linux 6.3-rc4).
> 
> Welcome and thanks for your review and comments!
> 
> 
> # Purpose of this patchset
> 
> The purpose of this pacthset is to replace all uses of kmap_atomic() in
> i915 with kmap_local_page() because the use of kmap_atomic() is being
> deprecated in favor of kmap_local_page()[1]. And 92b64bd (mm/highmem:
> add notes about conversions from kmap{,_atomic}()) has declared the
> deprecation of kmap_atomic().
> 
> 
> # Motivation for deprecating kmap_atomic() and using kmap_local_page()
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> 
> # Patch summary
> 
> Patch 1, 4-6 and 8-9 replace kamp_atomic()/kunmap_atomic() with
>         kmap_local_page()/kunmap_local() directly. With thses local
>         mappings, the page faults and preemption are allowed.
> 
> Patch 2 and 7 use memcpy_from_page() and memcpy_to_page() to replace
>         kamp_atomic()/kunmap_atomic(). These two variants of memcpy()
>         are based on the local mapping, so page faults and preemption
>         are also allowed in these two interfaces.
> 
> Patch 3 replaces kamp_atomic()/kunmap_atomic() with kmap_local_page()/
>         kunmap_local() and also diable page fault since the for special
>         handling (pls see the commit message).
> 
> 
> # Changes since v1
> 
> * Dropped hot plug related description in commit message since it has
>   nothing to do with kmap_local_page().
> * Emphasized the motivation for using kmap_local_page() in commit
>   message.
> * Rebased patch 1 on f47e630 (drm/i915/gem: Typecheck page lookups) to
>   keep the "idx" variable of type pgoff_t here.
> * Used memcpy_from_page() and memcpy_to_page() to replace
>   kmap_local_page() + memcpy() in patch 2.
> 
> 
> # Reference
> 
> [1]:
> https://lore.kernel.org/lkml/20221017093726.2070674-1-zhao1.liu@linux.intel.c
> om/ [1]:
> https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com ---
> Zhao Liu (9):
>   drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
>   drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
>   drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
>   drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
>   drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
>   drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
>   drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
>   drm/i915: Use kmap_local_page() in i915_cmd_parser.c
>   drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
> 

I _think_ that the "long delay" you mentioned in the first sentence has paid 
off in full. 

I don't see things to improve (except all those "kamp_atomic()" typo in the 
patches summary; however, typos are only in the cover so I'm sure they won't 
hurt anybody). 

Each of the nine patches listed above looks good to me, so they are all…

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks!

Fabio

PS: Obviously there was no need to reconfirm my tag for patch 3/9. A single 
tag that catches all patches is easier for a lazy person like me :-)

>
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c       | 10 +++++-----
>  drivers/gpu/drm/i915/gem/i915_gem_object.c           |  8 +++-----
>  drivers/gpu/drm/i915/gem/i915_gem_phys.c             | 10 ++--------
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c            |  6 ++++--
>  drivers/gpu/drm/i915/gem/selftests/huge_pages.c      |  6 +++---
>  .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c  | 12 ++++--------
>  .../gpu/drm/i915/gem/selftests/i915_gem_context.c    |  8 ++++----
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             |  5 +----
>  drivers/gpu/drm/i915/i915_cmd_parser.c               |  4 ++--
>  9 files changed, 28 insertions(+), 41 deletions(-)
> 
> --
> 2.34.1

Re: [Intel-gfx] [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()

[Zhao Liu]

Hi Fabio,

On Wed, Mar 29, 2023 at 06:03:38PM +0200, Fabio M. De Francesco wrote:
> Date: Wed, 29 Mar 2023 18:03:38 +0200
> From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
> Subject: Re: [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with
>  kmap_local_page()
> 
> On mercoledì 29 marzo 2023 09:32:11 CEST Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Hi list,
> > 
> > Sorry for a long delay since v1 [1]. This patchset is based on 197b6b6
> > (Linux 6.3-rc4).
> > 
> > Welcome and thanks for your review and comments!
> > 
> > 
> > # Purpose of this patchset
> > 
> > The purpose of this pacthset is to replace all uses of kmap_atomic() in
> > i915 with kmap_local_page() because the use of kmap_atomic() is being
> > deprecated in favor of kmap_local_page()[1]. And 92b64bd (mm/highmem:
> > add notes about conversions from kmap{,_atomic}()) has declared the
> > deprecation of kmap_atomic().
> > 
> > 
> > # Motivation for deprecating kmap_atomic() and using kmap_local_page()
> > 
> > The main difference between atomic and local mappings is that local
> > mappings doesn't disable page faults or preemption (the preemption is
> > disabled for !PREEMPT_RT case, otherwise it only disables migration).
> > 
> > With kmap_local_page(), we can avoid the often unwanted side effect of
> > unnecessary page faults and preemption disables.
> > 
> > 
> > # Patch summary
> > 
> > Patch 1, 4-6 and 8-9 replace kamp_atomic()/kunmap_atomic() with
> >         kmap_local_page()/kunmap_local() directly. With thses local
> >         mappings, the page faults and preemption are allowed.
> > 
> > Patch 2 and 7 use memcpy_from_page() and memcpy_to_page() to replace
> >         kamp_atomic()/kunmap_atomic(). These two variants of memcpy()
> >         are based on the local mapping, so page faults and preemption
> >         are also allowed in these two interfaces.
> > 
> > Patch 3 replaces kamp_atomic()/kunmap_atomic() with kmap_local_page()/
> >         kunmap_local() and also diable page fault since the for special
> >         handling (pls see the commit message).
> > 
> > 
> > # Changes since v1
> > 
> > * Dropped hot plug related description in commit message since it has
> >   nothing to do with kmap_local_page().
> > * Emphasized the motivation for using kmap_local_page() in commit
> >   message.
> > * Rebased patch 1 on f47e630 (drm/i915/gem: Typecheck page lookups) to
> >   keep the "idx" variable of type pgoff_t here.
> > * Used memcpy_from_page() and memcpy_to_page() to replace
> >   kmap_local_page() + memcpy() in patch 2.
> > 
> > 
> > # Reference
> > 
> > [1]:
> > https://lore.kernel.org/lkml/20221017093726.2070674-1-zhao1.liu@linux.intel.c
> > om/ [1]:
> > https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com ---
> > Zhao Liu (9):
> >   drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
> >   drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
> >   drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
> >   drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
> >   drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
> >   drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
> >   drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
> >   drm/i915: Use kmap_local_page() in i915_cmd_parser.c
> >   drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
> > 
> 
> I _think_ that the "long delay" you mentioned in the first sentence has paid 
> off in full. 
> 
> I don't see things to improve (except all those "kamp_atomic()" typo in the 
> patches summary; however, typos are only in the cover so I'm sure they won't 
> hurt anybody). 

Thanks a lot for your patience and your help! :-)

> 
> Each of the nine patches listed above looks good to me, so they are all…
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Thanks!
> 
> Fabio
> 
> PS: Obviously there was no need to reconfirm my tag for patch 3/9. A single 
> tag that catches all patches is easier for a lazy person like me :-)

The typos and this description still can be improved. I'll pay
attention in the future!

Thanks,
Zhao

> 
> >
> >  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c       | 10 +++++-----
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c           |  8 +++-----
> >  drivers/gpu/drm/i915/gem/i915_gem_phys.c             | 10 ++--------
> >  drivers/gpu/drm/i915/gem/i915_gem_shmem.c            |  6 ++++--
> >  drivers/gpu/drm/i915/gem/selftests/huge_pages.c      |  6 +++---
> >  .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c  | 12 ++++--------
> >  .../gpu/drm/i915/gem/selftests/i915_gem_context.c    |  8 ++++----
> >  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             |  5 +----
> >  drivers/gpu/drm/i915/i915_cmd_parser.c               |  4 ++--
> >  9 files changed, 28 insertions(+), 41 deletions(-)
> > 
> > --
> > 2.34.1
> 
> 
> 
> 

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Replace kmap_atomic() with kmap_local_page()

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12050 bytes --]

== Series Details ==

Series: drm/i915: Replace kmap_atomic() with kmap_local_page()
URL   : https://patchwork.freedesktop.org/series/115769/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12931_full -> Patchwork_115769v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_115769v1_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_big_fb@x-tiled-8bpp-rotate-270:
    - {shard-tglu}:       [SKIP][1] ([fdo#111614]) -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-tglu-7/igt@kms_big_fb@x-tiled-8bpp-rotate-270.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-tglu-3/igt@kms_big_fb@x-tiled-8bpp-rotate-270.html

  
Known issues
------------

  Here are the changes found in Patchwork_115769v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-glk9/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk2/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [PASS][5] -> [FAIL][6] ([i915#2842])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-apl3/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-apl6/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - shard-glk:          NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#4613])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk8/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@kms_ccs@pipe-b-bad-rotation-90-y_tiled_gen12_rc_ccs_cc:
    - shard-glk:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#3886])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk8/igt@kms_ccs@pipe-b-bad-rotation-90-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][9] -> [FAIL][10] ([i915#79])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk9/igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@flip-vs-suspend-interruptible@a-dp1:
    - shard-apl:          [PASS][11] -> [ABORT][12] ([i915#180])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-apl6/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html

  * igt@kms_flip_scaled_crc@flip-64bpp-yftile-to-16bpp-yftile-upscaling@pipe-a-valid-mode:
    - shard-glk:          NOTRUN -> [SKIP][13] ([fdo#109271]) +52 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk8/igt@kms_flip_scaled_crc@flip-64bpp-yftile-to-16bpp-yftile-upscaling@pipe-a-valid-mode.html

  * igt@kms_psr2_su@frontbuffer-xrgb8888:
    - shard-glk:          NOTRUN -> [SKIP][14] ([fdo#109271] / [i915#658])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk8/igt@kms_psr2_su@frontbuffer-xrgb8888.html

  
#### Possible fixes ####

  * igt@gem_exec_endless@dispatch@vcs1:
    - {shard-tglu}:       [TIMEOUT][15] ([i915#3778]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-tglu-9/igt@gem_exec_endless@dispatch@vcs1.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-tglu-6/igt@gem_exec_endless@dispatch@vcs1.html

  * igt@gem_lmem_swapping@smem-oom@lmem0:
    - {shard-dg1}:        [DMESG-WARN][17] ([i915#4936]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-dg1-16/igt@gem_lmem_swapping@smem-oom@lmem0.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-dg1-15/igt@gem_lmem_swapping@smem-oom@lmem0.html

  * igt@kms_addfb_basic@size-max:
    - {shard-dg1}:        [DMESG-WARN][19] -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-dg1-17/igt@kms_addfb_basic@size-max.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-dg1-18/igt@kms_addfb_basic@size-max.html

  * igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][21] ([i915#2122]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-glk3/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ac-hdmi-a1-hdmi-a2.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk1/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ac-hdmi-a1-hdmi-a2.html

  * {igt@perf@stress-open-close@0-rcs0}:
    - shard-glk:          [ABORT][23] ([i915#5213]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12931/shard-glk3/igt@perf@stress-open-close@0-rcs0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/shard-glk8/igt@perf@stress-open-close@0-rcs0.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3778]: https://gitlab.freedesktop.org/drm/intel/issues/3778
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4936]: https://gitlab.freedesktop.org/drm/intel/issues/4936
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5213]: https://gitlab.freedesktop.org/drm/intel/issues/5213
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6755]: https://gitlab.freedesktop.org/drm/intel/issues/6755
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8247]: https://gitlab.freedesktop.org/drm/intel/issues/8247


Build changes
-------------

  * Linux: CI_DRM_12931 -> Patchwork_115769v1

  CI-20190529: 20190529
  CI_DRM_12931: e1b8055e62c6f94ef94db3e7f125704ac0fab0b5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7223: 2cbfa210fc95d126edf9a60ae6ab4e96cf4fca7f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115769v1: e1b8055e62c6f94ef94db3e7f125704ac0fab0b5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115769v1/index.html

[-- Attachment #2: Type: text/html, Size: 8510 bytes --]