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

[Fabio M. De Francesco]

On lunedì 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@xxxxxxxxx>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].

Some words to explain why kmap_atomic was deprecated won't hurt. Many 
maintainers and reviewers, and also casual readers might not yet be aware of 
the reasons behind that deprecation.
 
> In the following patches, we can convert the calls of kmap_atomic() /
> kunmap_atomic() to kmap_local_page() / kunmap_local(), which can
> instead do the mapping / unmapping regardless of the context.

Readers are probably much more interested in what you did in the following 
patches and why you did it, instead of being informed about what "we can" do.

I would suggest something like "The following patches convert the calls to 
kmap_atomic() to kmap_local_page() [the rest looks OK]".

This could also be the place to say something about why we prefer 
kmap_local_page() to kmap_atomic(). 

Are you sure that the reasons that motivates your conversions are merely 
summarized to kmap_local_page() being able to do mappings regardless of 
context? I think you are missing the real reasons why. 

What about avoiding the often unwanted side effect of unnecessary page faults 
disables?

> 
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.

No news here. kmap_atomic() is "per thread, CPU local and not glocally 
visible". I cannot see any difference here between kmap_atomic() and 
kmap_local_page().

> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@xxxxxxxxx
> ---
> Zhao Liu (9):
>   drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
>   drm/i915: Use kmap_local_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             |  8 ++++----
>  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, 30 insertions(+), 37 deletions(-)

Thanks for helping with kmap_atomic() conversions to kmap_local_page().

Fabio