[Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

This series starts by converting the last easy kmap() uses to
kmap_local_page().

There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
ttm_bo_kmap_ttm() is called in a number of different ways including some which
are not thread local.  I have a patch to convert that call.  However, it is not
straight forward so it is not included in this series.

The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
conversion.


Ira Weiny (7):
drm/i915: Replace kmap() with kmap_local_page()
drm/amd: Replace kmap() with kmap_local_page()
drm/gma: Remove calls to kmap()
drm/radeon: Replace kmap() with kmap_local_page()
drm/msm: Alter comment to use kmap_local_page()
drm/amdgpu: Ensure kunmap is called on error
drm/radeon: Ensure kunmap is called on error

drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
drivers/gpu/drm/gma500/gma_display.c | 6 ++----
drivers/gpu/drm/gma500/mmu.c | 8 ++++----
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
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 | 4 ++--
drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
13 files changed, 32 insertions(+), 32 deletions(-)

--
2.31.1

[Intel-gfx] [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()

[ira.weiny]

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>
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c          | 4 ++--
 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              | 4 ++--
 drivers/gpu/drm/i915/i915_gem.c                    | 8 ++++----
 drivers/gpu/drm/i915/i915_gpu_error.c              | 4 ++--
 6 files changed, 16 insertions(+), 16 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..fa8b820e14aa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -597,9 +597,9 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
 		if (err < 0)
 			goto fail;
 
-		vaddr = kmap(page);
+		vaddr = kmap_local_page(page);
 		memcpy(vaddr, data, len);
-		kunmap(page);
+		kunmap_local(vaddr);
 
 		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..cba4f210d093 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c
@@ -104,7 +104,7 @@ static int __shmem_rw(struct file *file, loff_t off,
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		vaddr = kmap(page);
+		vaddr = kmap_local_page(page);
 		if (write) {
 			memcpy(vaddr + offset_in_page(off), ptr, this);
 			set_page_dirty(page);
@@ -112,7 +112,7 @@ static int __shmem_rw(struct file *file, loff_t off,
 			memcpy(ptr, vaddr + offset_in_page(off), this);
 		}
 		mark_page_accessed(page);
-		kunmap(page);
+		kunmap_local(vaddr);
 		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

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()

[Christoph Hellwig]

On Fri, Dec 10, 2021 at 03:23:58PM -0800, ira.weiny@intel.com wrote:
> -		vaddr = kmap(page);
> +		vaddr = kmap_local_page(page);
>  		memcpy(vaddr, data, len);
> -		kunmap(page);
> +		kunmap_local(vaddr);

memcpy_to_page?

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()

[Ira Weiny]

On Mon, Dec 13, 2021 at 01:04:07AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 10, 2021 at 03:23:58PM -0800, ira.weiny@intel.com wrote:
> > -		vaddr = kmap(page);
> > +		vaddr = kmap_local_page(page);
> >  		memcpy(vaddr, data, len);
> > -		kunmap(page);
> > +		kunmap_local(vaddr);
> 
> memcpy_to_page?

Opps!  Yea!

David, Daniel,

Do you prefer me to resent the entire series or reply to this message with a
V2?

Ira

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()

[Ville Syrjälä]

On Fri, Dec 10, 2021 at 03:23:58PM -0800, ira.weiny@intel.com wrote:
> 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>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c          | 4 ++--
>  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              | 4 ++--
>  drivers/gpu/drm/i915/i915_gem.c                    | 8 ++++----
>  drivers/gpu/drm/i915/i915_gpu_error.c              | 4 ++--
>  6 files changed, 16 insertions(+), 16 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..fa8b820e14aa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -597,9 +597,9 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
>  		if (err < 0)
>  			goto fail;
>  
> -		vaddr = kmap(page);
> +		vaddr = kmap_local_page(page);
>  		memcpy(vaddr, data, len);
> -		kunmap(page);
> +		kunmap_local(vaddr);
>  
>  		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);

Does kunmap_local() do some magic to make it work even when you
don't pass in the same value you got from kmap_local_page()?

>  	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);

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()

[Ira Weiny]

On Mon, Dec 13, 2021 at 02:39:59PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 10, 2021 at 03:23:58PM -0800, ira.weiny@intel.com wrote:
> > 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>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_shmem.c          | 4 ++--
> >  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              | 4 ++--
> >  drivers/gpu/drm/i915/i915_gem.c                    | 8 ++++----
> >  drivers/gpu/drm/i915/i915_gpu_error.c              | 4 ++--
> >  6 files changed, 16 insertions(+), 16 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..fa8b820e14aa 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -597,9 +597,9 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
> >  		if (err < 0)
> >  			goto fail;
> >  
> > -		vaddr = kmap(page);
> > +		vaddr = kmap_local_page(page);
> >  		memcpy(vaddr, data, len);
> > -		kunmap(page);
> > +		kunmap_local(vaddr);
> >  
> >  		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);
> 
> Does kunmap_local() do some magic to make it work even when you
> don't pass in the same value you got from kmap_local_page()?

Yes.  It sounds like a patch like this would be nice to clarify?

Ira

diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index 0a0b2b09b1b8..fb2d3e033c01 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -246,6 +246,17 @@ do {                                                               \
        __kunmap_atomic(__addr);                                \
 } while (0)
 
+/**
+ * kunmap_local - Unmap a page mapped via kmap_local_page().
+ * @__addr: An address within the page mapped
+ *
+ * __addr is often an address returned from kmap_local_page().  However,
+ * this address can be any address within the mapped page.  It does not need to
+ * be the exact address returned from kmap_local_page()
+ *
+ * Unmapping should be done in the reverse order of the mapping.  See
+ * kmap_local_page() for details.
+ */
 #define kunmap_local(__addr)                                   \
 do {                                                           \
        BUILD_BUG_ON(__same_type((__addr), struct page *));     \

[Intel-gfx] [PATCH V2] drm/i915: Replace kmap() with kmap_local_page()

[ira.weiny]

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

[Intel-gfx] [PATCH 2/7] drm/amd: Replace kmap() with kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated.  These maps are thread local and can be
replaced with kmap_local_page().

Replace kmap() with kmap_local_page()

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c875f1cdd2af..a3a2c73a44bb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -2281,9 +2281,9 @@ static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
 		if (p->mapping != adev->mman.bdev.dev_mapping)
 			return -EPERM;
 
-		ptr = kmap(p);
+		ptr = kmap_local_page(p);
 		r = copy_to_user(buf, ptr + off, bytes);
-		kunmap(p);
+		kunmap_local(ptr);
 		if (r)
 			return -EFAULT;
 
@@ -2332,9 +2332,9 @@ static ssize_t amdgpu_iomem_write(struct file *f, const char __user *buf,
 		if (p->mapping != adev->mman.bdev.dev_mapping)
 			return -EPERM;
 
-		ptr = kmap(p);
+		ptr = kmap_local_page(p);
 		r = copy_from_user(ptr + off, buf, bytes);
-		kunmap(p);
+		kunmap_local(ptr);
 		if (r)
 			return -EFAULT;
 
-- 
2.31.1

[Intel-gfx] [PATCH 3/7] drm/gma: Remove calls to kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and these instances are easy to convert to
kmap_local_page().

Furthermore, in gma_crtc_cursor_set() use the memcpy_from_page() helper
instead of an open coded use of kmap_local_page().

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/gpu/drm/gma500/gma_display.c | 6 ++----
 drivers/gpu/drm/gma500/mmu.c         | 8 ++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index cbcecbaa041b..caf7c7b321a9 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -334,7 +334,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	struct gtt_range *gt;
 	struct gtt_range *cursor_gt = gma_crtc->cursor_gt;
 	struct drm_gem_object *obj;
-	void *tmp_dst, *tmp_src;
+	void *tmp_dst;
 	int ret = 0, i, cursor_pages;
 
 	/* If we didn't get a handle then turn the cursor off */
@@ -400,9 +400,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 		/* Copy the cursor to cursor mem */
 		tmp_dst = dev_priv->vram_addr + cursor_gt->offset;
 		for (i = 0; i < cursor_pages; i++) {
-			tmp_src = kmap(gt->pages[i]);
-			memcpy(tmp_dst, tmp_src, PAGE_SIZE);
-			kunmap(gt->pages[i]);
+			memcpy_from_page(tmp_dst, gt->pages[i], 0, PAGE_SIZE);
 			tmp_dst += PAGE_SIZE;
 		}
 
diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
index fe9ace2a7967..a70b01ccdf70 100644
--- a/drivers/gpu/drm/gma500/mmu.c
+++ b/drivers/gpu/drm/gma500/mmu.c
@@ -184,17 +184,17 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct psb_mmu_driver *driver,
 		pd->invalid_pte = 0;
 	}
 
-	v = kmap(pd->dummy_pt);
+	v = kmap_local_page(pd->dummy_pt);
 	for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
 		v[i] = pd->invalid_pte;
 
-	kunmap(pd->dummy_pt);
+	kunmap_local(v);
 
-	v = kmap(pd->p);
+	v = kmap_local_page(pd->p);
 	for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
 		v[i] = pd->invalid_pde;
 
-	kunmap(pd->p);
+	kunmap_local(v);
 
 	clear_page(kmap(pd->dummy_page));
 	kunmap(pd->dummy_page);
-- 
2.31.1

[Intel-gfx] [PATCH 4/7] drm/radeon: Replace kmap() with kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and this usage is local to the thread.  Use
kmap_local_page() instead.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 11b21d605584..76d7906e1785 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -907,11 +907,11 @@ static ssize_t radeon_ttm_gtt_read(struct file *f, char __user *buf,
 
 		page = rdev->gart.pages[p];
 		if (page) {
-			ptr = kmap(page);
+			ptr = kmap_local_page(page);
 			ptr += off;
 
 			r = copy_to_user(buf, ptr, cur_size);
-			kunmap(rdev->gart.pages[p]);
+			kunmap_local(ptr);
 		} else
 			r = clear_user(buf, cur_size);
 
-- 
2.31.1

[Intel-gfx] [PATCH 5/7] drm/msm: Alter comment to use kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated.  So this comment could be misleading in the
future.

Change this comment to point to using kmap_local_page().  While here
remove 'we' from the comment.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 282628d6b72c..654ae0d13eaf 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -438,8 +438,8 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
 		return -EINVAL;
 	}
 
-	/* For now, just map the entire thing.  Eventually we probably
-	 * to do it page-by-page, w/ kmap() if not vmap()d..
+	/* For now, just map the entire thing.  Eventually it should probably
+	 * be done page-by-page, w/ kmap_local_page() if not vmap()d..
 	 */
 	ptr = msm_gem_get_vaddr_locked(&obj->base);
 
-- 
2.31.1

[Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The default case leaves the buffer object mapped in error.

Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.

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

---
NOTE: It seems like this function could use a fair bit of refactoring
but this is the easiest way to fix the actual bug.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index 6f8de11a17f1..b3ffd0f6b35f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
 		return 0;
 
 	default:
+		amdgpu_bo_kunmap(bo);
 		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
 	}
 
-- 
2.31.1

Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[Christian König]

Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> From: Ira Weiny <ira.weiny@intel.com>
>
> The default case leaves the buffer object mapped in error.
>
> Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.

Mhm, good catch. But why do you want to do this in the first place?

Christian.

>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---
> NOTE: It seems like this function could use a fair bit of refactoring
> but this is the easiest way to fix the actual bug.
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
>   1 file changed, 1 insertion(+)
> nice
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> index 6f8de11a17f1..b3ffd0f6b35f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
>   		return 0;
>   
>   	default:
> +		amdgpu_bo_kunmap(bo);
>   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
>   	}
>   

Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[Ira Weiny]

On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The default case leaves the buffer object mapped in error.
> > 
> > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> 
> Mhm, good catch. But why do you want to do this in the first place?

I'm not sure I understand the question.

Any mapping of memory should be paired with an unmapping when no longer needed.
And this is supported by the call to amdgpu_bo_kunmap() in the other
non-default cases.

Do you believe the mapping is not needed?

Ira

> 
> Christian.
> 
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > NOTE: It seems like this function could use a fair bit of refactoring
> > but this is the easiest way to fix the actual bug.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> >   1 file changed, 1 insertion(+)
> > nice
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> >   		return 0;
> >   	default:
> > +		amdgpu_bo_kunmap(bo);
> >   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> >   	}
> 

Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[Christian König]

Am 14.12.21 um 04:37 schrieb Ira Weiny:
> On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
>> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
>>> From: Ira Weiny <ira.weiny@intel.com>
>>>
>>> The default case leaves the buffer object mapped in error.
>>>
>>> Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
>> Mhm, good catch. But why do you want to do this in the first place?
> I'm not sure I understand the question.
>
> Any mapping of memory should be paired with an unmapping when no longer needed.
> And this is supported by the call to amdgpu_bo_kunmap() in the other
> non-default cases.
>
> Do you believe the mapping is not needed?

No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), 
it either creates the mapping or return the cached pointer.

A call to amdgpu_bo_kunmap() is only done in a few places where we know 
that the created mapping most likely won't be needed any more. If that's 
not done the mapping is automatically destroyed when the BO is moved or 
freed up.

I mean good bug fix, but you seem to see this as some kind of 
prerequisite to some follow up work converting TTM to use kmap_local() 
which most likely won't work in the first place.

Regards,
Christian.

>
> Ira
>
>> Christian.
>>
>>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>>>
>>> ---
>>> NOTE: It seems like this function could use a fair bit of refactoring
>>> but this is the easiest way to fix the actual bug.
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>> nice
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> index 6f8de11a17f1..b3ffd0f6b35f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
>>>    		return 0;
>>>    	default:
>>> +		amdgpu_bo_kunmap(bo);
>>>    		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
>>>    	}

Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[Ira Weiny]

On Tue, Dec 14, 2021 at 08:09:29AM +0100, Christian König wrote:
> Am 14.12.21 um 04:37 schrieb Ira Weiny:
> > On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> > > Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > The default case leaves the buffer object mapped in error.
> > > > 
> > > > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> > > Mhm, good catch. But why do you want to do this in the first place?
> > I'm not sure I understand the question.
> > 
> > Any mapping of memory should be paired with an unmapping when no longer needed.
> > And this is supported by the call to amdgpu_bo_kunmap() in the other
> > non-default cases.
> > 
> > Do you believe the mapping is not needed?
> 
> No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), it
> either creates the mapping or return the cached pointer.

Ah I missed that.  Thanks.

> 
> A call to amdgpu_bo_kunmap() is only done in a few places where we know that
> the created mapping most likely won't be needed any more. If that's not done
> the mapping is automatically destroyed when the BO is moved or freed up.
> 
> I mean good bug fix, but you seem to see this as some kind of prerequisite
> to some follow up work converting TTM to use kmap_local() which most likely
> won't work in the first place.

Sure.  I see now that it is more complicated than I thought but I never thought
of this as a strict prerequisite.  Just something I found while trying to
figure out how this works.

How much of a speed up is it to maintain the ttm_bo_map_kmap map type?  Could
this all be done with vmap and just remove the kmap stuff?

