[v2,9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

  From: Zhao Liu <zhao1.liu@intel.com>

The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1], and this patch converts the calls 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 (the preemption is
disabled for !PREEMPT_RT case, otherwise it only disables migration).

With kmap_local_page(), we can avoid the often unwanted side effect of
unnecessary page faults and preemption disables.

In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
kmap_atomic() in eb_relocate_entry(), and is unmapped by
kunmap_atomic() in reloc_cache_reset().

And this mapping/unmapping occurs in two places: one is in
eb_relocate_vma(), and another is in eb_relocate_vma_slow().

The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
need to disable pagefaults and preemption during the above mapping/
unmapping.

So it can simply use kmap_local_page() / kunmap_local() that can
instead do the mapping / unmapping regardless of the context.

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: No code change since v1. Added description of the motivation of
    using kmap_local_page() and "Suggested-by" tag of Fabio.

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credits:
  Ira: Referred to his task document, review comments.
  Fabio: Referred to his boiler plate commit message and his description
         about why kmap_local_page() should be preferred.
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the calls 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 (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
> kmap_atomic() in eb_relocate_entry(), and is unmapped by
> kunmap_atomic() in reloc_cache_reset().

First off thanks for the series and sticking with this.  That said this
patch kind of threw me for a loop because tracing the map/unmap calls did
not make sense to me.  See below.

> 
> And this mapping/unmapping occurs in two places: one is in
> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
> 
> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
> need to disable pagefaults and preemption during the above mapping/
> unmapping.
> 
> So it can simply use kmap_local_page() / kunmap_local() that can
> instead do the mapping / unmapping regardless of the context.
> 
> 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: No code change since v1. Added description of the motivation of
>     using kmap_local_page() and "Suggested-by" tag of Fabio.
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Suggested by credits:
>   Ira: Referred to his task document, review comments.
>   Fabio: Referred to his boiler plate commit message and his description
>          about why kmap_local_page() should be preferred.
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 9dce2957b4e5..805565edd148 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1151,7 +1151,7 @@ static void reloc_cache_unmap(struct reloc_cache *cache)
>  
>  	vaddr = unmask_page(cache->vaddr);
>  	if (cache->vaddr & KMAP)
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);

In the cover letter you don't mention this unmap path.  Rather you mention
only reloc_cache_reset().

After digging into this and considering these are kmap_atomic() calls I
_think_ what you have is ok.  But I think I'd like to see the call paths
documented a bit more clearly.  Or perhaps cleaned up a lot.

For example I see the following call possibility from a user ioctl.  In
this trace I see 2 examples where something is unmapped first.  I don't
understand why that is required?  I would assume reloc_cache_unmap() and
reloc_kmap() are helpers called from somewhere else requiring a remapping
of the cache but I don't see it.

i915_gem_execbuffer2_ioctl()
eb_relocate_parse()
eb_relocate_parse_slow()
eb_relocate_vma_slow()
	eb_relocate_entry()
		reloc_cache_unmap()
			kunmap_atomic()  <=== HERE!
		reloc_cache_remap()
			kmap_atomic()
		relocate_entry()
			reloc_vaddr()
				reloc_kmap()
					kunmap_atomic() <== HERE!
					kmap_atomic()

	reloc_cache_reset()
		kunmap_atomic()

Could these mappings be cleaned up a lot more?  Perhaps by removing some
of the helper functions which AFAICT are left over from older versions of
the code?

Also as an aside I think it is really bad that eb_relocate_entry() returns
negative errors in a u64.  Better to get the types right IMO.

Thanks for the series!
Ira

