| Message ID | 20190823021216.5862-9-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/8] drm/panfrost: Fix possible suspend in panfrost_remove | expand |
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) >
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
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.
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
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.
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) >
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)
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(-)