Message-ID: <642654876a503_375f7e294e@iweiny-mobl.notmuch>
Date:   Thu, 30 Mar 2023 20:33:27 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Zhao Liu <zhao1.liu@...ux.intel.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Matthew Auld <matthew.auld@...el.com>,
        Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>,
        Nirmoy Das <nirmoy.das@...el.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Christian König <christian.koenig@....com>,
        <intel-gfx@...ts.freedesktop.org>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
CC:     Ira Weiny <ira.weiny@...el.com>,
        "Fabio M . De Francesco" <fmdefrancesco@...il.com>,
        Zhenyu Wang <zhenyu.z.wang@...el.com>,
        Zhao Liu <zhao1.liu@...el.com>,
        Dave Hansen <dave.hansen@...el.com>
Subject: Re: [PATCH v2 6/9] drm/i915: Use kmap_local_page() in
 gem/selftests/i915_gem_context.c

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@...el.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption.
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults or preemption disables.
> 
> In drm/i915/gem/selftests/i915_gem_context.c, functions cpu_fill() and
> cpu_check() mainly uses mapping to flush cache and check/assign the
> value.
> 
> There're 2 reasons why cpu_fill() and cpu_check() don't need to disable
> pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. cpu_fill() and cpu_check() call
> drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. Since
> CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in
> drm_clflush_virt_range(), the flush operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, cpu_fill() and cpu_check() are functions where the use of
> kmap_local_page() in place of kmap_atomic() is correctly suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * No code change since v1, and added description of the motivation of
>   using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@...el.com>
> Suggested-by: Ira Weiny <ira.weiny@...el.com>

First off I think this is fine.

But as I looked at this final selftests patch I began to wonder how the
memory being mapped here and in the previous selftests patches are
allocated.  Does highmem need to be considered at all?  Unfortunately, I
could not determine where the memory in the SG list of this test gem
object was allocated.

AFAICS cpu_fill() is only called in create_test_object().  Digging into
huge_gem_object() did not reveal where these pages were allocated from.

I wonder if these kmap_local_page() calls could be removed entirely based
on knowing that the pages were allocated from low mem?  Removing yet
another user of highmem altogether would be best if possible.

Do you know how these test objects are created?  Do the pages come from
user space somehow?

Regardless this is still a step in the right direction so:

Reviewed-by: Ira Weiny <ira.weiny@...el.com>

> Suggested-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Signed-off-by: Zhao Liu <zhao1.liu@...el.com>
> ---
> Suggested by credits:
>   Dave: Referred to his explanation about cache flush.
>   Ira: Referred to his task document, review comments and explanation
>        about cache flush.
>   Fabio: Referred to his boiler plate commit message and his description
>          about why kmap_local_page() should be preferred.
> ---
>  drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index a81fa6a20f5a..dcbc0b8e3323 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -481,12 +481,12 @@ static int cpu_fill(struct drm_i915_gem_object *obj, u32 value)
>  	for (n = 0; n < real_page_count(obj); n++) {
>  		u32 *map;
>  
> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>  		for (m = 0; m < DW_PER_PAGE; m++)
>  			map[m] = value;
>  		if (!has_llc)
>  			drm_clflush_virt_range(map, PAGE_SIZE);
> -		kunmap_atomic(map);
> +		kunmap_local(map);
>  	}
>  
>  	i915_gem_object_finish_access(obj);
> @@ -512,7 +512,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>  	for (n = 0; n < real_page_count(obj); n++) {
>  		u32 *map, m;
>  
> -		map = kmap_atomic(i915_gem_object_get_page(obj, n));
> +		map = kmap_local_page(i915_gem_object_get_page(obj, n));
>  		if (needs_flush & CLFLUSH_BEFORE)
>  			drm_clflush_virt_range(map, PAGE_SIZE);
>  
> @@ -538,7 +538,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj,
>  		}
>  
>  out_unmap:
> -		kunmap_atomic(map);
> +		kunmap_local(map);
>  		if (err)
>  			break;
>  	}
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists