RFC: drm/lima: fix calling drm_mm_init with an empty range

[PATCH] RFC: drm/lima: fix calling drm_mm_init with an empty range

Posted by Alban Browaeys 2 years ago

For the empty_vm initialization the range is empty which is not supported
by drm_mm_init.

With CONFIG_DRM_DEBUG_MM set, I get:
------------[ cut here ]------------
 kernel BUG at drivers/gpu/drm/drm_mm.c:965!
 Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
 Modules linked in: lima(+) drm_shmem_helper gpu_sched s5p_jpeg s5p_g2d
 videobuf2_dma_contig videobuf2_memops v4l2_mem2mem videobuf2_v4l2
 videobuf2_common s5p_cec tun fuse configfs auth_rpcgss sunrpc ip_tables
 x_tables autofs4 btrfs lzo_compress zlib_deflate raid10 raid456
 async_raid6_recov async_memcpy async_pq async_xor async_tx xor xor_neon
 raid6_pq libcrc32c raid1 raid0 linear md_mod dm_mirror dm_region_hash
 dm_log hid_logitech_hidpp hid_logitech_dj
 CPU: 0 PID: 1033 Comm: systemd-udevd Not tainted 6.4.0-rc1-debug+ #230
 Hardware name: Samsung Exynos (Flattened Device Tree)
 PC is at drm_mm_init+0x94/0x98
 LR is at 0x0
 Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
  drm_mm_init from lima_vm_create+0xcc/0x108 [lima]
  lima_vm_create [lima] from lima_device_init+0xd8/0x4a0 [lima]
  lima_device_init [lima] from lima_pdev_probe.part.0+0x6c/0x158 [lima]
  lima_pdev_probe.part.0 [lima] from platform_probe+0x64/0xc0
  platform_probe from call_driver_probe+0x2c/0x110

The drm_mm.c line 965 is:
drivers/gpu/drm/drm_mm.c
void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
{
        DRM_MM_BUG_ON(start + size <= start);

lima_vm_create is called with va_start and va_end both unset
in lima_device_init line 371:
ldev->empty_vm = lima_vm_create(ldev);

Signed-off-by: Alban Browaeys <alban.browaeys@gmail.com>
---
 drivers/gpu/drm/lima/lima_device.c |  2 +-
 drivers/gpu/drm/lima/lima_drv.c    |  2 +-
 drivers/gpu/drm/lima/lima_vm.c     | 10 +++++++---
 drivers/gpu/drm/lima/lima_vm.h     |  3 ++-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/lima/lima_device.c b/drivers/gpu/drm/lima/lima_device.c
index 02cef0cea657..bd3afff0f44a 100644
--- a/drivers/gpu/drm/lima/lima_device.c
+++ b/drivers/gpu/drm/lima/lima_device.c
@@ -368,7 +368,7 @@ int lima_device_init(struct lima_device *ldev)
 	if (err)
 		goto err_out0;
 
-	ldev->empty_vm = lima_vm_create(ldev);
+	ldev->empty_vm = lima_vm_create(ldev, false);
 	if (!ldev->empty_vm) {
 		err = -ENOMEM;
 		goto err_out1;
diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
index 10fd9154cc46..ca09142e0ac1 100644
--- a/drivers/gpu/drm/lima/lima_drv.c
+++ b/drivers/gpu/drm/lima/lima_drv.c
@@ -219,7 +219,7 @@ static int lima_drm_driver_open(struct drm_device *dev, struct drm_file *file)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->vm = lima_vm_create(ldev);
+	priv->vm = lima_vm_create(ldev, true);
 	if (!priv->vm) {
 		err = -ENOMEM;
 		goto err_out0;
diff --git a/drivers/gpu/drm/lima/lima_vm.c b/drivers/gpu/drm/lima/lima_vm.c
index 2b2739adc7f5..7f9775eefd78 100644
--- a/drivers/gpu/drm/lima/lima_vm.c
+++ b/drivers/gpu/drm/lima/lima_vm.c
@@ -197,7 +197,7 @@ u32 lima_vm_get_va(struct lima_vm *vm, struct lima_bo *bo)
 	return ret;
 }
 
-struct lima_vm *lima_vm_create(struct lima_device *dev)
+struct lima_vm *lima_vm_create(struct lima_device *dev, bool has_drm_mm)
 {
 	struct lima_vm *vm;
 
@@ -221,7 +221,10 @@ struct lima_vm *lima_vm_create(struct lima_device *dev)
 			goto err_out1;
 	}
 
-	drm_mm_init(&vm->mm, dev->va_start, dev->va_end - dev->va_start);
+	if (has_drm_mm) {
+		vm->has_drm_mm = true;
+		drm_mm_init(&vm->mm, dev->va_start, dev->va_end - dev->va_start);
+	}
 
 	return vm;
 
@@ -237,7 +240,8 @@ void lima_vm_release(struct kref *kref)
 	struct lima_vm *vm = container_of(kref, struct lima_vm, refcount);
 	int i;
 
-	drm_mm_takedown(&vm->mm);
+	if (vm->has_drm_mm)
+		drm_mm_takedown(&vm->mm);
 
 	for (i = 0; i < LIMA_VM_NUM_BT; i++) {
 		if (vm->bts[i].cpu)
diff --git a/drivers/gpu/drm/lima/lima_vm.h b/drivers/gpu/drm/lima/lima_vm.h
index 3a7c74822d8b..e7443f410d6d 100644
--- a/drivers/gpu/drm/lima/lima_vm.h
+++ b/drivers/gpu/drm/lima/lima_vm.h
@@ -30,6 +30,7 @@ struct lima_vm {
 	struct mutex lock;
 	struct kref refcount;
 
+	bool has_drm_mm;
 	struct drm_mm mm;
 
 	struct lima_device *dev;
@@ -43,7 +44,7 @@ void lima_vm_bo_del(struct lima_vm *vm, struct lima_bo *bo);
 
 u32 lima_vm_get_va(struct lima_vm *vm, struct lima_bo *bo);
 
-struct lima_vm *lima_vm_create(struct lima_device *dev);
+struct lima_vm *lima_vm_create(struct lima_device *dev, bool has_drm_mm);
 void lima_vm_release(struct kref *kref);
 
 static inline struct lima_vm *lima_vm_get(struct lima_vm *vm)
-- 
2.39.2

Re: [PATCH] RFC: drm/lima: fix calling drm_mm_init with an empty range

Posted by Qiang Yu 2 years ago

Thanks for the fix. It could be done in a simpler way that swap the
va_start/va_end init/fini and empty_vm create/release.

On Thu, Dec 14, 2023 at 5:04 PM Alban Browaeys <alban.browaeys@gmail.com> wrote:
>
> For the empty_vm initialization the range is empty which is not supported
> by drm_mm_init.
>
> With CONFIG_DRM_DEBUG_MM set, I get:
> ------------[ cut here ]------------
>  kernel BUG at drivers/gpu/drm/drm_mm.c:965!
>  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>  Modules linked in: lima(+) drm_shmem_helper gpu_sched s5p_jpeg s5p_g2d
>  videobuf2_dma_contig videobuf2_memops v4l2_mem2mem videobuf2_v4l2
>  videobuf2_common s5p_cec tun fuse configfs auth_rpcgss sunrpc ip_tables
>  x_tables autofs4 btrfs lzo_compress zlib_deflate raid10 raid456
>  async_raid6_recov async_memcpy async_pq async_xor async_tx xor xor_neon
>  raid6_pq libcrc32c raid1 raid0 linear md_mod dm_mirror dm_region_hash
>  dm_log hid_logitech_hidpp hid_logitech_dj
>  CPU: 0 PID: 1033 Comm: systemd-udevd Not tainted 6.4.0-rc1-debug+ #230
>  Hardware name: Samsung Exynos (Flattened Device Tree)
>  PC is at drm_mm_init+0x94/0x98
>  LR is at 0x0
>  Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
>   drm_mm_init from lima_vm_create+0xcc/0x108 [lima]
>   lima_vm_create [lima] from lima_device_init+0xd8/0x4a0 [lima]
>   lima_device_init [lima] from lima_pdev_probe.part.0+0x6c/0x158 [lima]
>   lima_pdev_probe.part.0 [lima] from platform_probe+0x64/0xc0
>   platform_probe from call_driver_probe+0x2c/0x110
>
> The drm_mm.c line 965 is:
> drivers/gpu/drm/drm_mm.c
> void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
> {
>         DRM_MM_BUG_ON(start + size <= start);
>
> lima_vm_create is called with va_start and va_end both unset
> in lima_device_init line 371:
> ldev->empty_vm = lima_vm_create(ldev);
>
> Signed-off-by: Alban Browaeys <alban.browaeys@gmail.com>
> ---
>  drivers/gpu/drm/lima/lima_device.c |  2 +-
>  drivers/gpu/drm/lima/lima_drv.c    |  2 +-
>  drivers/gpu/drm/lima/lima_vm.c     | 10 +++++++---
>  drivers/gpu/drm/lima/lima_vm.h     |  3 ++-
>  4 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_device.c b/drivers/gpu/drm/lima/lima_device.c
> index 02cef0cea657..bd3afff0f44a 100644
> --- a/drivers/gpu/drm/lima/lima_device.c
> +++ b/drivers/gpu/drm/lima/lima_device.c
> @@ -368,7 +368,7 @@ int lima_device_init(struct lima_device *ldev)
>         if (err)
>                 goto err_out0;
>
> -       ldev->empty_vm = lima_vm_create(ldev);
> +       ldev->empty_vm = lima_vm_create(ldev, false);
>         if (!ldev->empty_vm) {
>                 err = -ENOMEM;
>                 goto err_out1;
> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
> index 10fd9154cc46..ca09142e0ac1 100644
> --- a/drivers/gpu/drm/lima/lima_drv.c
> +++ b/drivers/gpu/drm/lima/lima_drv.c
> @@ -219,7 +219,7 @@ static int lima_drm_driver_open(struct drm_device *dev, struct drm_file *file)
>         if (!priv)
>                 return -ENOMEM;
>
> -       priv->vm = lima_vm_create(ldev);
> +       priv->vm = lima_vm_create(ldev, true);
>         if (!priv->vm) {
>                 err = -ENOMEM;
>                 goto err_out0;
> diff --git a/drivers/gpu/drm/lima/lima_vm.c b/drivers/gpu/drm/lima/lima_vm.c
> index 2b2739adc7f5..7f9775eefd78 100644
> --- a/drivers/gpu/drm/lima/lima_vm.c
> +++ b/drivers/gpu/drm/lima/lima_vm.c
> @@ -197,7 +197,7 @@ u32 lima_vm_get_va(struct lima_vm *vm, struct lima_bo *bo)
>         return ret;
>  }
>
> -struct lima_vm *lima_vm_create(struct lima_device *dev)
> +struct lima_vm *lima_vm_create(struct lima_device *dev, bool has_drm_mm)
>  {
>         struct lima_vm *vm;
>
> @@ -221,7 +221,10 @@ struct lima_vm *lima_vm_create(struct lima_device *dev)
>                         goto err_out1;
>         }
>
> -       drm_mm_init(&vm->mm, dev->va_start, dev->va_end - dev->va_start);
> +       if (has_drm_mm) {
> +               vm->has_drm_mm = true;
> +               drm_mm_init(&vm->mm, dev->va_start, dev->va_end - dev->va_start);
> +       }
>
>         return vm;
>
> @@ -237,7 +240,8 @@ void lima_vm_release(struct kref *kref)
>         struct lima_vm *vm = container_of(kref, struct lima_vm, refcount);
>         int i;
>
> -       drm_mm_takedown(&vm->mm);
> +       if (vm->has_drm_mm)
> +               drm_mm_takedown(&vm->mm);
>
>         for (i = 0; i < LIMA_VM_NUM_BT; i++) {
>                 if (vm->bts[i].cpu)
> diff --git a/drivers/gpu/drm/lima/lima_vm.h b/drivers/gpu/drm/lima/lima_vm.h
> index 3a7c74822d8b..e7443f410d6d 100644
> --- a/drivers/gpu/drm/lima/lima_vm.h
> +++ b/drivers/gpu/drm/lima/lima_vm.h
> @@ -30,6 +30,7 @@ struct lima_vm {
>         struct mutex lock;
>         struct kref refcount;
>
> +       bool has_drm_mm;
>         struct drm_mm mm;
>
>         struct lima_device *dev;
> @@ -43,7 +44,7 @@ void lima_vm_bo_del(struct lima_vm *vm, struct lima_bo *bo);
>
>  u32 lima_vm_get_va(struct lima_vm *vm, struct lima_bo *bo);
>
> -struct lima_vm *lima_vm_create(struct lima_device *dev);
> +struct lima_vm *lima_vm_create(struct lima_device *dev, bool has_drm_mm);
>  void lima_vm_release(struct kref *kref);
>
>  static inline struct lima_vm *lima_vm_get(struct lima_vm *vm)
> --
> 2.39.2
>

Re: [PATCH] RFC: drm/lima: fix calling drm_mm_init with an empty range

Posted by Alban Browaeys 2 years ago

Le lundi 18 décembre 2023 à 10:50 +0800, Qiang Yu a écrit :
> Thanks for the fix. It could be done in a simpler way that swap the
> va_start/va_end init/fini and empty_vm create/release.

The thing is I do not get what you suggest by swaping :-/

Do you mean I should check for "dev->va_end - dev->va_start"  not to be
zero instead of passing a boolean and setting a flag on the structure?

PS: This was an RFC to raise the issue, that would be fine to me if you
take it further.


Alban

> On Thu, Dec 14, 2023 at 5:04 PM Alban Browaeys
> <alban.browaeys@gmail.com> wrote:
> > 
> > For the empty_vm initialization the range is empty which is not
> > supported
> > by drm_mm_init.
> > 
> > With CONFIG_DRM_DEBUG_MM set, I get:
> > ------------[ cut here ]------------
> >  kernel BUG at drivers/gpu/drm/drm_mm.c:965!
> >  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> >  Modules linked in: lima(+) drm_shmem_helper gpu_sched s5p_jpeg
> > s5p_g2d
> >  videobuf2_dma_contig videobuf2_memops v4l2_mem2mem videobuf2_v4l2
> >  videobuf2_common s5p_cec tun fuse configfs auth_rpcgss sunrpc
> > ip_tables
> >  x_tables autofs4 btrfs lzo_compress zlib_deflate raid10 raid456
> >  async_raid6_recov async_memcpy async_pq async_xor async_tx xor
> > xor_neon
> >  raid6_pq libcrc32c raid1 raid0 linear md_mod dm_mirror
> > dm_region_hash
> >  dm_log hid_logitech_hidpp hid_logitech_dj
> >  CPU: 0 PID: 1033 Comm: systemd-udevd Not tainted 6.4.0-rc1-debug+
> > #230
> >  Hardware name: Samsung Exynos (Flattened Device Tree)
> >  PC is at drm_mm_init+0x94/0x98
> >  LR is at 0x0
> >  Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> >   drm_mm_init from lima_vm_create+0xcc/0x108 [lima]
> >   lima_vm_create [lima] from lima_device_init+0xd8/0x4a0 [lima]
> >   lima_device_init [lima] from lima_pdev_probe.part.0+0x6c/0x158
> > [lima]
> >   lima_pdev_probe.part.0 [lima] from platform_probe+0x64/0xc0
> >   platform_probe from call_driver_probe+0x2c/0x110
> > 
> > The drm_mm.c line 965 is:
> > drivers/gpu/drm/drm_mm.c
> > void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
> > {
> >         DRM_MM_BUG_ON(start + size <= start);
> > 
> > lima_vm_create is called with va_start and va_end both unset
> > in lima_device_init line 371:
> > ldev->empty_vm = lima_vm_create(ldev);
> > 
> > Signed-off-by: Alban Browaeys <alban.browaeys@gmail.com>
> > ---
> >  drivers/gpu/drm/lima/lima_device.c |  2 +-
> >  drivers/gpu/drm/lima/lima_drv.c    |  2 +-
> >  drivers/gpu/drm/lima/lima_vm.c     | 10 +++++++---
> >  drivers/gpu/drm/lima/lima_vm.h     |  3 ++-
> >  4 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/lima/lima_device.c
> > b/drivers/gpu/drm/lima/lima_device.c
> > index 02cef0cea657..bd3afff0f44a 100644
> > --- a/drivers/gpu/drm/lima/lima_device.c
> > +++ b/drivers/gpu/drm/lima/lima_device.c
> > @@ -368,7 +368,7 @@ int lima_device_init(struct lima_device *ldev)
> >         if (err)
> >                 goto err_out0;
> > 
> > -       ldev->empty_vm = lima_vm_create(ldev);
> > +       ldev->empty_vm = lima_vm_create(ldev, false);
> >         if (!ldev->empty_vm) {
> >                 err = -ENOMEM;
> >                 goto err_out1;
> > diff --git a/drivers/gpu/drm/lima/lima_drv.c
> > b/drivers/gpu/drm/lima/lima_drv.c
> > index 10fd9154cc46..ca09142e0ac1 100644
> > --- a/drivers/gpu/drm/lima/lima_drv.c
> > +++ b/drivers/gpu/drm/lima/lima_drv.c
> > @@ -219,7 +219,7 @@ static int lima_drm_driver_open(struct
> > drm_device *dev, struct drm_file *file)
> >         if (!priv)
> >                 return -ENOMEM;
> > 
> > -       priv->vm = lima_vm_create(ldev);
> > +       priv->vm = lima_vm_create(ldev, true);
> >         if (!priv->vm) {
> >                 err = -ENOMEM;
> >                 goto err_out0;
> > diff --git a/drivers/gpu/drm/lima/lima_vm.c
> > b/drivers/gpu/drm/lima/lima_vm.c
> > index 2b2739adc7f5..7f9775eefd78 100644
> > --- a/drivers/gpu/drm/lima/lima_vm.c
> > +++ b/drivers/gpu/drm/lima/lima_vm.c
> > @@ -197,7 +197,7 @@ u32 lima_vm_get_va(struct lima_vm *vm, struct
> > lima_bo *bo)
> >         return ret;
> >  }
> > 
> > -struct lima_vm *lima_vm_create(struct lima_device *dev)
> > +struct lima_vm *lima_vm_create(struct lima_device *dev, bool
> > has_drm_mm)
> >  {
> >         struct lima_vm *vm;
> > 
> > @@ -221,7 +221,10 @@ struct lima_vm *lima_vm_create(struct
> > lima_device *dev)
> >                         goto err_out1;
> >         }
> > 
> > -       drm_mm_init(&vm->mm, dev->va_start, dev->va_end - dev-
> > >va_start);
> > +       if (has_drm_mm) {
> > +               vm->has_drm_mm = true;
> > +               drm_mm_init(&vm->mm, dev->va_start, dev->va_end -
> > dev->va_start);
> > +       }
> > 
> >         return vm;
> > 
> > @@ -237,7 +240,8 @@ void lima_vm_release(struct kref *kref)
> >         struct lima_vm *vm = container_of(kref, struct lima_vm,
> > refcount);
> >         int i;
> > 
> > -       drm_mm_takedown(&vm->mm);
> > +       if (vm->has_drm_mm)
> > +               drm_mm_takedown(&vm->mm);
> > 
> >         for (i = 0; i < LIMA_VM_NUM_BT; i++) {
> >                 if (vm->bts[i].cpu)
> > diff --git a/drivers/gpu/drm/lima/lima_vm.h
> > b/drivers/gpu/drm/lima/lima_vm.h
> > index 3a7c74822d8b..e7443f410d6d 100644
> > --- a/drivers/gpu/drm/lima/lima_vm.h
> > +++ b/drivers/gpu/drm/lima/lima_vm.h
> > @@ -30,6 +30,7 @@ struct lima_vm {
> >         struct mutex lock;
> >         struct kref refcount;
> > 
> > +       bool has_drm_mm;
> >         struct drm_mm mm;
> > 
> >         struct lima_device *dev;
> > @@ -43,7 +44,7 @@ void lima_vm_bo_del(struct lima_vm *vm, struct
> > lima_bo *bo);
> > 
> >  u32 lima_vm_get_va(struct lima_vm *vm, struct lima_bo *bo);
> > 
> > -struct lima_vm *lima_vm_create(struct lima_device *dev);
> > +struct lima_vm *lima_vm_create(struct lima_device *dev, bool
> > has_drm_mm);
> >  void lima_vm_release(struct kref *kref);
> > 
> >  static inline struct lima_vm *lima_vm_get(struct lima_vm *vm)
> > --
> > 2.39.2
> >