Message-ID: <20180213164205.GA7599@jcrouse-lnx.qualcomm.com>
Date:   Tue, 13 Feb 2018 09:42:05 -0700
From:   Jordan Crouse <jcrouse@...eaurora.org>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Vivek Gautam <vivek.gautam@...eaurora.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, linux-pm@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        Will Deacon <will.deacon@....com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>, " <iommu@...ts.linux-foundation.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        freedreno@...ts.freedesktop.org,
        Robin Murphy <robin.murphy@....com>, sboyd@...eaurora.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with
 runtime suppliers

On Tue, Feb 13, 2018 at 06:10:38PM +0900, Tomasz Figa wrote:
> Hi Vivek,
> 
> Thanks for the patch. Please see my comments inline.
> 
> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
> <vivek.gautam@...eaurora.org> wrote:
> > While handling the concerned iommu, there should not be a
> > need to power control the drm devices from iommu interface.
> > If these drm devices need to be powered around this time,
> > the respective drivers should take care of this.
> >
> > Replace the pm_runtime_get/put_sync(<drm_device>) with
> > pm_runtime_get/put_suppliers(<drm_device>) calls, to power-up
> > the connected iommu through the device link interface.
> > In case the device link is not setup these get/put_suppliers()
> > calls will be a no-op, and the iommu driver should take care of
> > powering on its devices accordingly.
> >
> > Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.org>
> > ---
> >  drivers/gpu/drm/msm/msm_iommu.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> > index b23d33622f37..1ab629bbee69 100644
> > --- a/drivers/gpu/drm/msm/msm_iommu.c
> > +++ b/drivers/gpu/drm/msm/msm_iommu.c
> > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names,
> >         struct msm_iommu *iommu = to_msm_iommu(mmu);
> >         int ret;
> >
> > -       pm_runtime_get_sync(mmu->dev);
> > +       pm_runtime_get_suppliers(mmu->dev);
> >         ret = iommu_attach_device(iommu->domain, mmu->dev);
> > -       pm_runtime_put_sync(mmu->dev);
> > +       pm_runtime_put_suppliers(mmu->dev);
> 
> For me, it looks like a wrong place to handle runtime PM of IOMMU
> here. iommu_attach_device() calls into IOMMU driver's attach_device()
> callback and that's where necessary runtime PM gets should happen, if
> any. In other words, driver A (MSM DRM driver) shouldn't be dealing
> with power state of device controlled by driver B (ARM SMMU).
 
This whole thing is confused by the fact that on MSM the GPU and the GPU IOMMU
share some of the same clocks and power rail so turning on the GPU also
turned on the IOMMU register banks by extension.

But if we put that aside the question is who should be responsible for
controlling the power in this relationship and there are several good reasons to
leave it up to the client device. The most important reason is when we move to
the per-instance model where the GPU self-programmings the SMMU registers. In
that case, the driver will need to make sure that the SMMU is powered up before
submitting the command and then removing the power vote when the commands
are done to save energy.

Additionally, there might be legitimate reasons in the driver to batch
operations - you may wish to attach the device and then map several global
buffers immediately - having driver side control prevents several unneeded power
transitions.

Perhaps the right answer is to do both - allow for the driver to enable the
supplier but also do the right power operations at the appropriately places in
the IOMMU driver.

> This is also important for the reasons I stated in my comments to
> "[PATCH v7 1/6] base: power: runtime: Export
> pm_runtime_get/put_suppliers". Quoting for everyone's convenience:
> 
> >> There are however cases in which the consumer wants to power-on
> >> the supplier, but not itself.
> >> E.g., A Graphics or multimedia driver wants to power-on the SMMU
> >> to unmap a buffer and finish the TLB operations without powering
> >> on itself.
> >
> >This sounds strange to me. If the SMMU is powered down, wouldn't the
> >TLB lose its contents as well (and so no flushing needed)?
> >

> >Other than that, what kind of hardware operations would be needed
> >besides just updating the page tables from the CPU?
> >

> In other words, the SMMU driver can deal with hardware state based on
> return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use()
> and decide whether some operations are necessary or not, e.g.
> - a state restore is necessary if the domain was powered off, but we
> are bringing the master on,
> - a flush may not be required when (un)mapping with the domain powered off,
> - etc.

I agree that there is probably some advanced logic that we can do to
conclusively figure out the state of the hardware and improve the behavior.
I would love to see the SMMU driver get smarter but for the moment we can't
trust it and so we need to force the behavior from the GPU driver. The current
code works for a5x and earlier but on sdm845 we can (no longer) treat the GPU
and the SMMU as the same device for power purposes so we need this code. If at
some point in the future we can start to selectively remove the supplier calls
I wouldn't mind one bit.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists