[PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Rob Herring]

On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy <robin.murphy at arm.com> wrote:
>
> On 23/08/2019 14:18, Rob Herring wrote:
> > On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy at arm.com> wrote:
> >>
> >> On 23/08/2019 03:12, Rob Herring wrote:
> >>> tlb_inv_context() hook is only called when freeing the page tables. There's
> >>> no point in flushing only to free the page tables immediately following.
> >>
> >> FWIW, in general the point of flushing is *because* we're about to free
> >> the pagetables - if there's any possibility that the hardware could
> >> continue to issue translation table walks (speculative or otherwise)
> >> after those pages have been reused by someone else, TLB badness may ensue.
> >>
> >> For panfrost in particular I suspect we can probably get away without
> >> it, at least for the moment, but it might be worth moving the flush to
> >> mmu_disable() for complete peace of mind (which kind of preempts the
> >> sort of thing that per-process AS switching will want anyway).
> >
> > There's bigger problem that mmu_disable() is still only called for AS0
> > and only for driver unload.
>
> Sure, but AS0 is the only one we ever enable, and conceptually we do
> that once at probe time (AFAICS it stays nominally live through resets
> and resumes), so while it may be the least-clever AS usage possible it's
> at least self-consistent. And at the moment the only time we'll call
> .tlb_inv_context is from panfrost_mmu_fini() when we're unconditionally
> poking registers anyway, so either way I don't think there's any actual
> problem today - I'm viewing this patch as getting things straight in
> preparation for the future.

It was only AS0, but we now have per FD AS.

> > I guess we should fix that and then figure
> > out where a flush is needed if at all. I would think changing the TTBR
> > would be enough to quiesce the h/w and TLBs. That seems to be the case
> > in my testing of switching address spaces.
>
>  From a quick scan through kbase, kbase_mmu_disable() appears to perform
> an full FLUSH_MEM before rewriting TRANSTAB, and it looks like that does
> get called when scheduling out a context. That's in line with what I was
> imagining, so unless we know better for sure, we may as well play it
> safe and follow the same pattern.

Agreed.

Rob