Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On sabato 17 giugno 2023 20:04:20 CEST 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 NIT: _can_ 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 > > Signed-off-by: Sumitra Sharma > --- > > Changes in v2: > - Replace kmap() with kmap_local_page(). > - Change commit subject and message. With the changes that Ira suggested and the minor fix I'm proposing to the commit message, it looks good to me too, so this patch is... Reviewed-by: Fabio M. De Francesco However, as far as I'm concerned, our nits don't necessarily require any newer version, especially because Tvrtko has already sent this patch for their CI. Thanks, Fabio P.S.: As Sumitra says both kmap() and kmap_local_page() allows preemption in non atomic context. Furthermore, Tvrtko confirmed that the pages can come from HIGHMEM, therefore kmap_local_page for local temporary mapping is unavoidable. Last thing... Thomas thinks he wants to make it run atomically (if I understood one of his messages correctly). As I already responded, nothing prevents someone does another patch just to disable preemption (or to enter atomic context by other means) around the code marked by kmap_local_page() / kunmap_local() because these functions work perfectly _also_ in atomic context (including interrupts). But this is not something that Sumitra should be worried about. > > 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(, 1); > > - s = kmap(page); > + s = kmap_local_page(page); > ret = compress_page(compress, s, dst, false); > - kunmap(page); > + kunmap_local(s); > > drm_clflush_pages(, 1); > > -- > 2.25.1
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On mercoledì 21 giugno 2023 11:06:51 CEST Thomas Hellström (Intel) wrote: > > I think one thing worth mentioning in the context of this patch is that > IIRC kmap_local_page() will block offlining of the mapping CPU until > kunmap_local(), Migration is disabled. > so while I haven't seen any guidelines around the usage > of this api for long-held mappings, It would be advisable to not use it for long term mappings, if possible. These "local" mappings should better be help for not too long duration. > I figure it's wise to keep the > mapping duration short, or at least avoid sleeping with a > kmap_local_page() map active. Nothing prevents a call of preempt_disable() around the section of code between kmap_local_page() / kunmap_local(). If it is needed, local mappings could also be acquired under spinlocks and/or with disabled interrupts. I don't know the code, however, everything cited above could be the subject of a subsequent patch. Regards, Fabio > I figured, while page compression is probably to be considered "slow" > it's probably not slow enough to motivate kmap() instead of > kmap_local_page(), but if anyone feels differently, perhaps it should be > considered. > > With that said, my Reviewed-by: still stands. > > /Thomas >
Re: [PATCH] drm/gma500: Replace kmap{,_atomic}() with page_address()
On martedì 20 giugno 2023 20:01:48 CEST Sumitra Sharma wrote:
> Remove unnecessary calls to kmap{,_atomic}() when acquiring
> pages using GFP_DMA32.
>
> The GFP_DMA32 uses the DMA32 zone to satisfy the allocation
> requests. Therefore, pages allocated with GFP_DMA32 cannot
> come from Highmem.
>
> Avoid using calls to kmap_local_page() / kunmap_local() and
> kmap() / kunmap() in the psb_mmu_alloc_pd function. Instead,
> utilize page_address().
>
> Remove the usage of kmap_atomic() / kunmap_atomic() in the
> psb_mmu_alloc_pt function. Use page_address() instead.
>
> Substitute kmap_atomic(pt->p) / kunmap_atomic(pt->v) calls
> in the psb_mmu_pt_alloc_map_lock() and psb_mmu_pt_unmap_unlock()
> functions with page_address(pt->p). This is possible as
> pt = psb_mmu_alloc_pt(pd) allocates a page using
> pt->p = alloc_page(GFP_DMA32).
Sumitra,
I'm sorry because this patch cannot acked with this commit message.
This commit message is missing two _really_ important information. Therefore,
it is not acked. Please check again what I write below and either add the
missing information or change the code accordingly...
You should assure everybody that the code between the old kmap_atomic() /
kunmap_atomic() doesn't depend either on implicit pagefault_disable() or
preempt_disable() calls or both.
Please read again the section of the Highmem's documentation regarding
kmap_atomic() at https://docs.kernel.org/mm/highmem.html
In particular take care to read and understand what "[] the code between calls
to kmap_atomic() and kunmap_atomic() may implicitly depend on the side effects
of atomic mappings, i.e. disabling page faults or preemption, or both. In that
case, explicit calls to pagefault_disable() or preempt_disable() or both must
be made in conjunction with the use of kmap_local_page().".
Please study carefully also the following patch from Zhao, suggested by Ira
and reviewed by Ira and I: "[PATCH v2 3/9] drm/i915: Use kmap_local_page() in
gem/i915_gem_shmem.c". It's not yet reached upstream so you need to find it in
lore.kernel.org at
https://lore.kernel.org/lkml/20230329073220.3982460-4-zhao1@linux.intel.com/
Please note that, in turn, that patch also contains a link to a patch from Ira
who too had to disable faults (https://lore.kernel.org/all/
20220813220034.806698-1-ira.we...@intel.com)
I haven't yet looked at your code. If any sections do depend on those implicit
disables, you should act accordingly and add one or both of the above-
mentioned calls, even in cases where you get rid of local mappings.
Instead if the sections don't depend on the mentioned side effects, you should
write something like what I wrote in "[PATCH] NFS: Convert kmap_atomic() to
kmap_local_folio()" that you can find at https://lore.kernel.org/lkml/
20230503172411.3356-1-fmdefrance...@gmail.com/ or, by by using "git show
4b71e2416ec4".
Thanks for working on this,
Fabio
>
> Suggested-by: Ira Weiny
> Signed-off-by: Sumitra Sharma
> ---
> drivers/gpu/drm/gma500/mmu.c | 17 +
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
> index a70b01ccdf70..59aa5661e56a 100644
> --- a/drivers/gpu/drm/gma500/mmu.c
> +++ b/drivers/gpu/drm/gma500/mmu.c
> @@ -184,20 +184,15 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct
> psb_mmu_driver *driver, pd->invalid_pte = 0;
> }
>
> - v = kmap_local_page(pd->dummy_pt);
> + v = page_address(pd->dummy_pt);
> for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
> v[i] = pd->invalid_pte;
>
> - kunmap_local(v);
> -
> - v = kmap_local_page(pd->p);
> + v = page_address(pd->p);
> for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
> v[i] = pd->invalid_pde;
>
> - kunmap_local(v);
> -
> - clear_page(kmap(pd->dummy_page));
> - kunmap(pd->dummy_page);
> + clear_page(page_address(pd->dummy_page));
>
> pd->tables = vmalloc_user(sizeof(struct psb_mmu_pt *) * 1024);
> if (!pd->tables)
> @@ -279,7 +274,7 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct
> psb_mmu_pd *pd)
>
> spin_lock(lock);
>
> - v = kmap_atomic(pt->p);
> + v = page_address(pt->p);
> clf = (uint8_t *) v;
> ptes = (uint32_t *) v;
> for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
> @@ -293,7 +288,6 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct
> psb_mmu_pd *pd) }
> mb();
> }
> - kunmap_atomic(v);
> spin_unlock(lock);
>
> pt->count = 0;
> @@ -339,7 +333,7 @@ static struct psb_mmu_pt
*psb_mmu_pt_alloc_map_lock(struct
> psb_mmu_pd *pd, atomic_set(>driver->needs_tlbflush, 1);
> }
> }
> - pt->v = kmap_atomic(pt->p);
> + pt->v = page_address(pt->p);
> return pt;
> }
>
> @@ -365,7 +359,6 @@ static void psb_mmu_pt_unmap_unlock(struct psb_mmu_pt
*pt)
> struct psb_mmu_pd *pd = pt->pd;
> uint32_t *v;
>
> -
[PATCH v2 2/3] drm/i915/gt: Replace kmap() with kmap_local_page()
kmap() s been deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Obviously, thread locality implies that the kernel virtual addresses are
only valid in the context of the callers. The use of kmap_local_page() in
i915/gt doesn't break the above-mentioned constraint, so it should be
preferred to kmap().
Therefore, replace kmap() with kmap_local_page() in i915/gt.
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
drivers/gpu/drm/i915/gt/shmem_utils.c| 11 ---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
index 37d0b0fe791d..89295c6921d6 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
@@ -749,7 +749,7 @@ static void swizzle_page(struct page *page)
char *vaddr;
int i;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
for (i = 0; i < PAGE_SIZE; i += 128) {
memcpy(temp, [i], 64);
@@ -757,7 +757,7 @@ static void swizzle_page(struct page *page)
memcpy([i + 64], temp, 64);
}
- kunmap(page);
+ kunmap_local(vaddr);
}
/**
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c
b/drivers/gpu/drm/i915/gt/shmem_utils.c
index 449c9ed44382..be809839a241 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c
@@ -101,22 +101,19 @@ static int __shmem_rw(struct file *file, loff_t off,
unsigned int this =
min_t(size_t, PAGE_SIZE - offset_in_page(off), len);
struct page *page;
- void *vaddr;
page = shmem_read_mapping_page_gfp(file->f_mapping, pfn,
GFP_KERNEL);
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
if (write) {
- memcpy(vaddr + offset_in_page(off), ptr, this);
+ memcpy_to_page(page, offset_in_page(off), ptr, this);
set_page_dirty(page);
} else {
- memcpy(ptr, vaddr + offset_in_page(off), this);
+ memcpy_from_page(ptr, page, offset_in_page(off), this);
}
mark_page_accessed(page);
- kunmap(page);
put_page(page);
len -= this;
@@ -143,11 +140,11 @@ int shmem_read_to_iosys_map(struct file *file, loff_t off,
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
iosys_map_memcpy_to(map, map_off, vaddr + offset_in_page(off),
this);
mark_page_accessed(page);
- kunmap(page);
+ kunmap_local(vaddr);
put_page(page);
len -= this;
--
2.40.0
[PATCH v2 3/3] drm/i915/gem: Replace kmap() with kmap_local_page()
kmap() s been deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Obviously, thread locality implies that the kernel virtual addresses are
only valid in the context of the callers. The kmap_local_page() use in
i915/gem doesn't break the above-mentioned constraint, so it should be
preferred to kmap().
Therefore, replace kmap() with kmap_local_page() in i915/gem and use
memcpy_to_page() where it is possible to avoid the open coding of
mapping + memcpy() + un-mapping.
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 37d1efcd3ca6..8856a6409e83 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -657,16 +657,14 @@ i915_gem_object_create_shmem_from_data(struct
drm_i915_private *dev_priv,
do {
unsigned int len = min_t(typeof(size), size, PAGE_SIZE);
struct page *page;
- void *pgdata, *vaddr;
+ void *pgdata;
err = aops->write_begin(file, file->f_mapping, offset, len,
, );
if (err < 0)
goto fail;
- vaddr = kmap(page);
- memcpy(vaddr, data, len);
- kunmap(page);
+ memcpy_to_page(page, 0, data, len);
err = aops->write_end(file, file->f_mapping, offset, len, len,
page, pgdata);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 56279908ed30..5fd9e1ee2340 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -155,7 +155,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu,
row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to
page (%lu + %u [0x%lx]) of 0x%x, found 0x%x\n",
@@ -173,7 +173,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
out:
i915_gem_object_lock(obj, NULL);
@@ -251,7 +251,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u
[%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected
write to page (%lu + %u [0x%lx]) of 0x%x, found 0x%x\n",
@@ -269,7 +269,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
if (err)
return err;
--
2.40.0
[PATCH v2 1/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() has been deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. Obviously, thread locality implies that the kernel voirtual addresses are valid only in the context of the callers. kmap_local_page() use in i915_gem.c doesn't break the above-mentioned constraint, so it should be preferred to kmap(). Therefore, replace kmap() with kmap_local_page() in i915_gem.c Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/i915/i915_gem.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 35950fa91406..c35248555e42 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -212,14 +212,14 @@ shmem_pread(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush) drm_clflush_virt_range(vaddr + offset, len); ret = __copy_to_user(user_data, vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } @@ -643,7 +643,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush_before) drm_clflush_virt_range(vaddr + offset, len); @@ -652,7 +652,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, if (!ret && needs_clflush_after) drm_clflush_virt_range(vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } -- 2.40.0
[PATCH v2 0/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() has been deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). The tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and so they are still valid. Furthermore, kmap_local_page() is faster than kmap() in kernels with HIGHMEM enabled. Thread locality implies that the kernel virtual addresses returned by kmap_local_page() are only valid in the context of the callers. This constraint is never violated with the conversions in this series, because the pointers are never handed to other threads, so the local mappings are allowed and preferred. Therefore, replace kmap() with kmap_local_page() in drm/i915/, drm/i915/gem/, drm/i915/gt/. Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco v1->v2: Do some changes in the text of the cover letter and in the commit messages. There are no changes in the code of any of the three patches. Fabio M. De Francesco (3): drm/i915: Replace kmap() with kmap_local_page() drm/i915/gt: Replace kmap() with kmap_local_page() drm/i915/gem: Replace kmap() with kmap_local_page() drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++ drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- drivers/gpu/drm/i915/gt/shmem_utils.c | 11 --- drivers/gpu/drm/i915/i915_gem.c| 8 5 files changed, 16 insertions(+), 21 deletions(-) -- 2.40.0
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: > > Zhao Liu wrote: > >> From: Zhao Liu > >> > >> 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.we...@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 > >> Suggested-by: Fabio M. De Francesco > >> Signed-off-by: Zhao Liu > >> --- > >> > >> 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
Re: [PATCH v2 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
On mercoledì 29 marzo 2023 09:32:11 CEST Zhao Liu wrote:
> From: Zhao Liu
>
> Hi list,
>
> Sorry for a long delay since v1 [1]. This patchset is based on 197b6b6
> (Linux 6.3-rc4).
>
> Welcome and thanks for your review and comments!
>
>
> # Purpose of this patchset
>
> The purpose of this pacthset is to replace all uses of kmap_atomic() in
> i915 with kmap_local_page() because the use of kmap_atomic() is being
> deprecated in favor of kmap_local_page()[1]. And 92b64bd (mm/highmem:
> add notes about conversions from kmap{,_atomic}()) has declared the
> deprecation of kmap_atomic().
>
>
> # Motivation for deprecating kmap_atomic() and using 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.
>
>
> # Patch summary
>
> Patch 1, 4-6 and 8-9 replace kamp_atomic()/kunmap_atomic() with
> kmap_local_page()/kunmap_local() directly. With thses local
> mappings, the page faults and preemption are allowed.
>
> Patch 2 and 7 use memcpy_from_page() and memcpy_to_page() to replace
> kamp_atomic()/kunmap_atomic(). These two variants of memcpy()
> are based on the local mapping, so page faults and preemption
> are also allowed in these two interfaces.
>
> Patch 3 replaces kamp_atomic()/kunmap_atomic() with kmap_local_page()/
> kunmap_local() and also diable page fault since the for special
> handling (pls see the commit message).
>
>
> # Changes since v1
>
> * Dropped hot plug related description in commit message since it has
> nothing to do with kmap_local_page().
> * Emphasized the motivation for using kmap_local_page() in commit
> message.
> * Rebased patch 1 on f47e630 (drm/i915/gem: Typecheck page lookups) to
> keep the "idx" variable of type pgoff_t here.
> * Used memcpy_from_page() and memcpy_to_page() to replace
> kmap_local_page() + memcpy() in patch 2.
>
>
> # Reference
>
> [1]:
> https://lore.kernel.org/lkml/20221017093726.2070674-1-zhao1.liu@linux.intel.c
> om/ [1]:
> https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com ---
> Zhao Liu (9):
> drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
> drm/i915: Use memcpy_[from/to]_page() in gem/i915_gem_pyhs.c
> drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
> drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
> drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
> drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
> drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
> drm/i915: Use kmap_local_page() in i915_cmd_parser.c
> drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
>
I _think_ that the "long delay" you mentioned in the first sentence has paid
off in full.
I don't see things to improve (except all those "kamp_atomic()" typo in the
patches summary; however, typos are only in the cover so I'm sure they won't
hurt anybody).
Each of the nine patches listed above looks good to me, so they are all…
Reviewed-by: Fabio M. De Francesco
Thanks!
Fabio
PS: Obviously there was no need to reconfirm my tag for patch 3/9. A single
tag that catches all patches is easier for a lazy person like me :-)
>
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-
> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 10 ++
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 6 --
> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 6 +++---
> .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c | 12
> .../gpu/drm/i915/gem/selftests/i915_gem_context.c| 8
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 +
> drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++--
> 9 files changed, 28 insertions(+), 41 deletions(-)
>
> --
> 2.34.1
Re: [PATCH 3/9] drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
On lunedì 17 ottobre 2022 11:37:19 CET Zhao Liu wrote: > From: Zhao Liu > > 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. > > In drm/i915/gem/i915_gem_shmem.c, the function shmem_pwrite() need to > disable pagefault to eliminate the potential recursion fault[2]. But > here __copy_from_user_inatomic() doesn't need to disable preemption and > local mapping is valid for sched in/out. > So it can use kmap_local_page() / kunmap_local() with > pagefault_disable() / pagefault_enable() to replace atomic mapping. > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com > [2]: https://patchwork.freedesktop.org/patch/295840/ > > Suggested-by: Ira Weiny > Signed-off-by: Zhao Liu > --- > Suggested by credits: > Ira: Referred to his suggestions about keeping pagefault_disable(). > --- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) The commit message explains clearly and the changes to the code look good. The necessity to reuse pagefault_disable() / pagefault_enable() from the main kmap_atomic() side effect is a nice catch. Reviewed-by: Fabio M. De Francesco Thanks! > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index f42ca1179f37..e279a3e30c02 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -472,11 +472,13 @@ shmem_pwrite(struct drm_i915_gem_object *obj, > if (err < 0) > return err; > > - vaddr = kmap_atomic(page); > + vaddr = kmap_local_page(page); > + pagefault_disable(); > unwritten = __copy_from_user_inatomic(vaddr + pg, > user_data, > len); > - kunmap_atomic(vaddr); > + pagefault_enable(); > + kunmap_local(vaddr); > > err = aops->write_end(obj->base.filp, mapping, offset, len, > len - unwritten, page, data);
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 > > > > > > 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/20220813220034.806698-1-ira.we...@intel.com > > > > > > Suggested-by: Dave Hansen > > > Suggested-by: Ira Weiny > > > Suggested-by: Fabio M. De Francesco > > > Signed-off-by: Zhao Liu > > > --- > > > > > > 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. > > > > > > --- > > > > > > dri
Re: [RESEND PATCH] drm/amd/amdgpu: Replace kmap() with kmap_local_page()
On lunedì 17 ottobre 2022 18:53:24 CET Alex Deucher wrote: > Applied. Thanks! > The same report about which I just wrote in my previous email to you is also referring to this patch which later changed status to "Not Applicable". It points to https://patchwork.linuxtv.org/project/linux-media/patch/ 20220812175753.22926-1-fmdefrance...@gmail.com/ Can you please let me understand why? Thanks, Fabio
Re: [PATCH] drm/radeon: Replace kmap() with kmap_local_page()
On lunedì 17 ottobre 2022 18:52:10 CET Alex Deucher wrote: > Applied. Thanks! Many thanks to you! However, about a week ago, I received a report saying that this patch is "Not Applicable". That email was also referring to another patch, for which I'll reply in its own thread. That report has a link to https://patchwork.linuxtv.org/project/linux-media/ patch/20221013210714.16320-1-fmdefrance...@gmail.com/ Can you please let me understand why, despite it was applied, this patch later shifted "State" to "Not Applicable"? Thanks, Fabio
Re: [PATCH 2/9] drm/i915: Use kmap_local_page() in gem/i915_gem_pyhs.c
On lunedì 17 ottobre 2022 11:37:18 CEST Zhao Liu wrote: > From: Zhao Liu > > 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. > > In drm/i915/gem/i915_gem_phys.c, the functions > i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys() > don't need to disable pagefaults and preemption for mapping because of > these 2 reasons: > > 1. The flush operation is safe for CPU hotplug when preemption is not > disabled. In drm/i915/gem/i915_gem_object.c, the functions > i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys() > 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_get_pages_phys() and > i915_gem_object_put_pages_phys() are two 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(). > I have here the same questions as in 1/9. > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com > > Suggested-by: Dave Hansen > Suggested-by: Ira Weiny > Suggested-by: Fabio M. De Francesco > Signed-off-by: Zhao Liu > --- > 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_phys.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c index 0d0e46dae559..d602ba19ecb2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > @@ -66,10 +66,10 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object > *obj) if (IS_ERR(page)) > goto err_st; > > - src = kmap_atomic(page); > + src = kmap_local_page(page); > memcpy(dst, src, PAGE_SIZE); > drm_clflush_virt_range(dst, PAGE_SIZE); > - kunmap_atomic(src); > + kunmap_local(src); Please use memcpy_from_page() instead of open coding mapping + memcpy() + unmapping. > > put_page(page); > dst += PAGE_SIZE; > @@ -114,10 +114,10 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj, > if (IS_ERR(page)) > continue; > > - dst = kmap_atomic(page); > + dst = kmap_local_page(page); > drm_clflush_virt_range(src, PAGE_SIZE); > memcpy(dst, src, PAGE_SIZE); > - kunmap_atomic(dst); > + kunmap_local(dst); For the same reasons said above, memcpy_to_page() should be used here and avoid open coding of three functions. Using those helpers forces you to move drm_clflush_virt_range() out of the mapping / un-mapping region. I may be wrong, however I'm pretty sure that the relative positions of each of those call sites is something that cannot be randomly chosen. Thanks, Fabio > > set_page_dirty(page); > if (obj->mm.madv == I915_MADV_WILLNEED)
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
>
> 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/20220813220034.806698-1-ira.we...@intel.com
>
> Suggested-by: Dave Hansen
> Suggested-by: Ira Weiny
> Suggested-by: Fabio M. De Francesco
> Signed-off-by: Zhao Liu
> ---
> 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
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
On lunedì 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote: > From: Zhao Liu > > The use of kmap_atomic() is being deprecated in favor of > kmap_local_page()[1]. Some words to explain why kmap_atomic was deprecated won't hurt. Many maintainers and reviewers, and also casual readers might not yet be aware of the reasons behind that deprecation. > In the following patches, we can convert the calls of kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local(), which can > instead do the mapping / unmapping regardless of the context. Readers are probably much more interested in what you did in the following patches and why you did it, instead of being informed about what "we can" do. I would suggest something like "The following patches convert the calls to kmap_atomic() to kmap_local_page() [the rest looks OK]". This could also be the place to say something about why we prefer kmap_local_page() to kmap_atomic(). Are you sure that the reasons that motivates your conversions are merely summarized to kmap_local_page() being able to do mappings regardless of context? I think you are missing the real reasons why. What about avoiding the often unwanted side effect of unnecessary page faults disables? > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. No news here. kmap_atomic() is "per thread, CPU local and not glocally visible". I cannot see any difference here between kmap_atomic() and kmap_local_page(). > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com > --- > Zhao Liu (9): > drm/i915: Use kmap_local_page() in gem/i915_gem_object.c > drm/i915: Use kmap_local_page() in gem/i915_gem_pyhs.c > drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c > drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c > drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c > drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c > drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c > drm/i915: Use kmap_local_page() in i915_cmd_parser.c > drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++- > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 8 > drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 6 -- > drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 6 +++--- > .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c | 12 > .../gpu/drm/i915/gem/selftests/i915_gem_context.c| 8 > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 + > drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++-- > 9 files changed, 30 insertions(+), 37 deletions(-) Thanks for helping with kmap_atomic() conversions to kmap_local_page(). Fabio
[RESEND PATCH 3/3] drm/i915/gem: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Since its use in i915/gem is safe everywhere, it should be preferred.
Therefore, replace kmap() with kmap_local_page() in i915/gem. Instead of
open-coding local map + memcpy + local unmap, use memcpy_to_page() in a
suited call site.
Cc: "Venkataramanan, Anirudh"
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4eed3dd90ba8..2bc6ab9964ff 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -640,16 +640,14 @@ i915_gem_object_create_shmem_from_data(struct
drm_i915_private *dev_priv,
do {
unsigned int len = min_t(typeof(size), size, PAGE_SIZE);
struct page *page;
- void *pgdata, *vaddr;
+ void *pgdata;
err = aops->write_begin(file, file->f_mapping, offset, len,
, );
if (err < 0)
goto fail;
- vaddr = kmap(page);
- memcpy(vaddr, data, len);
- kunmap(page);
+ memcpy_to_page(page, 0, data, len);
err = aops->write_end(file, file->f_mapping, offset, len, len,
page, pgdata);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 3ced9948a331..bb25b50b5688 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -153,7 +153,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu,
row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to
page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -171,7 +171,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
out:
i915_gem_object_lock(obj, NULL);
@@ -249,7 +249,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u
[%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected
write to page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -267,7 +267,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
if (err)
return err;
--
2.37.1
[RESEND PATCH 2/3] drm/i915/gt: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Since its use in i915/gt is safe everywhere, it should be preferred.
Therefore, replace kmap() with kmap_local_page() in i915/gt. Instead of
open-coding local mappings + memcpy() + local unmappings, use
the memcpy_{from,to}_page() helpers where these are better suited.
Cc: "Venkataramanan, Anirudh"
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
drivers/gpu/drm/i915/gt/shmem_utils.c| 11 ---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
index 6ebda3d65086..21d8ce40b897 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
@@ -747,7 +747,7 @@ static void swizzle_page(struct page *page)
char *vaddr;
int i;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
for (i = 0; i < PAGE_SIZE; i += 128) {
memcpy(temp, [i], 64);
@@ -755,7 +755,7 @@ static void swizzle_page(struct page *page)
memcpy([i + 64], temp, 64);
}
- kunmap(page);
+ kunmap_local(vaddr);
}
/**
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c
b/drivers/gpu/drm/i915/gt/shmem_utils.c
index 402f085f3a02..48edbb8a33e5 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c
@@ -98,22 +98,19 @@ static int __shmem_rw(struct file *file, loff_t off,
unsigned int this =
min_t(size_t, PAGE_SIZE - offset_in_page(off), len);
struct page *page;
- void *vaddr;
page = shmem_read_mapping_page_gfp(file->f_mapping, pfn,
GFP_KERNEL);
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
if (write) {
- memcpy(vaddr + offset_in_page(off), ptr, this);
+ memcpy_to_page(page, offset_in_page(off), ptr, this);
set_page_dirty(page);
} else {
- memcpy(ptr, vaddr + offset_in_page(off), this);
+ memcpy_from_page(ptr, page, offset_in_page(off), this);
}
mark_page_accessed(page);
- kunmap(page);
put_page(page);
len -= this;
@@ -140,11 +137,11 @@ int shmem_read_to_iosys_map(struct file *file, loff_t off,
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
iosys_map_memcpy_to(map, map_off, vaddr + offset_in_page(off),
this);
mark_page_accessed(page);
- kunmap(page);
+ kunmap_local(vaddr);
put_page(page);
len -= this;
--
2.37.1
[RESEND PATCH 1/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. Since its use in i915_gem.c is safe everywhere, it should be preferred. Therefore, replace kmap() with kmap_local_page() in i915_gem.c Cc: "Venkataramanan, Anirudh" Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/i915/i915_gem.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 702e5b89be22..43effce60e1b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -212,14 +212,14 @@ shmem_pread(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush) drm_clflush_virt_range(vaddr + offset, len); ret = __copy_to_user(user_data, vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } @@ -634,7 +634,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush_before) drm_clflush_virt_range(vaddr + offset, len); @@ -643,7 +643,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, if (!ret && needs_clflush_after) drm_clflush_virt_range(vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } -- 2.37.1
[RESEND PATCH 0/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and still valid. Since its use in drm/i915 is safe everywhere, it should be preferred. Therefore, replace kmap() with kmap_local_page() in drm/i915. These changes should be tested in an 32 bits system, booting a kernel with HIGHMEM enabled. Unfortunately I have no i915 based hardware, therefore any help with testing would be greatly appreciated. I'm resending this little series because I suspect that it has been lost, since it was submitted on Aug 11, 2022. In the meantime I'm adding one more recipient (Anirudh) who is helping, along with others, Ira and me with these conversions / removals of kmap() and kmap_atomic() Cc: "Venkataramanan, Anirudh" Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco Fabio M. De Francesco (3): drm/i915: Replace kmap() with kmap_local_page() drm/i915/gt: Replace kmap() with kmap_local_page() drm/i915/gem: Replace kmap() with kmap_local_page() drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++ drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- drivers/gpu/drm/i915/gt/shmem_utils.c | 11 --- drivers/gpu/drm/i915/i915_gem.c| 8 5 files changed, 16 insertions(+), 21 deletions(-) -- 2.37.1
[RESEND PATCH] drm/amd/amdgpu: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. Since its use in amdgpu/amdgpu_ttm.c is safe, it should be preferred. Therefore, replace kmap() with kmap_local_page() in amdgpu/amdgpu_ttm.c. Suggested-by: Ira Weiny Acked-by: Christian König Signed-off-by: Fabio M. De Francesco --- I'm resending because I suspect that this patch might have been lost. In the meantime I added an "Acked-by" tag from Christian K.. Obviviously, there are no further changes in the code. drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 3b4c19412625..c11657b5915f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -2301,9 +2301,9 @@ static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf, if (p->mapping != adev->mman.bdev.dev_mapping) return -EPERM; - ptr = kmap(p); + ptr = kmap_local_page(p); r = copy_to_user(buf, ptr + off, bytes); - kunmap(p); + kunmap_local(ptr); if (r) return -EFAULT; @@ -2352,9 +2352,9 @@ static ssize_t amdgpu_iomem_write(struct file *f, const char __user *buf, if (p->mapping != adev->mman.bdev.dev_mapping) return -EPERM; - ptr = kmap(p); + ptr = kmap_local_page(p); r = copy_from_user(ptr + off, buf, bytes); - kunmap(p); + kunmap_local(ptr); if (r) return -EFAULT; -- 2.37.1
[PATCH] drm/radeon: Replace kmap() with kmap_local_page()
The use of kmap() is being deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
the mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and still valid.
Therefore, replace kmap() with kmap_local_page() in radeon_ttm_gtt_read().
Cc: "Venkataramanan, Anirudh"
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index d33fec488713..bdb4c0e0736b 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -869,11 +869,11 @@ static ssize_t radeon_ttm_gtt_read(struct file *f, char
__user *buf,
page = rdev->gart.pages[p];
if (page) {
- ptr = kmap(page);
+ ptr = kmap_local_page(page);
ptr += off;
r = copy_to_user(buf, ptr, cur_size);
- kunmap(rdev->gart.pages[p]);
+ kunmap_local(ptr);
} else
r = clear_user(buf, cur_size);
--
2.37.3
[PATCH] drm/amd/amdgpu: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. Since its use in amdgpu/amdgpu_ttm.c is safe, it should be preferred. Therefore, replace kmap() with kmap_local_page() in amdgpu/amdgpu_ttm.c. Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 3b4c19412625..c11657b5915f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -2301,9 +2301,9 @@ static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf, if (p->mapping != adev->mman.bdev.dev_mapping) return -EPERM; - ptr = kmap(p); + ptr = kmap_local_page(p); r = copy_to_user(buf, ptr + off, bytes); - kunmap(p); + kunmap_local(ptr); if (r) return -EFAULT; @@ -2352,9 +2352,9 @@ static ssize_t amdgpu_iomem_write(struct file *f, const char __user *buf, if (p->mapping != adev->mman.bdev.dev_mapping) return -EPERM; - ptr = kmap(p); + ptr = kmap_local_page(p); r = copy_from_user(ptr + off, buf, bytes); - kunmap(p); + kunmap_local(ptr); if (r) return -EFAULT; -- 2.37.1
[PATCH 2/3] drm/i915/gt: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Since its use in i915/gt is safe everywhere, it should be preferred.
Therefore, replace kmap() with kmap_local_page() in i915/gt
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
drivers/gpu/drm/i915/gt/shmem_utils.c| 11 ---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
index 6ebda3d65086..21d8ce40b897 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
@@ -747,7 +747,7 @@ static void swizzle_page(struct page *page)
char *vaddr;
int i;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
for (i = 0; i < PAGE_SIZE; i += 128) {
memcpy(temp, [i], 64);
@@ -755,7 +755,7 @@ static void swizzle_page(struct page *page)
memcpy([i + 64], temp, 64);
}
- kunmap(page);
+ kunmap_local(vaddr);
}
/**
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c
b/drivers/gpu/drm/i915/gt/shmem_utils.c
index 402f085f3a02..48edbb8a33e5 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c
@@ -98,22 +98,19 @@ static int __shmem_rw(struct file *file, loff_t off,
unsigned int this =
min_t(size_t, PAGE_SIZE - offset_in_page(off), len);
struct page *page;
- void *vaddr;
page = shmem_read_mapping_page_gfp(file->f_mapping, pfn,
GFP_KERNEL);
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
if (write) {
- memcpy(vaddr + offset_in_page(off), ptr, this);
+ memcpy_to_page(page, offset_in_page(off), ptr, this);
set_page_dirty(page);
} else {
- memcpy(ptr, vaddr + offset_in_page(off), this);
+ memcpy_from_page(ptr, page, offset_in_page(off), this);
}
mark_page_accessed(page);
- kunmap(page);
put_page(page);
len -= this;
@@ -140,11 +137,11 @@ int shmem_read_to_iosys_map(struct file *file, loff_t off,
if (IS_ERR(page))
return PTR_ERR(page);
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
iosys_map_memcpy_to(map, map_off, vaddr + offset_in_page(off),
this);
mark_page_accessed(page);
- kunmap(page);
+ kunmap_local(vaddr);
put_page(page);
len -= this;
--
2.37.1
[PATCH 0/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and still valid. Since its use in drm/i915 is safe everywhere, it should be preferred. Therefore, replace kmap() with kmap_local_page() in drm/i915. These changes should be tested in an 32 bits system, booting a kernel with HIGHMEM enabled. Unfortunately I have no i915 based hardware, therefore any help with testing would be greatly appreciated. Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco Fabio M. De Francesco (3): drm/i915: Replace kmap() with kmap_local_page() drm/i915/gt: Replace kmap() with kmap_local_page() drm/i915/gem: Replace kmap() with kmap_local_page() drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++ drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- drivers/gpu/drm/i915/gt/shmem_utils.c | 11 --- drivers/gpu/drm/i915/i915_gem.c| 8 5 files changed, 16 insertions(+), 21 deletions(-) -- 2.37.1
[PATCH 3/3] drm/i915/gem: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page().
There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.
With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
the tasks can be preempted and, when they are scheduled to run again, the
kernel virtual addresses are restored and are still valid.
Since its use in i915/gem is safe everywhere, it should be preferred.
Therefore, replace kmap() with kmap_local_page() in i915/gem.
Suggested-by: Ira Weiny
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4eed3dd90ba8..2bc6ab9964ff 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -640,16 +640,14 @@ i915_gem_object_create_shmem_from_data(struct
drm_i915_private *dev_priv,
do {
unsigned int len = min_t(typeof(size), size, PAGE_SIZE);
struct page *page;
- void *pgdata, *vaddr;
+ void *pgdata;
err = aops->write_begin(file, file->f_mapping, offset, len,
, );
if (err < 0)
goto fail;
- vaddr = kmap(page);
- memcpy(vaddr, data, len);
- kunmap(page);
+ memcpy_to_page(page, 0, data, len);
err = aops->write_end(file, file->f_mapping, offset, len, len,
page, pgdata);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 3ced9948a331..bb25b50b5688 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -153,7 +153,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu,
row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to
page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -171,7 +171,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
out:
i915_gem_object_lock(obj, NULL);
@@ -249,7 +249,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(to_gt(i915));
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u
[%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected
write to page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -267,7 +267,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
if (err)
return err;
--
2.37.1
[PATCH 1/3] drm/i915: Replace kmap() with kmap_local_page()
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. Since its use in i915_gem.c is safe everywhere, it should be preferred. Therefore, replace kmap() with kmap_local_page() in i915_gem.c Suggested-by: Ira Weiny Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/i915/i915_gem.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 702e5b89be22..43effce60e1b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -212,14 +212,14 @@ shmem_pread(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush) drm_clflush_virt_range(vaddr + offset, len); ret = __copy_to_user(user_data, vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } @@ -634,7 +634,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, char *vaddr; int ret; - vaddr = kmap(page); + vaddr = kmap_local_page(page); if (needs_clflush_before) drm_clflush_virt_range(vaddr + offset, len); @@ -643,7 +643,7 @@ shmem_pwrite(struct page *page, int offset, int len, char __user *user_data, if (!ret && needs_clflush_after) drm_clflush_virt_range(vaddr + offset, len); - kunmap(page); + kunmap_local(vaddr); return ret ? -EFAULT : 0; } -- 2.37.1
Re: [syzbot] WARNING in component_del
On martedì 8 febbraio 2022 08:51:29 CET syzbot wrote:
> syzbot has found a reproducer for the following issue on:
>
> HEAD commit:555f3d7be91a Merge tag '5.17-rc3-ksmbd-server-fixes' of gi..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=130a0c2c70
> kernel config: https://syzkaller.appspot.com/x/.config?x=266de9da75c71a45
> dashboard link: https://syzkaller.appspot.com/bug?extid=60df062e1c41940cae0f
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils
> for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15880d8470
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14de0c77b0
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+60df062e1c41940ca...@syzkaller.appspotmail.com
>
> [ cut here ]
> WARNING: CPU: 0 PID: 3598 at drivers/base/component.c:767
> component_del+0x40c/0x540 drivers/base/component.c:765
> Modules linked in:
> CPU: 0 PID: 3598 Comm: syz-executor255 Not tainted
> 5.17.0-rc3-syzkaller-00020-g555f3d7be91a #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> RIP: 0010:component_del+0x40c/0x540 drivers/base/component.c:767
> Code: 00 48 39 6b 20 75 82 e8 72 b1 07 fd 48 c7 43 20 00 00 00 00 e9 70 ff ff
> ff e8 60 b1 07 fd 48 c7 c7 20 aa 67 8c e8 84 d4 db 04 <0f> 0b 31 ed e8 4b b1
> 07 fd 48 89 ef 5b 5d 41 5c 41 5d 41 5e 41 5f
> RSP: 0018:c90001aafa68 EFLAGS: 00010286
> RAX: RBX: dc00 RCX: 8880745c8000
> RDX: RSI: 0008 RDI: c90001aaf9b0
> RBP: 8c67a9e0 R08: 0001 R09: c90001aaf9b7
> R10: f52000355f36 R11: 0001 R12: 88801dce5008
> R13: 8a4c0dc0 R14: 88801dce5008 R15: 88801dce5000
> FS: 56461300() GS:8880b9c0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 7fb3739a5130 CR3: 1996f000 CR4: 00350ef0
> Call Trace:
>
> usb_hub_remove_port_device+0x272/0x370 drivers/usb/core/port.c:653
> hub_disconnect+0x171/0x510 drivers/usb/core/hub.c:1737
> usb_unbind_interface+0x1d8/0x8e0 drivers/usb/core/driver.c:458
> __device_release_driver+0x5d7/0x700 drivers/base/dd.c:1206
> device_release_driver_internal drivers/base/dd.c:1237 [inline]
> device_release_driver+0x26/0x40 drivers/base/dd.c:1260
> usb_driver_release_interface+0x102/0x180 drivers/usb/core/driver.c:627
> proc_ioctl.part.0+0x4d6/0x560 drivers/usb/core/devio.c:2332
> proc_ioctl drivers/usb/core/devio.c:170 [inline]
> proc_ioctl_default drivers/usb/core/devio.c:2375 [inline]
> usbdev_do_ioctl drivers/usb/core/devio.c:2731 [inline]
> usbdev_ioctl+0x2b29/0x36c0 drivers/usb/core/devio.c:2791
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:874 [inline]
> __se_sys_ioctl fs/ioctl.c:860 [inline]
> __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7fb3739346f9
> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 b1 14 00 00 90 48 89 f8 48 89 f7 48
> 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73
> 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:7fff3db9d808 EFLAGS: 0246 ORIG_RAX: 0010
> RAX: ffda RBX: 7fb373978194 RCX: 7fb3739346f9
> RDX: 2380 RSI: c0105512 RDI: 0003
> RBP: R08: 7fff3db9d280 R09: 0001
> R10: R11: 0246 R12: 7fff3db9d81c
> R13: 431bde82d7b634db R14: R15:
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller-bugs+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/1e4c2e05d77cfcbb%40google.com.
>
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index c2bbf97a79be..8455b235976a 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -605,8 +605,11 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
find_and_link_peer(hub, port1);
retval = component_add(_dev->dev, _ops);
- if (retval)
+ if (retval) {
dev_warn(_dev->dev, "failed to add component\n");
+ device_unregister(_dev->dev);
+ return retval;
+ }
/*
* Enable runtime pm and hold a refernce that hub_configure()
[PATCH v2] drm/amd/amdgpu: Fix errors in documentation of function parameters
In the documentation of functions, removed excess parameters, described
undocumented ones, and fixed syntax errors.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: Cc'ed all the maintainers.
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 12 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 4 +++-
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 8
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 2e9b16fb3fcd..bf2939b6eb43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -76,7 +76,7 @@ struct amdgpu_atif {
/**
* amdgpu_atif_call - call an ATIF method
*
- * @handle: acpi handle
+ * @atif: acpi handle
* @function: the ATIF function to execute
* @params: ATIF function params
*
@@ -166,7 +166,6 @@ static void amdgpu_atif_parse_functions(struct
amdgpu_atif_functions *f, u32 mas
/**
* amdgpu_atif_verify_interface - verify ATIF
*
- * @handle: acpi handle
* @atif: amdgpu atif struct
*
* Execute the ATIF_FUNCTION_VERIFY_INTERFACE ATIF function
@@ -240,8 +239,7 @@ static acpi_handle amdgpu_atif_probe_handle(acpi_handle
dhandle)
/**
* amdgpu_atif_get_notification_params - determine notify configuration
*
- * @handle: acpi handle
- * @n: atif notification configuration struct
+ * @atif: acpi handle
*
* Execute the ATIF_FUNCTION_GET_SYSTEM_PARAMETERS ATIF function
* to determine if a notifier is used and if so which one
@@ -304,7 +302,7 @@ static int amdgpu_atif_get_notification_params(struct
amdgpu_atif *atif)
/**
* amdgpu_atif_query_backlight_caps - get min and max backlight input signal
*
- * @handle: acpi handle
+ * @atif: acpi handle
*
* Execute the QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS ATIF function
* to determine the acceptable range of backlight values
@@ -363,7 +361,7 @@ static int amdgpu_atif_query_backlight_caps(struct
amdgpu_atif *atif)
/**
* amdgpu_atif_get_sbios_requests - get requested sbios event
*
- * @handle: acpi handle
+ * @atif: acpi handle
* @req: atif sbios request struct
*
* Execute the ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS ATIF function
@@ -899,6 +897,8 @@ void amdgpu_acpi_fini(struct amdgpu_device *adev)
/**
* amdgpu_acpi_is_s0ix_supported
*
+ * @adev: amdgpu_device_pointer
+ *
* returns true if supported, false if not.
*/
bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
index 5af464933976..98d31ebad9ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
@@ -111,6 +111,8 @@ static const char *amdkfd_fence_get_timeline_name(struct
dma_fence *f)
* a KFD BO and schedules a job to move the BO.
* If fence is already signaled return true.
* If fence is not signaled schedule a evict KFD process work item.
+ *
+ * @f: dma_fence
*/
static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
{
@@ -131,7 +133,7 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence
*f)
/**
* amdkfd_fence_release - callback that fence can be freed
*
- * @fence: fence
+ * @f: dma_fence
*
* This function is called when the reference count becomes zero.
* Drops the mm_struct reference and RCU schedules freeing up the fence.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index b43e68fc1378..ed3014fbb563 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -719,7 +719,7 @@ static void unlock_spi_csq_mutexes(struct amdgpu_device
*adev)
}
/**
- * @get_wave_count: Read device registers to get number of waves in flight for
+ * get_wave_count: Read device registers to get number of waves in flight for
* a particular queue. The method also returns the VMID associated with the
* queue.
*
@@ -755,19 +755,19 @@ static void get_wave_count(struct amdgpu_device *adev,
int queue_idx,
}
/**
- * @kgd_gfx_v9_get_cu_occupancy: Reads relevant registers associated with each
+ * kgd_gfx_v9_get_cu_occupancy: Reads relevant registers associated with each
* shader engine and aggregates the number of waves that are in flight for the
* process whose pasid is provided as a parameter. The process could have ZERO
* or more queues running and submitting waves to compute units.
*
* @kgd: Handle of device from which to get number of waves in flight
* @pasid: Identifies the process for which this query call is invoked
- * @wave_cnt: Output parameter updated with number of waves in flight that
+ * @pasid_wave_cnt: Output parameter updated with number of waves in flight
that
* belong to process with given pasid
* @max_waves_per_cu: Output parameter updated with maximum number of waves
[PATCH v3] drm/drm_file.c: Define drm_send_event_helper() as 'static'
drm_send_event_helper() has not prototype, it has internal linkage and
therefore it should be defined with storage class 'static'.
Signed-off-by: Fabio M. De Francesco
---
Changes from v2: Removed all the other lines in function comment.
Changes from v1: As suggested by Daniel Vetter, removed unnecessary
kernel-doc comments.
drivers/gpu/drm/drm_file.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 7efbccffc2ea..d4f0bac6f8f8 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -774,19 +774,7 @@ void drm_event_cancel_free(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_event_cancel_free);
-/**
- * drm_send_event_helper - send DRM event to file descriptor
- * @dev: DRM device
- * @e: DRM event to deliver
- * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC
- * time domain
- *
- * This helper function sends the event @e, initialized with
- * drm_event_reserve_init(), to its associated userspace DRM file.
- * The timestamp variant of dma_fence_signal is used when the caller
- * sends a valid timestamp.
- */
-void drm_send_event_helper(struct drm_device *dev,
+static void drm_send_event_helper(struct drm_device *dev,
struct drm_pending_event *e, ktime_t timestamp)
{
assert_spin_locked(>event_lock);
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock
drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.
Suggested-by: Daniel Vetter
Suggested-by: Matthew Wilcox
Signed-off-by: Fabio M. De Francesco
Reviewed-by: Matthew Wilcox (Oracle)
---
Changes from v3: CC'ed more (previously missing) maintainers.
Changes from v2: Drop file name from the Subject. Cc'ed all maintainers.
Changes from v1: Removed unnecessary braces around single statement
block.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 80130c1c0c68..39204dbc168b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1595,17 +1595,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
- drm_modeset_lock_all(drm_dev);
-
drm_for_each_crtc(crtc, drm_dev) {
- if (crtc->state->active) {
+ drm_modeset_lock(>mutex, NULL);
+ if (crtc->state->active)
ret = -EBUSY;
+ drm_modeset_unlock(>mutex);
+ if (ret < 0)
break;
- }
}
- drm_modeset_unlock_all(drm_dev);
-
} else {
struct drm_connector *list_connector;
struct drm_connector_list_iter iter;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/drm_file.c: Define drm_send_event_helper() as 'static'
drm_send_event_helper() has not prototype, it has internal linkage and
therefore it should be defined with storage class 'static'.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: As suggested by Daniel Vetter, removed unnecessary
kernel-doc comments.
drivers/gpu/drm/drm_file.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 7efbccffc2ea..a32e0d4f3604 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -774,19 +774,15 @@ void drm_event_cancel_free(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_event_cancel_free);
-/**
+/*
* drm_send_event_helper - send DRM event to file descriptor
- * @dev: DRM device
- * @e: DRM event to deliver
- * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC
- * time domain
*
- * This helper function sends the event @e, initialized with
+ * This helper function sends the event e, initialized with
* drm_event_reserve_init(), to its associated userspace DRM file.
* The timestamp variant of dma_fence_signal is used when the caller
* sends a valid timestamp.
*/
-void drm_send_event_helper(struct drm_device *dev,
+static void drm_send_event_helper(struct drm_device *dev,
struct drm_pending_event *e, ktime_t timestamp)
{
assert_spin_locked(>event_lock);
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3] drm/amd/amdgpu: Replace drm_modeset_lock_all with drm_modeset_lock
drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.
Suggested-by: Daniel Vetter
Suggested-by: Matthew Wilcox
Signed-off-by: Fabio M. De Francesco
Reviewed-by: Matthew Wilcox (Oracle)
---
Changes from v2: Drop file name from the Subject. Cc'ed all maintainers.
Changes from v1: Removed unnecessary braces around single statement
block.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..adfeec2b17c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1439,17 +1439,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
- drm_modeset_lock_all(drm_dev);
-
drm_for_each_crtc(crtc, drm_dev) {
- if (crtc->state->active) {
+ drm_modeset_lock(>mutex, NULL);
+ if (crtc->state->active)
ret = -EBUSY;
+ drm_modeset_unlock(>mutex);
+ if (ret < 0)
break;
- }
}
- drm_modeset_unlock_all(drm_dev);
-
} else {
struct drm_connector *list_connector;
struct drm_connector_list_iter iter;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock
On Monday, April 26, 2021 6:11:11 PM CEST Daniel Vetter wrote:
> On Thu, Apr 22, 2021 at 05:50:34PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> > > - drm_modeset_lock_all(drm_dev);
> > > -
> > >
> > > drm_for_each_crtc(crtc, drm_dev) {
> > >
> > > + drm_modeset_lock(>mutex, NULL);
> > >
> > > if (crtc->state->active) {
> > >
> > > ret = -EBUSY;
> > >
> > > - break;
> > >
> > > }
> > >
> > > + drm_modeset_unlock(>mutex);
> > > + if (ret < 0)
> > > + break;
> > >
> > > }
> > >
> > > - drm_modeset_unlock_all(drm_dev);
> > > -
> >
> > I might remove the {} around ret = -EBUSY, but this is good.
> >
> > Reviewed-by: Matthew Wilcox (Oracle)
>
> Yup patch looks good, but it's not cc'ed to drm/amdgpu maintainers/m-l, so
> likely won't get picked up. Can you pls check scripts/get_maintainers for
> anything you've missed, add those and resend with Willy's r-b tag
> included?
>
> Then Alex can pick it up for merging.
>
> Thanks, Daniel
>
I had already submitted a v2 of this patch with an added 'Review-by' Matthew
Wilcox under my name. It removed the unnecessary braces that willy pointed
out. However I see that not all maintainers had been cc'ed, so I'm going to
resend it as v3.
Thanks,
Fabio
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/amd/pm/powerplay/hwmgr: Fix kernel-doc syntax in documentation
Fixed kernel-doc syntax errors in documentation of functions. Signed-off-by: Fabio M. De Francesco --- Changes from v1: Reword both the subject and the log message drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c index b1038d30c8dc..f503e61faa60 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c @@ -275,7 +275,7 @@ static const ATOM_VOLTAGE_OBJECT_V3 *atomctrl_lookup_voltage_type_v3( } /** - * atomctrl_get_memory_pll_dividers_si(). + * atomctrl_get_memory_pll_dividers_si * * @hwmgr: input parameter: pointer to HwMgr * @clock_value: input parameter: memory clock @@ -328,7 +328,7 @@ int atomctrl_get_memory_pll_dividers_si( } /** - * atomctrl_get_memory_pll_dividers_vi(). + * atomctrl_get_memory_pll_dividers_vi * * @hwmgr: input parameter: pointer to HwMgr * @clock_value: input parameter: memory clock @@ -1104,7 +1104,7 @@ int atomctrl_calculate_voltage_evv_on_sclk( } /** - * atomctrl_get_voltage_evv_on_sclk gets voltage via call to ATOM COMMAND table. + * atomctrl_get_voltage_evv_on_sclk: gets voltage via call to ATOM COMMAND table. * @hwmgr: input: pointer to hwManager * @voltage_type: input: type of EVV voltage VDDC or VDDGFX * @sclk: input: in 10Khz unit. DPM state SCLK frequency @@ -1144,7 +1144,7 @@ int atomctrl_get_voltage_evv_on_sclk( } /** - * atomctrl_get_voltage_evv gets voltage via call to ATOM COMMAND table. + * atomctrl_get_voltage_evv: gets voltage via call to ATOM COMMAND table. * @hwmgr: input: pointer to hwManager * @virtual_voltage_id: input: voltage id which match per voltage DPM state: 0xff01, 0xff02.. 0xff08 * @voltage: output: real voltage level in unit of mv -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/amdkfd/kfd_process.c: Fix kernel-doc syntax error
Fixed a kernel-doc error in the documentation of a function. Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index 65803e153a22..12587fa768a9 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -250,7 +250,7 @@ static void kfd_sdma_activity_worker(struct work_struct *work) } /** - * @kfd_get_cu_occupancy() - Collect number of waves in-flight on this device + * @kfd_get_cu_occupancy - Collect number of waves in-flight on this device * by current process. Translates acquired wave count into number of compute units * that are occupied. * -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/pm/powerplay/hwmgr/ppatomctrl.c: Fix kerne-doc syntax in comments
In the documentation of functions, fixed kernel-doc syntax errors. Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c index b1038d30c8dc..f503e61faa60 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c @@ -275,7 +275,7 @@ static const ATOM_VOLTAGE_OBJECT_V3 *atomctrl_lookup_voltage_type_v3( } /** - * atomctrl_get_memory_pll_dividers_si(). + * atomctrl_get_memory_pll_dividers_si * * @hwmgr: input parameter: pointer to HwMgr * @clock_value: input parameter: memory clock @@ -328,7 +328,7 @@ int atomctrl_get_memory_pll_dividers_si( } /** - * atomctrl_get_memory_pll_dividers_vi(). + * atomctrl_get_memory_pll_dividers_vi * * @hwmgr: input parameter: pointer to HwMgr * @clock_value: input parameter: memory clock @@ -1104,7 +1104,7 @@ int atomctrl_calculate_voltage_evv_on_sclk( } /** - * atomctrl_get_voltage_evv_on_sclk gets voltage via call to ATOM COMMAND table. + * atomctrl_get_voltage_evv_on_sclk: gets voltage via call to ATOM COMMAND table. * @hwmgr: input: pointer to hwManager * @voltage_type: input: type of EVV voltage VDDC or VDDGFX * @sclk: input: in 10Khz unit. DPM state SCLK frequency @@ -1144,7 +1144,7 @@ int atomctrl_get_voltage_evv_on_sclk( } /** - * atomctrl_get_voltage_evv gets voltage via call to ATOM COMMAND table. + * atomctrl_get_voltage_evv: gets voltage via call to ATOM COMMAND table. * @hwmgr: input: pointer to hwManager * @virtual_voltage_id: input: voltage id which match per voltage DPM state: 0xff01, 0xff02.. 0xff08 * @voltage: output: real voltage level in unit of mv -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/amdgpu: Fix errors in documentation of function parameters
In the function documentation, I removed the excess parameters,
described the undocumented ones, and fixed the syntax errors.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 12 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 4 +++-
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 8
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 2e9b16fb3fcd..bf2939b6eb43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -76,7 +76,7 @@ struct amdgpu_atif {
/**
* amdgpu_atif_call - call an ATIF method
*
- * @handle: acpi handle
+ * @atif: acpi handle
* @function: the ATIF function to execute
* @params: ATIF function params
*
@@ -166,7 +166,6 @@ static void amdgpu_atif_parse_functions(struct
amdgpu_atif_functions *f, u32 mas
/**
* amdgpu_atif_verify_interface - verify ATIF
*
- * @handle: acpi handle
* @atif: amdgpu atif struct
*
* Execute the ATIF_FUNCTION_VERIFY_INTERFACE ATIF function
@@ -240,8 +239,7 @@ static acpi_handle amdgpu_atif_probe_handle(acpi_handle
dhandle)
/**
* amdgpu_atif_get_notification_params - determine notify configuration
*
- * @handle: acpi handle
- * @n: atif notification configuration struct
+ * @atif: acpi handle
*
* Execute the ATIF_FUNCTION_GET_SYSTEM_PARAMETERS ATIF function
* to determine if a notifier is used and if so which one
@@ -304,7 +302,7 @@ static int amdgpu_atif_get_notification_params(struct
amdgpu_atif *atif)
/**
* amdgpu_atif_query_backlight_caps - get min and max backlight input signal
*
- * @handle: acpi handle
+ * @atif: acpi handle
*
* Execute the QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS ATIF function
* to determine the acceptable range of backlight values
@@ -363,7 +361,7 @@ static int amdgpu_atif_query_backlight_caps(struct
amdgpu_atif *atif)
/**
* amdgpu_atif_get_sbios_requests - get requested sbios event
*
- * @handle: acpi handle
+ * @atif: acpi handle
* @req: atif sbios request struct
*
* Execute the ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS ATIF function
@@ -899,6 +897,8 @@ void amdgpu_acpi_fini(struct amdgpu_device *adev)
/**
* amdgpu_acpi_is_s0ix_supported
*
+ * @adev: amdgpu_device_pointer
+ *
* returns true if supported, false if not.
*/
bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
index 5af464933976..98d31ebad9ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
@@ -111,6 +111,8 @@ static const char *amdkfd_fence_get_timeline_name(struct
dma_fence *f)
* a KFD BO and schedules a job to move the BO.
* If fence is already signaled return true.
* If fence is not signaled schedule a evict KFD process work item.
+ *
+ * @f: dma_fence
*/
static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
{
@@ -131,7 +133,7 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence
*f)
/**
* amdkfd_fence_release - callback that fence can be freed
*
- * @fence: fence
+ * @f: dma_fence
*
* This function is called when the reference count becomes zero.
* Drops the mm_struct reference and RCU schedules freeing up the fence.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index b43e68fc1378..ed3014fbb563 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -719,7 +719,7 @@ static void unlock_spi_csq_mutexes(struct amdgpu_device
*adev)
}
/**
- * @get_wave_count: Read device registers to get number of waves in flight for
+ * get_wave_count: Read device registers to get number of waves in flight for
* a particular queue. The method also returns the VMID associated with the
* queue.
*
@@ -755,19 +755,19 @@ static void get_wave_count(struct amdgpu_device *adev,
int queue_idx,
}
/**
- * @kgd_gfx_v9_get_cu_occupancy: Reads relevant registers associated with each
+ * kgd_gfx_v9_get_cu_occupancy: Reads relevant registers associated with each
* shader engine and aggregates the number of waves that are in flight for the
* process whose pasid is provided as a parameter. The process could have ZERO
* or more queues running and submitting waves to compute units.
*
* @kgd: Handle of device from which to get number of waves in flight
* @pasid: Identifies the process for which this query call is invoked
- * @wave_cnt: Output parameter updated with number of waves in flight that
+ * @pasid_wave_cnt: Output parameter updated with number of waves in flight
that
* belong to process with given pasid
* @max_waves_per_cu: Output parameter updated with maximum number of waves
* possible per Compute Unit
[PATCH v2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock
drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.
Suggested-by: Daniel Vetter
Suggested-by: Matthew Wilcox
Signed-off-by: Fabio M. De Francesco
Reviewed-by: Matthew Wilcox (Oracle)
---
Changes from v1: Removed unnecessary braces around single statement
block.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..adfeec2b17c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1439,17 +1439,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
- drm_modeset_lock_all(drm_dev);
-
drm_for_each_crtc(crtc, drm_dev) {
- if (crtc->state->active) {
+ drm_modeset_lock(>mutex, NULL);
+ if (crtc->state->active)
ret = -EBUSY;
+ drm_modeset_unlock(>mutex);
+ if (ret < 0)
break;
- }
}
- drm_modeset_unlock_all(drm_dev);
-
} else {
struct drm_connector *list_connector;
struct drm_connector_list_iter iter;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/drm_file.c: Define drm_send_event_helper() as 'static'
drm_send_event_helper() has not prototype, it has internal linkage and
therefore it should be defined with storage class 'static'.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/drm_file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 7efbccffc2ea..17f38d873972 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -786,7 +786,7 @@ EXPORT_SYMBOL(drm_event_cancel_free);
* The timestamp variant of dma_fence_signal is used when the caller
* sends a valid timestamp.
*/
-void drm_send_event_helper(struct drm_device *dev,
+static void drm_send_event_helper(struct drm_device *dev,
struct drm_pending_event *e, ktime_t timestamp)
{
assert_spin_locked(>event_lock);
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock
drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.
Suggested-by: Daniel Vetter
Suggested-by: Matthew Wilcox
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..bce8f6793d8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1439,17 +1439,16 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
- drm_modeset_lock_all(drm_dev);
-
drm_for_each_crtc(crtc, drm_dev) {
+ drm_modeset_lock(>mutex, NULL);
if (crtc->state->active) {
ret = -EBUSY;
- break;
}
+ drm_modeset_unlock(>mutex);
+ if (ret < 0)
+ break;
}
- drm_modeset_unlock_all(drm_dev);
-
} else {
struct drm_connector *list_connector;
struct drm_connector_list_iter iter;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*()
This second patch makes use of the API that has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 856903db34c5..43dd77c227ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1441,12 +1441,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
struct drm_modeset_acquire_ctx ctx;
int ret_lock = 0;
-retry:
- drm_modeset_lock_all_ctx(drm_dev, );
- if(ret_lock == -EDEADLK) {
- drm_modeset_backoff();
- goto retry;
- }
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1455,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_drop_locks();
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else {
struct drm_connector *list_connector;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()
Replace the deprecated API with new helpers, according to the
TODO list of the DRM subsystem.
In this first patch drm_modeset_lock_all() is replaced with
drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(),
the latter doesn’t take the global drm_mode_config.mutex
since that lock isn’t required for modeset state changes.
Signed-off-by: Fabio M. De Francesco
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..856903db34c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
-
- drm_modeset_lock_all(drm_dev);
+ struct drm_modeset_acquire_ctx ctx;
+ int ret_lock = 0;
+
+retry:
+ drm_modeset_lock_all_ctx(drm_dev, );
+ if(ret_lock == -EDEADLK) {
+ drm_modeset_backoff();
+ goto retry;
+ }
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_unlock_all(drm_dev);
+ drm_modeset_drop_locks();
} else {
struct drm_connector *list_connector;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*()
Replace the deprecated API with new helpers, according to the TODO list of the DRM subsystem. The new API has been introduced with commit b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset locks using a local context. This has the advantage of reducing boilerplate. The work is carried out in two consecutive logical steps: the first patch converts the code to use drm_modeset_lock_all(), then the second does the final replace with DRM_MODESET_LOCK_ALL_*(). Changes from v2: The work is split in two consecutive logical steps. Changes from v1: Added further information to the commit message. Fabio M. De Francesco (2): drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx() drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*() drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
On Tuesday, April 20, 2021 7:49:35 PM CEST Daniel Vetter wrote:
> On Mon, Apr 19, 2021 at 05:03:40PM +0200, Fabio M. De Francesco wrote:
> > Replace the deprecated API with new helpers, according to the TODO list
> > of the DRM subsystem. The new API has been introduced with commit
> > b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
> > locks using a local context and has the advantage of reducing boilerplate.
>
> So this is only half of the story, because the boilerplate only exists
> when you're using drm_modeset_lock_all_ctx. Which should be used for
> modern atomic display drivers everywhere.
>
> But drm_modeset_lock_all_ctx isn't exactly the same as
> drm_modeset_lock_all, so this needs to be explained.
>
> Now the problem with drm_modeset_lock_all is that it hides a memory
> allocation, and if that allocation fails then we just die. Which isn't
> great really, but in practice the kernel tries really hard to never fail
> this allocation. That's why we move the drm_modeset_acquire_ctx onto the
> stack.
>
> I think for understanding what's going on here you'd first need to convert
> the code to the full open-code boilerplate using drm_modeset_lock_all_ctx,
> with explanations of why the changes are ok. Then replace it with the
> convenient macro. Once it's clear what's going on under the hood it should
> then be easier to explain the situation in subsequent conversions with
> just one patch.
> -Daniel
>
Thanks for the clarification. I'll go through this code again using the path
you showed. Eventually I will send out another patch.
Fabio
>
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > Changes from v1: Added further information in the commit message.
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 --
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
6447cd6ca5a8..e1a71579f8e6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -32,6 +32,7 @@
> >
> > #include
> >
> > #include
> >
> > +#include
> >
> > #include
> > #include
> > #include
> >
> > @@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev,
bool fbcon)
> >
> > if (!amdgpu_device_has_dc_support(adev)) {
> >
> > /* turn off display hw */
> >
> > - drm_modeset_lock_all(dev);
> > + struct drm_modeset_acquire_ctx ctx;
> > + int ret;
> > +
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > drm_connector_list_iter_begin(dev, );
> > drm_for_each_connector_iter(connector, )
> >
> > drm_helper_connector_dpms(connector,
> >
> >
DRM_MODE_DPMS_OFF);
> >
> > drm_connector_list_iter_end();
> >
> > - drm_modeset_unlock_all(dev);
> > - /* unpin the front buffers and cursors */
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> > + /* unpin the front buffers and cursors */
> >
> > list_for_each_entry(crtc, >mode_config.crtc_list,
head) {
> >
> > struct amdgpu_crtc *amdgpu_crtc =
to_amdgpu_crtc(crtc);
> > struct drm_framebuffer *fb = crtc->primary-
>fb;
> >
> > @@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev,
bool fbcon)
> >
> > /* blat the mode back in */
> > if (fbcon) {
> >
> > if (!amdgpu_device_has_dc_support(adev)) {
> >
> > + struct drm_modeset_acquire_ctx ctx;
> > + int ret;
> > +
> >
> > /* pre DCE11 */
> > drm_helper_resume_force_mode(dev);
> >
> > /* turn on display hw */
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0,
ret);
> >
> > drm_connector_list_iter_begin(dev, );
> > drm_for_each_connector_iter(connector,
)
> >
> >
drm_helper_connector_dpms(connector,
> >
> >
DRM_MODE_DPMS_ON);
> >
> > drm_connector_list_iter_end();
> >
> > -
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> >
> > }
> > amdgpu_fbdev_set_suspend(adev, 0);
> >
> > }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem. The new API has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..0e9b7a180ee7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1438,8 +1438,10 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
+ struct drm_modeset_acquire_ctx ctx;
+ int ret_lock;
- drm_modeset_lock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1448,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_unlock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else {
struct drm_connector *list_connector;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem. The new API has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: Added further information in the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6447cd6ca5a8..e1a71579f8e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include
#include
@@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
if (!amdgpu_device_has_dc_support(adev)) {
/* turn off display hw */
- drm_modeset_lock_all(dev);
+ struct drm_modeset_acquire_ctx ctx;
+ int ret;
+
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
drm_connector_list_iter_begin(dev, );
drm_for_each_connector_iter(connector, )
drm_helper_connector_dpms(connector,
DRM_MODE_DPMS_OFF);
drm_connector_list_iter_end();
- drm_modeset_unlock_all(dev);
- /* unpin the front buffers and cursors */
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
+ /* unpin the front buffers and cursors */
list_for_each_entry(crtc, >mode_config.crtc_list, head) {
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
struct drm_framebuffer *fb = crtc->primary->fb;
@@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev, bool
fbcon)
/* blat the mode back in */
if (fbcon) {
if (!amdgpu_device_has_dc_support(adev)) {
+ struct drm_modeset_acquire_ctx ctx;
+ int ret;
+
/* pre DCE11 */
drm_helper_resume_force_mode(dev);
/* turn on display hw */
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
drm_connector_list_iter_begin(dev, );
drm_for_each_connector_iter(connector, )
drm_helper_connector_dpms(connector,
DRM_MODE_DPMS_ON);
drm_connector_list_iter_end();
-
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
}
amdgpu_fbdev_set_suspend(adev, 0);
}
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/2] Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with new helpers, according to the TODO list of the DRM subsystem. The new API has been introduced with commit b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset locks using a local context. This has the advantage of reducing boilerplate. Changes from v1: Added further information to the commit message. Fabio M. De Francesco (2): drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_* drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_* drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 -- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 6 -- 2 files changed, 16 insertions(+), 8 deletions(-) -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
On Monday, April 19, 2021 3:08:51 PM CEST Julia Lawall wrote:
> On Mon, 19 Apr 2021, Fabio M. De Francesco wrote:
> > Replace the deprecated API with new helpers, according to the TODO list
> > of the DRM subsystem.
>
> The commit message will perhaps not be very meaningful one year from now.
> You could say for example DRM_MODESET_LOCK_ALL_BEGIN was introduced in
> commit XXX (there is a proper format for referring to other commits) for
> what purpose. And then say that you are making the replacement.
>
> Actually, I'm a little surprised to see something that looks like a
> function call be replaced by something that looks like a macro. Maybe it
> was a macro all along, and this is just making that more clear. In any
> case, if I were to look at this commit, I would appreciate a little more
> context information.
>
> julia
>
I have made that message in line with an old commit (9bcaa3fe58ab) that had
been taken by D. Vetter (the DRM maintainer). That message didn't explain more
than referring to the TODO list. I try to stick with each subsystem's
conventions.
However, I agree with you: referring to a TODO list, which some day will
surely change, is not the best means to provide context information.
>From that macro documentation
>(https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.DRM_MODESET_LOCK_ALL_BEGIN):
"DRM_MODESET_LOCK_ALL_BEGIN simplifies grabbing all modeset locks using a
local context. This has the advantage of reducing boilerplate, [...]".
(Please, note that I haven't copied the last part of the paragraph because it
talks about checking the return value but I think it's useless because the
only possible return value is 0).
If more context information is needed, I would add the above-mentioned note to
my commit message and submit a v2 patch.
Is it the right way to solve the issue that you pointed out?
Thanks in advance,
Fabio
>
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 --
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
> > 6447cd6ca5a8..e1a71579f8e6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -32,6 +32,7 @@
> >
> > #include
> >
> > #include
> >
> > +#include
> >
> > #include
> > #include
> > #include
> >
> > @@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev,
> > bool fbcon)>
> > if (!amdgpu_device_has_dc_support(adev)) {
> >
> > /* turn off display hw */
> >
> > - drm_modeset_lock_all(dev);
> > + struct drm_modeset_acquire_ctx ctx;
> > + int ret;
> > +
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > drm_connector_list_iter_begin(dev, );
> > drm_for_each_connector_iter(connector, )
> >
> > drm_helper_connector_dpms(connector,
> >
> >
DRM_MODE_DPMS_OFF);
> >
> > drm_connector_list_iter_end();
> >
> > - drm_modeset_unlock_all(dev);
> > - /* unpin the front buffers and cursors */
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> > + /* unpin the front buffers and cursors */
> >
> > list_for_each_entry(crtc, >mode_config.crtc_list,
head) {
> >
> > struct amdgpu_crtc *amdgpu_crtc =
to_amdgpu_crtc(crtc);
> > struct drm_framebuffer *fb = crtc->primary-
>fb;
> >
> > @@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev,
> > bool fbcon)>
> > /* blat the mode back in */
> > if (fbcon) {
> >
> > if (!amdgpu_device_has_dc_support(adev)) {
> >
> > + struct drm_modeset_acquire_ctx ctx;
> > + int ret;
> > +
> >
> > /* pre DCE11 */
> > drm_helper_resume_force_mode(dev);
> >
> > /* turn on display hw */
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0,
ret);
> >
> > drm_connector_list_iter_begin(dev, );
> >
[PATCH 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..0e9b7a180ee7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1438,8 +1438,10 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
+ struct drm_modeset_acquire_ctx ctx;
+ int ret_lock;
- drm_modeset_lock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1448,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_unlock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else {
struct drm_connector *list_connector;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6447cd6ca5a8..e1a71579f8e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include
#include
@@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
if (!amdgpu_device_has_dc_support(adev)) {
/* turn off display hw */
- drm_modeset_lock_all(dev);
+ struct drm_modeset_acquire_ctx ctx;
+ int ret;
+
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
drm_connector_list_iter_begin(dev, );
drm_for_each_connector_iter(connector, )
drm_helper_connector_dpms(connector,
DRM_MODE_DPMS_OFF);
drm_connector_list_iter_end();
- drm_modeset_unlock_all(dev);
- /* unpin the front buffers and cursors */
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
+ /* unpin the front buffers and cursors */
list_for_each_entry(crtc, >mode_config.crtc_list, head) {
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
struct drm_framebuffer *fb = crtc->primary->fb;
@@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev, bool
fbcon)
/* blat the mode back in */
if (fbcon) {
if (!amdgpu_device_has_dc_support(adev)) {
+ struct drm_modeset_acquire_ctx ctx;
+ int ret;
+
/* pre DCE11 */
drm_helper_resume_force_mode(dev);
/* turn on display hw */
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
drm_connector_list_iter_begin(dev, );
drm_for_each_connector_iter(connector, )
drm_helper_connector_dpms(connector,
DRM_MODE_DPMS_ON);
drm_connector_list_iter_end();
-
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
}
amdgpu_fbdev_set_suspend(adev, 0);
}
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/amd/amdgpu: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
According to the TODO list of the DRM subsystem, replace the deprecated drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*(). Fabio M. De Francesco (2): drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_* drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_* drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 -- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 6 -- 2 files changed, 16 insertions(+), 8 deletions(-) -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/dispnv50: disp.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*
Replace the deprecated API with DRM_MODESET_LOCK_ALL_* helpers (according
to the TODO list of the DRM subsystem).
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 828f48d5bdd4..e167bf96ff12 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -666,16 +666,19 @@ nv50_audio_component_bind(struct device *kdev, struct
device *hda_kdev,
struct drm_device *drm_dev = dev_get_drvdata(kdev);
struct nouveau_drm *drm = nouveau_drm(drm_dev);
struct drm_audio_component *acomp = data;
+ struct drm_modeset_acquire_ctx ctx;
+ int ret = 0;
if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
return -ENOMEM;
- drm_modeset_lock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
acomp->ops = _audio_component_ops;
acomp->dev = kdev;
drm->audio.component = acomp;
- drm_modeset_unlock_all(drm_dev);
- return 0;
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
+
+ return ret;
}
static void
@@ -685,12 +688,14 @@ nv50_audio_component_unbind(struct device *kdev, struct
device *hda_kdev,
struct drm_device *drm_dev = dev_get_drvdata(kdev);
struct nouveau_drm *drm = nouveau_drm(drm_dev);
struct drm_audio_component *acomp = data;
+ struct drm_modeset_acquire_ctx ctx;
+ int ret = 0;
- drm_modeset_lock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
drm->audio.component = NULL;
acomp->ops = NULL;
acomp->dev = NULL;
- drm_modeset_unlock_all(drm_dev);
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
}
static const struct component_ops nv50_audio_component_bind_ops = {
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/drm_bufs.c: In switch, add break in default case
Added a "break" in the default case of a switch select statement. GCC complains, although this "break" is not strictly necessary for the code to work as expected. Signed-off-by: Fabio M. De Francesco --- Changes from v1: Added the reason why of this change in the log. drivers/gpu/drm/drm_bufs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index e3d77dfefb0a..fc40aa0adf73 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -79,7 +79,7 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, return entry; break; default: /* Make gcc happy */ - ; + break; } if (entry->map->offset == map->offset) return entry; -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH Resend] drm/drm_bufs.c: In switch, add break in default case
On Saturday, April 17, 2021 5:45:46 PM CEST Julia Lawall wrote: > On Sat, 17 Apr 2021, Fabio M. De Francesco wrote: > > Added a 'break' in the default case of a switch selection statement. > > Why? > GCC issues a warning, even if it is not strictly necessary for the code to work properly. Do you think that I have to explain in the patch log the reason why I made that change? Thanks, Fabio > > julia > > > Signed-off-by: Fabio M. De Francesco > > --- > > > > drivers/gpu/drm/drm_bufs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > > index e3d77dfefb0a..fc40aa0adf73 100644 > > --- a/drivers/gpu/drm/drm_bufs.c > > +++ b/drivers/gpu/drm/drm_bufs.c > > @@ -79,7 +79,7 @@ static struct drm_map_list *drm_find_matching_map(struct > > drm_device *dev,> > > return entry; > > > > break; > > > > default: /* Make gcc happy */ > > > > - ; > > + break; > > > > } > > if (entry->map->offset == map->offset) > > > > return entry; > > > > -- > > 2.31.1 > > > > -- > > You received this message because you are subscribed to the Google Groups > > "outreachy-kernel" group. To unsubscribe from this group and stop receiving > > emails from it, send an email to > > outreachy-kernel+unsubscr...@googlegroups.com. To view this discussion on > > the web visit > > https://groups.google.com/d/msgid/outreachy-kernel/20210417153540.22017-1-f > > mdefrancesco%40gmail.com. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH Resend] drm/drm_bufs.c: In switch, add break in default case
Added a 'break' in the default case of a switch selection statement. Signed-off-by: Fabio M. De Francesco --- drivers/gpu/drm/drm_bufs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index e3d77dfefb0a..fc40aa0adf73 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -79,7 +79,7 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, return entry; break; default: /* Make gcc happy */ - ; + break; } if (entry->map->offset == map->offset) return entry; -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH] drm: nouveau: dispnv50: disp.c: Remove set but unused variables
On Thursday, April 15, 2021 9:57:07 PM CEST Julia Lawall wrote:
> On Thu, 15 Apr 2021, Fabio M. De Francesco wrote:
> > Removed two set but unused variables.
>
> Would it be useful to use the values?
>
It would be useful if one wants to check returns for errors (which are
signaled by res different than 0).
However most other drm code never checks it (as in drivers/gpu/drm/radeon/
radeon_dp_mst.c: lines 424 and 453).
Differently, drivers/gpu/drm/i915/display/intel_dp_mst.c does some logging:
ret = drm_dp_update_payload_part1(_dp->mst_mgr);
if (ret) {
drm_dbg_kms(>drm, "failed to update payload %d\n", ret);
I could do like the second example, but I'm not sure maintainers would like
it.
What do you think about it?
Thanks,
Fabio
>
> julia
>
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 +++-
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c index
> > 196612addfd6..828f48d5bdd4 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1386,12 +1386,11 @@ nv50_mstm_cleanup(struct nv50_mstm *mstm)
> >
> > {
> >
> > struct nouveau_drm *drm = nouveau_drm(mstm->outp->base.base.dev);
> > struct drm_encoder *encoder;
> >
> > - int ret;
> >
> > NV_ATOMIC(drm, "%s: mstm cleanup\n", mstm->outp->base.base.name);
> >
> > - ret = drm_dp_check_act_status(>mgr);
> > + drm_dp_check_act_status(>mgr);
> >
> > - ret = drm_dp_update_payload_part2(>mgr);
> > + drm_dp_update_payload_part2(>mgr);
> >
> > drm_for_each_encoder(encoder, mstm->outp->base.base.dev) {
> >
> > if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST) {
> >
> > @@ -1410,10 +1409,9 @@ nv50_mstm_prepare(struct nv50_mstm *mstm)
> >
> > {
> >
> > struct nouveau_drm *drm = nouveau_drm(mstm->outp->base.base.dev);
> > struct drm_encoder *encoder;
> >
> > - int ret;
> >
> > NV_ATOMIC(drm, "%s: mstm prepare\n", mstm->outp->base.base.name);
> >
> > - ret = drm_dp_update_payload_part1(>mgr);
> > + drm_dp_update_payload_part1(>mgr);
> >
> > drm_for_each_encoder(encoder, mstm->outp->base.base.dev) {
> >
> > if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST) {
> >
> > --
> > 2.31.1
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "outreachy-kernel" group. To unsubscribe from this group and
> > stop receiving emails from it, send an email to
> > outreachy-kernel+unsubscr...@googlegroups.com. To view this discussion
> > on the web visit
> > https://groups.google.com/d/msgid/outreachy-kernel/20210415194423.4880
> > -1-fmdefrancesco%40gmail.com.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: nouveau: dispnv50: disp.c: Remove set but unused variables
Removed two set but unused variables.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 196612addfd6..828f48d5bdd4 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1386,12 +1386,11 @@ nv50_mstm_cleanup(struct nv50_mstm *mstm)
{
struct nouveau_drm *drm = nouveau_drm(mstm->outp->base.base.dev);
struct drm_encoder *encoder;
- int ret;
NV_ATOMIC(drm, "%s: mstm cleanup\n", mstm->outp->base.base.name);
- ret = drm_dp_check_act_status(>mgr);
+ drm_dp_check_act_status(>mgr);
- ret = drm_dp_update_payload_part2(>mgr);
+ drm_dp_update_payload_part2(>mgr);
drm_for_each_encoder(encoder, mstm->outp->base.base.dev) {
if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST) {
@@ -1410,10 +1409,9 @@ nv50_mstm_prepare(struct nv50_mstm *mstm)
{
struct nouveau_drm *drm = nouveau_drm(mstm->outp->base.base.dev);
struct drm_encoder *encoder;
- int ret;
NV_ATOMIC(drm, "%s: mstm prepare\n", mstm->outp->base.base.name);
- ret = drm_dp_update_payload_part1(>mgr);
+ drm_dp_update_payload_part1(>mgr);
drm_for_each_encoder(encoder, mstm->outp->base.base.dev) {
if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST) {
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: nouveau: nouveau_bo.c: Remove set but unused variables
Removed two variables set but unused.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/nouveau/nouveau_bo.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 2d5d68fc15c2..d0eac5375533 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1247,7 +1247,6 @@ nouveau_ttm_tt_populate(struct ttm_device *bdev,
{
struct ttm_tt *ttm_dma = (void *)ttm;
struct nouveau_drm *drm;
- struct device *dev;
bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
if (ttm_tt_is_populated(ttm))
@@ -1260,7 +1259,6 @@ nouveau_ttm_tt_populate(struct ttm_device *bdev,
}
drm = nouveau_bdev(bdev);
- dev = drm->dev->dev;
return ttm_pool_alloc(>ttm.bdev.pool, ttm, ctx);
}
@@ -1270,14 +1268,12 @@ nouveau_ttm_tt_unpopulate(struct ttm_device *bdev,
struct ttm_tt *ttm)
{
struct nouveau_drm *drm;
- struct device *dev;
bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
if (slave)
return;
drm = nouveau_bdev(bdev);
- dev = drm->dev->dev;
return ttm_pool_free(>ttm.bdev.pool, ttm);
}
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm: drm_atomic_helper.c: Correct comments format
Corrected comments format in accordance to the Linux style guides.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: Rewrote the "Subject" of the patches in the series
drivers/gpu/drm/drm_atomic_helper.c | 32 +++--
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index cd748ff61162..bc3487964fb5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1018,8 +1018,10 @@ disable_outputs(struct drm_device *dev, struct
drm_atomic_state *old_state)
struct drm_encoder *encoder;
struct drm_bridge *bridge;
- /* Shut down everything that's in the changeset and currently
-* still on. So need to check the old, saved state. */
+ /*
+* Shut down everything that's in the changeset and currently
+* still on. So need to check the old, saved state.
+*/
if (!old_conn_state->crtc)
continue;
@@ -1409,7 +1411,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
* @dev: DRM device
* @state: atomic state object with old state structures
* @pre_swap: If true, do an interruptible wait, and @state is the new state.
- * Otherwise @state is the old state.
+ * Otherwise @state is the old state.
*
* For implicit sync, driver should fish the exclusive fence out from the
* incoming fb's and stash it in the drm_plane_state. This is called after
@@ -1953,8 +1955,10 @@ static int stall_checks(struct drm_crtc *crtc, bool
nonblock)
list_for_each_entry(commit, >commit_list, commit_entry) {
if (i == 0) {
completed = try_wait_for_completion(>flip_done);
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (!completed && nonblock) {
spin_unlock(>commit_lock);
DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a
previous commit\n",
@@ -2103,9 +2107,11 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
if (ret)
return ret;
- /* Drivers only send out events when at least either current or
+ /*
+* Drivers only send out events when at least either current or
* new CRTC state is active. Complete right away if everything
-* stays off. */
+* stays off.
+*/
if (!old_crtc_state->active && !new_crtc_state->active) {
complete_all(>flip_done);
continue;
@@ -2137,8 +2143,10 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
}
for_each_oldnew_connector_in_state(state, conn, old_conn_state,
new_conn_state, i) {
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (nonblock && old_conn_state->commit &&
!try_wait_for_completion(_conn_state->commit->flip_done)) {
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a
previous commit\n",
@@ -2156,8 +2164,10 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
}
for_each_oldnew_plane_in_state(state, plane, old_plane_state,
new_plane_state, i) {
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (nonblock && old_plane_state->commit &&
!try_wait_for_completion(_plane_state->commit->flip_done)) {
DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous
commit\n",
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] drm: drm_atomic_helper.c: Replace "unsigned" with "unsigned int"
Replaced "unsigned with "unsigned int" since the latter is preferred.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: Rewrote the "Subject" of the patches in the series
drivers/gpu/drm/drm_atomic_helper.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index f2b3e28d938b..cd748ff61162 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -106,7 +106,7 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct drm_encoder *encoder;
- unsigned encoder_mask = 0;
+ unsigned int encoder_mask = 0;
int i, ret = 0;
/*
@@ -609,7 +609,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
struct drm_connector *connector;
struct drm_connector_state *old_connector_state, *new_connector_state;
int i, ret;
- unsigned connectors_mask = 0;
+ unsigned int connectors_mask = 0;
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
bool has_connectors =
@@ -1478,7 +1478,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
int i, ret;
- unsigned crtc_mask = 0;
+ unsigned int crtc_mask = 0;
/*
* Legacy cursor ioctls are completely unsynced, and userspace
@@ -2575,7 +2575,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
drm_crtc_state *old_crtc_state)
struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(old_state, crtc);
struct drm_plane *plane;
- unsigned plane_mask;
+ unsigned int plane_mask;
plane_mask = old_crtc_state->plane_mask;
plane_mask |= new_crtc_state->plane_mask;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/2] drm: drm_atomic_helper.c: Change types and format comments
Replaced "unsigned" with "unsigned int" and formatted comments according to the Linux style guidelines. Issues detected by checkpatch.pl. Changes from v1: Rewrote the "Subject" of the patches in the series. Fabio M. De Francesco (2): gpu: drm: Replace "unsigned" with "unsigned int" gpu: drm: Correct comments format drivers/gpu/drm/drm_atomic_helper.c | 40 ++--- 1 file changed, 25 insertions(+), 15 deletions(-) -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] gpu: drm: Replace bare "unsigned" with "unsigned int"
On Monday, April 12, 2021 2:11:59 PM CEST Daniel Vetter wrote:
> On Mon, Apr 12, 2021 at 12:53:09PM +0200, Fabio M. De Francesco wrote:
> > Replaced the type "unsigned" with "unsigned int" because it is
> > preferred. Issue detected by checkpatch.pl.
>
> Huh, I didn't know that, TIL.
>
> > Signed-off-by: Fabio M. De Francesco
>
> Thanks for your patche, merged to drm-misc-next for 5.14.
> -Daniel
>
I am happy that my first dri-devel patch has been accepted.
Thanks,
Fabio
>
> > ---
> >
> > drivers/gpu/drm/drm_atomic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > index 5b4547e0f775..46dceb51c90f 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1302,8 +1302,8 @@ int drm_atomic_check_only(struct drm_atomic_state
> > *state)>
> > struct drm_crtc_state *new_crtc_state;
> > struct drm_connector *conn;
> > struct drm_connector_state *conn_state;
> >
> > - unsigned requested_crtc = 0;
> > - unsigned affected_crtc = 0;
> > + unsigned int requested_crtc = 0;
> > + unsigned int affected_crtc = 0;
> >
> > int i, ret = 0;
> >
> > DRM_DEBUG_ATOMIC("checking %p\n", state);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] gpu: drm: Correct comments format
Corrected comments format in accordance to the Linux style guides.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/drm_atomic_helper.c | 32 +++--
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index cd748ff61162..bc3487964fb5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1018,8 +1018,10 @@ disable_outputs(struct drm_device *dev, struct
drm_atomic_state *old_state)
struct drm_encoder *encoder;
struct drm_bridge *bridge;
- /* Shut down everything that's in the changeset and currently
-* still on. So need to check the old, saved state. */
+ /*
+* Shut down everything that's in the changeset and currently
+* still on. So need to check the old, saved state.
+*/
if (!old_conn_state->crtc)
continue;
@@ -1409,7 +1411,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
* @dev: DRM device
* @state: atomic state object with old state structures
* @pre_swap: If true, do an interruptible wait, and @state is the new state.
- * Otherwise @state is the old state.
+ * Otherwise @state is the old state.
*
* For implicit sync, driver should fish the exclusive fence out from the
* incoming fb's and stash it in the drm_plane_state. This is called after
@@ -1953,8 +1955,10 @@ static int stall_checks(struct drm_crtc *crtc, bool
nonblock)
list_for_each_entry(commit, >commit_list, commit_entry) {
if (i == 0) {
completed = try_wait_for_completion(>flip_done);
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (!completed && nonblock) {
spin_unlock(>commit_lock);
DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a
previous commit\n",
@@ -2103,9 +2107,11 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
if (ret)
return ret;
- /* Drivers only send out events when at least either current or
+ /*
+* Drivers only send out events when at least either current or
* new CRTC state is active. Complete right away if everything
-* stays off. */
+* stays off.
+*/
if (!old_crtc_state->active && !new_crtc_state->active) {
complete_all(>flip_done);
continue;
@@ -2137,8 +2143,10 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
}
for_each_oldnew_connector_in_state(state, conn, old_conn_state,
new_conn_state, i) {
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (nonblock && old_conn_state->commit &&
!try_wait_for_completion(_conn_state->commit->flip_done)) {
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a
previous commit\n",
@@ -2156,8 +2164,10 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
}
for_each_oldnew_plane_in_state(state, plane, old_plane_state,
new_plane_state, i) {
- /* Userspace is not allowed to get ahead of the previous
-* commit with nonblocking ones. */
+ /*
+* Userspace is not allowed to get ahead of the previous
+* commit with nonblocking ones.
+*/
if (nonblock && old_plane_state->commit &&
!try_wait_for_completion(_plane_state->commit->flip_done)) {
DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous
commit\n",
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] gpu: drm: Change types and format comments
Replaced "unsigned" with "unsigned int" and formatted comments according to the Linux style guidelines. Issues detected by checkpatch.pl. Fabio M. De Francesco (2): gpu: drm: Replace "unsigned" with "unsigned int" gpu: drm: Correct comments format drivers/gpu/drm/drm_atomic_helper.c | 40 ++--- 1 file changed, 25 insertions(+), 15 deletions(-) -- 2.31.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] gpu: drm: Replace "unsigned" with "unsigned int"
Replaced "unsigned with "unsigned int" since the latter is preferred.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/drm_atomic_helper.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index f2b3e28d938b..cd748ff61162 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -106,7 +106,7 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct drm_encoder *encoder;
- unsigned encoder_mask = 0;
+ unsigned int encoder_mask = 0;
int i, ret = 0;
/*
@@ -609,7 +609,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
struct drm_connector *connector;
struct drm_connector_state *old_connector_state, *new_connector_state;
int i, ret;
- unsigned connectors_mask = 0;
+ unsigned int connectors_mask = 0;
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
bool has_connectors =
@@ -1478,7 +1478,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
int i, ret;
- unsigned crtc_mask = 0;
+ unsigned int crtc_mask = 0;
/*
* Legacy cursor ioctls are completely unsynced, and userspace
@@ -2575,7 +2575,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
drm_crtc_state *old_crtc_state)
struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(old_state, crtc);
struct drm_plane *plane;
- unsigned plane_mask;
+ unsigned int plane_mask;
plane_mask = old_crtc_state->plane_mask;
plane_mask |= new_crtc_state->plane_mask;
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] gpu: drm: Replace bare "unsigned" with "unsigned int"
Replaced the type "unsigned" with "unsigned int" because it is
preferred. Issue detected by checkpatch.pl.
Signed-off-by: Fabio M. De Francesco
---
drivers/gpu/drm/drm_atomic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5b4547e0f775..46dceb51c90f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1302,8 +1302,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
struct drm_crtc_state *new_crtc_state;
struct drm_connector *conn;
struct drm_connector_state *conn_state;
- unsigned requested_crtc = 0;
- unsigned affected_crtc = 0;
+ unsigned int requested_crtc = 0;
+ unsigned int affected_crtc = 0;
int i, ret = 0;
DRM_DEBUG_ATOMIC("checking %p\n", state);
--
2.31.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
