Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
