[PATCH] drm/panthor: Fix handling of partial GPU mapping of BOs

[PATCH] drm/panthor: Fix handling of partial GPU mapping of BOs

[Akash Goel]

This commit fixes the handling of partial GPU mapping of buffer objects
in Panthor.
VM_BIND ioctl allows Userspace to partially map the BOs to GPU.
To map a BO, Panthor walks through the sg_table to retrieve the physical
address of pages. If the mapping is created at an offset into the BO,
then the scatterlist(s) at the beginning have to be skipped to reach the
one corresponding to the offset. But the case where the offset didn't
point to the first page of desired scatterlist wasn't handled correctly.
The bug caused the partial GPU mapping of BO to go wrong for the said
case, as the pages didn't get map at the expected virtual address and
consequently there were kernel warnings on unmap.

Signed-off-by: Akash Goel <akash.goel@arm.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index d8cc9e7d064e..6bc188d9a9ad 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -957,6 +957,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
 
 		paddr += offset;
 		len -= offset;
+		offset = 0;
 		len = min_t(size_t, len, size);
 		size -= len;
 
-- 
2.25.1

Re: [PATCH] drm/panthor: Fix handling of partial GPU mapping of BOs

[Steven Price]

On 11/11/2024 09:26, Akash Goel wrote:
> This commit fixes the handling of partial GPU mapping of buffer objects
> in Panthor.
> VM_BIND ioctl allows Userspace to partially map the BOs to GPU.
> To map a BO, Panthor walks through the sg_table to retrieve the physical
> address of pages. If the mapping is created at an offset into the BO,
> then the scatterlist(s) at the beginning have to be skipped to reach the
> one corresponding to the offset. But the case where the offset didn't
> point to the first page of desired scatterlist wasn't handled correctly.
> The bug caused the partial GPU mapping of BO to go wrong for the said
> case, as the pages didn't get map at the expected virtual address and
> consequently there were kernel warnings on unmap.
> 
> Signed-off-by: Akash Goel <akash.goel@arm.com>

Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")

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

Thanks,
Steve

> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index d8cc9e7d064e..6bc188d9a9ad 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -957,6 +957,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
>  
>  		paddr += offset;
>  		len -= offset;
> +		offset = 0;
>  		len = min_t(size_t, len, size);
>  		size -= len;
>  

Re: [PATCH] drm/panthor: Fix handling of partial GPU mapping of BOs

[Boris Brezillon]

On Mon, 11 Nov 2024 11:31:12 +0000
Steven Price <steven.price@arm.com> wrote:

> On 11/11/2024 09:26, Akash Goel wrote:
> > This commit fixes the handling of partial GPU mapping of buffer objects
> > in Panthor.
> > VM_BIND ioctl allows Userspace to partially map the BOs to GPU.
> > To map a BO, Panthor walks through the sg_table to retrieve the physical
> > address of pages. If the mapping is created at an offset into the BO,
> > then the scatterlist(s) at the beginning have to be skipped to reach the
> > one corresponding to the offset. But the case where the offset didn't
> > point to the first page of desired scatterlist wasn't handled correctly.
> > The bug caused the partial GPU mapping of BO to go wrong for the said
> > case, as the pages didn't get map at the expected virtual address and
> > consequently there were kernel warnings on unmap.
> > 
> > Signed-off-by: Akash Goel <akash.goel@arm.com>  
> 
> Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")
> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
> Thanks,
> Steve
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index d8cc9e7d064e..6bc188d9a9ad 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -957,6 +957,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
> >  
> >  		paddr += offset;
> >  		len -= offset;
> > +		offset = 0;
> >  		len = min_t(size_t, len, size);
> >  		size -= len;
> >    
> 

Re: [PATCH] drm/panthor: Fix handling of partial GPU mapping of BOs

[Liviu Dudau]

On Mon, Nov 11, 2024 at 09:26:21AM +0000, Akash Goel wrote:
> This commit fixes the handling of partial GPU mapping of buffer objects
> in Panthor.
> VM_BIND ioctl allows Userspace to partially map the BOs to GPU.
> To map a BO, Panthor walks through the sg_table to retrieve the physical
> address of pages. If the mapping is created at an offset into the BO,
> then the scatterlist(s) at the beginning have to be skipped to reach the
> one corresponding to the offset. But the case where the offset didn't
> point to the first page of desired scatterlist wasn't handled correctly.
> The bug caused the partial GPU mapping of BO to go wrong for the said
> case, as the pages didn't get map at the expected virtual address and
> consequently there were kernel warnings on unmap.

Maybe it's just me, but I would find it easier to figure out what's being
fixed here if commit message said something like:

When the BO being mapped spans multiple scatterlists, offset is not cleared
after the starting scatterlist, leading to holes in the mapping.


If I understand it correctly you found this based on some WARN() being triggered,
so maybe adding the dump here would've helped too.


> 
> Signed-off-by: Akash Goel <akash.goel@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index d8cc9e7d064e..6bc188d9a9ad 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -957,6 +957,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
>  
>  		paddr += offset;
>  		len -= offset;
> +		offset = 0;
>  		len = min_t(size_t, len, size);
>  		size -= len;

Again, my preference so feel free to ignore, but I would put the resetting of offset at the
end of for_each_sgtable_dma_sg() loop, after the if (!size) break lines. That way it is clear
that it applies to the next iteration of the loop.

Regardsless of the changes you're going to make,

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

Best regards,
Liviu

>  
> -- 
> 2.25.1
> 

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