[PATCH] drm/panfrost: Update io-pgtable API

[Dmitry Osipenko]

On 11/4/22 23:37, Robin Murphy wrote:
> On 2022-11-04 20:11, Dmitry Osipenko wrote:
>> On 8/23/22 01:01, Robin Murphy wrote:
>>> Convert to io-pgtable's bulk {map,unmap}_pages() APIs, to help the old
>>> single-page interfaces eventually go away. Unmapping heap BOs still
>>> wants to be done a page at a time, but everything else can get the full
>>> benefit of the more efficient interface.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>> ---
>>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 40 +++++++++++++++----------
>>>   1 file changed, 25 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index b285a8001b1d..e246d914e7f6 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -248,11 +248,15 @@ void panfrost_mmu_reset(struct panfrost_device
>>> *pfdev)
>>>       mmu_write(pfdev, MMU_INT_MASK, ~0);
>>>   }
>>>   -static size_t get_pgsize(u64 addr, size_t size)
>>> +static size_t get_pgsize(u64 addr, size_t size, size_t *count)
>>>   {
>>> -    if (addr & (SZ_2M - 1) || size < SZ_2M)
>>> -        return SZ_4K;
>>> +    size_t blk_offset = -addr % SZ_2M;
>>>   +    if (blk_offset || size < SZ_2M) {
>>> +        *count = min_not_zero(blk_offset, size) / SZ_4K;
>>> +        return SZ_4K;
>>> +    }
>>> +    *count = size / SZ_2M;
>>>       return SZ_2M;
>>>   }
>>>   @@ -287,12 +291,16 @@ static int mmu_map_sg(struct panfrost_device
>>> *pfdev, struct panfrost_mmu *mmu,
>>>           dev_dbg(pfdev->dev, "map: as=%d, iova=%llx, paddr=%lx,
>>> len=%zx", mmu->as, iova, paddr, len);
>>>             while (len) {
>>> -            size_t pgsize = get_pgsize(iova | paddr, len);
>>> +            size_t pgcount, mapped = 0;
>>> +            size_t pgsize = get_pgsize(iova | paddr, len, &pgcount);
>>>   -            ops->map(ops, iova, paddr, pgsize, prot, GFP_KERNEL);
>>> -            iova += pgsize;
>>> -            paddr += pgsize;
>>> -            len -= pgsize;
>>> +            ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
>>> +                       GFP_KERNEL, &mapped);
>>> +            /* Don't get stuck if things have gone wrong */
>>> +            mapped = max(mapped, pgsize);
>>> +            iova += mapped;
>>> +            paddr += mapped;
>>> +            len -= mapped;
>>>           }
>>>       }
>>>   @@ -344,15 +352,17 @@ void panfrost_mmu_unmap(struct
>>> panfrost_gem_mapping *mapping)
>>>           mapping->mmu->as, iova, len);
>>>         while (unmapped_len < len) {
>>> -        size_t unmapped_page;
>>> -        size_t pgsize = get_pgsize(iova, len - unmapped_len);
>>> +        size_t unmapped_page, pgcount;
>>> +        size_t pgsize = get_pgsize(iova, len - unmapped_len, &pgcount);
>>>   -        if (ops->iova_to_phys(ops, iova)) {
>>> -            unmapped_page = ops->unmap(ops, iova, pgsize, NULL);
>>> -            WARN_ON(unmapped_page != pgsize);
>>> +        if (bo->is_heap)
>>> +            pgcount = 1;
>>> +        if (!bo->is_heap || ops->iova_to_phys(ops, iova)) {
>>> +            unmapped_page = ops->unmap_pages(ops, iova, pgsize,
>>> pgcount, NULL);
>>> +            WARN_ON(unmapped_page != pgsize * pgcount);
>>
>> This patch causes this WARN_ON to trigger. It doesn't happen all the
>> time, I see that the whole unmapped area is mapped. Initially, I thought
>> that this happens because it tries to unmap a partially mapped range,
>> but I checked that ops->iova_to_phys() returns address for all 4k chunks.
>>
>> For example the pgsize * pgcount = 0x8000000, while returned
>> unmapped_page = 0x6000000.
>>
>> I don't see this problem with this patch reverted. This is using today's
>> linux-next. Any ideas?
> 
> What's the base IOVA in such a case? I'm wondering if the truncated size
> lines up to any interesting boundary. Presumably you're not seeing any
> additional warnings from io-pgtable itself?

No warnings from io-pgtable. It succeeds for 0x32000000 and fails for
0x3a000000 using same size 0x8000000. It actually fails only for the
0x3a000000 as far as I see from my logs. Perhaps it indeed has to do
something with the boundary.

-- 
Best regards,
Dmitry