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

[Rob Herring]

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.

> 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?

Rob