[v2,8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context
diff mbox series

Message ID 20190823021216.5862-9-robh@kernel.org
State New
Headers show
Series
  • [v2,1/8] drm/panfrost: Fix possible suspend in panfrost_remove
Related show

Commit Message

Rob Herring Aug. 23, 2019, 2:12 a.m. UTC
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.
There is also a problem that we could be accessing the h/w when suspended.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v2: new patch

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Robin Murphy Aug. 23, 2019, 12:56 p.m. UTC | #1
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).

Robin.

> There is also a problem that we could be accessing the h/w when suspended.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ccf671a9c3fb..9f85275a896c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -328,11 +328,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   }
>   
>   static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
>   
>   static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>   				     size_t granule, bool leaf, void *cookie)
>
Rob Herring Aug. 23, 2019, 1:18 p.m. UTC | #2
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. 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.

Rob
Robin Murphy Aug. 23, 2019, 2:05 p.m. UTC | #3
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.

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

Robin.
Rob Herring Aug. 23, 2019, 2:26 p.m. UTC | #4
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.

> > 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
Robin Murphy Aug. 23, 2019, 2:56 p.m. UTC | #5
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.
Steven Price Aug. 23, 2019, 3:12 p.m. UTC | #6
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.
> There is also a problem that we could be accessing the h/w when suspended.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

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

Although it might be worth trying to capture the justification about 
this in a comment somewhere - there's been a fair bit of discussion 
about this...

Steve

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ccf671a9c3fb..9f85275a896c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -328,11 +328,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   }
>   
>   static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
>   
>   static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>   				     size_t granule, bool leaf, void *cookie)
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ccf671a9c3fb..9f85275a896c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -328,11 +328,7 @@  void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 }
 
 static void mmu_tlb_inv_context_s1(void *cookie)
-{
-	struct panfrost_file_priv *priv = cookie;
-
-	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
-}
+{}
 
 static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 				     size_t granule, bool leaf, void *cookie)