Ira

> 
> Regards,
> Christian.
> 
> > 
> > Ira
> > 
> > > Christian.
> > > 
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > ---
> > > > NOTE: It seems like this function could use a fair bit of refactoring
> > > > but this is the easiest way to fix the actual bug.
> > > > ---
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > nice
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > > > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > > > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> > > >    		return 0;
> > > >    	default:
> > > > +		amdgpu_bo_kunmap(bo);
> > > >    		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> > > >    	}
> 

Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error

[Christian König]

Am 15.12.21 um 22:09 schrieb Ira Weiny:
> On Tue, Dec 14, 2021 at 08:09:29AM +0100, Christian König wrote:
>> Am 14.12.21 um 04:37 schrieb Ira Weiny:
>>> On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
>>>> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
>>>>> From: Ira Weiny <ira.weiny@intel.com>
>>>>>
>>>>> The default case leaves the buffer object mapped in error.
>>>>>
>>>>> Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
>>>> Mhm, good catch. But why do you want to do this in the first place?
>>> I'm not sure I understand the question.
>>>
>>> Any mapping of memory should be paired with an unmapping when no longer needed.
>>> And this is supported by the call to amdgpu_bo_kunmap() in the other
>>> non-default cases.
>>>
>>> Do you believe the mapping is not needed?
>> No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), it
>> either creates the mapping or return the cached pointer.
> Ah I missed that.  Thanks.
>
>> A call to amdgpu_bo_kunmap() is only done in a few places where we know that
>> the created mapping most likely won't be needed any more. If that's not done
>> the mapping is automatically destroyed when the BO is moved or freed up.
>>
>> I mean good bug fix, but you seem to see this as some kind of prerequisite
>> to some follow up work converting TTM to use kmap_local() which most likely
>> won't work in the first place.
> Sure.  I see now that it is more complicated than I thought but I never thought
> of this as a strict prerequisite.  Just something I found while trying to
> figure out how this works.
>
> How much of a speed up is it to maintain the ttm_bo_map_kmap map type?

Good question. I don't really know.

This used to be pretty important for older drivers since there the 
kernel needs to kmap individual pages and patch them up before sending 
the command stream to the hardware.

It most likely doesn't matter for modern hardware.

> Could this all be done with vmap and just remove the kmap stuff?

Maybe, but I wouldn't bet on it and I don't really want to touch any of 
the old drivers to figure that out.

Christian.

>
> Ira
>
>> Regards,
>> Christian.
>>
>>> Ira
>>>
>>>> Christian.
>>>>
>>>>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>>>>>
>>>>> ---
>>>>> NOTE: It seems like this function could use a fair bit of refactoring
>>>>> but this is the easiest way to fix the actual bug.
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>> nice
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>>>> index 6f8de11a17f1..b3ffd0f6b35f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>>>> @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
>>>>>     		return 0;
>>>>>     	default:
>>>>> +		amdgpu_bo_kunmap(bo);
>>>>>     		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
>>>>>     	}

