On Fri, Aug 23, 2019 at 10:44 AM Robin Murphy <robin.mur...@arm.com> wrote: > > On 23/08/2019 16:05, Steven Price wrote: > > On 23/08/2019 12:11, Robin Murphy wrote: > >> On 23/08/2019 03:12, Rob Herring wrote: > >>> There is no point in resuming the h/w just to do flush operations and > >>> doing so takes several locks which cause lockdep issues with the > >>> shrinker. > >>> Rework the flush operations to only happen when the h/w is already > >>> awake. > >>> This avoids taking any locks associated with resuming. > >>> > >>> Cc: Tomeu Vizoso <tomeu.viz...@collabora.com> > >>> Cc: Steven Price <steven.pr...@arm.com> > >>> Cc: Alyssa Rosenzweig <alyssa.rosenzw...@collabora.com> > >>> Cc: David Airlie <airl...@linux.ie> > >>> Cc: Daniel Vetter <dan...@ffwll.ch> > >>> Signed-off-by: Rob Herring <r...@kernel.org> > >>> --- > >>> v2: new patch > >>> > >>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++------------- > >>> 1 file changed, 20 insertions(+), 21 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>> index 842bdd7cf6be..ccf671a9c3fb 100644 > >>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>> @@ -220,6 +220,23 @@ static size_t get_pgsize(u64 addr, size_t size) > >>> return SZ_2M; > >>> } > >>> +void panfrost_mmu_flush_range(struct panfrost_device *pfdev, > >>> + struct panfrost_mmu *mmu, > >>> + u64 iova, size_t size) > >>> +{ > >>> + if (mmu->as < 0) > >>> + return; > >>> + > >>> + /* Flush the PTs only if we're already awake */ > >>> + if (!pm_runtime_get_if_in_use(pfdev->dev)) > >>> + return; > >> > >> Is the MMU guaranteed to be reset on resume such that the TLBs will > >> always come up clean? Otherwise there are potentially corners where > >> stale entries that we skip here might hang around if the hardware lies > >> about powering things down. > > > > Assuming runtime PM has actually committed to the power off, then on > > power on panfrost_device_resume() will be called which performs a reset > > of the GPU which will clear the L2/TLBs so there shouldn't be any > > problem there. > > OK, if panfrost_gpu_soft_reset() is sufficient to guarantee clean TLBs > then this looks equivalent to what we did for arm-smmu, so I've no > complaints in that regard. > > However on second look I've now noticed the panfrost_mmu_flush_range() > calls being moved outside of mmu->lock protection. Forgive me if there's > basic DRM knowledge I'm missing here, but is there any possibility for > multiple threads to create/import/free objects simultaneously on the > same FD such that two mmu_hw_do_operation() calls could race and > interfere with each other in terms of the > AS_LOCKADDR/AS_COMMAND/AS_STATUS dance?
Yes, we could have multiple threads. Not really any good reason it's moved out of the mmu->lock other than just to avoid any nesting (though that seemed fine in testing). The newly added as_lock will serialize mmu_hw_do_operation(). So the mmu->lock is just serializing page table writes. Rob _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel