Re: [PATCH V1 10/10] drm: Remove drm specific kmap_atomic code
Am 30.04.20 um 22:38 schrieb ira.we...@intel.com:
From: Ira Weiny
kmap_atomic_prot() is now exported by all architectures. Use this
function rather than open coding a driver specific kmap_atomic.
Signed-off-by: Ira Weiny
Ah, yes looking into this once more this was on my TODO list for quite a
while as well.
Patch is Reviewed-by: Christian König , feel
free to push it upstream through whatever channel you like or ping me if
I should pick it up into drm-misc-next.
Regards,
Christian.
---
drivers/gpu/drm/ttm/ttm_bo_util.c| 56 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 16
include/drm/ttm/ttm_bo_api.h | 4 --
3 files changed, 12 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 52d2b71f1588..f09b096ba4fd 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -257,54 +257,6 @@ static int ttm_copy_io_page(void *dst, void *src, unsigned
long page)
return 0;
}
-#ifdef CONFIG_X86
-#define __ttm_kmap_atomic_prot(__page, __prot) kmap_atomic_prot(__page, __prot)
-#define __ttm_kunmap_atomic(__addr) kunmap_atomic(__addr)
-#else
-#define __ttm_kmap_atomic_prot(__page, __prot) vmap(&__page, 1, 0, __prot)
-#define __ttm_kunmap_atomic(__addr) vunmap(__addr)
-#endif
-
-
-/**
- * ttm_kmap_atomic_prot - Efficient kernel map of a single page with
- * specified page protection.
- *
- * @page: The page to map.
- * @prot: The page protection.
- *
- * This function maps a TTM page using the kmap_atomic api if available,
- * otherwise falls back to vmap. The user must make sure that the
- * specified page does not have an aliased mapping with a different caching
- * policy unless the architecture explicitly allows it. Also mapping and
- * unmapping using this api must be correctly nested. Unmapping should
- * occur in the reverse order of mapping.
- */
-void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot)
-{
- if (pgprot_val(prot) == pgprot_val(PAGE_KERNEL))
- return kmap_atomic(page);
- else
- return __ttm_kmap_atomic_prot(page, prot);
-}
-EXPORT_SYMBOL(ttm_kmap_atomic_prot);
-
-/**
- * ttm_kunmap_atomic_prot - Unmap a page that was mapped using
- * ttm_kmap_atomic_prot.
- *
- * @addr: The virtual address from the map.
- * @prot: The page protection.
- */
-void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot)
-{
- if (pgprot_val(prot) == pgprot_val(PAGE_KERNEL))
- kunmap_atomic(addr);
- else
- __ttm_kunmap_atomic(addr);
-}
-EXPORT_SYMBOL(ttm_kunmap_atomic_prot);
-
static int ttm_copy_io_ttm_page(struct ttm_tt *ttm, void *src,
unsigned long page,
pgprot_t prot)
@@ -316,13 +268,13 @@ static int ttm_copy_io_ttm_page(struct ttm_tt *ttm, void
*src,
return -ENOMEM;
src = (void *)((unsigned long)src + (page << PAGE_SHIFT));
- dst = ttm_kmap_atomic_prot(d, prot);
+ dst = kmap_atomic_prot(d, prot);
if (!dst)
return -ENOMEM;
memcpy_fromio(dst, src, PAGE_SIZE);
- ttm_kunmap_atomic_prot(dst, prot);
+ kunmap_atomic(dst);
return 0;
}
@@ -338,13 +290,13 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void
*dst,
return -ENOMEM;
dst = (void *)((unsigned long)dst + (page << PAGE_SHIFT));
- src = ttm_kmap_atomic_prot(s, prot);
+ src = kmap_atomic_prot(s, prot);
if (!src)
return -ENOMEM;
memcpy_toio(dst, src, PAGE_SIZE);
- ttm_kunmap_atomic_prot(src, prot);
+ kunmap_atomic(src);
return 0;
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
index bb46ca0c458f..94d456a1d1a9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
@@ -374,12 +374,12 @@ static int vmw_bo_cpu_blit_line(struct
vmw_bo_blit_line_data *d,
copy_size = min_t(u32, copy_size, PAGE_SIZE - src_page_offset);
if (unmap_src) {
- ttm_kunmap_atomic_prot(d->src_addr, d->src_prot);
+ kunmap_atomic(d->src_addr);
d->src_addr = NULL;
}
if (unmap_dst) {
- ttm_kunmap_atomic_prot(d->dst_addr, d->dst_prot);
+ kunmap_atomic(d->dst_addr);
d->dst_addr = NULL;
}
@@ -388,8 +388,8 @@ static int vmw_bo_cpu_blit_line(struct vmw_bo_blit_line_data *d,
return -EINVAL;
d->dst_addr =
- ttm_kmap_atomic_prot(d->dst_pages[dst_page],
-d->dst_prot);
+ kmap_atomic_prot(d->dst_pages[dst_page],
+d->dst_prot);
Re: [PATCH V1 01/10] arch/kmap: Remove BUG_ON()
On Thu, Apr 30, 2020 at 01:38:36PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > Replace the use of BUG_ON(in_interrupt()) in the kmap() and kunmap() > in favor of might_sleep(). > > Besides the benefits of might_sleep(), this normalizes the > implementations such that they can be made generic in subsequent > patches. > > Reviewed-by: Dan Williams > Signed-off-by: Ira Weiny Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 02/10] arch/xtensa: Move kmap build bug out of the way
On Thu, Apr 30, 2020 at 01:38:37PM -0700, ira.we...@intel.com wrote:
> @@ -88,6 +88,11 @@ void __init kmap_init(void)
> {
> unsigned long kmap_vstart;
>
> + /* Check if this memory layout is broken because PKMAP overlaps
> + * page table.
> + */
> + BUILD_BUG_ON(PKMAP_BASE <
> + TLBTEMP_BASE_1 + TLBTEMP_SIZE);
This can fit on a single line. Otherwise looks good:
Reviewed-by: Christoph Hellwig
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 03/10] arch/kmap: Remove redundant arch specific kmaps
Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 04/10] arch/kunmap: Remove duplicate kunmap implementations
On Thu, Apr 30, 2020 at 01:38:39PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > All architectures do exactly the same thing for kunmap(); remove all the > duplicate definitions and lift the call to the core. > > This also has the benefit of changing kmap_unmap() on a number of > architectures to be an inline call rather than an actual function. > > Signed-off-by: Ira Weiny Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 05/10] arch/kmap_atomic: Consolidate duplicate code
On Thu, Apr 30, 2020 at 01:38:40PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > Every arch has the same code to ensure atomic operations and a check for > !HIGHMEM page. > > Remove the duplicate code by defining a core kmap_atomic() which only > calls the arch specific kmap_atomic_high() when the page is high memory. > > Signed-off-by: Ira Weiny Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 06/10] arch/kunmap_atomic: Consolidate duplicate code
On Thu, Apr 30, 2020 at 01:38:41PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > Every single architecture (including !CONFIG_HIGHMEM) calls... > > pagefault_enable(); > preempt_enable(); > > ... before returning from __kunmap_atomic(). Lift this code into the > kunmap_atomic() macro. > > While we are at it rename __kunmap_atomic() to kunmap_atomic_high() to > be consistent. > > Signed-off-by: Ira Weiny Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
sparc-related comment, to Re: [PATCH V1 07/10] arch/kmap: Ensure kmap_prot visibility
> --- a/arch/sparc/mm/highmem.c > +++ b/arch/sparc/mm/highmem.c > @@ -33,6 +33,7 @@ > #include > > pgprot_t kmap_prot; > +EXPORT_SYMBOL(kmap_prot); Btw, I don't see why sparc needs this as a variable, as there is just a single assignment to it. If sparc is sorted out we can always make it a define, and use a define for kmap_prot that defaults to PAGE_KERNEL, avoiding a little more duplication. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 08/10] arch/kmap: Don't hard code kmap_prot values
On Thu, Apr 30, 2020 at 01:38:43PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > To support kmap_atomic_prot() on all architectures each arch must > support protections passed in to them. > > Change csky, mips, nds32 and xtensa to use their global kmap_prot value > rather than a hard coded value which was equal. Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 09/10] arch/kmap: Define kmap_atomic_prot() for all arch's
On Thu, Apr 30, 2020 at 01:38:44PM -0700, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> To support kmap_atomic_prot(), all architectures need to support
> protections passed to their kmap_atomic_high() function. Pass
> protections into kmap_atomic_high() and change the name to
> kmap_atomic_high_prot() to match.
>
> Then define kmap_atomic_prot() as a core function which calls
> kmap_atomic_high_prot() when needed.
>
> Finally, redefine kmap_atomic() as a wrapper of kmap_atomic_prot() with
> the default kmap_prot exported by the architectures.
Looks good,
Reviewed-by: Christoph Hellwig
But can you also consolidate the kmap_atomic_high_prot and
kunmap_atomic_high in linux/highmem.h instead of keeping the duplicates
in all arch headers?
>
> Signed-off-by: Ira Weiny
> ---
> arch/arc/include/asm/highmem.h| 2 +-
> arch/arc/mm/highmem.c | 6 +++---
> arch/arm/include/asm/highmem.h| 2 +-
> arch/arm/mm/highmem.c | 6 +++---
> arch/csky/include/asm/highmem.h | 2 +-
> arch/csky/mm/highmem.c| 6 +++---
> arch/microblaze/include/asm/highmem.h | 7 +--
> arch/microblaze/mm/highmem.c | 4 ++--
> arch/mips/include/asm/highmem.h | 2 +-
> arch/mips/mm/highmem.c| 6 +++---
> arch/nds32/include/asm/highmem.h | 2 +-
> arch/nds32/mm/highmem.c | 6 +++---
> arch/powerpc/include/asm/highmem.h| 8 +---
> arch/powerpc/mm/highmem.c | 4 ++--
> arch/sparc/include/asm/highmem.h | 2 +-
> arch/sparc/mm/highmem.c | 6 +++---
> arch/x86/include/asm/highmem.h| 6 +-
> arch/x86/mm/highmem_32.c | 4 ++--
> arch/xtensa/include/asm/highmem.h | 2 +-
> arch/xtensa/mm/highmem.c | 6 +++---
> include/linux/highmem.h | 5 +++--
> 21 files changed, 40 insertions(+), 54 deletions(-)
>
> diff --git a/arch/arc/include/asm/highmem.h b/arch/arc/include/asm/highmem.h
> index e16531495620..09f86bde6809 100644
> --- a/arch/arc/include/asm/highmem.h
> +++ b/arch/arc/include/asm/highmem.h
> @@ -30,7 +30,7 @@
>
> #include
>
> -extern void *kmap_atomic_high(struct page *page);
> +extern void *kmap_atomic_high_prot(struct page *page, pgprot_t prot);
> extern void kunmap_atomic_high(void *kvaddr);
>
> extern void kmap_init(void);
> diff --git a/arch/arc/mm/highmem.c b/arch/arc/mm/highmem.c
> index 5d3eab4ac0b0..479b0d72d3cf 100644
> --- a/arch/arc/mm/highmem.c
> +++ b/arch/arc/mm/highmem.c
> @@ -49,7 +49,7 @@
> extern pte_t * pkmap_page_table;
> static pte_t * fixmap_page_table;
>
> -void *kmap_atomic_high(struct page *page)
> +void *kmap_atomic_high_prot(struct page *page, pgprot_t prot)
> {
> int idx, cpu_idx;
> unsigned long vaddr;
> @@ -59,11 +59,11 @@ void *kmap_atomic_high(struct page *page)
> vaddr = FIXMAP_ADDR(idx);
>
> set_pte_at(&init_mm, vaddr, fixmap_page_table + idx,
> -mk_pte(page, kmap_prot));
> +mk_pte(page, prot));
>
> return (void *)vaddr;
> }
> -EXPORT_SYMBOL(kmap_atomic_high);
> +EXPORT_SYMBOL(kmap_atomic_high_prot);
>
> void kunmap_atomic_high(void *kv)
> {
> diff --git a/arch/arm/include/asm/highmem.h b/arch/arm/include/asm/highmem.h
> index a9d5e9bce1cc..e35f2f73f6aa 100644
> --- a/arch/arm/include/asm/highmem.h
> +++ b/arch/arm/include/asm/highmem.h
> @@ -60,7 +60,7 @@ static inline void *kmap_high_get(struct page *page)
> * when CONFIG_HIGHMEM is not set.
> */
> #ifdef CONFIG_HIGHMEM
> -extern void *kmap_atomic_high(struct page *page);
> +extern void *kmap_atomic_high_prot(struct page *page, pgprot_t prot);
> extern void kunmap_atomic_high(void *kvaddr);
> extern void *kmap_atomic_pfn(unsigned long pfn);
> #endif
> diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c
> index ac8394655a6e..e013f6b81328 100644
> --- a/arch/arm/mm/highmem.c
> +++ b/arch/arm/mm/highmem.c
> @@ -31,7 +31,7 @@ static inline pte_t get_fixmap_pte(unsigned long vaddr)
> return *ptep;
> }
>
> -void *kmap_atomic_high(struct page *page)
> +void *kmap_atomic_high_prot(struct page *page, pgprot_t prot)
> {
> unsigned int idx;
> unsigned long vaddr;
> @@ -67,11 +67,11 @@ void *kmap_atomic_high(struct page *page)
>* in place, so the contained TLB flush ensures the TLB is updated
>* with the new mapping.
>*/
> - set_fixmap_pte(idx, mk_pte(page, kmap_prot));
> + set_fixmap_pte(idx, mk_pte(page, prot));
>
> return (void *)vaddr;
> }
> -EXPORT_SYMBOL(kmap_atomic_high);
> +EXPORT_SYMBOL(kmap_atomic_high_prot);
>
> void kunmap_atomic_high(void *kvaddr)
> {
> diff --git a/arch/csky/include/asm/highmem.h b/arch/csky/include/asm/highmem.h
> index 5bbbe59e60a9..59854c7ccf78 100644
> --- a/arch/csky/include/asm/highmem.h
> +++ b/arch/csky/include/asm/highmem.h
> @@ -32,7 +32,7 @@ extern pte_t *pkmap_page_table;
>
> #define ARCH_HAS_KMAP_FLUSH_TLB
> extern void kmap_flush_tlb
Re: [PATCH V1 10/10] drm: Remove drm specific kmap_atomic code
Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 00/10] Remove duplicated kmap code
In addition to the work already it the series, it seems like LAST_PKMAP_MASK, PKMAP_ADDR and PKMAP_NR can also be consolidated to common code. Also kmap_atomic_high_prot / kmap_atomic_pfn could move into common code, maybe keyed off a symbol selected by the actual users that need it. It also seems like it doesn't actually ever need to be exported. This in turn would lead to being able to allow io_mapping_map_atomic_wc on all architectures, which might make nouveau and qxl happy, but maybe that can be left for another series. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: sparc-related comment, to Re: [PATCH V1 07/10] arch/kmap: Ensure kmap_prot visibility
On Fri, May 01, 2020 at 01:44:46AM -0700, Christoph Hellwig wrote: > > --- a/arch/sparc/mm/highmem.c > > +++ b/arch/sparc/mm/highmem.c > > @@ -33,6 +33,7 @@ > > #include > > > > pgprot_t kmap_prot; > > +EXPORT_SYMBOL(kmap_prot); > > Btw, I don't see why sparc needs this as a variable, as there is just > a single assignment to it. Because sparc uses non-standard defines which I'm not familiar with. kmap_prot = __pgprot(SRMMU_ET_PTE | SRMMU_PRIV | SRMMU_CACHE); SRMMU_ET_PTE and friends are defined in arch/sparc/include/asm/pgtsrmmu.h Since I can't readily test sparc this was easier to put out than let 0-day crank on the entire series checking if including that header in the common header chain would be an issue. > > If sparc is sorted out we can always make it a define, and use a define > for kmap_prot that defaults to PAGE_KERNEL, avoiding a little > more duplication. Agreed. But it seems easier as a follow up (for me with 0-day). Perhaps someone from sparc can weigh in on the specifics of those defines and why they are different from the normal ones? Or even provide a follow on patch? Ira ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 00/10] Remove duplicated kmap code
On Fri, May 01, 2020 at 01:54:56AM -0700, Christoph Hellwig wrote: > In addition to the work already it the series, it seems like > LAST_PKMAP_MASK, PKMAP_ADDR and PKMAP_NR can also be consolidated > to common code. Agreed, I mentioned in the cover letter there are similarities... > > Also kmap_atomic_high_prot / kmap_atomic_pfn could move into common > code, maybe keyed off a symbol selected by the actual users that > need it. It also seems like it doesn't actually ever need to be > exported. ... but these are not as readily obvious, at least to me. I do see a pattern but the differences seemed subtle enough that it would take a while to ensure correctness. So I'd like to see this series go in and build on it. > > This in turn would lead to being able to allow io_mapping_map_atomic_wc > on all architectures, which might make nouveau and qxl happy, but maybe > that can be left for another series. I agree, that this should be follow on patches. I still need to fix the bisect-ability and I don't want to bog down 0-day with a longer series. Thanks for the review! Ira ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