[Intel-gfx] [PATCH 7/7] drm/radeon: Ensure kunmap is called on error

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The default case leaves the buffer object mapped in error.

Add radeon_bo_kunmap() to that case to ensure the mapping is cleaned up.

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

---
NOTE: It seems like this function could use a fair bit of refactoring
but this is the easiest way to fix the actual bug.
---
 drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c
index 2ea86919d953..7462010e0e6d 100644
--- a/drivers/gpu/drm/radeon/radeon_uvd.c
+++ b/drivers/gpu/drm/radeon/radeon_uvd.c
@@ -563,6 +563,7 @@ static int radeon_uvd_cs_msg(struct radeon_cs_parser *p, struct radeon_bo *bo,
 
 	default:
 
+		radeon_bo_kunmap(bo);
 		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
 		return -EINVAL;
 	}
-- 
2.31.1

[Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions

[Patchwork]

== Series Details ==

Series: DRM kmap() fixes and kmap_local_page() conversions
URL   : https://patchwork.freedesktop.org/series/97889/
State : failure

== Summary ==

Applying: drm/i915: Replace kmap() with kmap_local_page()
Applying: drm/amd: Replace kmap() with kmap_local_page()
Applying: drm/gma: Remove calls to kmap()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/gma500/gma_display.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/gma500/gma_display.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/gma500/gma_display.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 drm/gma: Remove calls to kmap()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions (rev2)

[Patchwork]

== Series Details ==

Series: DRM kmap() fixes and kmap_local_page() conversions (rev2)
URL   : https://patchwork.freedesktop.org/series/97889/
State : failure

== Summary ==

Applying: drm/i915: Replace kmap() with kmap_local_page()
error: patch failed: include/linux/highmem-internal.h:246
error: include/linux/highmem-internal.h: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Using index info to reconstruct a base tree...
Patch failed at 0001 drm/i915: Replace kmap() with kmap_local_page()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions (rev3)

[Patchwork]

== Series Details ==

Series: DRM kmap() fixes and kmap_local_page() conversions (rev3)
URL   : https://patchwork.freedesktop.org/series/97889/
State : failure

== Summary ==

Applying: drm/i915: Replace kmap() with kmap_local_page()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_shmem.c
M	drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
M	drivers/gpu/drm/i915/i915_gem.c
M	drivers/gpu/drm/i915/i915_gpu_error.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_gpu_error.c
Auto-merging drivers/gpu/drm/i915/i915_gem.c
Auto-merging drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_shmem.c
Applying: drm/amd: Replace kmap() with kmap_local_page()
Applying: drm/gma: Remove calls to kmap()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/gma500/gma_display.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/gma500/gma_display.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/gma500/gma_display.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 drm/gma: Remove calls to kmap()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Ira Weiny]

On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> This series starts by converting the last easy kmap() uses to
> kmap_local_page().
> 
> There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
> ttm_bo_kmap_ttm() is called in a number of different ways including some which
> are not thread local.  I have a patch to convert that call.  However, it is not
> straight forward so it is not included in this series.
> 
> The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
> conversion.

Gentile ping on this series?  Will it make this merge window?

Thanks,
Ira

> 
> 
> Ira Weiny (7):
> drm/i915: Replace kmap() with kmap_local_page()
> drm/amd: Replace kmap() with kmap_local_page()
> drm/gma: Remove calls to kmap()
> drm/radeon: Replace kmap() with kmap_local_page()
> drm/msm: Alter comment to use kmap_local_page()
> drm/amdgpu: Ensure kunmap is called on error
> drm/radeon: Ensure kunmap is called on error
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> drivers/gpu/drm/gma500/gma_display.c | 6 ++----
> drivers/gpu/drm/gma500/mmu.c | 8 ++++----
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
> 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 | 4 ++--
> drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
> 13 files changed, 32 insertions(+), 32 deletions(-)
> 
> --
> 2.31.1
> 

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Daniel Vetter]

On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote:
> On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > This series starts by converting the last easy kmap() uses to
> > kmap_local_page().
> > 
> > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
> > ttm_bo_kmap_ttm() is called in a number of different ways including some which
> > are not thread local.  I have a patch to convert that call.  However, it is not
> > straight forward so it is not included in this series.
> > 
> > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
> > conversion.
> 
> Gentile ping on this series?  Will it make this merge window?

I think this fell through the cracks and so no. Note that generally we
feature-freeze drm tree around -rc6 anyway for the upcoming merge window,
so you were cutting this all a bit close anyway. Also looks like the ttm
kmap caching question didn't get resolved?

Anyway if patches are stuck resend with RESEND and if people still don't
pick them up poke me and I'll apply as fallback.

Cheers, Daniel

> 
> Thanks,
> Ira
> 
> > 
> > 
> > Ira Weiny (7):
> > drm/i915: Replace kmap() with kmap_local_page()
> > drm/amd: Replace kmap() with kmap_local_page()
> > drm/gma: Remove calls to kmap()
> > drm/radeon: Replace kmap() with kmap_local_page()
> > drm/msm: Alter comment to use kmap_local_page()
> > drm/amdgpu: Ensure kunmap is called on error
> > drm/radeon: Ensure kunmap is called on error
> > 
> > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
> > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> > drivers/gpu/drm/gma500/gma_display.c | 6 ++----
> > drivers/gpu/drm/gma500/mmu.c | 8 ++++----
> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
> > 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 | 4 ++--
> > drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
> > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
> > drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
> > 13 files changed, 32 insertions(+), 32 deletions(-)
> > 
> > --
> > 2.31.1
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Ira Weiny]

On Wed, Jan 19, 2022 at 06:24:22PM +0100, Daniel Vetter wrote:
> On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote:
> > On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > This series starts by converting the last easy kmap() uses to
> > > kmap_local_page().
> > > 
> > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
> > > ttm_bo_kmap_ttm() is called in a number of different ways including some which
> > > are not thread local.  I have a patch to convert that call.  However, it is not
> > > straight forward so it is not included in this series.
> > > 
> > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
> > > conversion.
> > 
> > Gentile ping on this series?  Will it make this merge window?
> 
> I think this fell through the cracks and so no. Note that generally we
> feature-freeze drm tree around -rc6 anyway for the upcoming merge window,
> so you were cutting this all a bit close anyway.

Ok, No problem.  I just had not heard if this was picked up or not.

> Also looks like the ttm
> kmap caching question didn't get resolved?

I'm sorry I thought it was resolve for this series.  Christian said the patches
in this series were "a good bug fix" even if not strictly necessary.[1]  Beyond
this series I was discussing where to go from here, and is it possible to go
further with more changes.[2]  At the moment I don't think I will.

Christian did I misunderstand?  I can drop patch 6 and 7 if they are not proper
bug fixes or at least clarifications to the code.

Ira

[1] https://lore.kernel.org/lkml/c3b173ea-6509-ebbe-b5f9-eeb29f1ce57e@amd.com/
[2] https://lore.kernel.org/lkml/20211215210949.GW3538886@iweiny-DESK2.sc.intel.com/

> 
> Anyway if patches are stuck resend with RESEND and if people still don't
> pick them up poke me and I'll apply as fallback.
> 
> Cheers, Daniel
> 
> > 
> > Thanks,
> > Ira
> > 
> > > 
> > > 
> > > Ira Weiny (7):
> > > drm/i915: Replace kmap() with kmap_local_page()
> > > drm/amd: Replace kmap() with kmap_local_page()
> > > drm/gma: Remove calls to kmap()
> > > drm/radeon: Replace kmap() with kmap_local_page()
> > > drm/msm: Alter comment to use kmap_local_page()
> > > drm/amdgpu: Ensure kunmap is called on error
> > > drm/radeon: Ensure kunmap is called on error
> > > 
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> > > drivers/gpu/drm/gma500/gma_display.c | 6 ++----
> > > drivers/gpu/drm/gma500/mmu.c | 8 ++++----
> > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
> > > 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 | 4 ++--
> > > drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
> > > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
> > > drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
> > > 13 files changed, 32 insertions(+), 32 deletions(-)
> > > 
> > > --
> > > 2.31.1
> > > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Christian König]

Am 20.01.22 um 00:55 schrieb Ira Weiny:
> On Wed, Jan 19, 2022 at 06:24:22PM +0100, Daniel Vetter wrote:
>> On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote:
>>> On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
>>>> From: Ira Weiny <ira.weiny@intel.com>
>>>>
>>>> This series starts by converting the last easy kmap() uses to
>>>> kmap_local_page().
>>>>
>>>> There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
>>>> ttm_bo_kmap_ttm() is called in a number of different ways including some which
>>>> are not thread local.  I have a patch to convert that call.  However, it is not
>>>> straight forward so it is not included in this series.
>>>>
>>>> The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
>>>> conversion.
>>> Gentile ping on this series?  Will it make this merge window?
>> I think this fell through the cracks and so no. Note that generally we
>> feature-freeze drm tree around -rc6 anyway for the upcoming merge window,
>> so you were cutting this all a bit close anyway.
> Ok, No problem.  I just had not heard if this was picked up or not.
>
>> Also looks like the ttm
>> kmap caching question didn't get resolved?
> I'm sorry I thought it was resolve for this series.  Christian said the patches
> in this series were "a good bug fix" even if not strictly necessary.[1]  Beyond
> this series I was discussing where to go from here, and is it possible to go
> further with more changes.[2]  At the moment I don't think I will.
>
> Christian did I misunderstand?  I can drop patch 6 and 7 if they are not proper
> bug fixes or at least clarifications to the code.

