From: Zhao Liu <[email protected]>
The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1].
The main difference between atomic and local mappings is that local
mappings doesn't disable page faults or preemption.
There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
need to disable pagefaults and preemption for mapping:
1. The flush operation is safe for CPU hotplug when preemption is not
disabled. In drm/i915/gem/i915_gem_object.c, the function
i915_gem_object_read_from_page_kmap() calls 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 and any issue with cpu's being added or removed
can be handled safely.
2. Any context switch caused by preemption or sleep (pagefault may
cause sleep) doesn't affect the validity of local mapping.
Therefore, i915_gem_object_read_from_page_kmap() is a function 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().
And remove the redundant variable that stores the address of the mapped
page since kunmap_local() can accept any pointer within the page.
[1]: https://lore.kernel.org/all/[email protected]
Suggested-by: Dave Hansen <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Suggested-by: Fabio M. De Francesco <[email protected]>
Signed-off-by: Zhao Liu <[email protected]>
---
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.
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 369006c5317f..a0072abed75e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -413,17 +413,15 @@ void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,
static void
i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64 offset, void *dst, int size)
{
- void *src_map;
void *src_ptr;
- src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >> PAGE_SHIFT));
-
- src_ptr = src_map + offset_in_page(offset);
+ src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >> PAGE_SHIFT))
+ + offset_in_page(offset);
if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
drm_clflush_virt_range(src_ptr, size);
memcpy(dst, src_ptr, size);
- kunmap_atomic(src_map);
+ kunmap_local(src_ptr);
}
static void
--
2.34.1
On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> From: Zhao Liu <[email protected]>
>
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].
>
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption.
You are right about about page faults which are never disabled by
kmap_local_page(). However kmap_atomic might not disable preemption. It
depends on CONFIG_PREEMPT_RT.
Please refer to how kmap_atomic_prot() works (this function is called by
kmap_atomic() when kernels have HIGHMEM enabled).
>
> There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> need to disable pagefaults and preemption for mapping:
>
> 1. The flush operation is safe for CPU hotplug when preemption is not
> disabled.
I'm confused here. Why are you talking about CPU hotplug?
In any case, developers should never rely on implicit calls of
preempt_disable() for the reasons said above. Therefore, flush operations
should be allowed regardless that kmap_atomic() potential side effect.
> In drm/i915/gem/i915_gem_object.c, the function
> i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
If I recall correctly, drm_clflush_virt_range() can always be called with page
faults and preemption enabled. If so, this is enough to say that the
conversion is safe.
Is this code explicitly related to flushing the cache lines before removing /
adding CPUs? If I recall correctly, there are several other reasons behind the
need to issue cache lines flushes. Am I wrong about this?
Can you please say more about what I'm missing here?
> 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 and any issue with cpu's being added or removed
> can be handled safely.
Again your main concern is about CPU hotplug.
Even if I'm missing something, do we really need all these details about the
inner workings of drm_clflush_virt_range()?
I'm not an expert, so may be that I'm wrong about all I wrote above.
Therefore, can you please elaborate a little more for readers with very little
knowledge of these kinds of things (like me and perhaps others)?
> 2. Any context switch caused by preemption or sleep (pagefault may
> cause sleep) doesn't affect the validity of local mapping.
I'd replace "preemption or sleep" with "preemption and page faults" since
yourself then added that page faults lead to tasks being put to sleep.
> Therefore, i915_gem_object_read_from_page_kmap() is a function 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().
>
> And remove the redundant variable that stores the address of the mapped
> page since kunmap_local() can accept any pointer within the page.
>
> [1]: https://lore.kernel.org/all/[email protected]
>
> Suggested-by: Dave Hansen <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Suggested-by: Fabio M. De Francesco <[email protected]>
> Signed-off-by: Zhao Liu <[email protected]>
> ---
> 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.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c index
369006c5317f..a0072abed75e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -413,17 +413,15 @@ void __i915_gem_object_invalidate_frontbuffer(struct
> drm_i915_gem_object *obj, static void
> i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64
offset, void
> *dst, int size) {
> - void *src_map;
> void *src_ptr;
>
> - src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
PAGE_SHIFT));
> -
> - src_ptr = src_map + offset_in_page(offset);
> + src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >>
PAGE_SHIFT))
> + + offset_in_page(offset);
> if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> drm_clflush_virt_range(src_ptr, size);
> memcpy(dst, src_ptr, size);
>
> - kunmap_atomic(src_map);
> + kunmap_local(src_ptr);
> }
>
> static void
The changes look good, but I'd like to better understand the commit message.
Thanks,
Fabio
On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > The use of kmap_atomic() is being deprecated in favor of
> > kmap_local_page()[1].
> >
> > The main difference between atomic and local mappings is that local
> > mappings doesn't disable page faults or preemption.
>
> You are right about about page faults which are never disabled by
> kmap_local_page(). However kmap_atomic might not disable preemption. It
> depends on CONFIG_PREEMPT_RT.
>
> Please refer to how kmap_atomic_prot() works (this function is called by
> kmap_atomic() when kernels have HIGHMEM enabled).
>
> >
> > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > need to disable pagefaults and preemption for mapping:
> >
> > 1. The flush operation is safe for CPU hotplug when preemption is not
> > disabled.
>
> I'm confused here. Why are you talking about CPU hotplug?
I agree with Fabio here. I'm not making the connection between cpu hotplug and
this code path.
Ira
> In any case, developers should never rely on implicit calls of
> preempt_disable() for the reasons said above. Therefore, flush operations
> should be allowed regardless that kmap_atomic() potential side effect.
>
> > In drm/i915/gem/i915_gem_object.c, the function
> > i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
>
> If I recall correctly, drm_clflush_virt_range() can always be called with page
> faults and preemption enabled. If so, this is enough to say that the
> conversion is safe.
>
> Is this code explicitly related to flushing the cache lines before removing /
> adding CPUs? If I recall correctly, there are several other reasons behind the
> need to issue cache lines flushes. Am I wrong about this?
>
> Can you please say more about what I'm missing here?
>
> > 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 and any issue with cpu's being added or removed
> > can be handled safely.
>
> Again your main concern is about CPU hotplug.
>
> Even if I'm missing something, do we really need all these details about the
> inner workings of drm_clflush_virt_range()?
>
> I'm not an expert, so may be that I'm wrong about all I wrote above.
>
> Therefore, can you please elaborate a little more for readers with very little
> knowledge of these kinds of things (like me and perhaps others)?
>
> > 2. Any context switch caused by preemption or sleep (pagefault may
> > cause sleep) doesn't affect the validity of local mapping.
>
> I'd replace "preemption or sleep" with "preemption and page faults" since
> yourself then added that page faults lead to tasks being put to sleep.
>
> > Therefore, i915_gem_object_read_from_page_kmap() is a function 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().
> >
> > And remove the redundant variable that stores the address of the mapped
> > page since kunmap_local() can accept any pointer within the page.
> >
> > [1]: https://lore.kernel.org/all/[email protected]
> >
> > Suggested-by: Dave Hansen <[email protected]>
> > Suggested-by: Ira Weiny <[email protected]>
> > Suggested-by: Fabio M. De Francesco <[email protected]>
> > Signed-off-by: Zhao Liu <[email protected]>
> > ---
> > 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.
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c index
> 369006c5317f..a0072abed75e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -413,17 +413,15 @@ void __i915_gem_object_invalidate_frontbuffer(struct
> > drm_i915_gem_object *obj, static void
> > i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64
> offset, void
> > *dst, int size) {
> > - void *src_map;
> > void *src_ptr;
> >
> > - src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
> PAGE_SHIFT));
> > -
> > - src_ptr = src_map + offset_in_page(offset);
> > + src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >>
> PAGE_SHIFT))
> > + + offset_in_page(offset);
> > if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> > drm_clflush_virt_range(src_ptr, size);
> > memcpy(dst, src_ptr, size);
> >
> > - kunmap_atomic(src_map);
> > + kunmap_local(src_ptr);
> > }
> >
> > static void
>
> The changes look good, but I'd like to better understand the commit message.
>
> Thanks,
>
> Fabio
>
>
On gioved? 3 novembre 2022 17:51:23 CET Ira Weiny wrote:
> On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> > On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > > From: Zhao Liu <[email protected]>
> > >
> > > The use of kmap_atomic() is being deprecated in favor of
> > > kmap_local_page()[1].
> > >
> > > The main difference between atomic and local mappings is that local
> > > mappings doesn't disable page faults or preemption.
> >
> > You are right about about page faults which are never disabled by
> > kmap_local_page(). However kmap_atomic might not disable preemption. It
> > depends on CONFIG_PREEMPT_RT.
> >
> > Please refer to how kmap_atomic_prot() works (this function is called by
> > kmap_atomic() when kernels have HIGHMEM enabled).
> >
> > > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > > need to disable pagefaults and preemption for mapping:
> > >
> > > 1. The flush operation is safe for CPU hotplug when preemption is not
> > > disabled.
> >
> > I'm confused here. Why are you talking about CPU hotplug?
>
> I agree with Fabio here. I'm not making the connection between cpu hotplug
and
> this code path.
>
> Ira
@Zhao,
I'd like to add that I was about to put my reviewed-by tag. The other things I
objected are minor nits. Please just clarify this connection.
Your code is good and deserves to be applied.
Fabio
>
> > In any case, developers should never rely on implicit calls of
> > preempt_disable() for the reasons said above. Therefore, flush operations
> > should be allowed regardless that kmap_atomic() potential side effect.
> >
> > > In drm/i915/gem/i915_gem_object.c, the function
> > > i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
> >
> > If I recall correctly, drm_clflush_virt_range() can always be called with
page
> > faults and preemption enabled. If so, this is enough to say that the
> > conversion is safe.
> >
> > Is this code explicitly related to flushing the cache lines before
removing /
> > adding CPUs? If I recall correctly, there are several other reasons behind
the
> > need to issue cache lines flushes. Am I wrong about this?
> >
> > Can you please say more about what I'm missing here?
> >
> > > 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 and any issue with cpu's being added or removed
> > > can be handled safely.
> >
> > Again your main concern is about CPU hotplug.
> >
> > Even if I'm missing something, do we really need all these details about
the
> > inner workings of drm_clflush_virt_range()?
> >
> > I'm not an expert, so may be that I'm wrong about all I wrote above.
> >
> > Therefore, can you please elaborate a little more for readers with very
little
> > knowledge of these kinds of things (like me and perhaps others)?
> >
> > > 2. Any context switch caused by preemption or sleep (pagefault may
> > > cause sleep) doesn't affect the validity of local mapping.
> >
> > I'd replace "preemption or sleep" with "preemption and page faults" since
> > yourself then added that page faults lead to tasks being put to sleep.
> >
> > > Therefore, i915_gem_object_read_from_page_kmap() is a function 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().
> > >
> > > And remove the redundant variable that stores the address of the mapped
> > > page since kunmap_local() can accept any pointer within the page.
> > >
> > > [1]: https://lore.kernel.org/all/[email protected]
> > >
> > > Suggested-by: Dave Hansen <[email protected]>
> > > Suggested-by: Ira Weiny <[email protected]>
> > > Suggested-by: Fabio M. De Francesco <[email protected]>
> > > Signed-off-by: Zhao Liu <[email protected]>
> > > ---
> > >
> > > 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.
> > >
> > > ---
> > >
> > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c index
> >
> > 369006c5317f..a0072abed75e 100644
> >
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > @@ -413,17 +413,15 @@ void
__i915_gem_object_invalidate_frontbuffer(struct
> > > drm_i915_gem_object *obj, static void
> > >
> > > i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj,
u64
> >
> > offset, void
> >
> > > *dst, int size) {
> > > - void *src_map;
> > >
> > > void *src_ptr;
> > >
> > > - src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
> >
> > PAGE_SHIFT));
> >
> > > -
> > > - src_ptr = src_map + offset_in_page(offset);
> > > + src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >>
> >
> > PAGE_SHIFT))
> >
> > > + + offset_in_page(offset);
> > >
> > > if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> > >
> > > drm_clflush_virt_range(src_ptr, size);
> > >
> > > memcpy(dst, src_ptr, size);
> > >
> > > - kunmap_atomic(src_map);
> > > + kunmap_local(src_ptr);
> > >
> > > }
> > >
> > > static void
> >
> > The changes look good, but I'd like to better understand the commit
message.
> >
> > Thanks,
> >
> > Fabio
On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> Date: Sat, 29 Oct 2022 13:17:03 +0200
> From: "Fabio M. De Francesco" <[email protected]>
> Subject: Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in
> gem/i915_gem_object.c
>
> On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > The use of kmap_atomic() is being deprecated in favor of
> > kmap_local_page()[1].
> >
> > The main difference between atomic and local mappings is that local
> > mappings doesn't disable page faults or preemption.
>
> You are right about about page faults which are never disabled by
> kmap_local_page(). However kmap_atomic might not disable preemption. It
> depends on CONFIG_PREEMPT_RT.
>
> Please refer to how kmap_atomic_prot() works (this function is called by
> kmap_atomic() when kernels have HIGHMEM enabled).
Yes, there is some ambiguity here. What about "The main difference between
atomic and local mappings is that local mappings never disable page faults
or preemption"?
>
> >
> > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > need to disable pagefaults and preemption for mapping:
> >
> > 1. The flush operation is safe for CPU hotplug when preemption is not
> > disabled.
>
> I'm confused here. Why are you talking about CPU hotplug?
> In any case, developers should never rely on implicit calls of
> preempt_disable() for the reasons said above. Therefore, flush operations
> should be allowed regardless that kmap_atomic() potential side effect.
Sorry, it's my fault, my misunderstanding about the connection between hotplug
and flush here. When mapping exists, the cpu cannot be unplugged via CPU-hotplug.
But whether plug or unplug, it has nothing to do with flush. I will delete this
wrong description.
My initial consideration is that this interface of flush may require an atomic
context, so I want to explain more from the details of its implementation
that cache consistency can be guaranteed without atomic context. Is this
consideration redundant?
Also, do I need to state that migration is still ok for this flush interface
here (since __kmap_local_page_prot() doesn't always disable migration)?
> > In drm/i915/gem/i915_gem_object.c, the function
> > i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
>
> If I recall correctly, drm_clflush_virt_range() can always be called with page
> faults and preemption enabled. If so, this is enough to say that the
> conversion is safe.
>
> Is this code explicitly related to flushing the cache lines before removing /
> adding CPUs? If I recall correctly, there are several other reasons behind the
> need to issue cache lines flushes. Am I wrong about this?
>
> Can you please say more about what I'm missing here?
>
> > 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 and any issue with cpu's being added or removed
> > can be handled safely.
>
> Again your main concern is about CPU hotplug.
>
> Even if I'm missing something, do we really need all these details about the
> inner workings of drm_clflush_virt_range()?
>
> I'm not an expert, so may be that I'm wrong about all I wrote above.
>
> Therefore, can you please elaborate a little more for readers with very little
> knowledge of these kinds of things (like me and perhaps others)?
>
> > 2. Any context switch caused by preemption or sleep (pagefault may
> > cause sleep) doesn't affect the validity of local mapping.
>
> I'd replace "preemption or sleep" with "preemption and page faults" since
> yourself then added that page faults lead to tasks being put to sleep.
Thanks, good advice.
Zhao
On Thu, Nov 03, 2022 at 08:22:04PM +0100, Fabio M. De Francesco wrote:
> Date: Thu, 03 Nov 2022 20:22:04 +0100
> From: "Fabio M. De Francesco" <[email protected]>
> Subject: Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in
> gem/i915_gem_object.c
>
> On gioved? 3 novembre 2022 17:51:23 CET Ira Weiny wrote:
> > On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> > > On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > > > From: Zhao Liu <[email protected]>
> > > >
> > > > The use of kmap_atomic() is being deprecated in favor of
> > > > kmap_local_page()[1].
> > > >
> > > > The main difference between atomic and local mappings is that local
> > > > mappings doesn't disable page faults or preemption.
> > >
> > > You are right about about page faults which are never disabled by
> > > kmap_local_page(). However kmap_atomic might not disable preemption. It
> > > depends on CONFIG_PREEMPT_RT.
> > >
> > > Please refer to how kmap_atomic_prot() works (this function is called by
> > > kmap_atomic() when kernels have HIGHMEM enabled).
> > >
> > > > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > > > need to disable pagefaults and preemption for mapping:
> > > >
> > > > 1. The flush operation is safe for CPU hotplug when preemption is not
> > > > disabled.
> > >
> > > I'm confused here. Why are you talking about CPU hotplug?
> >
> > I agree with Fabio here. I'm not making the connection between cpu hotplug
> and
> > this code path.
> >
> > Ira
>
> @Zhao,
>
> I'd like to add that I was about to put my reviewed-by tag. The other things I
> objected are minor nits. Please just clarify this connection.
Thanks Fabio for your comments! Sorry I missed the mails that day. This connection
is my misunderstanding. Other thoughts please refer to my reply to your first email
in this thread.
Thanks,
Zhao
On Thu, Nov 03, 2022 at 09:51:23AM -0700, Ira Weiny wrote:
> Date: Thu, 3 Nov 2022 09:51:23 -0700
> From: Ira Weiny <[email protected]>
> Subject: Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in
> gem/i915_gem_object.c
>
> On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> > On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > > From: Zhao Liu <[email protected]>
> > >
> > > The use of kmap_atomic() is being deprecated in favor of
> > > kmap_local_page()[1].
> > >
> > > The main difference between atomic and local mappings is that local
> > > mappings doesn't disable page faults or preemption.
> >
> > You are right about about page faults which are never disabled by
> > kmap_local_page(). However kmap_atomic might not disable preemption. It
> > depends on CONFIG_PREEMPT_RT.
> >
> > Please refer to how kmap_atomic_prot() works (this function is called by
> > kmap_atomic() when kernels have HIGHMEM enabled).
> >
> > >
> > > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > > need to disable pagefaults and preemption for mapping:
> > >
> > > 1. The flush operation is safe for CPU hotplug when preemption is not
> > > disabled.
> >
> > I'm confused here. Why are you talking about CPU hotplug?
>
> I agree with Fabio here. I'm not making the connection between cpu hotplug and
> this code path.
Sorry, my misunderstanding. Will delete this wrong explanation.
Thanks,
Zhao