[PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Steven Price]

On 28/08/2019 13:35, Rob Herring wrote:
> On Wed, Aug 28, 2019 at 5:55 AM Steven Price <steven.price at arm.com> wrote:
>>
>> On 26/08/2019 23:33, Rob Herring wrote:
>>> Currently, page tables are freed without disabling the address space first.
>>> This probably is fine as we'll switch to new page tables when the address
>>> space is allocated again and runtime PM suspend will reset the GPU
>>> clearing the registers. However, it's better to clean up after ourselves.
>>> There is also a problem that we could be accessing the h/w in
>>> tlb_inv_context() when suspended.
>>>
>>> Rework the disable code to make sure we flush caches/TLBs and disable the
>>> address space before freeing the page tables if we are not suspended. As
>>> the tlb_inv_context() hook is only called when freeing the page tables and
>>> we do a flush before disabling the AS, lets remove the flush from
>>> tlb_inv_context and avoid any runtime PM issues.
>>>
>>> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
>>> Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
>>> Cc: Steven Price <steven.price at arm.com>
>>> Cc: Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com>
>>> Cc: David Airlie <airlied at linux.ie>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>> ---
>>> v3:
>>>  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
>>>
>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index d1ebde3327fe..387d830cb7cf 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>>>       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>>  }
>>>
>>> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>>  {
>>> +     mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
>>> +
>>>       mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>>>       mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
>>>
>>
>> At the end of this function we have:
>>
>> |       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>
>> which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
>> one thing I have just noticed is that write_cmd() doesn't wait for
>> AS_ACTIVE to be cleared. This means that the GPU has accepted the
>> command but might not have finished the flush.
>>
>> When freeing page tables we obviously need to wait for the MMU flush to
>> complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
>> because there's a back-to-back set of MMU commands so the second one
>> will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
>> mmu_hw_do_operation() waits for the flush to complete.
> 
> I've copied what's in kbase which doesn't wait AFAICT.

Hmm, well it does in that it calls kbase_mmu_flush_invalidate_noretain()
before disable_as() [1]. But that is equivalent to what you have above.

Oh well, I guess matching kbase is safer here, even if kbase seems to be
doing extra work.

[1]
https://gitlab.freedesktop.org/panfrost/mali_kbase/blob/master/driver/product/kernel/drivers/gpu/arm/midgard/mali_kbase_mmu.c#L1568

>> I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
>> write_cmd - could they use mmu_hw_do_operation_locked() instead?
> 
> mmu_hw_do_operation_locked() also does a lock_region. I guess that
> would be harmless?

Yes, locking regions should be harmless - it just prevents the MMU from
translating the locked region. However you do need to ensure that the
region is unlocked again and UPDATE isn't specified to perform the UNLOCK.

So actually all things considered your patch looks fine - it matches
kbase (even if I don't fully understand why kbase is doing it that way),
and fixes a bug. So:

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

(and sorry for the noise!)

Steve