Yeah, it is indeed a correct cleanup. I would just *not* put a CC stable 
on it because it doesn't really fix anything.

Christian.

>
> Ira
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2Fc3b173ea-6509-ebbe-b5f9-eeb29f1ce57e%40amd.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=4p7jCB6pB4nlcUtLWh6K2Sso9X%2BsRSK7mcD8UavzztQ%3D&amp;reserved=0
> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20211215210949.GW3538886%40iweiny-DESK2.sc.intel.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=6%2BGfSKshg8Xr%2FXJshiU28yHzbg2HcVisVJLDU6tVUT4%3D&amp;reserved=0
>
>> Anyway if patches are stuck resend with RESEND and if people still don't
>> pick them up poke me and I'll apply as fallback.
>>
>> Cheers, Daniel
>>
>>> Thanks,
>>> Ira
>>>
>>>>
>>>> Ira Weiny (7):
>>>> drm/i915: Replace kmap() with kmap_local_page()
>>>> drm/amd: Replace kmap() with kmap_local_page()
>>>> drm/gma: Remove calls to kmap()
>>>> drm/radeon: Replace kmap() with kmap_local_page()
>>>> drm/msm: Alter comment to use kmap_local_page()
>>>> drm/amdgpu: Ensure kunmap is called on error
>>>> drm/radeon: Ensure kunmap is called on error
>>>>
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
>>>> drivers/gpu/drm/gma500/gma_display.c | 6 ++----
>>>> drivers/gpu/drm/gma500/mmu.c | 8 ++++----
>>>> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
>>>> 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 | 4 ++--
>>>> drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
>>>> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>>>> drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
>>>> drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
>>>> 13 files changed, 32 insertions(+), 32 deletions(-)
>>>>
>>>> --
>>>> 2.31.1
>>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NLqm91HCdllhW%2BrQ8aHMLXhYGkOJrYffpjsIJZWaFBc%3D&amp;reserved=0

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Daniel Vetter]

On Thu, Jan 20, 2022 at 09:16:35AM +0100, Christian König wrote:
> Am 20.01.22 um 00:55 schrieb Ira Weiny:
> > On Wed, Jan 19, 2022 at 06:24:22PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote:
> > > > On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
> > > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > > 
> > > > > This series starts by converting the last easy kmap() uses to
> > > > > kmap_local_page().
> > > > > 
> > > > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
> > > > > ttm_bo_kmap_ttm() is called in a number of different ways including some which
> > > > > are not thread local.  I have a patch to convert that call.  However, it is not
> > > > > straight forward so it is not included in this series.
> > > > > 
> > > > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
> > > > > conversion.
> > > > Gentile ping on this series?  Will it make this merge window?
> > > I think this fell through the cracks and so no. Note that generally we
> > > feature-freeze drm tree around -rc6 anyway for the upcoming merge window,
> > > so you were cutting this all a bit close anyway.
> > Ok, No problem.  I just had not heard if this was picked up or not.
> > 
> > > Also looks like the ttm
> > > kmap caching question didn't get resolved?
> > I'm sorry I thought it was resolve for this series.  Christian said the patches
> > in this series were "a good bug fix" even if not strictly necessary.[1]  Beyond
> > this series I was discussing where to go from here, and is it possible to go
> > further with more changes.[2]  At the moment I don't think I will.
> > 
> > Christian did I misunderstand?  I can drop patch 6 and 7 if they are not proper
> > bug fixes or at least clarifications to the code.
> 
> Yeah, it is indeed a correct cleanup. I would just *not* put a CC stable on
> it because it doesn't really fix anything.

Ok can you pls get the amd/radeon ones stuffed into alex' tree? Or do we
want to put all the ttm ones into drm-misc instead?
-Daniel

