On 23/08/2019 15:26, Rob Herring wrote:
On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
On 23/08/2019 14:18, Rob Herring wrote:
On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy@xxxxxxx> 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.
Ah, silly me, I forgot to look at -next...
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.
I guess in that case we probably want it in panfrost_mmu_as_put() when
as_count falls to 0, to balance the corresponding enable in as_get(). If
the tables are only freed later when the FD is closed and whichever AS
is probably in use by some other job, that's even more reason that
.tlb_inv_context is now the wrong place to be touching hardware.
Robin.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