>  	else
>  		io_mapping_unmap_atomic((void __iomem *)vaddr);
>  }
> @@ -1167,7 +1167,7 @@ static void reloc_cache_remap(struct reloc_cache *cache,
>  	if (cache->vaddr & KMAP) {
>  		struct page *page = i915_gem_object_get_page(obj, cache->page);
>  
> -		vaddr = kmap_atomic(page);
> +		vaddr = kmap_local_page(page);
>  		cache->vaddr = unmask_flags(cache->vaddr) |
>  			(unsigned long)vaddr;
>  	} else {
> @@ -1197,7 +1197,7 @@ static void reloc_cache_reset(struct reloc_cache *cache, struct i915_execbuffer
>  		if (cache->vaddr & CLFLUSH_AFTER)
>  			mb();
>  
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);
>  		i915_gem_object_finish_access(obj);
>  	} else {
>  		struct i915_ggtt *ggtt = cache_to_ggtt(cache);
> @@ -1229,7 +1229,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>  	struct page *page;
>  
>  	if (cache->vaddr) {
> -		kunmap_atomic(unmask_page(cache->vaddr));
> +		kunmap_local(unmask_page(cache->vaddr));
>  	} else {
>  		unsigned int flushes;
>  		int err;
> @@ -1251,7 +1251,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>  	if (!obj->mm.dirty)
>  		set_page_dirty(page);
>  
> -	vaddr = kmap_atomic(page);
> +	vaddr = kmap_local_page(page);
>  	cache->vaddr = unmask_flags(cache->vaddr) | (unsigned long)vaddr;
>  	cache->page = pageno;
>  
> -- 
> 2.34.1
>
  

On 31/03/2023 05:18, Ira Weiny wrote:
> Zhao Liu wrote:
>> From: Zhao Liu <zhao1.liu@intel.com>
>>
>> The use of kmap_atomic() is being deprecated in favor of
>> kmap_local_page()[1], and this patch converts the calls 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 (the preemption is
>> disabled for !PREEMPT_RT case, otherwise it only disables migration).
>>
>> With kmap_local_page(), we can avoid the often unwanted side effect of
>> unnecessary page faults and preemption disables.
>>
>> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
>> kmap_atomic() in eb_relocate_entry(), and is unmapped by
>> kunmap_atomic() in reloc_cache_reset().
> 
> First off thanks for the series and sticking with this.  That said this
> patch kind of threw me for a loop because tracing the map/unmap calls did
> not make sense to me.  See below.
> 
>>
>> And this mapping/unmapping occurs in two places: one is in
>> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
>>
>> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
>> need to disable pagefaults and preemption during the above mapping/
>> unmapping.
>>
>> So it can simply use kmap_local_page() / kunmap_local() that can
>> instead do the mapping / unmapping regardless of the context.
>>
>> 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: No code change since v1. Added description of the motivation of
>>      using kmap_local_page() and "Suggested-by" tag of Fabio.
>>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> ---
>> Suggested by credits:
>>    Ira: Referred to his task document, review comments.
>>    Fabio: Referred to his boiler plate commit message and his description
>>           about why kmap_local_page() should be preferred.
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 9dce2957b4e5..805565edd148 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -1151,7 +1151,7 @@ static void reloc_cache_unmap(struct reloc_cache *cache)
>>   
>>   	vaddr = unmask_page(cache->vaddr);
>>   	if (cache->vaddr & KMAP)
>> -		kunmap_atomic(vaddr);
>> +		kunmap_local(vaddr);
> 
> In the cover letter you don't mention this unmap path.  Rather you mention
> only reloc_cache_reset().
> 
> After digging into this and considering these are kmap_atomic() calls I
> _think_ what you have is ok.  But I think I'd like to see the call paths
> documented a bit more clearly.  Or perhaps cleaned up a lot.
> 
> For example I see the following call possibility from a user ioctl.  In
> this trace I see 2 examples where something is unmapped first.  I don't
> understand why that is required?  I would assume reloc_cache_unmap() and
> reloc_kmap() are helpers called from somewhere else requiring a remapping
> of the cache but I don't see it.

Reloc_cache_unmap is called from eb_relocate_entry.

The confusing part unmap appears first is just because reloc_cache is a 
stateful setup. The previous mapping is kept around until reset (callers 
moves to a different parent object), and unampped/remapped once moved to 
a different page within that object.

However I am unsure if disabling pagefaulting is needed or not. Thomas, 
Matt, being the last to touch this area, perhaps you could have a look? 
Because I notice we have a fallback iomap path which still uses 
io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is 
safe, does the iomap side also needs converting to 
io_mapping_map_local_wc? Or they have separate requirements?

Regards,

Tvrtko

> 
> i915_gem_execbuffer2_ioctl()
> eb_relocate_parse()
> eb_relocate_parse_slow()
> eb_relocate_vma_slow()
> 	eb_relocate_entry()
> 		reloc_cache_unmap()
> 			kunmap_atomic()  <=== HERE!
> 		reloc_cache_remap()
> 			kmap_atomic()
> 		relocate_entry()
> 			reloc_vaddr()
> 				reloc_kmap()
> 					kunmap_atomic() <== HERE!
> 					kmap_atomic()
> 
> 	reloc_cache_reset()
> 		kunmap_atomic()
> 
> Could these mappings be cleaned up a lot more?  Perhaps by removing some
> of the helper functions which AFAICT are left over from older versions of
> the code?
> 
> Also as an aside I think it is really bad that eb_relocate_entry() returns
> negative errors in a u64.  Better to get the types right IMO.
> 
> Thanks for the series!
> Ira
> 
>>   	else
>>   		io_mapping_unmap_atomic((void __iomem *)vaddr);
>>   }
>> @@ -1167,7 +1167,7 @@ static void reloc_cache_remap(struct reloc_cache *cache,
>>   	if (cache->vaddr & KMAP) {
>>   		struct page *page = i915_gem_object_get_page(obj, cache->page);
>>   
>> -		vaddr = kmap_atomic(page);
>> +		vaddr = kmap_local_page(page);
>>   		cache->vaddr = unmask_flags(cache->vaddr) |
>>   			(unsigned long)vaddr;
>>   	} else {
>> @@ -1197,7 +1197,7 @@ static void reloc_cache_reset(struct reloc_cache *cache, struct i915_execbuffer
>>   		if (cache->vaddr & CLFLUSH_AFTER)
>>   			mb();
>>   
>> -		kunmap_atomic(vaddr);
>> +		kunmap_local(vaddr);
>>   		i915_gem_object_finish_access(obj);
>>   	} else {
>>   		struct i915_ggtt *ggtt = cache_to_ggtt(cache);
>> @@ -1229,7 +1229,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>>   	struct page *page;
>>   
>>   	if (cache->vaddr) {
>> -		kunmap_atomic(unmask_page(cache->vaddr));
>> +		kunmap_local(unmask_page(cache->vaddr));
>>   	} else {
>>   		unsigned int flushes;
>>   		int err;
>> @@ -1251,7 +1251,7 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>>   	if (!obj->mm.dirty)
>>   		set_page_dirty(page);
>>   
>> -	vaddr = kmap_atomic(page);
>> +	vaddr = kmap_local_page(page);
>>   	cache->vaddr = unmask_flags(cache->vaddr) | (unsigned long)vaddr;
>>   	cache->page = pageno;
>>   
>> -- 
>> 2.34.1
>>
> 
>
  

On venerdì 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
> On 31/03/2023 05:18, Ira Weiny wrote:
> > Zhao Liu wrote:
> >> From: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> The use of kmap_atomic() is being deprecated in favor of
> >> kmap_local_page()[1], and this patch converts the calls 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 (the preemption is
> >> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> >> 
> >> With kmap_local_page(), we can avoid the often unwanted side effect of
> >> unnecessary page faults and preemption disables.
> >> 
> >> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
> >> kmap_atomic() in eb_relocate_entry(), and is unmapped by
> >> kunmap_atomic() in reloc_cache_reset().
> > 
> > First off thanks for the series and sticking with this.  That said this
> > patch kind of threw me for a loop because tracing the map/unmap calls did
> > not make sense to me.  See below.
> > 
> >> And this mapping/unmapping occurs in two places: one is in
> >> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
> >> 
> >> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
> >> need to disable pagefaults and preemption during the above mapping/
> >> unmapping.
> >> 
> >> So it can simply use kmap_local_page() / kunmap_local() that can
> >> instead do the mapping / unmapping regardless of the context.
> >> 
> >> 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: No code change since v1. Added description of the motivation of
> >> 
> >>      using kmap_local_page() and "Suggested-by" tag of Fabio.
> >> 
> >> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> >> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> >> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> ---
> >> 
> >> Suggested by credits:
> >>    Ira: Referred to his task document, review comments.
> >>    Fabio: Referred to his boiler plate commit message and his description
> >>    
> >>           about why kmap_local_page() should be preferred.
> >> 
> >> ---
> >> 
> >>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >> 

[snip]
 
> However I am unsure if disabling pagefaulting is needed or not. Thomas,
> Matt, being the last to touch this area, perhaps you could have a look?
> Because I notice we have a fallback iomap path which still uses
> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> safe, does the iomap side also needs converting to
> io_mapping_map_local_wc? Or they have separate requirements?

AFAIK, the requirements for io_mapping_map_local_wc() are the same as for 
kmap_local_page(): the kernel virtual address is _only_ valid in the caller 
context, and map/unmap nesting must be done in stack-based ordering (LIFO).

I think a follow up patch could safely switch to io_mapping_map_local_wc() / 
io_mapping_unmap_local_wc since the address is local to context.

However, not being an expert, reading your note now I suspect that I'm missing 
something. Can I ask why you think that page-faults disabling might be 
necessary? 

Thanks,

Fabio

> Regards,
> 
> Tvrtko
  

Thanks all for your review!

On Fri, Mar 31, 2023 at 05:32:17PM +0200, Fabio M. De Francesco wrote:
> Date: Fri, 31 Mar 2023 17:32:17 +0200
> From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>  gem/i915_gem_execbuffer.c
> 
> On venerd? 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
> > On 31/03/2023 05:18, Ira Weiny wrote:
> 

[snip]

>  
> > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > Matt, being the last to touch this area, perhaps you could have a look?
> > Because I notice we have a fallback iomap path which still uses
> > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > safe, does the iomap side also needs converting to
> > io_mapping_map_local_wc? Or they have separate requirements?
> 
> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for 
> kmap_local_page(): the kernel virtual address is _only_ valid in the caller 
> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> 
> I think a follow up patch could safely switch to io_mapping_map_local_wc() / 
> io_mapping_unmap_local_wc since the address is local to context.
> 
> However, not being an expert, reading your note now I suspect that I'm missing 
> something. Can I ask why you think that page-faults disabling might be 
> necessary? 


About the disabling of pagefault here, could you please talk more about
it? :-)

From previous discussions and commit history, I didn't find relevant
information and I lack background knowledge about it...

If we have the reason to diable pagefault, I will fix and refresh the new
version.

Thanks,
Zhao

> 
> Thanks,
> 
> Fabio
> 
> > Regards,
> > 
> > Tvrtko
> 
> 
>
  

On 31/03/2023 16:32, Fabio M. De Francesco wrote:
> On venerdì 31 marzo 2023 13:30:20 CEST Tvrtko Ursulin wrote:
>> On 31/03/2023 05:18, Ira Weiny wrote:
>>> Zhao Liu wrote:
>>>> From: Zhao Liu <zhao1.liu@intel.com>
>>>>
>>>> The use of kmap_atomic() is being deprecated in favor of
>>>> kmap_local_page()[1], and this patch converts the calls 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 (the preemption is
>>>> disabled for !PREEMPT_RT case, otherwise it only disables migration).
>>>>
>>>> With kmap_local_page(), we can avoid the often unwanted side effect of
>>>> unnecessary page faults and preemption disables.
>>>>
>>>> In i915_gem_execbuffer.c, eb->reloc_cache.vaddr is mapped by
>>>> kmap_atomic() in eb_relocate_entry(), and is unmapped by
>>>> kunmap_atomic() in reloc_cache_reset().
>>>
>>> First off thanks for the series and sticking with this.  That said this
>>> patch kind of threw me for a loop because tracing the map/unmap calls did
>>> not make sense to me.  See below.
>>>
>>>> And this mapping/unmapping occurs in two places: one is in
>>>> eb_relocate_vma(), and another is in eb_relocate_vma_slow().
>>>>
>>>> The function eb_relocate_vma() or eb_relocate_vma_slow() doesn't
>>>> need to disable pagefaults and preemption during the above mapping/
>>>> unmapping.
>>>>
>>>> So it can simply use kmap_local_page() / kunmap_local() that can
>>>> instead do the mapping / unmapping regardless of the context.
>>>>
>>>> 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: No code change since v1. Added description of the motivation of
>>>>
>>>>       using kmap_local_page() and "Suggested-by" tag of Fabio.
>>>>
>>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>>> ---
>>>>
>>>> Suggested by credits:
>>>>     Ira: Referred to his task document, review comments.
>>>>     Fabio: Referred to his boiler plate commit message and his description
>>>>     
>>>>            about why kmap_local_page() should be preferred.
>>>>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
> 
> [snip]
>   
>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>> Matt, being the last to touch this area, perhaps you could have a look?
>> Because I notice we have a fallback iomap path which still uses
>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>> safe, does the iomap side also needs converting to
>> io_mapping_map_local_wc? Or they have separate requirements?
> 
> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> 
> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> io_mapping_unmap_local_wc since the address is local to context.
> 
> However, not being an expert, reading your note now I suspect that I'm missing
> something. Can I ask why you think that page-faults disabling might be
> necessary?

I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.

I guess it will work since the copying is done like this anyway:

		/*
		 * This is the fast path and we cannot handle a pagefault
		 * whilst holding the struct mutex lest the user pass in the
		 * relocations contained within a mmaped bo. For in such a case
		 * we, the page fault handler would call i915_gem_fault() and
		 * we would try to acquire the struct mutex again. Obviously
		 * this is bad and so lockdep complains vehemently.
		 */
		pagefault_disable();
		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
		pagefault_enable();
		if (unlikely(copied)) {
			remain = -EFAULT;
			goto out;
		}

Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.

Patch to convert the io_mapping_map_atomic_wc can indeed come later.

In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?

Regards,

Tvrtko
  

Hi Tvrtko,

On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:

[snip]

> > 
> > [snip]
> > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > Matt, being the last to touch this area, perhaps you could have a look?
> > > Because I notice we have a fallback iomap path which still uses
> > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > safe, does the iomap side also needs converting to
> > > io_mapping_map_local_wc? Or they have separate requirements?
> > 
> > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > 
> > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > io_mapping_unmap_local_wc since the address is local to context.
> > 
> > However, not being an expert, reading your note now I suspect that I'm missing
> > something. Can I ask why you think that page-faults disabling might be
> > necessary?
> 
> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> 
> I guess it will work since the copying is done like this anyway:
> 
> 		/*
> 		 * This is the fast path and we cannot handle a pagefault
> 		 * whilst holding the struct mutex lest the user pass in the
> 		 * relocations contained within a mmaped bo. For in such a case
> 		 * we, the page fault handler would call i915_gem_fault() and
> 		 * we would try to acquire the struct mutex again. Obviously
> 		 * this is bad and so lockdep complains vehemently.
> 		 */
> 		pagefault_disable();
> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> 		pagefault_enable();
> 		if (unlikely(copied)) {
> 			remain = -EFAULT;
> 			goto out;
> 		}
> 
> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.

Thanks for your explanation!

> 
> Patch to convert the io_mapping_map_atomic_wc can indeed come later.

Okay, I will also look at this.

> 
> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?

Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?

Thanks,
Zhao

> 
> Regards,
> 
> Tvrtko
  

On 14/04/2023 11:45, Zhao Liu wrote:
> Hi Tvrtko,
> 
> On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> 
> [snip]
> 
>>>
>>> [snip]
>>>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>>>> Matt, being the last to touch this area, perhaps you could have a look?
>>>> Because I notice we have a fallback iomap path which still uses
>>>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>>>> safe, does the iomap side also needs converting to
>>>> io_mapping_map_local_wc? Or they have separate requirements?
>>>
>>> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
>>> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
>>> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
>>>
>>> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
>>> io_mapping_unmap_local_wc since the address is local to context.
>>>
>>> However, not being an expert, reading your note now I suspect that I'm missing
>>> something. Can I ask why you think that page-faults disabling might be
>>> necessary?
>>
>> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
>>
>> I guess it will work since the copying is done like this anyway:
>>
>> 		/*
>> 		 * This is the fast path and we cannot handle a pagefault
>> 		 * whilst holding the struct mutex lest the user pass in the
>> 		 * relocations contained within a mmaped bo. For in such a case
>> 		 * we, the page fault handler would call i915_gem_fault() and
>> 		 * we would try to acquire the struct mutex again. Obviously
>> 		 * this is bad and so lockdep complains vehemently.
>> 		 */
>> 		pagefault_disable();
>> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
>> 		pagefault_enable();
>> 		if (unlikely(copied)) {
>> 			remain = -EFAULT;
>> 			goto out;
>> 		}
>>
>> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> 
> Thanks for your explanation!
> 
>>
>> Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> 
> Okay, I will also look at this.
> 
>>
>> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> 
> Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?

Yes, but just because we failed to review and merge in time, not because 
you did not provide patches in time.

Regards,

Tvrtko
  

On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/04/2023 11:45, Zhao Liu wrote:
> > Hi Tvrtko,
> > 
> > On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> > 
> > [snip]
> > 
> > > > 
> > > > [snip]
> > > > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > > > Matt, being the last to touch this area, perhaps you could have a look?
> > > > > Because I notice we have a fallback iomap path which still uses
> > > > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > > > safe, does the iomap side also needs converting to
> > > > > io_mapping_map_local_wc? Or they have separate requirements?
> > > > 
> > > > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > > > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > > > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > > > 
> > > > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > > > io_mapping_unmap_local_wc since the address is local to context.
> > > > 
> > > > However, not being an expert, reading your note now I suspect that I'm missing
> > > > something. Can I ask why you think that page-faults disabling might be
> > > > necessary?
> > > 
> > > I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> > > 
> > > I guess it will work since the copying is done like this anyway:
> > > 
> > > 		/*
> > > 		 * This is the fast path and we cannot handle a pagefault
> > > 		 * whilst holding the struct mutex lest the user pass in the
> > > 		 * relocations contained within a mmaped bo. For in such a case
> > > 		 * we, the page fault handler would call i915_gem_fault() and
> > > 		 * we would try to acquire the struct mutex again. Obviously
> > > 		 * this is bad and so lockdep complains vehemently.
> > > 		 */
> > > 		pagefault_disable();
> > > 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> > > 		pagefault_enable();
> > > 		if (unlikely(copied)) {
> > > 			remain = -EFAULT;
> > > 			goto out;
> > > 		}
> > > 
> > > Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> > 
> > Thanks for your explanation!
> > 
> > > 
> > > Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> > 
> > Okay, I will also look at this.
> > 
> > > 
> > > In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> > 
> > Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
> 
> Yes, but just because we failed to review and merge in time, not because you
> did not provide patches in time.

It is worth mentioning that under drm we close the merge window earlier.
Around -rc5.

So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
is going to be sent there is already closed.

> 
> Regards,
> 
> Tvrtko
>
  

Hi Rodrigo and Tvrtko,

It seems this series is missed in v6.5.
This work should not be forgotten. Let me rebase and refresh the version.

Regards,
Zhao

On Mon, Apr 17, 2023 at 10:53:28AM -0400, Rodrigo Vivi wrote:
> Date: Mon, 17 Apr 2023 10:53:28 -0400
> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>  gem/i915_gem_execbuffer.c
> 
> On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 14/04/2023 11:45, Zhao Liu wrote:
> > > Hi Tvrtko,
> > > 
> > > On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > [snip]
> > > 
> > > > > 
> > > > > [snip]
> > > > > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > > > > Matt, being the last to touch this area, perhaps you could have a look?
> > > > > > Because I notice we have a fallback iomap path which still uses
> > > > > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > > > > safe, does the iomap side also needs converting to
> > > > > > io_mapping_map_local_wc? Or they have separate requirements?
> > > > > 
> > > > > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > > > > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > > > > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > > > > 
> > > > > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > > > > io_mapping_unmap_local_wc since the address is local to context.
> > > > > 
> > > > > However, not being an expert, reading your note now I suspect that I'm missing
> > > > > something. Can I ask why you think that page-faults disabling might be
> > > > > necessary?
> > > > 
> > > > I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> > > > 
> > > > I guess it will work since the copying is done like this anyway:
> > > > 
> > > > 		/*
> > > > 		 * This is the fast path and we cannot handle a pagefault
> > > > 		 * whilst holding the struct mutex lest the user pass in the
> > > > 		 * relocations contained within a mmaped bo. For in such a case
> > > > 		 * we, the page fault handler would call i915_gem_fault() and
> > > > 		 * we would try to acquire the struct mutex again. Obviously
> > > > 		 * this is bad and so lockdep complains vehemently.
> > > > 		 */
> > > > 		pagefault_disable();
> > > > 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> > > > 		pagefault_enable();
> > > > 		if (unlikely(copied)) {
> > > > 			remain = -EFAULT;
> > > > 			goto out;
> > > > 		}
> > > > 
> > > > Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> > > 
> > > Thanks for your explanation!
> > > 
> > > > 
> > > > Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> > > 
> > > Okay, I will also look at this.
> > > 
> > > > 
> > > > In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> > > 
> > > Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
> > 
> > Yes, but just because we failed to review and merge in time, not because you
> > did not provide patches in time.
> 
> It is worth mentioning that under drm we close the merge window earlier.
> Around -rc5.
> 
> So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
> is going to be sent there is already closed.
> 
> > 
> > Regards,
> > 
> > Tvrtko
> >
  

Hi,

On 18/10/2023 17:19, Zhao Liu wrote:
> Hi Rodrigo and Tvrtko,
> 
> It seems this series is missed in v6.5.
> This work should not be forgotten. Let me rebase and refresh the version.

Right it seems we did not manage to social engineer any reviews. Please 
do respin and we will try again.

Regards,

Tvrtko

> 
> Regards,
> Zhao
> 
> On Mon, Apr 17, 2023 at 10:53:28AM -0400, Rodrigo Vivi wrote:
>> Date: Mon, 17 Apr 2023 10:53:28 -0400
>> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
>>   gem/i915_gem_execbuffer.c
>>
>> On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
>>>
>>> On 14/04/2023 11:45, Zhao Liu wrote:
>>>> Hi Tvrtko,
>>>>
>>>> On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> [snip]
>>>>
>>>>>>
>>>>>> [snip]
>>>>>>> However I am unsure if disabling pagefaulting is needed or not. Thomas,
>>>>>>> Matt, being the last to touch this area, perhaps you could have a look?
>>>>>>> Because I notice we have a fallback iomap path which still uses
>>>>>>> io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
>>>>>>> safe, does the iomap side also needs converting to
>>>>>>> io_mapping_map_local_wc? Or they have separate requirements?
>>>>>>
>>>>>> AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
>>>>>> kmap_local_page(): the kernel virtual address is _only_ valid in the caller
>>>>>> context, and map/unmap nesting must be done in stack-based ordering (LIFO).
>>>>>>
>>>>>> I think a follow up patch could safely switch to io_mapping_map_local_wc() /
>>>>>> io_mapping_unmap_local_wc since the address is local to context.
>>>>>>
>>>>>> However, not being an expert, reading your note now I suspect that I'm missing
>>>>>> something. Can I ask why you think that page-faults disabling might be
>>>>>> necessary?
>>>>>
>>>>> I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
>>>>>
>>>>> I guess it will work since the copying is done like this anyway:
>>>>>
>>>>> 		/*
>>>>> 		 * This is the fast path and we cannot handle a pagefault
>>>>> 		 * whilst holding the struct mutex lest the user pass in the
>>>>> 		 * relocations contained within a mmaped bo. For in such a case
>>>>> 		 * we, the page fault handler would call i915_gem_fault() and
>>>>> 		 * we would try to acquire the struct mutex again. Obviously
>>>>> 		 * this is bad and so lockdep complains vehemently.
>>>>> 		 */
>>>>> 		pagefault_disable();
>>>>> 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
>>>>> 		pagefault_enable();
>>>>> 		if (unlikely(copied)) {
>>>>> 			remain = -EFAULT;
>>>>> 			goto out;
>>>>> 		}
>>>>>
>>>>> Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
>>>>
>>>> Thanks for your explanation!
>>>>
>>>>>
>>>>> Patch to convert the io_mapping_map_atomic_wc can indeed come later.
>>>>
>>>> Okay, I will also look at this.
>>>>
>>>>>
>>>>> In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
>>>>
>>>> Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
>>>
>>> Yes, but just because we failed to review and merge in time, not because you
>>> did not provide patches in time.
>>
>> It is worth mentioning that under drm we close the merge window earlier.
>> Around -rc5.
>>
>> So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
>> is going to be sent there is already closed.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>