> 
> Christian.
> 
> > 
> > Ira
> > 
> > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2Fc3b173ea-6509-ebbe-b5f9-eeb29f1ce57e%40amd.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=4p7jCB6pB4nlcUtLWh6K2Sso9X%2BsRSK7mcD8UavzztQ%3D&amp;reserved=0
> > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20211215210949.GW3538886%40iweiny-DESK2.sc.intel.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=6%2BGfSKshg8Xr%2FXJshiU28yHzbg2HcVisVJLDU6tVUT4%3D&amp;reserved=0
> > 
> > > Anyway if patches are stuck resend with RESEND and if people still don't
> > > pick them up poke me and I'll apply as fallback.
> > > 
> > > Cheers, Daniel
> > > 
> > > > Thanks,
> > > > Ira
> > > > 
> > > > > 
> > > > > Ira Weiny (7):
> > > > > drm/i915: Replace kmap() with kmap_local_page()
> > > > > drm/amd: Replace kmap() with kmap_local_page()
> > > > > drm/gma: Remove calls to kmap()
> > > > > drm/radeon: Replace kmap() with kmap_local_page()
> > > > > drm/msm: Alter comment to use kmap_local_page()
> > > > > drm/amdgpu: Ensure kunmap is called on error
> > > > > drm/radeon: Ensure kunmap is called on error
> > > > > 
> > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++----
> > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> > > > > drivers/gpu/drm/gma500/gma_display.c | 6 ++----
> > > > > drivers/gpu/drm/gma500/mmu.c | 8 ++++----
> > > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
> > > > > 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 | 4 ++--
> > > > > drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > > > > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
> > > > > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
> > > > > drivers/gpu/drm/radeon/radeon_uvd.c | 1 +
> > > > > 13 files changed, 32 insertions(+), 32 deletions(-)
> > > > > 
> > > > > --
> > > > > 2.31.1
> > > > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C5e0192210d4640adb88b08d9dba734b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782333459591089%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NLqm91HCdllhW%2BrQ8aHMLXhYGkOJrYffpjsIJZWaFBc%3D&amp;reserved=0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions

[Ira Weiny]

On Thu, Jan 20, 2022 at 04:48:50PM +0100, Daniel Vetter wrote:
> On Thu, Jan 20, 2022 at 09:16:35AM +0100, Christian König wrote:
> > Am 20.01.22 um 00:55 schrieb Ira Weiny:
> > > On Wed, Jan 19, 2022 at 06:24:22PM +0100, Daniel Vetter wrote:
> > > > On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote:
> > > > > On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote:
> > > > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > > > 
> > > > > > This series starts by converting the last easy kmap() uses to
> > > > > > kmap_local_page().
> > > > > > 
> > > > > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm().  Unfortunately,
> > > > > > ttm_bo_kmap_ttm() is called in a number of different ways including some which
> > > > > > are not thread local.  I have a patch to convert that call.  However, it is not
> > > > > > straight forward so it is not included in this series.
> > > > > > 
> > > > > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm()
> > > > > > conversion.
> > > > > Gentile ping on this series?  Will it make this merge window?
> > > > I think this fell through the cracks and so no. Note that generally we
> > > > feature-freeze drm tree around -rc6 anyway for the upcoming merge window,
> > > > so you were cutting this all a bit close anyway.
> > > Ok, No problem.  I just had not heard if this was picked up or not.
> > > 
> > > > Also looks like the ttm
> > > > kmap caching question didn't get resolved?
> > > I'm sorry I thought it was resolve for this series.  Christian said the patches
> > > in this series were "a good bug fix" even if not strictly necessary.[1]  Beyond
> > > this series I was discussing where to go from here, and is it possible to go
> > > further with more changes.[2]  At the moment I don't think I will.
> > > 
> > > Christian did I misunderstand?  I can drop patch 6 and 7 if they are not proper
> > > bug fixes or at least clarifications to the code.
> > 
> > Yeah, it is indeed a correct cleanup. I would just *not* put a CC stable on
> > it because it doesn't really fix anything.
> 
> Ok can you pls get the amd/radeon ones stuffed into alex' tree? Or do we
> want to put all the ttm ones into drm-misc instead?

I just updated to the latest master and there is a minor conflict.  Since this
is not going in this window.  Let me rebase and resend.

Ira

> -Daniel
> 

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

[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

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

[Ira Weiny]

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
> 

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

[Thomas Hellström (Intel)]

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
>>

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

[Tvrtko Ursulin]

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

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

[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
> > 
> 
> 

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

[Ira Weiny]

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
> > > 
> > 
> > 

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

[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
> > > > 
> > > 
> > > 
> 
> 

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

[Thomas Hellström (Intel)]

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
>>>>>
>>>>
>>

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

[Fabio M. De Francesco]

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
> 

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

[Ira Weiny]

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

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

[Thomas Hellström (Intel)]

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

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

[Tvrtko Ursulin]

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

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

[Tvrtko Ursulin]

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

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

[Fabio M. De Francesco]

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