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

[Ira Weiny <ira.weiny@intel.com>]

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