Re: [PATCH 3/9] drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
On lunedì 17 ottobre 2022 11:37:19 CET Zhao Liu wrote: > From: Zhao Liu > > 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. > > 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.we...@intel.com > [2]: https://patchwork.freedesktop.org/patch/295840/ > > Suggested-by: Ira Weiny > Signed-off-by: Zhao Liu > --- > Suggested by credits: > Ira: Referred to his suggestions about keeping pagefault_disable(). > --- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) The commit message explains clearly and the changes to the code look good. The necessity to reuse pagefault_disable() / pagefault_enable() from the main kmap_atomic() side effect is a nice catch. Reviewed-by: Fabio M. De Francesco Thanks! > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index f42ca1179f37..e279a3e30c02 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -472,11 +472,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);
Re: [PATCH 3/9] drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
On Mon, Oct 17, 2022 at 05:37:19PM +0800, Zhao Liu wrote: > From: Zhao Liu > > 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. > > 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.we...@intel.com > [2]: https://patchwork.freedesktop.org/patch/295840/ > > Suggested-by: Ira Weiny Reviewed-by: Ira Weiny > Signed-off-by: Zhao Liu > --- > Suggested by credits: > Ira: Referred to his suggestions about keeping pagefault_disable(). > --- > 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 f42ca1179f37..e279a3e30c02 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -472,11 +472,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 >
[PATCH 3/9] drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
From: Zhao Liu 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. 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.we...@intel.com [2]: https://patchwork.freedesktop.org/patch/295840/ Suggested-by: Ira Weiny Signed-off-by: Zhao Liu --- Suggested by credits: Ira: Referred to his suggestions about keeping pagefault_disable(). --- 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 f42ca1179f37..e279a3e30c02 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -472,11 +472,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