On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> wrote: > Hi Tomasz, > > On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: >> On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam >> <vivek.gautam@xxxxxxxxxxxxxx> wrote: >>> Hi Tomasz, >>> >>> On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: >>>> On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark <robdclark@xxxxxxxxx> wrote: >>>>> On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: >>>>>> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark <robdclark@xxxxxxxxx> wrote: >>>>>>> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> 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@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). >>>>>>> >>>>>>> Note that we end up having to do the same, because of iommu_unmap() >>>>>>> while DRM driver is powered off.. it might be cleaner if it was all >>>>>>> self contained in the iommu driver, but that would make it so other >>>>>>> drivers couldn't call iommu_unmap() from an irq handler, which is >>>>>>> apparently something that some of them want to do.. >>>>>> >>>>>> I'd assume that runtime PM status is already guaranteed to be active >>>>>> when the IRQ handler is running, by some other means (e.g. >>>>>> pm_runtime_get_sync() called earlier, when queuing some work to the >>>>>> hardware). Otherwise, I'm not sure how a powered down device could >>>>>> trigger an IRQ. >>>>>> >>>>>> So, if the master device power is already on, suppliers should be >>>>>> powered on as well, thanks to device links. >>>>>> >>>>> >>>>> umm, that is kindof the inverse of the problem.. the problem is >>>>> things like gpu driver (and v4l2 drivers that import dma-buf's, >>>>> afaict).. they will potentially call iommu->unmap() when device is not >>>>> active (due to userspace or things beyond the control of the driver).. >>>>> so *they* would want iommu to do pm get/put calls. >>>> >>>> Which is fine and which is actually already done by one of the patches >>>> in this series, not for map/unmap, but probe, add_device, >>>> remove_device. Having parts of the API doing it inside the callback >>>> and other parts outside sounds at least inconsistent. >>>> >>>>> But other drivers >>>>> trying to unmap from irq ctx would not. Which is the contradictory >>>>> requirement that lead to the idea of iommu user powering up iommu for >>>>> unmap. >>>> >>>> Sorry, maybe I wasn't clear. My last message was supposed to show that >>>> it's not contradictory at all, because "other drivers trying to unmap >>>> from irq ctx" would already have called pm_runtime_get_*() earlier >>>> from a non-irq ctx, which would have also done the same on all the >>>> linked suppliers, including the IOMMU. The ultimate result would be >>>> that the map/unmap() of the IOMMU driver calling pm_runtime_get_sync() >>>> would do nothing besides incrementing the reference count. >>> >>> The entire point was to avoid the slowpath that pm_runtime_get/put_sync() >>> would add in map/unmap. It would not be correct to add a slowpath in irq_ctx >>> for taking care of non-irq_ctx and for the situations where master is already >>> powered-off. >> >> Correct me if I'm wrong, but I believe that with what I'm proposing >> there wouldn't be any slow path. > > Yea, but only when the power domain is irq-safe? And not all platforms > enable irq-safe power domains. For instance, msm doesn't enable its > gdsc power domains as irq-safe. > Is it something i am missing? irq-safe would matter if there would exist a case when the call is done from IRQ context and the power is off. As I explained in a), it shouldn't happen. Best regards, Tomasz > >> >> a) For IRQ context, the master is already powered on and so the SMMU >> is also powered on, through respective device link. >> pm_runtime_get_sync() would ultimately just increment the runtime PM >> usage count. >> >> b) For a case when the master is already powered off (which wouldn't >> be IRQ context, for the reason stated in a)), powering on the SMMU is >> unavoidable, if the SMMU hardware really needs to be accessed (i.e. >> some TLBs need to be invalidated, if their state is preserved despite >> master being powered down). >> >>> >>>> >>>>> >>>>> There has already been some discussion about this on various earlier >>>>> permutations of this patchset. I think we have exhausted all other >>>>> options. >>>> >>>> I guess I should have read those. Let me do that now. >>> Yea, i point to the thread in cover letter and [PATCH 1/6]. >>> Thanks. >> >> I read through all the links in the cover letter and I could see other >> attempts not working out indeed, but they were different from what I'm >> proposing. >> >> There was also a point raised that __pm_runtime_resume() called from >> pm_runtime_get_sync() would grab dev->power_lock spinlock, which is >> true, except that if the device is already active, it would do it only >> for the time of checking device state, so I doubt it would really be a >> significant point of contention. >> >> Best regards, >> Tomasz >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
