[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAAFQd5AjopiX6fDgD+mO+-+d0yj-swEnVCNvccWRBSMO+XVJkA@mail.gmail.com>
Date: Tue, 13 Feb 2018 18:10:38 +0900
From: Tomasz Figa <tfiga@...omium.org>
To: Vivek Gautam <vivek.gautam@...eaurora.org>
Cc: "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
Roedel <joro@...tes.org>," <joro@...tes.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Robin Murphy <robin.murphy@....com>,
Will Deacon <will.deacon@....com>,
Rob Clark <robdclark@...il.com>,
"list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
Roedel <joro@...tes.org>," <iommu@...ts.linux-foundation.org>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
dri-devel <dri-devel@...ts.freedesktop.org>,
freedreno@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
Greg KH <gregkh@...uxfoundation.org>, sboyd@...eaurora.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
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 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.
Best regards,
Tomasz
Powered by blists - more mailing lists