Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <CAAFQd5C-9mbd3hDSvz10a1oiO0--FT-L4EpsAYcALxxUvk6Fjg@mail.gmail.com>
References: <1517999482-17317-1-git-send-email-vivek.gautam@codeaurora.org>
 <1517999482-17317-7-git-send-email-vivek.gautam@codeaurora.org>
 <CAAFQd5AjopiX6fDgD+mO+-+d0yj-swEnVCNvccWRBSMO+XVJkA@mail.gmail.com>
 <20180213164205.GA7599@jcrouse-lnx.qualcomm.com> <CAAFQd5CjQRFATfh-mRQv5J=WefYuxBVTkk=Ju09FoqA-Or5Cvg@mail.gmail.com>
 <20180214154850.GA25422@jcrouse-lnx.qualcomm.com> <CAF6AEGtFOwBtXw7HvDSpEj+w78dsg9+p2SVk9e-uDuCE23Fpbw@mail.gmail.com>
 <CAAFQd5C-9mbd3hDSvz10a1oiO0--FT-L4EpsAYcALxxUvk6Fjg@mail.gmail.com>
From:   Rob Clark <robdclark@gmail.com>
Date:   Thu, 15 Feb 2018 09:14:40 -0500
Message-ID: <CAF6AEGvWhG83gYOAHSkx4_t3y_y4WH3xEmE3VXzVnO8VExJ0Vw@mail.gmail.com>
Subject: Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls
 with runtime suppliers
To:     Tomasz Figa <tfiga@chromium.org>
Cc:     Vivek Gautam <vivek.gautam@codeaurora.org>,
        Mark Rutland <mark.rutland@arm.com>,
        devicetree@vger.kernel.org, Linux PM <linux-pm@vger.kernel.org>,
        David Airlie <airlied@linux.ie>,
        Will Deacon <will.deacon@arm.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        "list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg
        Roedel <joro@8bytes.org>," <iommu@lists.linux-foundation.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Stephen Boyd <sboyd@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 14, 2018 at 11:09 PM, Tomasz Figa <tfiga@chromium.org> wrote:
> On Thu, Feb 15, 2018 at 1:12 AM, Rob Clark <robdclark@gmail.com> wrote:
>> On Wed, Feb 14, 2018 at 10:48 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>>> On Wed, Feb 14, 2018 at 12:31:29PM +0900, Tomasz Figa wrote:
>>>>
>>>> - When submitting commands to the GPU, the GPU driver will
>>>> pm_runtime_get_sync() on the GPU device, which will automatically do
>>>> the same on all the linked suppliers, which would also include the
>>>> SMMU itself. The role of device links here is exactly that the GPU
>>>> driver doesn't have to care which other devices need to be brought up.
>>>
>>> This is true.  Assuming that the device link works correctly we would not need
>>> to explicitly power the SMMU which makes my point entirely moot.
>>
>> Just to point out what motivated this patchset, the biggest problem is
>> iommu_unmap() because that can happen when GPU is not powered on (or
>> in the v4l2 case, because some other device dropped it's reference to
>> the dma-buf allowing it to be free'd).  Currently we pm get/put the
>> GPU device around unmap, but it is kinda silly to boot up the GPU just
>> to unmap a buffer.
>
> Note that in V4L2 both mapping and unmapping can happen completely
> without involving the driver. So AFAICT the approach being implemented
> by this patchset will not work, because there will be no one to power
> up the IOMMU before the operation. Moreover, there are platforms for
> which there is no reason to power up the IOMMU just for map/unmap,
> because the hardware state is lost anyway and the only real work
> needed is updating the page tables in memory. (I feel like this is
> actually true for most of the platforms in the wild, but this is based
> purely on the not so small number of platforms I worked with, haven't
> bothered looking for more general evidence.)
>

At least as far as drm/msm/adreno, I'm not terribly concerned about
other platforms that don't need to power up iommu.  It's not really
the same situation as a IP block that shows up in all different
vendor's SoCs.

But if you can convince Robin to go for get/put_sync() calls inside
the iommu driver, I'm fine with that approach too.  That is what I do
in qcom_iommu already.  But if not I'd like to at least solve this for
some platforms if we can't solve for all.

BR,
-R

>>
>> (Semi-related, I would also like to batch map/unmap's, I just haven't
>> gotten around to implementing it yet.. but that would be another case
>> where a single get_supplier()/put_supplier() outside of the iommu
>> would make sense instead of pm_get/put() inside the iommu driver's
>> ->unmap().)
>>
>> If you really dislike the get/put_supplier() approach, then perhaps we
>> need iommu_pm_get()/iommu_pm_put() operations that the iommu user
>> could use to accomplish the same thing?
>
> I'm afraid this wouldn't work for V4L2 either. And I still haven't
> been given any evidence that the approach I'm suggesting, which relies
> only on existing pieces of infrastructure and which worked for both
> Exynos and Rockchip, including V4L2, wouldn't work for SMMU and/or QC
> SoCs.
>
> Best regards,
> Tomasz