[PATCH v3 1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel

[PATCH v3 1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel

[Boris Brezillon]

When mapping an IO region, the pseudo-file offset is dependent on the
userspace architecture. panthor_device_mmio_offset() abstracts 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.

Solve that by patching vm_pgoff early in panthor_mmap(). With
this in place, we no longer need the panthor_device_mmio_offset()
helper.

v3:
- No changes

v2:
- Kill panthor_device_mmio_offset()

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>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.c |  8 ++++----
 drivers/gpu/drm/panthor/panthor_device.h | 24 ------------------------
 drivers/gpu/drm/panthor/panthor_drv.c    | 17 ++++++++++++++++-
 3 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index bfe8da4a6e4c..3ddc4ba0acbe 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 = (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)))
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 51c9d61b6796..7ee4987a3796 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -365,30 +365,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 ff484506229f..0097a01d0fc7 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);
-- 
2.44.0

[PATCH v3 2/3] drm/panthor: Fix ordering in _irq_suspend()

[Boris Brezillon]

Make sure we set suspended=true last to avoid generating an irq storm
in the unlikely case where an IRQ happens between the suspended=true
assignment and the _INT_MASK update.

We also move the mask=0 assignment before writing to the _INT_MASK
register to prevent the thread handler from unmasking the interrupt
behind our back. This means we might lose events if there were some
pending when we get to suspend the IRQ, but that's fine.
The synchronize_irq() we have in the _irq_suspend() path was not
there to make sure all IRQs are processed, just to make sure we don't
have registers accesses coming from the irq handlers after
_irq_suspend() has been called. If there's a need to have all pending
IRQs processed, it should happen before _irq_suspend() is called.

v3:
- Add Steve's R-b

v2:
- New patch

Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
Reported-by: Steven Price <steven.price@arm.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 7ee4987a3796..3a930a368ae1 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -325,7 +325,7 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
 {												\
 	int cookie;										\
 												\
-	atomic_set(&pirq->suspended, true);							\
+	pirq->mask = 0;										\
 												\
 	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
 		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
@@ -333,7 +333,7 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
 		drm_dev_exit(cookie);								\
 	}											\
 												\
-	pirq->mask = 0;										\
+	atomic_set(&pirq->suspended, true);							\
 }												\
 												\
 static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask)	\
-- 
2.44.0

[PATCH v3 3/3] drm/panthor: Drop the dev_enter/exit() sections in _irq_suspend/resume()

[Boris Brezillon]

There's no reason for _irq_suspend/resume() to be called after the
device has been unplugged, and keeping this dev_enter/exit()
section in _irq_suspend() is turns _irq_suspend() into a NOP
when called from the _unplug() functions, which we don't want.

v3:
- New patch

Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.h | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 3a930a368ae1..99ddc41f2626 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -326,13 +326,8 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
 	int cookie;										\
 												\
 	pirq->mask = 0;										\
-												\
-	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
-		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
-		synchronize_irq(pirq->irq);							\
-		drm_dev_exit(cookie);								\
-	}											\
-												\
+	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);					\
+	synchronize_irq(pirq->irq);								\
 	atomic_set(&pirq->suspended, true);							\
 }												\
 												\
@@ -342,12 +337,8 @@ static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u
 												\
 	atomic_set(&pirq->suspended, false);							\
 	pirq->mask = mask;									\
-												\
-	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
-		gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);			\
-		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);			\
-		drm_dev_exit(cookie);								\
-	}											\
+	gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);				\
+	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);				\
 }												\
 												\
 static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev,			\
-- 
2.44.0

Re: [PATCH v3 3/3] drm/panthor: Drop the dev_enter/exit() sections in _irq_suspend/resume()

[Boris Brezillon]

On Tue, 26 Mar 2024 12:12:05 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> There's no reason for _irq_suspend/resume() to be called after the
> device has been unplugged, and keeping this dev_enter/exit()
> section in _irq_suspend() is turns _irq_suspend() into a NOP

                            ^ s/is turns/turns/

> when called from the _unplug() functions, which we don't want.

Re: [PATCH v3 1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel

[Liviu Dudau]

On Tue, Mar 26, 2024 at 12:12:03PM +0100, Boris Brezillon wrote:
> When mapping an IO region, the pseudo-file offset is dependent on the
> userspace architecture. panthor_device_mmio_offset() abstracts 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.
> 
> Solve that by patching vm_pgoff early in panthor_mmap(). With
> this in place, we no longer need the panthor_device_mmio_offset()
> helper.
> 
> v3:
> - No changes
> 
> v2:
> - Kill panthor_device_mmio_offset()
> 
> 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>
> Reviewed-by: Steven Price <steven.price@arm.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  8 ++++----
>  drivers/gpu/drm/panthor/panthor_device.h | 24 ------------------------
>  drivers/gpu/drm/panthor/panthor_drv.c    | 17 ++++++++++++++++-
>  3 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index bfe8da4a6e4c..3ddc4ba0acbe 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 = (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)))
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..7ee4987a3796 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -365,30 +365,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 ff484506229f..0097a01d0fc7 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);
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v3 2/3] drm/panthor: Fix ordering in _irq_suspend()

