From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> To: Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux PM <linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, David Airlie <airlied-cv59FeDIM0c@public.gmane.org>, "Rafael J. Wysocki" <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org>, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>, "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, dri-devel <dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>, Linux Kernel Mailing List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, freedreno <freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, linux-arm-msm <linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Date: Thu, 15 Feb 2018 17:14:45 +0000 [thread overview] Message-ID: <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> (raw) In-Reply-To: <CAAFQd5A9B-di9svtiJbvk2hz1U1xo61rTY5vt6AD+KR5iMcG-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On 15/02/18 04:17, Tomasz Figa wrote: [...] >> Could you elaborate on what kind of locking you are concerned about? >> As I explained before, the normally happening fast path would lock >> dev->power_lock only for the brief moment of incrementing the runtime >> PM usage counter. > > My bad, that's not even it. > > The atomic usage counter is incremented beforehands, without any > locking [1] and the spinlock is acquired only for the sake of > validating that device's runtime PM state remained valid indeed [2], > which would be the case in the fast path of the same driver doing two > mappings in parallel, with the master powered on (and so the SMMU, > through device links; if master was not powered on already, powering > on the SMMU is unavoidable anyway and it would add much more latency > than the spinlock itself). We now have no locking at all in the map path, and only a per-domain lock around TLB sync in unmap which is unfortunately necessary for correctness; the latter isn't too terrible, since in "serious" hardware it should only be serialising a few cpus serving the same device against each other (e.g. for multiple queues on a single NIC). Putting in a global lock which serialises *all* concurrent map and unmap calls for *all* unrelated devices makes things worse. Period. Even if the lock itself were held for the minimum possible time, i.e. trivially "spin_lock(&lock); spin_unlock(&lock)", the cost of repeatedly bouncing that one cache line around between 96 CPUs across two sockets is not negligible. > [1] http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L1028 > [2] http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L613 > > In any case, I can't imagine this working with V4L2 or anything else > relying on any memory management more generic than calling IOMMU API > directly from the driver, with the IOMMU device having runtime PM > enabled, but without managing the runtime PM from the IOMMU driver's > callbacks that need access to the hardware. As I mentioned before, > only the IOMMU driver knows when exactly the real hardware access > needs to be done (e.g. Rockchip/Exynos don't need to do that for > map/unmap if the power is down, but some implementations of SMMU with > TLB powered separately might need to do so). It's worth noting that Exynos and Rockchip are relatively small self-contained IP blocks integrated closely with the interfaces of their relevant master devices; SMMU is an architecture, implementations of which may be large, distributed, and have complex and wildly differing internal topologies. As such, it's a lot harder to make hardware-specific assumptions and/or be correct for all possible cases. Don't get me wrong, I do ultimately agree that the IOMMU driver is the only agent who ultimately knows what calls are going to be necessary for whatever operation it's performing on its own hardware*; it's just that for SMMU it needs to be implemented in a way that has zero impact on the cases where it doesn't matter, because it's not viable to specialise that driver for any particular IP implementation/use-case. Robin. *AFAICS it still makes some sense to have the get_suppliers option as well, though - the IOMMU driver does what it needs for correctness internally, but the external consumer doing something non-standard can can grab and hold the link around multiple calls to short-circuit that.
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Tomasz Figa <tfiga@chromium.org> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>, Will Deacon <will.deacon@arm.com>, Rob Clark <robdclark@gmail.com>, "list@263.net:IOMMU DRIVERS" <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, devicetree@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Linux PM <linux-pm@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, freedreno <freedreno@lists.freedesktop.org>, David Airlie <airlied@linux.ie>, Greg KH <gregkh@linuxfoundation.org>, Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, jcrouse@codeaurora.org Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Date: Thu, 15 Feb 2018 17:14:45 +0000 [thread overview] Message-ID: <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> (raw) In-Reply-To: <CAAFQd5A9B-di9svtiJbvk2hz1U1xo61rTY5vt6AD+KR5iMcG-A@mail.gmail.com> On 15/02/18 04:17, Tomasz Figa wrote: [...] >> Could you elaborate on what kind of locking you are concerned about? >> As I explained before, the normally happening fast path would lock >> dev->power_lock only for the brief moment of incrementing the runtime >> PM usage counter. > > My bad, that's not even it. > > The atomic usage counter is incremented beforehands, without any > locking [1] and the spinlock is acquired only for the sake of > validating that device's runtime PM state remained valid indeed [2], > which would be the case in the fast path of the same driver doing two > mappings in parallel, with the master powered on (and so the SMMU, > through device links; if master was not powered on already, powering > on the SMMU is unavoidable anyway and it would add much more latency > than the spinlock itself). We now have no locking at all in the map path, and only a per-domain lock around TLB sync in unmap which is unfortunately necessary for correctness; the latter isn't too terrible, since in "serious" hardware it should only be serialising a few cpus serving the same device against each other (e.g. for multiple queues on a single NIC). Putting in a global lock which serialises *all* concurrent map and unmap calls for *all* unrelated devices makes things worse. Period. Even if the lock itself were held for the minimum possible time, i.e. trivially "spin_lock(&lock); spin_unlock(&lock)", the cost of repeatedly bouncing that one cache line around between 96 CPUs across two sockets is not negligible. > [1] http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L1028 > [2] http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L613 > > In any case, I can't imagine this working with V4L2 or anything else > relying on any memory management more generic than calling IOMMU API > directly from the driver, with the IOMMU device having runtime PM > enabled, but without managing the runtime PM from the IOMMU driver's > callbacks that need access to the hardware. As I mentioned before, > only the IOMMU driver knows when exactly the real hardware access > needs to be done (e.g. Rockchip/Exynos don't need to do that for > map/unmap if the power is down, but some implementations of SMMU with > TLB powered separately might need to do so). It's worth noting that Exynos and Rockchip are relatively small self-contained IP blocks integrated closely with the interfaces of their relevant master devices; SMMU is an architecture, implementations of which may be large, distributed, and have complex and wildly differing internal topologies. As such, it's a lot harder to make hardware-specific assumptions and/or be correct for all possible cases. Don't get me wrong, I do ultimately agree that the IOMMU driver is the only agent who ultimately knows what calls are going to be necessary for whatever operation it's performing on its own hardware*; it's just that for SMMU it needs to be implemented in a way that has zero impact on the cases where it doesn't matter, because it's not viable to specialise that driver for any particular IP implementation/use-case. Robin. *AFAICS it still makes some sense to have the get_suppliers option as well, though - the IOMMU driver does what it needs for correctness internally, but the external consumer doing something non-standard can can grab and hold the link around multiple calls to short-circuit that.
next prev parent reply other threads:[~2018-02-15 17:14 UTC|newest] Thread overview: 103+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-07 10:31 [PATCH v7 0/6] iommu/arm-smmu: Add runtime pm/sleep support Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam [not found] ` <1517999482-17317-1-git-send-email-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-02-07 10:31 ` [PATCH v7 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam 2018-02-13 7:44 ` Tomasz Figa 2018-02-13 7:44 ` Tomasz Figa [not found] ` <CAAFQd5BmroRf-C8dQkvTKHWK1psGnNi1t7g-q=Xce6KjrGTsdw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-13 12:00 ` Robin Murphy 2018-02-13 12:00 ` Robin Murphy 2018-02-13 12:54 ` Tomasz Figa 2018-02-13 12:54 ` Tomasz Figa 2018-02-13 13:37 ` Robin Murphy 2018-02-13 13:37 ` Robin Murphy 2018-02-07 10:31 ` [PATCH v7 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam 2018-02-13 8:03 ` Tomasz Figa 2018-02-13 8:03 ` Tomasz Figa [not found] ` <CAAFQd5B2u8RL-tdB3qgPxVUcXnsBSEhXRBZWxqO-w6rYKAiOtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-13 10:25 ` Vivek Gautam 2018-02-13 10:25 ` Vivek Gautam 2018-02-14 3:45 ` Tomasz Figa 2018-02-14 3:45 ` Tomasz Figa 2018-02-07 10:31 ` [PATCH v7 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam 2018-02-13 8:24 ` Tomasz Figa 2018-02-13 8:24 ` Tomasz Figa [not found] ` <CAAFQd5DxYhkK61VDAesby6bT+FtG2nqsbHQRvxkhrsSS0KWtog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-13 12:57 ` Robin Murphy 2018-02-13 12:57 ` Robin Murphy [not found] ` <906051dd-8898-ec6f-5ad4-3f37716292cf-5wv7dgnIgG8@public.gmane.org> 2018-02-13 13:52 ` Tomasz Figa 2018-02-13 13:52 ` Tomasz Figa [not found] ` <CAAFQd5DJtQYPg5S3Ep2bK27+D5rQiKuA-uPfMDUon3FudmGF0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 8:24 ` Vivek Gautam 2018-02-14 8:24 ` Vivek Gautam 2018-02-14 8:28 ` Vivek Gautam 2018-02-14 8:28 ` Vivek Gautam [not found] ` <1517999482-17317-4-git-send-email-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-02-22 23:52 ` Jordan Crouse 2018-02-22 23:52 ` Jordan Crouse [not found] ` <20180222235200.GA18743-9PYrDHPZ2Orvke4nUoYGnHL1okKdlPRT@public.gmane.org> 2018-02-23 10:36 ` Vivek Gautam 2018-02-23 10:36 ` Vivek Gautam [not found] ` <CAFp+6iGQ5Vckui14Jb=V0uk_Pjes95hOxo=KBijR4yxPeDDzFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-23 15:40 ` [Freedreno] " Jordan Crouse 2018-02-23 15:40 ` Jordan Crouse [not found] ` <20180223154048.GB18743-9PYrDHPZ2Orvke4nUoYGnHL1okKdlPRT@public.gmane.org> 2018-02-23 17:43 ` Vivek Gautam 2018-02-23 17:43 ` [Freedreno] " Vivek Gautam 2018-02-07 10:31 ` [PATCH v7 4/6] iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam 2018-02-13 8:31 ` Tomasz Figa 2018-02-13 8:31 ` Tomasz Figa [not found] ` <CAAFQd5BQAs9=N27_Z0pJNSrndFY3vFin2KBP44UtiW+tMXy5nQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-13 10:14 ` Vivek Gautam 2018-02-13 10:14 ` Vivek Gautam 2018-02-07 10:31 ` [PATCH v7 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant Vivek Gautam 2018-02-07 10:31 ` [PATCH v7 5/6] iommu/arm-smmu: Add support for qcom,smmu-v2 variant Vivek Gautam 2018-02-09 10:57 ` [PATCH v8 " Vivek Gautam 2018-02-13 8:57 ` Tomasz Figa 2018-02-13 8:57 ` Tomasz Figa 2018-02-07 10:31 ` [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Vivek Gautam 2018-02-07 10:31 ` Vivek Gautam 2018-02-13 9:10 ` Tomasz Figa 2018-02-13 9:10 ` Tomasz Figa [not found] ` <CAAFQd5AjopiX6fDgD+mO+-+d0yj-swEnVCNvccWRBSMO+XVJkA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-13 16:42 ` Jordan Crouse 2018-02-13 16:42 ` Jordan Crouse 2018-02-14 3:31 ` Tomasz Figa [not found] ` <CAAFQd5CjQRFATfh-mRQv5J=WefYuxBVTkk=Ju09FoqA-Or5Cvg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 15:48 ` Jordan Crouse 2018-02-14 15:48 ` Jordan Crouse [not found] ` <20180214154850.GA25422-9PYrDHPZ2Orvke4nUoYGnHL1okKdlPRT@public.gmane.org> 2018-02-14 16:12 ` [Freedreno] " Rob Clark 2018-02-14 16:12 ` Rob Clark 2018-02-15 4:09 ` Tomasz Figa 2018-02-15 4:09 ` Tomasz Figa [not found] ` <CAAFQd5C-9mbd3hDSvz10a1oiO0--FT-L4EpsAYcALxxUvk6Fjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-15 14:14 ` Rob Clark 2018-02-15 14:14 ` [Freedreno] " Rob Clark 2018-02-13 18:03 ` Rob Clark 2018-02-13 18:03 ` Rob Clark 2018-02-14 1:59 ` Tomasz Figa 2018-02-14 1:59 ` Tomasz Figa [not found] ` <CAAFQd5BKRumpEfAKNF_RKS-ZZ8D671DfOz4vB2+w1SV3aG9NxQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 2:13 ` Rob Clark 2018-02-14 2:13 ` Rob Clark [not found] ` <CAF6AEGuNZJKtwGZ5mLfqNND2jtU+HYM11UONfAtVTzoM0QVpdg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 3:01 ` Tomasz Figa 2018-02-14 3:01 ` Tomasz Figa [not found] ` <CAAFQd5BZJ1G0RG32hYErNzPRvisBhhiSNCBsjbzfm0WzO=DnsQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 4:17 ` Vivek Gautam 2018-02-14 4:17 ` Vivek Gautam [not found] ` <CAFp+6iHaycK=CcE1S15EeuMkaw8LnW0ebptU0hM6tUtWdeEOtA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 5:38 ` Tomasz Figa 2018-02-14 5:38 ` Tomasz Figa [not found] ` <CAAFQd5Afj-Bj+3wHwmF2tT7y=46EsYEtO_mXfY6stXBgHutEUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 9:13 ` Vivek Gautam 2018-02-14 9:13 ` Vivek Gautam [not found] ` <CAFp+6iGX6pr+MdPSSHHG=qOnhHky_8OHiDqAcJ9UudEUv=JMHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 9:16 ` Tomasz Figa 2018-02-14 9:16 ` Tomasz Figa [not found] ` <CAAFQd5DiwAugGnPOTw0+XrEfef9x-n-vx59JFuXpNawjiXHwCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 10:33 ` Vivek Gautam 2018-02-14 10:33 ` Vivek Gautam [not found] ` <CAFp+6iEW0faeHDfzN_F1bRrHGcVo3sPCk4HSY=t9dnEvHkDkYw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-14 16:03 ` Robin Murphy 2018-02-14 16:03 ` Robin Murphy 2018-02-15 3:17 ` Tomasz Figa [not found] ` <CAAFQd5AmG1zSm+CouXOCJbs8SNGFk1-RqfU1nWGjMGJMB-qfvw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-15 4:17 ` Tomasz Figa 2018-02-15 4:17 ` Tomasz Figa [not found] ` <CAAFQd5A9B-di9svtiJbvk2hz1U1xo61rTY5vt6AD+KR5iMcG-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-15 17:14 ` Robin Murphy [this message] 2018-02-15 17:14 ` Robin Murphy [not found] ` <7406f1ce-c2c9-a6bd-2886-5a34de45add6-5wv7dgnIgG8@public.gmane.org> 2018-02-16 0:13 ` Tomasz Figa 2018-02-16 0:13 ` Tomasz Figa [not found] ` <CAAFQd5DkLtq2w00=Zd4sMDB4QOWqi7R-zgydECJXLdTmaHty+g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-22 8:13 ` Tomasz Figa 2018-02-22 8:13 ` Tomasz Figa [not found] ` <CAAFQd5CQoDqunAunwoVo7W=QXa=ET=eJ2s_j9j+3YgAR2EGgCg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-22 13:30 ` Rob Clark 2018-02-22 13:30 ` [Freedreno] " Rob Clark 2018-02-22 13:45 ` Robin Murphy 2018-02-22 13:45 ` Robin Murphy 2018-02-22 14:12 ` Tomasz Figa 2018-02-22 14:12 ` Tomasz Figa [not found] ` <CAAFQd5CzOJ0oS8LLcKP-DgyXSXgzafg5CWPAvnR-QDPS+DZAUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-02-22 17:24 ` Vivek Gautam 2018-02-22 17:24 ` Vivek Gautam
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com \ --to=robin.murphy-5wv7dgnigg8@public.gmane.org \ --cc=airlied-cv59FeDIM0c@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \ --cc=freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \ --cc=gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org \ --cc=iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \ --cc=linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \ --cc=rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \ --cc=will.deacon-5wv7dgnIgG8@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.