2021-03-04 17:54:01

by Thomas Gleixner

Subject: [patch 1/7] drm/ttm: Replace kmap_atomic() usage

From: Thomas Gleixner <[email protected]>

There is no reason to disable pagefaults and preemption as a side effect of
kmap_atomic_prot().

Use kmap_local_page_prot() instead and document the reasoning for the
mapping usage with the given pgprot.

Remove the NULL pointer check for the map. These functions return a valid
address for valid pages and the return was bogus anyway as it would have
left preemption and pagefaults disabled.

Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Christian Koenig <[email protected]>
Cc: Huang Rui <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
---
drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)

--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -181,13 +181,15 @@ static int ttm_copy_io_ttm_page(struct t
return -ENOMEM;

src = (void *)((unsigned long)src + (page << PAGE_SHIFT));
- dst = kmap_atomic_prot(d, prot);
- if (!dst)
- return -ENOMEM;
+ /*
+ * Ensure that a highmem page is mapped with the correct
+ * pgprot. For non highmem the mapping is already there.
+ */
+ dst = kmap_local_page_prot(d, prot);

memcpy_fromio(dst, src, PAGE_SIZE);

- kunmap_atomic(dst);
+ kunmap_local(dst);

return 0;
}
@@ -203,13 +205,15 @@ static int ttm_copy_ttm_io_page(struct t
return -ENOMEM;

dst = (void *)((unsigned long)dst + (page << PAGE_SHIFT));
- src = kmap_atomic_prot(s, prot);
- if (!src)
- return -ENOMEM;
+ /*
+ * Ensure that a highmem page is mapped with the correct
+ * pgprot. For non highmem the mapping is already there.
+ */
+ src = kmap_local_page_prot(s, prot);

memcpy_toio(dst, src, PAGE_SIZE);

- kunmap_atomic(src);
+ kunmap_local(src);

return 0;
}

2021-03-05 00:54:30

by Christian König

Subject: Re: [patch 1/7] drm/ttm: Replace kmap_atomic() usage

Am 03.03.21 um 14:20 schrieb Thomas Gleixner:
> From: Thomas Gleixner <[email protected]>
>
> There is no reason to disable pagefaults and preemption as a side effect of
> kmap_atomic_prot().
>
> Use kmap_local_page_prot() instead and document the reasoning for the
> mapping usage with the given pgprot.
>
> Remove the NULL pointer check for the map. These functions return a valid
> address for valid pages and the return was bogus anyway as it would have
> left preemption and pagefaults disabled.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Christian Koenig <[email protected]>
> Cc: Huang Rui <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> ---
> drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -181,13 +181,15 @@ static int ttm_copy_io_ttm_page(struct t
> return -ENOMEM;
>
> src = (void *)((unsigned long)src + (page << PAGE_SHIFT));
> - dst = kmap_atomic_prot(d, prot);
> - if (!dst)
> - return -ENOMEM;
> + /*
> + * Ensure that a highmem page is mapped with the correct
> + * pgprot. For non highmem the mapping is already there.
> + */

I find the comment a bit misleading. Maybe write:

/*
 * Locally map highmem pages with the correct pgprot.
 * Normal memory should already have the correct pgprot in the linear
mapping.
 */

Apart from that looks good to me.

Regards,
Christian.

> + dst = kmap_local_page_prot(d, prot);
>
> memcpy_fromio(dst, src, PAGE_SIZE);
>
> - kunmap_atomic(dst);
> + kunmap_local(dst);
>
> return 0;
> }
> @@ -203,13 +205,15 @@ static int ttm_copy_ttm_io_page(struct t
> return -ENOMEM;
>
> dst = (void *)((unsigned long)dst + (page << PAGE_SHIFT));
> - src = kmap_atomic_prot(s, prot);
> - if (!src)
> - return -ENOMEM;
> + /*
> + * Ensure that a highmem page is mapped with the correct
> + * pgprot. For non highmem the mapping is already there.
> + */
> + src = kmap_local_page_prot(s, prot);
>
> memcpy_toio(dst, src, PAGE_SIZE);
>
> - kunmap_atomic(src);
> + kunmap_local(src);
>
> return 0;
> }
>
>