[Liviu Dudau]

On Tue, Mar 26, 2024 at 12:12:04PM +0100, Boris Brezillon wrote:
> Make sure we set suspended=true last to avoid generating an irq storm
> in the unlikely case where an IRQ happens between the suspended=true
> assignment and the _INT_MASK update.
> 
> We also move the mask=0 assignment before writing to the _INT_MASK
> register to prevent the thread handler from unmasking the interrupt
> behind our back. This means we might lose events if there were some
> pending when we get to suspend the IRQ, but that's fine.
> The synchronize_irq() we have in the _irq_suspend() path was not
> there to make sure all IRQs are processed, just to make sure we don't
> have registers accesses coming from the irq handlers after
> _irq_suspend() has been called. If there's a need to have all pending
> IRQs processed, it should happen before _irq_suspend() is called.
> 
> v3:
> - Add Steve's R-b
> 
> v2:
> - New patch
> 
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Reported-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Steven Price <steven.price@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 7ee4987a3796..3a930a368ae1 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -325,7 +325,7 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
>  {												\
>  	int cookie;										\
>  												\
> -	atomic_set(&pirq->suspended, true);							\
> +	pirq->mask = 0;										\
>  												\
>  	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
>  		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
> @@ -333,7 +333,7 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
>  		drm_dev_exit(cookie);								\
>  	}											\
>  												\
> -	pirq->mask = 0;										\
> +	atomic_set(&pirq->suspended, true);							\
>  }												\
>  												\
>  static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask)	\
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v3 3/3] drm/panthor: Drop the dev_enter/exit() sections in _irq_suspend/resume()

[Liviu Dudau]

On Tue, Mar 26, 2024 at 12:12:05PM +0100, Boris Brezillon wrote:
> There's no reason for _irq_suspend/resume() to be called after the
> device has been unplugged, and keeping this dev_enter/exit()
> section in _irq_suspend() is turns _irq_suspend() into a NOP
> when called from the _unplug() functions, which we don't want.
> 
> v3:
> - New patch
> 
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.h | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 3a930a368ae1..99ddc41f2626 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -326,13 +326,8 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
>  	int cookie;										\
>  												\
>  	pirq->mask = 0;										\
> -												\
> -	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
> -		synchronize_irq(pirq->irq);							\
> -		drm_dev_exit(cookie);								\
> -	}											\
> -												\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);					\
> +	synchronize_irq(pirq->irq);								\
>  	atomic_set(&pirq->suspended, true);							\
>  }												\
>  												\
> @@ -342,12 +337,8 @@ static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u
>  												\
>  	atomic_set(&pirq->suspended, false);							\
>  	pirq->mask = mask;									\
> -												\
> -	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);			\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);			\
> -		drm_dev_exit(cookie);								\
> -	}											\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);				\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);				\
>  }												\
>  												\
>  static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev,			\
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v3 3/3] drm/panthor: Drop the dev_enter/exit() sections in _irq_suspend/resume()

[Steven Price]

On 26/03/2024 11:12, Boris Brezillon wrote:
> There's no reason for _irq_suspend/resume() to be called after the
> device has been unplugged, and keeping this dev_enter/exit()
> section in _irq_suspend() is turns _irq_suspend() into a NOP
> when called from the _unplug() functions, which we don't want.
> 
> v3:
> - New patch
> 
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

LGTM

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.h | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 3a930a368ae1..99ddc41f2626 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -326,13 +326,8 @@ static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)
>  	int cookie;										\
>  												\
>  	pirq->mask = 0;										\
> -												\
> -	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
> -		synchronize_irq(pirq->irq);							\
> -		drm_dev_exit(cookie);								\
> -	}											\
> -												\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);					\
> +	synchronize_irq(pirq->irq);								\
>  	atomic_set(&pirq->suspended, true);							\
>  }												\
>  												\
> @@ -342,12 +337,8 @@ static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u
>  												\
>  	atomic_set(&pirq->suspended, false);							\
>  	pirq->mask = mask;									\
> -												\
> -	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);			\
> -		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);			\
> -		drm_dev_exit(cookie);								\
> -	}											\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);				\
> +	gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);				\
>  }												\
>  												\
>  static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev,			\

Re: [PATCH v3 1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel

[Boris Brezillon]

On Tue, 26 Mar 2024 12:12:03 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> When mapping an IO region, the pseudo-file offset is dependent on the
> userspace architecture. panthor_device_mmio_offset() abstracts 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.
> 
> Solve that by patching vm_pgoff early in panthor_mmap(). With
> this in place, we no longer need the panthor_device_mmio_offset()
> helper.
> 
> v3:
> - No changes
> 
> v2:
> - Kill panthor_device_mmio_offset()
> 
> 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>
> Reviewed-by: Steven Price <steven.price@arm.com>

All 3 patches queued to drm-misc-next.