* [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel
@ 2024-03-25 10:41 Boris Brezillon
2024-03-25 10:41 ` [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path Boris Brezillon
2024-03-25 11:12 ` [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Steven Price
0 siblings, 2 replies; 7+ messages in thread
From: Boris Brezillon @ 2024-03-25 10:41 UTC (permalink / raw
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, kernel, Lukas F . Hartmann
When mapping an IO region, the pseudo-file offset is dependent on the
userspace architecture. panthor_device_mmio_offset() abstract that away
for us by turning a userspace MMIO offset into its kernel equivalent,
but we were not updating vm_area_struct::vm_pgoff accordingly, leading
us to attach the MMIO region to the wrong file offset.
This has implications when we start mixing 64 bit and 32 bit apps, but
that's only really a problem when we start having more that 2^43 bytes of
memory allocated, which is very unlikely to happen.
What's more problematic is the fact this turns our
unmap_mapping_range(DRM_PANTHOR_USER_MMIO_OFFSET) calls, which are
supposed to kill the MMIO mapping when entering suspend, into NOPs.
Which means we either keep the dummy flush_id mapping active at all
times, or we risk a BUS_FAULT if the MMIO region was mapped, and the
GPU is suspended after that.
Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
Reported-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reported-by: Lukas F. Hartmann <lukas@mntmn.com>
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/10835
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/gpu/drm/panthor/panthor_device.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index bfe8da4a6e4c..a18fd4e4b77c 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -334,7 +334,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
{
struct vm_area_struct *vma = vmf->vma;
struct panthor_device *ptdev = vma->vm_private_data;
- u64 id = (u64)vma->vm_pgoff << PAGE_SHIFT;
+ u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
unsigned long pfn;
pgprot_t pgprot;
vm_fault_t ret;
@@ -347,7 +347,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
mutex_lock(&ptdev->pm.mmio_lock);
active = atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE;
- switch (panthor_device_mmio_offset(id)) {
+ switch (offset) {
case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
if (active)
pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
@@ -378,9 +378,9 @@ static const struct vm_operations_struct panthor_mmio_vm_ops = {
int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *vma)
{
- u64 id = (u64)vma->vm_pgoff << PAGE_SHIFT;
+ u64 offset = panthor_device_mmio_offset((u64)vma->vm_pgoff << PAGE_SHIFT);
- switch (panthor_device_mmio_offset(id)) {
+ switch (offset) {
case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
if (vma->vm_end - vma->vm_start != PAGE_SIZE ||
(vma->vm_flags & (VM_WRITE | VM_EXEC)))
@@ -392,6 +392,9 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
return -EINVAL;
}
+ /* Adjust vm_pgoff for 32-bit userspace on 64-bit kernel. */
+ vma->vm_pgoff = offset >> PAGE_SHIFT;
+
/* Defer actual mapping to the fault handler. */
vma->vm_private_data = ptdev;
vma->vm_ops = &panthor_mmio_vm_ops;
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path
2024-03-25 10:41 [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Boris Brezillon
@ 2024-03-25 10:41 ` Boris Brezillon
2024-03-25 11:17 ` Steven Price
2024-03-25 11:12 ` [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Steven Price
1 sibling, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2024-03-25 10:41 UTC (permalink / raw
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, kernel
panthor_xxx_irq_suspend() doesn't mask the interrupts if drm_dev_unplug()
has been called, which is always the case when our panthor_xxx_unplug()
helpers are called. Fix that by introducing a panthor_xxx_unplug() helper
that does what panthor_xxx_irq_suspend() except it does it
unconditionally.
Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Found inadvertently while debugging another issue. I guess I managed to
call rmmod during a PING and that led to the FW interrupt handler
being executed after the device suspend happened.
---
drivers/gpu/drm/panthor/panthor_device.h | 8 ++++++++
drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
drivers/gpu/drm/panthor/panthor_mmu.c | 2 +-
4 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 51c9d61b6796..ba43d5ea4e96 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -321,6 +321,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
return ret; \
} \
\
+static inline void panthor_ ## __name ## _irq_unplug(struct panthor_irq *pirq) \
+{ \
+ pirq->mask = 0; \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
+ synchronize_irq(pirq->irq); \
+ atomic_set(&pirq->suspended, true); \
+} \
+ \
static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
{ \
int cookie; \
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 33c87a59834e..7a9710a38c5f 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -1128,7 +1128,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
/* Make sure the IRQ handler can be called after that point. */
if (ptdev->fw->irq.irq)
- panthor_job_irq_suspend(&ptdev->fw->irq);
+ panthor_job_irq_unplug(&ptdev->fw->irq);
panthor_fw_stop(ptdev);
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index 6dbbc4cfbe7e..b84c5b650fd9 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -174,7 +174,7 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
unsigned long flags;
/* Make sure the IRQ handler is not running after that point. */
- panthor_gpu_irq_suspend(&ptdev->gpu->irq);
+ panthor_gpu_irq_unplug(&ptdev->gpu->irq);
/* Wake-up all waiters. */
spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index fdd35249169f..1f333cdded0f 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -2622,7 +2622,7 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
*/
void panthor_mmu_unplug(struct panthor_device *ptdev)
{
- panthor_mmu_irq_suspend(&ptdev->mmu->irq);
+ panthor_mmu_irq_unplug(&ptdev->mmu->irq);
mutex_lock(&ptdev->mmu->as.slots_lock);
for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel
2024-03-25 10:41 [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Boris Brezillon
2024-03-25 10:41 ` [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path Boris Brezillon
@ 2024-03-25 11:12 ` Steven Price
2024-03-25 11:24 ` Boris Brezillon
1 sibling, 1 reply; 7+ messages in thread
From: Steven Price @ 2024-03-25 11:12 UTC (permalink / raw
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, kernel, Lukas F . Hartmann
On 25/03/2024 10:41, Boris Brezillon wrote:
> When mapping an IO region, the pseudo-file offset is dependent on the
> userspace architecture. panthor_device_mmio_offset() abstract that away
> for us by turning a userspace MMIO offset into its kernel equivalent,
> but we were not updating vm_area_struct::vm_pgoff accordingly, leading
> us to attach the MMIO region to the wrong file offset.
>
> This has implications when we start mixing 64 bit and 32 bit apps, but
> that's only really a problem when we start having more that 2^43 bytes of
> memory allocated, which is very unlikely to happen.
>
> What's more problematic is the fact this turns our
> unmap_mapping_range(DRM_PANTHOR_USER_MMIO_OFFSET) calls, which are
> supposed to kill the MMIO mapping when entering suspend, into NOPs.
> Which means we either keep the dummy flush_id mapping active at all
> times, or we risk a BUS_FAULT if the MMIO region was mapped, and the
> GPU is suspended after that.
>
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Reported-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Reported-by: Lukas F. Hartmann <lukas@mntmn.com>
> Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/10835
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Pesky 32 bit again ;)
Looks fine, although I'm wondering whether you'd consider squashing
something like the below on top? I think it helps contain the 32 bit
specific code to the one place. Either way:
Reviewed-by: Steven Price <steven.price@arm.com>
---
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index f7f8184b1992..75276cbeba20 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -392,7 +392,7 @@ static const struct vm_operations_struct panthor_mmio_vm_ops = {
int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *vma)
{
- u64 offset = panthor_device_mmio_offset((u64)vma->vm_pgoff << PAGE_SHIFT);
+ u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
switch (offset) {
case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
@@ -406,9 +406,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
return -EINVAL;
}
- /* Adjust vm_pgoff for 32-bit userspace on 64-bit kernel. */
- vma->vm_pgoff = offset >> PAGE_SHIFT;
-
/* Defer actual mapping to the fault handler. */
vma->vm_private_data = ptdev;
vma->vm_ops = &panthor_mmio_vm_ops;
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 8e2922c79aca..99ad576912b3 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -373,30 +373,6 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
pirq); \
}
-/**
- * panthor_device_mmio_offset() - Turn a user MMIO offset into a kernel one
- * @offset: Offset to convert.
- *
- * With 32-bit systems being limited by the 32-bit representation of mmap2's
- * pgoffset field, we need to make the MMIO offset arch specific. This function
- * converts a user MMIO offset into something the kernel driver understands.
- *
- * If the kernel and userspace architecture match, the offset is unchanged. If
- * the kernel is 64-bit and userspace is 32-bit, the offset is adjusted to match
- * 64-bit offsets. 32-bit kernel with 64-bit userspace is impossible.
- *
- * Return: Adjusted offset.
- */
-static inline u64 panthor_device_mmio_offset(u64 offset)
-{
-#ifdef CONFIG_ARM64
- if (test_tsk_thread_flag(current, TIF_32BIT))
- offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT - DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
-#endif
-
- return offset;
-}
-
extern struct workqueue_struct *panthor_cleanup_wq;
#endif
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 11b3ccd58f85..730dd0c69cb8 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1327,7 +1327,22 @@ static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
if (!drm_dev_enter(file->minor->dev, &cookie))
return -ENODEV;
- if (panthor_device_mmio_offset(offset) >= DRM_PANTHOR_USER_MMIO_OFFSET)
+#ifdef CONFIG_ARM64
+ /*
+ * With 32-bit systems being limited by the 32-bit representation of
+ * mmap2's pgoffset field, we need to make the MMIO offset arch
+ * specific. This converts a user MMIO offset into something the kernel
+ * driver understands.
+ */
+ if (test_tsk_thread_flag(current, TIF_32BIT) &&
+ offset >= DRM_PANTHOR_USER_MMIO_OFFSET_32BIT) {
+ offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT -
+ DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
+ vma->vm_pgoff = offset >> PAGE_SHIFT;
+ }
+#endif
+
+ if (offset >= DRM_PANTHOR_USER_MMIO_OFFSET)
ret = panthor_device_mmap_io(ptdev, vma);
else
ret = drm_gem_mmap(filp, vma);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path
2024-03-25 10:41 ` [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path Boris Brezillon
@ 2024-03-25 11:17 ` Steven Price
2024-03-25 11:43 ` Boris Brezillon
0 siblings, 1 reply; 7+ messages in thread
From: Steven Price @ 2024-03-25 11:17 UTC (permalink / raw
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe; +Cc: dri-devel, kernel
On 25/03/2024 10:41, Boris Brezillon wrote:
> panthor_xxx_irq_suspend() doesn't mask the interrupts if drm_dev_unplug()
> has been called, which is always the case when our panthor_xxx_unplug()
> helpers are called. Fix that by introducing a panthor_xxx_unplug() helper
> that does what panthor_xxx_irq_suspend() except it does it
> unconditionally.
>
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Found inadvertently while debugging another issue. I guess I managed to
> call rmmod during a PING and that led to the FW interrupt handler
> being executed after the device suspend happened.
> ---
> drivers/gpu/drm/panthor/panthor_device.h | 8 ++++++++
> drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
> drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
> drivers/gpu/drm/panthor/panthor_mmu.c | 2 +-
> 4 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..ba43d5ea4e96 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -321,6 +321,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> return ret; \
> } \
> \
> +static inline void panthor_ ## __name ## _irq_unplug(struct panthor_irq *pirq) \
> +{ \
> + pirq->mask = 0; \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> + synchronize_irq(pirq->irq); \
> + atomic_set(&pirq->suspended, true); \
> +} \
> + \
This does things in a different order to _irq_suspend, is there a good
reason?
I'd expect:
{
atomic_set(&pirq->suspended, true);
gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);
synchronize_irq(pirq->irq);
pirq->mask = 0;
}
In particular I'm wondering if having the atomic_set after
synchronize_irq() could cause problems with an irq handler changing the
INT_MASK again (although AFAICT it should end up setting it to 0).
Otherwise this change looks good.
Thanks,
Steve
> static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> { \
> int cookie; \
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 33c87a59834e..7a9710a38c5f 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1128,7 +1128,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>
> /* Make sure the IRQ handler can be called after that point. */
> if (ptdev->fw->irq.irq)
> - panthor_job_irq_suspend(&ptdev->fw->irq);
> + panthor_job_irq_unplug(&ptdev->fw->irq);
>
> panthor_fw_stop(ptdev);
>
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 6dbbc4cfbe7e..b84c5b650fd9 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -174,7 +174,7 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
> unsigned long flags;
>
> /* Make sure the IRQ handler is not running after that point. */
> - panthor_gpu_irq_suspend(&ptdev->gpu->irq);
> + panthor_gpu_irq_unplug(&ptdev->gpu->irq);
>
> /* Wake-up all waiters. */
> spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index fdd35249169f..1f333cdded0f 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2622,7 +2622,7 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
> */
> void panthor_mmu_unplug(struct panthor_device *ptdev)
> {
> - panthor_mmu_irq_suspend(&ptdev->mmu->irq);
> + panthor_mmu_irq_unplug(&ptdev->mmu->irq);
>
> mutex_lock(&ptdev->mmu->as.slots_lock);
> for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel
2024-03-25 11:12 ` [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Steven Price
@ 2024-03-25 11:24 ` Boris Brezillon
0 siblings, 0 replies; 7+ messages in thread
From: Boris Brezillon @ 2024-03-25 11:24 UTC (permalink / raw
To: Steven Price
Cc: Liviu Dudau, Adrián Larumbe, dri-devel, kernel,
Lukas F . Hartmann
On Mon, 25 Mar 2024 11:12:40 +0000
Steven Price <steven.price@arm.com> wrote:
> On 25/03/2024 10:41, Boris Brezillon wrote:
> > When mapping an IO region, the pseudo-file offset is dependent on the
> > userspace architecture. panthor_device_mmio_offset() abstract that away
> > for us by turning a userspace MMIO offset into its kernel equivalent,
> > but we were not updating vm_area_struct::vm_pgoff accordingly, leading
> > us to attach the MMIO region to the wrong file offset.
> >
> > This has implications when we start mixing 64 bit and 32 bit apps, but
> > that's only really a problem when we start having more that 2^43 bytes of
> > memory allocated, which is very unlikely to happen.
> >
> > What's more problematic is the fact this turns our
> > unmap_mapping_range(DRM_PANTHOR_USER_MMIO_OFFSET) calls, which are
> > supposed to kill the MMIO mapping when entering suspend, into NOPs.
> > Which means we either keep the dummy flush_id mapping active at all
> > times, or we risk a BUS_FAULT if the MMIO region was mapped, and the
> > GPU is suspended after that.
> >
> > Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> > Reported-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Reported-by: Lukas F. Hartmann <lukas@mntmn.com>
> > Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/10835
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>
> Pesky 32 bit again ;)
>
> Looks fine, although I'm wondering whether you'd consider squashing
> something like the below on top? I think it helps contain the 32 bit
> specific code to the one place.
I like that. I'll steal your changes for v2 and update the commit
message to reflect the fact we no longer need this
panthor_device_mmio_offset() helper.
> Either way:
>
> Reviewed-by: Steven Price <steven.price@arm.com>
>
> ---
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index f7f8184b1992..75276cbeba20 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -392,7 +392,7 @@ static const struct vm_operations_struct panthor_mmio_vm_ops = {
>
> int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *vma)
> {
> - u64 offset = panthor_device_mmio_offset((u64)vma->vm_pgoff << PAGE_SHIFT);
> + u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
>
> switch (offset) {
> case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
> @@ -406,9 +406,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
> return -EINVAL;
> }
>
> - /* Adjust vm_pgoff for 32-bit userspace on 64-bit kernel. */
> - vma->vm_pgoff = offset >> PAGE_SHIFT;
> -
> /* Defer actual mapping to the fault handler. */
> vma->vm_private_data = ptdev;
> vma->vm_ops = &panthor_mmio_vm_ops;
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 8e2922c79aca..99ad576912b3 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -373,30 +373,6 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
> pirq); \
> }
>
> -/**
> - * panthor_device_mmio_offset() - Turn a user MMIO offset into a kernel one
> - * @offset: Offset to convert.
> - *
> - * With 32-bit systems being limited by the 32-bit representation of mmap2's
> - * pgoffset field, we need to make the MMIO offset arch specific. This function
> - * converts a user MMIO offset into something the kernel driver understands.
> - *
> - * If the kernel and userspace architecture match, the offset is unchanged. If
> - * the kernel is 64-bit and userspace is 32-bit, the offset is adjusted to match
> - * 64-bit offsets. 32-bit kernel with 64-bit userspace is impossible.
> - *
> - * Return: Adjusted offset.
> - */
> -static inline u64 panthor_device_mmio_offset(u64 offset)
> -{
> -#ifdef CONFIG_ARM64
> - if (test_tsk_thread_flag(current, TIF_32BIT))
> - offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT - DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
> -#endif
> -
> - return offset;
> -}
> -
> extern struct workqueue_struct *panthor_cleanup_wq;
>
> #endif
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 11b3ccd58f85..730dd0c69cb8 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1327,7 +1327,22 @@ static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
> if (!drm_dev_enter(file->minor->dev, &cookie))
> return -ENODEV;
>
> - if (panthor_device_mmio_offset(offset) >= DRM_PANTHOR_USER_MMIO_OFFSET)
> +#ifdef CONFIG_ARM64
> + /*
> + * With 32-bit systems being limited by the 32-bit representation of
> + * mmap2's pgoffset field, we need to make the MMIO offset arch
> + * specific. This converts a user MMIO offset into something the kernel
> + * driver understands.
> + */
> + if (test_tsk_thread_flag(current, TIF_32BIT) &&
> + offset >= DRM_PANTHOR_USER_MMIO_OFFSET_32BIT) {
> + offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT -
> + DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
> + vma->vm_pgoff = offset >> PAGE_SHIFT;
> + }
> +#endif
> +
> + if (offset >= DRM_PANTHOR_USER_MMIO_OFFSET)
> ret = panthor_device_mmap_io(ptdev, vma);
> else
> ret = drm_gem_mmap(filp, vma);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path
2024-03-25 11:17 ` Steven Price
@ 2024-03-25 11:43 ` Boris Brezillon
2024-03-25 12:00 ` Steven Price
0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2024-03-25 11:43 UTC (permalink / raw
To: Steven Price; +Cc: Liviu Dudau, Adrián Larumbe, dri-devel, kernel
On Mon, 25 Mar 2024 11:17:24 +0000
Steven Price <steven.price@arm.com> wrote:
> On 25/03/2024 10:41, Boris Brezillon wrote:
> > panthor_xxx_irq_suspend() doesn't mask the interrupts if drm_dev_unplug()
> > has been called, which is always the case when our panthor_xxx_unplug()
> > helpers are called. Fix that by introducing a panthor_xxx_unplug() helper
> > that does what panthor_xxx_irq_suspend() except it does it
> > unconditionally.
> >
> > Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Found inadvertently while debugging another issue. I guess I managed to
> > call rmmod during a PING and that led to the FW interrupt handler
> > being executed after the device suspend happened.
> > ---
> > drivers/gpu/drm/panthor/panthor_device.h | 8 ++++++++
> > drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
> > drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
> > drivers/gpu/drm/panthor/panthor_mmu.c | 2 +-
> > 4 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index 51c9d61b6796..ba43d5ea4e96 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -321,6 +321,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> > return ret; \
> > } \
> > \
> > +static inline void panthor_ ## __name ## _irq_unplug(struct panthor_irq *pirq) \
> > +{ \
> > + pirq->mask = 0; \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> > + synchronize_irq(pirq->irq); \
> > + atomic_set(&pirq->suspended, true); \
> > +} \
> > + \
>
> This does things in a different order to _irq_suspend, is there a good
> reason?
> I'd expect:
>
> {
> atomic_set(&pirq->suspended, true);
> gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);
> synchronize_irq(pirq->irq);
> pirq->mask = 0;
> }
>
> In particular I'm wondering if having the atomic_set after
> synchronize_irq() could cause problems with an irq handler changing the
> INT_MASK again (although AFAICT it should end up setting it to 0).
Hm, now that you mention it, I'm wondering if the ordering in
_irq_suspend() is not problematic actually. If we set suspended=true
before anything else in the __irq_suspend() path, and just after than,
an interrupt kicks is. In that case, the hard irq handler will return
IRQ_NONE even though the irqs are not masked (_INT_MASK not zero), which
might lead to an interrupt flood (the interrupt is neither processed nor
masked), which is probably recoverable on a multi-core system
(_irq_suspend() should end up masking the interrupts at some point), but
still not an ideal situation.
Masking the interrupts, synchronizing, and finally flagging the IRQ as
suspended sounds safer for both the suspend and unplug cases. What do
you think?
>
> Otherwise this change looks good.
>
> Thanks,
>
> Steve
>
> > static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> > { \
> > int cookie; \
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index 33c87a59834e..7a9710a38c5f 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -1128,7 +1128,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> >
> > /* Make sure the IRQ handler can be called after that point. */
> > if (ptdev->fw->irq.irq)
> > - panthor_job_irq_suspend(&ptdev->fw->irq);
> > + panthor_job_irq_unplug(&ptdev->fw->irq);
> >
> > panthor_fw_stop(ptdev);
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> > index 6dbbc4cfbe7e..b84c5b650fd9 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> > @@ -174,7 +174,7 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
> > unsigned long flags;
> >
> > /* Make sure the IRQ handler is not running after that point. */
> > - panthor_gpu_irq_suspend(&ptdev->gpu->irq);
> > + panthor_gpu_irq_unplug(&ptdev->gpu->irq);
> >
> > /* Wake-up all waiters. */
> > spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index fdd35249169f..1f333cdded0f 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -2622,7 +2622,7 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
> > */
> > void panthor_mmu_unplug(struct panthor_device *ptdev)
> > {
> > - panthor_mmu_irq_suspend(&ptdev->mmu->irq);
> > + panthor_mmu_irq_unplug(&ptdev->mmu->irq);
> >
> > mutex_lock(&ptdev->mmu->as.slots_lock);
> > for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path
2024-03-25 11:43 ` Boris Brezillon
@ 2024-03-25 12:00 ` Steven Price
0 siblings, 0 replies; 7+ messages in thread
From: Steven Price @ 2024-03-25 12:00 UTC (permalink / raw
To: Boris Brezillon; +Cc: Liviu Dudau, Adrián Larumbe, dri-devel, kernel
On 25/03/2024 11:43, Boris Brezillon wrote:
> On Mon, 25 Mar 2024 11:17:24 +0000
> Steven Price <steven.price@arm.com> wrote:
>
>> On 25/03/2024 10:41, Boris Brezillon wrote:
>>> panthor_xxx_irq_suspend() doesn't mask the interrupts if drm_dev_unplug()
>>> has been called, which is always the case when our panthor_xxx_unplug()
>>> helpers are called. Fix that by introducing a panthor_xxx_unplug() helper
>>> that does what panthor_xxx_irq_suspend() except it does it
>>> unconditionally.
>>>
>>> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>> ---
>>> Found inadvertently while debugging another issue. I guess I managed to
>>> call rmmod during a PING and that led to the FW interrupt handler
>>> being executed after the device suspend happened.
>>> ---
>>> drivers/gpu/drm/panthor/panthor_device.h | 8 ++++++++
>>> drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
>>> drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
>>> drivers/gpu/drm/panthor/panthor_mmu.c | 2 +-
>>> 4 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
>>> index 51c9d61b6796..ba43d5ea4e96 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_device.h
>>> +++ b/drivers/gpu/drm/panthor/panthor_device.h
>>> @@ -321,6 +321,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
>>> return ret; \
>>> } \
>>> \
>>> +static inline void panthor_ ## __name ## _irq_unplug(struct panthor_irq *pirq) \
>>> +{ \
>>> + pirq->mask = 0; \
>>> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
>>> + synchronize_irq(pirq->irq); \
>>> + atomic_set(&pirq->suspended, true); \
>>> +} \
>>> + \
>>
>> This does things in a different order to _irq_suspend, is there a good
>> reason?
>> I'd expect:
>>
>> {
>> atomic_set(&pirq->suspended, true);
>> gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);
>> synchronize_irq(pirq->irq);
>> pirq->mask = 0;
>> }
>>
>> In particular I'm wondering if having the atomic_set after
>> synchronize_irq() could cause problems with an irq handler changing the
>> INT_MASK again (although AFAICT it should end up setting it to 0).
>
> Hm, now that you mention it, I'm wondering if the ordering in
> _irq_suspend() is not problematic actually. If we set suspended=true
> before anything else in the __irq_suspend() path, and just after than,
> an interrupt kicks is. In that case, the hard irq handler will return
> IRQ_NONE even though the irqs are not masked (_INT_MASK not zero), which
> might lead to an interrupt flood (the interrupt is neither processed nor
> masked), which is probably recoverable on a multi-core system
> (_irq_suspend() should end up masking the interrupts at some point), but
> still not an ideal situation.
>
> Masking the interrupts, synchronizing, and finally flagging the IRQ as
> suspended sounds safer for both the suspend and unplug cases. What do
> you think?
I hadn't scrolled up to look at the raw handler - but now you mention
it... Yes setting suspended to true before clearing INT_MASK is clearly
a bug and as you say in theory could lead to an interrupt storm.
So it's the suspend case that needs fixing not your new unplug one. Well
at least flagging up the difference uncovered a different bug ;)
Thanks,
Steve
>>
>> Otherwise this change looks good.
>>
>> Thanks,
>>
>> Steve
>>
>>> static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
>>> { \
>>> int cookie; \
>>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>>> index 33c87a59834e..7a9710a38c5f 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>>> @@ -1128,7 +1128,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>>>
>>> /* Make sure the IRQ handler can be called after that point. */
>>> if (ptdev->fw->irq.irq)
>>> - panthor_job_irq_suspend(&ptdev->fw->irq);
>>> + panthor_job_irq_unplug(&ptdev->fw->irq);
>>>
>>> panthor_fw_stop(ptdev);
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
>>> index 6dbbc4cfbe7e..b84c5b650fd9 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
>>> @@ -174,7 +174,7 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
>>> unsigned long flags;
>>>
>>> /* Make sure the IRQ handler is not running after that point. */
>>> - panthor_gpu_irq_suspend(&ptdev->gpu->irq);
>>> + panthor_gpu_irq_unplug(&ptdev->gpu->irq);
>>>
>>> /* Wake-up all waiters. */
>>> spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
>>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> index fdd35249169f..1f333cdded0f 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> @@ -2622,7 +2622,7 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
>>> */
>>> void panthor_mmu_unplug(struct panthor_device *ptdev)
>>> {
>>> - panthor_mmu_irq_suspend(&ptdev->mmu->irq);
>>> + panthor_mmu_irq_unplug(&ptdev->mmu->irq);
>>>
>>> mutex_lock(&ptdev->mmu->as.slots_lock);
>>> for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-25 12:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-25 10:41 [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Boris Brezillon
2024-03-25 10:41 ` [PATCH 2/2] drm/panthor: Actually suspend IRQs in the unplug path Boris Brezillon
2024-03-25 11:17 ` Steven Price
2024-03-25 11:43 ` Boris Brezillon
2024-03-25 12:00 ` Steven Price
2024-03-25 11:12 ` [PATCH 1/2] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel Steven Price
2024-03-25 11:24 ` Boris Brezillon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).