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

From: Robin Murphy <robin.murphy@arm.com>
To: Rob Herring <robh@kernel.org>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	David Airlie <airlied@linux.ie>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Steven Price <steven.price@arm.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context
Date: Fri, 23 Aug 2019 15:56:38 +0100	[thread overview]
Message-ID: <b55cc937-c5cf-3c7b-1cac-6d78ea9bdcab@arm.com> (raw)
In-Reply-To: <CAL_JsqJgjdw9WVXYPtxA4F0B8NfXVQ00NWqcM_bW582QbQjp9w@mail.gmail.com>

On 23/08/2019 15:26, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy <robin.murphy@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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel