| Message ID | 20241111092621.763285-1-akash.goel@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panthor: Fix handling of partial GPU mapping of BOs | expand |
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; >
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 >
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; > > >
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;
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(+)