* [PATCH V2] drm/i915: Replace kmap() with kmap_local_page()
2021-12-10 23:23 [PATCH 1/7] " ira.weiny
@ 2021-12-22 6:08 ` ira.weiny
0 siblings, 0 replies; 15+ messages in thread
From: ira.weiny @ 2021-12-22 6:08 UTC (permalink / raw
To: David Airlie, Daniel Vetter, Patrik Jakobsson, Rob Clark,
Sean Paul
Cc: Ira Weiny, amd-gfx, dri-devel, linux-kernel, intel-gfx,
linux-arm-msm
From: Ira Weiny <ira.weiny@intel.com>
kmap() is being deprecated and these usages are all local to the thread
so there is no reason kmap_local_page() can't be used.
Replace kmap() calls with kmap_local_page().
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
NOTE: I'm sending as a follow on to the V1 patch. Please let me know if you
prefer the entire series instead.
Changes for V2:
From Christoph Helwig
Prefer the use of memcpy_*_page() where appropriate.
---
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++----
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 ++++----
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
drivers/gpu/drm/i915/gt/shmem_utils.c | 7 ++-----
drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
6 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index d77da59fae04..842e089aaaa5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -589,7 +589,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
do {
unsigned int len = min_t(typeof(size), size, PAGE_SIZE);
struct page *page;
- void *pgdata, *vaddr;
+ void *pgdata;
err = pagecache_write_begin(file, file->f_mapping,
offset, len, 0,
@@ -597,9 +597,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
if (err < 0)
goto fail;
- vaddr = kmap(page);
- memcpy(vaddr, data, len);
- kunmap(page);
+ memcpy_to_page(page, 0, data, len);
err = pagecache_write_end(file, file->f_mapping,
offset, len, len,
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 6d30cdfa80f3..e59e1725e29d 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -144,7 +144,7 @@ static int check_partial_mapping(struct drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(&to_i915(obj->base.dev)->gt);
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -162,7 +162,7 @@ static int check_partial_mapping(struct drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
out:
__i915_vma_put(vma);
@@ -237,7 +237,7 @@ static int check_partial_mappings(struct drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(&to_i915(obj->base.dev)->gt);
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -255,7 +255,7 @@ static int check_partial_mappings(struct drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
if (err)
return err;
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
index f8948de72036..743a414f86f3 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
@@ -743,7 +743,7 @@ static void swizzle_page(struct page *page)
char *vaddr;
int i;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
for (i = 0; i < PAGE_SIZE; i += 128) {
memcpy(temp, &vaddr[i], 64);
@@ -751,7 +751,7 @@ static void swizzle_page(struct page *page)
memcpy(&vaddr[i + 64], temp, 64);
}
- kunmap(page);
+ kunmap_local(vaddr);
}
/**
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c b/drivers/gpu/drm/i915/gt/shmem_utils.c
index 0683b27a3890..d47f262d2f07 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c
@@ -97,22 +97,19 @@ static int __shmem_rw(struct file *file, loff_t off,
unsigned int this =
min_t(size_t, PAGE_SIZE - offset_in_page(off), len);
struct page *page;
- void *vaddr;
page = shmem_read_mapping_page_gfp(file->f_mapping, pfn,
GFP_KERNEL);
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
if (write) {
- memcpy(vaddr + offset_in_page(off), ptr, this);
+ memcpy_to_page(page, offset_in_page(off), ptr, this);
set_page_dirty(page);
} else {
- memcpy(ptr, vaddr + offset_in_page(off), this);
+ memcpy_from_page(ptr, page, offset_in_page(off), this);
}
mark_page_accessed(page);
- kunmap(page);
put_page(page);
len -= this;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 981e383d1a5d..af5adb187ca4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -196,14 +196,14 @@ shmem_pread(struct page *page, int offset, int len, char __user *user_data,
char *vaddr;
int ret;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
if (needs_clflush)
drm_clflush_virt_range(vaddr + offset, len);
ret = __copy_to_user(user_data, vaddr + offset, len);
- kunmap(page);
+ kunmap_local(vaddr);
return ret ? -EFAULT : 0;
}
@@ -618,7 +618,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data,
char *vaddr;
int ret;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
if (needs_clflush_before)
drm_clflush_virt_range(vaddr + offset, len);
@@ -627,7 +627,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data,
if (!ret && needs_clflush_after)
drm_clflush_virt_range(vaddr + offset, len);
- kunmap(page);
+ kunmap_local(vaddr);
return ret ? -EFAULT : 0;
}
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2a2d7643b551..c526d7892081 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1094,9 +1094,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
drm_clflush_pages(&page, 1);
- s = kmap(page);
+ s = kmap_local_page(page);
ret = compress_page(compress, s, dst, false);
- kunmap(page);
+ kunmap_local(s);
drm_clflush_pages(&page, 1);
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
@ 2023-06-17 18:04 Sumitra Sharma
2023-06-18 18:11 ` Ira Weiny
2023-06-24 0:10 ` Fabio M. De Francesco
0 siblings, 2 replies; 15+ messages in thread
From: Sumitra Sharma @ 2023-06-17 18:04 UTC (permalink / raw
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel
Cc: Ira Weiny, Fabio, Deepak R Varma, Sumitra Sharma
kmap() has been deprecated in favor of the kmap_local_page()
due to high cost, restricted mapping space, the overhead of a
global lock for synchronization, and making the process sleep
in the absence of free slots.
kmap_local_page() is faster than kmap() and offers thread-local
and CPU-local mappings, take pagefaults in a local kmap region
and preserves preemption by saving the mappings of outgoing tasks
and restoring those of the incoming one during a context switch.
The mapping is kept thread local in the function
“i915_vma_coredump_create” in i915_gpu_error.c
Therefore, replace kmap() with kmap_local_page().
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---
Changes in v2:
- Replace kmap() with kmap_local_page().
- Change commit subject and message.
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index f020c0086fbc..bc41500eedf5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
drm_clflush_pages(&page, 1);
- s = kmap(page);
+ s = kmap_local_page(page);
ret = compress_page(compress, s, dst, false);
- kunmap(page);
+ kunmap_local(s);
drm_clflush_pages(&page, 1);
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-17 18:04 [PATCH v2] drm/i915: Replace kmap() with kmap_local_page() Sumitra Sharma
@ 2023-06-18 18:11 ` Ira Weiny
2023-06-19 7:59 ` Thomas Hellström (Intel)
2023-06-19 15:45 ` Sumitra Sharma
2023-06-24 0:10 ` Fabio M. De Francesco
1 sibling, 2 replies; 15+ messages in thread
From: Ira Weiny @ 2023-06-18 18:11 UTC (permalink / raw
To: Sumitra Sharma, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Daniel Vetter, intel-gfx, dri-devel,
linux-kernel, Thomas Hellström (Intel)
Cc: Ira Weiny, Fabio, Deepak R Varma, Sumitra Sharma
Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of a
> global lock for synchronization, and making the process sleep
> in the absence of free slots.
>
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region
> and preserves preemption by saving the mappings of outgoing tasks
> and restoring those of the incoming one during a context switch.
>
> The mapping is kept thread local in the function
> “i915_vma_coredump_create” in i915_gpu_error.c
>
> Therefore, replace kmap() with kmap_local_page().
>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>
NIT: No need for the line break between Suggested-by and your signed off line.
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
>
> Changes in v2:
> - Replace kmap() with kmap_local_page().
Generally it is customary to attribute a change like this to those who
suggested it in a V1 review.
For example:
- Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
Also I don't see Thomas on the new email list. Since he took the time to
review V1 he might want to check this version out. I've added him to the
'To:' list.
Also a link to V1 is nice. B4 formats it like this:
- Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
All that said the code looks good to me. So with the above changes.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> - Change commit subject and message.
>
> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index f020c0086fbc..bc41500eedf5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>
> drm_clflush_pages(&page, 1);
>
> - s = kmap(page);
> + s = kmap_local_page(page);
> ret = compress_page(compress, s, dst, false);
> - kunmap(page);
> + kunmap_local(s);
>
> drm_clflush_pages(&page, 1);
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-18 18:11 ` Ira Weiny
@ 2023-06-19 7:59 ` Thomas Hellström (Intel)
2023-06-19 8:44 ` Tvrtko Ursulin
2023-06-19 15:45 ` Sumitra Sharma
1 sibling, 1 reply; 15+ messages in thread
From: Thomas Hellström (Intel) @ 2023-06-19 7:59 UTC (permalink / raw
To: Ira Weiny, Sumitra Sharma, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
intel-gfx, dri-devel, linux-kernel
Cc: Fabio, Deepak R Varma
On 6/18/23 20:11, Ira Weiny wrote:
> Sumitra Sharma wrote:
>> kmap() has been deprecated in favor of the kmap_local_page()
>> due to high cost, restricted mapping space, the overhead of a
>> global lock for synchronization, and making the process sleep
>> in the absence of free slots.
>>
>> kmap_local_page() is faster than kmap() and offers thread-local
>> and CPU-local mappings, take pagefaults in a local kmap region
>> and preserves preemption by saving the mappings of outgoing tasks
>> and restoring those of the incoming one during a context switch.
>>
>> The mapping is kept thread local in the function
>> “i915_vma_coredump_create” in i915_gpu_error.c
>>
>> Therefore, replace kmap() with kmap_local_page().
>>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>
> NIT: No need for the line break between Suggested-by and your signed off line.
>
>> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
>> ---
>>
>> Changes in v2:
>> - Replace kmap() with kmap_local_page().
> Generally it is customary to attribute a change like this to those who
> suggested it in a V1 review.
>
> For example:
>
> - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
>
> Also I don't see Thomas on the new email list. Since he took the time to
> review V1 he might want to check this version out. I've added him to the
> 'To:' list.
Thanks.
> Also a link to V1 is nice. B4 formats it like this:
>
> - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
>
> All that said the code looks good to me. So with the above changes.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
LGTM. Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>
>> - Change commit subject and message.
>>
>> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index f020c0086fbc..bc41500eedf5 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>>
>> drm_clflush_pages(&page, 1);
>>
>> - s = kmap(page);
>> + s = kmap_local_page(page);
>> ret = compress_page(compress, s, dst, false);
>> - kunmap(page);
>> + kunmap_local(s);
>>
>> drm_clflush_pages(&page, 1);
>>
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-19 7:59 ` Thomas Hellström (Intel)
@ 2023-06-19 8:44 ` Tvrtko Ursulin
0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-06-19 8:44 UTC (permalink / raw
To: Thomas Hellström (Intel), Ira Weiny, Sumitra Sharma,
Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, intel-gfx, dri-devel, linux-kernel
Cc: Fabio, Deepak R Varma
On 19/06/2023 08:59, Thomas Hellström (Intel) wrote:
>
> On 6/18/23 20:11, Ira Weiny wrote:
>> Sumitra Sharma wrote:
>>> kmap() has been deprecated in favor of the kmap_local_page()
>>> due to high cost, restricted mapping space, the overhead of a
>>> global lock for synchronization, and making the process sleep
>>> in the absence of free slots.
>>>
>>> kmap_local_page() is faster than kmap() and offers thread-local
>>> and CPU-local mappings, take pagefaults in a local kmap region
>>> and preserves preemption by saving the mappings of outgoing tasks
>>> and restoring those of the incoming one during a context switch.
>>>
>>> The mapping is kept thread local in the function
>>> “i915_vma_coredump_create” in i915_gpu_error.c
>>>
>>> Therefore, replace kmap() with kmap_local_page().
>>>
>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>>
>> NIT: No need for the line break between Suggested-by and your signed
>> off line.
>>
>>> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Replace kmap() with kmap_local_page().
>> Generally it is customary to attribute a change like this to those who
>> suggested it in a V1 review.
>>
>> For example:
>>
>> - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
>>
>> Also I don't see Thomas on the new email list. Since he took the time to
>> review V1 he might want to check this version out. I've added him to the
>> 'To:' list.
>
> Thanks.
>
>
>> Also a link to V1 is nice. B4 formats it like this:
>>
>> - Link to v1:
>> https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
>>
>> All that said the code looks good to me. So with the above changes.
>>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
> LGTM. Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Thanks all! I'll just re-send the patch for our CI, since it didn't get
picked up automatically (stuck in moderation perhaps), with all r-b tags
added and extra line space removed and merge it if results will be green.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-18 18:11 ` Ira Weiny
2023-06-19 7:59 ` Thomas Hellström (Intel)
@ 2023-06-19 15:45 ` Sumitra Sharma
2023-06-20 13:23 ` Ira Weiny
1 sibling, 1 reply; 15+ messages in thread
From: Sumitra Sharma @ 2023-06-19 15:45 UTC (permalink / raw
To: Ira Weiny
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Thomas Hellström (Intel), Fabio, Deepak R Varma,
Sumitra Sharma
On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
> Sumitra Sharma wrote:
> > kmap() has been deprecated in favor of the kmap_local_page()
> > due to high cost, restricted mapping space, the overhead of a
> > global lock for synchronization, and making the process sleep
> > in the absence of free slots.
> >
> > kmap_local_page() is faster than kmap() and offers thread-local
> > and CPU-local mappings, take pagefaults in a local kmap region
> > and preserves preemption by saving the mappings of outgoing tasks
> > and restoring those of the incoming one during a context switch.
> >
> > The mapping is kept thread local in the function
> > “i915_vma_coredump_create” in i915_gpu_error.c
> >
> > Therefore, replace kmap() with kmap_local_page().
> >
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> >
>
> NIT: No need for the line break between Suggested-by and your signed off line.
>
Hi Ira,
What does NIT stand for?
Thank you. I will take care about the line breaks.
> > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > ---
> >
> > Changes in v2:
> > - Replace kmap() with kmap_local_page().
>
> Generally it is customary to attribute a change like this to those who
> suggested it in a V1 review.
>
> For example:
>
> - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
>
> Also I don't see Thomas on the new email list. Since he took the time to
> review V1 he might want to check this version out. I've added him to the
> 'To:' list.
>
> Also a link to V1 is nice. B4 formats it like this:
>
> - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
>
> All that said the code looks good to me. So with the above changes.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
I have noted down the points mentioned above. Thank you again.
I am not supposed to create another version of this patch for
adding the above mentions, as you and Thomas both gave this patch
a reviewed-by tag. Right?
Thanks & regards
Sumitra
PS: I am new to the open source vocabulary terms.
> > - Change commit subject and message.
> >
> > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index f020c0086fbc..bc41500eedf5 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
> >
> > drm_clflush_pages(&page, 1);
> >
> > - s = kmap(page);
> > + s = kmap_local_page(page);
> > ret = compress_page(compress, s, dst, false);
> > - kunmap(page);
> > + kunmap_local(s);
> >
> > drm_clflush_pages(&page, 1);
> >
> > --
> > 2.25.1
> >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-19 15:45 ` Sumitra Sharma
@ 2023-06-20 13:23 ` Ira Weiny
2023-06-20 18:07 ` Sumitra Sharma
2023-06-26 9:02 ` Tvrtko Ursulin
0 siblings, 2 replies; 15+ messages in thread
From: Ira Weiny @ 2023-06-20 13:23 UTC (permalink / raw
To: Sumitra Sharma, Ira Weiny
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Thomas Hellström (Intel), Fabio, Deepak R Varma,
Sumitra Sharma
Sumitra Sharma wrote:
> On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
> > Sumitra Sharma wrote:
> > > kmap() has been deprecated in favor of the kmap_local_page()
> > > due to high cost, restricted mapping space, the overhead of a
> > > global lock for synchronization, and making the process sleep
> > > in the absence of free slots.
> > >
> > > kmap_local_page() is faster than kmap() and offers thread-local
> > > and CPU-local mappings, take pagefaults in a local kmap region
> > > and preserves preemption by saving the mappings of outgoing tasks
> > > and restoring those of the incoming one during a context switch.
> > >
> > > The mapping is kept thread local in the function
> > > “i915_vma_coredump_create” in i915_gpu_error.c
> > >
> > > Therefore, replace kmap() with kmap_local_page().
> > >
> > > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > >
> >
> > NIT: No need for the line break between Suggested-by and your signed off line.
> >
>
> Hi Ira,
>
> What does NIT stand for?
Shorthand for 'nitpicking'.
"giving too much attention to details that are not important, especially
as a way of criticizing: "
- https://dictionary.cambridge.org/dictionary/english/nitpicking
Via email this is a way for authors of an email to indicate something is
technically wrong but while nicely acknowledging that it is not very
significant and could be seen as overly critical.
For this particular comment I'm showing something to pay attention to next
time but that was not a big deal this time around.
>
> Thank you. I will take care about the line breaks.
>
> > > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Replace kmap() with kmap_local_page().
> >
> > Generally it is customary to attribute a change like this to those who
> > suggested it in a V1 review.
> >
> > For example:
> >
> > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
> >
> > Also I don't see Thomas on the new email list. Since he took the time to
> > review V1 he might want to check this version out. I've added him to the
> > 'To:' list.
> >
> > Also a link to V1 is nice. B4 formats it like this:
> >
> > - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
> >
> > All that said the code looks good to me. So with the above changes.
> >
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> >
>
> I have noted down the points mentioned above. Thank you again.
>
> I am not supposed to create another version of this patch for
> adding the above mentions, as you and Thomas both gave this patch
> a reviewed-by tag. Right?
>
Based on this response[*] from Tvrtko I think this version can move
through without a v3.
Thanks!
Ira
[*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7b38@linux.intel.com/
<quote>
Thanks all! I'll just re-send the patch for our CI, since it didn't get
picked up automatically (stuck in moderation perhaps), with all r-b tags
added and extra line space removed and merge it if results will be green.
Regards,
Tvrtko
</quote>
>
> Thanks & regards
> Sumitra
>
> PS: I am new to the open source vocabulary terms.
>
> > > - Change commit subject and message.
> > >
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index f020c0086fbc..bc41500eedf5 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
> > >
> > > drm_clflush_pages(&page, 1);
> > >
> > > - s = kmap(page);
> > > + s = kmap_local_page(page);
> > > ret = compress_page(compress, s, dst, false);
> > > - kunmap(page);
> > > + kunmap_local(s);
> > >
> > > drm_clflush_pages(&page, 1);
> > >
> > > --
> > > 2.25.1
> > >
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-20 13:23 ` Ira Weiny
@ 2023-06-20 18:07 ` Sumitra Sharma
2023-06-21 9:06 ` Thomas Hellström (Intel)
2023-06-26 9:02 ` Tvrtko Ursulin
1 sibling, 1 reply; 15+ messages in thread
From: Sumitra Sharma @ 2023-06-20 18:07 UTC (permalink / raw
To: Ira Weiny
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Thomas Hellström (Intel), Fabio, Deepak R Varma,
Sumitra Sharma
On Tue, Jun 20, 2023 at 06:23:38AM -0700, Ira Weiny wrote:
> Sumitra Sharma wrote:
> > On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
> > > Sumitra Sharma wrote:
> > > > kmap() has been deprecated in favor of the kmap_local_page()
> > > > due to high cost, restricted mapping space, the overhead of a
> > > > global lock for synchronization, and making the process sleep
> > > > in the absence of free slots.
> > > >
> > > > kmap_local_page() is faster than kmap() and offers thread-local
> > > > and CPU-local mappings, take pagefaults in a local kmap region
> > > > and preserves preemption by saving the mappings of outgoing tasks
> > > > and restoring those of the incoming one during a context switch.
> > > >
> > > > The mapping is kept thread local in the function
> > > > “i915_vma_coredump_create” in i915_gpu_error.c
> > > >
> > > > Therefore, replace kmap() with kmap_local_page().
> > > >
> > > > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > > >
> > >
> > > NIT: No need for the line break between Suggested-by and your signed off line.
> > >
> >
> > Hi Ira,
> >
> > What does NIT stand for?
>
> Shorthand for 'nitpicking'.
>
> "giving too much attention to details that are not important, especially
> as a way of criticizing: "
>
> - https://dictionary.cambridge.org/dictionary/english/nitpicking
>
> Via email this is a way for authors of an email to indicate something is
> technically wrong but while nicely acknowledging that it is not very
> significant and could be seen as overly critical.
>
> For this particular comment I'm showing something to pay attention to next
> time but that was not a big deal this time around.
>
Hi Ira,
Thank for your explanation on NIT.
> >
> > Thank you. I will take care about the line breaks.
> >
> > > > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Replace kmap() with kmap_local_page().
> > >
> > > Generally it is customary to attribute a change like this to those who
> > > suggested it in a V1 review.
> > >
> > > For example:
> > >
> > > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
> > >
> > > Also I don't see Thomas on the new email list. Since he took the time to
> > > review V1 he might want to check this version out. I've added him to the
> > > 'To:' list.
> > >
> > > Also a link to V1 is nice. B4 formats it like this:
> > >
> > > - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
> > >
> > > All that said the code looks good to me. So with the above changes.
> > >
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > >
> >
> > I have noted down the points mentioned above. Thank you again.
> >
> > I am not supposed to create another version of this patch for
> > adding the above mentions, as you and Thomas both gave this patch
> > a reviewed-by tag. Right?
> >
>
> Based on this response[*] from Tvrtko I think this version can move
> through without a v3.
Okay!
Thanks & regards
Sumitra
>
> Thanks!
> Ira
>
> [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7b38@linux.intel.com/
>
> <quote>
> Thanks all! I'll just re-send the patch for our CI, since it didn't get
> picked up automatically (stuck in moderation perhaps), with all r-b tags
> added and extra line space removed and merge it if results will be green.
>
> Regards,
>
> Tvrtko
> </quote>
>
>
> >
> > Thanks & regards
> > Sumitra
> >
> > PS: I am new to the open source vocabulary terms.
> >
> > > > - Change commit subject and message.
> > > >
> > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > index f020c0086fbc..bc41500eedf5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
> > > >
> > > > drm_clflush_pages(&page, 1);
> > > >
> > > > - s = kmap(page);
> > > > + s = kmap_local_page(page);
> > > > ret = compress_page(compress, s, dst, false);
> > > > - kunmap(page);
> > > > + kunmap_local(s);
> > > >
> > > > drm_clflush_pages(&page, 1);
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-20 18:07 ` Sumitra Sharma
@ 2023-06-21 9:06 ` Thomas Hellström (Intel)
2023-06-21 13:24 ` Fabio M. De Francesco
2023-06-21 16:35 ` Ira Weiny
0 siblings, 2 replies; 15+ messages in thread
From: Thomas Hellström (Intel) @ 2023-06-21 9:06 UTC (permalink / raw
To: Sumitra Sharma, Ira Weiny
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Fabio, Deepak R Varma
On 6/20/23 20:07, Sumitra Sharma wrote:
> On Tue, Jun 20, 2023 at 06:23:38AM -0700, Ira Weiny wrote:
>> Sumitra Sharma wrote:
>>> On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
>>>> Sumitra Sharma wrote:
>>>>> kmap() has been deprecated in favor of the kmap_local_page()
>>>>> due to high cost, restricted mapping space, the overhead of a
>>>>> global lock for synchronization, and making the process sleep
>>>>> in the absence of free slots.
>>>>>
>>>>> kmap_local_page() is faster than kmap() and offers thread-local
>>>>> and CPU-local mappings, take pagefaults in a local kmap region
>>>>> and preserves preemption by saving the mappings of outgoing tasks
>>>>> and restoring those of the incoming one during a context switch.
>>>>>
>>>>> The mapping is kept thread local in the function
>>>>> “i915_vma_coredump_create” in i915_gpu_error.c
>>>>>
>>>>> Therefore, replace kmap() with kmap_local_page().
>>>>>
>>>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>>>>
>>>> NIT: No need for the line break between Suggested-by and your signed off line.
>>>>
>>> Hi Ira,
>>>
>>> What does NIT stand for?
>> Shorthand for 'nitpicking'.
>>
>> "giving too much attention to details that are not important, especially
>> as a way of criticizing: "
>>
>> - https://dictionary.cambridge.org/dictionary/english/nitpicking
>>
>> Via email this is a way for authors of an email to indicate something is
>> technically wrong but while nicely acknowledging that it is not very
>> significant and could be seen as overly critical.
>>
>> For this particular comment I'm showing something to pay attention to next
>> time but that was not a big deal this time around.
>>
> Hi Ira,
>
> Thank for your explanation on NIT.
>
>
>>> Thank you. I will take care about the line breaks.
>>>
>>>>> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Replace kmap() with kmap_local_page().
>>>> Generally it is customary to attribute a change like this to those who
>>>> suggested it in a V1 review.
>>>>
>>>> For example:
>>>>
>>>> - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
>>>>
>>>> Also I don't see Thomas on the new email list. Since he took the time to
>>>> review V1 he might want to check this version out. I've added him to the
>>>> 'To:' list.
>>>>
>>>> Also a link to V1 is nice. B4 formats it like this:
>>>>
>>>> - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
>>>>
>>>> All that said the code looks good to me. So with the above changes.
>>>>
>>>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>>>>
>>> I have noted down the points mentioned above. Thank you again.
>>>
>>> I am not supposed to create another version of this patch for
>>> adding the above mentions, as you and Thomas both gave this patch
>>> a reviewed-by tag. Right?
>>>
>> Based on this response[*] from Tvrtko I think this version can move
>> through without a v3.
> Okay!
>
>
> Thanks & regards
> Sumitra
I think one thing worth mentioning in the context of this patch is that
IIRC kmap_local_page() will block offlining of the mapping CPU until
kunmap_local(), so while I haven't seen any guidelines around the usage
of this api for long-held mappings, I figure it's wise to keep the
mapping duration short, or at least avoid sleeping with a
kmap_local_page() map active.
I figured, while page compression is probably to be considered "slow"
it's probably not slow enough to motivate kmap() instead of
kmap_local_page(), but if anyone feels differently, perhaps it should be
considered.
With that said, my Reviewed-by: still stands.
/Thomas
>
>> Thanks!
>> Ira
>>
>> [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7b38@linux.intel.com/
>>
>> <quote>
>> Thanks all! I'll just re-send the patch for our CI, since it didn't get
>> picked up automatically (stuck in moderation perhaps), with all r-b tags
>> added and extra line space removed and merge it if results will be green.
>>
>> Regards,
>>
>> Tvrtko
>> </quote>
>>
>>
>>> Thanks & regards
>>> Sumitra
>>>
>>> PS: I am new to the open source vocabulary terms.
>>>
>>>>> - Change commit subject and message.
>>>>>
>>>>> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> index f020c0086fbc..bc41500eedf5 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>>>>>
>>>>> drm_clflush_pages(&page, 1);
>>>>>
>>>>> - s = kmap(page);
>>>>> + s = kmap_local_page(page);
>>>>> ret = compress_page(compress, s, dst, false);
>>>>> - kunmap(page);
>>>>> + kunmap_local(s);
>>>>>
>>>>> drm_clflush_pages(&page, 1);
>>>>>
>>>>> --
>>>>> 2.25.1
>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-21 9:06 ` Thomas Hellström (Intel)
@ 2023-06-21 13:24 ` Fabio M. De Francesco
2023-06-21 16:35 ` Ira Weiny
1 sibling, 0 replies; 15+ messages in thread
From: Fabio M. De Francesco @ 2023-06-21 13:24 UTC (permalink / raw
To: Sumitra Sharma, Ira Weiny, Thomas Hellström (Intel)
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Deepak R Varma
On mercoledì 21 giugno 2023 11:06:51 CEST Thomas Hellström (Intel) wrote:
>
> I think one thing worth mentioning in the context of this patch is that
> IIRC kmap_local_page() will block offlining of the mapping CPU until
> kunmap_local(),
Migration is disabled.
> so while I haven't seen any guidelines around the usage
> of this api for long-held mappings,
It would be advisable to not use it for long term mappings, if possible. These
"local" mappings should better be help for not too long duration.
> I figure it's wise to keep the
> mapping duration short, or at least avoid sleeping with a
> kmap_local_page() map active.
Nothing prevents a call of preempt_disable() around the section of code
between kmap_local_page() / kunmap_local(). If it is needed, local mappings
could also be acquired under spinlocks and/or with disabled interrupts.
I don't know the code, however, everything cited above could be the subject of
a subsequent patch.
Regards,
Fabio
> I figured, while page compression is probably to be considered "slow"
> it's probably not slow enough to motivate kmap() instead of
> kmap_local_page(), but if anyone feels differently, perhaps it should be
> considered.
>
> With that said, my Reviewed-by: still stands.
>
> /Thomas
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-21 9:06 ` Thomas Hellström (Intel)
2023-06-21 13:24 ` Fabio M. De Francesco
@ 2023-06-21 16:35 ` Ira Weiny
2023-06-21 18:51 ` Thomas Hellström (Intel)
1 sibling, 1 reply; 15+ messages in thread
From: Ira Weiny @ 2023-06-21 16:35 UTC (permalink / raw
To: Thomas Hellström (Intel), Sumitra Sharma, Ira Weiny
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Fabio, Deepak R Varma
Thomas Hellström (Intel) wrote:
>
> I think one thing worth mentioning in the context of this patch is that
> IIRC kmap_local_page() will block offlining of the mapping CPU until
> kunmap_local(), so while I haven't seen any guidelines around the usage
> of this api for long-held mappings, I figure it's wise to keep the
> mapping duration short, or at least avoid sleeping with a
> kmap_local_page() map active.
>
> I figured, while page compression is probably to be considered "slow"
> it's probably not slow enough to motivate kmap() instead of
> kmap_local_page(), but if anyone feels differently, perhaps it should be
> considered.
What you say is all true. But remember the mappings are only actually
created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also
they must suffer such performance issues because there is just no other
way around supporting them.
Also Sumitra, and our kmap conversion project in general, is focusing on
not using kmap* if at all possible. Thus the reason V1 tried to use
page_address().
Could we guarantee the i915 driver is excluded from all HIGHMEM systems?
>
> With that said, my Reviewed-by: still stands.
Thanks!
Ira
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-21 16:35 ` Ira Weiny
@ 2023-06-21 18:51 ` Thomas Hellström (Intel)
2023-06-22 9:40 ` Tvrtko Ursulin
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Hellström (Intel) @ 2023-06-21 18:51 UTC (permalink / raw
To: Ira Weiny, Sumitra Sharma
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Fabio, Deepak R Varma
On 6/21/23 18:35, Ira Weiny wrote:
> Thomas Hellström (Intel) wrote:
>> I think one thing worth mentioning in the context of this patch is that
>> IIRC kmap_local_page() will block offlining of the mapping CPU until
>> kunmap_local(), so while I haven't seen any guidelines around the usage
>> of this api for long-held mappings, I figure it's wise to keep the
>> mapping duration short, or at least avoid sleeping with a
>> kmap_local_page() map active.
>>
>> I figured, while page compression is probably to be considered "slow"
>> it's probably not slow enough to motivate kmap() instead of
>> kmap_local_page(), but if anyone feels differently, perhaps it should be
>> considered.
> What you say is all true. But remember the mappings are only actually
> created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also
> they must suffer such performance issues because there is just no other
> way around supporting them.
>
> Also Sumitra, and our kmap conversion project in general, is focusing on
> not using kmap* if at all possible. Thus the reason V1 tried to use
> page_address().
>
> Could we guarantee the i915 driver is excluded from all HIGHMEM systems?
The i915 maintainers might want to chime in here, but I would say no, we
can't, although we don't care much about optimizing for them. Same for
the new xe driver.
Thanks,
/Thomas
>
>> With that said, my Reviewed-by: still stands.
> Thanks!
> Ira
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-21 18:51 ` Thomas Hellström (Intel)
@ 2023-06-22 9:40 ` Tvrtko Ursulin
0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-06-22 9:40 UTC (permalink / raw
To: Thomas Hellström (Intel), Ira Weiny, Sumitra Sharma
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, intel-gfx, dri-devel, linux-kernel, Fabio,
Deepak R Varma
On 21/06/2023 19:51, Thomas Hellström (Intel) wrote:
>
> On 6/21/23 18:35, Ira Weiny wrote:
>> Thomas Hellström (Intel) wrote:
>>> I think one thing worth mentioning in the context of this patch is that
>>> IIRC kmap_local_page() will block offlining of the mapping CPU until
>>> kunmap_local(), so while I haven't seen any guidelines around the usage
>>> of this api for long-held mappings, I figure it's wise to keep the
>>> mapping duration short, or at least avoid sleeping with a
>>> kmap_local_page() map active.
>>>
>>> I figured, while page compression is probably to be considered "slow"
>>> it's probably not slow enough to motivate kmap() instead of
>>> kmap_local_page(), but if anyone feels differently, perhaps it should be
>>> considered.
>> What you say is all true. But remember the mappings are only actually
>> created on a HIGHMEM system. HIGHMEM systems are increasingly rare.
>> Also
>> they must suffer such performance issues because there is just no other
>> way around supporting them.
>>
>> Also Sumitra, and our kmap conversion project in general, is focusing on
>> not using kmap* if at all possible. Thus the reason V1 tried to use
>> page_address().
>>
>> Could we guarantee the i915 driver is excluded from all HIGHMEM systems?
>
> The i915 maintainers might want to chime in here, but I would say no, we
> can't, although we don't care much about optimizing for them. Same for
> the new xe driver.
AFAIK i915 works on such systems so I don't think we can drop support
just like that. Not sure what the process would be. Perhaps as part of a
wider kernel deprecation would make most sense.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-17 18:04 [PATCH v2] drm/i915: Replace kmap() with kmap_local_page() Sumitra Sharma
2023-06-18 18:11 ` Ira Weiny
@ 2023-06-24 0:10 ` Fabio M. De Francesco
1 sibling, 0 replies; 15+ messages in thread
From: Fabio M. De Francesco @ 2023-06-24 0:10 UTC (permalink / raw
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Sumitra Sharma, Thomas Hellström (Intel)
Cc: Ira Weiny, Deepak R Varma, Sumitra Sharma
On sabato 17 giugno 2023 20:04:20 CEST Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of a
> global lock for synchronization, and making the process sleep
> in the absence of free slots.
>
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region
NIT: _can_ take pagefaults in a local kmap region
> and preserves preemption by saving the mappings of outgoing tasks
> and restoring those of the incoming one during a context switch.
>
> The mapping is kept thread local in the function
> “i915_vma_coredump_create” in i915_gpu_error.c
>
> Therefore, replace kmap() with kmap_local_page().
>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
>
> Changes in v2:
> - Replace kmap() with kmap_local_page().
> - Change commit subject and message.
With the changes that Ira suggested and the minor fix I'm proposing to the
commit message, it looks good to me too, so this patch is...
Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
However, as far as I'm concerned, our nits don't necessarily require any newer
version, especially because Tvrtko has already sent this patch for their CI.
Thanks,
Fabio
P.S.: As Sumitra says both kmap() and kmap_local_page() allows preemption in
non atomic context.
Furthermore, Tvrtko confirmed that the pages can come from HIGHMEM, therefore
kmap_local_page for local temporary mapping is unavoidable.
Last thing... Thomas thinks he wants to make it run atomically (if I
understood one of his messages correctly). As I already responded, nothing
prevents someone does another patch just to disable preemption (or to enter
atomic context by other means) around the code marked by kmap_local_page() /
kunmap_local() because these functions work perfectly _also_ in atomic context
(including interrupts). But this is not something that Sumitra should be
worried about.
>
> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c index f020c0086fbc..bc41500eedf5
> 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>
> drm_clflush_pages(&page, 1);
>
> - s = kmap(page);
> + s = kmap_local_page(page);
> ret = compress_page(compress, s, dst, false);
> - kunmap(page);
> + kunmap_local(s);
>
> drm_clflush_pages(&page, 1);
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
2023-06-20 13:23 ` Ira Weiny
2023-06-20 18:07 ` Sumitra Sharma
@ 2023-06-26 9:02 ` Tvrtko Ursulin
1 sibling, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-06-26 9:02 UTC (permalink / raw
To: Ira Weiny, Sumitra Sharma
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
Thomas Hellström (Intel), Fabio, Deepak R Varma
On 20/06/2023 14:23, Ira Weiny wrote:
> Sumitra Sharma wrote:
>> On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
>>> Sumitra Sharma wrote:
>>>> kmap() has been deprecated in favor of the kmap_local_page()
>>>> due to high cost, restricted mapping space, the overhead of a
>>>> global lock for synchronization, and making the process sleep
>>>> in the absence of free slots.
>>>>
>>>> kmap_local_page() is faster than kmap() and offers thread-local
>>>> and CPU-local mappings, take pagefaults in a local kmap region
>>>> and preserves preemption by saving the mappings of outgoing tasks
>>>> and restoring those of the incoming one during a context switch.
>>>>
>>>> The mapping is kept thread local in the function
>>>> “i915_vma_coredump_create” in i915_gpu_error.c
>>>>
>>>> Therefore, replace kmap() with kmap_local_page().
>>>>
>>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>>>
>>>
>>> NIT: No need for the line break between Suggested-by and your signed off line.
>>>
>>
>> Hi Ira,
>>
>> What does NIT stand for?
>
> Shorthand for 'nitpicking'.
>
> "giving too much attention to details that are not important, especially
> as a way of criticizing: "
>
> - https://dictionary.cambridge.org/dictionary/english/nitpicking
>
> Via email this is a way for authors of an email to indicate something is
> technically wrong but while nicely acknowledging that it is not very
> significant and could be seen as overly critical.
>
> For this particular comment I'm showing something to pay attention to next
> time but that was not a big deal this time around.
>
>>
>> Thank you. I will take care about the line breaks.
>>
>>>> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Replace kmap() with kmap_local_page().
>>>
>>> Generally it is customary to attribute a change like this to those who
>>> suggested it in a V1 review.
>>>
>>> For example:
>>>
>>> - Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
>>>
>>> Also I don't see Thomas on the new email list. Since he took the time to
>>> review V1 he might want to check this version out. I've added him to the
>>> 'To:' list.
>>>
>>> Also a link to V1 is nice. B4 formats it like this:
>>>
>>> - Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/
>>>
>>> All that said the code looks good to me. So with the above changes.
>>>
>>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>>>
>>
>> I have noted down the points mentioned above. Thank you again.
>>
>> I am not supposed to create another version of this patch for
>> adding the above mentions, as you and Thomas both gave this patch
>> a reviewed-by tag. Right?
>>
>
> Based on this response[*] from Tvrtko I think this version can move
> through without a v3.
>
> Thanks!
> Ira
>
> [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7b38@linux.intel.com/
>
> <quote>
> Thanks all! I'll just re-send the patch for our CI, since it didn't get
> picked up automatically (stuck in moderation perhaps), with all r-b tags
> added and extra line space removed and merge it if results will be green.
>
> Regards,
>
> Tvrtko
> </quote>
Pushed to drm-intel-gt-next, thanks for the patch and reviews!
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-06-26 9:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-17 18:04 [PATCH v2] drm/i915: Replace kmap() with kmap_local_page() Sumitra Sharma
2023-06-18 18:11 ` Ira Weiny
2023-06-19 7:59 ` Thomas Hellström (Intel)
2023-06-19 8:44 ` Tvrtko Ursulin
2023-06-19 15:45 ` Sumitra Sharma
2023-06-20 13:23 ` Ira Weiny
2023-06-20 18:07 ` Sumitra Sharma
2023-06-21 9:06 ` Thomas Hellström (Intel)
2023-06-21 13:24 ` Fabio M. De Francesco
2023-06-21 16:35 ` Ira Weiny
2023-06-21 18:51 ` Thomas Hellström (Intel)
2023-06-22 9:40 ` Tvrtko Ursulin
2023-06-26 9:02 ` Tvrtko Ursulin
2023-06-24 0:10 ` Fabio M. De Francesco
-- strict thread matches above, loose matches on Subject: below --
2021-12-10 23:23 [PATCH 1/7] " ira.weiny
2021-12-22 6:08 ` [PATCH V2] " ira.weiny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).